CRM-21062 Remove trailing zeros in tax rate for online Confirm/Thankyou forms -… #10856
Conversation
|
To clarify, the issue only occurs
I reproduced the issue on a build of master. Code looks good and the PR fixes the issue as described. @totten or @eileenmcnaughton could you merge please? |
|
More scary things happen that I have words for happy with variants of $lineItem $lineItems & $this->lineItem, $this->_lineItem....... in that Contribution page code. Floats are also kinda scary http://floating-point-gui.de/ Is it not possible to do the formatting in the tpl? |
| @@ -130,6 +119,20 @@ public function buildQuickForm() { | |||
| $this->assign('taxTerm', $taxTerm); | |||
| $this->assign('totalTaxAmount', $params['tax_amount']); | |||
| } | |||
|
|
|||
| if ($this->_priceSetId && !CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceSet', $this->_priceSetId, 'is_quick_config')) { | |||
| $this->assign('lineItem', $this->_lineItem); | |||
There was a problem hiding this comment.
@mattwire, per question from @eileenmcnaughton, why not do your type conversion / number formatting here, at the assign, rather than converting the _lineItem property of the form object itself?
|
I'd prefer to move formatting to .tpl, especially if it is just doing something to suppress trailing zeroes. More importantly, though, my guess is that different jurisdictions have different practices regarding how many digits are expected, eg always 3 even if there are trailing zeroes might be a practice somewhere. @systopia what is the expectation re: display of trailing zeroes on VAT? |
|
@JoeMurray @eileenmcnaughton The problem with putting the formatting in the tpl is that there seems to be no way of handling different values in a nice way. Eg. crmMoney will always format to 2dp but sales tax is a percentage. Here in the UK it looks really odd to display 20.00% VAT. Casting to float before passing to the template means the trailing zeros are never passed to smarty and it displays whatever percentage is passed, be it 20%, 19.6523%, 8.3% etc. The current behaviour displays 8 decimal places! I don't think it's the norm in many jurisdictions to force a number of decimal places for the percentage, but if it was somewhere, it would be easy enough to override at the template/jquery level. |
@JoeMurray: If you're asking me because you want to know how things are in Germany, then I can tell you that I've only ever seen the |
|
@mattwire moving to the assign rather than adding to the line item array alleviates my concern. Re tpl - I guess things like crmMoney were added as Civi smarty plugins so it might be the case that crmPercentage does make sense |
2c7d8ae to
d22caf0
Compare
|
@eileenmcnaughton @highfalutin @JoeMurray As discussed, I'm now copying the lineItems array and modifying that, so we don't touch the original. This should be good to merge now? |
|
Jenkins retest this please |
|
This feels safe to me now. I would prefer to go a step further & extract the code to a function shared between the screens ($this->assignLineItems()) or similar, but I don't feel it need to block this |
… safer change
Overview
Separated from CRM-20852 so that can be merged as Confirm/Thankyou pages don't have test coverage. I'm pretty certain this change has no impact as it's only touching display code.
Before
Tax rate is displayed with 8 decimal places: eg. 20.00000000%, 3.50000000%
After
Tax rate is displayed with no trailing zeros: eg. 20%, 3.5%
Comments
This PR should be tested on a couple of sites with tax and invoicing enabled.