From ddcc28ae4c8829f62b8c1e5d49478d7eb6b2a6d5 Mon Sep 17 00:00:00 2001 From: colemanw Date: Mon, 29 May 2023 16:19:30 -0400 Subject: [PATCH] APIv4 - Fix Activity contact virtual fields to work across joins --- .../Spec/Provider/ActivitySpecProvider.php | 21 ++- Civi/Test/Api4TestTrait.php | 4 + tests/phpunit/api/v4/Entity/ActivityTest.php | 132 ++++++------------ 3 files changed, 56 insertions(+), 101 deletions(-) diff --git a/Civi/Api4/Service/Spec/Provider/ActivitySpecProvider.php b/Civi/Api4/Service/Spec/Provider/ActivitySpecProvider.php index 5eb148517d..19933e820f 100644 --- a/Civi/Api4/Service/Spec/Provider/ActivitySpecProvider.php +++ b/Civi/Api4/Service/Spec/Provider/ActivitySpecProvider.php @@ -52,6 +52,7 @@ class ActivitySpecProvider extends \Civi\Core\Service\AutoService implements Gen $field = new FieldSpec('source_contact_id', 'Activity', 'Integer'); $field->setTitle(ts('Source Contact')); $field->setLabel(ts('Added by')); + $field->setColumnName('id'); $field->setDescription(ts('Contact who created this activity.')); $field->setRequired($action === 'create'); $field->setFkEntity('Contact'); @@ -62,23 +63,25 @@ class ActivitySpecProvider extends \Civi\Core\Service\AutoService implements Gen $field = new FieldSpec('target_contact_id', 'Activity', 'Array'); $field->setTitle(ts('Target Contacts')); $field->setLabel(ts('With Contacts')); + $field->setColumnName('id'); $field->setDescription(ts('Contacts involved in this activity.')); $field->setFkEntity('Contact'); $field->setInputType('EntityRef'); $field->setInputAttrs(['multiple' => TRUE]); + $field->setSerialize(\CRM_Core_DAO::SERIALIZE_COMMA); $field->setSqlRenderer([__CLASS__, 'renderSqlForActivityContactIds']); - $field->addOutputFormatter([__CLASS__, 'formatOutputForMultipleActivityContactIds']); $spec->addFieldSpec($field); $field = new FieldSpec('assignee_contact_id', 'Activity', 'Array'); $field->setTitle(ts('Assignee Contacts')); $field->setLabel(ts('Assigned to')); + $field->setColumnName('id'); $field->setDescription(ts('Contacts assigned to this activity.')); $field->setFkEntity('Contact'); $field->setInputType('EntityRef'); $field->setInputAttrs(['multiple' => TRUE]); + $field->setSerialize(\CRM_Core_DAO::SERIALIZE_COMMA); $field->setSqlRenderer([__CLASS__, 'renderSqlForActivityContactIds']); - $field->addOutputFormatter([__CLASS__, 'formatOutputForMultipleActivityContactIds']); $spec->addFieldSpec($field); } } @@ -100,16 +103,10 @@ class ActivitySpecProvider extends \Civi\Core\Service\AutoService implements Gen 'CRM_Activity_BAO_ActivityContact', 'record_type_id', $contactLinkTypes[$field['name']]); - return '(SELECT GROUP_CONCAT(`civicrm_activity_contact`.`contact_id`) ' - . 'FROM `civicrm_activity_contact` ' - . 'WHERE `civicrm_activity_contact`.`activity_id` = `a`.`id` ' - . 'AND record_type_id = ' . $recordTypeId . ')'; - } - - public static function formatOutputForMultipleActivityContactIds( - ?string &$value, array $row, array $field - ): void { - $value = explode(',', $value ?? ''); + return "(SELECT GROUP_CONCAT(`civicrm_activity_contact`.`contact_id`) + FROM `civicrm_activity_contact` + WHERE `civicrm_activity_contact`.`activity_id` = {$field['sql_name']} + AND record_type_id = $recordTypeId)"; } } diff --git a/Civi/Test/Api4TestTrait.php b/Civi/Test/Api4TestTrait.php index 1f9b9e2ba7..774c0f80ef 100644 --- a/Civi/Test/Api4TestTrait.php +++ b/Civi/Test/Api4TestTrait.php @@ -66,6 +66,10 @@ trait Api4TestTrait { * @noinspection PhpUnhandledExceptionInspection */ public function saveTestRecords(string $entityName, array $saveParams): Result { + // Shortcut for creating a bunch of records + if (is_int($saveParams['records'])) { + $saveParams['records'] = array_fill(0, $saveParams['records'], []); + } $saveParams += [ 'checkPermissions' => FALSE, 'defaults' => [], diff --git a/tests/phpunit/api/v4/Entity/ActivityTest.php b/tests/phpunit/api/v4/Entity/ActivityTest.php index 2c504d849b..80529ed00a 100644 --- a/tests/phpunit/api/v4/Entity/ActivityTest.php +++ b/tests/phpunit/api/v4/Entity/ActivityTest.php @@ -21,118 +21,72 @@ namespace api\v4\Entity; use Civi\Api4\Contact; use api\v4\Api4TestBase; use Civi\Api4\Activity; -use Civi\Api4\ActivityContact; use Civi\Test\TransactionalInterface; /** - * Assert that updating an activity does not affect the targets. - * - * This test was written specifically to test - * https://lab.civicrm.org/dev/core/-/issues/1428 - * * @group headless */ class ActivityTest extends Api4TestBase implements TransactionalInterface { - public function testActivityUpdate() { + public function testActivityContactVirtualFields() { + $c = $this->saveTestRecords('Contact', ['records' => 5])->column('id'); - $meetingActivityTypeID = \Civi\Api4\OptionValue::get() - ->addSelect('value') - ->addWhere('option_group_id:name', '=', 'activity_type') - ->addWhere('name', '=', 'Meeting') - ->execute()->first()['value']; + $sourceContactId = $c[2]; + $targetContactIds = [$c[0], $c[1]]; + $assigneeContactIds = [$c[3], $c[4]]; - $domainContactID = \CRM_Core_BAO_Domain::getDomain()->contact_id; - $c1 = Contact::create(FALSE)->addValue('first_name', '1')->execute()->first()['id']; - $c2 = Contact::create(FALSE)->addValue('first_name', '2')->execute()->first()['id']; - - $activityID = Activity::create(FALSE) - ->setValues([ - 'target_contact_id' => [$c1], - 'assignee_contact_id' => [$c2], - 'activity_type_id' => $meetingActivityTypeID, - 'source_contact_id' => $domainContactID, - 'subject' => 'test activity', - ])->execute()->first()['id']; + // Test that we can write to and read from the virtual fields. + $activityID = $this->createTestRecord('Activity', [ + 'target_contact_id' => $targetContactIds, + 'source_contact_id' => $sourceContactId, + 'subject' => '1234', + ])['id']; - // Activity create does not return a full record, so get the ID then do another get call... $activity = Activity::get(FALSE) - ->addSelect('id', 'subject', 'activity_type_id') + ->addSelect('source_contact_id', 'target_contact_id', 'assignee_contact_id') ->addWhere('id', '=', $activityID) ->execute()->first(); - $this->assertEquals($meetingActivityTypeID, $activity['activity_type_id']); - $this->assertEquals('test activity', $activity['subject']); - - // Now check we have the correct target and assignees. - $activityContacts = ActivityContact::get(FALSE) - ->addWhere('activity_id', '=', $activityID) - ->execute() - ->indexBy('contact_id')->column('record_type_id'); - - // 1 is assignee - // 2 is added - // 3 is target/with - $expectedActivityContacts = [$c1 => 3, $c2 => 1, $domainContactID => 2]; - ksort($expectedActivityContacts); - ksort($activityContacts); - $this->assertEquals($expectedActivityContacts, $activityContacts, "ActivityContacts not as expected."); + $this->assertEquals($sourceContactId, $activity['source_contact_id']); + $this->assertEquals($targetContactIds, $activity['target_contact_id']); + // This field was not set + $this->assertNull($activity['assignee_contact_id']); - // Test we can update the activity. + // Update to set assignee_contact_id Activity::update(FALSE) ->addWhere('id', '=', $activityID) - ->addValue('subject', 'updated subject') + ->addValue('assignee_contact_id', $assigneeContactIds) ->execute(); - // Repeat the tests. + // Affirm that assignee_contact_id was set and other fields remain unchanged $activity = Activity::get(FALSE) - ->addSelect('id', 'subject', 'activity_type_id') + ->addSelect('source_contact_id', 'target_contact_id', 'assignee_contact_id') ->addWhere('id', '=', $activityID) - ->execute()->first(); - $this->assertEquals($meetingActivityTypeID, $activity['activity_type_id']); - $this->assertEquals('updated subject', $activity['subject'], "Activity subject was not updated correctly by Activity::update."); - - // Now check we have the correct target and assignees. - $activityContacts = ActivityContact::get(FALSE) - ->addWhere('activity_id', '=', $activityID) - ->execute() - ->indexBy('contact_id')->column('record_type_id'); - ksort($activityContacts); - $this->assertEquals($expectedActivityContacts, $activityContacts, "ActivityContacts not as expected after update."); - } - - public function testActivityCreateAndGetVirtualFields() { - $meetingActivityTypeID = \Civi\Api4\OptionValue::get() - ->addSelect('value') - ->addWhere('option_group_id:name', '=', 'activity_type') - ->execute()->first()['value']; - - $sourceContactId = \CRM_Core_BAO_Domain::getDomain()->contact_id; - $c1 = Contact::create(FALSE)->addValue('first_name', '1')->execute()->first()['id']; - $c2 = Contact::create(FALSE)->addValue('first_name', '2')->execute()->first()['id']; - $c3 = Contact::create(FALSE)->addValue('first_name', '3')->execute()->first()['id']; - $c4 = Contact::create(FALSE)->addValue('first_name', '4')->execute()->first()['id']; - - $targetContactIds = [$c1, $c2]; - $assigneeContactIds = [$c3, $c4]; - - // Test that we can write to and read from the virtual fields. - $activityID = Activity::create(FALSE) - ->setValues([ - 'target_contact_id' => $targetContactIds, - 'assignee_contact_id' => $assigneeContactIds, - 'activity_type_id' => $meetingActivityTypeID, - 'source_contact_id' => $sourceContactId, - 'subject' => 'test activity', - ])->execute()->first()['id']; - - $activity = Activity::get(FALSE) - ->addSelect('id', 'source_contact_id', 'target_contact_id', 'assignee_contact_id') - ->addWhere('id', '=', $activityID) - ->execute()->first(); - + ->execute()->single(); $this->assertEquals($sourceContactId, $activity['source_contact_id']); $this->assertEquals($targetContactIds, $activity['target_contact_id']); $this->assertEquals($assigneeContactIds, $activity['assignee_contact_id']); + + // Sanity check for https://lab.civicrm.org/dev/core/-/issues/1428 + // Updating nothing should change nothing. + Activity::update(FALSE) + ->addWhere('id', '=', $activityID) + ->addValue('subject', '1234') + ->execute(); + + // Try fetching virtual fields via a join when Activity is not the primary entity + $contactGet = Contact::get(FALSE) + ->addSelect('activity.subject', 'activity.source_contact_id', 'activity.target_contact_id', 'activity.assignee_contact_id') + ->addWhere('id', '=', $sourceContactId) + ->addJoin('Activity AS activity', 'INNER', 'ActivityContact', + ['id', '=', 'activity.contact_id'], + ['activity.record_type_id:name', '=', '"Activity Source"'] + ) + ->execute()->single(); + $this->assertEquals('1234', $contactGet['activity.subject']); + $this->assertEquals($sourceContactId, $contactGet['activity.source_contact_id']); + $this->assertEquals($targetContactIds, $contactGet['activity.target_contact_id']); + $this->assertEquals($assigneeContactIds, $contactGet['activity.assignee_contact_id']); + } } -- 2.25.1