[HOLD for payment 2024-05-09] [HOLD for payment 2024-05-08] HIGH: [API Reliability] [$250] Multiple calls to OpenReport on signin

[m-natarajan]

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: 1.4.60-7
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: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712180869292309

Action Performed:

1. Sign in to NewDot

Expected Result:

Only one OpenReport is called since I open it only once

Actual Result:

It is called twice

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/~0138cc8ab53b98e8c0
• Upwork Job ID: 1777757590524878848
• Last Price Increase: 2024-04-16
• Automatic offers:
• abdulrahuman5196 | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @muttmuure

[melvin-bot]

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

[bernhardoj]

Proposal

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

There are 2 duplicate OpenReport request when opening a report for the first time, for example after sign in.

What is the root cause of that problem?

The first OpenReport is called when ReportScreen is mounted (fetchReportIfNeeded).

App/src/pages/home/ReportScreen.tsx

Lines 434 to 438 in f150229

	useEffect(() => {
	Timing.end(CONST.TIMING.CHAT_RENDER);
	Performance.markEnd(CONST.TIMING.CHAT_RENDER);
	fetchReportIfNeeded();

The second OpenReport is called when ReportActionsView is also mounted (openReportIfNecessary).

App/src/pages/home/report/ReportActionsView.tsx

Lines 238 to 245 in f150229

	useEffect(() => {
	if (reportActionID) {
	return;
	}
	const interactionTask = InteractionManager.runAfterInteractions(() => {
	openReportIfNecessary();
	});

Both openReportIfNecessary and openReportIfNecessary won't call OpenReport if shouldFetchReport is false.

App/src/pages/home/report/ReportActionsView.tsx

Lines 114 to 116 in f150229

	if (!shouldFetchReport(report)) {
	return;
	}

If shouldFetchReport is true, then the difference between openReportIfNecessary and openReportIfNecessary is, fetchReportIfNeeded won't call OpenReport if the report already exists

App/src/pages/home/ReportScreen.tsx

Lines 399 to 404 in f150229

	// It is possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that
	// is not stored locally yet. If report.reportID exists, then the report has been stored locally and nothing more needs to be done.
	// If it doesn't exist, then we fetch the report from the API.
	if (report.reportID && report.reportID === reportIDFromRoute && !reportMetadata?.isLoadingInitialReportActions) {
	return;
	}

but openReportIfNecessary will always call it.

App/src/pages/home/report/ReportActionsView.tsx

Lines 113 to 119 in f150229

	const openReportIfNecessary = () => {
	if (!shouldFetchReport(report)) {
	return;
	}
	Report.openReport(reportID, reportActionID);
	};

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

First, don't call openReportIfNecessary when ReportActionsView is mounted. Then, we need to decide whether we should always call OpenReport or not when ReportScreen is mounted even when we already have the report object. I would say yes so we always get the latest report data every time we visit a chat (and also because we did this in ReportActionsView, so the behavior is unchanged).

We can do that by removing this condition.

App/src/pages/home/ReportScreen.tsx

Lines 421 to 423 in 90412ec

	if (report.reportID && report.reportID === reportIDFromRoute && !reportMetadata?.isLoadingInitialReportActions) {
	return;
	}

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0138cc8ab53b98e8c0

[muttmuure]

Adding external

[melvin-bot]

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

[abdulrahuman5196]

Checking now

[abdulrahuman5196]

@bernhardoj I understand both ReportScreen.tsx and ReportActionsView.tsx component mount has OpenReport has in it.
But the thing i don't understand is, why is this happening only at signin?, since it doesn't happen when we open a chat after that.
I noticied, that OpenReport is called multiple times only when having a skeleton loading. What is the co-relation here?

I support removing the openReport call inside ReportActionsView, but wanted to be sure we won't cause any issue since its a super old code.

[bernhardoj]

But the thing i don't understand is, why is this happening only at signin?, since it doesn't happen when we open a chat after that.
I noticied, that OpenReport is called multiple times only when having a skeleton loading. What is the co-relation here?

I mentioned here that ReportScreen won't call OpenReport if the report object already exists. There is also a !reportMetadata?.isLoadingInitialReportActions condition which will be false when you open an existing report, but never load it yet. If you never load it yet, then the isLoadingInitialReportActions value will be true (default)

App/src/pages/home/ReportScreen.tsx

Lines 712 to 715 in 568921f

	reportMetadata: {
	key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_METADATA}${getReportID(route)}`,
	initialValue: {
	isLoadingInitialReportActions: true,

[abdulrahuman5196]

@bernhardoj If I disble ReportActionsView OpenReport API call, there isn't any OpenReport API call not made when opening a report.

Screen.Recording.2024-04-15.at.11.51.48.PM.mov

[melvin-bot]

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

[bernhardoj]

@abdulrahuman5196 yep, that's why I propose to remove the existing report condition from the ReportScreen

Then, we need to decide whether we should always call OpenReport or not when ReportScreen is mounted even when we already have the report object. I would say yes so we always get the latest report data every time we visit a chat (and also because we did this in ReportActionsView, so the behavior is unchanged).

I have updated my proposal to point out which code to remove

[melvin-bot]

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

[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:

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

[abdulrahuman5196]

@bernhardoj The PR has been reverted due to an issue. Kindly raise a another PR fixing the regression as well.

[bernhardoj]

@abdulrahuman5196 new PR is ready

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

• Fix multiple open report call when report screen is mounted #40624

If no regressions arise, payment will be issued on 2024-05-09. 🎊

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

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

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

[muttmuure]

Awaiting payment date

[melvin-bot]

@flodnv, @abdulrahuman5196, @muttmuure, @bernhardoj Eep! 4 days overdue now. Issues have feelings too...

[flodnv]

@muttmuure can we pay and close this?

[muttmuure]

Automation has failed, I will create a payment in Upwork tomorrow

[melvin-bot]

@flodnv, @abdulrahuman5196, @muttmuure, @bernhardoj Huh... This is 4 days overdue. Who can take care of this?

[muttmuure]

Paid