App: [HOLD for payment 2023-09-20] [$1000] Inconsistent validation of name in workspace settings

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:

Login to new dot
Open settings
Click on workspaces and select any workspace. Create one if there’s none present.
Click on settings.
Try to change the name of workspace to <lol>. Observe the error after clicking on save.
Now try to change the name to < lol>.
Click on save.
Now again try to change the name to <>.

Expected Result:

We shouldn’t be able to save to any of the following and error should be shown immediately after clicking save without going back.

Actual Result:

For <lol>, it is the ideal behaviour For <>, it actually gets saved without any problem For < lol>, it didn’t show error immediately, and goes back to workspace page.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.44-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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856

https://github.com/Expensify/App/assets/43996225/b17e5722-454a-4eef-b1b8-960c4263cf66

https://github.com/Expensify/App/assets/43996225/37763e02-bef8-4133-9b3f-5018042d0469

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL: Issue reported by: @shubham1206agra Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689605016201359

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0174a929d45c708370
Upwork Job ID: 1683849578868912128
Last Price Increase: 2023-08-15
Automatic offers:

shubham1206agra | Reporter | 26189493

About this issue

Original URL
State: closed
Created a year ago
Comments: 86 (65 by maintainers)

Most upvoted comments

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:

[@robertKozik] The PR that introduced the bug has been identified. Link to the PR: It was inconsistent behaviour between validation on FE and BE, no PR to blame on.

[@robertKozik] 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

[@robertKozik] 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

[@robertKozik] Determine if we should create a regression test for this bug. I believe there’s no need to create a regression test for this. The validation will remain consistent with the backend unless it undergoes a change. It’s difficult to reliably catch this with regression tests.

[@robertKozik] 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. N/A

robertKozik on Sep 13, 2023

Thanks for updating the proposal @sourcecodedeveloper, your proposal was first to store the whitelisted tags as array to imitate the backend flow there, it should be beneficial in fixing any of further inconsistents. That’s also why I think we should go with your proposal rather than one powerful regex proposed by @shubham1206agra, in this case unifying the logic between two ends should be prioritised. I’m opting for the @sourcecodedeveloper proposal as it was first to introduce similar flow to a BE validation flow, and needed only small adjustments after to minimise the regression possibility.

🎀 👀 🎀 C+ reviewed

robertKozik on Aug 17, 2023

thanks for clarifying @techievivek, If thats how its handled on BE we should imitate it on FE also. I’ve checked the regex and solution from @sourcecodedeveloper, it tests well and checks out all scenarios pointed out by @techievivek. As it was also the first after updating the conditions of the issue I’ll go with that solution

CC. @techievivek

robertKozik on Aug 17, 2023

I suggest to put this on hold for https://github.com/Expensify/App/issues/21133. That issue is waiting for decision here - https://github.com/Expensify/App/issues/21133#issuecomment-1674550063

Edit: if this issue is just about to update regex, then no need to hold.

aimane-chnaif on Aug 16, 2023

Proposal

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

Workspace name validation is not consistent. It allows <> but disallows <lol>

What is the root cause of that problem?

Root cause of the problem is this validation in Form https://github.com/Expensify/App/blob/9004c3968fcde7f46a473db871b4dd6ab2f3effc/src/components/Form.js#L126 This regex here https://github.com/Expensify/App/blob/9004c3968fcde7f46a473db871b4dd6ab2f3effc/src/CONST.js#L762 matches only if there is atleast one non space character before >. So it does not match <>

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

We can change the regex to

/<([^>\s]*)(?:[^>]*?)>/g

so that now it matches <> as well

What alternative solutions did you explore? (Optional)

c3024 on Jul 21, 2023