[HOLD for payment 2023-07-17] [$1000] App changes emoji tax number to actual number, causing confusion for user during manual bank account connection

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to the app settings.
2. Navigate to your workspace.
3. Navigate to the bank account section.
4. Manually connect a bank account using routing number 074000078 and account number 12345.
5. Add company information and use the tax number 0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣. Click continue.
6. Observe that the app does not differentiate the emoji number in the tax number field from the actual numbers, causing confusion for the user. When the user goes back to the form, the app changes the emoji number to the actual number.

Expected Result:

The app should differentiate the emoji number in the tax number field from the actual numbers and display a relevant error message if an incorrect format is entered. The app should not change the emoji number to the actual number; instead, it is preferable to display an error message initially.

Actual Result:

The app changes the emoji number in the tax number field to the actual number, causing confusion for the user.

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.29.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

screen-capture.82.1.webm

Recording.775.1.mp4

Expensify/Expensify Issue URL:

Issue reported by: @tewodrosGirmaA

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018839cf0c086e82de
• Upwork Job ID: 1671530734389030912
• Last Price Increase: 2023-07-05

[eh2077]

Proposal

Combined proposal from #21062 (comment) to fix Zip Code issue.

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

App changes emoji tax number to actual number

What is the root cause of that problem?

The root cause of this issue is that, in method ValidationUtils.isValidTaxID

App/src/libs/ValidationUtils.js

Lines 434 to 436 in bee8fd3

	function isValidTaxID(taxID) {
	return taxID && CONST.REGEX.TAX_ID.test(taxID.replace(CONST.REGEX.NON_NUMERIC, ''));
	}

We call taxID.replace(CONST.REGEX.NON_NUMERIC, '') to strip out non-digit characters before testing it.

"0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣".replace(/\D/g, '')

outputs 090909090, so testing CONST.REGEX.TAX_ID.test("090909090") returns true and 0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣ is treated as valid tax id input.

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

After checking other similar validation use cases in ValidationUtils, like isValidDebitCard and isValidSecurityCode, I find that they're not doing stripping out non-digit characters before testing regex pattern. So, in this case, I think also shouldn't strip out non-digit characters before testing regex.

To fix this issue, we can change this line

App/src/libs/ValidationUtils.js

Line 435 in bee8fd3

return taxID && CONST.REGEX.TAX_ID.test(taxID.replace(CONST.REGEX.NON_NUMERIC, ''));

to

return taxID && CONST.REGEX.TAX_ID.test(taxID);

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[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

[sonialiap]

😂 that is odd behavior, let's throw an error. Triaging to external

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018839cf0c086e82de

[melvin-bot]

Current assignee @sonialiap is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[sonialiap]

I propose we expand this issue's scope to prevent emoji entry (i.e. throw an error) into any of the bank form fields

[parasharrajat]

Entering an emoji in a field is not an issue. If a field does not support emoji, it should show an error below the field. We don't want to prevent the user from entering anything.

[parasharrajat]

Do we only want the numbers in Tax Id field?

[eh2077]

Hi @parasharrajat , from Slack discussion, the team suggested to fix ZIP and Tax ID together. I think they're similar but their root causes are different technically speaking. Do you agree to fix them together in this issue? If you do, then I'll be also happy to fix them together by combining my proposal from the other issue.

cc @sonialiap

[sonialiap]

Ah, thanks for pointing out that the root causes are different @eh2077, I don't have insight into the code part of the problem so that wasn't something I was aware of.

[sonialiap]

@parasharrajat US Tax ID numbers and US zipcodes consist of only numbers. When you type a letter into both fields in NewDot now, it throws an error about unsupported input. US tax IDs and zipcodes definitely do not consist of emojis so we should be throwing an error if a user enters an emoji.

Throwing errors is more in line with our existing practices than preventing input, so I think throwing an error is the way to go with this one.

To clarify, the tax ID and zip code fields should only accept numerals 0 1 2 3 4 5 6 7 8 9 and a dash -

[sonialiap]

I'm actually not sure if we want to accept the dash. While the dash can be present in both the tax ID and the zipcode, some systems request that the data is input without the dash. I'm not sure if we accept the dash on the back end but unless we're actively blocking it now, it's probably fine to include in the accepted symbols for both of these fields.

[sonialiap]

Regarding expanding the scope of this issue to throw an error for emoji input for all bank fields, unless you're this guy, you most likely don't have emojis as part of your banking details so I expect emojis to not be valid input for any of the fields. If we don't want to throw an error for emojis in these fields in general, fine by me, but we definitely don't want to accept emojis in tax ID and zip code.

[eh2077]

@sonialiap Below are the current formats expected to be save to the backend FYI

Tax ID: consists of 9 digits only
Zip code: 12345, 12345-1234, 12345 1234, which can be separated by a hyphen or space

[Nodebrute]

Proposal

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

App changes emoji tax number to actual number, causing confusion for user during manual bank account connection

What is the root cause of that problem?

We are have no validation for emojis in bank input fields

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

As @sonialiap mentioned we should throw an error whenever the user enters an emoji in the input field. To achieve this we can add an emoji check where we are validating. We already have an emojis regex in CONST.js we can use it to validate bank input fields
Example
For taxid we can add one other condition here && !CONST.REGEX.EMOJIS.test(taxID);
For zip code we can add one other condition here && !CONST.REGEX.EMOJIS.test(zipCode)

we can do this for the fields we want to set emoji check. In this way we are not messing up with already created regex.

Results here I have only added validation in taxid and zipcode

Screen.Recording.2023-06-23.at.6.44.31.AM.mov

What alternative solutions did you explore? (Optional)

[parasharrajat]

Sorry for the delay. Yeah, @eh2077 We should fix both fields together there. These issues are related and we have done the same in the past where we take a holistic approach to target root causes.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Nodebrute]

Hey, @parasharrajat thoughts on #21065 (comment)?

[melvin-bot]

📣 @eh2077 You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

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

Upwork job

[eh2077]

@parasharrajat The PR #22280 is ready for review, thanks!

[melvin-bot]

🎯 ⚡️ Woah @parasharrajat / @eh2077, 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 @eh2077 got assigned: 2023-07-05 16:24:18 Z
• when the PR got merged: 2023-07-06 17:30:04 UTC

On to the next one 🚀

[Christinadobrzyn]

Hey @parasharrajat @eh2077 curious if you think this issue will be resolved with this PR? Issue: when entering an account number "XXX868713" the 'X' is accepted as a number.

[eh2077]

Hi @Christinadobrzyn , this PR can't fix the issue you mentioned because their root causes are different.

[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.38-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: inconsistency error for zip code and tax ID contains emoji #22280

If no regressions arise, payment will be issued on 2023-07-17. 🎊

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

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] 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:
• [@parasharrajat] 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:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] 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:

[eh2077]

Friendly bump @sonialiap This is ready for payment

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Fix: remove maxLength property for taxId and expiration date fields #8764
• [@parasharrajat] 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: https://github.com/Expensify/App/pull/8764/files#r1269260018
• [@parasharrajat] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1689852097991399
• [@parasharrajat] Determine if we should create a regression test for this bug. yes
• [@parasharrajat] 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.

Regression Test Steps

1. Go to Settings => Workspaces => Select a Workspace => Bank account => Connect manually => Connect bank account
2. Fill in routing number 074000078 and account number 12345
3. Check the terms of use and click the continue button.
4. Try to enter zip codes one by one: 12️⃣345, 123456, and 123452️⃣. Verify they're all hinted as invalid zip codes.
5. Enter zip code 12345 and verify that it's a valid zip code
6. Try to enter tax IDs one by one: 0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣, 0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0️⃣9️⃣0, 1234567890. Verify they're all hinted as invalid tax IDs.
7. Enter tax ID 123456789 and verify that it's a valid tax ID

Do you agree 👍 or 👎 ?

[sonialiap]

Apologies for the delay! Somehow I missed this issue 🙈

@eh2077 offer sent for fix (+bonus) - paid ✔️
@tewodrosGirmaA - paid for reporting issue ✔️
@parasharrajat - to be paid for review(+bonus) via NewDot

[eh2077]

@sonialiap Accepted the offer thanks

[sonialiap]

Upwork paid and request to add regression test submitted, closing

[parasharrajat]

Payment requested 1500.

[JmillsExpensify]

Reviewed details for @parasharrajat. This is accurate based on summary from Business Reviewer and approved for payment in NewDot.