Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make onboarding continue from last visited onboarding page #47472

Merged
merged 4 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,9 @@ const ONYXKEYS = {
/** Onboarding Purpose selected by the user during Onboarding flow */
ONBOARDING_ADMINS_CHAT_REPORT_ID: 'onboardingAdminsChatReportID',

// Stores onboarding last visited path
ONBOARDING_LAST_VISITED_PATH: 'onboardingLastVisitedPath',

// Max width supported for HTML <canvas> element
MAX_CANVAS_WIDTH: 'maxCanvasWidth',

Expand Down Expand Up @@ -862,6 +865,7 @@ type OnyxValuesMapping = {
[ONYXKEYS.ONBOARDING_ERROR_MESSAGE]: string;
[ONYXKEYS.ONBOARDING_POLICY_ID]: string;
[ONYXKEYS.ONBOARDING_ADMINS_CHAT_REPORT_ID]: string;
[ONYXKEYS.ONBOARDING_LAST_VISITED_PATH]: string;
[ONYXKEYS.IS_SEARCHING_FOR_REPORTS]: boolean;
[ONYXKEYS.LAST_VISITED_PATH]: string | undefined;
[ONYXKEYS.VERIFY_3DS_SUBSCRIPTION]: string;
Expand Down
3 changes: 1 addition & 2 deletions src/components/ExplanationModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import Navigation from '@libs/Navigation/Navigation';
import variables from '@styles/variables';
import * as Welcome from '@userActions/Welcome';
import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
import FeatureTrainingModal from './FeatureTrainingModal';

function ExplanationModal() {
Expand All @@ -18,7 +17,7 @@ function ExplanationModal() {
onNotCompleted: () => {
setTimeout(() => {
Navigation.isNavigationReady().then(() => {
Navigation.navigate(ROUTES.ONBOARDING_ROOT.route);
Welcome.startOnboardingFlow();
});
}, variables.welcomeVideoDelay);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import * as Session from '@libs/actions/Session';
import interceptAnonymousUser from '@libs/interceptAnonymousUser';
import linkingConfig from '@libs/Navigation/linkingConfig';
import getAdaptedStateFromPath from '@libs/Navigation/linkingConfig/getAdaptedStateFromPath';
import Navigation, {navigationRef} from '@libs/Navigation/Navigation';
import Navigation from '@libs/Navigation/Navigation';
import type {RootStackParamList, State} from '@libs/Navigation/types';
import {isCentralPaneName} from '@libs/NavigationUtils';
import * as PolicyUtils from '@libs/PolicyUtils';
Expand Down Expand Up @@ -66,10 +64,7 @@ function BottomTabBar({selectedTab}: BottomTabBarProps) {
}

Welcome.isOnboardingFlowCompleted({
onNotCompleted: () => {
const {adaptedState} = getAdaptedStateFromPath(ROUTES.ONBOARDING_ROOT.route, linkingConfig.config);
navigationRef.resetRoot(adaptedState);
},
onNotCompleted: () => Welcome.startOnboardingFlow(),
});

// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
Expand Down
9 changes: 7 additions & 2 deletions src/libs/Navigation/NavigationRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ import hasCompletedGuidedSetupFlowSelector from '@libs/hasCompletedGuidedSetupFl
import Log from '@libs/Log';
import {getPathFromURL} from '@libs/Url';
import {updateLastVisitedPath} from '@userActions/App';
import {updateOnboardingLastVisitedPath} from '@userActions/Welcome';
import * as Welcome from '@userActions/Welcome';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Route} from '@src/ROUTES';
import ROUTES from '@src/ROUTES';
import type {Route} from '@src/ROUTES';
import AppNavigator from './AppNavigator';
import getPolicyIDFromState from './getPolicyIDFromState';
import linkingConfig from './linkingConfig';
Expand Down Expand Up @@ -58,6 +60,9 @@ function parseAndLogRoute(state: NavigationState) {

if (focusedRoute && !CONST.EXCLUDE_FROM_LAST_VISITED_PATH.includes(focusedRoute?.name)) {
updateLastVisitedPath(currentPath);
if (currentPath.startsWith(`/${ROUTES.ONBOARDING_ROOT.route}`)) {
updateOnboardingLastVisitedPath(currentPath);
}
}

// Don't log the route transitions from OldDot because they contain authTokens
Expand Down Expand Up @@ -98,7 +103,7 @@ function NavigationRoot({authenticated, lastVisitedPath, initialUrl, onReady, sh
// If the user haven't completed the flow, we want to always redirect them to the onboarding flow.
// We also make sure that the user is authenticated.
if (!hasCompletedGuidedSetupFlow && authenticated && !shouldShowRequire2FAModal) {
const {adaptedState} = getAdaptedStateFromPath(ROUTES.ONBOARDING_ROOT.route, linkingConfig.config);
const {adaptedState} = getAdaptedStateFromPath(Welcome.getOnboardingInitialPath(), linkingConfig.config);
return adaptedState;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type GetAdaptedStateReturnType = {
metainfo: Metainfo;
};

type GetAdaptedStateFromPath = (...args: Parameters<typeof getStateFromPath>) => GetAdaptedStateReturnType;
type GetAdaptedStateFromPath = (...args: [...Parameters<typeof getStateFromPath>, shouldReplacePathInNestedState?: boolean]) => GetAdaptedStateReturnType;

// The function getPathFromState that we are using in some places isn't working correctly without defined index.
const getRoutesWithIndex = (routes: NavigationPartialRoute[]): PartialState<NavigationState> => ({routes, index: routes.length - 1});
Expand Down Expand Up @@ -365,7 +365,7 @@ function getAdaptedState(state: PartialState<NavigationState<RootStackParamList>
};
}

const getAdaptedStateFromPath: GetAdaptedStateFromPath = (path, options) => {
const getAdaptedStateFromPath: GetAdaptedStateFromPath = (path, options, shouldReplacePathInNestedState = true) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am adding an optional parameter here, I really need it, more info here.

const normalizedPath = !path.startsWith('/') ? `/${path}` : path;
const pathWithoutPolicyID = getPathWithoutPolicyID(normalizedPath);
const isAnonymous = isAnonymousUser();
Expand All @@ -374,7 +374,9 @@ const getAdaptedStateFromPath: GetAdaptedStateFromPath = (path, options) => {
const policyID = isAnonymous ? undefined : extractPolicyIDFromPath(path);

const state = getStateFromPath(pathWithoutPolicyID, options) as PartialState<NavigationState<RootStackParamList>>;
replacePathInNestedState(state, path);
if (shouldReplacePathInNestedState) {
replacePathInNestedState(state, path);
}
if (state === undefined) {
throw new Error('Unable to parse path');
}
Expand Down
4 changes: 3 additions & 1 deletion src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2700,7 +2700,9 @@ function openReportFromDeepLink(url: string) {

// We need skip deeplinking if the user hasn't completed the guided setup flow.
if (!hasCompletedGuidedSetupFlow) {
Welcome.isOnboardingFlowCompleted({onNotCompleted: () => Navigation.navigate(ROUTES.ONBOARDING_ROOT.getRoute())});
Welcome.isOnboardingFlowCompleted({
onNotCompleted: () => Welcome.startOnboardingFlow(),
});
return;
}

Expand Down
50 changes: 47 additions & 3 deletions src/libs/actions/Welcome.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,24 @@ import type {OnyxUpdate} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import * as API from '@libs/API';
import {WRITE_COMMANDS} from '@libs/API/types';
import Navigation from '@libs/Navigation/Navigation';
import linkingConfig from '@libs/Navigation/linkingConfig';
import Navigation, {navigationRef} from '@libs/Navigation/Navigation';
import getStateFromPath from '@navigation/getStateFromPath';
import getAdaptedStateFromPath from '@navigation/linkingConfig/getAdaptedStateFromPath';
import variables from '@styles/variables';
import type {OnboardingPurposeType} from '@src/CONST';
import NAVIGATORS from '@src/NAVIGATORS';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {Route} from '@src/ROUTES';
import type Onboarding from '@src/types/onyx/Onboarding';
import type TryNewDot from '@src/types/onyx/TryNewDot';

type OnboardingData = Onboarding | [] | undefined;

let isLoadingReportData = true;
let tryNewDotData: TryNewDot | undefined;
let onboardingInitialPath = '';
let onboarding: OnboardingData;

type HasCompletedOnboardingFlowProps = {
Expand Down Expand Up @@ -46,15 +52,18 @@ function onServerDataReady(): Promise<void> {
return isServerDataReadyPromise;
}

let isOnboardingInProgress = false;
function isOnboardingFlowCompleted({onCompleted, onNotCompleted}: HasCompletedOnboardingFlowProps) {
isOnboardingFlowStatusKnownPromise.then(() => {
if (Array.isArray(onboarding) || onboarding?.hasCompletedGuidedSetupFlow === undefined) {
return;
}

if (onboarding?.hasCompletedGuidedSetupFlow) {
isOnboardingInProgress = false;
onCompleted?.();
} else {
} else if (!isOnboardingInProgress) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevents multiple onboarding flow running causing navigation issue.

isOnboardingInProgress = true;
onNotCompleted?.();
}
});
Expand Down Expand Up @@ -97,7 +106,7 @@ function handleHybridAppOnboarding() {
isOnboardingFlowCompleted({
onNotCompleted: () =>
setTimeout(() => {
Navigation.navigate(ROUTES.ONBOARDING_ROOT.route);
startOnboardingFlow();
}, variables.explanationModalDelay),
}),
});
Expand Down Expand Up @@ -152,6 +161,19 @@ function setOnboardingPolicyID(policyID?: string) {
Onyx.set(ONYXKEYS.ONBOARDING_POLICY_ID, policyID ?? null);
}

function updateOnboardingLastVisitedPath(path: string) {
Onyx.merge(ONYXKEYS.ONBOARDING_LAST_VISITED_PATH, path);
}

function getOnboardingInitialPath(): Route {
const state = getStateFromPath(onboardingInitialPath as Route);
if (state?.routes?.at(-1)?.name !== NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR) {
return ROUTES.ONBOARDING_ROOT.route as Route;
}

return onboardingInitialPath as Route;
}

function completeHybridAppOnboarding() {
const optimisticData: OnyxUpdate[] = [
{
Expand Down Expand Up @@ -180,6 +202,11 @@ function completeHybridAppOnboarding() {
API.write(WRITE_COMMANDS.COMPLETE_HYBRID_APP_ONBOARDING, {}, {optimisticData, failureData});
}

function startOnboardingFlow() {
const {adaptedState} = getAdaptedStateFromPath(getOnboardingInitialPath(), linkingConfig.config, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really need to add an optional parameter to getAdaptedStateFromPath because it is causing this bug:

The path is being appended to the URL inserted by the user:

Incorrect URL path:
incorrect-path-d.mp4

navigationRef.resetRoot(adaptedState);
}

Onyx.connect({
key: ONYXKEYS.NVP_ONBOARDING,
callback: (value) => {
Expand All @@ -188,6 +215,18 @@ Onyx.connect({
},
});

const onboardingLastVisitedPathConnection = Onyx.connect({
key: ONYXKEYS.ONBOARDING_LAST_VISITED_PATH,
callback: (value) => {
if (value === undefined) {
return;
}

onboardingInitialPath = value.substring(1);
Onyx.disconnect(onboardingLastVisitedPathConnection);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to disconnect here?

Copy link
Contributor Author

@tsa321 tsa321 Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we only need the initial value when the user opens the app or URL. I have tested it, the onboarding is triggered from bottomTabBar and openReportFromDeepLink, leading to navigation changes that affect the lastVisitedPath and will open incorrect page.

},
});

Onyx.connect({
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
initWithStoredValues: false,
Expand All @@ -213,16 +252,21 @@ function resetAllChecks() {
resolveOnboardingFlowStatus = resolve;
});
isLoadingReportData = true;
onboardingInitialPath = '';
isOnboardingInProgress = false;
}

export {
onServerDataReady,
isOnboardingFlowCompleted,
setOnboardingPurposeSelected,
getOnboardingInitialPath,
updateOnboardingLastVisitedPath,
resetAllChecks,
setOnboardingAdminsChatReportID,
setOnboardingPolicyID,
completeHybridAppOnboarding,
handleHybridAppOnboarding,
setOnboardingErrorMessage,
startOnboardingFlow,
};
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ function BaseOnboardingPersonalDetails({
Welcome.setOnboardingAdminsChatReportID();
Welcome.setOnboardingPolicyID();

Navigation.dismissModal();
// Navigate to HOME instead of dismissModal, because there is bug in small screen
// where the onboarding puropose page will be disaplayed briefly
Navigation.navigate(ROUTES.HOME);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am changing dismissModal to navigate HOME because there is a bug:

Onboarding purpose disaplyed briefly:
home-instead-of-dismiss-d.mp4


// Only navigate to concierge chat when central pane is visible
// Otherwise stay on the chats screen.
Expand Down
Loading