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

Handle currency symbols with a period in fromUSDToNumber #561

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

Li357
Copy link
Contributor

@Li357 Li357 commented Jul 28, 2023

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.

@Li357 Li357 requested a review from a team as a code owner July 28, 2023 20:25
@github-actions
Copy link

github-actions bot commented Jul 28, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from dangrous and removed request for a team July 28, 2023 20:25
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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would. fromCurrencyToNumber maybe? Just need to make sure we replace it everywhere

@Li357
Copy link
Contributor Author

Li357 commented Jul 28, 2023

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

Couple comments and questions! GH checks are also a bit sad.

__tests__/Str-test.js Outdated Show resolved Hide resolved
lib/str.js Outdated Show resolved Hide resolved
lib/str.js Outdated Show resolved Hide resolved
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would. fromCurrencyToNumber maybe? Just need to make sure we replace it everywhere

@Li357
Copy link
Contributor Author

Li357 commented Jul 31, 2023

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 \d+\.?\d* for 5 or 5.233 or 5., or \.\d+ for no-leading-zero amounts. I've added more tests for success cases that match the current behavior. There are some cases that now are parsed with this new code, such as ($5.00 returns 500 instead of NaN like before but I don't see that as too important.

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.

@Li357 Li357 requested a review from dangrous July 31, 2023 19:37
Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

Looks good!

@dangrous dangrous merged commit 1076899 into main Aug 2, 2023
@dangrous dangrous deleted the andrewli-parsecurrency branch August 2, 2023 17:41
if (amount.match(/\(.*\)/)) {
const modifiedAmount = amount.replace(/[()]/g, '');
amount = `-${modifiedAmount}`;
fromCurrencyToNumber(amountStr, allowFraction) {
Copy link
Contributor

@techievivek techievivek Aug 21, 2023

Choose a reason for hiding this comment

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

It looks like changes introduced in this PR have bough this regression https://github.com/Expensify/Expensify/issues/309211

  1. If the amount field is not present in the CSV file, it throws an error, originally we used to default to 0.
  2. If the amount is negative the amount calculated is NaN and further defaulting to 0?
Screenshot 2023-08-21 at 4 41 23 PM Screenshot 2023-08-21 at 4 42 56 PM

On PROD

Screenshot 2023-08-21 at 4 58 26 PM

With current change

Screenshot 2023-08-21 at 4 58 46 PM

Copy link
Contributor

@techievivek techievivek Aug 21, 2023

Choose a reason for hiding this comment

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

This also seems to cause another regression here: https://github.com/Expensify/Expensify/issues/309227

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you have a negative per diem? Seems like defaulting to 0 in that case would be correct, right?

For the other, maybe something like changing this line to have a default case of 0?

Will defer to @Li357 though

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

Successfully merging this pull request may close these issues.

3 participants