[PHPUnit test only] Adding in assertions re: Line Item and Contribution data-integrity.

[KarinG]

Overview

In 4.7.27 - one of our large professional association clients observed a data integrity issue were after Edit Contribution the Line Item subtotals + their Tax Amount no longer added up to Contribution total_amount [not talking rounding errors - June 2018 is off by > $9.5k]. The good news is that we've not yet observed it in 5.3.x since. However I could not find any tests that would secure/lock in data integrity between Line Items and Contribution totals.

Before

Unit test testLineItemAmountOnSaleTax -> checks that $lineItem['qty'] and $lineItem['tax_amount'] are unaffected upon a simple Save of the Edit Contribution form;

After

Unit test testLineItemAmountOnSaleTax -> now also checks that $lineItem['unit_price'] and $lineItem['line_total'] are also unaffected upon a simple Save of the Edit Contribution form;

In addition
Unit test testLineItemAmountOnSaleTax -> now also checks that $contribution['total_amount'] = $lineItem['line_total'] + $lineItem['tax_amount'];

In addition
Unit test testLineItemAmountOnSaleTax -> now also checks that $contribution['tax_amount'] = $lineItem['tax_amount'];

Technical Details

The tests I've added Fail on my local/buildkit - the unit_price and line_total are 55.00 after saving the Contribution Form [before saving the form -> lines 1323 - 1324 they were 50.00] - see screenshot:

Comments

Of course I realize we can't merge in a failing test - so want to here:
a) check to see if Jenkins agrees
b) get consensus that the additional assertions here are correct

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[KarinG]

Also check if $lineItem['unit_price'] and $lineItem['line_total'] have not changed on Resubmitting the Edit Contribution form.

[KarinG]

This is the data-integrity part - we must ensure things add up. Since there is only 1 Line Item in this test we don't have to worry about summation over Line Items.

[KarinG]

Jenkins - failed -> expected:
(first assertion that fails is):

CRM_Member_Form_MembershipTest::testLineItemAmountOnSaleTax
Failed asserting that '55.00' matches expected 50.0.

[eileenmcnaughton]

@KarinG so we obviously can't merge this because the test it adds doesn't pass....

What happens if you comment out that line? Sometimes the path to getting tests merged is to comment out the parts that still need fixing & then they can be re-enabled when the fix follows

@jusfreeman ping

[KarinG]

Thanks @eileenmcnaughton - Absolutely.

3 of the 4 assertions that I injected fail (and they should not fail - so three would need to be commented out):
$this->assertEquals(50.00, $lineItem['unit_price']);
$this->assertEquals(50.00, $lineItem['line_total']);
$this->assertEquals($contribution['total_amount'], $lineItem['line_total'] + $lineItem['tax_amount']);

1 of 4 assertions that I injected passes (correctly):
$this->assertEquals($contribution['tax_amount'], $lineItem['tax_amount']);

I'm hoping we can put our heads together towards making Sales Tax Math testable. Some of the Math obviously happens in the GUI layers (and it shouldn't).

ping @monishdeb and @mattwire as well.

[eileenmcnaughton]

OK cool - so I guess for this PR the main outcome is to trigger discussion & focus on those issues. Let's keep the PR open to facilitate discussion for a bit & then close & drop back to tracking via gitlab it it doesn't become mergeable in the next couple of weeks

[KarinG]

If we get consensus on these Assertions - perhaps we can merge them in commented out - with a note;

[eileenmcnaughton]

That works - let's see if people agree on the assertations

[KarinG]

I did some research and found that @monishdeb intended to inject at least 2 of the 3 assertions that I'm now trying to provisionally inject - earlier this year:

https://gist.github.com/monishdeb/ab5d62fb6799f4e3d99f8e75432a64ca

However the commit that went into the code base is omitting two previously gist-ed assertions: re: lineItem['unit_price'] and lineItem['line_total'] https://github.com/civicrm/civicrm-core/pull/11655/files

I understand why these assertions had to be omitted at that time - because those assertions fail (off by $5).

Context was a PR by @mattwire : #11461

My thoughts:

• I think we want to be able to make these assertions in phpunit tests? If yes can we provisionally commit this PR with // commented out and make that our goal - that we can lock in a test that ensures lineItem $ add up to contribution $
• The GUI (using CiviCRM itself) is fine - which means somewhere else there is a $5 offset to make two wrongs right. I can dig in and look into that if anyone finds that is the next step here?
• I'm not trying to solve a bug for a specific client - trying to help prevent more in future.

[mlutfy]

+1 more tests in this area of the code.

+1 merge them commented-in, while work is done to figure out why the form behaves differently when used from the GUI?

Do we have a Gitlab issue for the failing tests? It would be important to cross-reference it in the code. Otherwise they'll probably get cleaned out at some point.

[JoeMurray]

I think that at a high level the contribution total should equal the sum of (the qty*amount for each line item plus tax on line item). I focus on the financial_item and financial_trxn tables and am less confident knowing how accurately the tax in line item tracks the accounting value stored in financial_item.

+1 for more tests in this area.

[eileenmcnaughton]

@mlutfy I think rather than a gitlab issue the commented out lines need a good chunk of code comments defending their existence & explaining whey then need to be fixed rather than than removed by cleaner-uppers

@KarinG sounds like you should update this to be commented out + commented & we can merge this & we'll at least get one extra assertion for now

[KarinG]

@eileenmcnaughton - I've made the edits - do you think the wording is clear?

[eileenmcnaughton]

@KarinG yes - I think this helps - merging

Interestingly I took a small foray into the membership code & found straight away an inconsistency on tax handling in membership renewal vs membership https://github.com/civicrm/civicrm-core/pull/12593/files#diff-349ca8bcf1ef264bf3ca9acc2593aa54R707

[KarinG]

@eileenmcnaughton - thank you for merging this and for any steps aiming to consolidate Tax Code.

$taxRate = $this->getTaxRateForFinancialType($values['financial_type_id']);

That should be a reliable way to get the taxRate - and should probably become the standard - I think @monishdeb wrote that and is already using that in many places.