Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dev/core#942 fix failure to render names for some activities #14231

Merged
merged 3 commits into from
May 12, 2019
Merged

dev/core#942 fix failure to render names for some activities #14231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e324b8
dev/core#942 fix failure to render names for some activities
eileenmcnaughton May 10, 2019
b2cfc4b
Fix Failing Activity Test
seamuslee001 May 11, 2019
cdd5f82
Proper fix tests
seamuslee001 May 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 65 additions & 29 deletions CRM/Activity/BAO/Activity.php
View file
Open in desktop
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_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))
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
Open in desktop
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
Open in desktop
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']);
}

}