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

Contact tokens - use metadata & standardised rendering #21761

Merged
merged 1 commit into from
Oct 10, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 7, 2021

Overview

Contact tokens - use metadata & standardised rendering.

This fixes a temporary regression (master only during refactor) where the contact tokens that were being advertised were not getting their labels translated.

It also gets us to the point where the contact tokens are being rendered based on metadata for all fields - meaning that expectations about formatting money & dates is consistent with other token entities and it uses the parent functions more.

The underlying issue is that the contact token class is using the bao query object mechanism to retrieve the contact & just treating that as the token contract - which means its a bit random. The goal is that we would have contact tokens that
reflect the token names elsewhere - which closely reflect apiv4.

However, since there are still some persisiting code paths that use legacy mechanisms to replace contact tokens I didn't want to make significant changes to the tokens we advertise in case they are being presented on a screen that does not support them.

Before

The mapping between apiv3 & v4 fields is opaque

After

The mapping is explicitly declared.

Minor changes are made to the advertised fields
https://github.com/civicrm/civicrm-core/pull/21761/files#diff-c3c60600f080b49f2a9029b82865098c2a2742fbcedce0ecd731376917d80e1bL513-L597

Of those

  • the ones where 'label' is added will still be handled fine through the legacy replaceContactTokens method (which strips the 'label' before resolving)
  • {contact.current_employer_id}' is now advertised as {contact.employer_id}. The old version still works but is not advertised.
    The new version may not work for flows using the old code path - but note that it would be an unusual token to use with 'current_employer` being more prevalent
  • I hated 'manual_geocode' as a token - it was too irrational even for me - I removed it

But the old fields still work with very minor output changes (ie 'on_hold' now renders to 0 not FALSE and birth_date no longer shows the time as 'midnight')

https://github.com/civicrm/civicrm-core/pull/21761/files#diff-c3c60600f080b49f2a9029b82865098c2a2742fbcedce0ecd731376917d80e1bR790-R848

Technical Details

While I wouldn't call the code 'elegant' this concludes the cleanup with consistent rendering of tokens, no calls to the BAO query object and the code being generally 'in the right place' - ie contacts have their own tokens class rather than being in 'token compat subscriber'

Comments

@civibot
Copy link

civibot bot commented Oct 7, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton changed the title Contact tokens metadata wip Contact tokens - use metadata & standardised rendering Oct 9, 2021
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton force-pushed the contact branch 6 times, most recently from 9457204 to 90cac86 Compare October 10, 2021 04:34
@@ -605,7 +600,10 @@ public function getActiveTokens(TokenValueEvent $e) {
* @param array $exposedFields
* @param string $prefix
*/
protected function addFieldToTokenMetadata(array $field, array $exposedFields, $prefix = ''): void {
protected function addFieldToTokenMetadata(array $field, array $exposedFields, string $prefix = ''): void {
if ($field['type'] !== 'Custom' && !in_array($field['name'], $exposedFields, TRUE)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

early return is just for clarity / marginal performance

@eileenmcnaughton eileenmcnaughton force-pushed the contact branch 2 times, most recently from d8ac287 to a721690 Compare October 10, 2021 04:50
@eileenmcnaughton
Copy link
Contributor Author

@colemanw @totten so this is the last piece for this round - passing now

@totten
Copy link
Member

totten commented Oct 10, 2021

I like that this splits getOldContactTokens() and getAllContactTokens(). To my eye, this test-coverage is the main thing demonstrating the safety of the change.

The only thing to point out is the list of changes from the description:

Of those

  • the ones where 'label' is added will still be handled fine through the legacy replaceContactTokens method (which strips the 'label' before resolving)
  • {contact.current_employer_id}' is now advertised as {contact.employer_id}. The old version still works but is not advertised.
    The new version may not work for flows using the old code path - but note that it would be an unusual token to use with 'current_employer` being more prevalent
  • I hated 'manual_geocode' as a token - it was too irrational even for me - I removed it

This needs to be propagated to the documentation where it summarizes token changes, right? That can be done async, of course.

@totten totten merged commit 1a10d7e into civicrm:master Oct 10, 2021
@eileenmcnaughton eileenmcnaughton deleted the contact branch October 10, 2021 21:10
@magnolia61
Copy link
Contributor

Hi @eileenmcnaughton
I found a problem in the 5.43 RC with contact core tokens not rendering in the body but most do render in the subject.
https://lab.civicrm.org/dev/core/-/issues/2907

@magnolia61
Copy link
Contributor

It seems the changes in this pr cause the issue

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 I just tried again to replicate - very simple attempt where I did a search for 50 participants & had a body of just {contact.first_name} and used send email. I understood that wouldn't work from your report but it did

@magnolia61
Copy link
Contributor

I think I found it: https://lab.civicrm.org/dev/core/-/issues/2907

@eileenmcnaughton
Copy link
Contributor Author

See #21812

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