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][FIX] sale workflow job email send before validation #2501

Conversation

TDu
Copy link
Member

@TDu TDu commented May 5, 2023

When using the sale_automatic_workflow_job module the validation of
the order is done by a job. But the sending of the confirmation is done
in line. Causing the email to be sent before the validation of the
order.

@TDu TDu force-pushed the 14-fix-sale-workflow-job-email-send-before-validation branch 4 times, most recently from ce91f5e to 722c843 Compare May 5, 2023 03:38
TDu added 2 commits May 5, 2023 05:47
When using the `sale_automatic_workflow_job` module the validation of
the order is done by a job. But the sending of the confirmation is done
in line. Causing the email to be sent before the validation of the
order.
The function signature has changed in inherited modules
@TDu TDu force-pushed the 14-fix-sale-workflow-job-email-send-before-validation branch from 722c843 to 90aa5b4 Compare May 5, 2023 03:47
@@ -150,6 +151,7 @@ def test_deleted_sale_order(self):
args=(
self.sale,
safe_eval(workflow.order_filter_id.domain) + workflow_domain,
False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
False,
send_order_confirmation=False,

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't ask for this as I feared you would point at me as "nitpicking" 🤣

@@ -45,6 +45,7 @@ def test_validate_sale_order(self):
("state", "=", "draft"),
("workflow_process_id", "=", self.sale.workflow_process_id.id),
],
False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
False,
send_order_confirmation=False,

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried but it does not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TDu what do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

def assert_job_delayed(self, delayable_cls, delayable, method_name, args):

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, args is a tuple/list here, no way to pass kwargs.

@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

simahawk commented Jun 6, 2023

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-2501-by-simahawk-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jun 6, 2023
Signed-off-by simahawk
@OCA-git-bot
Copy link
Contributor

@simahawk your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-2501-by-simahawk-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rousseldenis rousseldenis added this to the 14.0 milestone Jun 21, 2023
@rousseldenis
Copy link
Contributor

/ocabot merge patch

@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-2501-by-rousseldenis-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jun 21, 2023
Signed-off-by rousseldenis
@OCA-git-bot
Copy link
Contributor

@rousseldenis your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-2501-by-rousseldenis-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rousseldenis
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-2501-by-rousseldenis-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 408106c into OCA:14.0 Jun 22, 2023
@OCA-git-bot
Copy link
Contributor

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

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.

6 participants