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

Add userEnteredText as generic workflow template smarty variable #27162

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

This makes userEnteredText a property across all WorkflowMessages - with it outputting the smarty variable user_text

We already had receipt_text on the membership forms - but that is a bit too specific

  • user is perhaps a bit impersonal - I did contemplate form_text :

Before

What is the old user-interface or technical-contract (as appropriate)?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.

After

What changed? What is new old user-interface or technical-contract?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.

Technical Details

If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.

Comments

Anything else you would like the reviewer to note

@civibot
Copy link

civibot bot commented Aug 25, 2023

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers

protected $userEnteredText;

public function getUserEnteredText(): ?string {
return $this->userEnteredText ? htmlentities($this->userEnteredText): NULL;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if this is best - but we should probably do some form of escaping

Copy link
Contributor

Choose a reason for hiding this comment

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

This would apply to text templates too, right? So I don't think we can (unless there's something that decodes them for that somewhere).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand this as a generic thing that applies to all message types. It smells more like a reusable trait.

Re: escaping - the main thing is to be clear about the intended format: plain text or rich text (HTML)?

  • If content comes from an existing source, then it's an empirical question - what is that source? (If it's new content, then I guess it's discretionary - but still needs to be explicit.)
  • If the content is plain-text, then any HTML email generator needs HTML-ization -- escaping at a minimum and/or maybe some newline handling. (Plain text email generators won't that stuff.)
  • If the content is rich-text, then any HTML email generator should use a purifier. (Plain text email generators should use striptags or html2text.)

Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Aug 25, 2023

Choose a reason for hiding this comment

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

Hmm - I was thinking this was something generic enough to put in the 'all template' category - so that any workflow template from a form could include user content. Currently not all of them do - but where they do then I think any text could be entered - it it is coming from a free-text quick form input box

Copy link
Contributor

Choose a reason for hiding this comment

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

I think moving towards being able to add user content to pretty much any message template being sent would be a good thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the HTML versus plain text issue, see also #27173. I think it would make sense to have the user_text and the various receipt_texts all be HTML in the long run, then strip for plain text emails. It should be possible to use links in both the user_text and the receipt_texts. Our users have definitely complained about this in the past.

@larssandergreen
Copy link
Contributor

Approach makes sense to me.

@eileenmcnaughton
Copy link
Contributor Author

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen I've come back to this & I think addressed the issues - ie

  1. separate variables for text & html with conversions/ purify steps
  2. handling for code that passes in receipt_text via v3 apis to make that still available to templates that have been updated
  3. I think is there is userEntered text is should be displayed so I simplified the if

I'm still pondering a follow up that would move this chunk into the userEnteredText - effectively moving that to the setDefaults section. I think if I do that it would be part of splitting the ajax overload into it's own form - ie once #27613 & #27660 are merged

    {if !empty($isOnWaitlist)}
      <p>{ts}You have been added to the WAIT LIST for this event.{/ts}</p>
      <p>{ts}If space becomes available you will receive an email with a link to a web page where you can complete your registration.{/ts}</p>
    {elseif !empty($isRequireApproval)}
      <p>{ts}Your registration has been submitted.{/ts}</p>
      <p>{ts}Once your registration has been reviewed, you will receive an email with a link to a web page where you can complete the registration process.{/ts}</p>
    {elseif {contribution.is_pay_later|boolean} && {contribution.balance_amount|boolean}}
     <p>{event.pay_later_receipt}</p> {* FIXME: this might be text rather than HTML *}
    {/if}

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 6, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 6, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 6, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 7, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 9, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
@eileenmcnaughton eileenmcnaughton force-pushed the text branch 3 times, most recently from 74aa2ab to b096fb8 Compare October 10, 2023 05:01
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 11, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 11, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 11, 2023
This follows our historical practice of doing push receipts by
- adding the relevant receipt to the array of template in the upgrade
notification and
- (optionally) noting deprecated params on CRM_Utils_Token::getDeprecatedTokens

I didn't add lineItem to the deprecated array as it is pretty legit
to assign that within a foreach loop.

I also removed the assigns for the variables which are no longer relevant -
this can be followed up with more tidy up (some of which resolves php8.x issues)

Note that the template still uses
$billingName & $address - resolved in
civicrm#27692
$customGroup - resolved in civicrm#27596
(actually the variable remains but becomes non-leaky)
$credit_card_date,$credit_card_type (I propose moving to a paymet token)
$credit_card_expiry (I propose removing)
$event.confirm_text - addressed in civicrm#27162
This makes userEnteredText a property across all WorkflowMessages - with it outputting
the smarty variable user_text

We already had receipt_text on the membership forms - but that is a bit too specific
- user is perhaps a bit impersonal - I did contemplate form_text :
@eileenmcnaughton
Copy link
Contributor Author

all passed except style

@demeritcowboy
Copy link
Contributor

I can take a look at this tomorrow.

@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy

@demeritcowboy demeritcowboy merged commit 80b2344 into civicrm:master Dec 5, 2023
@demeritcowboy
Copy link
Contributor

I get CRM_Core_BAO_MessageTemplate: $params[subject] is deprecated. Use $params[messageTemplate][msg_subject] instead. Caller: CRM_Core_BAO_MessageTemplate::sendTemplate for emailing an invoice when you replace the subject, but I get that before the PR too.

@eileenmcnaughton eileenmcnaughton deleted the text branch December 5, 2023 19:07
@eileenmcnaughton
Copy link
Contributor Author

Thanks @demeritcowboy - that invoice one looks like an easy fix - have you looked at a code fix?

@agileware-justin
Copy link
Contributor

I think this change has introduced a problem, as reported here https://lab.civicrm.org/dev/core/-/issues/5025

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.

5 participants