Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for regression test] [$4000] Bug: Entering 0 as a name causes problems #13779

Closed
9 tasks
kavimuru opened this issue Dec 21, 2022 · 132 comments
Closed
9 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@kavimuru
Copy link

kavimuru commented Dec 21, 2022

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


Problem

Entering 0 as a name can cause strange errors because on the backend we check if the name is empty and "0" is considered empty in PHP. There are a few different manifestations of this problem listed below. Whoever is assigned to work on this issue should first try to find all occurrences of this problem in the app so we can fix them all at once.

Solution

Use front end validation to prevent a user from entering "0" as a name anywhere in the app. To do so, create a validation helper in the ValidationUtils that will return false if "0" is provided for a name. Next use that validation helper where relevant and in accordance with our form guidelines.

On the backend make sure that "0" can not be saved as a valid name anywhere in the app.

Additionally, let's match the front and back end validation exactly so that the user has timely feedback when they enter an invalid name.

Backend validation

  1. Can not be empty
  2. Can not be "0" (counts as empty).
  3. Can't have spaces
  4. 80 character length limit
  5. Can not match an existing room name on the same workspace
  1. Cannot be empty after trimming whitespace
  2. Can not be "0" (counts as empty).
  3. (We should add an 80 character limit IMO)
  • Profile display names
  1. The first name cannot contain 'expensify' or 'concierge'.
  2. ',' and ';' will be removed
  3. <= 50 characters long
  4. Neither name can be "0" (needs to be added)

Known bugs

1. Workspace name

Action Performed:

  1. Go to settings > workspace >General settings
  2. edit workspace name to 0
  3. click save

Expected Result:

0 should be considered an invalid workspace name and an error should be shown before clicking save

Actual Result:

0 is considered a valid workspace name until after you click save, then an error is shown that it's an invalid name.

Notes/Photos/Videos:
invalid workspace

Screen.Recording.2022-12-21.at.8.34.51.PM.mov

2. Profile name

Action Performed:

  1. Click on the profile icon
  2. Click on Profile
  3. Click display name
  4. Change the first name to 0
  5. Make sure the last name is empty
  6. Click Save

Expected Result:

0 should be considered an invalid profile name and an error should be shown before clicking save

Actual Result:

There is no error and on the profile page the display name appears as the user's email address

Notes/Photos/Videos:

Screen.Recording.2023-01-13.at.5.00.17.PM.mov

Workaround:

unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.2.42-2
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1671635151324279

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bc5fba9186abc46a
  • Upwork Job ID: 1616174276234260480
  • Last Price Increase: 2023-02-02
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 21, 2022

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 21, 2022
@kavimuru kavimuru changed the title [HOLD WAQ] - `Invalid workspace name' error shows while editing workspace name to 0 [HOLD WAQ] - Invalid workspace name error shows while editing workspace name to 0 Dec 21, 2022
@neil-marcellini
Copy link
Contributor

I'm going to make this weekly while it's on hold.

@neil-marcellini neil-marcellini added Weekly KSv2 and removed Daily KSv2 labels Dec 21, 2022
@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2022
@lschurr
Copy link
Contributor

lschurr commented Jan 3, 2023

Believe this is still on hold.

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 11, 2023
@lschurr lschurr added Monthly KSv2 and removed Weekly KSv2 labels Jan 11, 2023
@melvin-bot melvin-bot bot removed the Overdue label Jan 11, 2023
@neil-marcellini neil-marcellini changed the title [HOLD WAQ] - Invalid workspace name error shows while editing workspace name to 0 Bug: Invalid workspace name error shows while editing workspace name to 0 Jan 19, 2023
@neil-marcellini
Copy link
Contributor

WAQ is under control for now so I'm taking this off hold. I think we can make this external because we can show an error from the frontend if "0" is entered as a name.

Alternatively we could allow "0" as a valid name and fix it on the backend, but I doubt that we want to do that. I'll ask for some more opinions since I'm not sure.

@neil-marcellini
Copy link
Contributor

Posted in Slack here.

@neil-marcellini neil-marcellini changed the title Bug: Invalid workspace name error shows while editing workspace name to 0 Bug: Entering 0 as a name causes problems Jan 19, 2023
@neil-marcellini
Copy link
Contributor

In Slack we decided that "0" should not be a valid name and that it should be validated on the front end and the backend. I can take care of the backend and I'll mark the issue external so that a contributor can take care of the front end.

@neil-marcellini neil-marcellini added the External Added to denote the issue can be worked on by a contributor label Jan 19, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 19, 2023
@melvin-bot melvin-bot bot changed the title Bug: Entering 0 as a name causes problems [$1000] Bug: Entering 0 as a name causes problems Jan 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01bc5fba9186abc46a

@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Jan 19, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Feb 16, 2023

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

If no regressions arise, payment will be issued on 2023-02-23. 🎊

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.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@MelvinBot
Copy link

MelvinBot commented Feb 16, 2023

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:

  • [@mollfpr / @puneetlath] The PR that introduced the bug has been identified. Link to the PR: n/a this is more of an improvement than a bug
  • [@mollfpr / @puneetlath] 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 this is more of an improvement than a bug
  • [@mollfpr / @puneetlath] 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 this is more of an improvement than a bug
  • [@mollfpr] Propose regression test steps to ensure the same bug will not reach production again.
  • [@puneetlath / @lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/264847

@lschurr
Copy link
Contributor

lschurr commented Feb 17, 2023

Making this daily so we can work on the checklist.

@lschurr lschurr added Daily KSv2 and removed Weekly KSv2 labels Feb 17, 2023
@melvin-bot melvin-bot bot added the Overdue label Feb 20, 2023
@lschurr
Copy link
Contributor

lschurr commented Feb 20, 2023

Holding for a few more days for payment.

@melvin-bot melvin-bot bot removed the Overdue label Feb 20, 2023
@lschurr lschurr added Weekly KSv2 and removed Daily KSv2 labels Feb 20, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Feb 22, 2023

@puneetlath Do you agree we don't have a PR introducing this issue? We improved the RequestCallPage, DisplayNamePage, and LegalNamePage. We also update the BE to sync with the FE.

@puneetlath
Copy link
Contributor

Yes, I agree. This is more of an improvement than a bug fix I'd say.

@puneetlath
Copy link
Contributor

@gadhiyamanan can you please apply to this upwork job for the reporting bonus: https://www.upwork.com/jobs/~01bc5fba9186abc46a

@gadhiyamanan
Copy link
Contributor

@puneetlath applied

@gadhiyamanan
Copy link
Contributor

@puneetlath please close the upwork contract

@lschurr
Copy link
Contributor

lschurr commented Feb 22, 2023

Hi @mollfpr - be sure to take a look at the checklist and work through those steps. Specifically, could you confirm when you've proposed a regression test?

@puneetlath
Copy link
Contributor

@lschurr I took care of the payments, so just the regression test is left for you and @mollfpr.

@mollfpr
Copy link
Contributor

mollfpr commented Feb 23, 2023

Thanks @puneetlath! I’ll work on the checklist and the regression test.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 23, 2023
@puneetlath puneetlath changed the title [HOLD for payment 2023-02-23] [$4000] Bug: Entering 0 as a name causes problems [HOLD for regression test] [$4000] Bug: Entering 0 as a name causes problems Feb 23, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Feb 24, 2023

Proposed regression test:

RequestCallPage

  1. Go to the request call page
  2. Click Call me button with all fields are empty
  3. Verify it shows the errors on First name, Last name, and Phone number fields
  4. Fill the Phone number field with an invalid phone number (e.g 0000323223)
  5. Verify that it shows an error on the Phone number field
  6. Fill First name, Last name, and Phone number fields with correct value and submit the form
  7. Verify that the form is submitted

LegalNamePage

  1. Go to Settings > Profile > Personal details > Legal name
  2. Fill the Legal first name or Legal last name with ; or ,
  3. Verify that the form showing the invalid name error
  4. Empty the Legal first name nor Legal last name and submit the form
  5. Verify that the form shows the required fields error
  6. Fill in the fields and submit the form
  7. Verify that no error is showing and that the form submitted

DisplayNamePage

  1. Go to Settings > Profile > Display name
  2. Empty the fields and submit the form
  3. Verify that the form not showing any error and the form has been submitted
  4. Fill the fields with invalid char ; or , and submit the form
  5. Verify that it shows the invalid fields error
  6. Fill the First name field with the reserved words expensify or concierge and submit the form
  7. Verify that it shows the reserved word fields error to the first name
  8. Fill the fields with the correct value and submit the form
  9. Verify that the form submitted and the display name is changed

@melvin-bot melvin-bot bot added the Overdue label Feb 27, 2023
@lschurr
Copy link
Contributor

lschurr commented Feb 27, 2023

Thanks @mollfpr - I'll create a GH for this regression test today and will close this out.

@melvin-bot melvin-bot bot removed the Overdue label Feb 27, 2023
@lschurr
Copy link
Contributor

lschurr commented Feb 27, 2023

Alright, all set. Closing this one out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests