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

Redirect user to concierge when onboarding completed #42087

Merged
merged 14 commits into from
May 30, 2024
2 changes: 1 addition & 1 deletion src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ type OnyxValuesMapping = {
[ONYXKEYS.ACCOUNT]: OnyxTypes.Account;
[ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID]: string;
[ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER]: boolean;
[ONYXKEYS.NVP_ONBOARDING]: Onboarding | [];
[ONYXKEYS.NVP_ONBOARDING]: Onboarding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we change the type? for old accounts the nvp is an empty array so I think we need to keep this

[ONYXKEYS.ACTIVE_CLIENTS]: string[];
[ONYXKEYS.DEVICE_ID]: string;
[ONYXKEYS.IS_SIDEBAR_LOADED]: boolean;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
import {createStackNavigator} from '@react-navigation/stack';
import React from 'react';
import React, {useEffect} from 'react';
import {View} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import NoDropZone from '@components/DragAndDrop/NoDropZone';
import useOnboardingLayout from '@hooks/useOnboardingLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import OnboardingModalNavigatorScreenOptions from '@libs/Navigation/AppNavigator/OnboardingModalNavigatorScreenOptions';
import Navigation from '@libs/Navigation/Navigation';
import type {OnboardingModalNavigatorParamList} from '@libs/Navigation/types';
import OnboardingPersonalDetails from '@pages/OnboardingPersonalDetails';
import OnboardingPurpose from '@pages/OnboardingPurpose';
import OnboardingWork from '@pages/OnboardingWork';
import * as Report from '@userActions/Report';
import ONYXKEYS from '@src/ONYXKEYS';
import SCREENS from '@src/SCREENS';
import Overlay from './Overlay';

Expand All @@ -17,6 +21,22 @@ const Stack = createStackNavigator<OnboardingModalNavigatorParamList>();
function OnboardingModalNavigator() {
const styles = useThemeStyles();
const {shouldUseNarrowLayout} = useOnboardingLayout();
const [hasCompletedGuidedSetupFlow] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {selector: (onboarding) => onboarding?.hasCompletedGuidedSetupFlow ?? true});
mountiny marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (!hasCompletedGuidedSetupFlow) {
return;
}

// Need to go back to previous route and then redirect to Concierge,
// otherwise going back on concierge will go to onboarding and then redirected to concierge again
Navigation.goBack();
Report.navigateToConciergeChat();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Report.navigateToConciergeChat();

I think just going back without redirecting to Concierge is better. Let me know if there are any edge cases in which redirecting to Concierge is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suppose you are in a report and clicked https://dev.new.expensify.com:8082/onboarding link in a message. In this case, if there's only goBack, it will redirect user to onboarding first and then will go back to the current report. So need to redirect user to Concierge again.

What if there's only one Concierge redirect? If so, user will be redirect to concierge on deep link click, but if he clicks go back in the browser, it will redirect user to onbarding and then onboarding will redirect user back to concierge, so user can not go back... so these two actions are required and it will remove onboarding from route history...

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Navigation.goBack();
Report.navigateToConciergeChat();
Navigation.isNavigationReady().then(() => {
Navigation.goBack();
Report.navigateToConciergeChat();
}
navigatorNotReady.mp4

I see the navigator is not ready when navigating to these routes. So, it goes back to onboarding again as seen in the video.

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 thought, this component is defined in Route, so navigation should be ready already...Pushed fix as you mentioned

}, [hasCompletedGuidedSetupFlow]);

if (hasCompletedGuidedSetupFlow) {
return null;
}

return (
<NoDropZone>
Expand Down
Loading