dev/core#1522 Handle space as thousand separator

[jaapjansma]

Overview

, as decimal separator, and [space] as thousand separators leads to api errors
This PR fixes that. It also includes tests for testing different localization.

Issue 1522 contains a table of the analyses of which localized options are going wrong.

Before

• No tests for localization of currency with euro, different decimal and thousand separators
• Api error when creating a contribution with , is a decimal separtor and [space] a thousand separator

After

• Added a localized Contribution Create Api v3 test
• Added a localized test for CRM_Utils_Rule::money
• Changed CRM_Utils_Rule::money so it accepts different localized options.
• Api error is gone

Comments

Added deprecated flag to setCurrencySeparators in test/phpunit/CiviTest/CiviUnitTestCase.php

Also added three new functions setMonetaryThousandSeparator, setMonetaryDecimalPoint and setDefaultCurrency

The reasoning behind this is that the setCurrencySeperator only sets the thousand seperator and somehow magically decides what than the decimal seperator is.
I need more control of setting each of those, including the currency.

If necessary I can create a separate PR which removed the setCurrencySeparators from all tests and replace them by the new functions. I can and will do that after this PR has been approved and merged.

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

In terms of background about related issues we have hit - the current cleanMoney code doesn't cope well when the same string is passed through it twice. We had a lot of issues with money being converted to the wrong value.

We determined that we should cleanMoney as close to form submission as possible & it should always be clean by the time it hit the BAO. I thought at the time it should be clean before hitting the api but the risks in changing that were pretty high.

@colemanw it would be good to agree the best place to clean this for apiv4 while we can still change things

In terms of how the above affects this - a lot of the code in that function was because money was regularly being 'cleaned' more than once & it sought to avoid messing it up. When we added more decimal places to some fields the double-clean checks stopped working & we got a bunch of bugs.

I guess all of that is a long way of saying I'd be keen to have you run this in production for a bit @jaapjansma (also on sites with other separators preferably) & maybe we can merge once the next rc is cut so you will have had it in prod for a couple of months by the time it goes out, in case it surfaces some cases where double-cleaning causes issues.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

I think this will cause jenkins to fail - not sure if that is the issue giving the weird test error atm - the params need to be in the comment block (can you also add 'type' to the exisiting ones as we are trying to up our game - ie @param array $inputData

[eileenmcnaughton]

@jaapjansma I took another look at this & as it is just the rule it seems much safer to change & there is quite a bit of cover to ensure the later regex doesn't let through weird things so I think that this is pretty solid & if we can get tests passing we can merge.

Note the old issue mentioned is a loosening not a tightening https://issues.civicrm.org/jira/browse/CRM-7122

[eileenmcnaughton]

@jaapjansma style warnings....

[jaapjansma]

@eileenmcnaughton the test failed but it is an api v4 failure which does not have anything to do with what I have changed (I hope at least). Not sure what to do now

[eileenmcnaughton]

Unrelated fail