[HOLD for Payment 2023-12-21] [$500] App is calling OpenApp instead of ReconnectApp when reloading the page

[iwiznia]

Context https://expensify.slack.com/archives/C03SDMF9YJ2/p1701887907195709 (private)

OpenApp should only be called when we are first initializing the app (after login basically) and ReconnectApp in any other case, but right now, we always call OpenApp on every reload, which is wrong.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b01257e6ae73d2aa
• Upwork Job ID: 1732509709419290624
• Last Price Increase: 2023-12-06
• Automatic offers:
• fedirjh | Reviewer | 28015537
• paultsimura | Contributor | 28015538

[melvin-bot]

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

[melvin-bot]

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

[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 Contributor-plus team member for initial proposal review - @fedirjh (External)

[zukilover]

Proposal

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

App is calling openApp instead of reconnectApp on reload

What is the root cause of that problem?

On AuthScreen, app is thinking that it is transitioning from login as a new user while it shouldn't.

App/src/libs/SessionUtils.ts

Lines 25 to 26 in deda6ae

	return linkedEmail !== sessionEmail;
	}

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

On transition, email params is in the URL address bar, we can simply check if it doesn't exist, than it should not be treated as a transition.

Change this line here:

App/src/libs/SessionUtils.ts

Lines 16 to 18 in deda6ae

	if (paramsEmail === sessionEmail) {
	return false;
	}

to:

if (!paramsEmail || paramsEmail === sessionEmail) {
        return false;
    }

What alternative solutions did you explore? (Optional)

N/A

[paultsimura]

Proposal

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

OpenApp is called on every page refresh.

What is the root cause of that problem?

Disclaimer: There will be a lot of text, but please try to keep up with my thoughts as I tried a simpler solution, but it looks like a workaround, and while investigating, I found a deeper issue with the session handling. The RCA is long, the solution will be simple.

This particular issue comes from #29013, where the author had an intention to fix the case when there was an attempt to log in to different accounts during one session:

1. Try login as a suspended user
2. Go back
3. Log in with a real user

For some reason, it was decided to use the SessionUtils.isLoggingInAsNewUser, which isn't supposed to be here, as its purpose is to handle the transition URLs, which contain email in the URL params.

We already were checking the SessionUtils.didUserLogInDuringSession, which should do the job for that issue, but the didUserLogInDuringSession was (and is) working incorrectly.

Now to the actual investigation of the didUserLogInDuringSession logic:

This function is intended to indicate whether a user was logged in during the session – meaning there was no page reload after the login, or the user did not open the app being already logged in.

It relies on the Onyx SESSION updates, and this is the root cause of the incorrect behavior:

App/src/libs/SessionUtils.ts

Lines 30 to 45 in 5d45f0f

	// To tell if the user logged in during this session we will check the value of session.authToken once when the app's JS inits. When the user logs out
	// we can reset this flag so that it can be updated again.
	Onyx.connect({
	key: ONYXKEYS.SESSION,
	callback: (session) => {
	if (loggedInDuringSession) {
	return;
	}
	if (session?.authToken) {
	loggedInDuringSession = false;
	} else {
	loggedInDuringSession = true;
	}
	},
	});

However, when logging in as a suspended user, and clicking "back", we explicitly call redirectToSignIn, which resets Onyx data and the loggedInDuringSession value:

App/src/pages/signin/EmailDeliveryFailurePage.js

Line 78 in d521dd6

onPress={() => redirectToSignIn()}

App/src/libs/actions/SignInRedirect.ts

Lines 76 to 84 in 5d45f0f

	function redirectToSignIn(errorMessage?: string) {
	MainQueue.clear();
	HttpUtils.cancelPendingRequests();
	PersistedRequests.clear();
	NetworkConnection.clearReconnectionCallbacks();
	clearStorageAndRedirect(errorMessage);
	resetHomeRouteParams();
	SessionUtils.resetDidUserLogInDuringSession();
	}

And now we are redirected back to the SignIn page, relying on the Onyx SESSION update to set the loggedInDuringSession again to true, which will never happen because the Onyx SESSION does not change: it already was in the default state when signing in as a suspended user, so calling clearStorageAndRedirect did not make any change to the SESSION key, therefore the Onyx.connect in SessionUtils was not triggered and the didUserLogInDuringSession remains undefined after reset, which makes the SessionUtils.didUserLogInDuringSession() an unreliable source of truth. This all can be verified by adding corresponding logs inside the Onyx.connect in SessionUtils to track setting and resetting the value of loggedInDuringSession.

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

First, we should change the logic of the reset function to set the value to true instead of undefined. This is a safe change since resetDidUserLogInDuringSession is called only on redirectToSignIn, therefore we expect that when a user lands on the SignIn page, the sign-in process will be initiated, and we'll make use of loggedInDuringSession = true after the successful sign in.

function resetDidUserLogInDuringSession() {
    loggedInDuringSession = true;
}

App/src/libs/SessionUtils.ts

Lines 47 to 49 in 5d45f0f

	function resetDidUserLogInDuringSession() {
	loggedInDuringSession = undefined;
	}

Second, remove the change introduced in #30730 , since now this logic will be handled by the fixed SessionUtils.didUserLogInDuringSession():

const shouldGetAllData = !!isUsingMemoryOnlyKeys || SessionUtils.didUserLogInDuringSession();

App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

Line 150 in d180ba9

const shouldGetAllData = !!isUsingMemoryOnlyKeys || SessionUtils.didUserLogInDuringSession() || isLoggingInAsNewUser;

This does both:

Fixes current issue:

Screen.Recording.2023-12-07.at.11.57.21-compressed.mp4

Keeps https://github.com//issues/29013 fixed:

Screen.Recording.2023-12-07.at.12.06.13-compressed.mp4

What alternative solutions did you explore? (Optional)

[fedirjh]

@paultsimura Thanks for the deeper investigation, Your proposal makes sense to me, this is a regression from #30730. Your solution looks good to me as well.

---

@zukilover Thanks for the proposal, but it seems your solution is just a patch to the original issue.

---

let's proceed with @paultsimura's proposal.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[chiragsalian]

I think the proposal sounds good to me too.
Feel free to create the PR @paultsimura

[melvin-bot]

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

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

[paultsimura]

Thank you. The PR is ready for review: #32775

[paultsimura]

PR was deployed to production on Dec 14, the automation didn't work 🤔
cc: @sonialiap

[sonialiap]

Thanks for the ping, Pavlo! I'll update the issue title manually

[fedirjh]

BugZero Checklist:

• Link to the PR: fix: added new condition when we need to refetch data #30730
• Link to comment: fix: added new condition when we need to refetch data #30730 (comment)
• Link to discussion: @chiragsalian Should we start a discussion for this bug ? I think a regression test is highly recommended in similar cases.
• Determine if we should create a regression test for this bug: Yes

---

Regression Test Proposal

1. On Chrome Web , Open the App
2. Login to your account
3. Open Chrome devtools an select the networks tab :
4. Refresh the page
5. Verify that the ReconnectApp API request was executed :

• Do we agree 👍 or 👎

[sonialiap]

@paultsimura $500 - paid ✔️
@fedirjh $500 - paid ✔️