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

CRM-21244 Ensure consistancy with previous behavior where user emails … #11905

Merged
merged 1 commit into from
Apr 1, 2018

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Mar 31, 2018

…are first then system from emails.

Overview

This ensures consistency that @Stoob stumbled upon. User emails should be first then the system from emails.

Before

system from emails first user emails second

send_email_before

After

Previous behavior restored user emails then system from emails.
send_email_after

ping @Stoob @eileenmcnaughton @mattwire I think this restores the previous behavior.

@seamuslee001 seamuslee001 changed the title CRM-2144 Ensure consistancy with previous behavior where user emails … CRM-21244 Ensure consistancy with previous behavior where user emails … Mar 31, 2018
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 if @Stoob confirms it is this easily added to the tests from last round of changes here?

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton maybe i'll see what i can do

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton was able to add a unit test in, Also added screenshots to show the change i think this is what your after @Stoob

@Stoob
Copy link
Contributor

Stoob commented Mar 31, 2018

Yes, this does appear to work as far as ordering is concerned, thank you. Logged-in-user email is at top. I have another issue I'm seeing post 4.7.31 related to the 'from' address which I will post in #11047

@@ -324,10 +325,10 @@ public static function getFromEmail() {
if (!empty($emailVal['is_primary'])) {
$fromEmailHtml .= ' ' . ts('(preferred)');
}
$fromEmailValues[$emailId] = $fromEmailHtml;
$contactFromEmails[$fromEmail] = $fromEmailHtml;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changed keying here is at least then consistent with https://github.com/civicrm/civicrm-core/pull/11905/files#diff-dd5148d8b660fec931daeee2d6c2d201R290 which @Stoob reports on the other PR that it works

@seamuslee001
Copy link
Contributor Author

just cross posting @Stoob confirming this fixes both of his issues #11047 (comment)

@mattwire
Copy link
Contributor

Hey guys, happy with this. Re-ordering was an unintended side-effect but at least it only needs fixing in one place now instead of the six or so where the code was duplicated before :-)

…are first then system from emails.

Add unit test to try to verify order of emails

Fix issue where email was not being used as array key causing wrong email to be used
@seamuslee001 seamuslee001 changed the base branch from master to 5.0 March 31, 2018 21:27
@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton @monishdeb @totten i have just changed teh base of this to be 5.0 reasoning is that this is causing confusion in general users as per Stoob and others comments, it is a recent regression (email changes were in 4.7.31 iirc.)

@seamuslee001 seamuslee001 added 5.0 and removed master labels Mar 31, 2018
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@monishdeb
Copy link
Member

@monishdeb monishdeb merged commit fd36ea7 into civicrm:5.0 Apr 1, 2018
@eileenmcnaughton eileenmcnaughton deleted the CRM-21244-consistancy branch April 1, 2018 21:07
@seamuslee001
Copy link
Contributor Author

thanks @monishdeb

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

Successfully merging this pull request may close these issues.

5 participants