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

Financial#111: Contribution tokens always display amount with default… #18714

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

MegaphoneJon
Copy link
Contributor

… currency

https://lab.civicrm.org/dev/financial/-/issues/111

Overview

When using a contribution token that displays an amount (total amount, fee amount, net amount, non-deductible amount) the token formats the currency with the system default, regardless of the currency of the contribution.

Replication steps are on the ticket.

Before

Token renders amounts with the system default currency.

After

Token renders amounts with the currency of the contribution.

Comments

This seems fairly easy to review. Also, while submitting a Gitlab ticket for this, the auto-suggest actually pointed me to an existing ticket! I think that's the first time that's ever been successful for me.

@civibot
Copy link

civibot bot commented Oct 8, 2020

(Standard links)

@civibot civibot bot added the master label Oct 8, 2020
@demeritcowboy
Copy link
Contributor

I can take a look.
With gitlab autosuggest what's embarassing is when it suggests one of your own tickets you forgot about...

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS but missing one item unless I missed it
      • Can replicate the problem, although the first time I did it I was confused because of course I used CAD and everything looked fine, but ha ha it's the same symbol as USD. So, the currency has to be one that has a different symbol from the default currency.
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Separate issue: There's some deprecated warnings coming up from a recent commit but I'll make a lab ticket.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • Separate issue: That recursive helper looks like it has a lurking bug in it when used in another context, just it would probably usually get cast in a way where it wouldn't make a difference, like here. A legitimate 0 gets returned as null. But here it works out in the end anyway.
    • [r-maint] Undecided
      • Probably, ideally.
    • [r-test] PASS

@seamuslee001
Copy link
Contributor

I've added a unit test in #18715 to cover for this and merging based on @demeritcowboy 's review

@seamuslee001 seamuslee001 merged commit 0d193d5 into civicrm:master Oct 9, 2020
artfulrobot pushed a commit to artfulrobot/civicrm-core that referenced this pull request Oct 12, 2020
Financial#111: Contribution tokens always display amount with default…
@eileenmcnaughton
Copy link
Contributor

Noting this caused a regression
https://lab.civicrm.org/dev/core/-/issues/2344

although it's pretty obscure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants