Fix 'sleeper-bug' in api.

It turns out that the Activity.create BAO
- deletes the existing source contact for an activity if the param 'source_contact_id' is set
- deletes existing assignee contacts for an activity if the param 'assignee_contact_id' is empty
- deletes existing target contacts for an activity if the param 'target_contact_id' is empty

The v3 api provides some fairly convoluted wrapping to effectively convert is empty
to isset (via 2 params deleteActivityAssignment, deleteActivityTarget which would only
be anything other than true if the relevant value key is not set or we are operating from
the one place in universe that passes in this param (well actually it's true then too
https://github.com/eileenmcnaughton/civicrm_entity/commit/bd779c19bbf82ca22260615b4f7667d02eee1200
)

The v4 api does no handling and we can expect that on update activity contact records are
likely being lost - ditto with direct BAO calls that were probably QAd at some point but
may have changed over time.

This changes the BAO to treat the assignee & target params the same way as source
- ie do nothing if not set, but empty means to delete them

Remove support for api param deleteActivityAssignment, deleteActivityTarget

I did a universe search & it was only called from one place which
I fixed
https://github.com/eileenmcnaughton/civicrm_entity/commit/bd779c19bbf82ca22260615b4f7667d02eee1200

- it's never been a tested param so technically not supported

This is part of my effort to fix redundant queries in Activity.create regarding
activity contacts

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index 1e097c151c602c06a168dd5fa257a7359af47f2d..ad656ea90650daf860d1a87e3c6fa467e77a0b95 100644 (file)
--- a/CRM/Activity/BAO/Activity.php
+++ b/CRM/Activity/BAO/Activity.php
@@ -368,10 +368,8 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
       $assignmentParams = ['activity_id' => $activityId];
 
       if (is_array($params['assignee_contact_id'])) {
-        if (CRM_Utils_Array::value('deleteActivityAssignment', $params, TRUE)) {
-          // first delete existing assignments if any
-          self::deleteActivityContact($activityId, $assigneeID);
-        }
+        // first delete existing assignments if any
+        self::deleteActivityContact($activityId, $assigneeID);
 
         foreach ($params['assignee_contact_id'] as $acID) {
           if ($acID) {
@@ -403,10 +401,8 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
         }
       }
     }
-    else {
-      if (CRM_Utils_Array::value('deleteActivityAssignment', $params, TRUE)) {
-        self::deleteActivityContact($activityId, $assigneeID);
-      }
+    elseif (isset($params['assignee_contact_id'])) {
+      self::deleteActivityContact($activityId, $assigneeID);
     }
 
     if (is_a($resultAssignment, 'CRM_Core_Error')) {
@@ -421,10 +417,8 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
       $targetParams = ['activity_id' => $activityId];
       $resultTarget = [];
       if (is_array($params['target_contact_id'])) {
-        if (CRM_Utils_Array::value('deleteActivityTarget', $params, TRUE)) {
-          // first delete existing targets if any
-          self::deleteActivityContact($activityId, $targetID);
-        }
+        // first delete existing targets if any
+        self::deleteActivityContact($activityId, $targetID);
 
         foreach ($params['target_contact_id'] as $tid) {
           if ($tid) {
@@ -456,10 +450,8 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
         }
       }
     }
-    else {
-      if (CRM_Utils_Array::value('deleteActivityTarget', $params, TRUE)) {
-        self::deleteActivityContact($activityId, $targetID);
-      }
+    elseif (isset($params['target_contact_id'])) {
+      self::deleteActivityContact($activityId, $targetID);
     }
 
     // write to changelog before transaction is committed/rolled

diff --git a/Civi/Test/ContactTestTrait.php b/Civi/Test/ContactTestTrait.php
index 26991ea8cfdf44cf4135a97122fea5785e9fc8f0..065c6bccb7e6289aaaaac010a87f6ef02e7643af 100644 (file)
--- a/Civi/Test/ContactTestTrait.php
+++ b/Civi/Test/ContactTestTrait.php
@@ -74,7 +74,7 @@ trait ContactTestTrait {
    * @return int
    *   id of Individual created
    *
-   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public function individualCreate($params = [], $seq = 0, $random = FALSE) {
     $params = array_merge($this->sampleContact('Individual', $seq, $random), $params);

diff --git a/api/v3/Activity.php b/api/v3/Activity.php
index 8898fb22155d6391452d7d0407fbcddfe4078809..422ebff5905d98b3345d749f4f96a67eed860c4f 100644 (file)
--- a/api/v3/Activity.php
+++ b/api/v3/Activity.php
@@ -95,20 +95,6 @@ function civicrm_api3_activity_create($params) {
     }
   }
 
-  $deleteActivityAssignment = FALSE;
-  if (isset($params['assignee_contact_id'])) {
-    $deleteActivityAssignment = TRUE;
-  }
-
-  $deleteActivityTarget = FALSE;
-  if (isset($params['target_contact_id'])) {
-    $deleteActivityTarget = TRUE;
-  }
-
-  // this should all be handled at the BAO layer
-  $params['deleteActivityAssignment'] = CRM_Utils_Array::value('deleteActivityAssignment', $params, $deleteActivityAssignment);
-  $params['deleteActivityTarget'] = CRM_Utils_Array::value('deleteActivityTarget', $params, $deleteActivityTarget);
-
   if ($case_id && $createRevision) {
     // This is very similar to the copy-to-case action.
     if (!CRM_Utils_Array::crmIsEmptyArray($oldActivityValues['target_contact'])) {

diff --git a/tests/phpunit/api/v3/ActivityTest.php b/tests/phpunit/api/v3/ActivityTest.php
index 86e7d23062f850852c8ebb3de7a9b35c1db49273..2ab72b6b38aea61824e8943f3c8b99a76abda3ae 100644 (file)
--- a/tests/phpunit/api/v3/ActivityTest.php
+++ b/tests/phpunit/api/v3/ActivityTest.php
@@ -370,8 +370,10 @@ class api_v3_ActivityTest extends CiviUnitTestCase {
 
   /**
    * Test get returns target and assignee contacts.
+   *
    * @dataProvider versionThreeAndFour
    * @var int $version
+   * @throws \CRM_Core_Exception
    */
   public function testActivityReturnTargetAssignee($version) {
     $this->_apiversion = $version;
@@ -1010,50 +1012,69 @@ class api_v3_ActivityTest extends CiviUnitTestCase {
   }
 
   /**
-   * Test civicrm_activity_update() to update an existing activity
+   * Test api updates an existing activity, including removing activity contacts in apiv3 style.
    */
   public function testActivityUpdate() {
     $result = $this->callAPISuccess('activity', 'create', $this->_params);
-    $this->_contactID2 = $this->individualCreate();
 
     $params = [
       'id' => $result['id'],
       'subject' => 'Make-it-Happen Meeting',
-      'activity_date_time' => '20091011123456',
+      'activity_date_time' => '2009-10-11 12:34:56',
       'duration' => 120,
       'location' => '21, Park Avenue',
       'details' => 'Lets update Meeting',
       'status_id' => 1,
       'source_contact_id' => $this->_contactID,
-      'assignee_contact_id' => $this->_contactID2,
+      'assignee_contact_id' => $this->individualCreate(),
+      'target_contact_id' => $this->individualCreate(),
       'priority_id' => 1,
     ];
 
     $result = $this->callAPISuccess('activity', 'create', $params);
-    //hack on date comparison - really we should make getAndCheck smarter to handle dates
-    $params['activity_date_time'] = '2009-10-11 12:34:56';
+
     // we also unset source_contact_id since it is stored in an aux table
     unset($params['source_contact_id']);
-    //Check if assignee created.
-    $assignee = $this->callAPISuccess('ActivityContact', 'get', [
+    //Check if assignee & target created.
+    $this->callAPISuccessGetCount('ActivityContact', [
+      'activity_id' => $result['id'],
+      'record_type_id' => ['IN' => ['Activity Assignees', 'Activity Targets']],
+    ], 2);
+
+    // Replace the existing 1 per type with 3 per type.
+    $this->callAPISuccess('Activity', 'create', [
+      'id' => $result['id'],
+      'assignee_contact_id' => [$this->individualCreate(), $this->individualCreate(), $this->individualCreate()],
+      'target_contact_id' => [$this->individualCreate(), $this->individualCreate(), $this->individualCreate()],
+    ]);
+
+    //Check if assignee & target created.
+    $this->callAPISuccessGetCount('ActivityContact', [
       'activity_id' => $result['id'],
-      'return' => ["contact_id"],
-      'record_type_id' => "Activity Assignees",
+      'record_type_id' => ['IN' => ['Activity Assignees', 'Activity Targets']],
+    ], 6);
+
+    // Do a blank update - no deletes?
+    $this->callAPISuccess('Activity', 'create', [
+      'id' => $result['id'],
     ]);
-    $this->assertNotEmpty($assignee['values']);
+    $this->callAPISuccessGetCount('ActivityContact', [
+      'activity_id' => $result['id'],
+      'record_type_id' => ['IN' => ['Activity Assignees', 'Activity Targets']],
+    ], 6);
 
     //clear assignee contacts.
-    $updateParams = [
+    $this->callAPISuccess('Activity', 'create', [
       'id' => $result['id'],
       'assignee_contact_id' => [],
-    ];
-    $activity = $this->callAPISuccess('activity', 'create', $updateParams);
-    $assignee = $this->callAPISuccess('ActivityContact', 'get', [
-      'activity_id' => $activity['id'],
-      'return' => ["contact_id"],
-      'record_type_id' => "Activity Assignees",
+      'target_contact_id' => [],
     ]);
-    $this->assertEmpty($assignee['values']);
+    $this->callAPISuccessGetCount('ActivityContact', [
+      'activity_id' => $result['id'],
+      'record_type_id' => ['IN' => ['Activity Assignees', 'Activity Targets']],
+    ], 0);
+
+    unset($params['source_contact_id']);
     $this->getAndCheck($params, $result['id'], 'activity');
   }
 
@@ -1284,8 +1305,8 @@ class api_v3_ActivityTest extends CiviUnitTestCase {
       'priority_id' => 1,
     ];
 
-    $result = $this->callAPISuccess('activity', 'create', $params);
-    $findactivity = $this->callAPISuccess('Activity', 'Get', ['id' => $activity['id']]);
+    $this->callAPISuccess('Activity', 'create', $params);
+    $this->callAPISuccessGetSingle('Activity', ['id' => $activity['id']]);
   }
 
   /**