dev/core#1405 Strip any spaces from Option Group name parameter and r… #15937

seamuslee001 · 2019-11-23T00:36:30Z

…emove the name field from front end form as will be auto generated

Overview

This removes the name field from the form to add a new option group as really it should be a calculated field. It also Ensures that option values are fixed on upgrade

Before

Option group names can be created with a space in it causing errors when trying to add new option valuse

After

All option group names are lower case and for new option groups created primarily on the basis of the title of the option group.

Agileware Ref: CIVICRM-1373

ping @eileenmcnaughton @colemanw @jusfreeman

civibot · 2019-11-23T00:36:32Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

jusfreeman · 2019-11-25T00:54:02Z

Fantastic, thanks @seamuslee001 will test and report back here.

jusfreeman · 2019-11-25T00:55:11Z

Fixes this issue, https://lab.civicrm.org/dev/core/issues/1405

@seamuslee001 thanks also for including the Agileware Ref made it much easier to find the related issues. Legend!

jusfreeman

Hey @seamuslee001 - I think the main thing here is that the option group name should be lower case, not preserve mixed case from the title.

As a side comment, I really don't understand why CRM_Utils_String::titleToVar doesn't do the case conversion as that would save work here and possibly elsewhere too.

jusfreeman · 2019-11-25T05:45:14Z

CRM/Admin/Form/OptionGroup.php

+  public static function formRule($fields, $files, $self) {
+    $errors = [];
+    if ($self->_id) {
+      $name = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_OptionGrou', $self->_id, 'name');


Typo here.

Should be:
$name = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_OptionGroup', $self->_id, 'name');

jusfreeman · 2019-11-25T05:55:18Z

CRM/Core/BAO/OptionGroup.php

@@ -76,6 +76,12 @@ 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($params['title']);


Needs a strtolower here, otherwise Option Group Title: "Didgeridoo Kakapo" is saved as Option Group Name: "Didgeridoo_Kakapo" but should be "didgeridoo_kakapo"

$params['name'] = CRM_Utils_String::titleToVar(strtolower($params['title']));

jusfreeman · 2019-11-25T05:56:55Z

CRM/Core/BAO/OptionGroup.php

+      $params['name'] = CRM_Utils_String::titleToVar($params['title']);
+    }
+    elseif (!empty($params['name']) && strpos($params['name'], ' ')) {
+      $params['name'] = CRM_Utils_String::titleToVar($params['name']);


Also convert to lowercase for any existing mixed case Option Group Names.

$params['name'] = CRM_Utils_String::titleToVar(strtolower($params['name']));

jusfreeman · 2019-11-25T06:00:44Z

CRM/Admin/Form/OptionGroup.php

+      $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 already exists in the database. Option Group Names need to be unique');


Include the name in the error message so that user knows what value is conflicting, otherwise it's not clear. Shorter message too.

$errors['title'] = ts('Option Group name: ' . $name .' already exists. Option Group Names need to be unique');

jusfreeman · 2019-11-25T06:04:03Z

CRM/Upgrade/Incremental/php/FiveTwentyOne.php

+    foreach ($optionGroups as $optionGroup) {
+      $name = trim($optionGroup['name']);
+      if (strpos($name, ' ') !== FALSE) {
+        $fixedName = CRM_Utils_String::titleToVar($name);


Convert to lowercase here as well during the upgrade.

$fixedName = CRM_Utils_String::titleToVar(strtolower($name));

Probably should at least log what option group names have been changed so that at least CiviCRM developers are forewarned after the upgrade that potentially any custom integration referring to the option group by name will now be broken. Just a suggestion.

jusfreeman · 2019-11-25T06:05:00Z

tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php

+   */
+  public function testFixOptionGroupName() {
+    $name = 'This is a test Name';
+    $fixedName = CRM_Utils_String::titleToVar($name);


Need a strtolower here too.

jusfreeman · 2019-11-25T06:05:52Z

CRM/Core/BAO/OptionGroup.php

+    }
+    elseif (!empty($params['name']) && strpos($params['name'], ' ')) {
+      $params['name'] = CRM_Utils_String::titleToVar($params['name']);
+    }
    $optionGroup = new CRM_Core_DAO_OptionGroup();
    $optionGroup->copyValues($params);;


Typo with double ;;

jusfreeman · 2019-11-25T06:06:26Z

tests/phpunit/CRM/Core/BAO/OptionGroupTest.php

@@ -44,7 +44,7 @@ public function testEnsureOptionGroupExistsNewValue() {

    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));


Should be "bombed_again" - lower case.

jusfreeman

Changes requested

mattwire · 2019-11-28T03:23:21Z

@seamuslee001 Just wanted to check that this won't stop you manually setting name via API / xml option group import?

seamuslee001 · 2019-11-28T03:27:43Z

I don't think so but it would make it without spaces tho.

mattwire · 2019-11-28T03:28:40Z

I don't think so but it would make it without spaces tho.

Ok, that seems fine to me then

…emove 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

seamuslee001 · 2019-11-30T01:42:55Z

ok @jusfreeman have made those changes now

jusfreeman

@seamuslee001 looks good to me!

jusfreeman · 2019-12-02T05:30:22Z

#merge-on-pass

seamuslee001 · 2019-12-02T05:49:22Z

Merging as per review from @jusfreeman

civibot bot added the master label Nov 23, 2019

seamuslee001 force-pushed the dev_core_1405 branch 2 times, most recently from 5bd1bff to 5fbf3d3 Compare November 23, 2019 02:50

jusfreeman suggested changes Nov 25, 2019

View reviewed changes

jusfreeman suggested changes Nov 27, 2019

View reviewed changes

dev/core#1405 Strip any spaces from Option Group name parameter and r…

6ebc7a8

…emove 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

seamuslee001 force-pushed the dev_core_1405 branch from 5fbf3d3 to 6ebc7a8 Compare November 30, 2019 01:39

jusfreeman mentioned this pull request Dec 1, 2019

Code style, remove typo, double ;; from code where it makes sense to do so #16001

Merged

jusfreeman approved these changes Dec 1, 2019

View reviewed changes

seamuslee001 merged commit 7daf31d into civicrm:master Dec 2, 2019

seamuslee001 deleted the dev_core_1405 branch December 2, 2019 05:49

This was referenced Dec 14, 2019

dev/core#1477 Fix fatal error when editing reserved option group settings #16106

Closed

dev/core#1477 Fix fatal error when editing reserved option group settings #16107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#1405 Strip any spaces from Option Group name parameter and r… #15937

dev/core#1405 Strip any spaces from Option Group name parameter and r… #15937

seamuslee001 commented Nov 23, 2019 •

edited

Loading

civibot bot commented Nov 23, 2019

jusfreeman commented Nov 25, 2019

jusfreeman commented Nov 25, 2019

jusfreeman left a comment

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman Nov 25, 2019

jusfreeman left a comment

mattwire commented Nov 28, 2019

seamuslee001 commented Nov 28, 2019

mattwire commented Nov 28, 2019

seamuslee001 commented Nov 30, 2019

jusfreeman left a comment

jusfreeman commented Dec 2, 2019

seamuslee001 commented Dec 2, 2019

dev/core#1405 Strip any spaces from Option Group name parameter and r… #15937

dev/core#1405 Strip any spaces from Option Group name parameter and r… #15937

Conversation

seamuslee001 commented Nov 23, 2019 • edited Loading

Overview

Before

After

civibot bot commented Nov 23, 2019

jusfreeman commented Nov 25, 2019

jusfreeman commented Nov 25, 2019

jusfreeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jusfreeman left a comment

Choose a reason for hiding this comment

mattwire commented Nov 28, 2019

seamuslee001 commented Nov 28, 2019

mattwire commented Nov 28, 2019

seamuslee001 commented Nov 30, 2019

jusfreeman left a comment

Choose a reason for hiding this comment

jusfreeman commented Dec 2, 2019

seamuslee001 commented Dec 2, 2019

seamuslee001 commented Nov 23, 2019 •

edited

Loading