From 7e324b87fcfd75370db6213976dbe00a79e32bb3 Mon Sep 17 00:00:00 2001 From: eileenmcnaugton Date: Fri, 10 May 2019 16:13:11 +1200 Subject: [PATCH] dev/core#942 fix failure to render names for some activities Overview ---------------------------------------- Set limit for activity_contact retrieval to 0, allowing to retrieve more than 25 activity contacts when rendering the first 25 activities on the activity contact tab Before ---------------------------------------- ![before](https://user-images.githubusercontent.com/336308/57439801-e42a0580-729a-11e9-80a1-45df93d0c5eb.jpg) After ---------------------------------------- ![after](https://user-images.githubusercontent.com/336308/57439960-39fead80-729b-11e9-9701-acd79ff73497.jpg) Technical Details ---------------------------------------- This moves the logic for retrieving the target contacts back into the getActivities function. We are stil not wanting to bypass the ACLs so still using the api but strictly limiting the number of contacts we retrieve (at the cost of extra queries, but cheap ones). Some tests added on the Bulk Mail activity. Comments ---------------------------------------- --- CRM/Activity/BAO/Activity.php | 94 +++++++++++++------ api/v3/Activity.php | 4 + .../phpunit/CRM/Activity/BAO/ActivityTest.php | 68 ++++++++------ 3 files changed, 106 insertions(+), 60 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index af4961d5ab..5d4ca14c2f 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -705,7 +705,6 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { 'source_contact_id', 'source_contact_name', 'assignee_contact_id', - 'target_contact_id', 'assignee_contact_name', 'status_id', 'subject', @@ -719,7 +718,7 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { $activityParams['return'][] = $attr; } } - $result = civicrm_api3('Activity', 'Get', $activityParams); + $result = civicrm_api3('Activity', 'Get', $activityParams)['values']; $bulkActivityTypeID = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Bulk Email'); $allCampaigns = CRM_Campaign_BAO_Campaign::getCampaigns(NULL, NULL, FALSE, FALSE, FALSE, TRUE); @@ -730,44 +729,83 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { (CRM_Mailing_Info::workflowEnabled() && CRM_Core_Permission::check('create mailings')) ); + // @todo - get rid of this & just handle in the array declaration like we do with 'subject' etc. $mappingParams = [ - 'id' => 'activity_id', 'source_record_id' => 'source_record_id', 'activity_type_id' => 'activity_type_id', - 'activity_date_time' => 'activity_date_time', 'status_id' => 'status_id', - 'subject' => 'subject', 'campaign_id' => 'campaign_id', - 'assignee_contact_name' => 'assignee_contact_name', - 'source_contact_id' => 'source_contact_id', - 'source_contact_name' => 'source_contact_name', 'case_id' => 'case_id', ]; - foreach ($result['values'] as $id => $activity) { - - $activities[$id] = []; - - $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']); + if (empty($result)) { + $targetCount = []; + } + else { + $targetCount = CRM_Core_DAO::executeQuery(' + SELECT activity_id, count(*) as target_contact_count + FROM civicrm_activity_contact + INNER JOIN civicrm_contact c ON contact_id = c.id AND c.is_deleted = 0 + WHERE activity_id IN (' . implode(',', array_keys($result)) . ') + AND record_type_id = %1 + GROUP BY activity_id', [ + 1 => [ + CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_ActivityContact', 'record_type_id', 'Activity Targets'), + 'Integer' + ] + ])->fetchAll(); + } + foreach ($targetCount as $activityTarget) { + $result[$activityTarget['activity_id']]['target_contact_count'] = $activityTarget['target_contact_count']; + } + // Iterate through & do basic mappings & determine which ones we want to retrieve target count for. + foreach ($result as $id => $activity) { + $activities[$id] = [ + 'activity_id' => $activity['id'], + 'activity_date_time' => CRM_Utils_Array::value('activity_date_time', $activity), + 'subject' => CRM_Utils_Array::value('subject', $activity), + 'assignee_contact_name' => CRM_Utils_Array::value('assignee_contact_sort_name', $activity, []), + 'source_contact_id' => CRM_Utils_Array::value('source_contact_id', $activity), + 'source_contact_name' => CRM_Utils_Array::value('source_contact_name', $activity), + ]; + $activities[$id]['activity_type_name'] = CRM_Core_PseudoConstant::getName('CRM_Activity_BAO_Activity', 'activity_type_id', $activity['activity_type_id']); + $activities[$id]['activity_type'] = CRM_Core_PseudoConstant::getLabel('CRM_Activity_BAO_Activity', 'activity_type_id', $activity['activity_type_id']); + $activities[$id]['target_contact_count'] = CRM_Utils_Array::value('target_contact_count', $activity, 0); + if (!empty($activity['target_contact_count'])) { + $displayedTarget = civicrm_api3('ActivityContact', 'get', [ + 'activity_id' => $id, + 'check_permissions' => TRUE, + 'options' => ['limit' => 1], + 'record_type_id' => 'Activity Targets', + 'return' => ['contact_id.sort_name', 'contact_id'], + 'sequential' => 1, + ])['values']; + if (empty($displayedTarget[0])) { + $activities[$id]['target_contact_name'] = []; } - catch (CiviCRM_API3_Exception $e) { - // Really they should have names but a fatal here feels wrong. - $activities[$id]['target_contact_name'] = ''; + else { + $activities[$id]['target_contact_name'] = [$displayedTarget[0]['contact_id'] => $displayedTarget[0]['contact_id.sort_name']]; } } + if ($activities[$id]['activity_type_name'] === 'Bulk Email') { + $bulkActivities[] = $id; + // Get the total without permissions being passed but only display names after permissioning. + $activities[$id]['recipients'] = ts('(%1 recipients)', [1 => $activities[$id]['target_contact_count']]); + } + } + + // Eventually this second iteration should just handle the target contacts. It's a bit muddled at + // the moment as the bulk activity stuff needs unravelling & test coverage. + foreach ($result as $id => $activity) { + $isBulkActivity = (!$bulkActivityTypeID || ($bulkActivityTypeID === $activity['activity_type_id'])); foreach ($mappingParams as $apiKey => $expectedName) { if (in_array($apiKey, [ - 'assignee_contact_name', 'target_contact_name', ])) { - $activities[$id][$expectedName] = CRM_Utils_Array::value($apiKey, $activity, []); if ($isBulkActivity) { - $activities[$id]['recipients'] = ts('(%1 recipients)', [1 => count($activity['target_contact_name'])]); + // @todo - how is this used? Couldn't we use 'is_bulk' or something clearer? + // or the calling function could handle $activities[$id]['mailingId'] = FALSE; if ($accessCiviMail && ($mailingIDs === TRUE || in_array($activity['source_record_id'], $mailingIDs)) @@ -786,11 +824,9 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { } } else { + // @todo this generic assign could just be handled in array declaration earlier. $activities[$id][$expectedName] = CRM_Utils_Array::value($apiKey, $activity); - if ($apiKey == 'activity_type_id') { - $activities[$id]['activity_type'] = CRM_Core_PseudoConstant::getName('CRM_Activity_BAO_Activity', 'activity_type_id', $activities[$id][$expectedName]); - } - elseif ($apiKey == 'campaign_id') { + if ($apiKey == 'campaign_id') { $activities[$id]['campaign'] = CRM_Utils_Array::value($activities[$id][$expectedName], $allCampaigns); } } @@ -2525,7 +2561,7 @@ INNER JOIN civicrm_option_group grp ON (grp.id = option_group_id AND grp.name = elseif (!empty($values['recipients'])) { $activity['target_contact_name'] = $values['recipients']; } - elseif (isset($values['target_contact_counter']) && $values['target_contact_counter']) { + elseif (isset($values['target_contact_count']) && $values['target_contact_count']) { $activity['target_contact_name'] = ''; $firstTargetName = reset($values['target_contact_name']); $firstTargetContactID = key($values['target_contact_name']); @@ -2542,7 +2578,7 @@ INNER JOIN civicrm_option_group grp ON (grp.id = option_group_id AND grp.name = $activity['target_contact_name'] .= $targetLink; } - if ($extraCount = $values['target_contact_counter'] - 1) { + if ($extraCount = $values['target_contact_count'] - 1) { $activity['target_contact_name'] .= ";
" . "(" . ts('%1 more', [1 => $extraCount]) . ")"; } if ($showContactOverlay) { diff --git a/api/v3/Activity.php b/api/v3/Activity.php index 8940241508..4aa31a9da1 100644 --- a/api/v3/Activity.php +++ b/api/v3/Activity.php @@ -644,8 +644,10 @@ function _civicrm_api3_activity_fill_activity_contact_names(&$activities, $param 'activity_id', 'record_type_id', 'contact_id.display_name', + 'contact_id.sort_name', 'contact_id', ], + 'options' => ['limit' => 0], 'check_permissions' => !empty($params['check_permissions']), ]; if (count($activityContactTypes) < 3) { @@ -658,10 +660,12 @@ function _civicrm_api3_activity_fill_activity_contact_names(&$activities, $param if (in_array($recordType, ['target', 'assignee'])) { $activities[$activityContact['activity_id']][$recordType . '_contact_id'][] = $contactID; $activities[$activityContact['activity_id']][$recordType . '_contact_name'][$contactID] = isset($activityContact['contact_id.display_name']) ? $activityContact['contact_id.display_name'] : ''; + $activities[$activityContact['activity_id']][$recordType . '_contact_sort_name'][$contactID] = isset($activityContact['contact_id.sort_name']) ? $activityContact['contact_id.sort_name'] : ''; } else { $activities[$activityContact['activity_id']]['source_contact_id'] = $contactID; $activities[$activityContact['activity_id']]['source_contact_name'] = isset($activityContact['contact_id.display_name']) ? $activityContact['contact_id.display_name'] : ''; + $activities[$activityContact['activity_id']]['source_contact_sort_name'] = isset($activityContact['contact_id.sort_name']) ? $activityContact['contact_id.sort_name'] : ''; } } } diff --git a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php index 2ac124bbdc..27828d2d8e 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -539,12 +539,12 @@ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase { 'contact_id' => $contactId, 'context' => 'activity', ); - foreach (array(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']); - } - + $activities = CRM_Activity_BAO_Activity::getActivities($params); + //verify target count + $this->assertEquals($targetCount, $activities[1]['target_contact_count']); + $this->assertEquals([$targetContactIDs[0] => 'Anderson, Anthony'], $activities[1]['target_contact_name']); + $this->assertEquals('Anderson, Anthony', $activities[1]['source_contact_name']); + $this->assertEquals('Anderson, Anthony', $activities[1]['assignee_contact_name'][4]); } /** @@ -574,7 +574,7 @@ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase { /** * Test getActivities BAO method. */ - public function testGetActivitiesforContactSummary() { + public function testGetActivitiesForContactSummary() { $this->createTestActivities(); $contactID = 9; @@ -591,33 +591,37 @@ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase { //since we are loading activities from dataset, we know total number of activities for this contact // 5 activities, Contact Summary should show all activities $count = 5; - foreach (array(CRM_Activity_BAO_Activity::getActivities($params)) as $activities) { - - $this->assertEquals($count, count($activities)); - - foreach ($activities as $key => $value) { - $this->assertEquals($value['subject'], "subject {$key}", 'Verify activity subject is correct.'); + $activities = CRM_Activity_BAO_Activity::getActivities($params); + $this->assertEquals($count, count($activities)); + foreach ($activities as $key => $value) { + $this->assertEquals($value['subject'], "subject {$key}", 'Verify activity subject is correct.'); - if ($key > 8) { - $this->assertEquals($value['status_id'], 2, 'Verify all activities are scheduled.'); - } - else { - $this->assertEquals($value['status_id'], 1, 'Verify all activities are scheduled.'); - } + if ($key > 8) { + $this->assertEquals($value['status_id'], 2, 'Verify all activities are scheduled.'); + } + else { + $this->assertEquals($value['status_id'], 1, 'Verify all activities are scheduled.'); + } - if ($key > 8) { - $this->assertEquals($value['activity_type_id'], 1, 'Verify activity type is correct.'); - } - else { - $this->assertEquals($value['activity_type_id'], 2, 'Verify activity type is correct.'); - } + if ($key === 12) { + $this->assertEquals($value['activity_type'], 'Bulk Email', 'Verify activity type is correct.'); + $this->assertEquals('(2 recipients)', $value['recipients']); + $targetContactID = key($value['target_contact_name']); + // The 2 targets have ids 10 & 11. Since they are not sorted it could be either on some systems. + $this->assertTrue(in_array($targetContactID, [10, 11])); + } + elseif ($key > 8) { + $this->assertEquals($value['activity_type_id'], 1, 'Verify activity type is correct.'); + } + else { + $this->assertEquals($value['activity_type_id'], 2, 'Verify activity type is correct.'); + } - if ($key == 3) { - $this->assertArrayHasKey($contactID, $value['target_contact_name']); - } - elseif ($key == 4) { - $this->assertArrayHasKey($contactID, $value['assignee_contact_name']); - } + if ($key == 3) { + $this->assertEquals([$contactID => 'Test Contact ' . $contactID], $value['target_contact_name']); + } + elseif ($key == 4) { + $this->assertArrayHasKey($contactID, $value['assignee_contact_name']); } } } @@ -1320,6 +1324,8 @@ $text dirname(__FILE__) . '/activities_for_dashboard_count.xml' ) ); + // Make changes to improve variation in php since the xml method is brittle & relies on option values being unchanged. + $this->callAPISuccess('Activity', 'create', ['id' => 12, 'activity_type_id' => 'Bulk Email']); } } -- 2.25.1