Wait for Onyx to clear before signing in the transitioning user #8793
Conversation
melvin-bot
bot
requested review from
sketchydroide
and removed request for
a team
|
I just pushed one commit that I accidentally left out due to a merge conflict that I didn't notice. Ready for review. |
|
Have a few thoughts on this one, but they are better to share 1:1 |
|
Ready for review again. I have this other PR up to address the betas. Don't wait for betas while transitioning |
|
Fixed merge conflicts after "Don't wait for betas" was merged. |
|
I just tested all of the flows again and they are working. |
| @@ -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(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
suggestion: Use App.setOnyxDoneClearing()
| } | ||
| if (this.signInIfNeeded()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
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?
| if (!this.props.isOnyxDoneClearing) { | ||
| return; | ||
| } | ||
| if (this.state.isSigningIn) { |
There was a problem hiding this comment.
Is it possible that we only have this because we are setting state which triggers the component to update?
| return false; | ||
| } | ||
| Log.info('[LoginWithShortLivedTokenPage] User not signed in - signing in with short lived token'); | ||
| this.setState({isSigningIn: true}); |
There was a problem hiding this comment.
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...
- User was not signed in and needs to sign in
- 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?
| // 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
App/src/pages/home/sidebar/SidebarScreen.js
Lines 56 to 62 in 31e49ec
Kinda feels a bit all over the place. Gonna cc @luacmartins here too to get some thoughts.
There was a problem hiding this comment.
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.
| // 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 |
There was a problem hiding this comment.
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.
|
I admit that this implementation is confusing and doesn't read well. I will go through and address each of your comments as best I can, but before doing so, should I implement another approach? Should I create public and authenticated versions of this component as you mentioned yesterday? Or should I extract all of this logic into an action? I would prefer extracting it into an action. I can try to rework this to make it more readable, but I think the time would be better spent on a different implementation. |
It's hard for me to say if extracting things into an action would be preferable, because I'm not sure what the proposal is exactly and can't envision it. I do think it would help to think about the different scenarios a bit and maybe it will help lead us to a better solution. I suggested having two separate pages to handle this stuff because it feels like we are mixing concerns. If you imagine Possible navigation scenarios after we are signed in A. Navigate to an exitTo If we are in
If we are in
And when we are in I think that covers everything, but lemme know if I missed something. |
|
That's a great breakdown of the different scenarios that happen. If possible it would be great to eliminate B by having a separate component for workspace/new that would handle this code block from the authScreens. App/src/libs/Navigation/AppNavigator/AuthScreens.js Lines 134 to 137 in 4eadfbb I'm not really sure what the solution is for the WelcomeAction logic. It was a small tweak that I had to do and it has nothing to do with waiting for Onyx to clear. I will post a proposal for implementing this as an action. I think it will be more straight forward, especially in regards to waiting for Onyx to clear. If you think it shows promise we can go ahead with that, otherwise I can split it into two different pages. |
Oh hmm that's interesting. I was actually thinking that we could have an if/else in // pseudocode
if (exitTo === ROUTES.WORKSPACE_NEW) {
createPolicyAndNavigate();
} else {
Navigation.navigate(exitTo)
}That way the only thing |
ProposalSimplify LoginWithShortLivedTokenPage so that it only renders a loading spinner and calls one action on componentDidMount(): componentDidMount() {
const accountID = lodashGet(this.props.route.params, 'accountID', '');
const email = lodashGet(this.props.route.params, 'email', '');
const shortLivedToken = lodashGet(this.props.route.params, 'shortLivedToken', '');
const exitTo = decodeURIComponent(lodashGet(this.props.route.params, 'exitTo', ''));
Session.transitionUser(accountID, email, shortLivedToken, exitTo);
}At the top of the Session file connect to the Onyx session and store flag to mark when the user is transitioning. Create an action that will wait until the proper user is signed in and then navigate them to the exit route function transitionUser(accountID, email, shortLivedToken, exitTo) {
if (isUserTransitioning) {
return;
}
return new Promise(resolve => {
const isUserSignedIn = session && session.authToken;
if (isUserSignedIn) {
resolve();
} else if (session.email !== email) {
Log.info('[transitionUser] Different user signed in - signing out');
resolve(signInDifferentUser(email, shortLivedToken))
} else {
Log.info('[transitionUser] User not signed in - signing in with short lived token');
resolve(signInWithShortLivedToken(accountID, email, shortLivedToken))
}
})
.then(() => {
isUserTransitioning = false
Log.info('[transitionUser] Dismissing LoginWithShortLivedTokenPage and navigating to exitTo');
Navigation.dismissModal();
Navigation.navigate(exitTo);
});
}
Does this look like a mess or is it somewhat promising? |
The auth screens can mount when we are logged in as the wrong user, so wouldn't we have to handle basically the same cases? |
Not sure I understand the question, mind rephrasing? The proposal doesn't look too bad, but there is still a lot of responsibility for one method and I think we are really just dealing with 3 different states (sign in, sign out if needed, perform side effect based on route when logged in). But I'm probably missing something. I am assuming here that the responsibilities would go something like this:
The Anything that happens after that would depend on the I think the only problem with this is that AuthScreens doesn't know if we are an old user signing out, but it's easy enough to check that based on the route params. |
Yeah I think you are right that it is doing too much for one thing. As I was writing that proposal I started to see how nice it would be to have it split into two separate pages. I will work on a solution in the next few days that uses two separate pages and handles the navigation in the AuthScreens.
That's sort of what I was getting at with my question, but I suppose it's not too many cases since we know that we are signed in. Thanks for helping me think through this. |
|
@marcaaron Based our discussion above, here is what I'm planning to implement in another branch. Please let me know if this sounds good or if you have suggestions.
class LogOutOldUserPage extends Component {
componentDidMount() {
const email = lodashGet(this.props.route.params, 'email', '');
if (this.props.session && this.props.session.email !== email) {
Log.info('[LogOutOldUserPage] Different user signed in - signing out');
Session.signOutAndRedirectToSignIn();
}
}
render() {
return <FullScreenLoadingIndicator />;
}
}
class LogInWithShortLivedTokenPage extends Component {
componentDidMount() {
if (!this.props.isOnyxDoneClearing) {
return;
}
this.signInTransitioningUser();
}
componentDidUpdate() {
if (!this.props.isOnyxDoneClearing) {
return;
}
this.signInTransitioningUser();
}
signInTransitioningUser() {
const accountID = lodashGet(this.props.route.params, 'accountID', '');
const email = lodashGet(this.props.route.params, 'email', '');
const shortLivedToken = lodashGet(this.props.route.params, 'shortLivedToken', '');
Log.info('[LoginWithShortLivedTokenPage] signing in the transitioning user');
Session.signInWithShortLivedToken(accountID, email, shortLivedToken);
}
render() {
return <FullScreenLoadingIndicator />;
}
}
Linking.getInitialURL()
.then((url) => {
if (this.shouldCreateFreePolicy(url)) {
Policy.createAndGetPolicyList();
return;
} else {
Policy.getPolicyList();
// 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', ''));
// 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 `/`
// and by calling dismissModal(), the /transition/... route is removed from history so the user will get taken to `/` if they cancel out of the new workspace modal.
Log.info('[AuthScreens] Dismissing LogOutOldUserPage and navigating to the transition exit route');
Navigation.dismissModal();
Navigation.navigate(exitTo);
}
}); |
NAB, but I think it can just stay in
👍
👍
I still question whether we should use
That works for now though it would be better if the logic in the |
|
Ok cool, I will start working on that.
How would you wait for Onyx to finish clearing? My best idea would be to store a promise in the session file. let afterOnyxClears = new Promise(resolve => resolve());Then I would make function signOutAndRedirectToSignIn() {
signOut();
afterOnxyClears = redirectToSignIn();
Log.info('Redirecting to Sign In because signOut() was called');
}Then in signInWithShortLivedToken I would wait for onyx to finish clearing function signInWithShortLivedToken(email, shortLivedToken) {
Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});
afterOnyxClears.then(() => {
afterOnyxClears = new Promise(resolve => resolve());
createTemporaryLogin(shortLivedToken, email).then((response) => {
if (response.jsonCode === 200) {
User.getUserDetails();
Onyx.merge(ONYXKEYS.ACCOUNT, {success: true});
} else {
const error = lodashGet(response, 'message', 'Unable to login.');
Onyx.merge(ONYXKEYS.ACCOUNT, {error});
}
}).finally(() => {
Onyx.merge(ONYXKEYS.ACCOUNT, {loading: false});
});
});
}I could probably refactor that to make it cleaner, but does that roughly work and make sense? I'm not super skilled with promise magic. How would you do it? |
There's this componentDidMount() {
waitForOnyxToClear().then(() => signInWithShortLivedToken()))
}That said, I think your proposal is fine without this - and I think anytime we have to use |
Details
As an alternative to modifying how Onyx updates subscribers when initializing the default key states, wait for Onyx to finish clearing and initializing before signing in the transitioning user.
Related Issue
$ #8676
Tests
Below are tests for all of the transition flows from Web-Expensify to NewDot. This PR specifically fixes flow C, but we should test all of them.
@gmail.comaddressE. Pricing select free
E.1 Creating a free policy
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android