From 6ebc7a890065ce16986559445416561b3530699d Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Sat, 23 Nov 2019 11:18:34 +1100 Subject: [PATCH] dev/core#1405 Strip any spaces from Option Group name parameter and remove the name field from front end form as will be auto generated Ensure that option group names are lower csae Ensure that option group names are created as lower case and fix as per Justin's review --- CRM/Admin/Form/OptionGroup.php | 38 +++++++++++++------ CRM/Core/BAO/OptionGroup.php | 9 +++++ CRM/Upgrade/Incremental/php/FiveTwentyOne.php | 31 +++++++++++++-- .../phpunit/CRM/Core/BAO/OptionGroupTest.php | 4 +- .../CRM/Upgrade/Incremental/BaseTest.php | 23 +++++++++++ tests/phpunit/api/v3/OptionGroupTest.php | 2 +- tests/phpunit/api/v3/OptionValueTest.php | 4 +- 7 files changed, 92 insertions(+), 19 deletions(-) diff --git a/CRM/Admin/Form/OptionGroup.php b/CRM/Admin/Form/OptionGroup.php index 062be56ce9df..9d89d74f013e 100644 --- a/CRM/Admin/Form/OptionGroup.php +++ b/CRM/Admin/Form/OptionGroup.php @@ -38,17 +38,6 @@ public function buildQuickForm() { CRM_Utils_System::setTitle(ts('Dropdown Options')); $this->applyFilter('__ALL__', 'trim'); - $this->add('text', - 'name', - ts('Name'), - CRM_Core_DAO::getAttribute('CRM_Core_DAO_OptionGroup', 'name'), - TRUE - ); - $this->addRule('name', - ts('Name already exists in Database.'), - 'objectExists', - ['CRM_Core_DAO_OptionGroup', $this->_id] - ); $this->add('text', 'title', @@ -90,6 +79,33 @@ public function buildQuickForm() { } $this->assign('id', $this->_id); + $this->addFormRule(['CRM_Admin_Form_OptionGroup', 'formRule'], $this); + } + + /** + * Global form rule. + * + * @param array $fields + * The input form values. + * + * @param $files + * @param $self + * + * @return bool|array + * true if no errors, else array of errors + */ + public static function formRule($fields, $files, $self) { + $errors = []; + if ($self->_id) { + $name = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_OptionGroup', $self->_id, 'name'); + } + else { + $name = CRM_Utils_String::titleToVar(strtolower($fields['title'])); + } + if (!CRM_Core_DAO::objectExists($name, 'CRM_Core_DAO_OptionGroup', $self->_id)) { + $errors['title'] = ts('Option Group name ' . $name . ' already exists in the database. Option Group Names need to be unique'); + } + return empty($errors) ? TRUE : $errors; } /** diff --git a/CRM/Core/BAO/OptionGroup.php b/CRM/Core/BAO/OptionGroup.php index 48911dc89803..efa75688b9d1 100644 --- a/CRM/Core/BAO/OptionGroup.php +++ b/CRM/Core/BAO/OptionGroup.php @@ -76,6 +76,15 @@ public static function add(&$params, $ids = []) { CRM_Core_Error::deprecatedFunctionWarning('no $ids array'); $params['id'] = $ids['optionGroup']; } + if (empty($params['name']) && empty($params['id'])) { + $params['name'] = CRM_Utils_String::titleToVar(strtolower($params['title'])); + } + elseif (!empty($params['name']) && strpos($params['name'], ' ')) { + $params['name'] = CRM_Utils_String::titleToVar(strtolower($params['name'])); + } + elseif (!empty($params['name'])) { + $params['name'] = strtolower($params['name']); + } $optionGroup = new CRM_Core_DAO_OptionGroup(); $optionGroup->copyValues($params);; $optionGroup->save(); diff --git a/CRM/Upgrade/Incremental/php/FiveTwentyOne.php b/CRM/Upgrade/Incremental/php/FiveTwentyOne.php index 894cdd780c08..6cbffc39d9a4 100644 --- a/CRM/Upgrade/Incremental/php/FiveTwentyOne.php +++ b/CRM/Upgrade/Incremental/php/FiveTwentyOne.php @@ -65,8 +65,33 @@ public function setPostUpgradeMessage(&$postUpgradeMessage, $rev) { // // The above is an exception because 'Upgrade DB to %1: SQL' is generic & reusable. // } - // public static function taskFoo(CRM_Queue_TaskContext $ctx, ...) { - // return TRUE; - // } + /** + * Upgrade function. + * + * @param string $rev + */ + public function upgrade_5_21_alpha1($rev) { + $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev); + $this->addTask('dev/core#1405 Fix option group names that contain spaces', 'fixOptionGroupName'); + } + + public static function fixOptionGroupName() { + $optionGroups = \Civi\Api4\OptionGroup::get() + ->setCheckPermissions(FALSE) + ->execute(); + foreach ($optionGroups as $optionGroup) { + $name = trim($optionGroup['name']); + if (strpos($name, ' ') !== FALSE) { + $fixedName = CRM_Utils_String::titleToVar(strtolower($name)); + \Civi::log()->debug('5.21 Upgrade Option Group name ' . $name . ' converted to ' . $fixedName); + \Civi\Api4\OptionGroup::update() + ->addWhere('id', '=', $optionGroup['id']) + ->addValue('name', $fixedName) + ->setCheckPermissions(FALSE) + ->execute(); + } + } + return TRUE; + } } diff --git a/tests/phpunit/CRM/Core/BAO/OptionGroupTest.php b/tests/phpunit/CRM/Core/BAO/OptionGroupTest.php index 260b2d35850d..1672fa495e2c 100644 --- a/tests/phpunit/CRM/Core/BAO/OptionGroupTest.php +++ b/tests/phpunit/CRM/Core/BAO/OptionGroupTest.php @@ -40,11 +40,11 @@ public function testEnsureOptionGroupExistsExistingValue() { public function testEnsureOptionGroupExistsNewValue() { CRM_Core_BAO_OptionGroup::ensureOptionGroupExists(['name' => 'Bombed']); $optionGroups = $this->callAPISuccess('OptionValue', 'getoptions', ['field' => 'option_group_id'])['values']; - $this->assertTrue(in_array('Bombed', $optionGroups)); + $this->assertTrue(in_array('bombed', $optionGroups)); CRM_Core_BAO_OptionGroup::ensureOptionGroupExists(['name' => 'Bombed Again']); $optionGroups = $this->callAPISuccess('OptionValue', 'getoptions', ['field' => 'option_group_id'])['values']; - $this->assertTrue(in_array('Bombed Again', $optionGroups)); + $this->assertTrue(in_array('bombed_again', $optionGroups)); } } diff --git a/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php b/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php index ffe2b28056bd..9510e4f77d8c 100644 --- a/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php +++ b/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php @@ -467,4 +467,27 @@ public function testConvertUpgradeContributeSettings() { $this->assertEquals(1, Civi::settings()->get('deferred_revenue_enabled')); } + /** + * dev/core#1405 Test fixing option groups with spaces in the name + */ + public function testFixOptionGroupName() { + $name = 'This is a test Name'; + $fixedName = CRM_Utils_String::titleToVar(strtolower($name)); + $optionGroup = $this->callAPISuccess('OptionGroup', 'create', [ + 'title' => 'Test Option Group', + 'name' => $name, + ]); + // API is hardened to strip the spaces to lets re-add in now + CRM_Core_DAO::executeQuery("UPDATE civicrm_option_group SET name = %1 WHERE id = %2", [ + 1 => [$name, 'String'], + 2 => [$optionGroup['id'], 'Positive'], + ]); + $preUpgrade = $this->callAPISuccess('OptionGroup', 'getsingle', ['id' => $optionGroup['id']]); + $this->assertEquals($name, $preUpgrade['name']); + CRM_Upgrade_Incremental_php_FiveTwentyOne::fixOptionGroupName(); + $postUpgrade = $this->callAPISuccess('OptionGroup', 'getsingle', ['id' => $optionGroup['id']]); + $this->assertEquals($fixedName, $postUpgrade['name'], 'Ensure that the spaces have been removed from OptionGroup name'); + $this->assertEquals($postUpgrade['name'], $optionGroup['values'][$optionGroup['id']]['name'], 'Ensure that the fixed name matches what the API would produce'); + } + } diff --git a/tests/phpunit/api/v3/OptionGroupTest.php b/tests/phpunit/api/v3/OptionGroupTest.php index 1fbd6546c1c2..1a5272c2c2e4 100644 --- a/tests/phpunit/api/v3/OptionGroupTest.php +++ b/tests/phpunit/api/v3/OptionGroupTest.php @@ -94,7 +94,7 @@ public function testGetOptionCreateSuccess() { public function testGetOptionCreateFailOnDuplicate() { $params = [ 'sequential' => 1, - 'name' => 'civicrm_dup entry', + 'name' => 'civicrm_dup_entry', 'is_reserved' => 1, 'is_active' => 1, ]; diff --git a/tests/phpunit/api/v3/OptionValueTest.php b/tests/phpunit/api/v3/OptionValueTest.php index cdc24455d407..623296a559d9 100644 --- a/tests/phpunit/api/v3/OptionValueTest.php +++ b/tests/phpunit/api/v3/OptionValueTest.php @@ -393,7 +393,7 @@ public function testCreateOptionValueWithSameValueLanguagesException() { public function testCreateOptionValueWithSameValueDiffOptionGroup() { $og = $this->callAPISuccess('option_group', 'create', [ - 'name' => 'our test Option Group for with group id', + 'name' => 'our test Option Group', 'is_active' => 1, ]); // create a option value @@ -401,7 +401,7 @@ public function testCreateOptionValueWithSameValueDiffOptionGroup() { ['option_group_id' => $og['id'], 'label' => 'test option value'] ); $og2 = $this->callAPISuccess('option_group', 'create', [ - 'name' => 'our test Option Group for with group id 2', + 'name' => 'our test Option Group 2', 'is_active' => 1, ]); // update option value without 'option_group_id'