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 062be56ce9..9d89d74f01 100644 --- a/CRM/Admin/Form/OptionGroup.php +++ b/CRM/Admin/Form/OptionGroup.php @@ -38,17 +38,6 @@ class CRM_Admin_Form_OptionGroup extends CRM_Admin_Form { 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 @@ class CRM_Admin_Form_OptionGroup extends CRM_Admin_Form { } $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 48911dc898..efa75688b9 100644 --- a/CRM/Core/BAO/OptionGroup.php +++ b/CRM/Core/BAO/OptionGroup.php @@ -76,6 +76,15 @@ class CRM_Core_BAO_OptionGroup extends CRM_Core_DAO_OptionGroup { 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 894cdd780c..6cbffc39d9 100644 --- a/CRM/Upgrade/Incremental/php/FiveTwentyOne.php +++ b/CRM/Upgrade/Incremental/php/FiveTwentyOne.php @@ -65,8 +65,33 @@ class CRM_Upgrade_Incremental_php_FiveTwentyOne extends CRM_Upgrade_Incremental_ // // 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 260b2d3585..1672fa495e 100644 --- a/tests/phpunit/CRM/Core/BAO/OptionGroupTest.php +++ b/tests/phpunit/CRM/Core/BAO/OptionGroupTest.php @@ -40,11 +40,11 @@ class CRM_Core_BAO_OptionGroupTest extends CiviUnitTestCase { 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 ffe2b28056..9510e4f77d 100644 --- a/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php +++ b/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php @@ -467,4 +467,27 @@ class CRM_Upgrade_Incremental_BaseTest extends CiviUnitTestCase { $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 1fbd6546c1..1a5272c2c2 100644 --- a/tests/phpunit/api/v3/OptionGroupTest.php +++ b/tests/phpunit/api/v3/OptionGroupTest.php @@ -94,7 +94,7 @@ class api_v3_OptionGroupTest extends CiviUnitTestCase { 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 cdc24455d4..623296a559 100644 --- a/tests/phpunit/api/v3/OptionValueTest.php +++ b/tests/phpunit/api/v3/OptionValueTest.php @@ -393,7 +393,7 @@ class api_v3_OptionValueTest extends CiviUnitTestCase { 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 @@ class api_v3_OptionValueTest extends CiviUnitTestCase { ['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' -- 2.25.1