From 4f39b1e67a6b56ba6fd6addfdc12f8991fb8130f Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Sat, 15 May 2021 13:27:08 +1200 Subject: [PATCH] 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 --- CRM/Contact/BAO/GroupContactCache.php | 20 +++--- .../CRM/Contact/BAO/GroupContactCacheTest.php | 69 ++++++++++--------- tests/phpunit/CRM/Contact/BAO/GroupTest.php | 2 +- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 2 +- tests/phpunit/CRM/Group/Page/AjaxTest.php | 6 +- tests/phpunit/CRM/Mailing/BAO/MailingTest.php | 37 +++++----- 6 files changed, 72 insertions(+), 64 deletions(-) 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. -- 2.25.1