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

Wait for Onyx to clear before signing in the transitioning user #8793

Closed
wants to merge 8 commits into from
3 changes: 3 additions & 0 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ export default {
// Boolean flag set when workspace is being created
IS_CREATING_WORKSPACE: 'isCreatingWorkspace',

// Boolean flag set when Onyx finishes clearing
IS_ONYX_DONE_CLEARING: 'isOnyxDoneClearing',

// Note: These are Persisted Requests - not all requests in the main queue as the key name might lead one to believe
PERSISTED_REQUESTS: 'networkRequestQueue',

Expand Down
5 changes: 5 additions & 0 deletions src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,15 @@ function triggerUpdateAvailable() {
Onyx.set(ONYXKEYS.UPDATE_AVAILABLE, true);
}

function setOnyxDoneClearing() {
Onyx.set(ONYXKEYS.IS_ONYX_DONE_CLEARING, true);
}

export {
setCurrentURL,
setLocale,
setSidebarLoaded,
getLocale,
triggerUpdateAvailable,
setOnyxDoneClearing,
};
1 change: 1 addition & 0 deletions src/libs/actions/SignInRedirect.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ function clearStorageAndRedirect(errorMessage) {

// `Onyx.clear` reinitialize the Onyx instance with initial values so use `Onyx.merge` instead of `Onyx.set`.
Onyx.merge(ONYXKEYS.SESSION, {error: errorMessage});
Onyx.set(ONYXKEYS.IS_ONYX_DONE_CLEARING, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Use App.setOnyxDoneClearing()

});
}

Expand Down
19 changes: 11 additions & 8 deletions src/libs/actions/WelcomeActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as Policy from './Policy';
import ONYXKEYS from '../../ONYXKEYS';
import NameValuePair from './NameValuePair';
import CONST from '../../CONST';
import Log from '../Log';

/* Flag for new users used to show welcome actions on first load */
let isFirstTimeNewExpensifyUser = false;
Expand Down Expand Up @@ -58,21 +59,23 @@ function show({routes, hideCreateMenu}) {
// Set the NVP back to false so we don't automatically run welcome actions again
NameValuePair.set(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, false, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER);

// We want to display the Workspace chat first since that means a user is already in a Workspace and doesn't need to create another one
const workspaceChatReport = _.find(allReports, report => ReportUtils.isPolicyExpenseChat(report));
if (workspaceChatReport) {
Navigation.navigate(ROUTES.getReportRoute(workspaceChatReport.reportID));
return;
}

// If we are rendering the SidebarScreen at the same time as a workspace route that means we've already created a workspace via workspace/new and should not open the global
// create menu right now.
const topRouteName = lodashGet(_.last(routes), 'name', '');
const loginWithShortLivedTokenRoute = _.find(routes, route => route.name === 'LogInWithShortLivedToken');
const exitingToWorkspaceRoute = lodashGet(loginWithShortLivedTokenRoute, 'params.exitTo', '') === 'workspace/new';
const isDisplayingWorkspaceRoute = topRouteName.toLowerCase().includes('workspace') || exitingToWorkspaceRoute;

// It's also possible that we already have a workspace policy. In either case we will not hide the menu but do still want to set the NVP in this case
// We want to display the Workspace chat first since that means a user is already in a Workspace and doesn't need to create another one
// Only navigate to the workspace chat if we are not displaying a workspace route
Comment on lines +69 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly confused by some of this logic so gonna rubber duck for a second...

  • When we are logging in for the first time we check to see if either the exitTo or url is sending us to a "workspace" page
  • We also check the list of reports to see if we already have a workspace.
  • If we are intentionally navigating to a specific workspace then we do nothing otherwise we navigate to the first workspace we find.

Does that all sound correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, I guess my next question would be...

Is this related to waiting for Onyx to clear? Or is it related to a different issue where we were incorrectly redirecting a user based on whether they have any workspace instead of checking if we should let the routing do it's thing?

I guess overall I'm just confused at the interplay between the LoginWithShortLivedTokenPage seemingly responsible for some navigation stuff and then the SidebarScreen is also responsible for navigation here:

componentDidMount() {
Performance.markStart(CONST.TIMING.SIDEBAR_LOADED);
Timing.start(CONST.TIMING.SIDEBAR_LOADED, true);
const routes = lodashGet(this.props.navigation.getState(), 'routes', []);
WelcomeAction.show({routes, hideCreateMenu: this.hideCreateMenu});
}

Kinda feels a bit all over the place. Gonna cc @luacmartins here too to get some thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly am I looking at?

Kinda feels a bit all over the place. Gonna cc @luacmartins here too to get some thoughts.

I agree that we should keep all this navigation in one place if possible.

const workspaceChatReport = _.find(allReports, report => ReportUtils.isPolicyExpenseChat(report));
if (workspaceChatReport && !isDisplayingWorkspaceRoute) {
Log.info('[WelcomeActions] Navigating to the workspace chat report');
Navigation.navigate(ROUTES.getReportRoute(workspaceChatReport.reportID));
return;
}

// It's also possible that we already have a workspace policy. In either case we will not toggle the menu but do still want to set the NVP in this case
// since the user does not need to create a workspace.
if (!Policy.isAdminOfFreePolicy(allPolicies) && !isDisplayingWorkspaceRoute) {
hideCreateMenu();
Expand Down
77 changes: 57 additions & 20 deletions src/pages/LogInWithShortLivedTokenPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import Navigation from '../libs/Navigation/Navigation';
import Log from '../libs/Log';

const propTypes = {
/** Boolean flag set when Onyx finishes clearing */
isOnyxDoneClearing: PropTypes.bool,

/** The parameters needed to authenticate with a short lived token are in the URL */
route: PropTypes.shape({
/** The name of the route */
Expand Down Expand Up @@ -42,47 +45,61 @@ const propTypes = {
};

const defaultProps = {
isOnyxDoneClearing: false,
route: {
params: {},
},
session: {},
};

class LogInWithShortLivedTokenPage extends Component {
componentDidMount() {
const email = lodashGet(this.props.route.params, 'email', '');
const shortLivedToken = lodashGet(this.props.route.params, 'shortLivedToken', '');
constructor(props) {
super(props);
this.state = {
isSigningIn: false,
};
}

const isUserSignedIn = this.props.session && this.props.session.authToken;
if (!isUserSignedIn) {
Log.info('[LoginWithShortLivedTokenPage] User not signed in - signing in with short lived token');
Session.signInWithShortLivedToken(email, shortLivedToken);
componentDidMount() {
// We need to wait for Onyx to finish clearing before signing in because
// it will re-initialize with the initialKeyStates, which can notify
// subscribers with an out of date value causing the user to be signed
// out again. See https://github.com/Expensify/react-native-onyx/issues/125
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: This comment should be removed as it provides a lot of specific context that doesn't quite make sense unless you have a lot of context on your specific view of what's happening here and the issue.

Separately, we are linking to a bug report which implies we should be fixing an issue in Onyx. Left a comment on that issue and think we should give it more thought.

suggestion: If we want to say anything here about why we are waiting for Onyx to finish clearing we could say something like:

If we are signing an old user out then we'll want to wait until they are fully signed out and Onyx has reinitialized as a blank slate before signing in the next user.

if (!this.props.isOnyxDoneClearing) {
return;
}

if (this.signOutIfNeeded(email)) {
if (this.signInIfNeeded()) {
return;
}

Log.info('[LoginWithShortLivedTokenPage] User is signed in');

// exitTo is URI encoded because it could contain a variable number of slashes (i.e. "workspace/new" vs "workspace/<ID>/card")
const exitTo = decodeURIComponent(lodashGet(this.props.route.params, 'exitTo', ''));
if (exitTo === ROUTES.WORKSPACE_NEW) {
// New workspace creation is handled in AuthScreens, not in its own screen
Log.info('[LoginWithShortLivedTokenPage] exitTo is workspace/new - handling new workspace creation in AuthScreens');
if (this.signOutIfNeeded()) {
return;
}

Log.info('[LoginWithShortLivedTokenPage] User is signed in');
this.navigateToExitRoute();
}

componentDidUpdate() {
if (!this.props.isOnyxDoneClearing) {
return;
}
if (this.state.isSigningIn) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that we only have this because we are setting state which triggers the component to update?

return;
}
if (this.signInIfNeeded()) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: having this logic in componentDidUpdate() makes it hard to connect the dots on which scenarios we are dealing with.

e.g. this.props.isOnyxDoneClearing is covering a very specific situation which is:

previous user was signed in so we signed them out and we are now waiting for previous user to fully sign out so we can sign in the new user

I'm curious if we can rework this so that it's clear which situation we are handling each time this component updates or mounts. Or at least document which checks are relevant for the different flows?

this.navigateToExitRoute();
}

navigateToExitRoute() {
// exitTo is URI encoded because it could contain a variable number of slashes (i.e. "workspace/new" vs "workspace/<ID>/card")
const exitTo = decodeURIComponent(lodashGet(this.props.route.params, 'exitTo', ''));
if (exitTo === ROUTES.WORKSPACE_NEW) {
// New workspace creation is handled in AuthScreens, not in its own screen
Log.info('[LoginWithShortLivedTokenPage] exitTo is workspace/new - handling new workspace creation in AuthScreens');
return;
}

// In order to navigate to a modal, we first have to dismiss the current modal. Without dismissing the current modal, if the user cancels out of the workspace modal,
// then they will be routed back to /transition/<accountID>/<email>/<authToken>/workspace/<policyID>/card and we don't want that. We want them to go back to `/`
Expand All @@ -92,16 +109,33 @@ class LogInWithShortLivedTokenPage extends Component {
Navigation.navigate(exitTo);
}

/**
* Sign the user in if needed.
* @returns {Boolean} isSigningIn Whether we are currently signing in
*/
signInIfNeeded() {
const email = lodashGet(this.props.route.params, 'email', '');
const shortLivedToken = lodashGet(this.props.route.params, 'shortLivedToken', '');
const isUserSignedIn = this.props.session && this.props.session.authToken;
if (isUserSignedIn) {
return false;
}
Log.info('[LoginWithShortLivedTokenPage] User not signed in - signing in with short lived token');
this.setState({isSigningIn: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Why do we need to set this to state?

I guess my thinking here is that if we are calling signInIfNeeded then there are two scenarios...

  1. User was not signed in and needs to sign in
  2. User was signed in then signed out and now needs to sign in

Regardless, doesn't this component entirely mount again in both of these cases?

User gets signed in > Component mounts again in AuthScreens > this.state.isSigningIn would be false again because the component remounted?

Session.signInWithShortLivedToken(email, shortLivedToken);
return true;
}

/**
* If the user is trying to transition with a different account than the one
* they are currently signed in as we will sign them out, clear Onyx,
* and cancel all network requests. This component will mount again from
* PublicScreens and since they are no longer signed in, a request will be
* made to sign them in with their new account.
* @param {String} email The user's email passed as a route param.
* @returns {Boolean}
* @returns {Boolean} isSigningOut Whether we are currently signing out
*/
signOutIfNeeded(email) {
signOutIfNeeded() {
const email = lodashGet(this.props.route.params, 'email', '');
if (this.props.session && this.props.session.email === email) {
return false;
}
Expand All @@ -120,6 +154,9 @@ LogInWithShortLivedTokenPage.propTypes = propTypes;
LogInWithShortLivedTokenPage.defaultProps = defaultProps;

export default withOnyx({
isOnyxDoneClearing: {
key: ONYXKEYS.IS_ONYX_DONE_CLEARING,
},
session: {
key: ONYXKEYS.SESSION,
},
Expand Down
5 changes: 5 additions & 0 deletions src/setup/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ONYXKEYS from '../ONYXKEYS';
import CONST from '../CONST';
import platformSetup from './platformSetup';
import * as Metrics from '../libs/Metrics';
import * as App from '../libs/actions/App';

export default function () {
/*
Expand Down Expand Up @@ -36,6 +37,10 @@ export default function () {
},
});

// We need to set IS_ONYX_DONE_CLEARING outside of the initial key states, because
// initial key states are set within Onyx.clear.
App.setOnyxDoneClearing();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try explaining what is happening a different way? I've read that comment a few times, but not sure I get why we need to set the IS_ONYX_DONE_CLEARING key in this setup method instead of when Onyx.clear() is called.

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 will try an approach where the initial key state is [ONYXKEYS.IS_ONYX_DONE_CLEARING]: true. I don't remember why I didn't do that to begin with, maybe I will find out.


// Force app layout to work left to right because our design does not currently support devices using this mode
I18nManager.allowRTL(false);
I18nManager.forceRTL(false);
Expand Down