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
• [x] 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

[melvin-bot[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). https://github.com/Expensify/App/blob/f1502291a14d4e15e04249b241109f146bdb512f/src/pages/home/ReportScreen.tsx#L434-L438

The second OpenReport is called when ReportActionsView is also mounted (openReportIfNecessary). https://github.com/Expensify/App/blob/f1502291a14d4e15e04249b241109f146bdb512f/src/pages/home/report/ReportActionsView.tsx#L238-L245

Both openReportIfNecessary and openReportIfNecessary won't call OpenReport if shouldFetchReport is false. https://github.com/Expensify/App/blob/f1502291a14d4e15e04249b241109f146bdb512f/src/pages/home/report/ReportActionsView.tsx#L114-L116

If shouldFetchReport is true, then the difference between openReportIfNecessary and openReportIfNecessary is, fetchReportIfNeeded won't call OpenReport if the report already exists https://github.com/Expensify/App/blob/f1502291a14d4e15e04249b241109f146bdb512f/src/pages/home/ReportScreen.tsx#L399-L404

but openReportIfNecessary will always call it. https://github.com/Expensify/App/blob/f1502291a14d4e15e04249b241109f146bdb512f/src/pages/home/report/ReportActionsView.tsx#L113-L119

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. https://github.com/Expensify/App/blob/90412ec0c38cc1ccd0a1198cf1f4712474667906/src/pages/home/ReportScreen.tsx#L421-L423

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[muttmuure]

Adding external

[melvin-bot[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) https://github.com/Expensify/App/blob/568921fbca73dd27bdb2b4135d75bbede60b3d18/src/pages/home/ReportScreen.tsx#L712-L715

[abdulrahuman5196]

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

https://github.com/Expensify/App/assets/46707890/6437cc22-fd4a-43b3-b4c7-5c1d0c29759e

[melvin-bot[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[bot]]

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

[quinthar]

What are the next steps? What's the ETA?

[muttmuure]

@abdulrahuman5196 could you take a look at the updated proposal?

[abdulrahuman5196]

Hi, The updated proposal seems reasonable. I will some quick testing in my morning and approve if no issues.

[abdulrahuman5196]

@bernhardoj 's proposal here https://github.com/Expensify/App/issues/39673#issuecomment-2041127917 looks good on high level and address the issue. IMO we should call OpenReport in only one place to have consistency. @bernhardoj I still saw some reports not making OpenReport API call with the mentioned changes but I think that could be fixed in the PR. We should test with different kind of reports as part of PR and make sure nothing breaks.

🎀 👀 🎀 C+ Reviewed

[melvin-bot[bot]]

Triggered auto assignment to @flodnv, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[flodnv]

The proposal sounds good to me but

@bernhardoj I still saw some reports not making OpenReport API call with the mentioned changes but I think that could be fixed in the PR. We should test with different kind of reports as part of PR and make sure nothing breaks.

Sounds weird and scary, so let's make sure to test this extensively please.

[melvin-bot[bot]]

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

📣 @bernhardoj 🎉 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 📖

[melvin-bot[bot]]

@flodnv @abdulrahuman5196 @muttmuure @bernhardoj 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!

[bernhardoj]

PR is ready

cc: @abdulrahuman5196

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.68-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/40624

If no regressions arise, payment will be issued on 2024-05-08. :confetti_ball:

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[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[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".