[HOLD for payment 2023-12-08] [$500] Connect Bank Account - first name not being validated on client side when adding a bank account

[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. Open the app
2. Open settings->workspaces->any workspace->bank account->connect manually
3. Fill in step 1 and 2 details and open step 3
4. Fill in correct details in step 3 and write numbers of symbols in first name field (used to trigger error)
5. Click on save and observe that app triggers error
6. Change first name field to proper name and save, it will move to next step and error is dismissed
7. Observe on workspace list or on profile avatar, there is no red dot currently
8. Reload on main page and observe that now red dot is displayed on profile avatar and on workspaces in workspace list even though error was fixed in step 6

Expected Result:

App should display an error when entering invalid symbols in the first name field

Actual Result:

App only shows a general "unexpected error" after hitting save and continue

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.69.0
Reproducible in staging?:
Reproducible in production?:
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

bank.account.errors.resurface.mp4

Recording.4454.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694445586380489

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012a5d35a91df14676
• Upwork Job ID: 1702051718946840576
• Last Price Increase: 2023-10-04
• Automatic offers:
• 0xmiroslav | Reviewer | 27455937
• tienifr | Contributor | 27455939
• dhanashree-sawant | Reporter | 27455942

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012a5d35a91df14676

[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 @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[peterdbarkerUK]

Kinda weird that it doesn't highlight the first name as an error either

(continuing to investigate, gotta step out for a sec)

[peterdbarkerUK]

I was able to reproduce it. I also triggered this issue.

[tienifr]

Proposal

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

App displays red dot on profile and on workspace list items on reload after fixing the error -> This is already fixed.

There's another issue here, we're not validating the names properly, the error should be caught in the client side rather than waiting for error from the server to return.

What is the root cause of that problem?

We're not validating the names on that screen properly

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

We should validate the first name and last name properly here in client side, we shouldn't allow numbers/special characters in the names. We can get the exact rules from the back-end and update the validation in the app accordingly.

[Updated] Here're the steps:
a. Add a isValidName method to here, with the Regex test for the valid name that matches the back-end.

function isValidName(value) {
    return /^[^\d^!#$%*=<>;{}"]+$/.test(value);
}

This matches the back-end because:

• It rules out all special characters that are forbidden by Onfido (see the Forbidden characters -> For names part here). We need to exclude those because internally the back-end calls to Onfido services and if there's any such special character, it will return the following Onfido validation error

"requestorIdentityOnfido": {
    "status": "unexpectedOnfidoError",
    "apiResult": {
        "reason": "Tested via external APIs",
        "onfidoError": "{\"error\":{\"type\":\"validation_error\",\"message\":\"There was a validation error on this request\",\"fields\":{\"first_name\":[\"forbidden character detected\"]}}}"
    },
},

• It checks that the name does not contain numbers. In the back-end we're stripping the number as confirmed here, so I think we should forbid the users from entering it because it will be stripped anyway which will cause confusion. (An alternate is to replicate exactly the back-end logic by try stripping all numbers, if the name is empty, throw validation error)

b. In here, we add the name validation as well for firstName and lastName

if (values.firstName && !ValidationUtils.isValidName(values.firstName)) {
    errors.firstName = 'bankAccount.error.firstName';
}

if (values.lastName && !ValidationUtils.isValidName(values.lastName)) {
    errors.lastName = 'bankAccount.error.lastName';
}

So it will look like this if we try to submit with invalid name in client side:

The above name errors are already defined, but we might want to clarify it so the user understands what characters are allowed/not allowed.

What alternative solutions did you explore? (Optional)

There're other steps in the bank account flow that also uses the names, we can consider adding the correct name validation to those places as well.

[kevinksullivan]

unassigning myself since we don't need duplicate BZ members

[0xmiros]

I am not sure backend validation logic will be shared publicly. Need to get answer from engineer.
Meanwhile, @tienifr do you have updated proposal with frontend validation fix without knowing backend?

[tienifr]

Sure @0xmiroslav, proposal updated to clarify the steps.

[peterdbarkerUK]

Onyx PR is merged

@0xmiroslav - were you able to get clarification on the validation logic?

[0xmiros]

We also need to wait for onyx version bump in app repo.

[melvin-bot]

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

[peterdbarkerUK]

Not overdue

[melvin-bot]

@peterdbarkerUK @0xmiroslav this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

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

[tienifr]

PR ready for review #30674.

[melvin-bot]

This issue has not been updated in over 15 days. @peterdbarkerUK, @stitesExpensify, @0xmiroslav, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

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

• Fix: Name is not validated in Connect Bank account page #30674

If no regressions arise, payment will be issued on 2023-12-08. 🎊

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:

• @0xmiroslav requires payment offer (Reviewer)
• @tienifr requires payment offer (Contributor)
• @dhanashree-sawant requires payment offer (Reporter)

[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:

• [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
• [@0xmiroslav] 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:
• [@0xmiroslav] 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:
• [@0xmiroslav] Determine if we should create a regression test for this bug.
• [@0xmiroslav] 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.
• [@peterdbarkerUK] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@peterdbarkerUK, @stitesExpensify, @0xmiroslav, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[peterdbarkerUK]

@0xmiroslav could you take a swing through the checklist? This feels like something we should add to TR

Paid all three, though there's an error with ending contracts.

[melvin-bot]

@peterdbarkerUK, @stitesExpensify, @0xmiroslav, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[0xmiros]

This is not regression. Not implemented originally
Regression test will be added in #32291

[peterdbarkerUK]

Job closed all done!