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

MPDX-7786 Don't round donations #833

Merged
merged 3 commits into from
Dec 14, 2023
Merged

MPDX-7786 Don't round donations #833

merged 3 commits into from
Dec 14, 2023

Conversation

caleballdrin
Copy link
Contributor

@caleballdrin caleballdrin commented Dec 5, 2023

Description

https://jira.cru.org/browse/MPDX-7786
The donations should always be shown with 2 decimal places unless it's '.00' in which case it can just be rendered without any decimal places.

I think we could get away with only doing this fix on the New MPDX since we're hopefuilly moving away from the old version soon.

Example pages this will need to be fixed on:
Contacts donation
Donations page
Tools
Reports

Changes I made

  • Change formatCurrency to show 2 decimal places but remove trailing 0's
  • Add formatCurrencyNoSymbol to format the number without the currency symbol
  • Replace math.round with the currency formatters

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@caleballdrin caleballdrin changed the title Dont round MPDX-7786 Don't round donations Dec 5, 2023
@caleballdrin caleballdrin requested review from dr-bizz and canac December 5, 2023 06:33
@caleballdrin caleballdrin self-assigned this Dec 5, 2023
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-833.d3dytjb8adxkk5.amplifyapp.com

canac
canac previously requested changes Dec 5, 2023
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're already touching all the places where currency is formatted, we should be using currencyFormat whenever we're dealing with money instead of manually appending the currency code like some components currently do.

@caleballdrin
Copy link
Contributor Author

@canac In some places we want to have both the currency symbol and the currency code. For example: $10 USD.

Other places we don't want the symbol in front of the number. Mainly because some countries don't have a symbol so it does this: UGX 10 UGX

On places like the donation report and 14 month report, there isn't much space and the numbers get cut off when there is a currency with no symbol and it shows the currency code twice.

I don't think there is a way to set the type to currency and then turn off the symbol. That is why I made formatCurrencyNoSymbol.

@canac
Copy link
Contributor

canac commented Dec 5, 2023

@caleballdrin Sorry, I'm still a little confused.

In some places we want to have both the currency symbol and the currency code. For example: $10 USD.

Why is that? Why don't we just trust the browser to do this for us? Also, that looks fine for English but looks weird in locales that put the symbol after the amount, i.e. new Intl.NumberFormat('de', { style: 'currency', currency: 'USD' }).format(10) + currency shows 10,00 $ USD.

Other places we don't want the symbol in front of the number. Mainly because some countries don't have a symbol so it does this: UGX 10 UGX

Why don't we want it to display like this with only one symbol: 10 UGX?

On places like the donation report and 14 month report, there isn't much space and the numbers get cut off when there is a currency with no symbol and it shows the currency code twice.

I see your point about not putting symbols in the table because there's not much space, but I'm still confused why we'd display an amount like $10 USD (with both the symbol and the currency code). Is it to make extra clear which currency we mean because some amounts refer to foreign currencies?

@caleballdrin
Copy link
Contributor Author

caleballdrin commented Dec 5, 2023

@caleballdrin Sorry, I'm still a little confused.

In some places we want to have both the currency symbol and the currency code. For example: $10 USD.

Why is that? Why don't we just trust the browser to do this for us? Also, that looks fine for English but looks weird in locales that put the symbol after the amount, i.e. new Intl.NumberFormat('de', { style: 'currency', currency: 'USD' }).format(10) + currency shows 10,00 $ USD.

We don't have to have that I suppose. Americans are used to seeing the $ sign in front of money and that is the way it currently is. We could trust the browser, but there are certain places where we want to make it clear what the currency is. Since some currencies use the same symbol. So in most places we want the code 'USD' to distinguish and in other places it is not necessary so just the code is enough.

Other places we don't want the symbol in front of the number. Mainly because some countries don't have a symbol so it does this: UGX 10 UGX

Why don't we want it to display like this with only one symbol: 10 UGX?

This is exactly the issue. We can't make something like 10 UGX with Intl.NumberFormat('ug', { style: 'currency', currency: 'UGX'. This is because by using style:'currency' it will automatically put the UGX at the front.

On places like the donation report and 14 month report, there isn't much space and the numbers get cut off when there is a currency with no symbol and it shows the currency code twice.

I see your point about not putting symbols in the table because there's not much space, but I'm still confused why we'd display an amount like $10 USD (with both the symbol and the currency code). Is it to make extra clear which currency we mean because some amounts refer to foreign currencies?

Yes, we want it to be clear when there are multiple currencies. Since some share the same symbol.

@canac
Copy link
Contributor

canac commented Dec 5, 2023

Since some currencies use the same symbol.

If that's true, we definitely want to disambiguate like you're saying. But do you have an example? I know that, for example, USD and CAD are shown differently (e.g. $10 vs CA$10, respectively).

This is exactly the issue. We can't make something like 10 UGX with Intl.NumberFormat('ug', { style: 'currency', currency: 'UGX'. This is because by using style:'currency' it will automatically put the UGX at the front.

I'm suggesting letting the browser handle whether to put it in the front or at the end based on the ISO formatting rules for that currency.

Yes, we want it to be clear when there are multiple currencies. Since some share the same symbol.

I agree in theory, but I wasn't able to find any currencies that use the same symbol.

Thanks for clarifying. I trust your judgement, so feel free to leave it the way you have it if you think that's best. It just seems more accurate to let the browser handle currency formatting the correct way for each currency instead of universally putting the symbol after the amount. I'm assuming that there aren't any ambiguous symbols since I used a script to check all currency codes listed here and none produced identical output after formatting.

@canac canac dismissed their stale review December 5, 2023 17:47

Feel free to use your approach if you prefer

@caleballdrin
Copy link
Contributor Author

caleballdrin commented Dec 5, 2023

If that's true, we definitely want to disambiguate like you're saying. But do you have an example? I know that, for example, USD and CAD are shown differently (e.g. $10 vs CA$10, respectively).

I agree in theory, but I wasn't able to find any currencies that use the same symbol.

When you click 'Add New Donation' the modal gives you the option to choose a currency. It shows both the currency code and the symbol. Many symbols use $. Like TTD, SRD, SGD, NZD, NAD, MXN all use the $. But I think you are right that when it actually formats it, it adds the letters in addition to the $. So maybe that is enough to distinguish.

I wonder if we need to talk to someone higher up about this decision. Because it seems kind of weird that they haven't used the symbols all this time. And instead, they manually put the currency code.

@canac
Copy link
Contributor

canac commented Dec 5, 2023

Good point about the new donation modal. Those options come from the server constants, which isn't based on the browser's i18n data. There are duplicates in those symbols, like you pointed out. Maybe we can ask Scott what he thinks at standup this afternoon.

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Almost there!

@caleballdrin caleballdrin requested a review from canac December 7, 2023 19:43
@caleballdrin caleballdrin added the On Staging Will be merged to the staging branch by Github Actions label Dec 7, 2023
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment from something I missed last time. Also, the test for the NaN case is failing.

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing! Thank you for doing this! It looks great.
Screenshot 2023-12-08 at 2 15 41 PM

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work pushing this across the finish line! 🎉

@caleballdrin caleballdrin merged commit 3169f95 into main Dec 14, 2023
14 of 15 checks passed
@caleballdrin caleballdrin deleted the dont-round branch December 14, 2023 21:03
canac added a commit that referenced this pull request Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants