dev/core#193: Ensure that tax amount is calculated when checking for order API line items

[omarabuhussein]

The original PR can be found here : #12372

Overview

When creating an Order using Order API a check is done to compare Contribution's total_amount against the sum of line item's line_total's. If Contribution total amount and Line Items' total are not equal, Order creation will fail.

Before

As we know:

The line item line_total do not include any tax. Tax will be recorded separately in line item tax_amount field.
The contribution total_amount do include tax. A separate contribution tax_amount field also records the amount of tax.

Since the validation only compares the sum of line_totals with contribution total_amount, whenever there is tax, the validation will fail.

For illustration consider following data set that should create 1 contribution with 2 line items for memberships.

order_data =
{
    'is_pay_later': True,
    'contact_id': 240L,
    'payment_instrument_id': 6,
    'line_items': [{
                       'line_item': {
                           '42': {
                               'price_field_value_id': 60L,
                               'price_field_id': 36L,
                               'entity_id': 1249L,
                               'tax_amount': 2.0,
                               'line_total': 10.0,
                               'financial_type_id': 2,
                               'label': 'General',
                               'entity_table': 'civicrm_membership',
                               'unit_price': 10,
                               'qty': '1'}}}, {
                       'line_item': {
                           '43': {
                               'price_field_value_id': 61L,
                               'price_field_id': 37L,
                               'entity_id': 1248L,
                               'tax_amount': 1.67,
                               'line_total': 8.33,
                               'financial_type_id': 2,
                               'label': 'Concession',
                               'entity_table': 'civicrm_membership',
                               'unit_price': '8.33',
                               'qty': '1'}}}],
    'tax_amount': 3.67,
    'total_amount': 22.0,
    'contribution_recur_id': 605L,
    'financial_type_id': 2,
    'fee_amount': 0,
    'payment_processor_id': 5L,
    'receive_date': u'2019-08-01',
    'contribution_status_id': 2}

Currently Calling the Order API to create Order with the above data results with "Line item total doesn't match with total amount". This is because total_amount (22.0) <> sum of line_totals (18.33)

After

The Line Items check now consider tax_amounts of line_items and order API works fine with the parameters above.

Other Notes

The checkLineItems() method is changed to return True on the success of the validation or False on its failure instead of throwing an exception,

[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).

[eileenmcnaughton]

@omarabuhussein code style CI failure

[JoeMurray]

Thanks for this very needed PR
@Monish please review in coming days

[mattwire]

@JoeMurray @monishdeb Did you get a chance to review this?

[monishdeb]

Or 1 line change:

$lineItemAmount += ($item['line_total'] + CRM_Utils_Array::value('tax_amount', $item, 0.00)); 

[monishdeb]

@omarabuhussein @mattwire earlier the intent might be to have native exception handler for line-item errors. But then if I go with the change it is doing the same thing but old code throw native exception instead of API exception although it is used only in order.create api. So I want to know more why do you think we should get rid of CRM_Contribute_Exception_CheckLineItemsException?

[mattwire]

I think there was an older PR where we discussed this. I didn't really like the return value of this function being changed but I could see why @omarabuhussein wanted to - it makes the function easier to test. But if there is a reason why we should keep the exception then it should stay.

[eileenmcnaughton]

Can't testing convenience be solved just by handling the exception in the test? Nuanced exceptions is a pattern we have been increasing not decreasing.

[omarabuhussein]

@eileenmcnaughton but that's does not solve how we can write 'positive' tests for this method : #12372 (comment)

[eileenmcnaughton]

@omarabuhussein well if negative is an exception then positive is no exception

[omarabuhussein]

yeah I realize that, it is just I don't like testing for "no exception thrown", something feels wrong about it to me. But if everyone insist to keep the exception (as it seem the case here) then I guess I have no other option.

[monishdeb]

@omarabuhussein I think you need to retain the CRM_Contribute_Exception_CheckLineItemsException and instead of just calling the checkLineitems() you should use the order.create API in a new unit test (in tests//phpunit/api/v3/OrderTest.php) with paramaters which should trigger the exception error and assert the try-catch the error message.

[omarabuhussein]

thanks @monishdeb for the review, really appreciate it.

instead of just calling the checkLineitems() you should use the order.create API in a new unit test (in tests//phpunit/api/v3/OrderTest.php) with paramaters which should trigger the exception error and assert the try-catch the error message.

But that means I will not test the method in isolation which means that this is a not a unit test anymore

I left more notes about why I did this here : #12372 (comment)

let me know what do u think when u have the chance.

[eileenmcnaughton]

@omarabuhussein "But that means I will not test the method in isolation which means that this is a not a unit test anymore"

This is true - what we refer to as unit tests are not unit tests from a purists point of view. However, we prioritise testing end to end api functions as those are out main integration points so it's kinda more useful to test Order api in e2e than to honour the concept of unit testing

[eileenmcnaughton]

It's also worth noting we only support people interacting via the api - ie we don't support people calling checkLineitems from their own extension -that function could change over time

[omarabuhussein]

I am aware of your view regarding testing @eileenmcnaughton , it is just when I see a chance to isolate a test then I will do it as I did here. But fine then I will bring the exception back and replace the tests with tests around the order API.

[omarabuhussein]

To keep the conversation clean and avoid them being hidden from doing a rebase, I am closing this PR and opening this instead : #13091

the new PR keep the exception and test the Order API instead.