From 734f268381c93b2983f6afd2c268cc7cdae37815 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Mon, 4 Feb 2019 13:10:44 +1300
Subject: [PATCH] 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.
---
 CRM/Activity/BAO/Activity.php                   | 16 ++++++++++------
 tests/phpunit/CRM/Activity/BAO/ActivityTest.php |  1 +
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index 478873183d..7249a8e6d6 100644
--- 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 4fbf7b511d..988148cf62 100644
--- 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']);
     }
 
   }
-- 
2.25.1