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

Enhance metadata for OptionGroups #25188

Merged
merged 2 commits into from
Jan 19, 2023
Merged

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Dec 16, 2022

Overview

Enhance metadata for OptionGroups

Extracted from #25180

Note that the URLs civicrm/admin/options/{edit,add,update} are created in other parts of that PR but will not adversely impact existing code.

@civibot
Copy link

civibot bot commented Dec 16, 2022

(Standard links)

@civibot civibot bot added the master label Dec 16, 2022
public static function getEditLink(array $field): string {
return "COALESCE(
(SELECT path FROM `civicrm_menu` WHERE path = CONCAT('civicrm/admin/options/', {$field['sql_name']})),
CONCAT('civicrm/admin/options/edit#/?gid=', `a`.`id`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aydun this is a very clever trick but I'm not entirely sold on the idea of these calc fields. Once we add something to the API there's an expectation that it will be supported in perpetuity, but this feels more like a temporary workaround to our messy menu situation. I feel like we just need to clean up the mess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @colemanw, it is a temporary workaround/hack. What if we add it as deprecated/hidden/your-assignment-is-to-eliminate-me?

We have various different functions being carried out by CRM_Admin_Form_Options and CRM_Admin_Page_Options accessed by various URL's and some URL's that look like they should be handled by them but are actually handled by other classes. I was looking for a way to let us slide in SK versions and retire the old stuff rather than undertake what looks like an awkward revision of it first.

If we were to clean the mess, what structure would you opt for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok @aydun I want to put some thought into that but have family visiting for the holidays which is not conducive to the same, but I will try soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem - I'm stopping in a day or so until New Year so no rush from me.

@colemanw
Copy link
Member

@aydun my proto-idea for how to tackle option groups is to actually embed the searchDisplay in a Smarty page instead of in an afform. That smarty page would preprocess similarly to the current one which extracts the option group id from the url, and then decides which columns the table needs hide depending on the shape of the option group, and sets the description of the search display based on the option group description.

@aydun
Copy link
Contributor Author

aydun commented Jan 12, 2023

@colemanw - as discussed, I've removed the calculated fields. Hopefully what's left is straightforward!

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@colemanw
Copy link
Member

@civicrm-builder add to whitelist

@colemanw
Copy link
Member

Thanks @aydun I've added the existing add/update paths. They might change in future but having them in metadata means we can update them without needing to update any search displays.

@colemanw colemanw merged commit 53990aa into civicrm:master Jan 19, 2023
@aydun
Copy link
Contributor Author

aydun commented Jan 19, 2023

Thanks @colemanw - makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants