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

Fixed issue with sending from do-not-reply@domain address #21455

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

jaapjansma
Copy link
Contributor

Fixed issue with sending from do-not-reply@domain address resulting in fatal errors because an invalid from address was used.

Overview

When confirming a group description another e-mail is send with a from e-mail address do-not-reply@domain.com when the mail account is not allowed to send on behalf of this e-mail address a fatal error is shown.
This PR sets the from address to the default domain e-mail address and leaves the reply to to do-not-reply@domain.com so when a user press reply on this e-mail it goes to the do-not-reply address.

Before

When it is not possible to send from do-not-reply@domain.com it might result in an error like below:

ccve

After

The from e-mail address used is the default domain from e-mail address. The reply-to is still do-not-reply so the fatal error is gone.

Comments

See this issue: https://lab.civicrm.org/dev/core/-/issues/645

@civibot
Copy link

civibot bot commented Sep 13, 2021

(Standard links)

@civibot civibot bot added the master label Sep 13, 2021
@eileenmcnaughton
Copy link
Contributor

Makes sense to me - @MegaphoneJon @mlutfy ?

@MegaphoneJon
Copy link
Contributor

This makes sense to me but I'm having trouble thinking through whether this could cause a problem on upgrade for existing sites. I can't think of where the default email address is used normally, but it seems possible that it's not a valid address for sending. I'll also ping @JoeMurray as the mind behind the Outbound Domain Enforcement extension.

I did do a quick grep and it looks like we've caught all the places where this might be used as a "From" address so that's one concern laid to rest.

@seamuslee001
Copy link
Contributor

I can understand this but it feels kind of odd in a way that you could only send from one specific email address for a domain, also just going to ping @agh1 as well here in case Andie has any thoughts on this as well

@mlutfy
Copy link
Member

mlutfy commented Sep 14, 2021

I suspect this is very rarely used, and would +1 the PR.

As a feature it makes sense, but it would probably be better to have a "From Email" that is configurable as a "no-reply address". Or a setting specifically for those kind of notifications. For people upgrading, the "breakage" will be that suddenly those notifications have a valid "from", so hitting reply might end up in their main mailbox. It's not too dramatic.

I would recommend tagging CRM_Core_BAO_Domain::getNoReplyEmailAddress() as @deprecated

@mlutfy mlutfy added the merge ready PR will be merged after a few days if there are no objections label Sep 18, 2021
@demeritcowboy
Copy link
Contributor

Has been merge-ready for a while. Changing to merge-on-pass and re-running tests since 11 days in this 5.43 cycle is enormous.

Jenkins retest this please.

@demeritcowboy demeritcowboy added merge on pass and removed merge ready PR will be merged after a few days if there are no objections labels Sep 29, 2021
@eileenmcnaughton
Copy link
Contributor

nice - note I did put this PR in the dev digest too

@jaapjansma
Copy link
Contributor Author

Looks like the failure is unrelated because the database seems to be gone.

Jenkins retest this please.

@mlutfy
Copy link
Member

mlutfy commented Sep 29, 2021

Seems like a legitimate test problem, but unrelated to this PR?

Test: CRM_Member_Form_Task_PDFLetterTest.testMembershipTokenReplacementInPDF - Failing since 6 builds

CRM_Member_Form_Task_PDFLetterTest::testMembershipTokenReplacementInPDF
Non-conformant hook_civicrm_alterMailParams(..., messageTemplate): Unrecognized keys: schema

@agh1
Copy link
Contributor

agh1 commented Sep 29, 2021

I'm so out of the loop that I forgot this was a setting to begin with! I agree with @MegaphoneJon, @mlutfy, and @seamuslee001 that it seems like the domain address isn't always going to be the best choice, but I understand that none of this is ideal.

@eileenmcnaughton
Copy link
Contributor

test this please

@demeritcowboy demeritcowboy merged commit 64b64da into civicrm:master Sep 29, 2021
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.

7 participants