fix of issue dev/core#127 (at gitlab), incorrect cache records for smart groups #12249

hosseinzoda · 2018-06-02T13:15:44Z

Overview

There was an issue with creating multiple smart group with related contact component mode.
https://lab.civicrm.org/dev/core/issues/127
filterRelatedContacts at Query.php, was not distinguishing different queries.

Before

After

Technical Details

I did add a check for already filtered queries at the beginning to prevent multiple calls to filter duplicate it. And it prevents to not distinguish a unique $arg_sig for each input.

civicrm-builder · 2018-06-02T13:15:45Z

Can one of the admins verify this patch?

seamuslee001 · 2018-06-02T22:39:15Z

Jenkins ok to test

seamuslee001 · 2018-06-02T22:59:39Z

@hosseinamin Hossin are you able to fix the style issues found by our test bot? https://test.civicrm.org/job/CiviCRM-Core-PR/20982/checkstyleResult/new/

hosseinzoda · 2018-06-03T01:19:17Z

@seamuslee001 did fix the styling issues.

seamuslee001 · 2018-06-03T01:20:54Z

looks like there are still a couple of issues https://test.civicrm.org/job/CiviCRM-Core-PR/20984/checkstyleResult/new/

eileenmcnaughton · 2018-06-03T05:19:30Z

test this please

eileenmcnaughton · 2018-06-03T05:22:07Z

tests/phpunit/WebTest/Contact/GroupAddTest.php

+  /**
+   *  group update_smart_groups action test
+   */
+  public function testGroupUpdateSmartGroupsForRelatedContacts() {


Unfortunately we don't really run these webtests anymore - @totten @colemanw perhaps the time has come to remove them to avoid things like this?

We would normally use unit tests - likely calling loadAll directly [EDITED - or by calling the api per my examples below]

I don't how the other tests would work. Did try to run the test with closest example i could find.

So this is probably the test I would start with

https://github.com/davecivicrm/civicrm-core/blob/fae75b583b102cf9cfdc64ba0adc08091f8dd5a8/tests/phpunit/api/v3/ContactTest.php#L3523

It already loads multiple groups - so that is good - but none of them have this specific related contact criteria that is hitting the problem

(I'm trying to find a test that creates a smart group now)

OK - this is from ActionSchedule Test

// create smart group which will contain all Male contacts $smartGroupParams = array('formValues' => array('gender_id' => 1)); $smartGroupID = $this->smartGroupCreate( $smartGroupParams, array( 'name' => 'new_smart_group', 'title' => 'New Smart Group', 'parents' => array($groupID => 1), ) );

So something like

public function testLoadMultipleRelatedGroups // create smart group which will contain all Male contacts $smartGroupParams = array('formValues' => array(... related group params 1....)); $smartGroupID = $this->smartGroupCreate( $smartGroupParams, array( 'name' => 'new_smart_group', 'title' => 'New Smart Group', 'parents' => array($groupID => 1), ) ); // create related group 2 // create a contact that would be in only 1 $result = $this->callApiSuccess('Contact', 'get', ['group' => $group1['id']); $result2 $this->callApiSuccess('Contact', 'get', ['group' => $group2['id']); // Check result is not cross-contaminated

I tried to perform a unit-test. I get an error. Can you direct me to a guide for preparing to run the test?
Here's the command.

phpunit4 --filter testAddCreateIndividual tests/phpunit/api/v3/ContactTest.php

Error

exception 'RuntimeException' with message '_populateDB requires CIVICRM_UF=UnitTests'

I normally run tests from within phpunit - this is what my command line (generated by phpunit) ooks like

phpunit.php --bootstrap /Users/emcnaughton/buildkit/build/dmaster/sites/all/modules/civicrm/tests/phpunit/CiviTest/bootstrap.php --configuration /Users/emcnaughton/buildkit/build/dmaster/sites/all/modules/civicrm/phpunit.xml.dist --filter "/::testAddCreateIndividual( .*)?$/" api_v3_ContactTest /Users/emcnaughton/buildkit/build/dmaster/sites/all/modules/civicrm/tests/phpunit/api/v3/ContactTest.php

Ohh - looks like this is specifically relevant https://docs.civicrm.org/dev/en/latest/testing/phpunit/

The doc helped. Thank you @eileenmcnaughton

eileenmcnaughton · 2018-06-03T05:22:50Z

CRM/Contact/BAO/GroupContactCache.php

@@ -81,19 +81,19 @@ public static function check($groupIDs) {
   * @return string
   *   the sql query which lists the groups that need to be refreshed
   */
-  public static function groupRefreshedClause($groupIDClause = NULL, $includeHiddenGroups = FALSE) {
+  public static function groupRefreshedClause($groupIDClause = NULL, $includeHiddenGroups = FALSE, $ignoreCache = FALSE) {


I think we can avoid adding $ignoreCache here - I think it's being added purely for the unit test? If we use the \Civi::statics pattern we can probably solve the test issue

That's ok. We can remove it with unit-test.
I don't think \Civi::statics will help with unit-test.

Edited: An independent call to removing cache. Before loadAll method can help.

eileenmcnaughton · 2018-06-03T05:25:10Z

CRM/Contact/BAO/Query.php

-    if (!$_rTypeProcessed) {
-      $_rTypeProcessed = TRUE;
-
+    static $_rTempCache = array();


so per above - the way we can avoid cross-test contamination is using \Civi::statics - eg.

$arg_sig = sha1("$from $where $having");
if (!isset(\Civi::statics[CLASS]['related_contacts_filter'][$arg_sig]) {
// buld that key}

return !isset(\Civi::statics[CLASS]['related_contacts_filter'][$arg_sig]

I have not placed these code for the unit-test. This is the fix. Maybe changing it to \Civi::statics has advantages that i don't know.

@hosseinamin yes - I realised that the use of $arg_sig is the test & I think it will fix the bug as described once we get the tests sorted. And thank you for working on this!

You may find that once your test running in the phpunit Continuous integration the type of static might need changing. The reason is that the Civi::statics cache is flushed between tests so you might see unexpected fails elsewhere in the suite.

We used to have 2 suites of tests - the phpunit ones & the selenium / webtests ones. The latter still exist but they are too full of false negatives to run whereas the former run against every PR that is submitted and are run nightly on different php / mysql combos

hosseinzoda · 2018-06-04T03:20:16Z

@eileenmcnaughton I've removed ignore_cache, and new testing is working for me now.

eileenmcnaughton · 2018-06-04T06:57:26Z

@hosseinamin - looks like a few style tweaks needed - https://test.civicrm.org/job/CiviCRM-Core-PR/20990/checkstyleResult/new/

eileenmcnaughton · 2018-06-04T07:23:19Z

I just checked & I can confirm the test fails without the patch and passes with it and the patch seems correct.

A couple of stylistic things

are you able to rebase -i & squash the commits into one (you will need to do a force push)
the use of references for variables is less preferred stylistically in our code base (in part because it can lead to mistakes. I'll add a couple of in-line comments
I think this might be your first PR to be merged? If so thank you and well done. Could you follow up with a PR to the contributor's file to add your details? https://github.com/civicrm/civicrm-core/blob/master/contributor-key.yml

eileenmcnaughton · 2018-06-04T07:23:34Z

CRM/Contact/BAO/Query.php

+    }
+    $arg_sig = sha1("$from $where $having");
+    if (isset($_rTempCache[$arg_sig])) {
+      $cache =& $_rTempCache[$arg_sig];


In this case referring as reference doesn't actually help because it's being used read only anyway

eileenmcnaughton · 2018-06-04T07:24:51Z

CRM/Contact/BAO/Query.php

+        "from" => "",
+        "where" => "",
+      );
+      $cache =& $_rTempCache[$arg_sig];


In this case I think it would be more consistent with other places to build up the $cache value by itself & assign it to
Civi::$statics[CLASS]['related_contacts_filter'][$arg_sig] right at the end of this else

… with testunit

hosseinzoda · 2018-06-04T07:53:50Z

Did the changes.

eileenmcnaughton · 2018-06-04T08:05:08Z

CRM/Contact/BAO/Query.php

+    $_rTempCache =& Civi::$statics[__CLASS__]['related_contacts_filter'];
+    // skip if filter has already applied
+    foreach ($_rTempCache as $acache) {
+      if (strpos($from, $acache['from']) !== FALSE) {


I'm just wondering about this - it seems a bit unpredictable - ie. like it would happen when it is a subset of the response & like hopefully the improved $sig_arg would make it unnecessary - did you hit any case when it was invoked in your testing?

Yes i received an error. for injection of select and where parts twice. This loop will prevent it.

Unfortunately we can't do much with $sig_arg. Since there's no unique argument given.

OK - I think that convinces me - if there are scenarios that are not correctly calculated they will still be less & we can add tests for them if we find any. This one won't regress with your test

I tried with turning the cache variable for each instance. The down side is, it is getting instantiated multiple times and going through filter.

eileenmcnaughton · 2018-06-04T08:24:12Z

@agh1 @guanhuan do either of you have any specific acknowledgement process for first time committers. This is a great first contribution - fixes a somewhat obscure but critical bug and has a test to prevent recurrence

seamuslee001 · 2018-06-04T10:03:42Z

merging as per the tag

agh1 · 2018-06-04T14:47:09Z

@eileenmcnaughton I have no particular procedure for bringing out the welcome wagon, but it would be great to have one. Frankly, I don't usually notice until compiling the credits for a release and noticing that someone's user name isn't in contributor-key.yml.

Realistically, that's probably where it has to happen--you can't count on reviewers to remember who's new when they're merging PRs. However, it's also not a bad time: it's one thing to have a PR merged, and you get emails about that automatically. It's another to have your contribution be out in the world in a release, so a welcome/congratulations note then would be nice.

agh1 · 2018-06-04T14:47:52Z

But to that end--welcome @hosseinamin and thanks for the contribution!

civicrm-builder added the master label Jun 2, 2018

hosseinzoda changed the title ~~fix for issue #127 (at gitlab) incorrect cache records for smart groups~~ fix for issue dev/core#127 (at gitlab) incorrect cache records for smart groups Jun 2, 2018

hosseinzoda changed the title ~~fix for issue dev/core#127 (at gitlab) incorrect cache records for smart groups~~ fix of issue dev/core#127 (at gitlab) incorrect cache records for smart groups Jun 2, 2018

hosseinzoda changed the title ~~fix of issue dev/core#127 (at gitlab) incorrect cache records for smart groups~~ fix of issue dev/core#127 (at gitlab), incorrect cache records for smart groups Jun 2, 2018

eileenmcnaughton reviewed Jun 3, 2018

View reviewed changes

eileenmcnaughton reviewed Jun 4, 2018

View reviewed changes

fix for issue dev/core#127, incorrect cache records for smart groups,…

2bedfb3

… with testunit

hosseinzoda force-pushed the master branch from 09fa309 to 2bedfb3 Compare June 4, 2018 07:52

eileenmcnaughton reviewed Jun 4, 2018

View reviewed changes

eileenmcnaughton added the merge on pass label Jun 4, 2018

seamuslee001 merged commit 0d4655e into civicrm:master Jun 4, 2018

fix of issue dev/core#127 (at gitlab), incorrect cache records for smart groups #12249

fix of issue dev/core#127 (at gitlab), incorrect cache records for smart groups #12249

Conversation

hosseinzoda commented Jun 2, 2018

Overview

Before

After

Technical Details

civicrm-builder commented Jun 2, 2018

seamuslee001 commented Jun 2, 2018

seamuslee001 commented Jun 2, 2018

hosseinzoda commented Jun 3, 2018

seamuslee001 commented Jun 3, 2018

eileenmcnaughton commented Jun 3, 2018

eileenmcnaughton Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

hosseinzoda Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hosseinzoda Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hosseinzoda Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hosseinzoda commented Jun 4, 2018

eileenmcnaughton commented Jun 4, 2018

eileenmcnaughton commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hosseinzoda commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jun 4, 2018

seamuslee001 commented Jun 4, 2018

agh1 commented Jun 4, 2018

agh1 commented Jun 4, 2018

eileenmcnaughton Jun 3, 2018 •

edited

Loading

eileenmcnaughton Jun 3, 2018 •

edited

Loading

hosseinzoda Jun 3, 2018 •

edited

Loading

hosseinzoda Jun 3, 2018 •

edited

Loading

hosseinzoda Jun 3, 2018 •

edited

Loading