Fix: remove maxLength property for taxId and expiration date fields #8764

eVoloshchak · 2022-04-25T12:31:14Z

Details

This fix is a result of the audit of all of the forms that aimed to fix any inconsistencies with modifying user input.
As was discussed in slack, we should Keep maxLength for some inputs that are not formatted (eg: CVV, last 4 digits of SSN, US zip code). Which means, we have to remove the maxLength property for TaxID field on CompanyStep and for expiration date field on Add Debit card flow. Additionally, we have to do the following on the CompanyStep page:

Change validation to strip all non-numeric characters and then validate that there are exactly 9 numbers
Strip all non-numeric characters from taxID on form submission
Change placeholder of the taxID field

Fixed Issues

#7524

Tests

Navigate to the add debit card page:
- Open Settings
- Payments
- Add payment method
- Debit card
Verify that expiration date filed does not have max length limit
Navigate to the company step page:
- Create a workspace if you don't have one
- Open Settings
- Select your workspace
- Connect bank account
- Connect manually
- Fill out form and press "Save & continue" to get to step 2
Verify that taxID field does not have max length limit
Verify that taxID field accepts non-numeric characters and validation is correct (no error if there are exactly 9 numbers)

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Navigate to the add debit card page:
- Open Settings
- Payments
- Add payment method
- Debit card
Verify that expiration date filed does not have max length limit
Navigate to the company step page:
- Create a workspace if you don't have one
- Open Settings
- Select your workspace
- Connect bank account
- Connect manually
- Fill out form and press "Save & continue" to get to step 2
Verify that taxID field does not have max length limit
Verify that taxID field accepts non-numeric characters and validation is correct (no error if there are exactly 9 numbers)

Screenshots

Web

cinnamon-20220425-2.mp4

cinnamon-20220425-1.mp4

Mobile Web

22-04-25-14-20-57.mp4

22-04-25-14-20-00.mp4

Desktop

I'm unable to launch the desktop app due to this bug. It is not present if you downgrade to certain node and npm versions, but since I use MacinCloud, I'm unable to do so. So if someone with a mac could test it and upload a video, I would highly appreciate it.

iOS

Screen.Recording.2022-04-25.at.11.43.21.AM.mp4

Screen.Recording.2022-04-25.at.1.31.15.PM.mp4

Android

22-04-25-12-00-18.mp4

22-04-25-11-58-23.mp4

…lidation

rushatgabhane · 2022-04-26T19:15:53Z

Navigate to the add debit card page

Navigate to the company step page

@eVoloshchak please overexplain the steps such that someone unfamiliar with the app can also follow them, thanks!

rushatgabhane · 2022-04-26T19:24:06Z

src/pages/ReimbursementAccount/CompanyStep.js

@@ -142,7 +142,7 @@ class CompanyStep extends React.Component {
            errors.website = true;
        }

-        if (!/[0-9]{9}/.test(this.state.companyTaxID)) {
+        if (!/^\d{9}$/.test(this.state.companyTaxID.replace(/\D/g, ''))) {


@eVoloshchak let's move this logic to ValidationUtils, and CONST
What do you say?

@eVoloshchak let's move this logic to ValidationUtils, and CONST What do you say?

All done

rushatgabhane

ignore this

rushatgabhane · 2022-04-27T01:39:47Z

src/libs/ValidationUtils.js

@@ -355,6 +355,16 @@ function isExistingRoomName(roomName, reports, policyID) {
    );
 }

+/**
+ * Validate that this is a valid tax ID (consisting of 9 digits)


Suggested change

* Validate that this is a valid tax ID (consisting of 9 digits)

* Checks if tax ID consists of 9 digits

rushatgabhane · 2022-04-27T01:41:48Z

src/pages/ReimbursementAccount/CompanyStep.js

@@ -176,7 +176,7 @@ class CompanyStep extends React.Component {
        }

        const incorporationDate = moment(this.state.incorporationDate).format(CONST.DATE.MOMENT_FORMAT_STRING);
-        BankAccounts.setupWithdrawalAccount({...this.state, incorporationDate});
+        BankAccounts.setupWithdrawalAccount({...this.state, incorporationDate, companyTaxID: this.state.companyTaxID.replace(/\D/g, '')});


NAB: @eVoloshchak we could move this regex to const too

True, updated the PR

rushatgabhane · 2022-04-27T01:41:55Z

src/libs/ValidationUtils.js

+ * @returns {Boolean}
+ */
+function isValidTaxID(taxID) {
+    return CONST.REGEX.TAX_ID.test(taxID.replace(/\D/g, ''));


rushatgabhane

@deetergp Looks good, and tests well.

#### PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

deetergp

Looks good. Thanks!

OSBotify · 2022-04-27T18:57:51Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-05-09T20:04:45Z

🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-05-13T16:13:41Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

parasharrajat · 2023-07-20T10:23:58Z

src/libs/ValidationUtils.js

+ * @returns {Boolean}
+ */
+function isValidTaxID(taxID) {
+    return CONST.REGEX.TAX_ID.test(taxID.replace(CONST.REGEX.NON_NUMERIC, ''));


taxID.replace(CONST.REGEX.NON_NUMERIC, '') allowed emojis to be used in the TaxId field which caused a regression #21065

eVoloshchak added 6 commits April 25, 2022 11:16

Remove maxLength from expiration date field

03624d7

Remove maxLength from taxID field

e3509ae

Strip all non-numeric characters and test for length of 9 in error va…

a48ba65

…lidation

Strip all non-numeric characters in companyTaxID on form submission

50caa13

Change taxID input placeholder

ff0468f

Fix linter errors

8c31474

eVoloshchak requested a review from a team as a code owner April 25, 2022 12:31

melvin-bot bot requested review from deetergp and rushatgabhane and removed request for a team April 25, 2022 12:31

eVoloshchak mentioned this pull request Apr 26, 2022

[HOLD for payment 2022-05-20][$1000] - Audit forms and fix inconsistencies with modifying user input #7524

Closed

20 tasks

rushatgabhane reviewed Apr 26, 2022

View reviewed changes

Move tax ID validation to ValidationUtils.js

70b90f3

rushatgabhane suggested changes Apr 27, 2022

View reviewed changes

eVoloshchak added 3 commits April 27, 2022 10:29

Change isValidTaxID comment

d104d5d

Move non-numeric regex to CONST.js

08076bf

Move non-numeric regex to CONST.js

f967968

rushatgabhane approved these changes Apr 27, 2022

View reviewed changes

deetergp approved these changes Apr 27, 2022

View reviewed changes

deetergp merged commit f72c7d5 into Expensify:main Apr 27, 2022

OSBotify mentioned this pull request May 10, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

This was referenced May 12, 2022

[med] JAWS+ Chrome: Role is not defined for the Save control. #8387

Closed

[med] Color Contrast: Many Pages: All edges of the text box fail contrast requirements. #8556

Closed

parasharrajat reviewed Jul 20, 2023

View reviewed changes

parasharrajat mentioned this pull request Jul 20, 2023

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

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: remove maxLength property for taxId and expiration date fields #8764

Fix: remove maxLength property for taxId and expiration date fields #8764

eVoloshchak commented Apr 25, 2022 •

edited

Loading

rushatgabhane commented Apr 26, 2022

rushatgabhane Apr 26, 2022

eVoloshchak Apr 26, 2022

rushatgabhane left a comment •

edited

Loading

rushatgabhane Apr 27, 2022

rushatgabhane Apr 27, 2022

eVoloshchak Apr 27, 2022

rushatgabhane Apr 27, 2022

rushatgabhane left a comment

deetergp left a comment

OSBotify commented Apr 27, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

parasharrajat Jul 20, 2023

	* Validate that this is a valid tax ID (consisting of 9 digits)
	* Checks if tax ID consists of 9 digits

Fix: remove maxLength property for taxId and expiration date fields #8764

Fix: remove maxLength property for taxId and expiration date fields #8764

Conversation

eVoloshchak commented Apr 25, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rushatgabhane commented Apr 26, 2022

rushatgabhane Apr 26, 2022

Choose a reason for hiding this comment

eVoloshchak Apr 26, 2022

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane Apr 27, 2022

Choose a reason for hiding this comment

rushatgabhane Apr 27, 2022

Choose a reason for hiding this comment

eVoloshchak Apr 27, 2022

Choose a reason for hiding this comment

rushatgabhane Apr 27, 2022

Choose a reason for hiding this comment

rushatgabhane left a comment

Choose a reason for hiding this comment

#### PR Reviewer Checklist

deetergp left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 27, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

parasharrajat Jul 20, 2023

Choose a reason for hiding this comment

eVoloshchak commented Apr 25, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading