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

dev/core#3962 Add 'boolean' as a filter for tokens #24923

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 8, 2022

Overview

dev/core#3962 Add 'boolean' as a filter for tokens

Before

Per https://lab.civicrm.org/dev/core/-/issues/3962 it is very difficult to combine smarty with tokens in text/plain messages as appostrophes break our existing work-around of using `{if '{contact.first_name}'}{/if}'

After

New modifier added |boolean e.g `{if '{contact.first_name|boolean}'}{/if}'

Technical Details

I was originally going to use the shorter 'bool' on the basis bool is a well established shortening of boolean within code. But of course not everyone reading it is familiar with that, or English speaking. I decided that as usual the marginal benefits of using an abbreviation don't stack up compared with the advantage of using an actual word that most people will understand (even if they don't understand the syntax)

Comments

The PR is mostly tests :-)

@civibot
Copy link

civibot bot commented Nov 8, 2022

No issue was found matching the number given in the pull request title. Please check the issue number.

@civibot
Copy link

civibot bot commented Nov 8, 2022

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

PR #24923: dev/core#3962 Add 'boolean'...
  No changes.
  GitHub pull request #24923 of commit 80f3f6c, no merge conflicts.
  CheckStyle: No warningsNo issues for 6 builds, i.e. since build: #52238Reference build: CiviCRM-Core-PR #52242
  Test Result (1 failure / +1)api_v3_JobTest.testCallUpdateGreetingCommaSeparatedParamsSuccess

PR #24923: dev/core#3962 Add 'boolean'...
No changes.
GitHub pull request #24923 of commit 80f3f6c, no merge conflicts.

CheckStyle: No warnings

No issues for 6 builds, i.e. since build: #52238
Reference build: CiviCRM-Core-PR #52242
Test Result (1 failure / +1)
api_v3_JobTest.testCallUpdateGreetingCommaSeparatedParamsSuccess

@eileenmcnaughton eileenmcnaughton force-pushed the filt_bool branch 3 times, most recently from 3c479be to ceeee88 Compare November 11, 2022 02:27
Add unit tests leveraging & demo-ing boolean fitler in greetings
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I finally fought test hell on this - are you OK to merge it? Good test to code ratio :-)

@demeritcowboy
Copy link
Contributor

Good job fighting tests. I'll take a look later.
(To ponder: Why do we still support text format?)

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy for example greetings - ie "Dear Dave" not "Dear Dave" ( special characters are a better example)

@demeritcowboy demeritcowboy merged commit 8d5af7d into civicrm:master Nov 18, 2022
@demeritcowboy
Copy link
Contributor

Some minor r-tech thoughts:

  • As per the comment at the top of the function this function is creeping towards looking like old-timey civi code.
  • I can see some potential for this filter to be mis-used, but you can do that with anything.

r-doc:

wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request Nov 22, 2022
https: //github.com/civicrm/civicrm-core/pull/24923/files

This will allow us to filter on {contact.first_name|boolean} in
smarty IF clauses for greetings

Bug: T321619
Change-Id: I1caf5439573264218b587f6ebd5f542860d68884
@eileenmcnaughton eileenmcnaughton deleted the filt_bool branch November 22, 2022 21:45
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.

2 participants