Fixed "Email Copy of Invoice", "Email Copy of Shipment" and "Email Copy of Creditmemo" Issues

[gwharton]

Description (*)

Magento is not consistent with the operation of the "Email Copy of ......" Checkboxes on the Invoice, Creditmemo and Shipment creation screens in admin.

On the invoice screen, if the box is unticked, but the global setting in Sales Emails is set to enabled, the email still gets sent.

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes New Invoice: Checkbox "Email Copy of Invoice" is useless #28511

Manual testing scenarios (*)

• Create test Product
• Purchase Test Product
• Set Config -> Sales -> Sales Emails to Enabled for Invoice, Shipment and Creditmemo
• Create Invoice and do NOT tick "Email copy of Invoice". Verify Email is NOT received
• Create Shipment and do NOT tick "Email copy of Shipment". Verify Email is NOT received
• Create Creditmemo and do NOT tick "Email copy of Creditmemo". Verify Email is NOT received
• Purchase Test Product
• Create Invoice and tick "Email copy of Invoice". Verify Email is received
• Create Shipment and tick "Email copy of Shipment". Verify Email is received
• Create Creditmemo and tick "Email copy of Creditmemo". Verify Email is received
• Purchase Test Product
• Set Config -> Sales -> Sales Emails to Disabled for Invoice, Shipment and Creditmemo
• Create Invoice. Verify Email is NOT received
• Create Shipment. Verify Email is NOT received
• Create Creditmemo. Verify Email is NOT received

Questions or comments

Unit test covered

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 are green)

[m2-assistant]

Hi @gwharton. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

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.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 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

[ihor-sviziev]

Hi @gwharton,
Looks like we need to change the default value of the checkbox on the "send emdail... " to be according to the value in the admin. What do you thing?

[gwharton]

The checkbox is only present if the global setting is enabled (which actually makes the additional check to see if it is enabled before sending email a little redundant), however......

So the desired operation is?

If the global setting is enabled, then the checkbox is visible, and checked by default. The user can uncheck if he doesn't want to send the email? If the global setting is disabled, the checkbox is not present and the email is not sent.

That sounds fair to me.

@ihor-sviziev I'll make that change, and update the failing integration test. Any idea on how to get around the Health Check test failure. Seems to be borking at the use of Registry in the unit tests?

[ihor-sviziev]

@gwharton,
I believe the decision what is correct behavior isn't really simple.

Technically "can send invoice email" or "can send credit memo email" should be kind of global. Seems like this global option was designed for automated email sending while such entity was created, but the checkbox in admin - for manual.
So in theory earlier we were able to not send such emails automatically, but still were able to send an email if we would like to notify our customer.

I though doing smart thing - if the option is enabled - let's check the checkbox, otherwise - do not check it by default, but seems like I was wrong.

Unfortunately I don't have enough time now in order to collect all the possible use cases in this area, but I believe all of them need to be checked to make sure we're not breaking any existing flow.

Would be good if someone else could provide their feedbacks.

[gwharton]

No problem. Happy to standby. Its pretty easy to implement the logic. Its coming up with the right logic thats the difficult bit. Will await further instruction.

If you are going to be able to send emails manually, with the global setting turned off, then the Template, Copy to etc global sales email settings need to be decoupled from the global enabled setting, as when the global setting is disabled, all of the email options disappear.

[aleron75]

@magento give me 2.4-develop instance

[magento-deployment-service]

Hi @aleron75. Thank you for your request. I'm working on Magento instance for you.

[magento-deployment-service]

Hi @aleron75, here is your Magento Instance: https://aec935f82d9674684453effe117752c8-2-4-develop.instances.magento-community.engineering
Admin access: https://aec935f82d9674684453effe117752c8-2-4-develop.instances.magento-community.engineering/admin_70a8
Login: 52a1623f
Password: e380c018e971

[gwharton]

I'm just going to drop a brain dump for you all to promote conversation on the item.

Currently the system works as follows.

- Global Setting for Invoice Emails is disabled
-- Create Invoice
--- Checkbox to send email is not visible. 
---- Save Invoice. No email is sent on invoice creation.

- Global Setting for Invoice Emails is enabled
-- Create Invoice
--- Checkbox to send email is unchecked (default).
---- Save Invoice. Email is sent despite checkbox being unchecked <--- NOT RIGHT.
-- Create Invoice
--- Checkbox to send email is checked.
---- Save Invoice. Email is sent.

- Global Setting for CreditMemo Emails is disabled
-- Create CreditMemo 
--- Checkbox to send email is not visible. 
---- Save CreditMemo. No email is sent on CreditMemo creation.

- Global Setting for CreditMemo Emails is enabled
-- Create CreditMemo 
--- Checkbox to send email is unchecked (default).
---- Save CreditMemo. Email is not sent.
-- Create CreditMemo 
--- Checkbox to send email is checked.
---- Save CreditMemo. Email is sent.

- Global Setting for Shipment Emails is disabled
-- Create Shipment 
--- Checkbox to send email is not visible. 
---- Save Shipment. No email is sent on Shipment creation.

- Global Setting for Shipment Emails is enabled
-- Create Shipment 
--- Checkbox to send email is unchecked (default).
---- Save Shipment. Email is not sent.
-- Create Shipment 
--- Checkbox to send email is checked.
---- Save Shipment. Email is sent.

The PR proposes two changes.

• The default for the checkbox on the create invoice/creditmemo/shipment page would change to be checked by default. You can still untick it to not send the email. If the global setting is disabled, the checkbox doesn't even appear.
• The inconsistency with the invoice page would be corrected, bringing it inline with how the shipment and creditmemo pages work. Currently the invoice page shows the checkbox, but actually ignores the contents of the checkbox so always sends the invoice (if the global setting is enabled). This PR would remove that ability. Was this put in on purpose?

So the question that now arrises is ....

"Do we want to be able to send emails manually from the invoice/creditmemo/shipment creation pages when the global setting for emails (for the specified type) is disabled?
"Should the global setting effectively disable the sending of those email types globally (currently doesn't)"
"Do we want to decouple the decision of whether to send the email when creating the invoice/creditmemo/shipment (i.e the checkbox on the creation page) from the global setting?"

i.e

• Global setting is disabled, checkbox visible on create page, and if ticked, the email is sent (currently can't do this).
• Global setting is enabled, checkbox visible on create page, and if unticked, the email is not sent (currently can't do this on invoice)

I like the idea of decoupling the global enabled setting from the creation page checkbox, so when you are creating the invoice/creditmemo/shipment it just does what the checkbox says. If this is the case, then further work needs doing on the config page

• Renaming the setting as it is no longer a global enable/disable flag.
• The other settings like template, and copy to, need to be decoupled from the enabled setting as these would still need to be set, even though the global setting is disabled.
• Other areas may need looking at where the global setting is used.
• Does the "Send Email" button at the top of the sales order view pages work properly. Does that always override the global setting?

[ihor-sviziev]

Please revert changes in setting checked state for checkboxes

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8472 has been created to process this Pull Request

[engcom-Alfa]

✔️ QA Passed

Main Case

Manual testing scenario:

1. Go to Admin -> Stores -> Configuration -> Sales -> Sales Emails -> Invoice and
   set Enable to Yes;
2. Create Order;
3. Go to Admin -> Sales -> Orders and open created before order for edit;
4. Click on the Invoice button;
5. Uncheck "Email Copy of Invoice";
6. Generate Invoice;

Before: ✖️ Email sent to Customer

After: ✔️ Email was not sent to Customer

Additional Cases

• Manual testing scenario:

1. Create test Product
2. Purchase Test Product
3. Set Config -> Sales -> Sales Emails to Enabled for Invoice, Shipment and Creditmemo
   Create Invoice and do NOT tick "Email copy of Invoice".

Actual Result: ✔️ An email was NOT received

4. Create Shipment and do NOT tick "Email copy of Shipment".

Actual Result: ✔️ An email was NOT received

5. Create Creditmemo and do NOT tick "Email copy of Creditmemo".

Actual Result: ✔️ An email was NOT received

• Manual testing scenario:

1. Purchase Test Product
2. Create Invoice and tick "Email copy of Invoice".

Actual Result: ✔️ An email was received

3. Create Shipment and tick "Email copy of Shipment".

Actual Result: ✔️ An email was received

4. Create Creditmemo and tick "Email copy of Creditmemo".

Actual Result: ✔️ An email was received

• Manual testing scenario:

1. Purchase Test Product
2. Set Config -> Sales -> Sales Emails to Disabled for Invoice, Shipment and Creditmemo
3. Create Invoice. Verify Email is NOT received

Actual Result: ✔️ An email was NOT received

4. Create Shipment. Verify Email is NOT received

Actual Result: ✔️ An email was NOT received

5. Create Creditmemo. Verify Email is NOT received

Actual Result: ✔️ An email was NOT received

[engcom-Alfa]

✔️ QA Passed

[m2-assistant]

Hi @gwharton, 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.