From 3e120a63e1efb94a28408945f3c55585a3a0cfa0 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Thu, 23 Jan 2020 11:08:30 +0000 Subject: [PATCH] Add CiviCase option for showing case activities in normal views Different aproach to tackling #16353 Fix array-to-string notice; Improve activity action links remove upgrader - as per Pradeep's suggestion update version added tweak way description is gotten update activity tests to check civicaseShowCaseActivities setting Fix another array-to-string conversion error Remove File On Case from activity View; add Manage Case fix case activity links when using caseShowCaseActivities remove ctx parameter for case activity links various US-Englishized Case settings texts. fix permission name --- CRM/Activity/BAO/Activity.php | 122 +++++++++--- CRM/Activity/Selector/Activity.php | 2 +- CRM/Admin/Form/Setting/Case.php | 1 + CRM/Case/BAO/Case.php | 3 +- CRM/Case/Info.php | 2 +- settings/Case.setting.php | 15 ++ templates/CRM/Admin/Form/Setting/Case.tpl | 6 + .../phpunit/CRM/Activity/BAO/ActivityTest.php | 175 +++++++++++++++++- 8 files changed, 295 insertions(+), 31 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 1f0e7a4fe8f..59b9d813667 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -697,6 +697,9 @@ public static function getActivities($params) { 'case_id', 'campaign_id', ]; + // Q. What does the code below achieve? case_id and campaign_id are already + // in the array, defined above, and this code adds them in again if their + // component is enabled? @fixme remove case_id and campaign_id from the array above? foreach (['case_id' => 'CiviCase', 'campaign_id' => 'CiviCampaign'] as $attr => $component) { if (in_array($component, self::activityComponents())) { $activityParams['return'][] = $attr; @@ -780,6 +783,7 @@ public static function getActivities($params) { // 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. + $caseIds = []; foreach ($result as $id => $activity) { $isBulkActivity = (!$bulkActivityTypeID || ($bulkActivityTypeID === $activity['activity_type_id'])); foreach ($mappingParams as $apiKey => $expectedName) { @@ -804,10 +808,9 @@ public static function getActivities($params) { // fetch case subject for case ID found if (!empty($activity['case_id'])) { - $activities[$id]['case_subject'] = civicrm_api3('Case', 'getvalue', [ - 'return' => 'subject', - 'id' => reset($activity['case_id']), - ]); + // Store cases; we'll look them up in one query below. We convert + // to int here so we can trust it for SQL. + $caseIds[$id] = (int) current($activity['case_id']); } } else { @@ -827,6 +830,15 @@ public static function getActivities($params) { $activities[$id]['is_recurring_activity'] = CRM_Core_BAO_RecurringEntity::getParentFor($id, 'civicrm_activity'); } + // Look up any case subjects we need in a single query and add them in the relevant activities under 'case_subject' + if ($caseIds) { + $subjects = CRM_Core_DAO::executeQuery('SELECT id, subject FROM civicrm_case WHERE id IN (' . implode(',', array_unique($caseIds)) . ')') + ->fetchMap('id', 'subject'); + foreach ($caseIds as $activityId => $caseId) { + $result[$activityId]['case_subject'] = $subjects[$caseId]; + } + } + return $activities; } @@ -2526,6 +2538,9 @@ public static function getContactActivitySelector(&$params) { // Format params and add links. $contactActivities = []; + // View-only activity types + $viewOnlyCaseActivityTypeIDs = array_flip(CRM_Activity_BAO_Activity::getViewOnlyActivityTypeIDs()); + if (!empty($activities)) { $activityStatus = CRM_Core_PseudoConstant::activityStatus(); @@ -2538,6 +2553,7 @@ public static function getContactActivitySelector(&$params) { } $mask = CRM_Core_Action::mask($permissions); + $userID = CRM_Core_Session::getLoggedInContactID(); foreach ($activities as $activityId => $values) { $activity = ['source_contact_name' => '', 'target_contact_name' => '']; @@ -2663,29 +2679,83 @@ public static function getContactActivitySelector(&$params) { $accessMailingReport = TRUE; } - $actionLinks = CRM_Activity_Selector_Activity::actionLinks( - CRM_Utils_Array::value('activity_type_id', $values), - CRM_Utils_Array::value('source_record_id', $values), - $accessMailingReport, - CRM_Utils_Array::value('activity_id', $values) - ); + // Get action links. + + // If this is a case activity, then we hand off to Case's actionLinks instead. + if (!empty($values['case_id']) && Civi::settings()->get('civicaseShowCaseActivities')) { + // This activity belongs to a case. + $caseId = current($values['case_id']); + + $activity['subject'] = $values['subject']; + + // Get the view and edit (update) links: + $caseActionLinks = + $actionLinks = array_intersect_key( + CRM_Case_Selector_Search::actionLinks(), + array_fill_keys([CRM_Core_Action::VIEW, CRM_Core_Action::UPDATE], NULL)); + + // Create a Manage Case link (using ADVANCED as can't use two VIEW ones) + $actionLinks[CRM_Core_Action::ADVANCED] = [ + "name" => 'Manage Case', + "url" => 'civicrm/contact/view/case', + 'qs' => 'reset=1&id=%%caseid%%&cid=%%cid%%&action=view&context=&selectedChild=case', + "title" => ts('Manage Case %1', [1 => $caseId]), + 'class' => 'no-popup', + ]; + + $caseLinkValues = [ + 'aid' => $activityId, + 'caseid' => $caseId, + 'cid' => current(CRM_Case_BAO_Case::getCaseClients($caseId) ?? []), + // Unlike other 'context' params, this 'ctx' param is appended raw to the URL. + 'cxt' => '', + ]; + + $caseActivityPermissions = CRM_Core_Action::VIEW | CRM_Core_Action::ADVANCED; + // Allow Edit link if: + // 1. Activity type is NOT view-only type. CRM-5871 + // 2. User has edit permission. + if (!isset($viewOnlyCaseActivityTypeIDs[$values['activity_type_id']]) + && CRM_Case_BAO_Case::checkPermission($activityId, 'edit', $values['activity_type_id'], $userID)) { + // We're allowed to edit. + $caseActivityPermissions |= CRM_Core_Action::UPDATE; + } - $actionMask = array_sum(array_keys($actionLinks)) & $mask; - - $activity['links'] = CRM_Core_Action::formLink($actionLinks, - $actionMask, - [ - 'id' => $values['activity_id'], - 'cid' => $params['contact_id'], - 'cxt' => $context, - 'caseid' => CRM_Utils_Array::value('case_id', $values), - ], - ts('more'), - FALSE, - 'activity.tab.row', - 'Activity', - $values['activity_id'] - ); + $activity['links'] = CRM_Core_Action::formLink($actionLinks, + $caseActivityPermissions, + $caseLinkValues, + ts('more'), + FALSE, + 'activity.tab.row', + 'Activity', + $values['activity_id'] + ); + } + else { + // Non-case activity + $actionLinks = CRM_Activity_Selector_Activity::actionLinks( + CRM_Utils_Array::value('activity_type_id', $values), + CRM_Utils_Array::value('source_record_id', $values), + $accessMailingReport, + CRM_Utils_Array::value('activity_id', $values) + ); + $actionMask = array_sum(array_keys($actionLinks)) & $mask; + + $activity['links'] = CRM_Core_Action::formLink($actionLinks, + $actionMask, + [ + 'id' => $values['activity_id'], + 'cid' => $params['contact_id'], + 'cxt' => $context, + 'caseid' => NULL, + ], + ts('more'), + FALSE, + 'activity.tab.row', + 'Activity', + $values['activity_id'] + ); + } if ($values['is_recurring_activity']) { $activity['is_recurring_activity'] = CRM_Core_BAO_RecurringEntity::getPositionAndCount($values['activity_id'], 'civicrm_activity'); diff --git a/CRM/Activity/Selector/Activity.php b/CRM/Activity/Selector/Activity.php index 744ee580bd5..724119a1106 100644 --- a/CRM/Activity/Selector/Activity.php +++ b/CRM/Activity/Selector/Activity.php @@ -459,7 +459,7 @@ public function &getRows($action, $offset, $rowCount, $sort, $output = NULL, $ca 'id' => $row['activity_id'], 'cid' => $this->_contactId, 'cxt' => $this->_context, - 'caseid' => CRM_Utils_Array::value('case_id', $row), + 'caseid' => isset($row['case_id']) ? current($row['case_id']) : NULL, ], ts('more'), FALSE, diff --git a/CRM/Admin/Form/Setting/Case.php b/CRM/Admin/Form/Setting/Case.php index 38331609214..58331a8b2f9 100644 --- a/CRM/Admin/Form/Setting/Case.php +++ b/CRM/Admin/Form/Setting/Case.php @@ -25,6 +25,7 @@ class CRM_Admin_Form_Setting_Case extends CRM_Admin_Form_Setting { 'civicaseAllowMultipleClients' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, 'civicaseNaturalActivityTypeSort' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, 'civicaseActivityRevisions' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, + 'civicaseShowCaseActivities' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, ]; /** diff --git a/CRM/Case/BAO/Case.php b/CRM/Case/BAO/Case.php index f481c016f0c..f24066c6fbb 100644 --- a/CRM/Case/BAO/Case.php +++ b/CRM/Case/BAO/Case.php @@ -15,6 +15,7 @@ * @copyright CiviCRM LLC https://civicrm.org/licensing */ + /** * This class contains the functions for Case Management. */ @@ -284,7 +285,7 @@ public static function retrieveContactIdsByCaseId($caseId, $contactID = NULL, $s * * @param int $activityId * - * @return int, case ID + * @return int|null, case ID */ public static function getCaseIdByActivityId($activityId) { $originalId = CRM_Core_DAO::singleValueQuery( diff --git a/CRM/Case/Info.php b/CRM/Case/Info.php index c25af90a007..0f996ee0c89 100644 --- a/CRM/Case/Info.php +++ b/CRM/Case/Info.php @@ -36,7 +36,7 @@ public function getInfo() { 'translatedName' => ts('CiviCase'), 'title' => ts('CiviCase Engine'), 'search' => 1, - 'showActivitiesInCore' => 0, + 'showActivitiesInCore' => Civi::settings()->get('civicaseShowCaseActivities') ?? 0, ]; } diff --git a/settings/Case.setting.php b/settings/Case.setting.php index 23c793bdd3e..44dc6055695 100644 --- a/settings/Case.setting.php +++ b/settings/Case.setting.php @@ -99,4 +99,19 @@ 'description' => ts('Enable tracking of activity revisions embedded within the "civicrm_activity" table. Alternatively, see "Administer => System Settings => Misc => Logging".'), 'help_text' => '', ], + 'civicaseShowCaseActivities' => [ + 'group_name' => 'CiviCRM Preferences', + 'group' => 'core', + 'name' => 'civicaseShowCaseActivities', + 'type' => 'Boolean', + 'quick_form_type' => 'YesNo', + 'default' => FALSE, + 'html_type' => 'radio', + 'add' => '5.24', + 'title' => ts('Include case activities in general activity views.'), + 'is_domain' => 1, + 'is_contact' => 0, + 'description' => ts('e.g. the Contact form\'s Activity tab listing. Without this ticked, activities that belong to a case are hidden (default behavior). Warning: enabling this option means that all case activities relating to a contact will be listed which could result in users without "access all cases and activities" permission being able to see see the summarized details (date, subject, assignees, status etc.). Such users will still be prevented from managing the case and viewing/editing the activity.'), + 'help_text' => '', + ], ]; diff --git a/templates/CRM/Admin/Form/Setting/Case.tpl b/templates/CRM/Admin/Form/Setting/Case.tpl index 5df543574fc..825826c6aad 100644 --- a/templates/CRM/Admin/Form/Setting/Case.tpl +++ b/templates/CRM/Admin/Form/Setting/Case.tpl @@ -37,6 +37,12 @@ {ts}Enable embedded tracking to activity revisions within the "civicrm_activity" table. Alternatively, see "Administer => System Settings => Misc => Logging".{/ts} + + {$form.civicaseShowCaseActivities.label} + {$form.civicaseShowCaseActivities.html}
+ {$civicaseShowCaseActivities_description} + +
{include file="CRM/common/formButtons.tpl" location="bottom"}
diff --git a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php index 890a2854a8d..236be3c440f 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -331,18 +331,36 @@ public function testDeleteActivityAssignment() { /** * Test getActivities BAO method for getting count. + * */ public function testGetActivitiesCountForAdminDashboard() { + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); $this->setUpForActivityDashboardTests(); + $this->addCaseWithActivity(); + CRM_Core_Config::singleton()->userPermissionClass->permissions[] = 'access all cases and activities'; + $activityCount = CRM_Activity_BAO_Activity::getActivitiesCount($this->_params); $this->assertEquals(8, $activityCount); + + // If we're showing case activities, we exepct to see one more (the scheduled meeting)... + $this->setShowCaseActivitiesInCore(TRUE); + $activityCount = CRM_Activity_BAO_Activity::getActivitiesCount($this->_params); + $this->assertEquals(9, $activityCount); + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); } /** * Test getActivities BAO method for getting count + * */ public function testGetActivitiesCountforNonAdminDashboard() { + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); $this->createTestActivities(); + $this->addCaseWithActivity(); + CRM_Core_Config::singleton()->userPermissionClass->permissions[] = 'access all cases and activities'; $params = [ 'contact_id' => 9, @@ -358,15 +376,27 @@ public function testGetActivitiesCountforNonAdminDashboard() { ]; //since we are loading activities from dataset, we know total number of activities for this contact - // 5 activities ( 2 scheduled, 3 Completed ), note that dashboard shows only scheduled activities + // 5 activities ( 2 scheduled, 3 Completed, 1 Scheduled Case activity ), + // note that dashboard shows only scheduled activities $this->assertEquals(2, CRM_Activity_BAO_Activity::getActivitiesCount($params)); + + // If we're showing case activities, we exepct to see one more (the scheduled meeting)... + $this->setShowCaseActivitiesInCore(TRUE); + $this->assertEquals(3, CRM_Activity_BAO_Activity::getActivitiesCount($params)); + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); } /** * Test getActivities BAO method for getting count + * */ public function testGetActivitiesCountforContactSummary() { + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); $this->createTestActivities(); + $this->addCaseWithActivity(); + CRM_Core_Config::singleton()->userPermissionClass->permissions[] = 'access all cases and activities'; $params = [ 'contact_id' => 9, @@ -380,8 +410,14 @@ public function testGetActivitiesCountforContactSummary() { ]; //since we are loading activities from dataset, we know total number of activities for this contact - // 5 activities, Contact Summary should show all activities + // 5 activities $this->assertEquals(5, CRM_Activity_BAO_Activity::getActivitiesCount($params)); + + // If we're showing case activities, we exepct to see one more (the scheduled meeting)... + $this->setShowCaseActivitiesInCore(TRUE); + $this->assertEquals(6, CRM_Activity_BAO_Activity::getActivitiesCount($params)); + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); } /** @@ -447,7 +483,11 @@ public function testGetActivitiesCountforContactSummaryWithNoActivities() { * Test getActivities BAO method. */ public function testGetActivitiesForAdminDashboard() { + $this->setShowCaseActivitiesInCore(FALSE); $this->setUpForActivityDashboardTests(); + $this->addCaseWithActivity(); + CRM_Core_Config::singleton()->userPermissionClass->permissions[] = 'access all cases and activities'; + $activitiesNew = CRM_Activity_BAO_Activity::getActivities($this->_params); // $this->assertEquals($activities, $activitiesDeprecatedFn); @@ -463,6 +503,16 @@ public function testGetActivitiesForAdminDashboard() { $this->assertEquals($value['status_id'], 1, 'Verify all activities are scheduled.'); } } + + // Now check that we get the scheduled meeting, if civicaseShowCaseActivities is set. + $this->setShowCaseActivitiesInCore(TRUE); + $activitiesNew = CRM_Activity_BAO_Activity::getActivities($this->_params); + $this->assertEquals(9, count($activitiesNew)); + // Scan through to find the meeting. + $this->assertTrue(in_array('test meeting activity', array_column($activitiesNew, 'subject')), + "failed to find scheduled case Meeting activity"); + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); } /** @@ -492,7 +542,10 @@ public function testGetActivitiesForAdminDashboardAclLimitedViewContacts() { * Test getActivities BAO method. */ public function testGetActivitiesforNonAdminDashboard() { + $this->setShowCaseActivitiesInCore(FALSE); $this->createTestActivities(); + $this->addCaseWithActivity(); + CRM_Core_Config::singleton()->userPermissionClass->permissions[] = 'access all cases and activities'; $contactID = 9; $params = [ @@ -527,6 +580,17 @@ public function testGetActivitiesforNonAdminDashboard() { } } } + + // Now check that we get the scheduled meeting, if civicaseShowCaseActivities is set. + $this->setShowCaseActivitiesInCore(TRUE); + $activities = CRM_Activity_BAO_Activity::getActivities($params); + $this->assertEquals(3, count($activities)); + // Scan through to find the meeting. + $this->assertTrue(in_array('test meeting activity', array_column($activities, 'subject')), + "failed to find scheduled case Meeting activity"); + + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); } /** @@ -586,7 +650,11 @@ public function testGetActivitiesforContactSummaryWithSortOptions() { * Test getActivities BAO method. */ public function testGetActivitiesForContactSummary() { + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); $this->createTestActivities(); + $this->addCaseWithActivity(); + CRM_Core_Config::singleton()->userPermissionClass->permissions[] = 'access all cases and activities'; $contactID = 9; $params = [ @@ -635,12 +703,24 @@ public function testGetActivitiesForContactSummary() { $this->assertArrayHasKey($contactID, $value['assignee_contact_name']); } } + + // Now check that we get the scheduled meeting, if civicaseShowCaseActivities is set. + $this->setShowCaseActivitiesInCore(TRUE); + $activities = CRM_Activity_BAO_Activity::getActivities($params); + $this->assertEquals(6, count($activities)); + // Scan through to find the meeting. + $this->assertTrue(in_array('test meeting activity', array_column($activities, 'subject')), + "failed to find scheduled case Meeting activity"); + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); } /** * Test getActivities BAO method. */ public function testGetActivitiesforContactSummaryWithActivities() { + // Reset to default + $this->setShowCaseActivitiesInCore(FALSE); $this->createTestActivities(); // parameters for different test cases, check each array key for the specific test-case @@ -1417,4 +1497,95 @@ public function testSendEmailWithCaseId() { $mut->stop(); } + /** + * Adds a case with one activity. + * + */ + protected function addCaseWithActivity() { + // case is not enabled by default do that now. + $enableResult = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCase'); + $this->assertTrue($enableResult, 'Cannot enable CiviCase in line ' . __LINE__); + + // We need a minimal case setup. + $case_type_id = civicrm_api3('CaseType', 'get', ['return' => 'id', 'name' => 'test_case_type'])['id'] ?? NULL; + if (!$case_type_id) { + $params = [ + 'name' => 'test_case_type', + 'title' => 'test_case_type', + "is_active" => "1", + "definition" => [ + "activityTypes" => [ + ["name" => "Open Case", "max_instances" => "1"], + ["name" => "Meeting"], + ], + "activitySets" => [ + [ + "name" => "standard_timeline", + "label" => "Standard Timeline", + "timeline" => "1", + "activityTypes" => [ + [ + "name" => "Open Case", + "status" => "Completed", + "label" => "Open Case", + "default_assignee_type" => "1", + ], + ], + ], + ], + "timelineActivityTypes" => [ + [ + "name" => "Open Case", + "status" => "Completed", + "label" => "Open Case", + "default_assignee_type" => "1", + ], + ], + "caseRoles" => [ + [ + "name" => "Case Coordinator", + "creator" => "1", + "manager" => "1", + ], + ], + ], + ]; + $case_type_id = $this->callAPISuccess('CaseType', 'create', $params)['id']; + } + + // Create a case with Contact #3 as the client. + $case_id = civicrm_api3('case', 'get', ['subject' => 'test case 1'])['id'] ?? NULL; + if (!$case_id) { + // Create case + $params = [ + 'subject' => 'test case 1', + 'contact_id' => 3, + 'status_id' => 'Open', + 'case_type_id' => $case_type_id, + 'creator_id' => 3, + ]; + $case_id = $this->callAPISuccess('case', 'create', $params)['id']; + } + + // Create a scheduled 'Meeting' activity that belongs to this case, but is + // assigned to contact #9 + $activity_id = $this->callAPISuccess('Activity', 'create', [ + 'activity_type_id' => 'Meeting', + 'status_id' => 'Scheduled', + 'case_id' => $case_id, + 'source_contact_id' => 3, + 'assignee_id' => [9], + 'subject' => 'test meeting activity', + ])['id'] ?? NULL; + } + + /** + * Change setting, and the cache of it. + */ + protected function setShowCaseActivitiesInCore(bool $val) { + Civi::settings()->set('civicaseShowCaseActivities', $val ? 1 : 0); + CRM_Core_Component::getEnabledComponents(); + Civi::$statics['CRM_Core_Component']['info']['CiviCase']->info['showActivitiesInCore'] = $val; + } + }