Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[14.0] edi: generate/send chain jobs + add identity exact match based on checksum #796

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Jul 5, 2023

Depends on:

See atomic commits for details.

Would be nice to use job chain for input too. Not a blocker tho.

@OCA-git-bot
Copy link
Contributor

Hi @etobella,
some modules you are maintaining are being modified, check this out!

@simahawk simahawk force-pushed the 14-edi-fmwk-imp-jobs branch 2 times, most recently from 2a56439 to 4437f44 Compare July 6, 2023 13:52
@simahawk
Copy link
Contributor Author

Indenty key does not work fine w/ job deps. We'll need this OCA/queue#546

The recommended way to execute actions on records
is to call `action_exchange_*` on
@simahawk simahawk changed the title [14.0] edi: generate/send chain jobs [14.0] edi: generate/send chain jobs + add indentity exact match Nov 27, 2023
@simahawk simahawk changed the title [14.0] edi: generate/send chain jobs + add indentity exact match [14.0] edi: generate/send chain jobs + add identity exact match Nov 27, 2023
@simahawk simahawk changed the title [14.0] edi: generate/send chain jobs + add identity exact match [14.0] edi: generate/send chain jobs + add identity exact match based on checksum Nov 27, 2023
Instead of waiting for the cron to pass again and send the file
chain the 2 jobs so that it gets sent right after generation.
* catch OSError
* add debug log
Prevents having duplicated jobs for the same record as far as possible.
@simahawk simahawk marked this pull request as ready for review November 27, 2023 15:06
Try to send out the file as fast as possible once the content is ready.
@simahawk
Copy link
Contributor Author

@etobella @HviorForgeFlow if you could pass by... 🙏

hasher = identity_exact_hasher(job_)
# Include files checksum
hasher.update(
str(sorted(job_.recordset.mapped("exchange_filechecksum"))).encode("utf-8")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do yo need to add the file checksum? The function already contains ID of the exchange record and function 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because is the only to way to know if the file is the same.
If by error you try to schedule another job you won't get duplicated jobs unless the content of the file is the same.
In most cases, if you rely only on the cron to process files you'll have the filter done by the check methods that will skip records not in the rate state or not w/ the right values to be generated/sent/received/processed.
However, if you force those checks and try to schedule a job for the a record whereas the file didn't change you shouldn't be able to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might come handy also to compare files w/o reading the whole file (eg: to check if you received the same file twice).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if the file is not generated?

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't want to set the approval, I just wanted to mark it 😢

Copy link
Member

@HviorForgeFlow HviorForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review 👍

ping @MiquelRForgeFlow

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@simahawk
Copy link
Contributor Author

@etobella good for you or you seen any possible regression on your side?

@etobella
Copy link
Member

I don't see any major regressions from my side. 🤔

I tested with other OCA reports and everything works fine

@simahawk
Copy link
Contributor Author

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-796-by-simahawk-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 075237a into OCA:14.0 Nov 30, 2023
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 0a8ae3f. Thanks a lot for contributing to OCA. ❤️

@etobella
Copy link
Member

etobella commented Dec 12, 2023

@simahawk I migrated edi_oca yesterday on a Production database with this change and it took a long time due to this change 😭 . I have +100.000 exchange records (At the end, I had to make a Manual migration)

Just a question, could we change it to use sha1 and not md5 in order to make a migration script? (there is a checksum for sha1 for attachments, so we can recover data from there?) At the end, I solved the issue for me, but it might be a problem for other users.

WDYT?

petrus-v added a commit to petrus-v/edi that referenced this pull request Dec 19, 2023
As long OSError are part of retryable exceptions and HTTPError
inherits from OSSError we are catching the new exception

* PR introducing that change: OCA#796
* HTTPError implementation: https://github.com/python/cpython/blob/3.12/Lib/urllib/error.py
barkat-matthias pushed a commit to barkat-matthias/edi that referenced this pull request Jun 9, 2024
As long OSError are part of retryable exceptions and HTTPError
inherits from OSSError we are catching the new exception

* PR introducing that change: OCA#796
* HTTPError implementation: https://github.com/python/cpython/blob/3.12/Lib/urllib/error.py
barkat-matthias pushed a commit to foodles-tech/edi that referenced this pull request Jun 10, 2024
As long OSError are part of retryable exceptions and HTTPError
inherits from OSSError we are catching the new exception

* PR introducing that change: OCA#796
* HTTPError implementation: https://github.com/python/cpython/blob/3.12/Lib/urllib/error.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants