Commit 734f2683 authored by eileen's avatar eileen

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.
parent f8f3fe34
......@@ -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'])));
......
......@@ -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']);
}
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment