-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Check if copy method is copy before sending comment email #29911
Check if copy method is copy before sending comment email #29911
Conversation
Hi @JeroenVanLeusden. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento create issue |
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.
@JeroenVanLeusden could you please cover changes with automated test?
Hi @JeroenVanLeusden, please cover your changes by automated test. Otherwise, we can't proceed with your PR. |
@sidolov Will do. However shouldn't we remove the entire ifelse statement and only send a copy/bcc if the customer itself actually receives an email? |
@JeroenVanLeusden do you mean to send an email when |
@sidolov Correct |
If we are going to be consistent with the comment
and I believe it's a right way to preserve backward compatible behavior, we should remove the
|
Fine by me. Will update this when i find some spare time. |
Hi @JeroenVanLeusden, will you continue with this PR? |
It's still on my todo list, hopefully will have some time end of this week. |
1ff5376
to
78dd01a
Compare
@magento run all tests |
@magento run all tests |
Hi @gabrieldagama, thank you for the review.
|
✔️ QA Passed Case 1Manual testing scenario:
Before: ✖️ a copy of the email was sent (but shouldn't have been sent because the customer notification was turned off and the copy was set to a hidden copy.) After: ✔️ email copy wasn't sent Case 2Manual testing scenario:
After: ✔️ email copy was sent |
Hi @JeroenVanLeusden, thank you for your contribution! |
Description (*)
As seen in the comments email copies should only be sent as separated emails if their copy method is copy.
Related Pull Requests
None I could find.
Fixed Issues (if relevant)
None I could find.
Manual testing scenarios (*)
Enable
sales_email/order_comment/enabled
Add email address to
sales_email/order_comment/copy_to
Set
sales_email/order_comment/copy_method
to bcc (default value).Place order.
Add order comment in order overview (adminhtml area) and uncheck notify customer.
Email copy should not be send because notify customer is disabled and copy set to bcc.
Questions or comments
Contribution checklist (*)
Resolved issues: