From eba57f7d2b24b26c968ce81cfadf9d35f875182b Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Tue, 18 Jan 2022 21:45:33 -0500 Subject: [PATCH] DAO - Centralize logic to derive unique name from label This function checks db indices to enforce uniqueness of "name" column when creating a new record via the `DAO::writeRecord()` method. --- CRM/Contact/BAO/SavedSearch.php | 13 ----- CRM/Core/DAO.php | 48 ++++++++++++++++++- ext/search_kit/search_kit.php | 9 ---- .../v4/SearchDisplay/SearchDisplayTest.php | 41 ++++++++++++++++ 4 files changed, 87 insertions(+), 24 deletions(-) diff --git a/CRM/Contact/BAO/SavedSearch.php b/CRM/Contact/BAO/SavedSearch.php index c9141eaa56..cd45672464 100644 --- a/CRM/Contact/BAO/SavedSearch.php +++ b/CRM/Contact/BAO/SavedSearch.php @@ -277,19 +277,6 @@ WHERE $where"; * @return \CRM_Contact_DAO_SavedSearch */ public static function create(&$params) { - // Auto-create unique name from label if supplied - if (empty($params['id']) && empty($params['name']) && !empty($params['label'])) { - $name = CRM_Utils_String::munge($params['label']); - $existing = Civi\Api4\SavedSearch::get(FALSE) - ->addWhere('name', 'LIKE', $name . '%') - ->addSelect('name') - ->execute()->column('name'); - $suffix = ''; - while (in_array($name . $suffix, $existing)) { - $suffix = '_' . (1 + str_replace('_', '', $suffix)); - } - $params['name'] = $name . $suffix; - } $loggedInContactID = CRM_Core_Session::getLoggedInContactID(); if ($loggedInContactID) { if (empty($params['id'])) { diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 2d28ec7ffe..140e929cda 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -918,10 +918,14 @@ class CRM_Core_DAO extends DB_DataObject { $entityName = CRM_Core_DAO_AllCoreTables::getBriefName($className); \CRM_Utils_Hook::pre($op, $entityName, $record['id'] ?? NULL, $record); - $instance = new $className(); + $fields = static::getSupportedFields(); + $instance = new static(); // Ensure fields exist before attempting to write to them - $values = array_intersect_key($record, self::getSupportedFields()); + $values = array_intersect_key($record, $fields); $instance->copyValues($values); + if (empty($values['id']) && array_key_exists('name', $fields) && empty($values['name']) && !empty($values['label'])) { + $instance->makeNameFromLabel(); + } $instance->save(); if (!empty($record['custom']) && is_array($record['custom'])) { @@ -3279,4 +3283,44 @@ SELECT contact_id return static::$_paths ?? []; } + /** + * When creating a record without a supplied name, + * create a unique, clean name derived from the label. + * + * Note: this function does nothing unless a unique index exists for "name" column. + */ + private function makeNameFromLabel() { + $indexNameWith = NULL; + // Look for a unique index which includes the "name" field + if (method_exists($this, 'indices')) { + foreach ($this->indices(FALSE) as $index) { + if (!empty($index['unique']) && in_array('name', $index['field'], TRUE)) { + $indexNameWith = $index['field']; + } + } + } + if (!$indexNameWith) { + // No unique index on "name", do nothing + return; + } + $name = CRM_Utils_String::munge($this->label, '_', 252); + + // Find existing records with the same name + $sql = new CRM_Utils_SQL_Select($this::getTableName()); + $sql->select(['id', 'name']); + $sql->where('name LIKE @name', ['@name' => $name . '%']); + // Include all fields that are part of the index + foreach (array_diff($indexNameWith, ['name']) as $field) { + $sql->where("`$field` = @val", ['@val' => $this->$field]); + } + $query = $sql->toSQL(); + $existing = self::executeQuery($query)->fetchMap('id', 'name'); + $dupes = 0; + $suffix = ''; + while (in_array($name . $suffix, $existing)) { + $suffix = '_' . (++$dupes); + } + $this->name = $name . $suffix; + } + } diff --git a/ext/search_kit/search_kit.php b/ext/search_kit/search_kit.php index 24a0e76058..b3163c327a 100644 --- a/ext/search_kit/search_kit.php +++ b/ext/search_kit/search_kit.php @@ -83,15 +83,6 @@ function search_kit_civicrm_entityTypes(&$entityTypes) { * Implements hook_civicrm_pre(). */ function search_kit_civicrm_pre($op, $entity, $id, &$params) { - // Supply default name/label when creating new SearchDisplay - if ($entity === 'SearchDisplay' && $op === 'create') { - if (empty($params['label'])) { - $params['label'] = $params['name']; - } - elseif (empty($params['name'])) { - $params['name'] = \CRM_Utils_String::munge($params['label']); - } - } // When deleting a saved search, also delete the displays // This would happen anyway in sql because of the ON DELETE CASCADE foreign key, // But this ensures that pre and post hooks are called diff --git a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php index eebceeaee1..d56366c6ad 100644 --- a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php +++ b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php @@ -1,6 +1,7 @@ assertNull($display['saved_search_id']); } + public function testAutoFormatName() { + // Create 3 saved searches; they should all get unique names + $savedSearch0 = SavedSearch::create(FALSE) + ->addValue('label', 'My test search') + ->execute()->first(); + $savedSearch1 = SavedSearch::create(FALSE) + ->addValue('label', 'My test search') + ->execute()->first(); + $savedSearch2 = SavedSearch::create(FALSE) + ->addValue('label', 'My test search') + ->execute()->first(); + // Name will be created from munged label + $this->assertEquals('My_test_search', $savedSearch0['name']); + // Name will have _1, _2, etc. appended to ensure it's unique + $this->assertEquals('My_test_search_1', $savedSearch1['name']); + $this->assertEquals('My_test_search_2', $savedSearch2['name']); + + $display0 = SearchDisplay::create() + ->addValue('saved_search_id', $savedSearch0['id']) + ->addValue('label', 'My test display') + ->addValue('type', 'table') + ->execute()->first(); + $display1 = SearchDisplay::create() + ->addValue('saved_search_id', $savedSearch0['id']) + ->addValue('label', 'My test display') + ->addValue('type', 'table') + ->execute()->first(); + $display2 = SearchDisplay::create() + ->addValue('saved_search_id', $savedSearch1['id']) + ->addValue('label', 'My test display') + ->addValue('type', 'table') + ->execute()->first(); + // Name will be created from munged label + $this->assertEquals('My_test_display', $display0['name']); + // Name will have _1 appended to ensure it's unique to savedSearch0 + $this->assertEquals('My_test_display_1', $display1['name']); + // This is for a different saved search so doesn't need a number appended + $this->assertEquals('My_test_display', $display2['name']); + } + } -- 2.25.1