Performance fix for alternate getActivity listing function

We have an alternate function to render the activiy listing on the contact tab. It is
getActivities whereas the other is deprecatedGetActivities.

It was developed in order to replace the other and we have tests that compare the results of the 2. It is better in that it
1) performs better (on a  WMF contact with many activities this is 'snappy' while the current deprecated one gives a  white screen time out) and
2) calls the selectWhereClause hook, allowing hook alteration and respecting preferred architecture.

However, we didn't go live with it in core because it
1) has a remnant performance bugs (this PR fixes the last of these)
2) implements ACLs differently - it uses generic functions whereas the deprecated one
applies more limited permissioning. This is something to clarify & resolve separately.

This PR fixes the last remaining performance issue - best described as
'When one of the activities to be displayed has many targets the activity listing is slow to load'

The reason for the slowness is that when 'target_contact_name' is passed to the api
the api does a call for each contact to fetch the contact's sort_name. For a bulk mailing that went to 50,000 people that equates to 50,000 extra queries.

However the actual display shows the first contact name and then gives a number for how many more should be retrieved. This PR hence does not ask the api for the display name, but rather does the check itself, but
only for 1 target contact rather than ALL

Note that a similar logic might be considered for assignee - I left that out of scope as I'm not
aware of situations where a large number of assignees would be assigned to a single activity.

The unit test ensures the output matches the deprecated function.

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index 478873183dd95ecfef95c5c3c172103f98895f0b..7249a8e6d6bba683e8da3e14b44152a9801a9c1f 100644 (file)
--- a/CRM/Activity/BAO/Activity.php
+++ b/CRM/Activity/BAO/Activity.php
@@ -706,7 +706,6 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
       'source_contact_name',
       'assignee_contact_id',
       'target_contact_id',
-      'target_contact_name',
       'assignee_contact_name',
       'status_id',
       'subject',
@@ -740,7 +739,6 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
       'subject' => 'subject',
       'campaign_id' => 'campaign_id',
       'assignee_contact_name' => 'assignee_contact_name',
-      'target_contact_name' => 'target_contact_name',
       'source_contact_id' => 'source_contact_id',
       'source_contact_name' => 'source_contact_name',
       'case_id' => 'case_id',
@@ -751,13 +749,19 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
       $activities[$id] = array();
 
       $isBulkActivity = (!$bulkActivityTypeID || ($bulkActivityTypeID === $activity['activity_type_id']));
-
+      $activities[$id]['target_contact_counter'] = count($activity['target_contact_id']);
+      if ($activities[$id]['target_contact_counter']) {
+        try {
+          $activities[$id]['target_contact_name'][$activity['target_contact_id'][0]] = civicrm_api3('Contact', 'getvalue', ['id' => $activity['target_contact_id'][0], 'return' => 'sort_name']);
+        }
+        catch (CiviCRM_API3_Exception $e) {
+          // Really they should have names but a fatal here feels wrong.
+          $activities[$id]['target_contact_name'] = '';
+        }
+      }
       foreach ($mappingParams as $apiKey => $expectedName) {
         if (in_array($apiKey, array('assignee_contact_name', 'target_contact_name'))) {
           $activities[$id][$expectedName] = CRM_Utils_Array::value($apiKey, $activity, array());
-          if ($apiKey == 'target_contact_name' && count($activity['target_contact_name'])) {
-            $activities[$id]['target_contact_counter'] = count($activity['target_contact_name']);
-          }
 
           if ($isBulkActivity) {
             $activities[$id]['recipients'] = ts('(%1 recipients)', array(1 => count($activity['target_contact_name'])));

diff --git a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php
index 4fbf7b511d88dc76677fe3e78155fc11ba8fc46a..988148cf622b26e2235df808b1c1beb98e0060da 100644 (file)
--- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php
+++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php
@@ -585,6 +585,7 @@ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase {
     foreach (array($activitiesDep, CRM_Activity_BAO_Activity::getActivities($params)) as $activities) {
       //verify target count
       $this->assertEquals($targetCount, $activities[1]['target_contact_counter']);
+      $this->assertEquals([$targetContactIDs[0] => 'Anderson, Anthony'], $activities[1]['target_contact_name']);
     }
 
   }