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

dev/mail#32 - Further checks - making mailing test email non-case-sensitive #13401

Merged

Conversation

martinh-pw
Copy link
Contributor

As per https://lab.civicrm.org/dev/mail/issues/32
Further work in addition to #13392

No longer assuming existing contact emails will be lowercase in the db, now will match them in a case insensitive way as well.

@civibot civibot bot added the master label Jan 4, 2019
@civibot
Copy link

civibot bot commented Jan 4, 2019

(Standard links)

@@ -637,7 +637,7 @@ function civicrm_api3_mailing_send_test($params) {
$query = CRM_Utils_SQL_Select::from('civicrm_email e')
->select(array('e.id', 'e.contact_id', 'e.email'))
->join('c', 'INNER JOIN civicrm_contact c ON e.contact_id = c.id')
->where('e.email IN (@emails)', array('@emails' => $testEmailParams['emails']))
->where('LOWER(e.email) IN (@emails)', array('@emails' => $testEmailParams['emails']))
Copy link
Contributor

Choose a reason for hiding this comment

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

actually we don't need this LOWER - mysql is case insensitive - it's php we need to wrangle.

Note that using LOWER will significantly slow down the query - a slightly similar example is in this blog https://civicrm.org/blog/eileen/guppies-have-died-for-that-query (diff function but same underlying issue)

@jusfreeman
Copy link
Contributor

@martinh-pw thanks for this fix

@eileenmcnaughton I've tested this PR with your amendment to remove LOWER from SQL. Can confirm this work. Nice fix.

@martinh-pw
Copy link
Contributor Author

@eileenmcnaughton @jusfreeman Thanks :)

Sorry but I'm unfamiliar with the process here, is there a way I should modify this PR somehow? Or to close it somehow and create a new one?

@eileenmcnaughton
Copy link
Contributor

@martinh-pw yes you can modify this - make the change & then do

git add -p
git commit --amend

Then you will have fixed up the commit.

You can then force push it to your branch with

git push -f myrepo mybranch

Update master from upstream
@martinh-pw
Copy link
Contributor Author

@eileenmcnaughton Done, nice trick!

@eileenmcnaughton eileenmcnaughton merged commit 2ba2ec7 into civicrm:master Feb 1, 2019
@eileenmcnaughton
Copy link
Contributor

looks good

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.

3 participants