CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.6 #9710
Conversation
| @@ -931,7 +931,8 @@ public static function getFirstLastDetails($contactID) { | |||
| */ | |||
| public static function calculateTaxAmount($amount, $taxRate) { | |||
| $taxAmount = array(); | |||
| $taxAmount['tax_amount'] = round(($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount), 2); | |||
| // There can not be any rounding at this stage - as this is prior to quantity multiplication | |||
| $taxAmount['tax_amount'] = ($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount); | |||
|
|
|||
| return $taxAmount; | |||
| } | |||
There was a problem hiding this comment.
Fundamentally wrong to round here - as at this stage - in this basic Utils function quantity has not yet been taken into account. Rounding here causes incorrect Sales Tax calculations.
| * @return int|void | ||
| * tax rate | ||
| */ | ||
| public static function calculateTaxRate($lineItemId) { |
There was a problem hiding this comment.
This function needs to be removed - it is fundamentally wrong to calculate Tax Rate using Tax Amount - which was calculated using Tax Rate.
| if ($field['html_type'] == 'Text') { | ||
| $taxAmount = $field['options'][$optionValueId]['tax_amount'] * $lineItem[$optionValueId]['qty']; | ||
| $taxAmount = round($field['options'][$optionValueId]['tax_amount'] * $lineItem[$optionValueId]['qty'], 2); |
There was a problem hiding this comment.
Now we can round - we have now multiplied by quantity.
| @@ -72,7 +72,7 @@ | |||
| {if $getTaxDetails} | |||
| <td class="right">{$line.line_total|crmMoney}</td> | |||
| {if $line.tax_rate != "" || $line.tax_amount != ""} | |||
| <td class="right">{$taxTerm} ({$line.tax_rate|string_format:"%.2f"}%)</td> | |||
| <td class="right">{$taxTerm} ({$line.tax_rate|string_format:"%.3f"}%)</td> | |||
There was a problem hiding this comment.
Some Tax Rates are 3 digits [like e.g. in QEO: 9.975%] - so let's not constrain them to 2 digits.
There was a problem hiding this comment.
Karen, I like what you are doing above, but I'm unsure why you would want to truncate the display of the tax rate on display here. If the user sets up more digits of accuracy when configuring the tax rate, shouldn't we just display what they entered? Our schema provides 8 digits of accuracy after the decimal. If you want to constrain to 3 digits on display, we should also constrain on input, and likely change the schema to reflect this new reduction in accuracy.
There was a problem hiding this comment.
I'm not introducing a new reduction in accuracy. In fact I'm increasing the accuracy with which the Tax Rate is displayed on the View Contribution screen - example: from 9.98 to 9.975 for admins who deal with QEO Sales Tax.
If we don't format it here - (neither .2 nor .3) then when orgs configure their financial accounts w/ 5% for tax rate -> it will display as 5.00000000 and for QEO 9.975% -> it will display as 9.97500000;
That's likely why this %.2f has always existed here. I'm just trying to relax that. The actual Tax Math is (w/ this PR) all done with all 8 decimals.
-- Karin
There was a problem hiding this comment.
Maybe try putting out all 8 decimals from the schema, then rtrim the trailing zeroes. This will make 10 appear as 10.%. If you would prefer in this case to get rid of the decimal point, then add rtrim on period after the rtrim on 0's. So most complicated would be: {$line.tax_rate|string_format:"%.8f"|rtrim:'0'|rtrim:'.'}
I haven't tested, but I expect this would render 10 as 10% and 9.975 as 9.975%.
There was a problem hiding this comment.
I only added this small/safe/fix to get 3 decimals (like QEO) @mlutfy Tax Rates displayed properly for admins. Please note that the PDF invoice - see above - does display the Tax Rate without any trailing zeros. There probably should be review/check to see how/where what is displayed re: Tax Rates [from input GUI to schema to output on screens etc] - but that's for another PR. These fundamental math issues need fixing now.
|
Steps to set up Sales Tax on a: 1. Enable Tax and Invoicing on: 2. Add a Financial Account on: 3. Associate that with a Financial Type on: 4. Create a Priceset on: 5. Go to a Contribution Page: Configure -> Amounts: 6. Ok: 7. Go to the Contribution -> View to see Tax Rate |
|
Jenkins re test this please |
1 similar comment
|
Jenkins re test this please |
|
jenkins test this please |
|
Not sure why Jenkins is unhappy here? |
|
Following the steps outlined above, I can confirm that the tax calculations are wrong without the patch and correct with the patch. Before patching (d46 master): View Contribution: Before patching (d46 master): Invoice PDF: After patching: View Contribution: After patching: Invoice PDF: Summary Failing tests in Jenkins Recommendation |
|
this seems like something that that LTS should have fixed in the next release , @eileenmcnaughton , @agileware |
|
Thanks, @jackrabbithanna . Since this is 4.6 my recommendation to merge probably goes to the LTS maintainers rather than Tim. |
|
@jackrabbithanna yes - it seems like people are advocating it for 4.6 too. There are a few related issues in JIRA so I would wait until all those are resolved in 4.7 & then ask the people involved to agree what exactly needs to go into 4.6 This has missed the cut off for the Feb 4.7 release (ie. the rc has been cut) & will be in the March release now - so for 4.6 it should go into the March or April release - which means I would not touch this PR until after the next release has gone out in order to avoid a situation where 4.6 has a fix not yet in 4.7 (avoiding that is the fundamental rule of the LTS). @twomice this PR is stalled on the fact we couldn't get the 4.7 version reviewed in time for the rc cut-off. @agileware has kindly offered to review this patch next week. I think you should add your review comments to the 4.7 version of the patch. I'll let @agileware make a call as to whether they need to do any further review on top. Like I said, there are still other tax issues in JIRA relating to tax so it would be good to check which of those remain afterwards. I would also note that on 4.7 @KarinG tested that paypal still works - I would suggest someone needs to confirm that on 4.6 separately before this is merged. |
|
Re: testing: I did a live PayPal as well as an Authorize.net transaction on 4.6 |
|
@eileenmcnaughton - @jackrabbithanna - @twomice - @adixon Ok so I've contributed back:
April - that's not ok. Alan and I have fixed this back in December in our 4.6 repo. We can't be the only ones that have orgs that need to file the correct $amounts with government? |
|
@KarinG if it was up to me, I'd probably hit this merge pull request button right now. Having the LTS have correct math would seem to me to have some priority. Your logic seems completely sound, I'm not sure what there is that refutes it, but community verification and consensus is also a good thing, and necessary all around. I do have trouble with the fact that a fix for 4.6 has to wait on not just one fix in 4.7, but a host of fixes. At the end of the day if @eileenmcnaughton says no then she's the boss and I don't feel right going against her wishes. She's been the good leader for the LTS for a long time. All I can say is that I hope you decide to keep working on getting the fix in within the parameters of the current political system. |
|
I'm not trying to bypass community consensus and verfication! But we all know a lot of PRs get in w/ a lot less than what I'm trying to CiviContribute back here; |
|
I not saying you are bypassing anything. IMO you are right on all counts |
|
@jackrabbithanna - I'm not the last word on the LTS now - you are. However, I do think it's vital we stick with not fixing things in 4.6 earlier than they are fixed in 4.7 because that means people might get regressions when they upgrade to 4.7 and it has always been a core principle of the LTS. In terms of the other fixes being approved first - that is what I would do - I would ensure it is completely bedded & fixed in 4.7 before looking at 4.6 & this fix seems like only part of the fixes needed. From what I can tell this is not a new regression & the bug has been around a long time. This particular PR has been open for 4 days. I think there may have been some whack-a-mole going on & Karin thinks this fix will be the start of the end of that (and I think she is correct about that). I did try to figure out before the RC was cut for 4.7 if there was enough consensus around the fix to get it merged for 4.7 for the rc - but although we did manage to get a volunteer to review on short notice, it was not going to happen in time to make the cut for the rc. In the past I have supported trying to get late breaking important fixes into the rc but what I found was that undermined the QA on the rc with people not seeing the rc as stable & waiting for the actual release to test it, so I do think it's important only regressions from the current rc are merged into the rc once branched, meaning most rcs should not changed once cut. In terms of timeframes - assuming the QA process is complete for the 4.7 patch next week it is up to you whether you merge this shortly after the next release goes out or wait until you are sure the tax issues really are resolved in 4.7 before starting to merge them in 4.6. I would opt for the latter if i were doing the rc, but it is absolutely your call. In general I would opt for slow & safe rather than rushing things in except for security fixes & recent regressions (& if this is in fact a recent regression that might change the call I would make here). |
|
@eileenmcnaughton Well I'm treading lightly and I look at you as a mentor, so your recommendations carry weight. I understand there being frustration to get the fixes now, and then there is the best practices we should follow. Building community consensus and verification is important as well. The issue to me is whether the stability of the LTS is lessened or increased with a PR. Fixing a tax math error should increase the stability, as long as unintended regressions don't result. I think it would be embarrassing if the world knew that the "Long Term Support/Stable" release, calculated taxes wrong and organizations could have errors on their tax returns and possibly get audited. That's no joke. They are preparing their tax forms now in the US, deadline april 15. So if we have to wait for April, we could have missed the window, and orgs will have bad tax returns. They could have already payed wrong taxes to the government in quarterly or monthly payments, for years! I think this is a serious situation, and as maintainer, I feel it is my responsibility to see what can be done, encourage those contributors who are passionate to fix a potentially embarrassing problem in the product. In fact if we don't put this in the release, perhaps we should write a blog article and inform organizations that their tax math could be wrong and to double check before they file any papers to their governments. So I do think there may be occasions where it is prudent to fix something in the LTS before 4.7. 4.7 may have more/different causes or associated edge cases with an issue. 4.7 may need more time to get a fix developed that works for the different or more complex general scenarios. Is this the case here? Are these 5 other issues associated with TaxMath for 4.7 present in 4.6? Does this PR make it all good as far as 4.6 is concerned? If that is so, then perhaps it should be merged. I don't know the answers to those questions, but I'd like feedback or to find out! |
|
@eileenmcnaughton - Not a recent regression - but a regression: The regression that introduces the tax_amount problem for priceSets with quantity field was: Prior to that it was fixed here: @jackrabbithanna - Even if we fix tax_amount values now - they will not be fixed retroactively - as the tax_amount is stored in the line_item table. So we may need a - careful double check your 2016 tax_amount values message - esp. if you use pricesets / quantity. |
|
These incorrect amounts SHOULD be in the scale of cents per transaction right? Hopefully organization's taxes shouldn't be WAY off, but even some off could have negative consequences... |
|
I think the issue is that I feel like there is a good chunk of work involved in answering all your questions. I am hoping that the people who have an interest in this will work together & get it all resolved for both releases & agree fixes in Feb so it can go out correctly for both versions for March. Like I said - I tried to see if anyone was prepared to put a couple of hours in between when the PR was submitted 4 days ago & when the PR was cut 2 days ago - but no-one could in that time frame and I think, to be fair, that a couple of hours was an underestimate since I'm over 3 hours on this already - just trying to get on top of where this is at without having reached conclusion as to whether it is safe or not. But, you are right this is a tricky issue. I am not an expert in this area of code and that drives my hesitancy. I actually made a deliberate decision some time ago to avoid the tax in CiviCRM based on my perception that it was not well tested & hence possibly not robust. That may have changed. I also have a bunch of general impressions - which may or may not be right from the PRs & issues I have skimmed over time - these may or may not be correct
All of those things make me hesitant to make a call on this for either 4.6 or 4.7 without following the normal process of having someone do a proper review against 4.7 & recommend it be merged, the merger assess whether they appear to have done an adequate review (often that is based on whether or not the reviewer is knowledgeable in the code area or we need a second opinion), and then check it still works against 4.6 & merge there too. So, rather than worry about trying to get this into the Feb release I have been trying to focus on corraling enthusiasm to get a solid review of this & a determination of how this fits with other issues / PRs in time for the March release. Note that if I personally were to review this issue at this stage I would put aside a whole day based on the above - however, there are other devs who have been much more actively involved in this particular area. |
|
Cross -posting - Karin answered questions while I was writing them |
|
So to summarize: tax_amounts have been accurate for much of 4.6 - (until late Feb 2016) - and have not been accurate for almost all of 4.7 |
|
PS - and I'm 16h into this - including digging into this spaghetti of code as well as my first attempts to fix the fundamental math issues - earlier this month. This is a much better more comprehensive PR [and I'm glad you pushed me for a PHPUnit test; it has made it better]. I'm not expecting it to be merged within days - but I was kinda hoping someone would be excited that (and I really think so) I've been able to get to the root cause here. There should be no debate over whether we should continue to patch/PR more - or start by fixing these fundamental math sequencing issues. |
|
PS Mark: we have seen erros of >1$ per line item - if you have a large quantity and small unit price. |
|
@KarinG yes - I read the patch and I absolutely agree that you have done a good job on it. I think we have our ducks in a row here - a patch that looks good & someone who has offered to review it & it is actually on track. I do hope we also reach clarity on which of the other prs & issues are actually just duplicates of this one. |
|
@civicrm-builder retest this please |
|
Test failures appear to be unrelated. |
|
@colemanw we were specifically NOT merging that because the relevant fix will not be in next weeks 4.7 release - per long discussion above. I don't think it needs to be reverted but FYI we were trying to adhere to the LTS rule of evaluating & merging to 4.7 first & not putting things in a 4.6 release until they are working upstream. |
|
Review of CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.6. #9710 The following tests were performed on a CiviCRM 4.7.15 installation before and after CRM-19908 patch is applied. Quick summary: Majority of test case results match with that for TaxMath Calculations 4.7, except for the following cases where 4.6 show correct results but 4.7 shows incorrect results: <Donation, pending pay later and price set>, <Membership, pending pay later and price set>, <Back-end Membership, credit card payment>, <Back-end Membership, pending pay later>. Test Case: Donation, credit card paymentSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Donation, pending pay laterSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Event registration, credit card paymentSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Membership, credit card paymentSet up:
Method 1
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Event, pending pay laterTest based on bug report, #23973#note-12 and #23973#note-10 Set up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Membership, pending pay laterTest based on bug report, #23973#note-12 and #23973#note-10 Set up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Recording a contribution using credit cardMethod:
Result:
Test Case Result:
Test Case: Event registration credit card payment and price setSet up:
Method:
Result:
Test Case Result:
Test Case: Membership, credit card payment and price setSet up:
Method:
Result:
Test Case Result:
Test Case: Donation, credit card payment and price setSet up:
Method:
Result:
Test Case Result:
Test Case: Donation, pending pay later and price setSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Event, pending pay later and price setSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Membership, pending pay later and price setSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Back-end Membership, credit card paymentSet up:
Method:
Result:
Test Case Result:
Test Case: Back-end Membership, pending pay laterSet up:
Method 1:
Result 1:
Test Case Result:
Method 2:
Result 2:
Test Case Result:
Test Case: Add Membership from search result and override membership amountSet up:
Method:
Result:
Test Case Result:
Test Case: Renew Membership from search result and override membership amountSet up:
Method:
Result:
Test Case Result:
|
|
This is great - it appears there are some issues remaining - are there tickets for them? |
|
@eileenmcnaughton relevant remaining issues are CRM-19538 (which has a bunch of facets relating to membership payments) and CRM-16656 (where which data was presented incorrectly was flipped by this / #9711 ) There's an option on CRM-18135 too, although that's storage precision / price specification rather than calculation. |
Please see https://issues.civicrm.org/jira/browse/CRM-19908 for full description/rationale.
This PR addresses two fundamentally wrong operations:
Purpose of these edits - three fold: to make the minimal edits required to:
Note: a PHPUnit test has been added to the 4.7 PR
This is the BEFORE: note Tax Rate is 5.01% and Tax Amount on that lineItem is $15.30 - this is incorrect:
This is the AFTER: note Tax Rate is 5% and Tax Amount on that lineItem is $15.26 - this is correct:
