Cleanup tracking on group.load

1) deprecate force parameter
2) ditch the static & let the db lookups speak for themselves

The force paramter seems to mostly be about the static & probably only exists to support tests.
The static doesn't make sense now because either they group needs loading or it doesn't.
The fact the process might have tried and succeeded or failed before doesn't need
to be recorded in the static

Cleanup up progress tracking in Group::load

Fix test to do things in a logical order so contacts exist when the group is created

diff --git a/CRM/Contact/BAO/GroupContactCache.php b/CRM/Contact/BAO/GroupContactCache.php
index e3cb43cb701778dab7caf8635192932dd361b1a9..54f7738e9afc5a5824dd157b42052e22773eff8b 100644 (file)
--- a/CRM/Contact/BAO/GroupContactCache.php
+++ b/CRM/Contact/BAO/GroupContactCache.php
@@ -19,8 +19,6 @@ use Civi\Api4\Query\SqlExpression;
  */
 class CRM_Contact_BAO_GroupContactCache extends CRM_Contact_DAO_GroupContactCache {
 
-  public static $_alreadyLoaded = [];
-
   /**
    * Get a list of caching modes.
    *
@@ -237,7 +235,6 @@ WHERE  id IN ( $groupIDs )
     CRM_Core_DAO::executeQuery($query, $params);
     // also update the cache_date for these groups
     CRM_Core_DAO::executeQuery($update, $params);
-    unset(self::$_alreadyLoaded[$groupID]);
 
     $transaction->commit();
   }
@@ -364,7 +361,7 @@ WHERE  id IN ( $groupIDs )
    * @param object $group
    *   The smart group that needs to be loaded.
    * @param bool $force
-   *   Should we force a search through.
+   *   deprecated parameter = Should we force a search through.
    *
    * @throws \API_Exception
    * @throws \CRM_Core_Exception
@@ -372,27 +369,26 @@ WHERE  id IN ( $groupIDs )
    */
   public static function load($group, $force = FALSE) {
     $groupID = (int) $group->id;
-    if (array_key_exists($groupID, self::$_alreadyLoaded) && !$force) {
-      return;
+    if ($force) {
+      CRM_Core_Error::deprecatedWarning('use invalidate group contact cache first.');
+      self::invalidateGroupContactCache($group->id);
     }
 
-    self::$_alreadyLoaded[$groupID] = 1;
-
     // FIXME: some other process could have actually done the work before we got here,
     // Ensure that work needs to be done before continuing
-    if (!$force && !self::shouldGroupBeRefreshed($groupID, TRUE)) {
+    if (!self::shouldGroupBeRefreshed($groupID, TRUE)) {
       return;
     }
 
+    // grab a lock so other processes don't compete and do the same query
+    $lock = Civi::lockManager()->acquire("data.core.group.{$groupID}");
+
     $groupContactsTempTable = CRM_Utils_SQL_TempTable::build()
       ->setCategory('gccache')
       ->setMemory();
     self::buildGroupContactTempTable([$groupID], $groupContactsTempTable);
     $tempTable = $groupContactsTempTable->getName();
 
-    // grab a lock so other processes don't compete and do the same query
-    $lock = Civi::lockManager()->acquire("data.core.group.{$groupID}");
-
     if (!$lock->isAcquired()) {
       // this can cause inconsistent results since we don't know if the other process
       // will fill up the cache before our calling routine needs it.

diff --git a/tests/phpunit/CRM/Contact/BAO/GroupContactCacheTest.php b/tests/phpunit/CRM/Contact/BAO/GroupContactCacheTest.php
index a0d06a8d263aae7e62208b7bb456b22c9f05da97..8eaf620dbf92c577732680edbe099adf00cce844 100644 (file)
--- a/tests/phpunit/CRM/Contact/BAO/GroupContactCacheTest.php
+++ b/tests/phpunit/CRM/Contact/BAO/GroupContactCacheTest.php
@@ -19,17 +19,21 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
 
   /**
    * Manually add and remove contacts from a smart group.
+   *
+   * @throws \API_Exception
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
-  public function testManualAddRemove() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+  public function testManualAddRemove(): void {
+    [$group, $living, $deceased] = $this->setupSmartGroup();
 
     // Add $n1 to $g
-    $this->callAPISuccess('group_contact', 'create', [
+    $this->callAPISuccess('GroupContact', 'create', [
       'contact_id' => $living[0]->id,
       'group_id' => $group->id,
     ]);
 
-    CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($group);
     $this->assertCacheMatches(
       [$deceased[0]->id, $deceased[1]->id, $deceased[2]->id, $living[0]->id],
       $group->id
@@ -42,7 +46,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
       'status' => 'Removed',
     ]);
 
-    CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($group);
     $this->assertCacheMatches(
       [
         $deceased[1]->id,
@@ -54,9 +58,15 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
   }
 
   /**
-   * Allow removing contact from a parent group even if contact is in a child group. (CRM-8858).
+   * Allow removing contact from a parent group even if contact is in a child
+   * group. (CRM-8858).
+   *
+   * @throws \CRM_Core_Exception
    */
-  public function testRemoveFromParentSmartGroup() {
+  public function testRemoveFromParentSmartGroup(): void {
+    // Create $c1, $c2, $c3
+    $deceased = $this->createTestObject('CRM_Contact_DAO_Contact', ['is_deceased' => 1], 3);
+
     // Create smart group $parent
     $params = [
       'name' => 'Deceased Contacts',
@@ -77,9 +87,6 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
     $child = CRM_Contact_BAO_Group::create($params);
     $this->registerTestObjects([$child]);
 
-    // Create $c1, $c2, $c3
-    $deceased = $this->createTestObject('CRM_Contact_DAO_Contact', ['is_deceased' => 1], 3);
-
     // Add $c1, $c2, $c3 to $child
     foreach ($deceased as $contact) {
       $this->callAPISuccess('group_contact', 'create', [
@@ -88,21 +95,21 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
       ]);
     }
 
-    CRM_Contact_BAO_GroupContactCache::load($parent, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($parent);
     $this->assertCacheMatches(
       [$deceased[0]->id, $deceased[1]->id, $deceased[2]->id],
       $parent->id
     );
 
     // Remove $c1 from $parent
-    $this->callAPISuccess('group_contact', 'create', [
+    $this->callAPISuccess('GroupContact', 'create', [
       'contact_id' => $deceased[0]->id,
       'group_id' => $parent->id,
       'status' => 'Removed',
     ]);
 
     // Assert $c1 not in $parent
-    CRM_Contact_BAO_GroupContactCache::load($parent, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($parent);
     $this->assertCacheMatches(
       [
         $deceased[1]->id,
@@ -129,7 +136,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    *   Array(int).
    * @param int $groupId
    */
-  public function assertCacheMatches($expectedContactIds, $groupId) {
+  public function assertCacheMatches($expectedContactIds, $groupId): void {
     $sql = 'SELECT contact_id FROM civicrm_group_contact_cache WHERE group_id = %1';
     $params = [1 => [$groupId, 'Integer']];
     $dao = CRM_Core_DAO::executeQuery($sql, $params);
@@ -147,7 +154,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * Test the opportunistic refresh cache function does not touch non-expired entries.
    */
   public function testOpportunisticRefreshCacheNoChangeIfNotExpired() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     $this->assertCacheMatches(
       [$deceased[0]->id, $deceased[1]->id, $deceased[2]->id],
@@ -162,7 +169,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * Test the opportunistic refresh cache function does refresh stale entries.
    */
   public function testOpportunisticRefreshChangeIfCacheDateFieldStale() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     CRM_Core_DAO::executeQuery('UPDATE civicrm_group SET cache_date = DATE_SUB(NOW(), INTERVAL 7 MINUTE) WHERE id = ' . $group->id);
     $group->find(TRUE);
@@ -177,7 +184,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * Test the opportunistic refresh cache function does refresh expired entries if mode is deterministic.
    */
   public function testOpportunisticRefreshNoChangeWithDeterministicSetting() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Setting', 'create', ['smart_group_cache_refresh_mode' => 'deterministic']);
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     $this->makeCacheStale($group);
@@ -190,7 +197,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * Test the deterministic cache function refreshes with the deterministic setting.
    */
   public function testDeterministicRefreshChangeWithDeterministicSetting() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Setting', 'create', ['smart_group_cache_refresh_mode' => 'deterministic']);
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     $this->makeCacheStale($group);
@@ -203,7 +210,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * Test the deterministic cache function refresh doesn't mess up non-expired.
    */
   public function testDeterministicRefreshChangeDoesNotTouchNonExpired() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Setting', 'create', ['smart_group_cache_refresh_mode' => 'deterministic']);
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     CRM_Contact_BAO_GroupContactCache::deterministicCacheFlush();
@@ -217,7 +224,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * (hey it's an opportunity!).
    */
   public function testDeterministicRefreshChangeWithOpportunisticSetting() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Setting', 'create', ['smart_group_cache_refresh_mode' => 'opportunistic']);
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     $this->makeCacheStale($group);
@@ -229,7 +236,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    * Test the api job wrapper around the deterministic refresh works.
    */
   public function testJobWrapper() {
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
     $this->callAPISuccess('Setting', 'create', ['smart_group_cache_refresh_mode' => 'opportunistic']);
     $this->callAPISuccess('Contact', 'create', ['id' => $deceased[0]->id, 'is_deceased' => 0]);
     $this->makeCacheStale($group);
@@ -314,7 +321,13 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
    *
    * @return array
    */
-  protected function setupSmartGroup() {
+  protected function setupSmartGroup(): array {
+    // Create contacts $y1, $y2, $y3 which do match $g; create $n1, $n2, $n3 which do not match $g
+    $living = $this->createTestObject('CRM_Contact_DAO_Contact', ['is_deceased' => 0], 3);
+    $deceased = $this->createTestObject('CRM_Contact_DAO_Contact', ['is_deceased' => 1], 3);
+    $this->assertCount(3, $deceased);
+    $this->assertCount(3, $living);
+
     $params = [
       'name' => 'Deceased Contacts',
       'title' => 'Deceased Contacts',
@@ -324,14 +337,8 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
     $group = CRM_Contact_BAO_Group::createSmartGroup($params);
     $this->registerTestObjects([$group]);
 
-    // Create contacts $y1, $y2, $y3 which do match $g; create $n1, $n2, $n3 which do not match $g
-    $living = $this->createTestObject('CRM_Contact_DAO_Contact', ['is_deceased' => 0], 3);
-    $deceased = $this->createTestObject('CRM_Contact_DAO_Contact', ['is_deceased' => 1], 3);
-    $this->assertEquals(3, count($deceased));
-    $this->assertEquals(3, count($living));
-
     // Assert: $g cache has exactly $y1, $y2, $y3
-    CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($group);
     $group->find(TRUE);
     $this->assertCacheMatches(
       [$deceased[0]->id, $deceased[1]->id, $deceased[2]->id],
@@ -393,7 +400,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
       'sort_name' => 1,
       'group' => 1,
     ];
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
 
     $params = [
       'name' => 'Living Contacts',
@@ -467,7 +474,7 @@ class CRM_Contact_BAO_GroupContactCacheTest extends CiviUnitTestCase {
       'sort_name' => 1,
       'group' => 1,
     ];
-    list($group, $living, $deceased) = $this->setupSmartGroup();
+    [$group, $living, $deceased] = $this->setupSmartGroup();
 
     $params = [
       'name' => 'Living Contacts',

diff --git a/tests/phpunit/CRM/Contact/BAO/GroupTest.php b/tests/phpunit/CRM/Contact/BAO/GroupTest.php
index 12a30dd15a08d2db86c7c5f7618907fd302f8981..14262dda0fdca44ff4e6c3e55e883e1809be919c 100644 (file)
--- a/tests/phpunit/CRM/Contact/BAO/GroupTest.php
+++ b/tests/phpunit/CRM/Contact/BAO/GroupTest.php
@@ -225,7 +225,7 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase {
       $group->id = $groupID;
       $group->find(TRUE);
 
-      CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+      CRM_Contact_BAO_GroupContactCache::load($group);
     }
   }
 

diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php
index 03dd50e4ef67e151816f9d0ee1598f8222628927..f3863cb9c5a6a0dcf6cb9b387fbdbe8245748029 100644 (file)
--- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php
+++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php
@@ -543,7 +543,7 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase {
     // the group & could generate invalid sql if a bug were introduced.
     $groupParams = ['title' => 'postal codes', 'formValues' => $params, 'is_active' => 1];
     $group = CRM_Contact_BAO_Group::createSmartGroup($groupParams);
-    CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($group);
   }
 
   /**

diff --git a/tests/phpunit/CRM/Group/Page/AjaxTest.php b/tests/phpunit/CRM/Group/Page/AjaxTest.php
index 71931a9f4a51fff8a5ca221e367c6a63e7b846fe..e2c3482d0f1eacbd78aaa290a703d035db119a44 100644 (file)
--- a/tests/phpunit/CRM/Group/Page/AjaxTest.php
+++ b/tests/phpunit/CRM/Group/Page/AjaxTest.php
@@ -561,8 +561,8 @@ class CRM_Group_Page_AjaxTest extends CiviUnitTestCase {
     $group = CRM_Contact_BAO_Group::createSmartGroup($groupParams);
 
     // Ensure the smart group is created.
-    $this->assertTrue(is_int($group->id), "Smart group created successfully.");
-    CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+    $this->assertIsInt($group->id, "Smart group created successfully.");
+    CRM_Contact_BAO_GroupContactCache::load($group);
 
     // Ensure it is populating the cache when loaded.
     $sql = 'SELECT contact_id FROM civicrm_group_contact_cache WHERE group_id = %1';
@@ -629,7 +629,7 @@ class CRM_Group_Page_AjaxTest extends CiviUnitTestCase {
 
     // Do it again, but this time don't clear group contact cache. Instead,
     // set it to expire.
-    CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+    CRM_Contact_BAO_GroupContactCache::load($group);
     $params['name'] = 'smartGroupCacheTimeout';
     $timeout = civicrm_api3('Setting', 'getvalue', $params);
     $timeout = intval($timeout) * 60;

diff --git a/tests/phpunit/CRM/Mailing/BAO/MailingTest.php b/tests/phpunit/CRM/Mailing/BAO/MailingTest.php
index eb4f87523949efc0f1aea4b517c39d43d6575c03..fbfab8db9cca59530b2206c97f5c5b23e576b8a5 100644 (file)
--- a/tests/phpunit/CRM/Mailing/BAO/MailingTest.php
+++ b/tests/phpunit/CRM/Mailing/BAO/MailingTest.php
@@ -270,9 +270,27 @@ class CRM_Mailing_BAO_MailingTest extends CiviUnitTestCase {
    * contact 8 : smart 5 (base)
    *
    * here 'contact 1 : static 0 (inc)' identified as static group $groupIDs[0]
-   *  that has 'contact 1' identified as $contactIDs[0] and Included in the mailing recipient list
+   *  that has 'contact 1' identified as $contactIDs[0] and Included in the
+   * mailing recipient list
+   *
+   * @throws \CiviCRM_API3_Exception
+   * @throws \CRM_Core_Exception
+   * @throws \API_Exception
    */
-  public function testgetRecipientsEmailGroupIncludeExclude() {
+  public function testGetRecipientsEmailGroupIncludeExclude(): void {
+    // Create contacts
+    $contactIDs = [
+      $this->individualCreate(['last_name' => 'smart5'], 0),
+      $this->individualCreate([], 1),
+      $this->individualCreate([], 2),
+      $this->individualCreate([], 3),
+      $this->individualCreate(['last_name' => 'smart3'], 4),
+      $this->individualCreate(['last_name' => 'smart3'], 5),
+      $this->individualCreate(['last_name' => 'smart4'], 6),
+      $this->individualCreate(['last_name' => 'smart4'], 7),
+      $this->individualCreate(['last_name' => 'smart5'], 8),
+    ];
+
     // Set up groups; 3 standard, 4 smart
     $groupIDs = [];
     for ($i = 0; $i < 7; $i++) {
@@ -291,19 +309,6 @@ class CRM_Mailing_BAO_MailingTest extends CiviUnitTestCase {
       }
     }
 
-    // Create contacts
-    $contactIDs = [
-      $this->individualCreate(['last_name' => 'smart5'], 0),
-      $this->individualCreate([], 1),
-      $this->individualCreate([], 2),
-      $this->individualCreate([], 3),
-      $this->individualCreate(['last_name' => 'smart3'], 4),
-      $this->individualCreate(['last_name' => 'smart3'], 5),
-      $this->individualCreate(['last_name' => 'smart4'], 6),
-      $this->individualCreate(['last_name' => 'smart4'], 7),
-      $this->individualCreate(['last_name' => 'smart5'], 8),
-    ];
-
     // Add contacts to static groups
     $this->callAPISuccess('GroupContact', 'Create', [
       'group_id' => $groupIDs[0],
@@ -331,7 +336,7 @@ class CRM_Mailing_BAO_MailingTest extends CiviUnitTestCase {
       $group = new CRM_Contact_DAO_Group();
       $group->id = $groupIDs[$i];
       $group->find(TRUE);
-      CRM_Contact_BAO_GroupContactCache::load($group, TRUE);
+      CRM_Contact_BAO_GroupContactCache::load($group);
     }
 
     // Check that we can include static groups in the mailing.