From 5b7cc7239ce2ddea5eb883f2dcdd8ad2f761f19c Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Mon, 27 Feb 2023 10:19:53 -0500 Subject: [PATCH] Note API - Fix joining to and updating Notes Multiple bugfixes: 1. Could not join to Notes from APIv4 because it would "helpfully" insert a contact_id clause in the join 2. Creating a note with no contact_id was defaulting to the contact the note was about rather than the logged-in user 3. Updating a note was clobbering the contact_id and the privacy fields 4. Note ACLs were being applied incorrectly to the Note.contact_id field --- CRM/Core/BAO/Note.php | 33 +++++++++------- CRM/Core/DAO.php | 4 +- Civi/Api4/Query/Api4SelectQuery.php | 5 +++ .../phpunit/api/v3/SyntaxConformanceTest.php | 1 - tests/phpunit/api/v4/Entity/NoteTest.php | 39 +++++++++++++++++++ 5 files changed, 65 insertions(+), 17 deletions(-) diff --git a/CRM/Core/BAO/Note.php b/CRM/Core/BAO/Note.php index b8abef965f..1385235c01 100644 --- a/CRM/Core/BAO/Note.php +++ b/CRM/Core/BAO/Note.php @@ -105,8 +105,7 @@ class CRM_Core_BAO_Note extends CRM_Core_DAO_Note implements \Civi\Core\HookInte * (reference) an assoc array of name/value pairs. * @param array $ids * (deprecated) associated array with note id - preferably set $params['id']. - * @return null|object - * $note CRM_Core_BAO_Note object + * @return CRM_Core_BAO_Note|null * @throws \CRM_Core_Exception */ public static function add(&$params, $ids = []) { @@ -121,23 +120,28 @@ class CRM_Core_BAO_Note extends CRM_Core_DAO_Note implements \Civi\Core\HookInte } } - $note = new CRM_Core_BAO_Note(); - - if (!isset($params['privacy'])) { - $params['privacy'] = 0; - } + $loggedInContactID = CRM_Core_Session::getLoggedInContactID(); - $note->copyValues($params); - if (empty($params['contact_id'])) { - if (CRM_Utils_Array::value('entity_table', $params) == 'civicrm_contact') { - $note->contact_id = $params['entity_id']; - } - } $id = $params['id'] ?? $ids['id'] ?? NULL; + + // Ugly legacy support for deprecated $ids param if ($id) { - $note->id = $id; + $params['id'] = $id; + } + // Set defaults for create mode + else { + $params += [ + 'privacy' => 0, + 'contact_id' => $loggedInContactID, + ]; + // If not created by a user, guess the note was self-created + if (empty($params['contact_id']) && $params['entity_table'] == 'civicrm_contact') { + $params['contact_id'] = $params['entity_id']; + } } + $note = new CRM_Core_BAO_Note(); + $note->copyValues($params); $note->save(); // check and attach and files as needed @@ -152,7 +156,6 @@ class CRM_Core_BAO_Note extends CRM_Core_DAO_Note implements \Civi\Core\HookInte $noteActions = FALSE; - $loggedInContactID = CRM_Core_Session::getLoggedInContactID(); if ($loggedInContactID) { if ($loggedInContactID == $note->entity_id) { $noteActions = TRUE; diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index b8c672a596..934b1a7db3 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -3118,9 +3118,11 @@ SELECT contact_id public function addSelectWhereClause() { $clauses = []; $fields = $this->fields(); + // Notes should check permissions on the entity_id field, not the contact_id field + $skipContactCheckFor = ['Note']; foreach ($fields as $fieldName => $field) { // Clause for contact-related entities like Email, Relationship, etc. - if (strpos($field['name'], 'contact_id') === 0 && CRM_Utils_Array::value('FKClassName', $field) == 'CRM_Contact_DAO_Contact') { + if (strpos($field['name'], 'contact_id') === 0 && !in_array($field['entity'], $skipContactCheckFor) && CRM_Utils_Array::value('FKClassName', $field) == 'CRM_Contact_DAO_Contact') { $contactClause = CRM_Utils_SQL::mergeSubquery('Contact'); if (!empty($contactClause)) { $clauses[$field['name']] = $contactClause; diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php index 284afe6bbd..7a523e09c2 100644 --- a/Civi/Api4/Query/Api4SelectQuery.php +++ b/Civi/Api4/Query/Api4SelectQuery.php @@ -829,9 +829,14 @@ class Api4SelectQuery { return FALSE; } foreach ([$sideA, $sideB] as $expr) { + // Check for explicit link to FK entity if ($expr === "$alias.id" || !empty($joinEntityFields[str_replace("$alias.", '', $expr)]['fk_entity'])) { return TRUE; } + // Check for dynamic FK + if ($expr === "$alias.entity_id") { + return TRUE; + } } return FALSE; }); diff --git a/tests/phpunit/api/v3/SyntaxConformanceTest.php b/tests/phpunit/api/v3/SyntaxConformanceTest.php index 4dbb0434da..4b5c9d84ee 100644 --- a/tests/phpunit/api/v3/SyntaxConformanceTest.php +++ b/tests/phpunit/api/v3/SyntaxConformanceTest.php @@ -503,7 +503,6 @@ class api_v3_SyntaxConformanceTest extends CiviUnitTestCase { 'UFJoin', 'Relationship', 'RelationshipType', - 'Note', 'Membership', 'Group', 'File', diff --git a/tests/phpunit/api/v4/Entity/NoteTest.php b/tests/phpunit/api/v4/Entity/NoteTest.php index 31bdacfb33..f1fcd08f23 100644 --- a/tests/phpunit/api/v4/Entity/NoteTest.php +++ b/tests/phpunit/api/v4/Entity/NoteTest.php @@ -19,6 +19,7 @@ namespace api\v4\Entity; use api\v4\Api4TestBase; +use Civi\Api4\Contact; use Civi\Api4\Note; use Civi\Test\TransactionalInterface; @@ -74,4 +75,42 @@ class NoteTest extends Api4TestBase implements TransactionalInterface { $this->assertCount(1, $existing); } + public function testJoinNotesFromContact() { + $userId = $this->createLoggedInUser(); + $c1 = $this->createTestRecord('Contact'); + $c2 = $this->createTestRecord('Contact'); + + // Create 2 notes for $c1 and 1 for $c2. + $notes = Note::save(FALSE) + ->setRecords([ + ['note' => 'Note1', 'entity_id' => $c1['id']], + ['note' => 'Note2', 'entity_id' => $c1['id']], + ['note' => 'Note3', 'entity_id' => $c2['id']], + ]) + ->setDefaults([ + 'entity_id' => $c1['id'], + 'entity_table' => 'civicrm_contact', + ])->execute(); + + $results = Contact::get(FALSE) + ->addWhere('id', 'IN', [$c1['id'], $c2['id']]) + ->addOrderBy('id') + ->addJoin('Note AS Contact_Note', + 'LEFT', + ['id', '=', 'Contact_Note.entity_id'], + ['Contact_Note.entity_table', '=', '"civicrm_contact"'] + ) + ->addSelect('id', 'Contact_Note.note', 'Contact_Note.contact_id') + ->execute()->indexBy('Contact_Note.note'); + + $this->assertCount(3, $results); + $this->assertEquals($c1['id'], $results['Note1']['id']); + $this->assertEquals($c1['id'], $results['Note2']['id']); + $this->assertEquals($c2['id'], $results['Note3']['id']); + // Note creator should have been set to current user + $this->assertEquals($userId, $results['Note1']['Contact_Note.contact_id']); + $this->assertEquals($userId, $results['Note2']['Contact_Note.contact_id']); + $this->assertEquals($userId, $results['Note3']['Contact_Note.contact_id']); + } + } -- 2.25.1