dev/core#1019 Fix currency formatting of Total Amount (with multi-currency form support)

[mlutfy]

• Followup to: Fix currency symbol for Total Amount on contribution page #17703
• Original issue: https://lab.civicrm.org/dev/core/-/issues/1019

Overview

Many/most non-English locales place the currency symbol after the numeric amount.

#16487 tried to fix this, but presumably caused a regression for contribution pages that use a currency other than the default site currency, which @mattwire reverted in #17703.

So for example, on an Event Registration page with a PriceSet, the currency amount is incorrectly displayed before the amount:

To reproduce:

• Go to Administer > Localization > Language, and set %a %c as the currency formatting option.
• Setup a contribution/event page with a priceset (so that "Total Amount" is displayed).

@mattwire Can you test if this fix works for you?

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

[mattwire]

@mlutfy Sorry I broke it! I'm not sure this is a good fix though. That CRM.formatMoney javascript function is pretty scary - it seems to be guessing the format based on an already formatted value.

I think what we really need to do is assign the currency, amount and format to the template and use a proper formatting library to do the actual formatting - for example http://openexchangerates.github.io/accounting.js/

The general consensus seems to be that currency should be formatted based on the users locale rather than the currency itself (https://ux.stackexchange.com/questions/22574/where-to-place-currency-symbol-when-localizing-and-what-to-do-with-odd-symbols) so a language with both French Canadian and English as locales should probably display the symbol after and before respectively.

Hopefully whatever we choose will help remove nasty hacks like this (which won't work with the currency symbol after): https://lab.civicrm.org/extensions/mjwshared/-/blob/master/js/crm.payment.js#L61

[mlutfy]

Agree that the code of CRM.formatMoney is ugly, but using a separate library seems like a separate issue. The library in itself will not help decide which currency format should be used (ex: currency symbol before or after).

The locale should be set by the page, not by the browser (I know that Stripe elements or Paypal sometimes use the browser locale, but that often means weird mixes of languages in the page).

• previously: currency position (moneyFormat) and currency symbol was initialized once
• your previous PR: use the currency symbol of the form, but hardcode the position (moneyFormat)
• this PR: unhardcode the moneyFormat.

[mattwire]

The locale should be set by the page, not by the browser (I know that Stripe elements or Paypal sometimes use the browser locale, but that often means weird mixes of languages in the page).

Agree! Stripe elements picks up the locale based on the current page locale since a few versions now :-)

this PR: unhardcode the moneyFormat.

Looking back at mine it's obvious that won't work in all situations. But what I really don't like is assigning a formatted amount to the template and then doing a load of work on it to work out how to format another amount. I know this seems simple but the currency formatting issue just keeps coming up over and over again in so many different places and we really need to solve it properly!

Another library which looks nice is: https://github.com/dinerojs/dinero.js - that just requires amount, currency + locale and it does the rest for you.

[mlutfy]

"Looking back at mine it's obvious that won't work in all situations. But [...]"

I understand that non-English installs are a minority, but it will stay that way if we keep breaking other languages.

I'm very much in favour of using a JS library, but I don't think it will fix the smarty mess?

With Dinero, how would we support situations where the contribution page uses EUR, but the site default is en-UK / GBP? With Dinero, we will need to provide it with the moneyFormat.

Example from Dinero:

// returns 5 000 $US
Dinero({ amount: 500000 })
  .setLocale('fr-FR')
  .toFormat('$0,0')

I agree any money PR is risky, since the contribution/event pages support a ton of different use-cases that are difficult to test. We are deploying this patch to all our clients, so hopefully we'll catch any regressions. The main use-case we don't often use is multi-currency (although one of our partner clients does, so I'll ask them to test).

[mattwire]

@bgm I didn't look into the library in too much detail but I thought you could just pass it currency and locale and it would do the rest - but I am not sure.

I have commented on mattermost - I am happy for this PR to be merged once tested as it fixes a regression but I'd like us to look at a decent currency formatting library for the client/js side soon!

[eileenmcnaughton]

@mlutfy @mattwire - this should really be against 5.33 / ported to 5.32.

Can we add comments to the code to make the temporary nature of the fix clear?

We are dropping 5.32 as soon as @totten has time - we won't hold it up for this but will include it if ready by then

[seamuslee001]

closing as no longer needed

[eileenmcnaughton]

No sure it's clear but not needed because it was merged via the rc