[hold for payment 2024-08-26] [$250] Submit expense - Not here page displayed in the background after a refresh

[lanitochka17]

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: 9.0.16
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Click on FAB
3. Click on start chat
4. Refresh the page
5. Observe everything works as normal
6. Click on FAB
7. Click on submit expense
8. Refresh the page

Expected Result:

Everything should work as normal just as it is on step 5

Actual Result:

Not here page gets displayed in the background when submit expense page is open on the right and user refreshes the page which is a different behavior that doesn't happen when other right hand 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

Bug6554859_1722100817716.4_5848162808948069362.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a60ec1cb3ed7a96b
• Upwork Job ID: 1820825832663945809
• Last Price Increase: 2024-08-06
• Automatic offers:
• nkdengineer | Contributor | 103466921

Issue Owner

Current Issue Owner: @sobitneupane

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (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.

[lanitochka17]

@joekaufmanexpensify 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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[tsa321]

Edited by proposal-police: This proposal was edited at 2024-08-05 06:59:14 UTC.

Proposal

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

A "not found" page is displayed in the background after refreshing the page or when creating an IOU request.

What is the root cause of that problem?

When a user clicks on the + Fab button to create an IOU request, the app generates an optimistic report and includes the ID in the route path of the create IOU request.

The report only contains the report name and an error field with a "not found" property.

When a user visits the app by entering a URL in the address bar or refreshing the page, the app adapts the root state by storing the correct navigation state/path to reach that address. This adaptation is handled by getAdaptedStateFromPath. For example, with a URL like /create/submit/start/1/1782162509946918/manual, getAdaptedStateFromPath tries to include the report screen in the navigation state due to the presence of the report ID in the URL, as seen in :

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 154 to 156 in 8fd61d6

	if (ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]) {
	return {name: SCREENS.REPORT, params: {reportID}};
	}

This results in the report screen being added to the navigation root state, even though it is not required for IOU request URLs to open the correct report in the background to start the IOU request step.

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

We should modify getMatchingRootRouteForRHPRoute to ensure that the report screen is only added to the root state if the URL path starts with /r/. This will prevent unnecessary addition of the report screen to navigation root state for URLs that do not require it.

We can adjust the condition in :

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 154 to 156 in 8fd61d6

	if (ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]) {
	return {name: SCREENS.REPORT, params: {reportID}};
	}

to be:

if (route?.path.indexOf('/r/') === 0 || route?.path.indexOf('r/') === 0) {
        const reportID = (route.params as Record < string, string | undefined > )?.reportID;
        if (ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]) {
            return {name: SCREENS.REPORT, params: {reportID}};
        }
    }

Additionally, if there are specific routes where the report screen must be included regardless of the URL path, we can add these routes to an exception list (currently I don't find any route).

[nkdengineer]

Proposal

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

Not here page gets displayed in the background when submit expense page is open on the right and user refreshes the page which is a different behavior that doesn't happen when other right hand pages are open

What is the root cause of that problem?

When we submit an expense from FAB, the reportID is random but we call OpenReport API here and then a invalid report is stored in Onyx.

App/src/pages/iou/request/step/withWritableReportOrNotFound.tsx

Line 69 in 2eb65f8

ReportActions.openReport(route.params.reportID);

Then the report screen with invalid reportID is added here when we open a RHP screen

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 154 to 156 in 2eb65f8

	if (ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]) {
	return {name: SCREENS.REPORT, params: {reportID}};
	}

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 187 in 2eb65f8

if (focusedRHPRoute) {

App/src/libs/Navigation/linkTo/index.ts

Lines 135 to 141 in 2eb65f8

	const {adaptedState, metainfo} = getAdaptedStateFromPath(path, linkingConfig.config);
	if (adaptedState && (metainfo.isCentralPaneAndBottomTabMandatory || metainfo.isFullScreenNavigatorMandatory)) {
	const diff = getPartialStateDiff(rootState, adaptedState as State<RootStackParamList>, metainfo);
	const diffActions = getActionsFromPartialDiff(diff);
	for (const diffAction of diffActions) {
	root.dispatch(diffAction);
	}

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

1. OPTIONAL: We should prevent calling the OpenReport API in the create flow

App/src/pages/iou/request/step/withWritableReportOrNotFound.tsx

Line 69 in 2eb65f8

ReportActions.openReport(route.params.reportID);

2. We should safely check whether the reportID exists or not here since some not found reports can be stored in Onyx

if (ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportID) {
    return {name: SCREENS.REPORT, params: {reportID}};
}

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 154 to 156 in 2eb65f8

	if (ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]) {
	return {name: SCREENS.REPORT, params: {reportID}};
	}

3. We can add the money request screen SCREENS.MONEY_REQUEST.CREATE here

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 24 in 2eb65f8

const RHP_SCREENS_OPENED_FROM_LHN = [SCREENS.SETTINGS.SHARE_CODE, SCREENS.SETTINGS.PROFILE.STATUS, SCREENS.SETTINGS.PREFERENCES.PRIORITY_MODE] satisfies Screen[];

What alternative solutions did you explore? (Optional)

[joekaufmanexpensify]

@tsa321 did you link this issue with the other one above because these have the same RCA?

[tsa321]

@joekaufmanexpensify this is simlar to #46713 and my proposal can fix it too, but RCA a little bit differerent

[melvin-bot]

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

[melvin-bot]

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

[joekaufmanexpensify]

@joekaufmanexpensify this is simlar to #46713 and my proposal can fix it too, but RCA a little bit different

Got it. If we can fix all of the various cases in one issue, that feels ideal IMO

[joekaufmanexpensify]

Pending proposal review

[joekaufmanexpensify]

@sobitneupane mind taking a look here?

[sobitneupane]

Thanks for the proposal @nkdengineer .

@nkdengineer Regarding the changes you have proposed,

OPTIONAL: We should prevent calling the OpenReport API in the create flow

It was added by this PR. Won't removing it cause regression?

The third change you proposed has already been implemented in the latest main branch.

[sobitneupane]

Thanks for your proposal @tsa321

We cannot implement the change you suggested. In some scenarios, when opening the app through a deep link, it's necessary to open the report in the background. For instance, if we're currently viewing a report at /r/abc and attempt to open a deep link like /create/submit/start/1/cde/manual, the report cde needs to be opened in the background.

[tsa321]

@sobitneupane I think it is not required. That route is similar when sumbit expense from FAB button. Which I think is not required to open the report.
If it is really required we can include it in exception list.

[nkdengineer]

It was added by #31529. Won't removing it cause regression?

@sobitneupane No I meant we will not call OpenReport API if we're in the create flow because in the create flow, the reportID is randomly and the edit flow also uses this hook

[sobitneupane]

Thanks for the update

Proposal from @nkdengineer looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[tsa321]

@sobitneupane If your last action, for example, is submitting expense report A, then opening report B and clicking QAB, report A does not open; it stays on report B. Based on this, it is not necessary to open the report with the reportID(cde) in /create/submit/start/1/cde/manual in background. Therefore, the assertion that report A must open in the background is incorrect.
It's not about whether we open the report in the background; it's about the navigation stack and state. We need to ensure that the generated state is correct and that subsequent navigation actions (such as going back or navigating) work as expected.
In mWeb/native navigation, there will be several related issues. When a user clicks FAB -> QAB and then uses the back button, it opens the report instead of returning to the LHN.

There are actually many issues related to `getAdaptedStateFromPath`. If you search through the issues, you'll find that my solution is more general and addresses the related problems.

[nkdengineer]

If your last action, for example, is submitting expense report A, then opening report B and clicking QAB, report A does not open; it stays on report B. Based on this, it is not necessary to open the report with the reportID(cde) in /create/submit/start/1/cde/manual in background. Therefore, the assertion that report A must open in the background is incorrect.

Just a note about this case: When we submit expenses to report A and then open report B, if we click on the QAB report B still appears. If we refresh the page, the report A is displayed as expected since we access the deep link has reportID as report A.

[melvin-bot]

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

[joekaufmanexpensify]

PR in review

[joekaufmanexpensify]

PR merged and on staging

[joekaufmanexpensify]

Hmm, automation did not work here. This went out to prod on the 19th, so is actually due today. Will tackle payment in the next day or so!

[joekaufmanexpensify]

Payments here are:

• $250 to @nkdengineer for PR via Upwork.
• $250 to @sobitneupane for C+ via NewDot.

[joekaufmanexpensify]

All set to issue payment!

[joekaufmanexpensify]

@nkdengineer $250 sent and contract ended

[joekaufmanexpensify]

Closing as only thing left here is for @sobitneupane to request payment via NewDot!

[garrettmknight]

$250 approved for @sobitneupane