CRM-21562 fix mis-saving when using Euro thousand separator

[eileenmcnaughton]

Overview

When we have a thousand separator of ',' then the line items (and amount passed to the payment processor) is massively incorrect. Test demonstrates this. Fix is removal of a line of 'misfix' from https://issues.civicrm.org/jira/browse/CRM-10974 - but will require more testing.

In testing I changed multiple tests to work with both separator types & found several places that were not working very well

Before

Line item of 10,0000000 with comma separator of ',' mis-converted to 100000000 when saving the line item

After

Line item correctly calculated - I tested using a text field with the amount being 2650.50 & tried it with both European & Anglo versions of the separators. The numbers still appeared correctly & I saw no repeat of CRM-10974 and CRM-21350 also appeared fixed by the change

Technical Details

The line_item format function calls 'cleanMoney' on a non-localised currency string, treating it as localised. This causes it to wipe out the legitimate decimal point. This is an inappropriate place (too deep) to be dealing with localised strings of code and in fact they are not. The passed in value of $amount_override has only recently started to be used & may not even be in the release version, but, if passed in it should be localised. I have some doubts about the logic of that param -but they are outside the scope of this

---

• CRM-21562: Currencies (like Euro) with a comma for thousand separator are being seriously mishandled

---

• CRM-21350: Dummy payment processor throws error with different decimal delimitor than .

[monishdeb]

I tested the patch in my local keeping thousand and decimal separator as '.' and ',' respectively. And tested it for Sale Tax, partial payment, payment using payment processor on Contribution and Event Registration page. Works fine for me.

However, I found a minor issue in 'Record Payment' form, which is not related though. Irrespective of the separator setting the default amount always formatted and set to 10000.00 which later throws a form rule error.

[monishdeb]

On further investigation I have ensured that in codebase the $amount_override carry the value from $params['partial_payment_total'] which is always in 1000.00 format as formatted by CRM_Utils_Rule::cleanMoney($params['partial_payment_total']) or DB amount value used

These are two important points where partial_payment_total parameter is assigned:
https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/BAO/Contribution.php#L3191
https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Form/Participant.php#L1431L1442

[mlutfy]

@magnolia61 Did you test this patch?

[magnolia61]

I tested this patch against decimal delimiter . and thousands , and vice versa.
Both with contribution pages and event registrations and complex price sets.
All works like a charm!

[monishdeb]

@eileenmcnaughton there are two related test failures and this will fix those :

diff --git a/CRM/Contribute/BAO/Contribution/Utils.php b/CRM/Contribute/BAO/Contribution/Utils.php
index 9e7995a..3286849 100644
--- a/CRM/Contribute/BAO/Contribution/Utils.php
+++ b/CRM/Contribute/BAO/Contribution/Utils.php
@@ -500,7 +500,9 @@ LIMIT 1
   public static function calculateTaxAmount($amount, $taxRate, $ugWeDoNotKnowIfItNeedsCleaning_Help = FALSE) {
     $taxAmount = array();
     if ($ugWeDoNotKnowIfItNeedsCleaning_Help) {
-      Civi::log()->warning('Deprecated function, make sure money is in usable format before calling this.', array('civi.tag' => 'deprecated'));
+      if (CIVICRM_UF !== 'UnitTests') {
+        Civi::log()->warning('Deprecated function, make sure money is in usable format before calling this.', array('civi.tag' => 'deprecated'));
+      }
       $amount = CRM_Utils_Rule::cleanMoney($amount);
     }
     // There can not be any rounding at this stage - as this is prior to quantity multiplication

[eileenmcnaughton]

@monishdeb I put that there so the unit tests would fail & we could repair any places which were tested (since we can do that with some confidence)

[eileenmcnaughton]

@monishdeb @colemanw @mlutfy I think this is good to merge now

[eileenmcnaughton]

I'm going to merge this based on @monishdeb review & @magnolia61 testing. I feel like this needs to be merged ASAP since it is the main driver for the Wed release & we really have it core for a couple of days before it goes out.

[monishdeb]

Cool, agree with the last change made for fixing test failure.