[HOLD for payment 2024-07-10] [$250] Login - Onboarding can be skipped if you kill the app after the modal opens

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

issue found when validating #43648
Version Number: 1.4.84-0
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: applause internal team
Slack conversation:

Action Performed:

1. Open the app
2. Log in with a new Gmail account
3. Kill the app after the onboarding modal opens
4. Open the app

Expected Result:

The modal should be open when I open the app again.

Actual Result:

Onboarding can be skipped if you kill the app after the modal opens.

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

Bug6514254_1718456579175.New.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a398eb407ba14887
• Upwork Job ID: 1802712363358554394
• Last Price Increase: 2024-06-17
• Automatic offers:
• bernhardoj | Contributor | 102794675

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot]

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

[m-natarajan]

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

[bernhardoj]

Proposal

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

Onboarding can be skipped if we reopen the app or I also found we can skip it if we go to /home on the web.

What is the root cause of that problem?

The onboarding modal will be shown if the onboarding data is ready. However, we set initWithStoredValues to false which means the data won't be initialized with the value from the onyx storage, thus the checkOnboardingDataReady is never called.

App/src/libs/actions/Welcome.ts

Lines 79 to 82 in 75c104b

	Onyx.connect({
	key: ONYXKEYS.NVP_ONBOARDING,
	initWithStoredValues: false,
	callback: (value) => {

which makes this promise never resolved.

App/src/libs/actions/Welcome.ts

Lines 31 to 32 in 75c104b

	function isOnboardingFlowCompleted({onCompleted, onNotCompleted}: HasCompletedOnboardingFlowProps) {
	isOnboardingFlowStatusKnownPromise.then(() => {

On the web, if we go to /home, the issue still happens.

If you see the image above, the app already navigates correctly to the onboarding, but somehow it navigates back to the home (/r).

It's because, when we open a link, it will trigger this "deep link".

App/src/Expensify.tsx

Lines 194 to 197 in 75c104b

	Linking.getInitialURL().then((url) => {
	setInitialUrl(url);
	Report.openReportFromDeepLink(url ?? '');
	});

In our case, the link is home. openReportFromDeepLink will open the link after we are navigated to the onboarding.

open home -> navigate to onboarding -> navigate to home

This doesn't happen in native because we pass the authenticated status and ignore it if we are authenticated because openReportFromDeepLink is made to handle a deep link (to an auth-protected screen) that is made while logged out, so the user is navigated to the screen after successfully signing in.

App/src/Expensify.tsx

Lines 200 to 203 in 75c104b

	Linking.addEventListener('url', (state) => {
	// We need to pass 'isAuthenticated' to avoid loading a non-existing profile page twice
	Report.openReportFromDeepLink(state.url, !isAuthenticatedRef.current);
	});

App/src/libs/actions/Report.ts

Line 2488 in 75c104b

function openReportFromDeepLink(url: string, shouldNavigate = true) {

App/src/libs/actions/Report.ts

Lines 2532 to 2534 in 75c104b

	if (!shouldNavigate) {
	return;
	}

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

To solve the first issue, we need to remove the initWithStoredValues

App/src/libs/actions/Welcome.ts

Lines 79 to 81 in 75c104b

	Onyx.connect({
	key: ONYXKEYS.NVP_ONBOARDING,
	initWithStoredValues: false,

For the second issue, we already have the authenticated status in openReportFromDeepLink, so we don't need to pass it anymore.

App/src/libs/actions/Report.ts

Line 2490 in 75c104b

const isAuthenticated = Session.hasAuthToken();

Then, replace this with isAuthenticated.

App/src/libs/actions/Report.ts

Line 2532 in 75c104b

if (!shouldNavigate) {

if (isAuthenticated) {
    return
}

[tsa321]

I had a proposal to change the checking condition to show onboarding in other issue:

The complete rough code is something like:

Complete rough code:

let resolveIsReadyPromise: (value?: Promise<void>) => void | undefined;
let isServerDataReadyPromise = new Promise<void>((resolve) => {
    resolveIsReadyPromise = resolve;
});

let resolveOnboardingFlowStatus: (value?: Promise<void>) => void | undefined;
let isOnboardingFlowStatusKnownPromise = new Promise<void>((resolve) => {
    resolveOnboardingFlowStatus = resolve;
});

let resolveRouteIsReady: (value?: Promise<void>) => void | undefined;
let isRouteReadyPromise = new Promise<void>((resolve) => {
    resolveRouteIsReady = resolve;
});

let resolveUserIsLoggedIn: (value?: Promise<void>) => void | undefined;
let isUserLoggedInPromise = new Promise<void>((resolve) => {
    resolveUserIsLoggedIn = resolve;
});

function onServerDataReady(): Promise<void> {
    return isServerDataReadyPromise;
}

const listenToSessionChange = function() {
  const sessionConeectionId = Onyx.connect({
      key: ONYXKEYS.SESSION,
      initWithStoredValues: false,
      callback: (session) => {
          if (session?.accountID === undefined || session?.authTokenType === CONST.AUTH_TOKEN_TYPES.ANONYMOUS) {
              return;
          }
          resolveUserIsLoggedIn?.();
          Onyx.disconnect(sessionConeectionId);
      },
  });
}

const listenToPathChange = function() {
  const connectionId = Onyx.connect({
      key: ONYXKEYS.LAST_VISITED_PATH,
      initWithStoredValues: true,
      callback: (cc) => {
          const rootRoute = navigationRef.getRootState()?.routes;
          const currentRouteName = rootRoute?.[rootRoute.length - 1]?.name;
          if (Boolean(currentRouteName && currentRouteName !== NAVIGATORS.BOTTOM_TAB_NAVIGATOR && currentRouteName !== NAVIGATORS.CENTRAL_PANE_NAVIGATOR)) {
            return;
          }
  
          resolveRouteIsReady();
          Onyx.disconnect(lastVistedConnectID);
      },
  });
}

const listenToNvpOnboardingChange = function() {
  const connectionId = Onyx.connect({
      key: ONYXKEYS.NVP_ONBOARDING,
      initWithStoredValues: true,
      callback: (value) => {
          if (value === null || value === undefined) {
              return;
          }
          onboarding = value;
          resolveOnboardingFlowStatus();
          Onyx.disconnect(connectionId);
      },
  });
}

function startOnBoarding(){
  listenToSessionChange();
  isUserLoggedInPromise
  .then(() => isServerDataReadyPromise)
  .then(() => {
      listenToNvpOnboardingChange();
      return isOnboardingFlowStatusKnownPromise;
  }).then(() => {
      listenToPathChange();
      return isRouteReadyPromise;
  }).then(() => {
      if (Array.isArray(onboarding) || onboarding?.hasCompletedGuidedSetupFlow === undefined) {
          return;
      }
  
      if (onboarding?.hasCompletedGuidedSetupFlow) {
          onCompleted?.();
      } else {
          Navigation.navigate(ROUTES.ONBOARDING_ROOT)
          //onNotCompleted?.();
      }
      return;
  });
}
startOnBoarding();

It checks on several onyx values, session (check whether user sign in) -> then check on isDataRedy -> nvp_onboarding check -> last visited path check. It consist of several promises and resolves which will automatically disconnect resolved onyx connection and make new connection to subsequent onyx check.

[melvin-bot]

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

[melvin-bot]

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

[Ollyws]

@bernhardoj's proposal LGTM.
🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-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 📖

[bernhardoj]

PR is ready

cc: @Ollyws

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

• Fix onboarding can be skipped if you kill the app after the modal opens #44062

If no regressions arise, payment will be issued on 2024-07-10. 🎊

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

• @Ollyws requires payment through NewDot Manual Requests
• @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:

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

[isabelastisser]

@Ollyws, please complete the checklist above. Thanks!

[bernhardoj]

Just FYI, I will request the payment through ND.

[bernhardoj]

Requested in ND

[isabelastisser]

Payment summary:

@Ollyws $250 C+ review / $250 pending via NewDot
@bernhardoj contributor / $250 pending via NewDot

[isabelastisser]

@Ollyws please complete the BZ list above. Thanks!

[JmillsExpensify]

$250 approved for @bernhardoj

[isabelastisser]

@Ollyws I DM'd you for visibility.

[Ollyws]

Apologies still trying to track down the source of this one

[Ollyws]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR:

#39687

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

#39687 (comment)

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

N/A

• Determine if we should create a regression test for this bug.

Yes

• 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.

Regression Test Proposal

1. Sign in as a new user or a user that hasn't completed onboarding yet
2. Verify the onboarding modal shows
3. (Android/iOS) Close the app and reopen (Web/mWeb/Desktop) Go to /home
4. Verify the onboarding modal shows

Do we agree 👍 or 👎

[Ollyws]

Requested in ND.

[isabelastisser]

@cristipaval, can you please review the regression test above? Thanks!

[cristipaval]

@cristipaval, can you please review the regression test above? Thanks!

yes they are fine

[isabelastisser]

All set!

[JmillsExpensify]

$250 approved for @Ollyws