[$500] Web - User can still access attachment thread after left the thread

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the app on chrome, reply on someone by sending an attachment and click on "reply on thread" on the attachment
2. Reply by sending messages
3. Open the [Attachment] message and click on "Leave thread".
4. "Hm... it's not here You don't have access to this chat" message will be shown but then you'll be redirected to the attachment you have left and supposed to have no access anymore (based on what the previous description showed).

Expected Result:

It should be showing a skeleton after user left the tread

Actual Result:

User can still access attachment thread after left the thread

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.67-2
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
Notes/Photos/Videos: Any additional supporting documentation

2023-09-11.22-20-43.mp4

Recording.4455.mp4

Expensify/Expensify Issue URL:
Issue reported by: @niatania102
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694446243712749

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0137f77b386e8c8eb4
• Upwork Job ID: 1702274114805768192
• Last Price Increase: 2023-09-21

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0137f77b386e8c8eb4

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to @garrettmknight (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[sumairq]

Proposed solution

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

User can still access attachment thread after they leave the the thread.

What is the root cause of that problem?**

In ReportScreenWe don't get rid of reportID even after leaving the thread due to which <FullPageNotFoundView> component does not replace the child components with not found view as it depends on 'repordID' returning a falsy value.

We see the You don't have access to this chat only temporarily because the report object is temporarily empty and unpopulated and as soon as it is available the chat shows again because the reportID is still there.

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

• • Make sure that the Report.leaveRoom() action is properly disposing off the reportID in report from state when a person leaves a thread with attachment.

What alternative solutions did you explore? (Optional)

none

[huzaifa-99]

Proposal

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

I believe that a user shouldn't be prevented from accessing a thread, even if they leave it. A thread is a part of a chat, and if user has access to a chat they should be able to access any thread in that chat.

Based on this assumption, I think the problem to solve is that the Hm... it's not here You don't have access to this chat message shouldn't be displayed (even momentarily) when visiting a thread that a user just left.

What is the root cause of that problem?

We clear the report object when a user leaves a thread and when the user visits the same thread report again, the empty report object causes the ReportScreen to show the 'Hmm.. its not there' message momentarily until the API call for OpenReport finishes.

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

In ReportScreen here, update the isLoading state to also handle empty report object cases by adding _.isEmpty(report), so it would become something like this

// pseudocode
const isLoading = !reportID || _.isEmpty(report) || !isSidebarLoaded || _.isEmpty(personalDetails) || firstRenderRef.current;

When the OpenReport call finishes this report object will be populated and will show the messages, during this load time the skeleton UI will be shown.

What alternative solutions did you explore? (Optional)

N/A

[bernhardoj]

Proposal

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

The written issue is, that the user can access the same thread after leaving it, but in my opinion, that is expected. I agree with @huzaifa-99 that the real issue is, that rejoining the same thread shouldn't show the not found page but should show skeletons.

What is the root cause of that problem?

When we leave a thread, we clear the report data from onyx.

App/src/libs/actions/Report.js

Lines 1794 to 1812 in 63a5776

	optimisticData: [
	{
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
	value: {
	stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
	statusNum: CONST.REPORT.STATUS.CLOSED,
	},
	},
	],
	// Manually clear the report using merge. Should not use set here since it would cause race condition
	// if it was called right after a merge.
	successData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
	value: _.object(reportKeys, Array(reportKeys.length).fill(null)),
	},
	],

In ReportScreen we show the not found page if reportID does not exist, isLoadingReportActions is false, and isLoading is false.

App/src/pages/home/ReportScreen.js

Lines 303 to 304 in 62b8e71

	<FullPageNotFoundView
	shouldShow={(!report.reportID && !report.isLoadingReportActions && !isLoading) || shouldHideReport}

When we rejoin the same thread, reportID is obviously empty but isLoadingReportActions should be true. The problem is, that we don't clear the report actions when leaving a room, so when we do an openReport request, we won't set the isLoadingReportActions property to true which is useful to "bypass" the not found page and show the skeletons.

App/src/libs/actions/Report.js

Lines 407 to 413 in 63a5776

	value: reportActionsExist(reportID)
	? {}
	: {
	isLoadingReportActions: true,
	isLoadingMoreReportActions: false,
	reportName: lodashGet(allReports, [reportID, 'reportName'], CONST.REPORT.DEFAULT_REPORT_NAME),
	},

App/src/pages/home/ReportScreen.js

Line 365 in 62b8e71

{(!isReportReadyForDisplay || isLoadingInitialReportActions || isLoading) && <ReportActionsSkeletonView containerHeight={skeletonViewContainerHeight} />}

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

Clear the report actions optimistically when leaving a thread and revert it when the request fails.

const reportActions = allReportActions[reportID];
optimisticData: [..., {
    onyxMethod: Onyx.METHOD.SET,
    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
    value: null,
}]
failureData: [..., {
    onyxMethod: Onyx.METHOD.SET,
    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
    value: reportActions,
}]

so when joining the thread again, we will reload the report actions too, and thus show the skeletons.

note: this solution currently doesn't work because we have an onyx issue where setting null doesn't clear the data.

[adelekennedy]

@eVoloshchak please review the above

[adelekennedy]

@eVoloshchak review on the above please

[eVoloshchak]

Is this bug still present?
I can't reproduce using the steps from the description, it already works exactly as it should, showing the skeleton while loading

Screen.Recording.2023-09-21.at.14.46.22.mov

[bernhardoj]

It is fixed in #26602

[melvin-bot]

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

[eVoloshchak]

@adelekennedy, could you close this one, please? It was resolved by another PR

[adelekennedy]

I think just the reporting bonus is due in that case

[adelekennedy]

asked in Slack