Ensure comma not formatted into amount

[eileenmcnaughton]

Overview

Ensure comma not formatted into amount

Before

Calling getAmount on CRM_Core_Payment adds a thousand separator

After

It doesn't

Technical Details

Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227

Comments

I think this must have regressed recently-ish when we messed with money formatting - but I'm not 100% sure when

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

[totten]

@eileenmcnaughton This passes a third parameter (en_US); in the actual method, it doesn't take a third parameter, but it does hard-code a reference to en_US.

I'm trying to understand the difference between "moneyNumber" and "machineNumber". Is the idea that "machineNumber" is supposed to be less context-sensitive (ie the machineNumber format isn't actually about your specific locale; it's about some general norm that's seen most wire-protocols; and a way to match that norm is to use en_US with GROUPING=>false)?

[eileenmcnaughton]

@totten so moneyNumber is a number formatted per the currency (but without the currency symbol) - eg. as you would do it in data entry

machineMoney is money formatted with the number of decimal places that the would be used for that currency but is machine-ready

You are right the locale can go now - I changed it - the currency is only relevant in that it might (in future) be something like JPY in which case it would be 1000 not 1000.10

[totten]

OK, cool.

Flagging "merge ready" (ie any additional feedback from @stesi561 could sway it), but it's de-facto "merge on pass" (ie the time for 5.49 is at-hand; after another test-run, there's no specific reason to hold it up).

[totten]

The relevant test is passing.

The test failure CRM_Core_DAOTest::testSqlModePresent is currently a common false-negative.

There's a change in the method moneyNumber(), which appears somewhat ancillary, but it's just a small refactoring (inline variable) to make the code of the old+new methods similar.

I don't have the specific environment to r-run, but that's why this area has aggressive test-coverage (eg ext/payflowpro/tests/phpunit/CRM/Core/Payment/PayflowProTest.php). If the tests pass, then I'm OK with it.

It would be appropriate to add more coverage via Civi\Core\FormatTest,but there is indirect coverage, so it's not a blocker.

I added a comment/question about machineNumber()'s signature.

[totten]

New test-run mentions a failure in CRM_Core_Payment_AuthorizeNetIPNTest::testIPNPaymentRecurNoReceipt. Based on MM chat, this appears to be a little flaky (reportedly, same error in other/unrelated PRs). Also, for both eileen and me, the test passes locally.