-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$250] Tag - No error when creating new tag with existing name when tag name has X: Y format #45067
Comments
Triggered auto assignment to @mallenexpensify ( |
@mallenexpensify 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 |
We think that this bug might be related to #wave-collect - Release 1 |
ProposalPlease re-state the problem that we are trying to solve in this issue.There is no tag existing error when trying to add or edit a tag in X: Y format. What is the root cause of that problem?When we add a tag in X: Y format, the colon (:) is escaped, so it becomes App/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx Lines 48 to 49 in 9c32293
For example, if we input What changes do you think we should make in order to solve the problem?Escape the tag name before doing the comparison. App/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx Lines 43 to 49 in 9c32293
We need to do the same for edit tag page. Also, there is another issue where adding Lines 289 to 291 in 9cd3257
To fix it, we only want to replace |
ProposalPlease re-state the problem that we are trying to solve in this issue.No error when creating new tag with existing name when tag name has X: Y format What is the root cause of that problem?When a new tag is added and the tag string contain a colon We have a function Line 295 in 1534ac2
Below a table with some tests I have done:
On Test 1, if the user type something in the form of But on Test 3, if the user type when adding a new tag So, as we notice, using What changes do you think we should make in order to solve the problem?As mentioned, using To make a fair comparisation between the value of the new tag and the one displayed using values from Onyx, we should follow a better path. One way to do that is by:
As we do for the display, we can use
What alternative solutions did you explore? |
Job added to Upwork: https://www.upwork.com/jobs/~0126cdffc6828dd304 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha ( |
Def seems like it can be External. @getusha , please review the proposals above. |
@mallenexpensify, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@mallenexpensify, @getusha Eep! 4 days overdue now. Issues have feelings too... |
this fell through the cracks, will give an update today! |
@bernhardoj the @devguest07 Instead of converting the entire array to match the tag name value, I think the best option is to just change the tag name value to match the tag name format in the tags array. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@bernhardoj could you try with |
@getusha it works fine. If I add If yes, then it's the issue with how we display the tag, specifically the
If you remove We need to update Lines 289 to 291 in fa738c6
|
@mallenexpensify, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@getusha 👀 on @bernhardoj 's comment above plz |
PR is ready cc: @getusha |
I opened a new PR to handle the edit tag case. cc: @getusha @techievivek |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
@bernhardoj, @getusha can you provide and update on where we're at here? I think payment is due now cuz the second PR hit production over a week ago. And, with/for the first PR, I see
So that doesn't seems like it was a regression. |
@mallenexpensify yes, I believe it's now ready for payment.
Correct. |
Contributor: @bernhardoj due $250 via NewDot @getusha we want a regression test here, right? Assuming so, plz complete the BZ checklist below. Thx 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:
|
Gentle bump @getusha on above. |
Not overdue. |
[@] The PR that introduced the bug has been identified. Link to the PR: I wouldn't call this a regression at it seems to be an edge case that must've been missed on feature implementation. Regression Test Proposal
Do we agree 👍 or 👎 |
Looks good, @mallenexpensify let's get it added to testRail, thanks. |
Requested in ND. |
$250 approved for @bernhardoj |
We good here! Test case created |
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.5-5
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4704925
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
When the tag name has X: Y format, there will be error indicating that the same tag already exists when creating a new tag with the same name (Step 5) and renaming tag to the existing tag name (Step 10)
Actual Result:
When the tag name has X: Y format, no error when creating a new tag with the same name (Step 5) and renaming tag to the same name (Step 10)
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6536532_1720494570528.bandicam_2024-07-09_11-03-34-672.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @getushaThe text was updated successfully, but these errors were encountered: