Skip to content
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 : wrong sender for sales emails sent from BO #11348

Closed
wants to merge 1 commit into from
Closed

Fix : wrong sender for sales emails sent from BO #11348

wants to merge 1 commit into from

Conversation

benjamin-volle
Copy link

Fix for wrong sender when sending sales emails from BO for non-default stores

Description

The sales emails are always sent from default sender when sent via BO, even if the sender is overriden in configs

Manual testing scenarios

  1. Change sales email sender for non-default store
  2. Send "order confirmation" email from BO

@benjamin-volle benjamin-volle changed the base branch from 2.2-develop to 2.1 October 10, 2017 15:26
@benjamin-volle benjamin-volle changed the title Fix/bo mail sender Fix : wrong sender for sales emails sent from BO Oct 10, 2017
@benjamin-volle
Copy link
Author

Should I change the destination branch to "develop" ?

@avoelkl
Copy link
Contributor

avoelkl commented Oct 11, 2017

Hi @synolia-bvo,
Does the problem exist in 2.2 as well or only in 2.1?

@avoelkl avoelkl self-assigned this Oct 11, 2017
@avoelkl
Copy link
Contributor

avoelkl commented Oct 11, 2017

Regarding your question:

Should I change the destination branch to "develop" ?

Please use either 2.2-developer or 2.1-develop (depending on where the problem exists). Thanks!

@benjamin-volle benjamin-volle changed the base branch from 2.1 to 2.1-develop October 11, 2017 08:40
@benjamin-volle
Copy link
Author

benjamin-volle commented Oct 11, 2017

Hello,

The problem is still present in verison 2.2

I changed destination to 2.1-develop.

If the problem exists in both version, do i need to do this PR on both branches ?

@PieterCappelle
Copy link
Contributor

PieterCappelle commented Oct 11, 2017

Hi. I fixed a lot of this in PR #11307 and #11308.

@avoelkl
Copy link
Contributor

avoelkl commented Oct 11, 2017

I see you also added some scope/store id's @PieterCappelle.
If I saw correctly the only addition in this PR is setting the correct from-address ($result = $this->_senderResolver->resolve($from, $this->scopeId);)
Can you both please check as well, especially @synolia-bvo if the PR from Pieter also matches your goal?

@avoelkl
Copy link
Contributor

avoelkl commented Oct 11, 2017

If the problem exists in both version, do i need to do this PR on both branches ?
Yes, 2.1-developer and 2.2-develop would be great.

Otherwise, only 2.2-develop should be fine as well and it will be backported later.

@benjamin-volle
Copy link
Author

Hi,

There is one modification that is not in @PieterCappelle 's PR in file app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php

@PieterCappelle : can you please add this modification to yours ?

@PieterCappelle
Copy link
Contributor

Done @synolia-bvo.

@benjamin-volle
Copy link
Author

Thank you. So can I close my PR ?

@PieterCappelle
Copy link
Contributor

Yes, but maybe wait until my PR is accepted?

@benjamin-volle
Copy link
Author

Ok, I'll do this. Thanks !

@avoelkl
Copy link
Contributor

avoelkl commented Oct 12, 2017

Awesome, thanks @PieterCappelle @synolia-bvo!

Current status: Waiting to get PR #11307 and #11308 merged.

@benjamin-volle
Copy link
Author

benjamin-volle commented Dec 6, 2017

Hi all,

Is there any news for these issues ? @avoelkl

Thanks,

@avoelkl
Copy link
Contributor

avoelkl commented Dec 18, 2017

Hi @synolia-bvo,
As I can see the related PR's #11307 and #11308 have been closed.
Any news on this @PieterCappelle @okorshenko ?

@benjamin-volle
Copy link
Author

Hi,

What should I do now ..?

@orlangur orlangur self-assigned this Aug 1, 2018
@orlangur
Copy link
Contributor

orlangur commented Aug 1, 2018

Issue was fixed for 2.2-develop in #14800 and can be backported based on it if still relevant.

@orlangur orlangur closed this Aug 1, 2018
@benjamin-volle
Copy link
Author

@orlangur The fix you referenced doesn't fix the same problem. My fix was for "sales emails " (like orders or invoices), not for reset password.

@orlangur
Copy link
Contributor

orlangur commented Aug 2, 2018

@synolia-bvo could you please report it if still relevant for 2.2-develop? All contributions nowadays should be merged to 2.2 release line first.

@benjamin-volle
Copy link
Author

It looks fixed in 2.2 in this commit : f2bfdd9

@orlangur
Copy link
Contributor

orlangur commented Aug 2, 2018

@synolia-bvo thanks for investigation! Then, if you really need it fixed in 2.1-develop, you can create a backport out of this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants