Fix decimal places for tax rate on registration confirm/thankyou #13827
Conversation
|
(Standard links)
|
|
Reviewing code I don't see anywhere that the variables into which floats are placed are used for later calculations or stored. This looks like the same approach as used in the earlier commit. I haven't done actual testing. So a tentative looks good to me. Sorry I don't have more time to do the testing to confirm my very strong sense that the values stored in the db in all use cases are the same after this is merged. |
| // Cast to float to display without trailing zero decimals | ||
| $tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate']; | ||
| } | ||
| if (!empty($v['tax_rate'])) { |
There was a problem hiding this comment.
This is just a cleanup I think? check for empty in place of check for isset + !== ''
| // Cast to float to display without trailing zero decimals | ||
| $tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate']; | ||
| } | ||
| if (!empty($v['tax_rate'])) { |
| $getTaxDetails = TRUE; | ||
| // Cast to float to display without trailing zero decimals | ||
| $tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate']; |
There was a problem hiding this comment.
so this looks like the actual change & the rest is all just renaming?
| $getTaxDetails = TRUE; | ||
| // Cast to float to display without trailing zero decimals | ||
| $tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate']; |
There was a problem hiding this comment.
Correct (in confirm + thank you)
|
@mattwire am I correct this is just a 2 line change mixed in with a lot of unrelated cleanup? I felt comfortable with the cleanup in the first 2 files but realised Id' need a deeper dive to look at the cleanup on the last 2 files. |
|
@eileenmcnaughton that's right. We should really extract this code from the 4 files into its own function but they have small differences in order of statements etc which need checking to do that. The changes here just bring them much closer together without risk of breakage. It will make a future extraction much easier |
|
@mattwire I'm still struggling with the fact that the various bits of cleanup seem only semi-related so I can't merge the ones I've checked , or even the fix, without doing a deeper review of the other changes |
|
@mattwire I had a go at making sense of this with the cleanup still in it. In the end I didn't see the value of renaming the variables as a step towards cleanup & I think setting up the 'right' function somewhere would be the right step. I see that function as looking a bit like an alter function but in the end I decided I was going down a rabbit hole & did this minor PR & am finishing up for the day |
|
@mattwire thinking more about this - I think a good step would be to add in a new Trait file called OnlinePaymentFormTrait & have the online pages share it - then we can extract assignLineItemsToTemplate in that Trait. I don't think aligning variable names is super helpful but we should switch each instance of using the shared function as separate steps so we don't miss stuff - I had a go here https://github.com/civicrm/civicrm-core/pull/13843/files |
|
Closing this as stale / much of the work from it is otherwise merged - once we merge #13899 then we will be in a good position to use that fn from these files |
Overview
This is the same change as #10856 but for the event confirm/thankyou pages.
Before
Tax rates shown with 8 decimal places for line items on event confirm/thankyou pages.
Examples: 20.00000000%; 8.90000000%
After
Tax rates shown with no trailing zeros for line items on event confirm/thankyou pages.
Examples: 20%, 8.9%
Technical Details
Cast to float per contribution page changes.
Also minor cleanup to make the code more consistent between the 4 pages (contribution/event confirm/thankyou) to allow for future consolidation.
Comments
@highfalutin @JoeMurray @kcristiano Could one of you please review? Noting the reference to the related PR that was merged in 2017