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

Unit test for CRM-20970 #10781

Merged
merged 1 commit into from
Aug 3, 2017
Merged

Unit test for CRM-20970 #10781

merged 1 commit into from
Aug 3, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 28, 2017

@eileenmcnaughton
Copy link
Contributor Author

This is a unit test to go with #10769 change

note that I identified a further issue - searching for postal_code < 50 will include NULL values.. If someone wants to QA & review that I will follow up with a patch - the change is

  $field = "IF (civicrm_address.postal_code REGEXP '^[0-9]{1,10}$', CAST(civicrm_address.postal_code AS UNSIGNED), 0)";

should be

  $field = "IF (civicrm_address.postal_code REGEXP '^[0-9]{1,10}$', CAST(civicrm_address.postal_code AS UNSIGNED), NULL)";

@mfb
Copy link
Contributor

mfb commented Jul 28, 2017

to reproduce bug we have to hit CRM_Contact_BAO_GroupContactCache::load()

so maybe also need a test in tests/phpunit/CRM/Contact/BAO/GroupContactCacheTest.php?

@eileenmcnaughton
Copy link
Contributor Author

@mfb I'll help you if you wanna write one! However, I don't think the bug is replicable on my mysql version at all as I was testing by running the query directly

@mfb
Copy link
Contributor

mfb commented Jul 31, 2017

I'll work on a test when I can.. Here's a minimal query that should fail on any MySQL with STRICT_TRANS_TABLES (it's on by default in MySQL 5.7):

CREATE TEMPORARY TABLE foo (SELECT CAST('12345678444455555555555555555555555555555555551314151617181920' AS UNSIGNED) AS bar);
ERROR 1292 (22007): Truncated incorrect INTEGER value: '12345678444455555555555555555555555555555555551314151617181920'

@mfb
Copy link
Contributor

mfb commented Aug 1, 2017

👍 ok this is actually ready to be merged, my bad. I verified that the existing test does in fact create a smartgroup and therefore reproduces the Exception (test output is "F"; if you comment out the failing assertion then we hit the following lines which report "E"). Dev environment = Ubuntu 17.04 with MySQL 5.7 where STRICT_TRANS_TABLES is on by default.

@monishdeb
Copy link
Member

Nice 👍

@monishdeb monishdeb merged commit 24695c9 into civicrm:master Aug 3, 2017
@eileenmcnaughton eileenmcnaughton deleted the postal branch August 3, 2017 23:32
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.

4 participants