From 831af101a1b0bf6f2e992d2888b6bd00fd9745a1 Mon Sep 17 00:00:00 2001 From: DemeritCowboy Date: Sat, 4 May 2019 21:07:53 -0400 Subject: [PATCH] fix change case status warning --- CRM/Case/Form/Activity.php | 5 +- CRM/Case/Form/Activity/ChangeCaseStatus.php | 2 +- tests/phpunit/CRM/Case/BAO/CaseTest.php | 124 +++++++++++++++++++- 3 files changed, 127 insertions(+), 4 deletions(-) diff --git a/CRM/Case/Form/Activity.php b/CRM/Case/Form/Activity.php index 4b9cdbe1dd..1d225d6b14 100644 --- a/CRM/Case/Form/Activity.php +++ b/CRM/Case/Form/Activity.php @@ -428,7 +428,10 @@ class CRM_Case_Form_Activity extends CRM_Activity_Form_Activity { } // store the submitted values in an array - $params = $this->controller->exportValues($this->_name); + // Explanation for why we only check the is_unittest element: Prior to adding that check, there was no check and so any $params passed in would have been overwritten. Just in case somebody is passing in some non-null params and that broken code would have inadvertently been working, we can maintain backwards compatibility by only checking for the is_unittest parameter, and so that broken code will still work. At the same time this allows unit tests to pass in a $params without it getting overwritten. See also PR #2077 for some discussion of when the $params parameter was added as a passed in variable. + if (empty($params['is_unittest'])) { + $params = $this->controller->exportValues($this->_name); + } //set parent id if its edit mode if ($parentId = CRM_Utils_Array::value('parent_id', $this->_defaults)) { diff --git a/CRM/Case/Form/Activity/ChangeCaseStatus.php b/CRM/Case/Form/Activity/ChangeCaseStatus.php index d0059b0cf2..6316f3a223 100644 --- a/CRM/Case/Form/Activity/ChangeCaseStatus.php +++ b/CRM/Case/Form/Activity/ChangeCaseStatus.php @@ -145,7 +145,7 @@ class CRM_Case_Form_Activity_ChangeCaseStatus { public static function beginPostProcess(&$form, &$params) { $params['id'] = CRM_Utils_Array::value('case_id', $params); - if ($params['updateLinkedCases'] === '1') { + if (CRM_Utils_Array::value('updateLinkedCases', $params) === '1') { $caseID = CRM_Utils_Array::first($form->_caseId); $cases = CRM_Case_BAO_Case::getRelatedCases($caseID); diff --git a/tests/phpunit/CRM/Case/BAO/CaseTest.php b/tests/phpunit/CRM/Case/BAO/CaseTest.php index 0d1c270d5f..fae0799d69 100644 --- a/tests/phpunit/CRM/Case/BAO/CaseTest.php +++ b/tests/phpunit/CRM/Case/BAO/CaseTest.php @@ -72,9 +72,14 @@ class CRM_Case_BAO_CaseTest extends CiviUnitTestCase { /** * Create and return case object of given Client ID. * @param $clientId + * @param $loggedInUser * @return CRM_Case_BAO_Case */ - private function createCase($clientId) { + private function createCase($clientId, $loggedInUser = NULL) { + if (empty($loggedInUser)) { + // backwards compatibility - but it's more typical that the creator is a different person than the client + $loggedInUser = $clientId; + } $caseParams = array( 'activity_subject' => 'Case Subject', 'client_id' => $clientId, @@ -88,7 +93,7 @@ class CRM_Case_BAO_CaseTest extends CiviUnitTestCase { 'activity_details' => '', ); $form = new CRM_Case_Form_Case(); - $caseObj = $form->testSubmit($caseParams, "OpenCase", $clientId, "standalone"); + $caseObj = $form->testSubmit($caseParams, "OpenCase", $loggedInUser, "standalone"); return $caseObj; } @@ -183,4 +188,119 @@ class CRM_Case_BAO_CaseTest extends CiviUnitTestCase { * } */ + /** + * Test various things after a case is closed. + * + * This annotation is not ideal, but without it there is some kind of + * messup that happens to quickform that persists between tests, e.g. + * it can't add maxfilesize validation rules. + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testCaseClosure() { + $loggedInUser = $this->createLoggedInUser(); + $client_id = $this->individualCreate(); + $caseObj = $this->createCase($client_id, $loggedInUser); + $case_id = $caseObj->id; + + // Get the case status option value for "Resolved" (name="Closed"). + $closed_status = $this->callAPISuccess('OptionValue', 'getValue', [ + 'return' => 'value', + 'option_group_id' => 'case_status', + 'name' => 'Closed', + ]); + $this->assertNotEmpty($closed_status); + + // Get the activity status option value for "Completed" + $completed_status = $this->callAPISuccess('OptionValue', 'getValue', [ + 'return' => 'value', + 'option_group_id' => 'activity_status', + 'name' => 'Completed', + ]); + $this->assertNotEmpty($completed_status); + + // Get the value for the activity type id we need to create + $atype = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Change Case Status'); + + // Now it gets weird. There doesn't seem to be a good way to test this, so we simulate a form and the various bits that go with it. + + // HTTP vars needed because that's how the form determines stuff + $oldMETHOD = empty($_SERVER['REQUEST_METHOD']) ? NULL : $_SERVER['REQUEST_METHOD']; + $oldGET = empty($_GET) ? [] : $_GET; + $oldREQUEST = empty($_REQUEST) ? [] : $_REQUEST; + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_GET['caseid'] = $case_id; + $_REQUEST['caseid'] = $case_id; + $_GET['cid'] = $client_id; + $_REQUEST['cid'] = $client_id; + $_GET['action'] = 'add'; + $_REQUEST['action'] = 'add'; + $_GET['reset'] = 1; + $_REQUEST['reset'] = 1; + $_GET['atype'] = $atype; + $_REQUEST['atype'] = $atype; + + $form = new CRM_Case_Form_Activity(); + $form->controller = new CRM_Core_Controller_Simple('CRM_Case_Form_Activity', 'Case Activity'); + $form->_activityTypeId = $atype; + $form->_activityTypeName = 'Change Case Status'; + $form->_activityTypeFile = 'ChangeCaseStatus'; + + $form->preProcess(); + $form->buildQuickForm(); + $form->setDefaultValues(); + + // Now submit the form. Store the date used so we can check it later. + + $t = time(); + $now_date = date('Y-m-d H:i:s', $t); + $now_date_date_only = date('Y-m-d', $t); + $actParams = [ + 'is_unittest' => TRUE, + 'case_status_id' => $closed_status, + '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 + ]; + + $form->postProcess($actParams); + + // Ok now let's check some things + + $result = $this->callAPISuccess('Case', 'get', [ + 'sequential' => 1, + 'id' => $case_id, + ]); + $caseData = array_shift($result['values']); + + $this->assertEquals($caseData['end_date'], $now_date_date_only); + $this->assertEquals($caseData['status_id'], $closed_status); + + // now get the latest activity and check some things for it + + $actId = max($caseData['activities']); + $this->assertNotEmpty($actId); + + $result = $this->callAPISuccess('Activity', 'get', [ + 'sequential' => 1, + 'id' => $actId, + ]); + $activity = array_shift($result['values']); + + $this->assertEquals($activity['subject'], 'Case status changed from Ongoing to Resolved'); + $this->assertEquals($activity['activity_date_time'], $now_date); + $this->assertEquals($activity['status_id'], $completed_status); + + // Now replace old globals + if (is_null($oldMETHOD)) { + unset($_SERVER['REQUEST_METHOD']); + } + else { + $_SERVER['REQUEST_METHOD'] = $oldMETHOD; + } + $_GET = $oldGET; + $_REQUEST = $oldREQUEST; + } + } -- 2.25.1