Merge pull request #15937 from seamuslee001/dev_core_1405

dev/core#1405 Strip any spaces from Option Group name parameter and r…
civicrm · Dec 2, 2019 · 7daf31d · 7daf31d
2 parents 5e128ce + 6ebc7a8
commit 7daf31d
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 19 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -393,15 +393,15 @@ 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
     $ov = $this->callAPISuccess('option_value', 'create',
       ['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'