Handle currency symbols with a period in fromUSDToNumber #561
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
lib/str.js
Outdated
| * @param {Boolean} allowFraction Flag indicating if fractions of cents should be | ||
| * allowed in the output. | ||
| * allowed in the output. | ||
| * | ||
| * @return {Number} The cent value of the @p amountStr. | ||
| */ | ||
| fromUSDToNumber(amountStr, allowFraction) { |
There was a problem hiding this comment.
Before we merge, we should consider if we want to rename this method since it now does double-duty by handling all types of currency strings (symbols + values) instead of just USD.
There was a problem hiding this comment.
Yeah I would. fromCurrencyToNumber maybe? Just need to make sure we replace it everywhere
|
I have read the CLA Document and I hereby sign the CLA |
lib/str.js
Outdated
| * @param {Boolean} allowFraction Flag indicating if fractions of cents should be | ||
| * allowed in the output. | ||
| * allowed in the output. | ||
| * | ||
| * @return {Number} The cent value of the @p amountStr. | ||
| */ | ||
| fromUSDToNumber(amountStr, allowFraction) { |
There was a problem hiding this comment.
Yeah I would. fromCurrencyToNumber maybe? Just need to make sure we replace it everywhere
|
Good catches, thanks! Turns out this was a little more annoying than I though. I've changed the regex to match based on the end of the amount string to match either Ideally, we'd pass the currency symbol because there might be some currency symbol that ends with a period that throws off this scheme but for now 🤷 I'll put in PRs in a bit to do the rename. |
| if (amount.match(/\(.*\)/)) { | ||
| const modifiedAmount = amount.replace(/[()]/g, ''); | ||
| amount = `-${modifiedAmount}`; | ||
| fromCurrencyToNumber(amountStr, allowFraction) { |
There was a problem hiding this comment.
It looks like changes introduced in this PR have bough this regression https://github.com/Expensify/Expensify/issues/309211
- If the amount field is not present in the CSV file, it throws an error, originally we used to default to 0.
- If the amount is negative the amount calculated is
NaNand further defaulting to 0?
On PROD
With current change
There was a problem hiding this comment.
This also seems to cause another regression here: https://github.com/Expensify/Expensify/issues/309227
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/303703
Tests
Added a unit test for Bs.S, the Venezuelan currency that has a period in the symbol itself. Before, this was parsing as NaN.
QA
In OldDot, Verify that when changing Settings > Policies > Reports > Report Currency to Bs.S then going to Settings > Policies > Expenses > Time and setting the Default Hourly Rate to Bs.S6.00 correctly autosaves.