Skip to content

fix: IOU - On entering amount with decimals, 3rd decimal 0 is added in IOU confirmation page. #44416

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.

Sign up for GitHub

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

Merged
thienlnam merged 6 commits into from
Jul 10, 2024
Merged

fix: IOU - On entering amount with decimals, 3rd decimal 0 is added in IOU confirmation page. #44416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
98ebb2b
fix: IOU - On entering amount with decimals, 3rd decimal 0 is added i…
Krishna2323 Jun 25, 2024
d8054b9
fix for android native.
Krishna2323 Jun 25, 2024
96d38de
fix for android native.
Krishna2323 Jun 25, 2024
5c09225
Merge branch 'Expensify:main' into krishna2323/issue/43945
Krishna2323 Jul 9, 2024
189c3a3
Merge branch 'Expensify:main' into krishna2323/issue/43945
Krishna2323 Jul 9, 2024
0747007
fix tests.
Krishna2323 Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/libs/CurrencyUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,11 @@ function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURR
style: 'currency',
currency: currencyWithFallback,

// We are forcing the number of decimals because we override the default number of decimals in the backend for RSD
// We are forcing the number of decimals because we override the default number of decimals in the backend for some currencies
// See: https://github.com/Expensify/PHP-Libs/pull/834
minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined,
minimumFractionDigits: getCurrencyDecimals(currency),

@Krishna2323 Krishna2323 Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rayane-djouah, we also need to set minimumFractionDigits, passing maximumFractionDigits without minimumFractionDigits crashes the app on android native. I believe we can simply use getCurrencyDecimals(currency), since it returns the correct value for currencies and the max is 2.

@Krishna2323 Krishna2323 Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, test is failing, I will check for other solutions tomorrow.

@rayane-d rayane-d Jun 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason for the crash on Android native? What error are you getting?

@Krishna2323 Krishna2323 Jun 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rayane-djouah, no error is shown, the app just closes without providing minimumFractionDigits.

@rayane-d rayane-d Jun 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you try to see in Android studios Logcat what error causes the app to crash?

@Krishna2323 Krishna2323 Jul 1, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rayane-djouah, sorry for delay, I tried but couldn't find Android studios Logcat, I haven't used it till now, can you pls tell me where I can find it?

@rayane-d rayane-d Jul 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Select View > Tool Windows > Logcat from the menu bar.

https://developer.android.com/studio/debug/logcat
https://developer.android.com/studio/debug/stacktraces

@rayane-d rayane-d Jul 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried debugging the crash, but I saw no error.
Looks like this was faced before here: #43128 (comment)

Based on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat#minimumfractiondigits :

the default for currency formatting is the larger of minimumFractionDigits and the number of minor unit digits provided by the ISO 4217 currency code list (2 if the list doesn't provide that information)

So it has the same behavior as getCurrencyDecimals 👍 We should be safe to use it instead of undefined

// For currencies that have decimal places > 2, floor to 2 instead as we don't support more than 2 decimal places.
maximumFractionDigits: 2,
Comment thread
Krishna2323 marked this conversation as resolved.
});
}

Expand Down Expand Up @@ -175,9 +177,11 @@ function convertToDisplayStringWithoutCurrency(amountInCents: number, currency:
style: 'currency',
currency,

// We are forcing the number of decimals because we override the default number of decimals in the backend for RSD
// We are forcing the number of decimals because we override the default number of decimals in the backend for some currencies
// See: https://github.com/Expensify/PHP-Libs/pull/834
minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined,
minimumFractionDigits: getCurrencyDecimals(currency),
// For currencies that have decimal places > 2, floor to 2 instead as we don't support more than 2 decimal places.
maximumFractionDigits: 2,
})
.filter((x) => x.type !== 'currency')
.filter((x) => x.type !== 'literal' || x.value.trim().length !== 0)
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/CurrencyUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,8 @@ describe('CurrencyUtils', () => {
['JPY', 2500.5, '¥25'],
['RSD', 100, 'RSD\xa01.00'],
['RSD', 145, 'RSD\xa01.45'],
['BHD', 12345, 'BHD\xa0123.450'],
['BHD', 1, 'BHD\xa00.010'],
['BHD', 12345, 'BHD\xa0123.45'],
['BHD', 1, 'BHD\xa00.01'],
])('Correctly displays %s', (currency, amount, expectedResult) => {
expect(CurrencyUtils.convertToDisplayString(amount, currency)).toBe(expectedResult);
});
Expand Down