Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new money formatting util for smarty formatting #22309

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Dec 23, 2021

Overview

Use new money formatting util for smarty formatting.

Note this does not affect formatting in most cases.

However, one important improvement is that when viewing a value in a dollar currency that differs from the site locale the currency is made explicit - making it possible to distinguish between (for example) USD and CAD at a glance in a way that is locale based.

Before

Smarty crmMoney function uses legacy formatting CRM_Utils_Money::format($amount, $currency)

Currency = USD
image

Currency = CAD
image

Currency = EUR
image

After

Switch to our new Civi::format()->money($amount, $currency) function

Currency = USD
image

Currency = CAD

image
Currency = EUR

image

Technical Details

Comments

@civibot
Copy link

civibot bot commented Dec 23, 2021

(Standard links)

@civibot civibot bot added the master label Dec 23, 2021
@yashodha
Copy link
Contributor

yashodha commented Dec 23, 2021

@eileenmcnaughton looks good to differentiate the currencies in the rows. We should extend the same to statistics as well.
Contact tab
stats_2

Search results
stats

@yashodha
Copy link
Contributor

and also update payment screen
update_payment

@eileenmcnaughton
Copy link
Contributor Author

@yashodha yes - this change would affect anything that goes through crmMoney smarty modifier

@JoeMurray
Copy link
Contributor

Where are these currency symbols coming from? I am used to ISO 4217 where currencies get a two letter prefix of the ISO two letter country code followed by a letter for the currency name, usually its first letter D for Dollar or Y for Yen, etc. E.g. CA$100.00 US$100.00 AU$100.00, or CAD$100.00 USD$100.00 AUD$100.00. I have never seen a standard that makes $ mean US dollar, CA$ mean Canadian dollar, and A$ mean Australian dollar.

@demeritcowboy
Copy link
Contributor

It's from Brick Money, but what comes out depends on your locale and the separator settings in the locale admin page. The above screenshots look like en_US, but e.g.

  • Civi::format()->money('10', 'USD', 'it_IT'); gives 10,00 USD
  • Civi::format()->money('10', 'USD', 'en_CA'); gives US$10.00
  • Civi::format()->money('10', 'USD', 'fr_CA'); gives 10,00 $ US (but might appear as 10.00 $ US depending on separator settings)
  • Civi::format()->money('10', 'CAD', 'en_CA'); gives $10.00 (note no currency identifier)

Leaving out the identifier makes sense in a single-currency install, but maybe not in a multi-currency install? I'm mixed opinion on the rest.

@eileenmcnaughton
Copy link
Contributor Author

Yeah - it changes according to your locale - my general feeling on that is that other people have put thought into what makes sense for the various locales and it is unlikely that we would come up with something that makes more sense

Note that the separator settings are ignored in favour of locale-sensible-separators if you use the define IGNORE_SEPARATOR_CONFIG - this should as good or better than using the civi settings (we will eventually remove it along with the settings)

@totten
Copy link
Member

totten commented Dec 30, 2021

@eileenmcnaughton Failure in CRM_Contribute_Form_Task_PDFLetterCommonTest::testPostProcessGroupByContact appears related.

(@JoeMurray) I have never seen a standard that makes $ mean US dollar, CA$ mean Canadian dollar, (etc)

(@demeritcowboy) ... what comes out depends on your locale ... The above screenshots look like (the locale is) en_US, ... Leaving out the (national) identifier makes sense in a single-currency install, but maybe not in a multi-currency install?

(@eileenmcnaughton) Yeah - it changes according to your locale - my general feeling on that is that other people have put thought into what makes sense for the various locales and it is unlikely that we would come up with something that makes more sense

So... Brick and Civi::format are trying to offer a compromise system that works across many different languages/currencies/user-roles/marketing-strategies/org-structures/etc. That is tricky...

I see Joe's point.... in some contexts, abbreviated $ formatting is going to be confusing. (Ex: Suppose a mid-sized regional org focuses on marine life in Lake Michigan - so it works with donors+staffers+funds in CA[ON] + US[MI,WI]. As a local resident in a border area, I would be aware of the two $ currencies - and I'd expect that $ ambiguity indicates trouble. If that system ever presents a bare $, then it will be awkward for some of those users. This differs from global orgs -- where I think users expect some software magic.*)

@demeritcowboy's suggestion sounds like an improvement. It still has some essential features of the current compromise -- it uses Brick's opinions for most language/currency issues (left/right placement, symbols, delimiters, etc), and it doesn't require you to fine-tune every caller. The improvement comes from using extra information about the org -- information that (AFAIK) isn't currently available to Brick.

@eileenmcnaughton hesitance to dig is also relatable -- this space is quite fiddly. I'm not sure I'd want this PR to depend on that issue.

Rephrase as a question: Should this subthread block the current PR? - or could it be forked off (eg to a separate lab.c.o or Brick Money issue)?

(My gut says to fork it off as a separate concern... Brick is already used in other parts of Civi. The current compromise in Brick gives better out-of-the-box behavior for more situations than the status-quo. But it could be better...)

This switches us from the old function to the new function
@eileenmcnaughton
Copy link
Contributor Author

The test is fixed now anyway!

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jan 1, 2022
@demeritcowboy demeritcowboy merged commit 5e1045c into civicrm:master Jan 4, 2022
@eileenmcnaughton eileenmcnaughton deleted the brickit branch January 4, 2022 21:40
@colemanw
Copy link
Member

colemanw commented Jan 8, 2022

@eileenmcnaughton CiviGrant is broken in 5.46 and I bisected my way to this commit. To reproduce:

  • Enable CiviGrant.
  • Create a new grant.
  • On the contact summary screen, "Grants" tab is broken.
  • The CIviGrant dashboard shows this:

image

@colemanw
Copy link
Member

colemanw commented Jan 8, 2022

@eileenmcnaughton this fixes it: #22427

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants