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

Reference token metadata internally fix custom tokens label bug #21767

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 7, 2021

Overview

Reference token metadata internally. This also fixes a metadata issue @magnolia61 identified where the metadata for custom fields was 'falling through' to the metadata for 'all fields' - resulting in 🏷️ variants being incorrectly added

Before

Data is stored in token metadata - it may ALSO be stored as field metadata. When we access via field metadata if will use getFields to fetch if not present

After

Referring to the token metadata array allows us to avoid an extra getfields & is a step towards only using that look up to build the token metadata

Technical Details

This references the token metadata rather than the metadata retrieved from the api
call. Since the former is cached it should save a look up -
albeit not when prefetch is happening, at this stage

Comments

@colemanw you only just merged it & I changed it :-) If this passes would be good to get it merged as #21761 builds on it

@civibot
Copy link

civibot bot commented Oct 7, 2021

(Standard links)

@civibot civibot bot added the master label Oct 7, 2021
This references the token metadata rather than the metadata retrieved from the api
call. Since the former is cached it should save a look up -
albeit not when prefetch is happening, at this stage
@eileenmcnaughton eileenmcnaughton changed the title Reference token metadata internally Reference token metadata internally fix custom tokens label bug Oct 7, 2021
@colemanw
Copy link
Member

colemanw commented Oct 8, 2021

Merging as this is rapidly evolving code and tests are passing.

@colemanw colemanw merged commit 7be60c2 into civicrm:master Oct 8, 2021
@colemanw colemanw deleted the meta branch October 8, 2021 02:48
@eileenmcnaughton
Copy link
Contributor Author

thanks @colemanw - I'll do more QA during rc too & @magnolia61 has also been doing a lot of testing

@magnolia61
Copy link
Contributor

magnolia61 commented Oct 8, 2021

@eileenmcnaughton I tested and can confirm this solves the problem. Also solved some radio yes/no fields to not show up in the list.
And also takes care of the custom_n:label field (when it should be without label) not to crash the reminder but to render blank.

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 thanks for re-testing!

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.

3 participants