-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fix sending duplicate emails #17484
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
Fix sending duplicate emails #17484
Conversation
|
Hi @iGerchak. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed - it's not good idea to clean collection every time. It's better to fix root of the issue - this object should NOT be shared. Please add shared="false" to https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Sales/etc/di.xml#L355-L395
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- AFAIK current page is set to 1 by default, no need to specify it
- Could you extract
50to system configuration, nearsales_email/general/async_sendingand make it configurable via admin? Please make it configurable globally only (the same assales_email/general/async_sending)
ce7c92b to
0b8bfaf
Compare
|
Reopened this PR just to restart Travis builds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not limiting email sending, you're limiting how many entities (orders/shipments/etc) will be processed per one cron run.
Please Update label and description
0b8bfaf to
a69ced7
Compare
|
Hi @ihor-sviziev, thank you for the review. |
ihor-sviziev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build on travis failed. Could you update tests?
|
Hi @iGerchak. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
Preconditions
Description
We installed Wyomind Cron Scheduler v1.4.0 and in the result, we have duplicates email (5-6 emails for 1 order).
This hotfix warns against similar situations with sending duplicate emails and adds a limit of 50 emails per cronjob.
Steps to reproduce:
Expected result
No duplicate Emails.
Actual result
Duplicate Emails.