[HOLD for payment 2024-07-24] [$250] Workspace - Bank account personal info isn't accepting characters with umlaut as name

lanitochka17 commented 4 months ago

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

Version Number: 1.4.85-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

Log in with a new expensifail account
Create a workspace
Enable Workflows
Navigate to workspace settings - Workflows
Click on "Connect bank account"
Choose the manual option
Input "011401533" for Routing Number
Input "1111222233331111" for Account Number
Input "Ádám" for first name
Input "Horváth for last name

Expected Result:

Characters with umlaut should be accepted

Actual Result:

Bank account personal info isn't accepting characters with umlaut as name. Please enter a valid name error appears. These characters are accepted in the Save the world - teacher and principal flows

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1962573b-b68d-4a66-9efd-b0db7326e78a

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01000955591143b611
Upwork Job ID: 1805814229750460922
Last Price Increase: 2024-06-26
Automatic offers:

suneox | Reviewer | 102945796
eucool | Contributor | 102945799

Issue Owner

Current Issue Owner: @JmillsExpensify

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 4 months ago

@JmillsExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 4 months ago

@JmillsExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01000955591143b611

JmillsExpensify commented 4 months ago

Opening up to community

melvin-bot[bot] commented 4 months ago

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

dominictb commented 4 months ago

Proposal

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

Bank account personal info isn't accepting characters with umlaut as name. Please enter a valid name error appears. These characters are accepted in the Save the world - teacher and principal flows

What is the root cause of that problem?

We don't allow users to enter the accented characters in

https://github.com/Expensify/App/blob/9862fdbc01fe36bdb6dee9554023178d9f476bdb/src/libs/ValidationUtils.ts#L352

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

Remove the above condition

What alternative solutions did you explore? (Optional)

We can update the isValidLegalName based on BE side.

eucool commented 4 months ago

Proposal

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

Bank account personal information doesn't accept umlaut characters.

What is the root cause of that problem?

We have a check in place which checks for isValidLegalName, this will not allow umlaut.

This is the RCA.

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

We should use isValidPersonName instead here, this will reject special characters and accept umlaut.

so we need to update the following block of code:

https://github.com/Expensify/App/blob/57e5d0e7197a9c21e6f221389bed87870c90ef0a/src/pages/ReimbursementAccount/PersonalInfo/substeps/FullName.tsx#L42-L49

At both the places above, we will use isValidPersonName util function:

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

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

What alternative solutions did you explore? (Optional)

suneox commented 4 months ago

Thanks for @dominictb & @eucool proposals. The RCA is correct and I have some feedback:

About @dominictb proposal to remove the condition check hasAccentedChars in the function isValidLegalName can works but it will raise regression in other places because that function allows accents/diacritics characters, but APIs from other places do not support accents/diacritics.

Data test:

Việt Nam: Công Phạm
Hungary: Horváth Ádám
France: Crème Brûlée
Spain: José Rodríguez
Portugal: João Silva
Germany: Müller Schön
Turkey: Ayşe Yılmaz

About @eucool solution, it seems like can work if we only update for Bank account personal info screen

We use isValidLegalName at a lot of places where the behaviour to reject umlaut is required so instead of updating that util, we should instead use isValidPersonName:

I don't think we need to update all of them will also raise regression with the data test above

Next step: Waiting @dominictb & @eucool update there proposal

eucool commented 4 months ago

@suneox what are the next steps here I proposed that we should use isValidPersonName in the bank account personal name page instead of the current isValidLegalName, what should I update in my proposal confused a little

I don't think we need to update all of them will also raise regression with the data test above

Yes is agree I just mentioned it for the additional point, the changes in my solution are simple:

https://github.com/Expensify/App/blob/57e5d0e7197a9c21e6f221389bed87870c90ef0a/src/pages/ReimbursementAccount/PersonalInfo/substeps/FullName.tsx#L42-L49

At both the places above, we will use isValidPersonName util function:

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

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

suneox commented 4 months ago

what should I update in my proposal confused a little

@eucool Ah, You should update your proposal with more detail as mentioned above and update unnecessary content, because the internal reviewer will review your solutions again instead of reading the entire conversation to get the context.

eucool commented 4 months ago

Updated proposal

Updated proposal to remove unwanted content and added some code

suneox commented 4 months ago

@eucool proposal LGTM we can go with this solution.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

📣 @suneox 🎉 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[bot] commented 4 months ago

📣 @eucool 🎉 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 📖

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

https://github.com/Expensify/App/pull/44689

If no regressions arise, payment will be issued on 2024-07-24. :confetti_ball:

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

@suneox requires payment automatic offer (Reviewer)
@eucool requires payment automatic offer (Contributor)

melvin-bot[bot] commented 4 months ago

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:

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

suneox commented 3 months ago

[x] [@suneox] The PR that introduced the bug has been identified. Link to the PR: I don't think there's a PR responsible for it, as this is an update validation rule and previous rule also match with BE API
[x] [@suneox] 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: N/A
[x] [@suneox] 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: N/A
[x] [@suneox] Determine if we should create a regression test for this bug: N/A

JmillsExpensify commented 3 months ago

Payment summary:

Contributor: @eucool $250
Reviewer: @suneox $250

JmillsExpensify commented 3 months ago

All contributors paid via Upwork and checklist complete. Closing this one!

Expensify / App