dev/core#226 : use decimal point from config to fix european numbers formating

[sunilpawar]

Overview

Format Amount using hardcoded decimal point, which is not fit for european format.

[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 wanna input on this?

Also looks like it needs a test....

[mattwire]

@sunilpawar @eileenmcnaughton I'll review properly on Thursday. However first thoughts:

1. We shouldn't really have a formatting function in the BAO object. How easy/sensible is it to move this to CRM_Utils_Money?
2. Can we use the existing CRM_Utils_Money::format() function here instead?

[sunilpawar]

@mattwire we can do this in format function

i tried with attached patch , it work for me.
number_format.txt

[mattwire]

@sunilpawar Ok, I'm not completely understanding the issue - I thought that CRM_Utils_Money::format(onlyNumber=TRUE) would return the formatting you require without requiring any changes.
It's a bit of a messy function and we can't really make changes to it as the risk of breaking things is too high.

• What is wrong with the format it returns?
• What pages/forms are you testing on where the values are not formatted correctly - can it be solved just by using crmMoney in the smarty templates?

[sunilpawar]

@mattwire
CRM_Utils_Money::format(onlyNumber=TRUE) not working properly because setlocale is not present there.
Here is Test Scenario.

• Use comma (,) as decimal separator, Thousands Separator 'space' in localisation.

• Register for Event (offline), (update action) then change fee selection -> choose lower fee option -> save.

• It will update payment record status to 'Pending refund', then in Contribution tab click on 'Record Refund' link. 'Refund Due' amount has be in localise format.

• Repeat same for another event with change fee selection with higher fee. This will change payment status to 'Partially paid'. You will see 'Record Payment' link, Balance Amount has be in localise format.

• In Both cases : CRM/Contribute/Form/AdditionalPayment.php file get used.

crmMoney is not going to work. (it calls: CRM_Utils_Money::format(onlyNumber = FALSE))
it add currency symbol to amount when onlyNumber = FALSE (we do not need symbol)

I think we have two options

1. modify CRM_Utils_Money::format(onlyNumber=TRUE) use setlocale
2. Go with this PR (with additional changes if required).

[mattwire]

@sunilpawar I did some work a while ago here to add a set of money formatters to CiviCRM: master...mattwire:money_formatters
But I never had time to finish the unit tests and did not have a specific example that it was trying to fix.

The function formatLocaleNumeric would do exactly what you require I think?

From my point of view I think that:

1. We cannot modify CRM_Utils_Money::format without high risk of breaking things.
2. All money formatting functions should be in CRM_Utils_Money (so CRM_Contribute_BAO_Contribution_Utils::formatAmount should be removed/replaced with something in CRM_Utils_Money).

Can you take a look at the code in the branch I've linked to and then propose what you think is the best way to solve this issue?

[eileenmcnaughton]

So a few thoughts

1. @sunilpawar can you update the PR description - there is a lot of info in the comments but the original PR description is not covering that info
2. any money formatting functions should be in CRM_Utils_Money - we should aim to remove (or at least actively deprecate) the money formatting from CRM/Contribute/BAO/Contribution/Utils.php as part of this. It seems to be otherwise used in createProportionalEntry (@monishdeb )
3. adding a new function is preferable to adding a new parameter to an existing one unless there is a really cogent case for adding a new parameter (ie. we have a 'habit' of making functions do very different things depending on input variables & that is not a great pattern). If we look at the existing
   CRM_Utils_Money::format() function there is relatively little overlap between the behaviour when $onlyNumber = FALSE is false and $onlyNumber = TRUE. The 2 behaviours SHOULD be separate functions (& we would ideally try deprecating passing $onlyNumber=TRUE to the money::format function
4. I'm trying to get my head around whether we should be taking account decimal places here at all - mostly in terms of what the underlying function needs to support. I guess we can punt that but it's a question for you really @mattwire in terms of 'if we add the 3 functions below, do they adequately support our projected need to support different numbers of decimal places for different currencies, given that the currency is not 'site-wide' but may be per-transaction and need to be passed in.
5. tests...

  /**
   * Format money for display (just numeric part) according to the current locale
   * Example 12,34 or 12.34
   *
   * @param $amount
   *
   * @return string
   */
  public static function formatLocaleNumeric($amount) {
    $config = CRM_Core_Config::singleton();
    $format = $config->moneyvalueformat;
    return self::formatNumeric($amount, $format);
 }

  /**
   * Format money for display (just numeric part). Specify format or use formatLocaleNumeric() instead.
   * Example: Depends on parameters it is called with
   *
   * @param $amount
   * @param $valueFormat
   *
   * @return string
   */
  public static function formatNumeric($amount, $valueFormat) {
    if (CRM_Utils_System::isNull($amount)) {
      return '';
    }

    $moneyFormatExists = self::moneyFormatExists();
    if (is_numeric($amount) && $moneyFormatExists) {
      $lc = setlocale(LC_MONETARY, 0);
      setlocale(LC_MONETARY, 'en_US.utf8', 'en_US', 'en_US.utf8', 'en_US', 'C');
      $amount = money_format($valueFormat, $amount);
      setlocale(LC_MONETARY, $lc);
    }
    return $amount;
  }

   /**
   * Format a monetary string.
   * Warn if php money_format() doesn't exist as they are likely to experience issues displaying currency.
   * @return bool
   */
  private static function moneyFormatExists() {
    // money_format() exists only in certain PHP install (CRM-650)
    if (!function_exists('money_format')) {
      Civi::log()->warning('PHP money_format function does not exist. Monetary amounts may not format correctly for display.');
      return FALSE;
    };
    return TRUE;
  }

[JoeMurray]

@mlutfy can give his 2 cents, but from my perspective we should never store a localized monetary amount. All processing of amounts should remain in a standard format, and only be localized to a different currency on display at presentation layer. http://userguide.icu-project.org/formatparse/numbers#TOC-Currency-Formatting provides a sample of how ICU implements currency localization.

[eileenmcnaughton]

Hmm - I think we need a screencast added to this PR too to make it really clear what we are talking about. I tried to replicate what you said below and it correctly changes the Contribution status to partially paid
- as you describe - this seems like CORRECT behaviour - so what are we trying to fix?

I have some code notes (below) but I think we need to clarify the bug description before worrying more about the code

The form should PRESENT money in a localised format but convert it as soon as the form is submitted. Only the standard decimal formatted currency should be used once submitted - ie
the AdditionalPayment form calls a function which does this

    foreach ($this->submittableMoneyFields as $moneyField) {
      if (isset($this->_params[$moneyField])) {
        $this->_params[$moneyField] = CRM_Utils_Rule::cleanMoney($this->_params[$moneyField]);
      }
    }

Looking at where $this->_refund comes from - it is from
``
$paymentInfo = CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType($this->_id, $entityType);
$paymentDetails = CRM_Contribute_BAO_Contribution::getPaymentInfo($this->_id, $this->_component, FALSE, TRUE);

$this->_amtPaid = $paymentDetails['paid'];
$this->_amtTotal = $paymentDetails['total'];

if (!empty($paymentInfo['refund_due'])) {
  $paymentAmt = $this->_refund = $paymentInfo['refund_due'];
  $this->_paymentType = 'refund';
}

And it appears to be correctly in 'db-formatted' format  - from what I understand

[sunilpawar]

@eileenmcnaughton @mattwire here screencast.

[eileenmcnaughton]

@sunilpawar OK - so this is equivalent to how it's done on the contribution form

    if ($this->_refund) {
      $defaults['total_amount'] = CRM_Utils_Money::format(abs($this->_refund), NULL, '%a');
    }
    elseif ($this->_owed) {
      $defaults['total_amount'] = CRM_Utils_Money::format($this->_owed, NULL, '%a');
    }

I note that the current formatting was set in https://issues.civicrm.org/jira/browse/CRM-20899 - but that seems unbroken by the above.

[eileenmcnaughton]

@sunilpawar I don't think this is mergeable / reviewable as is - from your screencast the issue is the formatting is not done in the setDefaults function - which is a safe place to do it. I feel like you had a bunch of time on upstreaming but not enough to follow through to get them merged.

If you don't have more time in the net couple of weeks we should close the PRs & track in gitlab until we have something review-ready

[eileenmcnaughton]

I think this #12626 is actually a fix for the same bug - but fix is inline with above comments

[mattwire]

@sunilpawar Can you do a fix on top of #12626 for this?

[sunilpawar]

@mattwire i can implement this I tested this and its working.

[eileenmcnaughton]

@sunilpawar once @mattwire has rebased #12684 can you test it - I think once that is done we'll be well placed on the currency separator formatting