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

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

Here, we are checking whether to call the OpenApp by shouldGetAllData. And when we reload the page, the isLoggingInAsNewUser is true because here it's called not from a transitioning URL, so it doesn't contain the email param (which the isLoggingInAsNewUser function expects).

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

Lines 149 to 150 in d180ba9

	const isLoggingInAsNewUser = !!session?.email && SessionUtils.isLoggingInAsNewUser(currentUrl, session.email);
	const shouldGetAllData = !!isUsingMemoryOnlyKeys || SessionUtils.didUserLogInDuringSession() || isLoggingInAsNewUser;

This condition was added here, and the author meant:

Check if current email is different that this saving into session

However, if no email is about to be saved into the session, we should return false.

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

We should modify SessionUtils.isLoggingInAsNewUser to return false if eventually no email was found by matching the URL:

- return linkedEmail !== sessionEmail;
+ return !!linkedEmail && linkedEmail !== sessionEmail;

App/src/libs/SessionUtils.ts

Lines 4 to 26 in 5d45f0f

	/**
	* Determine if the transitioning user is logging in as a new user.
	*/
	function isLoggingInAsNewUser(transitionURL: string, sessionEmail: string): boolean {
	// The OldDot mobile app does not URL encode the parameters, but OldDot web
	// does. We don't want to deploy OldDot mobile again, so as a work around we
	// compare the session email to both the decoded and raw email from the transition link.
	const params = new URLSearchParams(transitionURL);
	const paramsEmail = params.get('email');
	// If the email param matches what is stored in the session then we are
	// definitely not logging in as a new user
	if (paramsEmail === sessionEmail) {
	return false;
	}
	// If they do not match it might be due to encoding, so check the raw value
	// Capture the un-encoded text in the email param
	const emailParamRegex = /[?&]email=([^&]*)/g;
	const matches = emailParamRegex.exec(transitionURL);
	const linkedEmail = matches?.[1] ?? null;
	return linkedEmail !== sessionEmail;
	}

Result:

Screen.Recording.2023-12-07.at.09.47.49.mov

What alternative solutions did you explore? (Optional)

[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