Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Cleanup on CRM_Admin_Form_Options #24828

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[REF] Cleanup on CRM_Admin_Form_Options

Before

I coulldn't figure out what the code was doing & why

After

  • Form Field contactOptions renamed to contact_type_id - which is almost correct - it has a (hard-coded, I didn't change that) list of contact types to which it adds the mysterious ' "Multiple Contact Merge". Despite this extra it 'mostly' means 'contact_type_id'
  • less use of the mysterious $this->_gName - as with most abbreviations the meaning of this is clear right up until the end of the coding session at which point it becomes hard to decipher.
  • simplification around the validation of the submitted greeting - understanding this was where I started. I no longer call the cached function because for an admin form reducing complexity is more important that hitting the cache. I decided that all that was happening was it was ensuring that there were not 2 records for the same greeting + contact type + label & I r-run tested to check that didn't happen
    image
  • separating the check into a function allows sensible levels of code commenting
  • I determined the magic last merge option is not actually used for email_greetings so I removed from there

image

Technical Details

Comments

@civibot
Copy link

civibot bot commented Oct 26, 2022

(Standard links)

@civibot civibot bot added the master label Oct 26, 2022
@colemanw colemanw merged commit 437469f into civicrm:master Oct 28, 2022
@colemanw colemanw deleted the options_form branch October 28, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants