fix singleton warning url
authorDemeritCowboy <demeritcowboy@hotmail.com>
Mon, 15 Jul 2019 13:00:13 +0000 (09:00 -0400)
committerDemeritCowboy <demeritcowboy@hotmail.com>
Mon, 15 Jul 2019 15:01:43 +0000 (11:01 -0400)
CRM/Case/Form/Activity.php
CRM/Case/PseudoConstant.php
tests/phpunit/CRM/Case/BAO/CaseTest.php

index 1d225d6b1439d61d92e94d950b482c349bf2391a..f2927e11a696433a37e6c12e026dc5d86f7381c3 100644 (file)
@@ -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 <a %1>edit the existing activity</a>?", [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 <a %1>edit the existing activity</a>?", [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;
+  }
+
 }
index 7cc943acfa58a61c075c8f4d274fc191b5d02425..a1bb966de8f85f15c4e79046851351dc716abb98 100644 (file)
  */
 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];
   }
 
   /**
index fae0799d69994e8b9642c9ed261db7c142623d77..c9a24392a97bd0802c61147f529156ddd7d3de1d 100644 (file)
@@ -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);
+  }
+
 }