diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index af4961d5ab5e..3f1f37b6e318 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -705,7 +705,6 @@ public static function getActivities($params) { 'source_contact_id', 'source_contact_name', 'assignee_contact_id', - 'target_contact_id', 'assignee_contact_name', 'status_id', 'subject', @@ -719,7 +718,7 @@ public static function getActivities($params) { $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 @@ public static function getActivities($params) { (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_sort_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 @@ public static function getActivities($params) { } } 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 @@ public static function getContactActivitySelector(&$params) { 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 @@ public static function getContactActivitySelector(&$params) { $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 89402415080a..4aa31a9da1ba 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 2ac124bbdced..27828d2d8ed5 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -539,12 +539,12 @@ public function testTargetCountforContactSummary() { '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 @@ public function testGetActivitiesforContactSummaryWithSortOptions() { /** * Test getActivities BAO method. */ - public function testGetActivitiesforContactSummary() { + public function testGetActivitiesForContactSummary() { $this->createTestActivities(); $contactID = 9; @@ -591,33 +591,37 @@ public function testGetActivitiesforContactSummary() { //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 @@ protected function createTestActivities() { 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']); } }