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

Additional event tokens #26216

Merged
merged 1 commit into from
May 17, 2023
Merged

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 14, 2023

Overview

Additional event tokens

Before

We don't have tokens to deal with the notices in
https://lab.civicrm.org/dev/core/-/issues/4287

After

Tokens added - note that Ifollowed deprecated the existing {event.contact_phone} & added a bunch more rather than {event.loc_block_id.phone_id.phone} which would be more correct. I'm on the fence here about switching over (& deprecating the existing format)

Technical Details

Comments

@civibot
Copy link

civibot bot commented May 14, 2023

(Standard links)

@civibot civibot bot added the master label May 14, 2023
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton force-pushed the event_tokens branch 2 times, most recently from 739998e to da8499d Compare May 14, 2023 21:29
$tokens['contact_email']['text/html'] = $event['loc_block_id.email_id.email'];
$tokens['confirm_email_text']['text/plain'] = $event['confirm_email_text'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why these 3 are text/plain rather than text/html?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can kind of now looking at the ManageEvent get confirm_email_text and pay_later_text as those are text areas but pay_later_receipt is a wysiwyg https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Form/ManageEvent/Fee.php#L265 so should be text/html IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah great that you should ask ... there is!

If the value is stored as text, which this is, then specifying text/plain here means it gets converted to html in the token processor if rendering in html format.

@eileenmcnaughton eileenmcnaughton force-pushed the event_tokens branch 5 times, most recently from c81526d to d3db19a Compare May 15, 2023 02:12
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 I just pushed a style fix but this is otherwise working

The test changes will conflict with other open PRs so would be good to get this MOPed & I can rebase

image

@eileenmcnaughton eileenmcnaughton force-pushed the event_tokens branch 3 times, most recently from f25b456 to 1f3444a Compare May 15, 2023 13:32
'type' => 'calculated',
'options' => NULL,
'data_type' => 'String',
'audience' => 'sysadmin',
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a good reason why email 1 is user and email 2 is sysadmin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it's just that people get upset if we put too many tokens in the drop down cos it gets cluttered / easy to pick the wrong one

'title' => ts('Event Contact Email'),
'name' => 'contact_email',
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to put an upgrade status message or something about these tokens not working now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they should still work cost they are in the deprecated mapping

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 is this good to merge now?

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