[$250] Tag - No error when creating new tag with existing name when tag name has X: Y format #45067

lanitochka17 · 2024-07-09T12:29:02Z

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:

Go to staging.new.expensify.com
Go to Workspace settings > Tag
Add a tag in X: Y format
Click Add tag
Attempt to add the same tag in Step 3
Note that there is no error that the same tag already exists
Create a different tag
Click on the tag in Step 7
Rename it to the same tag in Step 3 and save it
Note that there is no error too and the old tag in Step 7 is gone

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

Upwork Job URL: https://www.upwork.com/jobs/~0126cdffc6828dd304
Upwork Job ID: 1811543862318164445
Last Price Increase: 2024-07-25

Issue Owner

Current Issue Owner: @getusha

melvin-bot · 2024-07-09T12:29:18Z

Triggered auto assignment to @mallenexpensify (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 · 2024-07-09T12:29:45Z

@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

lanitochka17 · 2024-07-09T12:30:13Z

We think that this bug might be related to #wave-collect - Release 1

bernhardoj · 2024-07-09T12:49:56Z

Proposal

Please 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 X\: Y. When we check whether there is an existing tag, we just use the tag name input from the user without escaping it.

App/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx

Lines 48 to 49 in 9c32293

    
           } else if (tags?.[tagName]) { 
        
               errors.tagName = translate('workspace.tags.existingTagError');

For example, if we input a: b, the tag will be saved as a\: b. When we try to add a: b again, it will check if a: b exists or not which doesn't.

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

    
           const tagName = values.tagName.trim(); 
        
           const {tags} = PolicyUtils.getTagList(policyTags, 0); 
        
           if (!ValidationUtils.isRequiredFulfilled(tagName)) { 
        
               errors.tagName = translate('workspace.tags.tagRequiredError'); 
        
           } else if (tags?.[tagName]) { 
        
               errors.tagName = translate('workspace.tags.existingTagError');

PolicyUtils.escapeTagName(values.tagName.trim())

We need to do the same for edit tag page.

Also, there is another issue where adding x:y and x\:y tags will show both tags as x:y in the tag list, confusing the user. It's because the code replaces \: or \\: with : from the tag name.

App/src/libs/PolicyUtils.ts

Lines 289 to 291 in 9cd3257

    
           function getCleanedTagName(tag: string) { 
        
               return tag?.replace(/\\{1,2}:/g, CONST.COLON); 
        
           }

To fix it, we only want to replace \: with :.
tag?.replace(/\\:/g, CONST.COLON);

devguest07 · 2024-07-09T22:21:11Z

Proposal

Please 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 : the backend will store it on Onyx adding a \ before the colon. So when we add a new tag that contain a colon, we need to check if it exist in the Onyx value. The issue is that the tags on Onyx has a \ added. And currently we are not checking the new tag with the \ similar to what we have stored on Onyx.

We have a function escapeTagName that takes a tag string with : and add the \ to make it similar to the Onyx values. We can use it in the comparisation.

App/src/libs/PolicyUtils.ts

Line 295 in 1534ac2

function escapeTagName(tag: string) {

Below a table with some tests I have done:

Input: is the value entered on the input form when addinga new tag
Onyx: how the new tag will be stored on the database
Escaped: the input value passed throw the escapeTagName function
Displayed: how the value will be displayed for the user on the tags list page

Test	Input	Onyx	Escaped	Displayed
1	`a: a`	`a\: a`	`a\: a`	`a: a`
2	`b\: b`	`b\\: b`	`b\\: b`	`b: b`
3	`c\\: c`	`c\\\: c`	`c\\\: c`	`c\: c`
4	`d\\\: d`	`d\\\\: d`	`d\\\\: d`	`d\\: d`

On Test 1, if the user type something in the form of X: Y we can use the escapeTagName to make sure we don't get a duplicate, as we notice from the table, the Onyx value is equal to the Escaped value, same for the input value and the displayed value. So, X: Y will not be repeated.

But on Test 3, if the user type when adding a new tag c\\: c the displayed value will be different. Comparing the escapeTagName with the Onyx value will show that the value already exist but it doesn't. Because the displayed value is not as the one on the input, it's different value c\: c.

So, as we notice, using escapeTagName to compare the Onyx value with the new tag is correct is just some cases where the new tag string have only a colon, but when the user use a colon and lore than a \ in the new tag value , the comparisation using escapeTagName will not be correct. The error will display that those new tag already exists but it doesn't.

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

As mentioned, using escapeTagName to compare the Onyx value with the new tag value will solve only some cases like Test 1 and Test 2, but in more complex examples like Test 3 and Test 4, the comparisation using escapeTagName will display wrong results.

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:

Take all the tags from Onyx and convert those values to an array with value similar to the tags displayed

App/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx

Line 30 in 5ce4e60

policyTags: OnyxEntry<PolicyTagList>;

As we do for the display, we can use getCleanedTagName to make sure we have the same values as displayed on the tags page.

App/src/pages/workspace/tags/WorkspaceTagsPage.tsx

Line 107 in d576e28

text: PolicyUtils.getCleanedTagName(tag.name),

Compare the new tag string to the new tags array:
Now we have a new array with the same values displayed, we can compare the entered new tag string with those values and make sure no value will be similar to the one from the input.

App/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx

Line 48 in 5ce4e60

} else if (tags?.[tagName]) {

What alternative solutions did you explore?

melvin-bot · 2024-07-11T23:31:30Z

Job added to Upwork: https://www.upwork.com/jobs/~0126cdffc6828dd304

melvin-bot · 2024-07-11T23:31:34Z

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

mallenexpensify · 2024-07-11T23:32:19Z

Def seems like it can be External. @getusha , please review the proposals above.
Also, I'd love to fix any other similar escaped bugs if we're able to do so at the same time

melvin-bot · 2024-07-15T18:31:00Z

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

melvin-bot · 2024-07-17T18:09:18Z

@mallenexpensify, @getusha Eep! 4 days overdue now. Issues have feelings too...

getusha · 2024-07-18T13:29:20Z

this fell through the cracks, will give an update today!

getusha · 2024-07-18T13:57:48Z

@bernhardoj the escapeTagName converts x:y -> x\\:y which will not be available in tags as a key, but i see the name value holds that value. and was able to get it work by changing the condition to Object.values(tags).find(v => v.name === tagName) could you check this and update your proposal?

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

melvin-bot · 2024-07-18T16:00:15Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

bernhardoj · 2024-07-19T09:39:26Z

@getusha the solution works fine for me.

web.mp4

the escapeTagName converts x:y -> x\:y which will not be available in tags as a key,

This is a similar concern that I answered here

Do you have any tag name that doesn't work for you?

getusha · 2024-07-19T09:49:47Z

@bernhardoj could you try with x\:y?

bernhardoj · 2024-07-20T04:10:46Z

@getusha it works fine. If I add x\:y twice, I will get an error. I guess the issue you mean is that if we add x:y and x\:y, there will be 2 x:y in the tag list, is that correct?

If yes, then it's the issue with how we display the tag, specifically the getCleanedTagName.

App/src/pages/workspace/tags/WorkspaceTagsPage.tsx

Line 108 in fa738c6

text: PolicyUtils.getCleanedTagName(tag.name),

If you remove getCleanedTagName, you will see both tags are different.

We need to update getCleanedTagName to only remove 1 \ (tag?.replace(/\\:/g, CONST.COLON);).

App/src/libs/PolicyUtils.ts

Lines 289 to 291 in fa738c6

    
           function getCleanedTagName(tag: string) { 
        
               return tag?.replace(/\\{1,2}:/g, CONST.COLON); 
        
           }

melvin-bot · 2024-07-22T18:29:19Z

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

mallenexpensify · 2024-07-22T19:55:26Z

@getusha 👀 on @bernhardoj 's comment above plz

bernhardoj · 2024-07-31T06:09:55Z

PR is ready

cc: @getusha

mallenexpensify · 2024-08-12T23:40:35Z

@getusha owes $500 from #32699 , let's use this issue to knock it down by $250, then we'll find another $250 one.

bernhardoj · 2024-08-13T03:22:55Z

I opened a new PR to handle the edit tag case.

cc: @getusha @techievivek

melvin-bot · 2024-08-14T17:28:06Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

mallenexpensify · 2024-08-26T15:11:06Z

@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

We've been saving tag name by escaping it before this PR, so this PR didn't cause it.

So that doesn't seems like it was a regression.

bernhardoj · 2024-08-26T15:18:33Z

@mallenexpensify yes, I believe it's now ready for payment.

So that doesn't seems like it was a regression.

Correct.

mallenexpensify · 2024-08-26T15:24:52Z

Contributor: @bernhardoj due $250 via NewDot
Contributor+: @getusha 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:

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

techievivek · 2024-08-29T05:45:53Z

Gentle bump @getusha on above.

techievivek · 2024-08-29T07:48:32Z

Not overdue.

getusha · 2024-08-29T11:02:44Z

[@] 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.
[@] 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
[@] 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
[@] Determine if we should create a regression test for this bug. Yes
[@] 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

Open the app
Go to Workspace settings > Tag
Add X:Y tag
Attempt to add the same tag X:Y
Verify A tag with this name already exists error appears
Add a different tag
Click on the tag in Step 6
Rename it to the same tag in Step 3 (X:Y) and save it
Verify A tag with this name already exists error appears

Do we agree 👍 or 👎

techievivek · 2024-08-30T05:01:11Z

Looks good, @mallenexpensify let's get it added to testRail, thanks.

bernhardoj · 2024-08-30T14:11:02Z

Requested in ND.

JmillsExpensify · 2024-08-30T17:29:30Z

$250 approved for @bernhardoj

mallenexpensify · 2024-08-30T23:02:38Z

We good here! Test case created

https://github.com/Expensify/Expensify/issues/424794

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 9, 2024

melvin-bot bot assigned mallenexpensify Jul 9, 2024

melvin-bot bot added the Overdue label Jul 11, 2024

mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Jul 11, 2024

melvin-bot bot changed the title ~~Tag - No error when creating new tag with existing name when tag name has X: Y format~~ [$250] Tag - No error when creating new tag with existing name when tag name has X: Y format Jul 11, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 11, 2024

melvin-bot bot assigned getusha Jul 11, 2024

melvin-bot bot removed the Overdue label Jul 11, 2024

melvin-bot bot added the Overdue label Jul 15, 2024

melvin-bot bot removed the Overdue label Jul 18, 2024

melvin-bot bot added the Overdue label Jul 22, 2024

melvin-bot bot added the Weekly KSv2 label Jul 31, 2024

mallenexpensify mentioned this issue Aug 12, 2024

[$500] Chat - Loading animation not shown when open 1:1 Conversation with uncached attachments #32699

Closed

6 tasks

bernhardoj mentioned this issue Aug 13, 2024

Fix user can edit tag name to existing tag name in X:Y format #47289

Merged

50 tasks

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 13, 2024

mallenexpensify removed the Reviewing Has a PR in review label Aug 26, 2024

melvin-bot bot added the Overdue label Aug 26, 2024

mallenexpensify added Daily KSv2 and removed Weekly KSv2 Overdue labels Aug 26, 2024

melvin-bot bot added Overdue labels Aug 26, 2024

melvin-bot bot removed the Overdue label Aug 26, 2024

melvin-bot bot added the Overdue label Aug 28, 2024

melvin-bot bot removed the Overdue label Aug 29, 2024

mallenexpensify closed this as completed Aug 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$250] Tag - No error when creating new tag with existing name when tag name has X: Y format #45067

[$250] Tag - No error when creating new tag with existing name when tag name has X: Y format #45067

lanitochka17 commented Jul 9, 2024 •

edited by techievivek

Loading

melvin-bot bot commented Jul 9, 2024

lanitochka17 commented Jul 9, 2024

lanitochka17 commented Jul 9, 2024

bernhardoj commented Jul 9, 2024 •

edited

Loading

devguest07 commented Jul 9, 2024

melvin-bot bot commented Jul 11, 2024

melvin-bot bot commented Jul 11, 2024

mallenexpensify commented Jul 11, 2024

melvin-bot bot commented Jul 15, 2024

melvin-bot bot commented Jul 17, 2024

getusha commented Jul 18, 2024

getusha commented Jul 18, 2024

melvin-bot bot commented Jul 18, 2024

bernhardoj commented Jul 19, 2024

getusha commented Jul 19, 2024

bernhardoj commented Jul 20, 2024

melvin-bot bot commented Jul 22, 2024

mallenexpensify commented Jul 22, 2024

bernhardoj commented Jul 31, 2024

mallenexpensify commented Aug 12, 2024

bernhardoj commented Aug 13, 2024

melvin-bot bot commented Aug 14, 2024

mallenexpensify commented Aug 26, 2024

bernhardoj commented Aug 26, 2024

mallenexpensify commented Aug 26, 2024

techievivek commented Aug 29, 2024

techievivek commented Aug 29, 2024

getusha commented Aug 29, 2024

techievivek commented Aug 30, 2024

bernhardoj commented Aug 30, 2024

JmillsExpensify commented Aug 30, 2024

mallenexpensify commented Aug 30, 2024

[$250] Tag - No error when creating new tag with existing name when tag name has X: Y format #45067

[$250] Tag - No error when creating new tag with existing name when tag name has X: Y format #45067

Comments

lanitochka17 commented Jul 9, 2024 • edited by techievivek Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jul 9, 2024

lanitochka17 commented Jul 9, 2024

lanitochka17 commented Jul 9, 2024

bernhardoj commented Jul 9, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

devguest07 commented Jul 9, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore?

melvin-bot bot commented Jul 11, 2024

melvin-bot bot commented Jul 11, 2024

mallenexpensify commented Jul 11, 2024

melvin-bot bot commented Jul 15, 2024

melvin-bot bot commented Jul 17, 2024

getusha commented Jul 18, 2024

getusha commented Jul 18, 2024

melvin-bot bot commented Jul 18, 2024

bernhardoj commented Jul 19, 2024

getusha commented Jul 19, 2024

bernhardoj commented Jul 20, 2024

melvin-bot bot commented Jul 22, 2024

mallenexpensify commented Jul 22, 2024

bernhardoj commented Jul 31, 2024

mallenexpensify commented Aug 12, 2024

bernhardoj commented Aug 13, 2024

melvin-bot bot commented Aug 14, 2024

mallenexpensify commented Aug 26, 2024

bernhardoj commented Aug 26, 2024

mallenexpensify commented Aug 26, 2024

techievivek commented Aug 29, 2024

techievivek commented Aug 29, 2024

getusha commented Aug 29, 2024

techievivek commented Aug 30, 2024

bernhardoj commented Aug 30, 2024

JmillsExpensify commented Aug 30, 2024

mallenexpensify commented Aug 30, 2024

lanitochka17 commented Jul 9, 2024 •

edited by techievivek

Loading

bernhardoj commented Jul 9, 2024 •

edited

Loading