dev/financial#180 Fix line item calculation regression on line items (incorrectly treating as exclusive) #21212
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
541-orig
branch
from
b04de0c to
97401ea
Compare
eileenmcnaughton
changed the title
eileenmcnaughton
changed the title
eileenmcnaughton
force-pushed
the
541-orig
branch
3 times, most recently
from
8f6f9b9 to
d8375c2
Compare
|
@eileenmcnaughton I can confirm that the patch correctly sets the line_item.line_total to be exclusive of tax_amount and is stored separately in line_item.tax_amount. Unlike for contribution.total_amount which is inclusive of tax_amount and there is special handling at Contribution.create api3. But there is a test failure https://test.civicrm.org/job/CiviCRM-Core-PR/43292/testReport/junit/(root)/api_v3_TaxContributionPageTest/testCreateUpdateContributionChangeTotal/ which is related and needs similar fix. As this patch is touching the essential part of financial code, I need anyone else to confirm my assessment. |
This was added civicrm@e967ce8#diff-a16d4d7449cf5f3a0616d1d282a32f27ab6d3f7d2726d076c02ad1d4d655af41R512 but incorrectly assumes the totalAmount is exclusive at this point - but it is inclusive
eileenmcnaughton
force-pushed
the
541-orig
branch
from
9ab745c to
0e4d461
Compare
| @@ -392,8 +392,8 @@ public function testCreateUpdateContributionChangeTotal(): void { | |||
| ]); | |||
|
|
|||
| $this->assertEquals('300.00', $lineItems); | |||
| $this->assertEquals('320.00', $this->_getFinancialTrxnAmount($contribution['id'])); | |||
| $this->assertEquals('320.00', $this->_getFinancialItemAmount($contribution['id'])); | |||
| $this->assertEquals('300.00', $this->_getFinancialTrxnAmount($contribution['id'])); | |||
There was a problem hiding this comment.
This reverts the changes made in the same commit as the original sin
eileenmcnaughton
force-pushed
the
541-orig
branch
from
0e4d461 to
0b7692b
Compare
|
Tested the changes on the following scenarios - Frontend - Online Contribution Page. Backend - New membership (w/o priceset) Webform - Webform purchase of New Membership. It passes on all the above steps - expected values are present in line_item table and the amounts looks fine on view contribution and view membership pages. I think we're good to merge this 👍 . |
|
Thank you @jitendrapurohit 👍 @eileenmcnaughton -> is this in addition to the other PR? |
|
@jitendrapurohit - re: webform and tax amount -> I think the better_line items branch addresses that (I use it in D7 and need to revamp it’s on our Google doc for D8/D9 port and inclusion). |
|
@KarinG this fixes line_item.total_amount being incorrectly inclusive when calculated in the BAO - I'm not 100% sure if we need an order api fix on top of this @jitendrapurohit I think you are saying
|
|
Also @jitendrapurohit I think this would mostly impact contribution.create api calls but not so much core forms - which generally pass in 'line_item' as a parameter to contribution.create |
|
@monishdeb I think we should merge this based on @jitendrapurohit & then focus on #21230 which should address the originally identified bug |
|
Agree. Merging now based on @jitendrapurohit @KarinG and my feedback. |
|
I'll add dev-master to our automated webform testing so that I can run this PR against all our tests. |
|
Thanks @monishdeb @jitendrapurohit @KarinG I've rebased #21230 to just have the order api fix I haven't updated the tests in it at this stage because I think it makes it possibly easier to check & agree the tests need updating |
|
Understood - just wanted to flag that no other issues surfaced on our end post merging this PR. |
|
thanks @KarinG |
|
And here's the direct link to the related Gitlab for anyone else who is wondering, https://lab.civicrm.org/dev/financial/-/issues/180 (this could be the job of a Gitbot) |
|
@agileware-justin you ARE our gitbot |
|
lol |
Overview
Fixes a 5.41 regression where total amount was assumed to be exclusive when generating line items and calculating tax when it is only sometimes exclusive
Before
Total amount is assumed to be tax-exclusive in all scenarios and line items calculated on that assumption
After
Total amount is assumed to be inclusive in the bao - the edge case where it is exclusive historically and tested to be so is moved to contribution.create api
Technical Details
A recent fix added tax_amount calculations to getLineItems and used that amount to calculate the tax amount.
However, in accordance with
api_v3_TaxContributionPageTestI reached the conclusion that the 'normal behaviour' was to treat the passed in amount as inclusive. In fact that is historically the case ONLY IF tax_amount is not passed in - and it seems that it most commonly IS passed in.This moves the weird handling to the v3 contribution api. The intention is that v4 contribution api and order api will always assume that it is inclusive - although we can discuss adding the flexibility to define exclusive instead from the order api.
Note that when line_item (e.g as core forms do) is passed the bug is not hit - the focus really is on the contribution.create api when it is allowing the BAO to calculate the line items.
Comments
@KarinG @mattwire @monishdeb @jitendrapurohit @christianwach this is the fix for the original sin - I think with this merged it will fix Contribution.api to 'do the right thing' when tax_amount is passed in. The fix for the order api becomes pretty simple at that point 05d1bba
However, I'm still expecting some tests that were altered AFTER the bug was introduced to fail with this or the order fix - just because the tests are wrong I think.