From 97399fa171fb9119bbf336bcbb5b9ddf7340a630 Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Tue, 8 Jun 2021 13:57:52 +1200
Subject: [PATCH] Fix the populateTempTable to be more direct

It's currently using a round-about method to populate the table which goes through
apiv3 and the query object before winding up at the load function
which makes the same 3 calls now being called directly

Note the tests api_v3_ReportTemplateTest as well as the apiv4 tests
cover this function
---
 CRM/Contact/BAO/GroupContactCache.php         | 35 ++++++++++++++++---
 tests/phpunit/api/v3/ReportTemplateTest.php   |  7 ++--
 .../phpunit/api/v4/Entity/SavedSearchTest.php |  6 +++-
 3 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/CRM/Contact/BAO/GroupContactCache.php b/CRM/Contact/BAO/GroupContactCache.php
index 22d3339d7b..c36fb2a1b5 100644
--- a/CRM/Contact/BAO/GroupContactCache.php
+++ b/CRM/Contact/BAO/GroupContactCache.php
@@ -638,13 +638,15 @@ ORDER BY   gc.contact_id, g.children
   }
 
   /**
-   * Populate a temporary table with group ids and contact ids.
+   * [Internal core function] Populate a temporary table with group ids and contact ids.
    *
    * Do not call this outside of core tested code - it WILL change.
    *
    * @param array[int] $groupIDs
    * @param string $temporaryTable
    *
+   * @throws \API_Exception
+   * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
    */
   public static function populateTemporaryTableWithContactsInGroups(array $groupIDs, string $temporaryTable): void {
@@ -657,14 +659,39 @@ ORDER BY   gc.contact_id, g.children
     ]);
     $smartGroups = array_keys($groups['values']);
 
-    $query = "
+    $query = '
        SELECT DISTINCT group_contact.contact_id as contact_id
        FROM civicrm_group_contact group_contact
-       WHERE group_contact.group_id IN (" . implode(', ', $childAndParentGroupIDs) . ")
+       WHERE group_contact.group_id IN (' . implode(', ', $childAndParentGroupIDs) . ")
        AND group_contact.status = 'Added' ";
 
     if (!empty($smartGroups)) {
-      CRM_Contact_BAO_GroupContactCache::check($smartGroups);
+      $groupContactsTempTable = CRM_Utils_SQL_TempTable::build()
+        ->setCategory('gccache')
+        ->setMemory();
+      $locks = self::getLocksForRefreshableGroupsTo($smartGroups);
+      if (!empty($locks)) {
+        self::buildGroupContactTempTable(array_keys($locks), $groupContactsTempTable);
+        // Note in theory we could do this transfer from the temp
+        // table to the group_contact_cache table out-of-process - possibly by
+        // continuing on after the browser is released (which seems to be
+        // possibly possible https://stackoverflow.com/questions/15273570/continue-processing-php-after-sending-http-response
+        // or by making the table durable and using a cron to process it (or an ajax call
+        // at the end to process out of the queue.
+        // if we did that we would union in DISTINCT contact_id FROM
+        // $groupContactsTempTable->getName()
+        // but still use the last union for array_diff_key($smartGroups, $locks)
+        // as that would hold the already-cached groups (if any).
+        // Also - if we switched to the 'triple union' approach described above
+        // we could throw a try-catch around this line since best-effort would
+        // be good enough & potentially improve user experience.
+        self::updateCacheFromTempTable($groupContactsTempTable, array_keys($locks));
+
+        foreach ($locks as $lock) {
+          $lock->release();
+        }
+      }
+
       $smartGroups = implode(',', $smartGroups);
       $query .= "
         UNION DISTINCT
diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php
index 589e175e3c..ddddfc0f17 100644
--- a/tests/phpunit/api/v3/ReportTemplateTest.php
+++ b/tests/phpunit/api/v3/ReportTemplateTest.php
@@ -537,7 +537,8 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
   }
 
   /**
-   * Test the group filter works on the contribution summary (with a smart group).
+   * Test the group filter works on the contribution summary (with a smart
+   * group).
    *
    * @dataProvider getMembershipAndContributionReportTemplatesForGroupTests
    *
@@ -545,6 +546,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
    *   Name of the template to test.
    *
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public function testContributionSummaryWithSmartGroupFilter(string $template): void {
     $groupID = $this->setUpPopulatedSmartGroup();
@@ -568,8 +570,9 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
    * @param string $template
    *
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
-  public function testContributionSummaryWithNotINSmartGroupFilter($template) {
+  public function testContributionSummaryWithNotINSmartGroupFilter($template): void {
     $groupID = $this->setUpPopulatedSmartGroup();
     $rows = $this->callAPISuccess('report_template', 'getrows', [
       'report_id' => 'contribute/summary',
diff --git a/tests/phpunit/api/v4/Entity/SavedSearchTest.php b/tests/phpunit/api/v4/Entity/SavedSearchTest.php
index 67b92b723c..072c5a0fac 100644
--- a/tests/phpunit/api/v4/Entity/SavedSearchTest.php
+++ b/tests/phpunit/api/v4/Entity/SavedSearchTest.php
@@ -28,7 +28,11 @@ use Civi\Api4\Email;
  */
 class SavedSearchTest extends UnitTestCase {
 
-  public function testContactSmartGroup() {
+  /**
+   * @throws \API_Exception
+   * @throws \Civi\API\Exception\NotImplementedException
+   */
+  public function testContactSmartGroup(): void {
     $in = Contact::create(FALSE)->addValue('first_name', 'yes')->addValue('do_not_phone', TRUE)->execute()->first();
     $out = Contact::create(FALSE)->addValue('first_name', 'no')->addValue('do_not_phone', FALSE)->execute()->first();
 
-- 
2.25.1