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

IuliiaHerets commented 2 months ago

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: 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): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

Workspace has independant tags set up on OD.

Go to staging.new.expensify.com
Go to workspace settings > Tags.
Click on any main tag.
Enable "Required" switch.
Select all the subtags > Disable.
Disable all the subtags.
Note that "Required" switch is locked and disabled.
Dismiss the RHP.
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:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/6bed2f7d-bc66-4c8b-95f6-ff1e9d1d5715

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[bot] commented 2 months ago

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 commented 2 months ago

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

IuliiaHerets commented 2 months ago

@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[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

Krishna2323 commented 2 months ago

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 https://github.com/Expensify/App/blob/12037eef23dfb54b73d05ea28bd0b7334a7ca153/src/libs/actions/Policy/Tag.ts#L230 https://github.com/Expensify/App/blob/12037eef23dfb54b73d05ea28bd0b7334a7ca153/src/libs/actions/Policy/Tag.ts#L257

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. https://github.com/Expensify/App/blob/12037eef23dfb54b73d05ea28bd0b7334a7ca153/src/libs/actions/Policy/Tag.ts#L764-L769

What alternative solutions did you explore? (Optional) 

Result

Shahidullah-Muffakir commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/Expensify/App/blob/12037eef23dfb54b73d05ea28bd0b7334a7ca153/src/libs/actions/Policy/Tag.ts#L286

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[bot] commented 2 months ago

📣 @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:

Make sure you've read and understood the contributing guidelines.
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.
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.
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[bot] commented 2 months ago

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

ikevin127 commented 2 months ago

♻️ Reviewing proposals.

ikevin127 commented 2 months ago

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

@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).
@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[bot] commented 2 months ago

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

ikevin127 commented 2 months ago

@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 https://github.com/Expensify/App/issues/47818#issuecomment-2304278094.

melvin-bot[bot] commented 2 months ago

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

ikevin127 commented 2 months ago

@puneetlath Callback to https://github.com/Expensify/App/issues/47818#issuecomment-2306068470, we need to decide which way to go with this issue.

melvin-bot[bot] commented 2 months ago

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

Krishna2323 commented 2 months ago

@puneetlath, friendly bump to check this comment.

@Krishna2323's https://github.com/Expensify/App/issues/47818#issuecomment-2303004480 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 commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 months ago

📣 @Shahidullah-Muffakir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

Krishna2323 commented 2 months ago

@puneetlath, the selected solution involves calling two API endpoints simultaneously. In my opinion, this approach could lead to multiple issues:

If the first call fails while the second call succeeds, the required field may be disabled even if subtags are enabled.
This approach deviates from our standard pattern, as we typically do not make two API calls simultaneously.

@ikevin127, what do you think?

ikevin127 commented 2 months ago

♻️ 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 commented 2 months ago

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

garrettmknight commented 2 months ago

@puneetlath / @ikevin127 might have a regression here

puneetlath commented 2 months ago

@Shahidullah-Muffakir can you take a look?

ikevin127 commented 2 months ago

@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 commented 2 months ago

@puneetlath @ikevin127 checking it out now.

Shahidullah-Muffakir commented 2 months ago

@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 : https://github.com/Expensify/App/blob/7dc25152d1595b7e58013de679fe26a7aa1634bd/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L103 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 commented 2 months ago

https://github.com/user-attachments/assets/6c12924a-20ae-4a1f-ba0b-9b74516f75b1

Shahidullah-Muffakir commented 2 months ago

Making the PR in few minutes

Shahidullah-Muffakir commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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.

https://github.com/user-attachments/assets/f3ac9b51-b468-4fb0-8615-c92c1da04f40

puneetlath commented 2 months ago

Doesn't sound like a regression to me.

ikevin127 commented 1 month ago

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-17] according to 3 days ago production deploy from https://github.com/Expensify/App/issues/48664#issuecomment-2338463845 (the PR is part of the deploy checklist).

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

cc @puneetlath @garrettmknight

puneetlath commented 1 month ago

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 commented 1 month ago

cc @puneetlath

puneetlath commented 1 month ago

@ikevin127 can you complete the checklist?

ikevin127 commented 1 month ago

[x] [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/43769
[x] [@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: https://github.com/Expensify/App/pull/43769#issuecomment-2362246763
[x] [@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.
[x] [@ikevin127] Determine if we should create a regression test for this bug.
[x] [@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

Go to Workspace Settings > Tags.
Click on any main tag to open its settings.
Enable the "Required" switch by toggling it on.
Select all the subtags associated with the main tag.
Disable all the selected subtags.
Verify that the "Required" switch becomes locked and is automatically toggled off.
Dismiss the Right-Hand Panel (RHP) by closing it.
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 commented 1 month ago

@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 commented 1 month ago

@puneetlath Offer aceepted, Thank you!

puneetlath commented 1 month ago

Great, all paid. Thanks everyone!

Expensify / App