Only format smarty aliases as money if specified #22547
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
6e3e042 to
1db76b7
Compare
|
test this please |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Jan 18, 2022
This gets us away from the empty checks & the isset that causes a crash with smarty modifiers. There is no change at the moment as to what is assigned so we don't need to make any changes to existing sites. The goal is to specify the mappings on the workflow template class later. That will allow us to remove significant amounts of code (& leakage points) from the form layer. However we still need to resolve civicrm#22547 because at the moment values in this template expect raw values - which is not the case with mapped params at the moment. Note the biggest thing to contemplate in this PR is the addition of an -always-there-in-contribution-smarty-templates smarty value isShowTax. The way it is currently checking is that there should pretty much be 'something' assigned if invoicing is enabled & it makes sense that tax would show even if zero if enabled - which is what I *think* is already happening (with maybe some random variation). I added 'taxTerm' as a domain token and I do kinda prefer that but it is available more places (unless we change the audience). invoice_notes is another contribution setting that effects display.
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Jan 18, 2022
This gets us away from the empty checks & the isset that causes a crash with smarty modifiers. There is no change at the moment as to what is assigned so we don't need to make any changes to existing sites. The goal is to specify the mappings on the workflow template class later. That will allow us to remove significant amounts of code (& leakage points) from the form layer. However we still need to resolve civicrm#22547 because at the moment values in this template expect raw values - which is not the case with mapped params at the moment. Note the biggest thing to contemplate in this PR is the addition of an -always-there-in-contribution-smarty-templates smarty value isShowTax. The way it is currently checking is that there should pretty much be 'something' assigned if invoicing is enabled & it makes sense that tax would show even if zero if enabled - which is what I *think* is already happening (with maybe some random variation). I added 'taxTerm' as a domain token and I do kinda prefer that but it is available more places (unless we change the audience). invoice_notes is another contribution setting that effects display.
|
This seems fine to me |
totten
reviewed
Jan 20, 2022
smarty aliases allow tokens to be assigned to the template as smarty variables. They are very new, have only been implemented in these 2 places and are not accessible from outside of core, which both have unit test cover. When trying to extend it to cover another variable (totalTaxAmount) in another template I found that a raw format is appropriate. I think it makes sense to explicitly specify. Code comment Co-authored-by: Tim Otten <totten@civicrm.org>
6909100 to
1c75b02
Compare
|
@seamuslee001 I'm confused - it says you closed this 'in #22560' - but I can't see any action you did on there that closed it - I've re-opened & re-based |
|
It's because the description there uses a magic word followed by the pr number, so github figures it means to autoclose it. Or maybe there is some plugin enabled that recognizes that phrase. |
|
@demeritcowboy ah that makes sense |
|
This seems to run ok. Maybe out of scope since it was like this before the PR but while the email itself has money formatting, the amount that appears on the email activity copy on the contact is raw. I'm not sure if it's always been like that. |
|
Thanks @demeritcowboy - I'm a bit surprised by that but hopefully I'll track it down.... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Only format smarty aliases as money if specified
Before
When defining a token that should be assigned as a smarty variable in a smarty template it is always assigned money formatted, if it is money
After
When defining a token that should be assigned as a smarty variable in a smarty template formatting should be specified
Technical Details
Smarty aliases allow tokens to be assigned to the template as smarty variables. They are very new,
have only been implemented in these 2 places and are not accessible from outside of core, which
both have unit test cover.
When trying to extend it to cover another variable (totalTaxAmount) in another template I found
that a raw format is appropriate. I think it makes sense to explicitly specify.
Comments