dev/financial#104 Use rounding and integers to compare monetary values…

[agileware-fj]

…with precision

Overview

Replace the questionable epsilon algorithm to compare two monetary amounts with rounded int casts
Ref dev/financial#104

The existing implementation causes issues e.g. when an external application provides, as required, a pre-rounded post-tax amount to the Order.create API and pre-tax amounts to the embedded line items.

Before

Comparing two amounts using CRM_Utils_Money::equals would defer to CRM_Utils_Money::subtractCurrencies to get a delta between the amounts, and then make sure they were within 1/1000th of a currency unit (an "epsilon" check) to verify that the amounts are indeed the same.
There's not much makes sense about this; notably, the subtractCurrencies function doesn't actually do what it says it does (integer subtraction with currency precision), but just juggles floats about.

After

CRM_Utils_Money::equals multiplies and casts both values to a precision shifted integer, and compares if equal

Comments

Also discussed on chat.civicrm.org

Agileware Ref CIVICRM-1368

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

[agileware-fj]

Test failure is relevant, although the message doesn't make sense.

CRM_Utils_MoneyTest::testEquals
Failed asserting that true is false.

• /home/jenkins/bknix-dfl/build/core-15856-1t5le/web/sites/all/modules/civicrm/tests/phpunit/CRM/Utils/MoneyTest.php:32
• /home/jenkins/bknix-dfl/build/core-15856-1t5le/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:218
• /home/jenkins/bknix-dfl/civicrm-buildkit/extern/phpunit6/phpunit6.phar:570

[seamuslee001]

@agileware-fj Francis what is is saying that in this test https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/CRM/Utils/MoneyTest.php#L32 rather than the Money::equals function returning false it returned TRUE or some other value instead. I think even if you return 0 it will fail because it didn't get an actual FALSE

[agileware-fj]

@seamuslee001 Thanks. The failure message is still Less Than Awesome.

Anyway I'll review and update the PR.

[agileware-fj]

Ah, I see that the problem is actually that I disagree with the test in terms of what this function is doing for the use case I'm trying to fix. I need to find out what else is calling this function and whether the test's truth can be adjusted to match mine...

[agileware-fj]

I've updated the test CRM_Utils_MoneyTest::testEquals to match the functionality here, based on the following considerations:

1. CRM_Utils_Money::equals is only used via the in this and directly related use cases via CRM_Contribute_BAO_Contribution::checkLineItems
2. The test was testing for an equality condition with an incorrect premise, as described in the ticket.

[mattwire]

I don't have time to review this right now, however this was the original PR when the equals function was added: #12352 So the reviewer should check that the logic and discussion there is still met by this improvement

[jusfreeman]

Ping @MiyaNoctem @JoeMurray would you be able to take some time to review this PR? Thanks!

[agileware-fj]

@MiyaNoctem @JoeMurray @mattwire anyone have any time coming up they could review this in? Thanks.

[jitendrapurohit]

Seems fine to me as far as the description is concerned. Tested the PR with various money comparisons and confirmed that it works fine in all the cases. Below are the manual testing values with the returned result -

• 1.23 & 1.234 = EQUAL. Before this PR, it used to return FALSE for this comparison.
• 1.234 & 1.235 = NOT EQUAL.
• 99.99 & 100 = NOT EQUAL.
• 99.995 & 100 = EQUAL.

This looks fine for the 2 decimal digits. Not sure if it's worth to consider 0.001 precision as mentioned in #12352. Is it really required? Ping @MiyaNoctem in case of any objections pls.