Fix email sender

[versdivers]

#16355

Preconditions

1. Magento 2.2.4
2. PhP 7.1
3. Apache

Steps to reproduce

1. Set general store email addresses
2. Set store specific email senders in Sales Emails
3. Send order/shipment/creditnota/invoice mail to customer

Expected result

Email is sent from store email address

Actual result

It sends from the mail server default mailadress

Why

In vendor \magento\module-sales\Model\Order\Email\SenderBuilder.php the configureEmailTemplate() function as been changed.

[magento-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @versdivers. 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 {$VERSION} instance - deploy vanilla Magento instance

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

[phoenix128]

Hello @versdivers ,
would you please sign the CLA in one of the previous comments?

[hostep]

Just FYI: but I think this PR fixes the same problem as another PR: #15119

The issue which #15119 tries to fix is #14952

@versdivers: what do you think, are these the same? And if yes: which fix would be the most correct one?

Update: there is also someone who claims that the Amazon Pay module (which was added to Magento 2.2.4) is causing this problem: #16231 (comment)

[phoenix128]

Hello @hostep ,
they are definitely the same and they applied the fix almost in the same way.

What I think:

• I like this PR because is not adding a new method setFromByStore like the other PR, but simply adding a new optional parameter to the existing setFrom. And I think we should avoid adding public methods when not necessary.
• The other PR has a test coverage that this PR is missing.

I think we should coordinate a kind of merge.

[versdivers]

@phoenix128 this is my first contribution. Pretty new to github. Not how i can do that

[versdivers]

@hostep I can confirm that this will infact also solve that problem. You can change the di.xml but they maybe set that there for a reason. Instead of just maybe breaking something somewhere else my fix is a soft fix that will keep compatibility. Not for the Fooman module because they change the setFrom function but you can just implement the same changes there.

[DanielRuf]

@phoenix128 this is my first contribution. Pretty new to github. Not how i can do that

Click on the button and fill in the details in the form and submit it.

[versdivers]

@DanielRuf Haha sorry. I signed it. Thanks!

[phoenix128]

Hello @versdivers ,
seems like you have some static tests failing.

FILE: ...agento/magento2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 113 | ERROR | Line exceeds maximum limit of 120 characters; contains 134
     |       | characters
--------------------------------------------------------------------------------

You also have 2 unit tests failing:

There were 2 errors:
1) Magento\Sales\Test\Unit\Model\Order\Email\SenderBuilderTest::testSend
Error: Call to a member function resolve() on null
/home/travis/build/magento/magento2/lib/internal/Magento/Framework/Mail/Template/TransportBuilder.php:169
/home/travis/build/magento/magento2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php:113
/home/travis/build/magento/magento2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php:63
/home/travis/build/magento/magento2/app/code/Magento/Sales/Test/Unit/Model/Order/Email/SenderBuilderTest.php:162
2) Magento\Sales\Test\Unit\Model\Order\Email\SenderBuilderTest::testSendCopyTo
Error: Call to a member function resolve() on null
/home/travis/build/magento/magento2/lib/internal/Magento/Framework/Mail/Template/TransportBuilder.php:169
/home/travis/build/magento/magento2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php:113
/home/travis/build/magento/magento2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php:93
/home/travis/build/magento/magento2/app/code/Magento/Sales/Test/Unit/Model/Order/Email/SenderBuilderTest.php:190

Can you please check them and fix it?
P.S: Can you also provice a unit test coverage for your new implementaion?

[versdivers]

@phoenix128
The first error is a matter of setting a breakline. So sure. Changed it.
Not that PHP has a hard break after X characters. Even though i know it is a OOP requirement so i agree that it has to be changed.

The second error is on the test Unit. I am not sure how those are setup and currently do not have the time to investigate test PHP files within Magento since they are buggy as they are and i have a big project to finish up.

I am just correcting a big bug within Magento 2.2.2 - 2.2.4 and giving the solution to the community so people can fix it themself or Magento can release a patch / update based on it.

I can confirm that this will work perfectly in a development environment or a production environment since this has been already implemented in multiple live websites that we control.

When the big project is done i will come back and setup a test enviroment for this change. For now i am afraid that this is all the information that i can provide. I do see why the testings on this has to be done and i am glad something like this is in place even for a small change like this.

If anyone has time they are free to implement the 2 changes themself and run the needed tests.

My apologies.

[phoenix128]

Hello @versdivers ,
not a problem, we will take care of it.

Thank you for your work and for your time.

[gwharton]

Only just seen this PR. Sorry I created an alternative PR with the same change, but with full test coverage.

#16461

[versdivers]

@gwharton :D Awesome! good job

[gwharton]

I am going to close this PR. There have been several PRs produced that are all trying to fix the same issue. I have collated everyones opinion and generated a new PR with full test coverage, Travis pass and Codacy pass. If you would like to collaborate further on this issue, you are encouraged to do so through PR #16461