(dev/core#594) Fix key-value mappings for WYSIWYG setting (editor_id). Accept keyColumn option.

[seamuslee001]

Overview

This fixes an issue where by the editor_id setting does not work properly because the javascript and other parts are expecting the string of the name of the Editor whereas in 719eda4#diff-45cac86e3687ecb3e1996985223d2a31 it was changed to keyed on values because that is what is the default for settings pseduoconstants

Before

• Saving the editor_id in the GUI leads to an invalid setting.
• In the metadata used for Settings.getoptions in APIv3, one can set an optionGroupName. The key-value mappings are implicitly based on value=>label.

After

• Saving the editor_id in the GUI leads to a valid setting.
• In the metadata used for Settings.getoptions in APIv3, one can set an optionGroupName. By default, the key-value mappings are based on value=>label (as before), but one may optionally customize with the keyColumn. This parallels the XML Schema convention that shapes most generic *.getoptions calls.

ping @mattwire @monishdeb @andrewpthompson @MegaphoneJon

[civibot]

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

[totten]

(CiviCRM Review Template WORD-1.2)

• General standards
  • (r-explain):
    • Issue: This does more than just change editor_id handling -- it expands the metadata format for pseudoconstant (to allow a new subkey keyColumn). That should be explained somewhere. (Resolved by updating PR description.)
    • Question: What (if any) steps are required for folks upgrading? Perhaps there's some handful of folks who need to resave the form?
  • (r-user) Pass
  • (r-doc) Pass
  • (r-run) Undecided: Haven't run it yet
• Developer standards
  • (r-tech) Pass:
    • The main thing going on is that pseudoconstant can now include a keyColumn; the default value, though, is chosen to match the previous hard-coded value. 👍
    • Tangentially, I did a grep to make sure that keyColumn wasn't already used in the metadata. It doesn't appear anywhere in universe/*/settings (incl core's settings/), so that seems safe. 👍
  • (r-code) Pass
  • (r-maint) Pass
  • (r-test) Pass

[seamuslee001]

@totten not sure but i used the similar principle as we have in the DAOs https://docs.civicrm.org/dev/en/latest/framework/database/schema-definition/#table-field-pseudoconstant

[totten]

@seamuslee001 Cool, that makes sense -- so it's basically adapting a convention from the DAO/fields metadata to also work in the settings metadata. I'll just update the description so that it reflects that.

[seamuslee001]

@totten i'm working on a post upgrade message now. In terms of running it i would check out the test site -> go to the display preferences -> set the editor to be ckeditor -> make sure the configure button is visible -> then after saving the page check a page with a text editor e.g. manage contribution page and check the ckeditor loads.

[seamuslee001]

@totten added the post upgrade message now

[andrewpthompson]

@seamuslee001 I've tested this patch and it works as expected.

[seamuslee001]

@totten if your happy with the upgrade message i think this is good for merge @monishdeb

[totten]

Thanks @andrewpthompson - that covers the last r-run criterion. 👍

@seamuslee001, I did a little copy-editing on the message. Merge-on-pass.

[seamuslee001]

thanks @totten

[seamuslee001]

Merging as per the tag