Allow sending email to BCC address when not notifying user.

[Hailong]

Description (*)

Order Comments Email Copy would NOT be sent to the specified email address if the send method is set to Bcc, and Notify Customer by Email is not checked when adding a comments from backbend.

The expected behaviour is that whether the Notify Customer by Email is checked, an email copy is sent to the specified email. While now if the Send Order Comments Email Copy Method config is set to Separate Email it goes well. But if the config is set to Bcc, the email can not be sent.

The problem is that the SenderBuilder is used by both order notification and comments notification, so the fix is to add a flag to fix the problem for comments notification.

Fixed Issues (if relevant)

Manual testing scenarios (*)

1. Navigate to Configuration->SALES->Sales Emails->Order Comments
2. Input email address in Send Order Comment Email Copy To
3. Select Bcc for Send Order Comments Email Copy Method.
4. Go to any order, add a new comment to the order and leave the Notify Customer by Email unchecked.
5. Verify the email address set in Step 2 can receive email after submitting the comment.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[m2-assistant]

Hi @Hailong. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[dmytro-ch]

Unfortunately, we are not allowed to change public methods signature according to the Backward Compatible Development Guide.

[Hailong]

@dmytro-ch thanks for your review. But I assume if the class is not annotated by @api, we can update the signature. This class is kind of an utility class used by other two classes in the same folder, which are annotated by @api.

[Hailong]

@dmytro-ch I do think it's not the best idea to add this flag here too, but this the minimal change to fix this issue. But let me try with a better approach anyway.

[Hailong]

I just moved the check of CopyMethod to the caller class, so that to avoid introducing the signature. It looks more clear this way.

[Hailong]

Updated the code to avoid modifying the method signature.

[dmytro-ch]

Hi @Hailong,
could you please check and fix the automated tests failures?
Once the changes are applied, please squash all commits into a single one in order to cleanup history.
Thank you!

[Hailong]

@dmytro-ch I just fixed the failures in UT. I also introduced two const in the Magento\Sales\Model\Order\Email\Sender class, but I'm not quite sure it's allowed in this class. If not, I will need to find some other place or replace them with plain strings.

[dmytro-ch]

Hi @Hailong, sorry for late reply and thanks for your patience!

Unfortunately, it's not allowed to define new public constants. Please check the Semantic Version Checker build for reference.

Could you please also update your working branch with the latest changes from mainline?

Thank you!

[Hailong]

Thank you @dmytro-ch , I have just removed the newly added constants. Let's see how the checking goes this time.

[Hailong]

@dmytro-ch The build checks look good this time, can you please check?

[engcom-Alfa]

Hi @Hailong ,
Could you, please, solve the problem with conflicting files?
Thanks!

[Hailong]

Hi @engcom-Alfa , the conflict has been resolved. Thanks.

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-5650 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[engcom-Alfa]

✔️ QA Passed

[m2-assistant]

Hi @Hailong, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.