[HOLD for payment 2023-10-13] [$500] Chat - App does not allow pasting number if numbers has comma in end for english and vice versa

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the app
2. Open any report
3. If language is English, write any number and write full stop in the end and copy it eg: '2000.'
4. Paste text in plus->request money and observe that it allows to paste the text
5. Similarly try copy number with comma in middle and observe that app allows to paste
6. Similarly, try copy number with comma in end and observe that now app does not allow to paste the number
7. If language is Spanish, follow all the above steps just use full stop in place of comma

Expected Result:

App should allow copy pasting number in request money with comma in end as it allows copy paste of number with comma in middle or copy paste of number with full stop in end (English language)

Actual Result:

App does not allow copy pasting number in request money with comma in end for English language and it doesn't allow copy paste number with full stop in end for Spanish language

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.74-2

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation:

windows.chrome.comma.or.fullstop.in.english.or.spanish.in.end.does.not.paste.amount.mp4

Recording.115.mp4

android.native.does.not.allow.to.paste.numbers.with.comma.1.mp4

Ipste.mp4

Ipse2.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695835603519559

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017fcdbf8f05309fae
• Upwork Job ID: 1708219578554970112
• Last Price Increase: 2023-09-30
• Automatic offers:
• Ollyws | Reviewer | 26999991
• c3024 | Contributor | 26999997
• dhanashree-sawant | Reporter | 26999998

[c3024]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pasting amount like 20, is not shown in the new request amount page.

What is the root cause of that problem?

We are using this regex here

App/src/libs/MoneyRequestUtils.ts

Lines 45 to 48 in bfda42d

	function validateAmount(amount: string): boolean {
	const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,2})?$/, 'i');
	return amount === '' || (decimalNumberRegex.test(amount) && calculateAmountLength(amount) <= CONST.IOU.AMOUNT_MAX_LENGTH);
	}

to validate the amount and this regex requires at least a digit to be followed by comma to be valid.
If the amount is not valid it just sets the previous amount.

What changes do you think we should make in order to solve the problem?

I think since we are allowing 20. as valid it is reasonable to allow 20, also as valid.
Regex for this would be

const decimalNumberRegex = new RegExp(/^\d+(,\d*)?(\.\d{0,2})?$/, 'i');

This accepts 20, but disallows 20,,, this is in line with the present regex allowing 20. but not 20...
We can change the regex if we need a different vaidation

What alternative solutions did you explore? (Optional)

[melvin-bot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017fcdbf8f05309fae

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External)

[Krishna2323]

Proposal

Please re-state the problem that we are trying to solve in this issue.

App does not allow pasting number if numbers has comma in end for english and vice versa.

What is the root cause of that problem?

The root cause of the problem lies in the regular expression used for validation. The current regex pattern doesn't account for numbers that end with a comma.

App/src/libs/MoneyRequestUtils.ts

Line 46 in bfda42d

const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,2})?$/, 'i');

What changes do you think we should make in order to solve the problem?

To address this issue, we should update the regular expression used for number validation to accommodate numbers with a trailing comma.

const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,2})?,?$/, 'i');

Result

[Ollyws]

@c3024 @Krishna2323 Thanks for the proposals, modifying this regex works but can either of you tell me why pasting a trailing comma works for when the language is set to English but not to Spanish?

[c3024]

, and . are interchanged in Spanish. So we can paste 20, in Spanish but not 20. @Ollyws

[c3024]

With the existing regex 20, works for Spanish but not English. 20. works for English but not for Spanish.
With the modified regex 20, and 20. work for both Spanish and English.

why pasting a trailing comma works for when the language is set to English but not to Spanish?

Which regex does this? As far as I see, existing regex allows 20, for Spanish but not for English. New regex allows 20, for both.
I don't see a case where 20, works for English but not Spanish like you mentioned. @Ollyws

[Ollyws]

@c3024 Thanks for the explanation. Sorry I meant Spanish but not to English?.
I think we need to define which punctuation is acceptable to have at the end here.
Because @c3024, your regex will allow 1,.
and @Krishna2323 your regex will allow 1.,
Not that it's a huge issue, just seems a little inconsistent to allow ,. and not .,, preferably just either , or . would be better.

[c3024]

Once it passes the validity check ',' ('.') are stripped in English (Spanish) here

App/src/pages/iou/steps/MoneyRequestAmountForm.js

Line 140 in 00933e1

const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);

So I think allowing (or disallowing) 1,. (or for that matter 1.,) is a non-issue. Restricting (or allowing) regex for these cases is not necessary in my opinion.

But if we want to restrict our regex to allow only , and . we can use this regex

const decimalNumberRegex = new RegExp(/^(\d+(,\d+)*)+(\.\d{0,2})?$|^\d+\.$|^(\d+,)+$/, 'i');

With this regex

Screen.Recording.2023-10-01.at.11.06.49.PM.mov

@Ollyws

[Ollyws]

I agree, it's not really an issue worth complicating the regex for.
@c3024's proposal looks good to me.
🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Krishna2323]

@Ollyws, sorry for the delay, have you tested @c3024's proposal? I'm unable to paste number with multiple commas.
Pls try with "20,0,0,0," or "10,0,0"

[Ollyws]

Yeah they changed the * quantifier for a ? but I assumed that was a typo.
const decimalNumberRegex = new RegExp(/^\d+(,\d*)*(\.\d{0,2})?$/, 'i'); is what I was testing with.

[Krishna2323]

@Ollyws, that's the reason I commented a proposal here, the provided proposal was not working correctly.

[Ollyws]

Thanks for you proposal but @c3024 posted the general solution first, and it includes the correct solution (albeit with one extraneous character) so I think it's fair to go with their proposal.

[Krishna2323]

@Ollyws okay, thanks for clarification, I was just trying to point out that the original proposal will not work.

[Krishna2323]

@Ollyws, sorry for disturbance again but this regex also has an issue, I can paste numbers like "1,,,,,,00,,,,,"

[Ollyws]

Yes I noticed this, I don't think it's necessarily unwanted behaviour as we strip all the commas anyway.

[Krishna2323]

@Ollyws, currently we disallow number with multiple commas, in our staging version we can't paste "10,,,0"

[Ollyws]

Yes I am aware of this.

[melvin-bot]

📣 @Ollyws 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[luacmartins]

Thanks for reviewing the proposals @Ollyws!

[melvin-bot]

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[c3024]

PR #28736 ready for review @Ollyws

[melvin-bot]

🎯 ⚡️ Woah @Ollyws / @c3024, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @c3024 got assigned: 2023-10-03 14:55:37 Z
• when the PR got merged: 2023-10-04 16:07:27 UTC

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.78-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: 28548 #28736

If no regressions arise, payment will be issued on 2023-10-13. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @Ollyws requires payment offer (Reviewer)
• @c3024 requires payment offer (Contributor)
• @dhanashree-sawant requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Ollyws] The PR that introduced the bug has been identified. Link to the PR:
• [@Ollyws] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@Ollyws] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Ollyws] Determine if we should create a regression test for this bug.
• [@Ollyws] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Ollyws]

BugZero Checklist:
I don't think we can really say this is a regression from any PR, it was just something not previously considered to add to the regex.
I don't think a regression test is necessary here as it is a very edge-case issue and doesn't affect the flow in any significant way.

[melvin-bot]

@Ollyws, @sonialiap, @luacmartins, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[luacmartins]

Agreed with @Ollyws's assessment. I believe we're just pending payment.

[sonialiap]

@Ollyws review + bonus = $750 - paid ✔️
@c3024 fix + bonus = $750 - paid ✔️
@dhanashree-sawant report = $50 - paid ✔️