[HOLD for Payment 2024-09-17] [$250] Multi tags - "Required" switch is enabled after disabling all subtags & reopening subtag list

[IuliiaHerets]

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: v9.0.23-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp https://expensify.testrail.io/index.php?/tests/view/4880485
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

• Workspace has independant tags set up on OD.

1. Go to staging.new.expensify.com
2. Go to workspace settings > Tags.
3. Click on any main tag.
4. Enable "Required" switch.
5. Select all the subtags > Disable.
6. Disable all the subtags.
7. Note that "Required" switch is locked and disabled.
8. Dismiss the RHP.
9. Click on the same main tag.

Expected Result:

"Required" switch will remain locked and disabled.

Actual Result:

Required" switch is enabled after disabling all subtags & reopening subtag list.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6578591_1724267622859.20240822_030758.mp4

Bug6578591_1724267622842!Independent.-.Multi.Level.tags.csv

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0135a412ea484831b7
• Upwork Job ID: 1826359908145270693
• Last Price Increase: 2024-08-28
• Automatic offers:
• ikevin127 | Reviewer | 103741874

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

Triggered auto assignment to @puneetlath (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.

[IuliiaHerets]

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

[IuliiaHerets]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0135a412ea484831b7

[melvin-bot]

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

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-08-21 21:02:12 UTC.

Proposal

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

Multi tags - "Required" switch is enabled after disabling all subtags & reopening subtag list

What is the root cause of that problem?

The required value is only toggled in the optimistic data, when the backend returns the response the required value is again turned to required.
https://github.com/Expensify/App/blob/12037eef23dfb54b73d05ea28bd0b7334a7ca153/src/libs/actions/Policy/Tag.ts#L218C29-L218C53

App/src/libs/actions/Policy/Tag.ts

Line 230 in 12037ee

...(shouldDisableRequiredTag ? {pendingFields: {required: null}} : {}),

App/src/libs/actions/Policy/Tag.ts

Line 257 in 12037ee

...(shouldDisableRequiredTag ? {pendingFields: {required: null}, required: policyTag.required} : {}),

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

• We should pass the required value to the backend through parameters object.
• We might also need to update the backend to return the required value accordingly.
• The implementation will be the same as we do in setPolicyTagsRequired.
  

  App/src/libs/actions/Policy/Tag.ts

  Lines 764 to 769 in 12037ee

  	const parameters: SetPolicyTagsRequired = {
  	policyID,
  	tagListIndex,
  	requireTagList: requiresTag,
  	};

What alternative solutions did you explore? (Optional)


Result

[Shahidullah-Muffakir]

Proposal

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

When all subtags are disabled, the "Required" toggle should be toggled off. However, it is currently showing as toggled on, which creates confusion for users.

What is the root cause of that problem?

Currently, the "Required" state is only updated when the user manually toggles the Required Toggle. The setPolicyTagsRequired function is invoked solely when the user interacts with the toggle. Consequently, when all subtags are disabled, this function is not called, resulting in the "Required" toggle remaining on. To update the "Required" state correctly, we should call this function whenever getHeaderButtons is executed. If the number of enabled tags is zero, the required button should be turned off.

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

To address this issue, we can implement a check to see if any tags are enabled. If not, we should call the setPolicyTagsRequired function to turn off the required toggle. The suggested code change is as follows:

    if (!Object.values(currentPolicyTag?.tags ?? {}).some((tag) => tag.enabled)) {
        Tag.setPolicyTagsRequired(policyID, false, route.params.orderWeight)
    }

Here is the video afterMy Updates:
https://github.com/user-attachments/assets/ad799020-db34-4c13-9ac5-65581db542c5

[nkdengineer]

I think this is a BE bug since we can calculate required value via tagListIndex and tags data as we do in optimistic data.

[struc]

Proposal

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

Disabling all subtags (individually or in bulk) should make the main tag not required.

What is the root cause of that problem?

We should update the required attribute on the main tag, if all the subtags are disabled but we are not.

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

I think the simplest solution is to just make a call to setPolicyTagsRequired when all the subtags are disabled.
This should also handle the case when the user disable the subtags one by one as well.

So right after

App/src/libs/actions/Policy/Tag.ts

Line 286 in 12037ee

API.write(WRITE_COMMANDS.SET_POLICY_TAGS_ENABLED, parameters, onyxData);

We can check if the tag should be marked as not required, then call:

    if (shouldDisableRequiredTag) {
        setPolicyTagsRequired(policyID, false, tagListIndex);
    }

What alternative solutions did you explore? (Optional)

Handle this situation on the backend.

[melvin-bot]

📣 @struc! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[ikevin127]

✅ I've reviewed the existing proposals and we have the following two acceptable options:

1. @Krishna2323's proposal which proposes as solution to fix this from the BE side, since the FE already sends all the required parameters. The proposal was reinforced by another contributor in #47818 (comment).
2. @Shahidullah-Muffakir's proposal which proposes as solution to fix this from the FE side such that if all sub-tags are disabled, we should have a check in the FE which would set Required as disabled and locked.

I need some help from the assigned CME here to decide the final way to go, I tend to lean towards the first proposal as the right way to do this.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[ikevin127]

@struc Thanks for your proposal. Unfortunately we're generally are not in favour of having separate calls as a result of one call which already sends all the required parameters needed in order to fix this issue from the BE side as mentioned in #47818 (comment).

[melvin-bot]

@puneetlath, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ikevin127]

@puneetlath Callback to #47818 (comment), we need to decide which way to go with this issue.

[melvin-bot]

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

[Krishna2323]

@puneetlath, friendly bump to check this comment.

@Krishna2323's #47818 (comment) which proposes as solution to fix this from the BE side, since the FE already sends all the required parameters.

@ikevin127, just to clarify, we need to send requireTagList param from the front-end, currently it's not included in the parameters.

[puneetlath]

I think I prefer the front-end solution. Let's go with that.

[ikevin127]

♻️ I'm fine with CME's decision, if we encounter issues with the selected solution and we do need to have BE changes I'm sure we will sort things out here on the issue such that everybody is compensated fairily.

@Shahidullah-Muffakir Please us know when we can expect a PR to be ready for review 🧑‍💻

[Shahidullah-Muffakir]

Thank you, I will submit the PR within 24 hours.

[garrettmknight]

@puneetlath / @ikevin127 might have a regression here

[puneetlath]

@Shahidullah-Muffakir can you take a look?

[ikevin127]

@Shahidullah-Muffakir Can you please look into the regression issue and if it indeed comes from our PR, you'd need to have a follow-up PR to fix the regression. Thanks in advance!

[Shahidullah-Muffakir]

@puneetlath @ikevin127 checking it out now.

[Shahidullah-Muffakir]

@puneetlath @ikevin127

To hide the "Required" label when all tags are disabled, we should implement a similar check to what we applied for the Required toggle. The current check only verifies if the policyTagList is required, but it doesn't account for the scenario where all subtags are disabled

in this :

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

Line 103 in 7dc2515

labelText={policyTagList.required ? translate('common.required') : undefined}

We can update the logic as follows:
labelText={policyTagList.required && !!Object.values(policyTagList?.tags ?? {}).some((tag) => tag.enabled) ? translate('common.required') : undefined}
This way, the label will only appear if the main tag is required and at least one subtag is enabled.

Additionally, I'm not sure if this issue is a regression—it seems more like a separate bug that was overlooked in the previous implementation.

[Shahidullah-Muffakir]

Screen.20Recording.202024-09-06.20at.2011.mp4

[Shahidullah-Muffakir]

Making the PR in few minutes

[Shahidullah-Muffakir]

@ikevin127 Could you guide me on the process for submitting a fix? Should I go ahead and create a PR directly, or is there anything else I need to do before proceeding? Thank you.

[ikevin127]

@Shahidullah-Muffakir Sure, you can go ahead and open a follow-up PR, tagging the other issue in the FIXED ISSUE section and leave blank for proposal - anything else is the same as any other PR.

You can also post in the other issue once you opened the PR to let people know that you opened PR with a fix as a follow-up.

Whether the issue was caused by your initial PR or not, we will determine soon and depending on the result we both will either have a penalty here (-50%) if it was indeed caused by the initial PR or possibly no penalty and a separate bounty for the other issue if it was indeed present before the initial changes.

[Shahidullah-Muffakir]

Thanks for the clarification, @ikevin127! I’ve already opened the follow-up PR and tagged the other issue.

I hope this issue wasn’t caused by my initial PR and that it doesn’t lead to a penalty. :)
Thanks again for your help!

[ikevin127]

Additionally, I'm not sure if this issue is a regression—it seems more like a separate bug that was overlooked in the previous implementation.

Regarding the regression, just looked into whether the issue exists on production where the PR was not deployed yet and it looks like it does, meaning the bug was indeed overlooked in a previous implementation (see video below).

The reason it surfaced now is because we fixed the issue of Required staying OFF and Locked and not going back to being ON after the RHP is re-opened. Whether this should be considered regression or not, I'll defer that to @puneetlath.

prod.mov

[puneetlath]

Doesn't sound like a regression to me.

[ikevin127]

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-17] according to 3 days ago production deploy from #48664 (comment) (the PR is part of the deploy checklist).

⚠️ We also don't have a BZ team member assigned for payment.

cc @puneetlath @garrettmknight

[puneetlath]

Updated the title. I'm a BZ member so I can handle payment. (I'm a bit different than most in that I'm both on the BZ team and on the engineering team).

[ikevin127]

cc @puneetlath

[puneetlath]

@ikevin127 can you complete the checklist?

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Hides required toggle in case dependent tags is enabled #43769
• [@ikevin127] 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: Hides required toggle in case dependent tags is enabled #43769 (comment)
• [@ikevin127] 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.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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. Go to Workspace Settings > Tags.
2. Click on any main tag to open its settings.
3. Enable the "Required" switch by toggling it on.
4. Select all the subtags associated with the main tag.
5. Disable all the selected subtags.
6. Verify that the "Required" switch becomes locked and is automatically toggled off.
7. Dismiss the Right-Hand Panel (RHP) by closing it.
8. Reopen the same main tag and verify that the "Required" switch remains off and locked, reflecting the disabled state of the subtags.

Do we agree 👍 or 👎.

cc @puneetlath

[puneetlath]

@ikevin127 has been paid.

@Shahidullah-Muffakir your offer is here: https://www.upwork.com/nx/wm/offer/104071660. Please ping me on this issue when you've accepted it.

[Shahidullah-Muffakir]

@puneetlath Offer aceepted, Thank you!