From 0d83522a2934da847aa5e7f7f1e1ab730e3c3875 Mon Sep 17 00:00:00 2001 From: DemeritCowboy Date: Mon, 15 Jul 2019 09:00:13 -0400 Subject: [PATCH] fix singleton warning url --- CRM/Case/Form/Activity.php | 122 ++++++++++++++++---- CRM/Case/PseudoConstant.php | 14 +-- tests/phpunit/CRM/Case/BAO/CaseTest.php | 147 +++++++++++++++++++++++- 3 files changed, 251 insertions(+), 32 deletions(-) diff --git a/CRM/Case/Form/Activity.php b/CRM/Case/Form/Activity.php index 1d225d6b14..f2927e11a6 100644 --- a/CRM/Case/Form/Activity.php +++ b/CRM/Case/Form/Activity.php @@ -178,29 +178,20 @@ class CRM_Case_Form_Activity extends CRM_Activity_Form_Activity { $caseType = $this->_caseType[$casePos]; $activityInst = $xmlProcessor->getMaxInstance($caseType); - // If not bounce back and also provide activity edit link + // If not bounce back and also provide activity edit link if only one existing activity if (isset($activityInst[$this->_activityTypeName])) { $activityCount = CRM_Case_BAO_Case::getCaseActivityCount($caseId, $this->_activityTypeId); - if ($activityCount >= $activityInst[$this->_activityTypeName]) { - if ($activityInst[$this->_activityTypeName] == 1) { - $atArray = ['activity_type_id' => $this->_activityTypeId]; - $activities = CRM_Case_BAO_Case::getCaseActivity($caseId, - $atArray, - $this->_currentUserId - ); - $activityId = CRM_Utils_Array::first(array_keys($activities['data'])); - $editUrl = CRM_Utils_System::url('civicrm/case/activity', - "reset=1&cid={$this->_currentlyViewedContactId}&caseid={$caseId}&action=update&id={$activityId}" - ); - } - CRM_Core_Error::statusBounce(ts("You can not add another '%1' activity to this case. %2", - [ - 1 => $this->_activityTypeName, - 2 => ts("Do you want to edit the existing activity?", [1 => "href='$editUrl'"]), - ] - ), - $url - ); + $editUrl = self::checkMaxInstances( + $caseId, + $this->_activityTypeId, + $activityInst[$this->_activityTypeName], + $this->_currentUserId, + $this->_currentlyViewedContactId, + $activityCount + ); + $bounceMessage = self::getMaxInstancesBounceMessage($editUrl, $this->_activityTypeName, $activityInst[$this->_activityTypeName], $activityCount); + if ($bounceMessage) { + CRM_Core_Error::statusBounce($bounceMessage, $url); } } } @@ -745,4 +736,93 @@ class CRM_Case_Form_Activity extends CRM_Activity_Form_Activity { return array_column($definitions['values'], 'definition', 'name'); } + /** + * Get the edit link for a case activity + * + * This isn't here for reusability - it was a pull out + * from preProcess to make it easier to test. + * There is CRM_Case_Selector_Search::addCaseActivityLinks but it would + * need some rejigging, and there's also a FIXME note there already. + * + * @param int $caseId + * @param int $activityTypeId + * @param int $currentUserId + * @param int $currentlyViewedContactId + * + * @return string + */ + public static function getCaseActivityEditLink($caseId, $activityTypeId, $currentUserId, $currentlyViewedContactId) { + $atArray = ['activity_type_id' => $activityTypeId]; + $activities = CRM_Case_BAO_Case::getCaseActivity($caseId, + $atArray, + $currentUserId + ); + $firstActivity = CRM_Utils_Array::first($activities['data']); + $activityId = empty($firstActivity['DT_RowId']) ? 0 : $firstActivity['DT_RowId']; + return CRM_Utils_System::url('civicrm/case/activity', + "reset=1&cid={$currentlyViewedContactId}&caseid={$caseId}&action=update&id={$activityId}" + ); + } + + /** + * Check the current activity count against max instances for a given case id and activity type. + * + * This isn't here for reusability - it was a pull out + * from preProcess to make it easier to test. + * + * @param int $caseId + * @param int $activityTypeId + * @param int $maxInstances + * @param int $currentUserId + * @param int $currentlyViewedContactId + * @param int $activityCount + * + * @return string + * If there is more than one existing activity of the given type then it's not clear which url to return so return null for the url. + */ + public static function checkMaxInstances($caseId, $activityTypeId, $maxInstances, $currentUserId, $currentlyViewedContactId, $activityCount) { + $editUrl = NULL; + if ($activityCount >= $maxInstances) { + if ($maxInstances == 1) { + $editUrl = self::getCaseActivityEditLink($caseId, $activityTypeId, $currentUserId, $currentlyViewedContactId); + } + } + return $editUrl; + } + + /** + * Compute the message text for the bounce message when max_instances is reached, depending on whether it's one or more than one. + * + * @param string $editUrl + * @param string $activityTypeName + * This is actually label!! But we do want label though in this function. + * @param int $maxInstances + * @param int $activityCount + * Count of existing activities of the given type on the case + * + * @return string + */ + public static function getMaxInstancesBounceMessage($editUrl, $activityTypeName, $maxInstances, $activityCount) { + $bounceMessage = NULL; + if ($activityCount >= $maxInstances) { + if ($maxInstances == 1) { + $bounceMessage = ts("You can not add another '%1' activity to this case. %2", + [ + 1 => $activityTypeName, + 2 => ts("Do you want to edit the existing activity?", [1 => "href='$editUrl'"]), + ] + ); + } + else { + // More than one instance, so don't provide a link. What would it be a link to anyway? + $bounceMessage = ts("You can not add another '%1' activity to this case.", + [ + 1 => $activityTypeName, + ] + ); + } + } + return $bounceMessage; + } + } diff --git a/CRM/Case/PseudoConstant.php b/CRM/Case/PseudoConstant.php index 7cc943acfa..a1bb966de8 100644 --- a/CRM/Case/PseudoConstant.php +++ b/CRM/Case/PseudoConstant.php @@ -36,12 +36,6 @@ */ class CRM_Case_PseudoConstant extends CRM_Core_PseudoConstant { - /** - * Activity type - * @var array - */ - public static $activityTypeList = []; - /** * Get all the case statues. * @@ -155,8 +149,8 @@ class CRM_Case_PseudoConstant extends CRM_Core_PseudoConstant { public static function &caseActivityType($indexName = TRUE, $all = FALSE) { $cache = (int) $indexName . '_' . (int) $all; - if (!array_key_exists($cache, self::$activityTypeList)) { - self::$activityTypeList[$cache] = []; + if (!isset(Civi::$statics[__CLASS__]['activityTypeList'][$cache])) { + Civi::$statics[__CLASS__]['activityTypeList'][$cache] = []; $query = " SELECT v.label as label ,v.value as value, v.name as name, v.description as description, v.icon @@ -194,9 +188,9 @@ class CRM_Case_PseudoConstant extends CRM_Core_PseudoConstant { $activityTypes[$index]['icon'] = $dao->icon; $activityTypes[$index]['description'] = $dao->description; } - self::$activityTypeList[$cache] = $activityTypes; + Civi::$statics[__CLASS__]['activityTypeList'][$cache] = $activityTypes; } - return self::$activityTypeList[$cache]; + return Civi::$statics[__CLASS__]['activityTypeList'][$cache]; } /** diff --git a/tests/phpunit/CRM/Case/BAO/CaseTest.php b/tests/phpunit/CRM/Case/BAO/CaseTest.php index fae0799d69..c9a24392a9 100644 --- a/tests/phpunit/CRM/Case/BAO/CaseTest.php +++ b/tests/phpunit/CRM/Case/BAO/CaseTest.php @@ -261,7 +261,8 @@ class CRM_Case_BAO_CaseTest extends CiviUnitTestCase { 'activity_date_time' => $now_date, 'target_contact_id' => $client_id, 'source_contact_id' => $loggedInUser, - 'subject' => 'null', // yeah this is extra weird, but without it you get the wrong subject + // yeah this is extra weird, but without it you get the wrong subject + 'subject' => 'null', ]; $form->postProcess($actParams); @@ -303,4 +304,148 @@ class CRM_Case_BAO_CaseTest extends CiviUnitTestCase { $_REQUEST = $oldREQUEST; } + /** + * Test max_instances + */ + public function testMaxInstances() { + $loggedInUser = $this->createLoggedInUser(); + $client_id = $this->individualCreate(); + $caseObj = $this->createCase($client_id, $loggedInUser); + $case_id = $caseObj->id; + + // Sanity check to make sure we'll be testing what we think we're testing. + $this->assertEquals($caseObj->case_type_id, 1); + + // Get the case type + $result = $this->callAPISuccess('CaseType', 'get', [ + 'sequential' => 1, + 'id' => 1, + ]); + $caseType = array_shift($result['values']); + $activityTypeName = $caseType['definition']['activityTypes'][1]['name']; + // Sanity check to make sure we'll be testing what we think we're testing. + $this->assertEquals($activityTypeName, "Medical evaluation"); + + // Look up the activity type label - we need it later + $result = $this->callAPISuccess('OptionValue', 'get', [ + 'sequential' => 1, + 'option_group_id' => 'activity_type', + 'name' => $activityTypeName, + ]); + $optionValue = array_shift($result['values']); + $activityTypeLabel = $optionValue['label']; + $this->assertNotEmpty($activityTypeLabel); + + // Locate the existing activity independently so we can check it + $result = $this->callAPISuccess('Activity', 'get', [ + 'sequential' => 1, + // this sometimes confuses me - pass in the name for the id + 'activity_type_id' => $activityTypeName, + ]); + // There should be only one in the database at this point so this should be the id. + $activity_id = $result['id']; + $this->assertNotEmpty($activity_id); + $this->assertGreaterThan(0, $activity_id); + $activityArr = array_shift($result['values']); + + // At the moment everything should be happy, although there's nothing to test because if max_instances has no value then nothing gets called, which is correct since it means unlimited. But we don't have a way to test that right now. For fun we could test max_instances=0 but that isn't the same as "not set". 0 would actually mean 0 are allowed, which is pointless, since then why would you even add the activity type to the config. + + // Update max instances for the activity type + // We're not really checking that the tested code has retrieved the new case type definition, just that given some numbers as input it returns the right thing as output, so these lines are mostly symbolic at the moment. + $caseType['definition']['activityTypes'][1]['max_instances'] = 1; + $this->callAPISuccess('CaseType', 'create', $caseType); + + // Now we should get a link back + $editUrl = CRM_Case_Form_Activity::checkMaxInstances( + $case_id, + $activityArr['activity_type_id'], + // max instances + 1, + $loggedInUser, + $client_id, + // existing activity count + 1 + ); + $this->assertNotNull($editUrl); + + $expectedUrl = CRM_Utils_System::url( + 'civicrm/case/activity', + "reset=1&cid={$client_id}&caseid={$case_id}&action=update&id={$activity_id}" + ); + $this->assertEquals($editUrl, $expectedUrl); + + // And also a bounce message is expected + $bounceMessage = CRM_Case_Form_Activity::getMaxInstancesBounceMessage( + $editUrl, + $activityTypeLabel, + // max instances, + 1, + // existing activity count + 1 + ); + $this->assertNotEmpty($bounceMessage); + + // Now check with max_instances = 2 + $caseType['definition']['activityTypes'][1]['max_instances'] = 2; + $this->callAPISuccess('CaseType', 'create', $caseType); + + // So it should now be back to being happy + $editUrl = CRM_Case_Form_Activity::checkMaxInstances( + $case_id, + $activityArr['activity_type_id'], + // max instances + 2, + $loggedInUser, + $client_id, + // existing activity count + 1 + ); + $this->assertNull($editUrl); + $bounceMessage = CRM_Case_Form_Activity::getMaxInstancesBounceMessage( + $editUrl, + $activityTypeLabel, + // max instances, + 2, + // existing activity count + 1 + ); + $this->assertEmpty($bounceMessage); + + // Add new activity check again + $newActivity = [ + 'case_id' => $case_id, + 'activity_type_id' => $activityArr['activity_type_id'], + 'status_id' => $activityArr['status_id'], + 'subject' => "A different subject", + 'activity_date_time' => date('Y-m-d H:i:s'), + 'source_contact_id' => $loggedInUser, + 'target_id' => $client_id, + ]; + $this->callAPISuccess('Activity', 'create', $newActivity); + + $editUrl = CRM_Case_Form_Activity::checkMaxInstances( + $case_id, + $activityArr['activity_type_id'], + // max instances + 2, + $loggedInUser, + $client_id, + // existing activity count + 2 + ); + // There should be no url here. + $this->assertNull($editUrl); + + // But there should be a warning message still. + $bounceMessage = CRM_Case_Form_Activity::getMaxInstancesBounceMessage( + $editUrl, + $activityTypeLabel, + // max instances, + 2, + // existing activity count + 2 + ); + $this->assertNotEmpty($bounceMessage); + } + } -- 2.25.1