[$500] Disabled tags are showing as required for "Multi level tags"

[m-natarajan]

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.43-12
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: @joeldavis
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708607040166479

Action Performed:

1. In OD as admin enable "People must tag expenses" for some workspace
2. Enable "Use multiple levels of tags"
3. Create/import some multi-level tags and mark at least one as disabled
4. Invite user B to the policy as employee
5. As employee go to the workspace chat invited to
6. Request money and enter the amount and click next

Expected Result:

Disabled tags shouldn't appear

Actual Result:

Disabled tag appear as required (Multi-level tags all show as required)

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019af1911417c9325b
• Upwork Job ID: 1760727691225997312
• Last Price Increase: 2024-02-22

Issue Owner

Current Issue Owner: @akinwale

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019af1911417c9325b

[melvin-bot]

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

[melvin-bot]

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

[dukenv0307]

Proposal

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

• Disabled tags are showing as required for "Multi level tags"

What is the root cause of that problem?

• I think the issue's title should be changed to: "No required tags are showing as required for "Multi level tags""
• We display required text based on the below:
  

  App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

  Line 777 in 94dc249

  rightLabel={isTagRequired ? translate('common.required') : ''}

• It is not correct in case of multiple levels tags, because in multiple levels tags, each tag already had a required field.

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

• In case of multiple levels tags, we can display the required text based on the required field in each tag. We can do it by updating the above logic to:

                    rightLabel={isMultipleLevelsTags ? (required ? translate('common.required') : '') : isTagRequired ? translate('common.required') : ''}

with

    const isMultipleLevelsTags = policyTagLists && policyTagLists.length > 1;

• We also need to update isSupplementary
• Note: BE need to return the required field for each tag in multiple levels tags beside the name and tags fields

What alternative solutions did you explore? (Optional)

• NA

[FitseTLT]

Proposal

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

Disabled tags are showing as required for "Multi level tags"

What is the root cause of that problem?

Currently we are displaying the tags menu without checking whether it is disabled or not here

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Lines 761 to 766 in 29ad878

	..._.map(policyTagLists, ({name}, index) => ({
	item: (
	<MenuItemWithTopDescription
	key={name}
	shouldShowRightIcon={!isReadOnly}
	title={TransactionUtils.getTag(transaction, index)}

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

When we disable a tag on OD it's required field is set to false so we can filter out tags with required false before displaying here

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Lines 761 to 766 in 29ad878

	..._.map(policyTagLists, ({name}, index) => ({
	item: (
	<MenuItemWithTopDescription
	key={name}
	shouldShowRightIcon={!isReadOnly}
	title={TransactionUtils.getTag(transaction, index)}

 ..._.map(
            _.filter(policyTagLists, ({required}) => required !== false),
            ({name}, index) => ({
                item: (
                    <MenuItemWithTopDescription
                        key={name}

What alternative solutions did you explore? (Optional)

[akinwale]

Since this is a fairly straightforward issue, we can go with the first correct solution, which can be found in @dukenv0307's proposal.

🎀👀🎀 C+ reviewed.

[melvin-bot]

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Christinadobrzyn]

awesome! We'll wait for @MonilBhavsar's review of #37095 (comment)

[MonilBhavsar]

@akinwale thanks for reviewing. I feel like @FitseTLT 's proposal makes more sense. If tag is disabled, we should not simply render the item rather than checking if it is required or not. What do you think?

[dukenv0307]

@MonilBhavsar With multiple levels tags, we just have a field required for each tag list. There is no disabled field for each tag list

[MonilBhavsar]

Thanks for quick response. I'll take a look and confirm

[MonilBhavsar]

Okay thanks makes sense @dukenv0307
Do you think the variable isTagRequired needs to have the updated condition?
and why we just update the label, we also need to update isSupplementary prop

Note: BE need to return the required field for each tag in multiple levels tags beside the name and tags fields

It is already sent, no?

[dukenv0307]

Do you think the variable isTagRequired needs to have the updated condition?

@MonilBhavsar Previously, when we do not have multiple levels tags, we use isTagRequired. Now we have multiple levels tags and single tag. So multiple levels tags will use required field in each tag list, and the single tag will use isTagRequired. So I think we do not need to update isTagRequired

[dukenv0307]

why we just update the label, we also need to update isSupplementary

Yeah, make sense since currently the isSupplementary is !!isTagRequired

[MonilBhavsar]

Can you please update your proposal then

[dukenv0307]

@MonilBhavsar I updated proposal to match the comment

[dukenv0307]

It is already sent, no?

Previously, it sent. But now didn`t. Maybe it is BE issue

[MonilBhavsar]

It should be sent in policyTags_ key. Can you please double check

[dukenv0307]

I just tested and here is from my side:

• There is no required field

[dukenv0307]

Here is the tags.csv in case you need it
independentTags.csv

[MonilBhavsar]

The structure looks odd to me.
Here's what I am seeing. May be you can try sign out and sign in?

[dukenv0307]

@MonilBhavsar Thanks. Now the required field is returned from my side

[MonilBhavsar]

Thanks! It will be great to test the PR with single level and multi level tags

[melvin-bot]

❌ There was an error making the offer to @akinwale for the Reviewer role. The BZ member will need to manually hire the contributor.

[melvin-bot]

❌ There was an error making the offer to @dukenv0307 for the Contributor role. The BZ member will need to manually hire the contributor.

[Christinadobrzyn]

I think this is right,

I hired @akinwale and @dukenv0307 in Upwork - https://www.upwork.com/jobs/~019af1911417c9325b

[Christinadobrzyn]

PR in the works #37388

[Christinadobrzyn]

PR in the works #37388

[Christinadobrzyn]

PR in the works #37388

[Christinadobrzyn]

Monitoring #37388

[Christinadobrzyn]

I think we're still watching PR #37388 which should be done soon - is that correct @MonilBhavsar?

[MonilBhavsar]

Yes, PR is merged. Should be deployed this week

[MonilBhavsar]

@dukenv0307 @akinwale we have a regression here. Can anyone please take a look #39916 (comment)

[akinwale]

@Christinadobrzyn Automation missed this. Payment for this can be processed (deployed to production 2024-04-10, payment due 2024-04-17) as #39916 is not being considered as a regression since the cause is a backend issue.

[akinwale]

@Christinadobrzyn bump on the above. Thanks.

[Christinadobrzyn]

Ah! Thanks for the nudge!

Payouts due:

• Contributor: $500 @dukenv0307 (paid in Upwork - https://www.upwork.com/nx/wm/offer/101112150)
• Contributor+: $500 @akinwale (paid in Upwork - https://www.upwork.com/nx/wm/offer/101112149)

Upwork job is here.

@akinwale Should we have a regression test for this?

[Christinadobrzyn]

DMd @akinwale about the regression test

[akinwale]

• [@akinwale] The PR that introduced the bug has been identified. Link to the PR:
• [@akinwale] 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:
• [@akinwale] 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:

Not a regression. Specifically handling the display of the required label on multi-level tags was not implemented.

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

• In OD, as admin, enable the "People must tag expenses" option for a workspace
• In OD, as admin, enable "Use multiple levels of tags
• Create or import a few multi-level tags and mark at least one as not required

Test Steps

1. As an admin, Invite an employee to the workspace.
2. As the employee, navigate to the workspace chat invited to.
3. Request money, enter an amount and click Next.
4. Click on the More button to display the rest of the list items.
5. Verify that only tags that have Required enabled in OD appear as required in ND (The text 'Required' is displayed next to the name of the corresponding tag in the list).

Do we agree 👍 or 👎?

[akinwale]

@Christinadobrzyn Done!

[Christinadobrzyn]

Created regression test - https://github.com/Expensify/Expensify/issues/395323

Closing!