[Pay when ntdiary is setup for NewDot payments][$500] Multilevel tag - Tag name in RHP is not the same as the tag being clicked on

[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.47-2
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• User is an employee of Collect workspace.
• The Collect workspace has dependant multilevel tag set up. (details here for how to setup)
• The multilevel tag is as follow:
  First level: State
  Second level: Region

1. Go to workspace chat.
2. Create a manual request with first and second level of tag.
3. Go to request details page.
4. Click the first level tag (State).
5. Click the second level tag (Region).

Multi Level tag
Dependent - Multi Level tags NEW.csv

Expected Result:

When the respective level tag is opened, the RHP will show the correct tag name (Step 4 and 5).

Actual Result:

In Step 4, when opening RHP for first level tag (State), the tag name is the second level tag (Region).
In Step 5, when opening RHP for second level tag (Region), the tag name is the second level tag (State).

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

Bug6403264_1709667194180.20240305_200515.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fff45536696176bf
• Upwork Job ID: 1765168266617380864
• Last Price Increase: 2024-03-10
• Automatic offers:
• ntdiary | Reviewer | 0
• tienifr | Contributor | 0

[melvin-bot]

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

[kbecciv]

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

[kbecciv]

We think that this bug might be related to #wave6-collect-submitters
CC @greg-schroeder

[gijoe0295]

Proposal

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

Multilevel tag - Tag name in RHP is not the same as the tag being clicked on

What is the root cause of that problem?

In MoneyRequestView, we sort the policy tags object by orderWeight here and here, then use the indexes of that ordered array as tagIndex to identify different tags.

Later in IOURequestStepTag, we used getTagListName to retrieved the tag name based on tagIndex. The problem is getTagListName used policyTags, which is an unordered object taken straight from Onyx.

In short, we first process the policy tags object from Onyx to an array sorted based on orderWeight but later when we retrieve the tag name, we used the plain tags object from Onyx, that causes the order of tags to be messed up.

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

We should modify the getTagListName to use the sorted/processed version of policy tags:

return getTagList(policyTagList, tagIndex).name;

Note: getTagLists and getTagList are different.

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @ntdiary (Internal)

[mallenexpensify]

@ntdiary can you help me here? I just spent ~20 mins trying to get the account setup to test.

1. On a collect plan, on expensify.com, I'm logged in as mallen@expensifail.com and I invited my test account

2. On staging.new.expensify.com I'm logged in with my test account

3. I don't see the workspace in LHN as an option

Anything you know of that I'm doing wrong?

[gijoe0295]

@mallenexpensify Please take a look at QA steps 1 - 3 from #34127. Also you need to log out then log in again then the collect workspace is there. If that still didn't work, please try to create a money request to the collect workspace from employee account by the bottom bar + button.

[Krishna2323]

Proposal

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

Multilevel tag - Tag name in RHP is not the same as the tag being clicked on

What is the root cause of that problem?

We display sorted tags by name, but we forgot to use getTagLists to sort before passing policyTagLists to getTagListName.

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 136 in a04481f

const policyTagLists = useMemo(() => PolicyUtils.getTagLists(policyTagList), [policyTagList]);

App/src/libs/PolicyUtils.ts

Lines 179 to 188 in a04481f

	function getTagLists(policyTagList: OnyxEntry<PolicyTagList>): Array<PolicyTagList[keyof PolicyTagList]> {
	if (isEmptyObject(policyTagList)) {
	return [];
	}
	return Object.values(policyTagList)
	.filter((policyTagListValue) => policyTagListValue !== null)
	.sort((tagA, tagB) => tagA.orderWeight - tagB.orderWeight);
	}

App/src/pages/iou/request/step/IOURequestStepTag.js

Line 76 in a04481f

const policyTagListName = PolicyUtils.getTagListName(policyTags, tagIndex);

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

We need to pass sorted policyTagLists to getTagListName.

    const policyTagLists = useMemo(() => PolicyUtils.getTagLists(policyTags), [policyTags]);
    const policyTagListName = PolicyUtils.getTagListName(policyTagLists, tagIndex);
    
    // Update getTagListName
    function getTagListName(policyTagList: PolicyTag[], tagIndex: number): string {
    if (!policyTagList.length) {
        return '';
    }
    return policyTagList[tagIndex]?.name ?? '';
}

Result

tag_list_name.mp4

[Krishna2323]

@mallenexpensify, I think this can be external.

[ntdiary]

Anything you know of that I'm doing wrong?

@mallenexpensify, I guess you forgot the step 2 (i.e., executing the js code) before inviting the employee account.
After executing it, then invite the test account , I can access the test workspace. 😄

[mallenexpensify]

I guess you forgot the step 2 (i.e., executing the js code)

Likely, thanks @ntdiary, are you able to reproduce? If so, do you think this can be external?

[ntdiary]

I guess you forgot the step 2 (i.e., executing the js code)

Likely, thanks @ntdiary, are you able to reproduce? If so, do you think this can be external?

@mallenexpensify, sure, I can reproduce it, and this can be fixed in NewDot, so I think this can be external. But I'm going to bed, so I can only review proposals tomorrow, if it's urgent, we can apply another c+ here. :)

[ntdiary]

Note: we all know that the code incorrectly uses the sorted index and the index generated by Object.keys, but when we raise a proposal, it would be better to take care of the possible impact of the modification, this logic is using the index generated by Object.keys :)

App/src/libs/ModifiedExpenseMessage.ts

Lines 197 to 198 in 401bb21

	Object.keys(policyTags).forEach((policyTagKey, index) => {
	const policyTagListName = PolicyUtils.getTagListName(policyTags, index) || localizedTagListName;

[melvin-bot]

Current assignee @ntdiary is eligible for the External assigner, not assigning anyone new.

[mallenexpensify]

Thx @ntdiary , added External

[tienifr]

Proposal

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

In Step 4, when opening RHP for first level tag (State), the tag name is the second level tag (Region).
In Step 5, when opening RHP for second level tag (Region), the tag name is the second level tag (State).

What is the root cause of that problem?

When we try to edit the tag, it will open a route with a tagIndex of 0, 1, ... This is the index of the tag in the list after sorted by orderWeight (let's call sorted index)

But when retrieving the tag name here, it's using the unsorted list to get the name, causing the switching issue in the OP.

Meanwhile, in other places like here, it's using the unsorted index to pass to getTagListName.

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

Part of the confusion here is that we're relying on index as a source of truth to pinpoint which tag we're referring to. I don't think this is a good practice because a same index can refer to different things on the same list with different ordering, like in this case.

Instead, the tag orderWeight will uniquely identify the tags in a list with any kind of ordering.

So we should (suggested in here):

• Use the orderWeight as the id to put in the route instead of tagIndex
• In the pages, get the orderWeight from the route
• Pass the orderWeight down to lower components instead of propagating the "dangerous" usage of tagIndex, like here
• Modify related methods like getTagListName to get the tag name based on orderWeight
• Double check other usage of tagIndex and try to use orderWeight instead (where applicable)

This approach will also work fine because the orderWeight can also uniquely identify a tag, similar to the tagKey

What alternative solutions did you explore? (Optional)

• We can sort the list in getTagListName before getting the name value (by using getTagList)
• In here, also sort the tags before doing the forEach to make sure the index is a sorted index (it will cause regressions without this step)
• Double check all other places that use getTagListName to make sure it passes a sorted index

[ntdiary]

Instead, the tag id (aka the tagKey) will uniquely identify the tags in a list with any kind of ordering.

@tienifr, Which field does tag id refer to? Is it something like these keys of Object (City/Region/State)? 😂

[tienifr]

@ntdiary PR #38988 is ready to review

[melvin-bot]

This issue has not been updated in over 15 days. @ntdiary, @NikkiWines, @mallenexpensify, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mallenexpensify]

PR hit staging two days ago

[melvin-bot]

This issue has not been updated in over 15 days. @ntdiary, @NikkiWines, @mallenexpensify, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mallenexpensify]

Eeeps, that damn Reviewing label wasn't removed and this didn't have 'awaiting payment', updated title AND it didn't go overdue. grrrrr......

Contributor: @tienifr paid $500 via Upwork
Contributor+: @ntdiary owed $500 via NewDot (he's in process of getting his account dialed in)

[mallenexpensify]

@ntdiary can you update the BZ checklist plz?

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:

[ntdiary]

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:

• [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: [CP Staging]Always sorting tags by orderWeight #37681
• [@ntdiary] 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: https://github.com/Expensify/App/pull/37681/files#r1601534718
• [@ntdiary] 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: No need
• [@ntdiary] Determine if we should create a regression test for this bug. Yes
• [@ntdiary] 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. As shown below
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Regression test:

Precondition:

1. User is an employee of Collect workspace.
2. The Collect workspace has dependant multilevel tag set up. (e.g., State/Region)

Steps:

1. Go to workspace chat.
2. Create a manual request with first and second level of tag.
3. Go to request details page.
4. Click the first level tag (State), verify the title of the RHP header is State.
5. Click the second level tag (Region), verify the title of the RHP header is Region.

[mallenexpensify]

From above

Contributor+: @ntdiary owed $500 via NewDot (he's in process of getting his account dialed in)

@ntdiary should be setup for NewDot payment now or very, very soon, so I'm going to close this and they can request payment once they're dialed in.

TestRail GH is here

• https://github.com/Expensify/Expensify/issues/399450

[JmillsExpensify]

$500 approved for @ntdiary