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

[luacmartins]

We should audit all our forms and fix any inconsistencies with modifying user input. The expected behavior is as follows:

1. User input should not be modified as they type.
2. User input that may include optional characters (e.g. (, ), - in a phone number) and should never be restricted on input, nor be modified or formatted on blur.
3. Instead we will format and clean the user input internally before using the value (e.g. making an API request where the user will never see this transformation happen)
4. Additionally, users should always be able to copy/paste whatever characters they want into fields.

To give a slightly more detailed example of how this would work with phone numbers we should:

• Allow any character to be entered in the field.
• On blur, strip all non-number characters (with the exception of + if the API accepts it) and validate the result against the E.164 regex pattern we use for a valid phone. Stripping characters should happen internally and result in NO UI change to the user.
• On submit, repeat validation and submit with the clean value.

Here's a list of forms to be audited:

• AddPlaidBankAccount.js
• ACHContractStep.js
• BankAccountStep.js
• CompanyStep.js
• RequestorStep.js
• ValidationStep.js
• Workspace - Invite new members
• Workspace - General settings
• IOU flow
• KYC flow
• New Room screen
• Add Debit card flow
• Add Personal Bank account flow
• Settings - Change Password
• Settings - Profile
• Login form
• Password form
• Set password
• New password form
• Request a Call modal

[MelvinBot]

Triggered auto assignment to @SofiedeVreese (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[luacmartins]

Actually, we might use Applause to do this audit for us. Removing the External label.

[MelvinBot]

12 days overdue now... This issue's end is nigh!

[MelvinBot]

12 days overdue. Walking. Toward. The. Light...

[luacmartins]

Nvm my previous comment, this can be handled by contributors! Adding External label.

[MelvinBot]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[luacmartins]

@puneetlath please note that we should add one milestone per 5 forms (4 milestones total).

[MelvinBot]

@puneetlath Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

@puneetlath 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[puneetlath]

Upwork job: https://www.upwork.com/jobs/~01021ef3f081c4d737

[MelvinBot]

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

[MelvinBot]

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

[rushatgabhane]

@deetergp I've verified the audit. All yours!

[eVoloshchak]

Alright, I'm aware that max length isn't our concern as per the issue description.

But having a max length doesn't allow a user to paste anything they want.

Yeah, I too was thinking about max length interfering with users should always be able to copy/paste whatever characters they want into fields requirement, but figured I should leave the max length as it is.

I'll remove max length, if that's the requirement, no problem :)

I'll prepare a list of errors I need English and Spanish translations for (something like "SSN/Zip/CVV can't be longer than x digits")
Can I start working on this or is it up for discussion on Slack?

[rushatgabhane]

Thanks for the quick feedback!

Can I start working on this

Not yet :) We need some consensus first.
Edit: slack 🧵 here https://expensify.slack.com/archives/C01GTK53T8Q/p1649702197809359

[JmillsExpensify]

Looks like this is still being discussed in Slack.

[rushatgabhane]

I ran a poll on slack on what to do with maxLength.

Keep maxLength for some inputs that are not formatted (eg: CVV, last 4 digits of SSN, US zip code)

@eVoloshchak Solution B has won. We can now start working on a PR for the same 🚀

[eVoloshchak]

I ran a poll on slack on what to do with maxLength.

Keep maxLength for some inputs that are not formatted (eg: CVV, last 4 digits of SSN, US zip code)

@eVoloshchak Solution B has won. We can now start working on a PR for the same rocket

Cool
So, just so we are on the same page, I should:

1. Remove maxLength for TaxID on CompanyStep
2. Remove maxLength of expiry date on Add Debit card flow

Is that correct?

[rushatgabhane]

@eVoloshchak yes!

[eVoloshchak]

@rushatgabhane, i have a few additional questions about the task

1. In CompanyStep TaxID field has the following placeholder: 9 digits, no hyphens. Since we are removing maxLength and allowing user to paste anything into this field, including hyphens, what should it be changed to?
2. Same page, same field: currently when user presses Save button, we check that TaxID doesn't contain anything besides numbers (/[0-9]{9}/.test(this.state.companyTaxID)) and if it does, we show the error. Should this behaviour persist, or should we just strip all non-digit characters when passing taxID to the API? Also, should we strip all non-digit characters, verify that they meet max length requirement (9) and show error if it's not the case?
3. Add Debit card flow, expiration date field: addOrRemoveSlashToExpiryDate adds slash to user's input, shoud we retain this behaviour?
4. If not, should we add the slash when sending expiryDate to the API?
5. Should isValidExpirationDate be changed in some way? Are there any other expirationDate formats that need to be supported, since we're dropping the maxLength?

[rushatgabhane]

@eVoloshchak

1. How about "9 digits" as the placeholder?

2. should we just strip all non-digit characters when passing taxID to the API? Also, should we strip all non-digit characters, verify that they meet max length requirement (9) and show error if it's not the case?

   Yep, format and clean the user input internally on blur and on making an API call.

3. Add Debit card flow, expiration date field: addOrRemoveSlashToExpiryDate adds slash to user's input, shoud we retain this behaviour?

   I think we could treat it as an exception. Because it isn't really "modifying" the input. Consider it a visual effect. @luacmartins is this okay?

4. N.A.

5. Should isValidExpirationDate be changed in some way? Are there any other expirationDate formats that need to be supported, since we're dropping the maxLength?

   No, I think it's good.

[luacmartins]

I think we could treat it as an exception. Because it isn't really "modifying" the input. Consider it a visual effect. @luacmartins is this okay?

Yea, I think this is ok.

[eVoloshchak]

@rushatgabhane

1. PR is up
2. There is one milestone per 5 forms, 16 forms were audited on April 11th. Should 3 milestones be paid for?

[rushatgabhane]

There is one milestone per 5 forms, 16 forms were audited on April 11th. Should 3 milestones be paid for?

cc: @JmillsExpensify

[JmillsExpensify]

@rushatgabhane Coming back from OOO and I'm not quite sure I'm following your last comment. What do you mean by three milestones? cc @luacmartins

[rushatgabhane]

@JmillsExpensify For this issue, there are 4 milestones on Upwork. Each milestone requires an audit of 5 pages.

And @eVoloshchak has audited 16 pages. So it's fair to pay for 3 milestones ($750). Let me know if this clarifies it. Thanks!

The remaining could be settled after the latest PR #8764 is on production for 7 days

[luacmartins]

@JmillsExpensify as per this comment, there should be 4x $250 milestones for this job, for a total of $1000. As of now, 3/4 milestones have been completed ($750).

@eVoloshchak when do you think you can complete the last 4 audits?

[eVoloshchak]

@eVoloshchak when do you think you can complete the last 4 audits?

I already did that, both PR's are merged, the latest one will soon be deployed to production

[luacmartins]

Ah nice! Thanks for linking that last PR!

Ok, so 3 milestones are complete and should be paid. The last one is waiting on this PR being deployed and the 7 day regression period.

[mallenexpensify]

@eVoloshchak paid 3 milestones totaling $750 (I'm not sure if the one's finished are aligned with the ones paid in Upwork but the total amounts will wash out).

cc @JmillsExpensify since you'll likely be the one issuing the final $250 payment 7 days after #8764 hits production (if no regressions)

[JmillsExpensify]

Awesome, thanks! Looks like we're still waiting on that last PR to hit production (and confirm no regressions).

[JmillsExpensify]

PR hit staging yesterday.

[JmillsExpensify]

A couple more days left on the 7-day hold for that last PR.

[JmillsExpensify]

Jumping in to process this one now and pay outstanding contributors.

[JmillsExpensify]

Got a little confused on the payments, but after re-adding the above I'll be issuing the final $250 payment. @rushatgabhane I've also closed out your C+ contract.

@luacmartins Can we also close out this ensure issue now?

[luacmartins]

@JmillsExpensify Yes! This was the last payment so I believe that all milestones have been paid out. Thanks for working on this!

[parasharrajat]

Shouldn't this be convered as part of this issue?

[luacmartins]

Hmm that seems to be out of scope for this issue.