From 2bd8b0289cfcf6934601b88ba179b23cacf82ade Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 23 Sep 2020 14:22:07 +1200 Subject: [PATCH] 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 --- CRM/Activity/BAO/Activity.php | 24 ++++------ Civi/Test/ContactTestTrait.php | 2 +- api/v3/Activity.php | 14 ------ tests/phpunit/api/v3/ActivityTest.php | 63 ++++++++++++++++++--------- 4 files changed, 51 insertions(+), 52 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 1e097c151c..ad656ea906 100644 --- 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 26991ea8cf..065c6bccb7 100644 --- 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 8898fb2215..422ebff590 100644 --- 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 86e7d23062..2ab72b6b38 100644 --- 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']]); } /** -- 2.25.1