Towards CRM-17753 - consolidate code location for do-not-reply@mydomain.com #12270
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
systememail
branch
from
8035b8a to
541a0a5
Compare
monishdeb
reviewed
Jun 7, 2018
| @@ -143,11 +143,11 @@ public static function confirm($contact_id, $subscribe_id, $hash) { | |||
| $mailParams = array( | |||
| 'groupName' => 'Mailing Event ' . $component->component_type, | |||
| 'subject' => $component->subject, | |||
| 'from' => "\"$domainEmailName\" <do-not-reply@$emailDomain>", | |||
| 'from' => "\"$domainEmailName\" <" . CRM_Core_BAO_Domain::getNoReplyEmailAddress() . '>', | |||
There was a problem hiding this comment.
@eileenmcnaughton you need to remove
$emailDomain = CRM_Core_BAO_MailSettings::defaultDomain();
above at line 121
monishdeb
reviewed
Jun 7, 2018
| @@ -189,7 +189,7 @@ public static function send($queue_id, &$mailing, &$bodyTxt, $replyto, &$bodyHTM | |||
| 'To' => $mailing->replyto_email, | |||
| 'From' => $from, | |||
| 'Reply-To' => empty($replyto) ? $eq->email : $replyto, | |||
| 'Return-Path' => "do-not-reply@{$emailDomain}", | |||
| 'Return-Path' => CRM_Core_BAO_Domain::getNoReplyEmailAddress(), | |||
There was a problem hiding this comment.
@eileenmcnaughton you need to remove
$emailDomain = CRM_Core_BAO_MailSettings::defaultDomain();
above at line 176
monishdeb
reviewed
Jun 7, 2018
| @@ -227,7 +227,7 @@ public function send_confirm_request($email) { | |||
| 'From' => "\"{$domainEmailName}\" <{$domainEmailAddress}>", | |||
| 'To' => $email, | |||
| 'Reply-To' => $confirm, | |||
| 'Return-Path' => "do-not-reply@$emailDomain", | |||
| 'Return-Path' => CRM_Core_BAO_Domain::getNoReplyEmailAddress(), | |||
There was a problem hiding this comment.
@eileenmcnaughton you need to remove
$emailDomain = CRM_Core_BAO_MailSettings::defaultDomain();
above at line 203
There was a problem hiding this comment.
I updated the others - but this one is still used
eileenmcnaughton
force-pushed
the
systememail
branch
from
541a0a5 to
3e8ace2
Compare
eileenmcnaughton
force-pushed
the
systememail
branch
from
3e8ace2 to
576fcb9
Compare
|
@monishdeb is this mergeable now? |
This was referenced Jun 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
#10648 and #10647 both seek to prevent the mailing out of emails with from being do-not-reply@mydomain.com which is bad practice. They are stalled on agreement on what the correct alternative is and on the need to consolidate the code to be able to make the change sensibly. This does the latter - ie. consolidates the code for a change but does not make one.
Before
do not reply address constructed in multiple places
After
do not reply constructed in only one (it would be possible to go further & also return the longer variant with the <> part but that can follow as a wrapper function later.
Technical Details
In terms of the bigger picture I would suggest that we use the civicrm_mail_settings table & store another type - although there might be a case for option value or setting
Comments