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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 32 additions & 0 deletions __tests__/Str-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,38 @@ describe('Str.sanitizeURL', () => {
});
});

describe('Str.fromCurrencyToNumber', () => {
it('Handles negative amounts with minus sign', () => {
expect(Str.fromCurrencyToNumber('-$5.23')).toBe(-523);
expect(Str.fromCurrencyToNumber('$-5.23')).toBe(-523);
});

it('Handles negative amounts with ()', () => {
expect(Str.fromCurrencyToNumber('($5.23)')).toBe(-523);
});

it('Handles fractional cents when allowed', () => {
expect(Str.fromCurrencyToNumber('$5.223', true)).toBe(522.3);
});

it('Handles amounts without leading zeros', () => {
expect(Str.fromCurrencyToNumber('$.23')).toBe(23);
});

it('Handles amounts without cents', () => {
expect(Str.fromCurrencyToNumber('$5')).toBe(500);
});

it('Handles currency symbols with a period', () => {
expect(Str.fromCurrencyToNumber('Bs.S2.48')).toBe(248);
expect(Str.fromCurrencyToNumber('Bs.S-2.48')).toBe(-248);
expect(Str.fromCurrencyToNumber('-Bs.S2.48')).toBe(-248);
expect(Str.fromCurrencyToNumber('(Bs.S2.48)')).toBe(-248);
expect(Str.fromCurrencyToNumber('Bs.S.48')).toBe(48);
expect(Str.fromCurrencyToNumber('Bs.S2')).toBe(200);
});
});

describe('Str.isValidEmail', () => {
it('Correctly detects a valid email', () => {
expect(Str.isValidEmail('[email protected]')).toBeTruthy();
Expand Down
37 changes: 28 additions & 9 deletions lib/str.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,25 @@ const Str = {
},

/**
* Converts a USD string into th number of cents it represents.
* Converts a currency string into the number of cents it represents.
*
* @param {String} amountStr A string representing a USD value.
* @param {Boolean} allowFraction Flag indicating if fractions of cents should be
* allowed in the output.
* @param {String} amountStr String representing a currency symbol and value, like $4.02 or Bs.S97.9
* @param {Boolean} allowFraction Flag indicating if fractions of cents should be allowed in the output.
*
* @return {Number} The cent value of the @p amountStr.
*/
fromUSDToNumber(amountStr, allowFraction) {
let amount = String(amountStr).replace(/[^\d.\-()]+/g, '');
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

// eslint-disable-next-line no-param-reassign
amountStr = String(amountStr);

// Ideally, we would pass the currency symbol directly here the same string
// could be interpreted differently based on currency symbol but for now
// we match the amount from the end of the string.
let amount = amountStr.match(/(-?(?:\.\d+|\d+\.?\d*))\)?$/)[1];

// We want to support both -$5.00 and $-5.00, so we check if the string starts with -
if (amountStr.match(/\(.*\)/) || amountStr[0] === '-') {
amount = `-${amount}`;
}
amount = Number(amount) * 100;

Expand All @@ -50,6 +56,19 @@ const Str = {
return allowFraction ? amount : Math.round(amount);
},

/**
* Wrapper around fromCurrencyToNumber
*
* @deprecated
* @param {String} amountStr String representing a currency symbol and value, like $4.02 or Bs.S97.9
* @param {Boolean} allowFraction Flag indicating if fractions of cents should be allowed in the output.
*
* @return {Number} The cent value of the @p amountStr.
*/
fromUSDToNumber(...args) {
return Str.fromCurrencyToNumber(...args);
},

/**
* Truncates the middle section of a string based on the max allowed length
* @param {string} fullStr
Expand Down