dev/core#155 Fix optiongroup is_reserved data and use when selecting option group for custom fields #12423

mattwire · 2018-07-05T13:35:35Z

Overview

As the is_reserved flag is not being shown in the UI, and is not editable from the UI it has not been used for anything in core even though it is there.

A common use-case is adding an option group and then adding a set of custom fields that use that option group (eg. for surveys) but that is currently not possible via the UI because the query that looks for available option groups is looking ONLY for option groups that are already linked to custom fields - a catch 22!

Before

Not possible to create optiongroups via the UI that can be used with custom fields.

After

Option groups that are created via the UI can be selected for use with custom fields.

Technical Details

The is_reserved flag has always existed in the database but has not been used and has not been set correctly in some cases.

Comments

This fixes a common UI workflow where you need to create a set of options for use by a set of custom fields (for multiple choice questions/forms/surveys eg. forms used to calculate health scores etc.).

Currently you have to create the set of options along with the first custom field and you have no control over it's name. Currently, only when created in this way can you select that option group for use by other custom fields.

civibot · 2018-07-05T13:35:37Z

(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).

colemanw

I think this makes sense, provided that we really do think the is_reserved flag should be used for this purpose.

In addition to my code nitpicks, if option groups are getting saved as reserved from the custom field screen, need to fix that as well.

colemanw · 2018-07-10T23:12:48Z

CRM/Upgrade/Incremental/sql/5.5.alpha1.mysql.tpl

+UPDATE civicrm_option_group AS cog INNER JOIN civicrm_custom_field AS ccf
+ON cog.id = ccf.option_group_id
+SET cog.is_reserved = 0 WHERE cog.is_active = 1 AND ccf.is_active = 1;
+UPDATE civicrm_option_group SET is_reserved = 1 WHERE name='environment';


The upgrade code does not affect new installs, so the environment group needs to be set to reserved in the install sql as well.

@colemanw I've updated the install sql as well now.

colemanw · 2018-07-10T23:15:26Z

CRM/Custom/Form/Field.php

+    $optionGroupParams = array(
+      'is_reserved' => 0,
+      'is_active' => 1,
+      'options' => array('limit' => 0, 'sort' => "title ASC"),


Set a return param here for efficiency if all you need is the title.

colemanw · 2018-07-10T23:16:06Z

CRM/Custom/Form/Field.php

+      $emptyOptGroup = FALSE;
+      foreach ($optionGroupMetadata['values'] as $id => $metadata) {
+        $optionGroups[$id] = $metadata['title'];
+      }


Consider using CRM_Utils_Array::collect() instead of a loop.

Nice! I didn't know about that function.

… of only ones that are already linked to custom fields

mattwire · 2018-07-11T12:05:05Z

@colemanw Thankyou very much for your review. I've made the requested changes. Could you confirm if you are happy from a code perspective now?

colemanw · 2018-07-11T13:18:23Z

This is shaping up nicely. However we still need to change the custom field form postSave to create a non-reserved group when saving a custom field with a new set of options.

Also I'm a little worried about non-reserved groups floating around in the system. Some extensions create option groups and they probably shouldn't be tampered with but extension authors may not have known to set them to reserved (I just checked and the api defaults to not reserved when creating a group). I guess we just post a memo to the dev channel about this and suggest extensions add an upgrade routine to reserve their option groups.

mattwire · 2018-07-14T14:13:22Z

This is shaping up nicely. However we still need to change the custom field form postSave to create a non-reserved group when saving a custom field with a new set of options.

@colemanw That is now changed.

mattwire · 2018-07-14T14:14:22Z

I guess we just post a memo to the dev channel about this and suggest extensions add an upgrade routine to reserve their option groups.

The whole thing re is_reserved was a bit of a mess before. Once we are happy for this to be merged I'll post a message to the dev list noting which version it will be in.

colemanw · 2018-07-14T21:37:24Z

I haven't had a chance to test this out yet but the code looks good and I agree with the concept.

eileenmcnaughton · 2018-07-16T00:39:27Z

I tested the upgrade script and that I can now chose an unreserved option group. @colemanw has approved the concept so merging

colemanw · 2018-08-22T20:48:12Z

@eileenmcnaughton @mattwire This PR, specifically commit c0fce6b has broken the custom field edit form.
To reproduce:

On a stock demo site, go to the custom fields screen.
Click on "View and Edit Fields" for Constituent Information
Click "Edit Field" next to "Most Important Issue"
The options will appear garbled, and you will be unable to save.

I think this is a critical regression and needs to be fixed immediately, but I'm hesitant to just revert this PR because it has an upgrade script. @mattwire - thoughts?

seamuslee001 · 2018-08-22T21:21:03Z

@colemanw i have opened a partial revert here #12717 and put that against 5.5. I think the changes in the CustomGroup file are fine and same with the xml and the upgrade step given they were tested

…12423

Fix for issue editing custom fields with option groups after #12423

…12423

civicrm-builder added the master label Jul 5, 2018

mattwire changed the title ~~Optiongroup isreserved~~ dev/core#155 Fix optiongroup is_reserved data and use when selecting option group for custom fields Jul 5, 2018

colemanw requested changes Jul 10, 2018

View reviewed changes

mattwire force-pushed the optiongroup_isreserved branch 2 times, most recently from 79cb9b0 to 91c29f6 Compare July 11, 2018 12:00

Switch to API and get all option groups that are not reserved instead…

c0fce6b

… of only ones that are already linked to custom fields

mattwire force-pushed the optiongroup_isreserved branch from 91c29f6 to 56196ac Compare July 11, 2018 12:01

Fix is_reserved flag on option groups

bd8a24d

mattwire force-pushed the optiongroup_isreserved branch from 56196ac to bd8a24d Compare July 14, 2018 14:12

mattwire mentioned this pull request Jul 14, 2018

dev/core#259 OptionGroup Admin UI workflow improvements #12473

Merged

colemanw approved these changes Jul 14, 2018

View reviewed changes

eileenmcnaughton merged commit cd761cf into civicrm:master Jul 16, 2018

eileenmcnaughton added the sig:extension support Supports being able to integrate extensions with CiviCRM label Jul 16, 2018

seamuslee001 mentioned this pull request Aug 22, 2018

Partial revert of PR 12423 to fix the display of the edit custom fiel… #12717

Closed

mattwire added a commit to mattwire/civicrm-core that referenced this pull request Aug 22, 2018

Fix for issue editing custom fields with option groups after civicrm#…

2a8d615

…12423

mattwire mentioned this pull request Aug 22, 2018

Fix for issue editing custom fields with option groups after #12423 #12718

Merged

mattwire added a commit to mattwire/civicrm-core that referenced this pull request Aug 22, 2018

Fix for issue editing custom fields with option groups after civicrm#…

99d187d

…12423

mattwire added a commit to mattwire/civicrm-core that referenced this pull request Aug 22, 2018

Fix for issue editing custom fields with option groups after civicrm#…

f46ffe8

…12423

monishdeb added a commit that referenced this pull request Aug 24, 2018

Merge pull request #12718 from mattwire/custom_field_optiongroup_fix

5bc1d29

Fix for issue editing custom fields with option groups after #12423

mattwire mentioned this pull request Aug 24, 2018

Further fix for updating custom fields linked to optiongroups. #12729

Merged

mattwire added a commit to mattwire/civicrm-core that referenced this pull request Aug 24, 2018

Fix for issue editing custom fields with option groups after civicrm#…

915c1f4

…12423

mattwire deleted the optiongroup_isreserved branch September 25, 2018 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#155 Fix optiongroup is_reserved data and use when selecting option group for custom fields #12423

dev/core#155 Fix optiongroup is_reserved data and use when selecting option group for custom fields #12423

mattwire commented Jul 5, 2018

civibot bot commented Jul 5, 2018

colemanw left a comment

colemanw Jul 10, 2018

mattwire Jul 11, 2018

colemanw Jul 10, 2018

mattwire Jul 11, 2018

colemanw Jul 10, 2018

mattwire Jul 11, 2018

mattwire commented Jul 11, 2018

colemanw commented Jul 11, 2018

mattwire commented Jul 14, 2018

mattwire commented Jul 14, 2018

colemanw commented Jul 14, 2018

eileenmcnaughton commented Jul 16, 2018

colemanw commented Aug 22, 2018

seamuslee001 commented Aug 22, 2018

dev/core#155 Fix optiongroup is_reserved data and use when selecting option group for custom fields #12423

dev/core#155 Fix optiongroup is_reserved data and use when selecting option group for custom fields #12423

Conversation

mattwire commented Jul 5, 2018

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 5, 2018

colemanw left a comment

Choose a reason for hiding this comment

colemanw Jul 10, 2018

Choose a reason for hiding this comment

mattwire Jul 11, 2018

Choose a reason for hiding this comment

colemanw Jul 10, 2018

Choose a reason for hiding this comment

mattwire Jul 11, 2018

Choose a reason for hiding this comment

colemanw Jul 10, 2018

Choose a reason for hiding this comment

mattwire Jul 11, 2018

Choose a reason for hiding this comment

mattwire commented Jul 11, 2018

colemanw commented Jul 11, 2018

mattwire commented Jul 14, 2018

mattwire commented Jul 14, 2018

colemanw commented Jul 14, 2018

eileenmcnaughton commented Jul 16, 2018

colemanw commented Aug 22, 2018

seamuslee001 commented Aug 22, 2018