Expensify
Tag - No error when creating new tag with existing name when tag name has X: Y format
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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?
- [x] Android: Native
- [x] Android: mWeb Chrome
- [x] iOS: Native
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/355b6472-f6b5-476a-890f-44205561a325
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.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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
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.
https://github.com/Expensify/App/blob/9c32293ab6c0465eec38f1c573b8b5a09e1e112d/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L48-L49
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. https://github.com/Expensify/App/blob/9c32293ab6c0465eec38f1c573b8b5a09e1e112d/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L43-L49
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.
https://github.com/Expensify/App/blob/9cd3257a099cf30fbf029f2bc2aa7ef74b61b445/src/libs/PolicyUtils.ts#L289-L291
To fix it, we only want to replace \: with :.
tag?.replace(/\\:/g, CONST.COLON);
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.
https://github.com/Expensify/App/blob/1534ac2182ab361d50d75ec9c75982446f5ff65f/src/libs/PolicyUtils.ts#L295
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
escapeTagNamefunction - 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 https://github.com/Expensify/App/blob/5ce4e60a9a5671634a9be88dbaf5d1ae313ccf81/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L30
As we do for the display, we can use getCleanedTagName to make sure we have the same values as displayed on the tags page.
https://github.com/Expensify/App/blob/d576e2829a455d8aa422feadb2bf88dea375ced1/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L107
- 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. https://github.com/Expensify/App/blob/5ce4e60a9a5671634a9be88dbaf5d1ae313ccf81/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L48
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 (External)
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
@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...
@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.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@getusha the solution works fine for me.
https://github.com/user-attachments/assets/e251349c-8183-4972-8ae3-3b675bda5632
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 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.
https://github.com/Expensify/App/blob/fa738c641e7d2b289850da7b4be11369c33b96d6/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L108
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);).
https://github.com/Expensify/App/blob/fa738c641e7d2b289850da7b4be11369c33b96d6/src/libs/PolicyUtils.ts#L289-L291
@mallenexpensify, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!
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?
Yes, could you update your proposal to include that case? thanks
@mallenexpensify @getusha this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@mallenexpensify, @techievivek, @getusha Huh... This is 4 days overdue. Who can take care of this?
@getusha owes $500 from https://github.com/Expensify/App/issues/32699 , let's use this issue to knock it down by $250, then we'll find another $250 one.
I opened a new PR to handle the edit tag case.
cc: @getusha @techievivek
⚠️ 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.