Skip to content

Commit

Permalink
Merge pull request #14223 from eileenmcnaughton/5.14
Browse files Browse the repository at this point in the history 
dev/core#942 fix failure to render names for some activities
  • Loading branch information
@seamuslee001
seamuslee001 committed May 11, 2019
2 parents c2f5bff + c2ce41b commit 6ed8da0
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 60 deletions.
94 changes: 65 additions & 29 deletions CRM/Activity/BAO/Activity.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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);
Expand All @@ -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_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))
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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']);
Expand All @@ -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'] .= ";<br />" . "(" . ts('%1 more', [1 => $extraCount]) . ")";
}
if ($showContactOverlay) {
Expand Down
4 changes: 4 additions & 0 deletions api/v3/Activity.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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'] : '';
}
}
}
Expand Down
68 changes: 37 additions & 31 deletions tests/phpunit/CRM/Activity/BAO/ActivityTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}

/**
Expand Down Expand Up @@ -574,7 +574,7 @@ public function testGetActivitiesforContactSummaryWithSortOptions() {
/**
* Test getActivities BAO method.
*/
public function testGetActivitiesforContactSummary() {
public function testGetActivitiesForContactSummary() {
$this->createTestActivities();

$contactID = 9;
Expand All @@ -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']);
}
}
}
Expand Down Expand Up @@ -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']);
}

}

0 comments on commit 6ed8da0

Please sign in to comment.