-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Conversation
@seamuslee001 if @Stoob confirms it is this easily added to the tests from last round of changes here? |
@eileenmcnaughton maybe i'll see what i can do |
@eileenmcnaughton was able to add a unit test in, Also added screenshots to show the change i think this is what your after @Stoob |
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; |
There was a problem hiding this comment.
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
just cross posting @Stoob confirming this fixes both of his issues #11047 (comment) |
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
1a2ad7d
to
669f45e
Compare
@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.) |
Jenkins re test this please |
thanks @monishdeb |
…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
After
Previous behavior restored user emails then system from emails.
ping @Stoob @eileenmcnaughton @mattwire I think this restores the previous behavior.