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'