[HOLD for payment 2024-02-07] [$500] [MEDIUM] Tags: Selected tag in Parent: Child format shows up multiple times in the search result list

[kbecciv]

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: 1.4.5.3
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #31775

Action Performed:

Precondition:

• The workspace has multiple tags in Parent: Child format added on OD.
• The workspace has more than 8 tags.

1. Log in to ND as employee.
2. Go to the workspace chat > + > Request money.
3. Create a manual request with a tag in Parent: Child format.
4. Click on the expense preview twice to open expense details page.
5. Click on Tag.
6. Search for the selected tag in the search field.

Expected Result:

The selected tag will not show up multiple times.

Actual Result:

The selected tag shows up multiple times.
This issue only happens with tags in Parent: Child format.

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6294857_1701277413365.bandicam_2023-11-29_23-43-36-515.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e80f76d4f90d5b88
• Upwork Job ID: 1729913749207519232
• Last Price Increase: 2023-11-29
• Automatic offers:
• FitseTLT | Contributor | 28065674
• jjcoffee | Contributor | 28116028

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[ZhenjaHorbach]

@FitseTLT

It looks like a regression
Or not
But obviously on your part)

[FitseTLT]

Yep regression

Need to take the backslash cleaning code to here

App/src/libs/PolicyUtils.ts

Lines 187 to 188 in 70813db

	function getTagList(policyTags: OnyxCollection<PolicyTags>, tagKey: string) {
	if (Object.keys(policyTags ?? {})?.length === 0) {

[FitseTLT]

But @jjcoffee Can we utilize migrateOnyx here to centralize cleaning of the policyTags so that we can ensure the policyTags are the cleaned version everywhere in the App to avoid future regressions ??

[barros001]

I think this is better fixed on the backend.

In general if you're getting data from a BE and you need to modify it for presentation purposes, you do not really modify the data itself, you only modify it when presenting it on the screen. The reason for this is that the backend will send you Test\: Child tag, and if you modify the data and use it throughout the front end, when you send it back to the BE, you will send it as "Test: Child". We don't want to send data to BE that's different than what the BE gave to you. It's the same concept when you need to display the user input back on an HTML page. You don't escape it when you read the data, you only escape when you're rendering it. Otherwise you might end up saving/sending to BE the escaped version of the input, which is wrong.

Also, there seem to be some of inconsistencies. The API that returns all tags for a given Policy will add the \ to the names, but the openReport api (transaction.tag) will not. I would expect all tags coming from the backend to have \ added to the names.

Fixing the backend so that slashes are not added in the first place is the proper way to fix it IMO.

[FitseTLT]

I know it is best to fix it on BE @barros001 but it is not and it is decided it should be treated in FE that's why we worked on it maybe to treat the case where the user's have already created Parent: child format tags. But @jjcoffee the perfect solution to this case is use our migrateOnyx isn't it? if not I will try to fix this regression and also create a solution to my ability to avoid future regression. What do u think?

[jjcoffee]

@FitseTLT Would you mind writing up your proposed fix in the proposal template format? Just so we can understand fully before moving ahead with it.

[FitseTLT]

@jjcoffee I have already raised a pr Have u checked it???

[jjcoffee]

@FitseTLT Yes, but I'd like to understand the fix before going ahead with it.

[FitseTLT]

Proposal

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

Selected tag in Parent: Child format shows up multiple times in the search result list

What is the root cause of that problem?

The root cause is

App/src/components/TagPicker/index.js

Lines 46 to 47 in e0d34b8

	const tags = [...selectedOptions, ..._.filter(policyTagList, (policyTag) => policyTag.enabled && !selectedNames.includes(policyTag.name))];
	return tags;

the filter here doesn't work as intended because policyTagList contains the uncleaned tags (from backslashes) while the selected tag is the cleaned tag so it is not filtered out and one from the policyTagList and one for the selected appear in the search result.

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

We should clean policy tags in PolicyUtils.getTagList like this

function getTagList(policyTags: OnyxCollection<PolicyTags>, tagKey: string) {
    if (Object.keys(policyTags ?? {})?.length === 0) {
        return {};
    }

    const policyTagKey = tagKey ?? Object.keys(policyTags ?? {})[0];
    const tags = cloneDeep(policyTags?.[policyTagKey]?.tags ?? {});

    // This is to remove unnecessary escaping backslash in tag name sent from backend for "Parent: Child" type of tags.
    Object.keys(tags).forEach((key) => {
        const cleanedTagName = tags[key]?.name?.replace(/\\{1,2}:/g, ':');
        if (cleanedTagName) {
            tags[key].name = cleanedTagName;
        }
    });

    return tags;
}

What alternative solutions did you explore? (Optional)

[jjcoffee]

Okay so by moving the cleanup to PolicyUtils.getTagList, we're ensuring that anywhere we get the tags, they'll always be the cleaned ones. That sounds good to me!

🎀👀🎀 C+ reviewed

[amyevans]

Thanks @FitseTLT! At least one platform video, please

[FitseTLT]

Added Android mWeb video.

[melvin-bot]

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

[melvin-bot]

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

• clean tag for old money confirmation list #35054

If no regressions arise, payment will be issued on 2024-02-07. 🎊

For reference, here are some details about the assignees on this issue:

• @jjcoffee requires payment automatic offer (Contributor)
• @FitseTLT requires payment automatic offer (Contributor)

[melvin-bot]

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:

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

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - behaviour has always been there
• 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

1. Create some hierarchial tags for a workspace in the form of "Parent: Child" in oldDot.
2. Go to that workspace's chat on newDot and tap the + in the composer to request money
3. Choose manual and enter any amount and proceed to confirmation page
4. Click Show more and then Tag
5. Search for a hierarchical tag and verify that each tag only displays once in the results.
6. Verify also that the hierarchical tags you have created in (1) appear as "Parent: Child" without any slashes before the colon.
7. Select a hierarchical tag and verify that it displays correctly on the confirmation page, as well as after you submit the request on the request details page.

Do we agree 👍 or 👎

[greg-schroeder]

Okay ... so this is a super confusing saga related to this. From what I can tell:

• This issue stemmed from a regression from [$500] [Wave 6: Tags] Tag written as "Parents: Child" appears as "Parents\: Child" #30734.
• PR 1 for this issue: fixed duplicate search result of the selected parent:child tag #32365
• PR 2 for this issue: clean tag for old money confirmation list #35054
• Based on Upwork records, both @jjcoffee and @FitseTLT were paid $500 in [$500] [Wave 6: Tags] Tag written as "Parents: Child" appears as "Parents\: Child" #30734

I don't think a regression penalty was actually applied from what I can tell, so based on everything here I don't think we will issue additional payments for this issue, as it's fixing multiple regressions with the same C/C+ involved, who were already paid the full price of the original.

[greg-schroeder]

filed regression test per #32203 (comment)

[jjcoffee]

@greg-schroeder I haven't received any payments on #30734. Since that issue was closed in favour of this one, I just ended the Upwork contract for it, maybe that's where the confusion stems from?

(Note Milestones paid is $0)

[greg-schroeder]

Hmm. When I looked at this yesterday it showed Paid, let me check again.

[greg-schroeder]

Ah. Well. It shows paid for @FitseTLT:

However, you're right, it just shows as Accepted for you. That's my bad, I just assumed both had been paid especially given the comments in the linked issue

[greg-schroeder]

I generated a new offer @jjcoffee - I'll pay ASAP once you accept, sorry about that!

[jjcoffee]

@greg-schroeder Haha oh I understand the confusion then, no worries at all! Offer accepted, thanks for looking into it!

[jjcoffee]

@greg-schroeder Friendly bump to complete the payout on the offer 🙇

[greg-schroeder]

done, sorry