[HOLD for payment 2024-03-13] [$500] Workspace switcher - LHN does not show IOU report and details page under "workspace" filter

[lanitochka17]

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-37.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:

Action Performed:

1. Create a new workspace
2. Invite a member to the workspace
3. With the newly created workspace as the selected workspace in workspace switcher (important), go to #announce room
4. Create a split bill
5. Click on the new 1:1 DM in LHN
6. Click on the preview in the main chat
7. Click on the preview in IOU report

Expected Result:

In Step 7 and 8, LHN will display the IOU report and IOU details page when these pages are open

Actual Result:

In Step 7 and 8, LHN does not display the IOU report and IOU details page when these pages are open

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

Bug6369798_1707250830181.bandicam_2024-02-07_01-36-30-108.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c9531d328a61c517
• Upwork Job ID: 1754981246357303296
• Last Price Increase: 2024-02-20
• Automatic offers:
• mollfpr | Reviewer | 0
• tienifr | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-split-p2p-chat-groups

[Tony-MK]

Proposal

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

Workspace switcher - LHN does not show IOU report and details page under "workspace" filter

What is the root cause of that problem?

We use getPolicyMembersByIdWithoutCurrentUser function to get policyMemberAccountIDs without the current user.

App/src/pages/home/sidebar/SidebarLinksData.js

Line 137 in 7f4cdce

const policyMemberAccountIDs = getPolicyMembersByIdWithoutCurrentUser(policyMembers, activeWorkspaceID, accountID);

When filtering the reports to display, we check if report.policyID is present, then return true if a single policy member is present in policyMemberAccountIDs.

App/src/libs/SidebarUtils.ts

Lines 176 to 177 in 7f4cdce

	reportsToDisplay = reportsToDisplay.filter((report) => ReportUtils.doesReportBelongToWorkspace(report, policyMemberAccountIDs, currentPolicyID));
	}

However, the policy members are an empty array because no other member is invited.

App/src/libs/ReportUtils.ts

Lines 898 to 902 in 7f4cdce

	function doesReportBelongToWorkspace(report: Report, policyMemberAccountIDs: number[], policyID?: string) {
	return (
	isConciergeChatReport(report) || (report.policyID === CONST.POLICY.ID_FAKE || !report.policyID ? hasParticipantInArray(report, policyMemberAccountIDs) : report.policyID === policyID)
	);
	}

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

In the doesReportBelongToWorkspace function, we can exempt particular reports, such as expense reports, from checking the hasParticipantInArray if report.policyID === policyID and the current user is in report.participantAccountIDs.

What alternative solutions did you explore? (Optional)

We also could check if the current user is in the array provided by the getParticipantsIDs function.

[tienifr]

Proposal

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

In Step 7 and 8, LHN does not display the IOU report and IOU details page when these pages are open

What is the root cause of that problem?

Currently in here, we have the logic to always show the report that's being opened.

However recently the workspace filter feature is added, when we filter by workspae, we have this additional filter that will filter out anything that doesn't belong to the workspace.

In that condition we don't have the condition to always include the report that's being opened, like we did in here. So if the currently opened report doesn't belong to the workspace, it will not show in LHN.

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

Add the logic to always include the report that's being opened to the workspace filtering logic here as well.

Or we can refactor a bit to move the workspace filtering logic here to inside shouldReportBeInOptionList (which already has the always-include-the-report-that's-being-opened logic)

What alternative solutions did you explore? (Optional)

If we split bill inside a workspace room, do not set the policyId of the report to the personal workspace, but set it to the workspace where the split bill is created.

Or another approach is in doesReportBelongToWorkspace, check if the split bill IOU/request was linked to a parent report action in a workspace room of the filtered workspace, if yes, it belongs to the workspace.

[mollfpr]

However, the policy members are an empty array because no other member is invited.

@Tony-MK I don't understand this. Could you elaborate a bit more?

---

Add the logic to always include the report that's being opened to the workspace filtering logic here as well.

This looks good, but I think we will have unexpected results. For example, while on the workspace switcher, we change the reportID with a chat that came from the other user not in the workspace. This will show that report from a user that is not in the workspace.

[tienifr]

This will show that report from a user that is not in the workspace.

@mollfpr I think that's expected because that's the report being opened so it should be visible in LHN. When the user navigates away it will disappear from the filter.

This is the same way it worked before in the app, even if the report is marked as hidden but it's opened by the user, it will still show in LHN.

[mollfpr]

@tienifr I believe that will tag as a regression.

[tienifr]

@tienifr I believe that will tag as a regression.

@mollfpr sorry can you clarify what you mean by this?

Thanks

[mollfpr]

I think that's expected because that's the report being opened so it should be visible in LHN. When the user navigates away it will disappear from the filter.

App/src/libs/ReportUtils.ts

Lines 3791 to 3796 in 58bdee3

	// Include the currently viewed report. If we excluded the currently viewed report, then there
	// would be no way to highlight it in the options list and it would be confusing to users because they lose
	// a sense of context.
	if (report.reportID === currentReportId) {
	return true;
	}

The comment it's reasonable though. So I think it's make sense with your proposal @tienifr, but I need second though on this to make sure that's it's need sees as a regression.

[melvin-bot]

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

[JmillsExpensify]

Did you want to bring this to open-source in Slack to discuss more?

[mollfpr]

Reposting the discussion to the #expensify-open-source. I didn't get any eye on the c+ channel 🥲

[melvin-bot]

@JmillsExpensify @mollfpr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@JmillsExpensify, @mollfpr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[mollfpr]

@JmillsExpensify Could you chime into the thread? Thank you!

[melvin-bot]

📣 @mollfpr 🎉 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]

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[tienifr]

PR ready for review #37589.

[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.47-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Current report does not show in LHN under workspace filter #37589

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

• @mollfpr requires payment automatic offer (Reviewer)
• @tienifr 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:

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

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#33280
#33302

The issue is from the ideal nav wave where we implementing the policy switcher.

[@mollfpr] 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/33302/files#r1524888218

[@mollfpr] 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:

The issue is from the ideal nav wave, so I don't think it's worth adding a checklist for now.

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

I think we can update this test case https://expensify.testrail.io/index.php?/suites/view/7722&group_by=cases:section_id&group_order=asc&display_deleted_cases=0&group_id=296775 expected result.

• Viewed IOU report and details page or currently viewed report

[mollfpr]

@JmillsExpensify Could you create the payment summary? Thank you!

[JmillsExpensify]

Payment summary:

• Contributor: @tienifr $500
• Contributor+: @mollfpr $500

[JmillsExpensify]

@trjExpensify Same here. Can I get your help with a confirmed payment summary before I release payment?

[JmillsExpensify]

Released payment for Contributor in the meantime given that's a separate process.

[trjExpensify]

Payment summary here looks good!

[JmillsExpensify]

Thanks! $500 approved for @mollfpr based on summary.