dev/core/#299 Fix mishandling of non decimal currency on additional payment form.

[eileenmcnaughton]

Overview

Fixes mis-loading of default for amount on record payment form when non decimal thousand separators used

Before

Currency loads with a decimal point even when a comma decimal separator is configured

After

Currency loads with a comma when a comma decimal separator is configured

Technical Details

I tried to use as much of @mattwire work as relevant to solve this
https://github.com/civicrm/civicrm-core/pull/12055/files

What I found in there was that the code assumed that the code extracted to formatNumericByFormat fixed thousand separators but it in fact didn't. What I found is it converted '70' to '70.00' but in fact it didn't & required the self::replaceCurrencySeparators($amount); sequence to do that work. As a result I made formatNumeric protected & exposed 2 wrappers

• formatLocaleNumericRoundedByCurrency
• formatLocaleNumericRoundedForDefaultCurrency

@mattwire what do you think - this brings across some but not all of your work in a specific context - which could go broader. My big concern with the existing money format function is that it tries to do something for everyone & handle too many things.

We could shorten the function names - ie.
formatLocaleNumericByCurrency
formatLocaleNumericRoundedDefaultCurrency
Although if we later thought 'we only sometimes want to round' we'd find someone adding a magic param & we would start rebuilding 'format'

We could actually solve this bug with the existing format function except we would lose the rounding that is in the current function.

Comments

[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]

@mattwire are you able to look at this - I have tried to incorporate your ideas as they relate to this specific issue

[mattwire]

• (r-explain) Pass
• (r-test) Pass? We really should have unit tests wrapping the new money functions to make sure they are locked in, extending stuff already in tests/phpunit/CRM/Utils/MoneyTest.php. However, I've tested this and it fixes a known issue, there also seems to be a lack of funding in this area so if tests are a follow-up PR that would be acceptable to me.
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Pass - tested and it fixes an issue on the form.
• (r-user) Pass
• (r-tech) Pass - badly needed functions for formatting currency in non-US locales.

[seamuslee001]

@eileenmcnaughton i'm happy to merge this as per Matt's review but would appreciate an answer re tests first

[eileenmcnaughton]

@seamuslee001 I'll merge & do some tests as a follow up