[$250] mWeb - Profile-Entering number & comma and saving first&last name shows inconsistent behavior

lanitochka17 commented 2 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: 9.0.13 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:

Go to https://staging.new.expensify.com/home
Tap 3 sections : profile icon -- profile , profile icon -- profile-- legal name, profile icon- save the world-- I know a teacher
Enter 1(number) and comma as first name and last name and try to save it in all three section

Expected Result:

Entering number & comma and saving first&last name must behave consistently across the app

Actual Result:

Entering number & comma and saving first&last name behaves inconsistently across the app

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/0be0bc4c-41e3-4202-8129-b5f79e11e542

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015c78d657370f836b
Upwork Job ID: 1819336887695900299
Last Price Increase: 2024-08-02
Automatic offers:

allgandalf | Reviewer | 103437212
nkdengineer | Contributor | 103437214

Issue Owner

Current Issue Owner: @allgandalf

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @bfitzexpensify (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 2 months ago

@bfitzexpensify 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

lanitochka17 commented 2 months ago

We think that this bug might be related to #vip-vsp

nkdengineer commented 2 months ago

Proposal

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

Entering number & comma and saving first&last name behaves inconsistently across the app

What is the root cause of that problem?

We are using three validate functions and three error messages in three places of the App

https://github.com/Expensify/App/blob/d8d7f7887ea28c70ba71a06be87fcacd2436e2bc/src/pages/settings/Profile/DisplayNamePage.tsx#L49-L50

https://github.com/Expensify/App/blob/d8d7f7887ea28c70ba71a06be87fcacd2436e2bc/src/pages/settings/Profile/PersonalDetails/LegalNamePage.tsx#L48-L49

https://github.com/Expensify/App/blob/c486e5627d3787ee7355a120e6d87002ad1ba6e0/src/pages/TeachersUnite/KnowATeacherPage.tsx#L61-L62

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

We should use only one method and one error message, the method and the error message can be discussed. In my opinion, legal name is another case. We can just be consistent for DisplayNamePage and KnowATeacherPage.

https://github.com/Expensify/App/blob/d8d7f7887ea28c70ba71a06be87fcacd2436e2bc/src/pages/settings/Profile/DisplayNamePage.tsx#L49-L50

https://github.com/Expensify/App/blob/d8d7f7887ea28c70ba71a06be87fcacd2436e2bc/src/pages/settings/Profile/PersonalDetails/LegalNamePage.tsx#L48-L49

https://github.com/Expensify/App/blob/c486e5627d3787ee7355a120e6d87002ad1ba6e0/src/pages/TeachersUnite/KnowATeacherPage.tsx#L61-L62

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~015c78d657370f836b

melvin-bot[bot] commented 1 month ago

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

mrandaklak commented 1 month ago

If you want the application to be consistent in storing first name and last name, you should use a common function to validate. Contributor details Your Expensify account email: mrandaklak@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0103c182d1104ab55e

melvin-bot[bot] commented 1 month ago

📣 @mrandaklak! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

mihna123 commented 1 month ago

Proposal

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

Inputing a number into Name field and a comma into Last Name field in "Display Name Page", "Legal Name Page" and "I Know a Teacher Page" show three different error messages.

What is the root cause of that problem?

All of these pages have different validation methods, which I think is fine because the display name should be more flexible than the legal name (for example numbers are fine in the display name). The cause of the problem is inconsistent error messages for similar errors: privatePersonalDetails.error.hasInvalidCharacter and personalDetails.error.hasInvalidCharacter are different. bankAccount.error.firstName & bankAccount.error.lastName error messages that are used in the "I Know a Teacher Page" are different too, but maybe this is intended based on error naming.

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

One option is to just change the error messages to mach one another. Other option is to use same error message on all three pages, but I think that this is less readable and more confusing.

Contributor details Your Expensify account email: mihailonvojinovic@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b24da7a68d33cecb

melvin-bot[bot] commented 1 month ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 1 month ago

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

allgandalf commented 1 month ago

I will review proposal today

allgandalf commented 1 month ago

Lets go with @nkdengineer 's proposal, clear RCA and solution.

We can just be consistent for DisplayNamePage and KnowATeacherPage.

I too agree with this!

allgandalf commented 1 month ago

Lets go with @nkdengineer 's proposal, clear RCA and solution.

We can just be consistent for DisplayNamePage and KnowATeacherPage.

I too agree with this!

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @allgandalf 🎉 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 1 month ago

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

nkdengineer commented 1 month ago

@allgandalf The PR is here.

allgandalf commented 1 month ago

Sorry, was stuck with something super urgent internally 🔥 (slack for reference), I will review the PR today

allgandalf commented 1 month ago

Thanks for the patience, all yours @blimpich

allgandalf commented 1 month ago

Automation failed, this was already deployed to production here

allgandalf commented 1 month ago

[x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/35229
[x] 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/35229/files#r1732191068
[x] 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] 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 Proposal

Go to Profile > DisplayName
Enter 1(number) and comma as first name and last name and try to save it in all three section

Verify that the last name error appears

Go to Save the world > I know a teacher
Do step 2 again

Verify that the error appears is the same

Do we agree 👍 or 👎

allgandalf commented 3 weeks ago

Can you close this one @bfitzexpensify ?

bfitzexpensify commented 3 weeks ago

I've left this open as the Upwork offer for @nkdengineer remains open for the moment - @nkdengineer, a bump to accept that offer, thank you!

nkdengineer commented 3 weeks ago

@bfitzexpensify Thanks for the bump, all good from me now

bfitzexpensify commented 3 weeks ago

Thanks! Payment sorted. Let's close this out.

Expensify / App