From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Sat, 15 May 2021 01:27:08 +0000 (+1200)
Subject: Cleanup tracking on group.load
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=4f39b1e67a6b56ba6fd6addfdc12f8991fb8130f;p=civicrm-core.git

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 e3cb43cb70..54f7738e9a 100644
--- 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 a0d06a8d26..8eaf620dbf 100644
--- 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 12a30dd15a..14262dda0f 100644
--- 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 03dd50e4ef..f3863cb9c5 100644
--- 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 71931a9f4a..e2c3482d0f 100644
--- 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 eb4f875239..fbfab8db9c 100644
--- 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.