-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix transitions to existing workspaces from OldDot #10949
Conversation
This reverts commit 46c7c43.
@@ -670,7 +670,6 @@ const CONST = { | |||
FREE: 'free', | |||
PERSONAL: 'personal', | |||
CORPORATE: 'corporate', | |||
TEAM: 'team', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chiragsalian Undoing your revert removed this policy type. Do we actually want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POLICY.TYPE.TEAM
isn't on main, so if you pull main I think this will go away. We don't use it anywhere.
Transitions seem to be working fine with the changes however there's another subtle issue where a new workspace created from OldDot will briefly appear as "not found" while the policy is loading Screen.Recording.2022-09-13.at.12.16.46.PM.movEDIT: Since we're navigating to the @neil-marcellini thoughts on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give a more thorough review later on.
src/pages/LogOutPreviousUserPage.js
Outdated
} else { | ||
const exitTo = lodashGet(this.props, 'route.params.exitTo'); | ||
Navigation.dismissModal(); | ||
Navigation.navigate(exitTo || ROUTES.HOME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is creating the problem with the "not found" page. We should only navigate to the workspace after it was created from here
App/src/libs/actions/Policy.js
Line 942 in d312075
Navigation.navigate(ROUTES.getWorkspaceInitialRoute(policyID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we have 2 cases where we're either transitioning to create a new workspace or transitioning to open an existing workspace. This navigation would be used for an existing workspace (as well as any other exitTo
destination we want to use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Navigation to the exit to route when not logging in a new user is handled here, which might not have been triggered previously with the componentShouldUpdate
implementation.
Lines 183 to 187 in d312075
if (!isLoggingInAsNewUser && exitTo) { | |
// We must call dismissModal() to remove the /transition route from history | |
Navigation.dismissModal(); | |
Navigation.navigate(exitTo); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay thanks it looks like this is running but not working because the NavigationContainer isn't ready in time. We might need to add a way to queue navigation for when the container is ready.
However, this transition logic being split up between AuthScreens calling App.setUpPoliciesAndNavigate()
and LogOutPreviousUserPage is very confusing. What do you think about consolidating it in LogOutPreviousUserPage and then renaming it to something like TransitionUserPage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay thanks it looks like this is running but not working because the NavigationContainer isn't ready in time. We might need to add a way to queue navigation for when the container is ready.
Good discovery that the NavigationContainer isn't ready, I remember that being a problem before. I would undo this commit where you removed that callback to wait until the navigation is ready e891e71.
However, this transition logic being split up between AuthScreens calling
App.setUpPoliciesAndNavigate()
and LogOutPreviousUserPage is very confusing. What do you think about consolidating it in LogOutPreviousUserPage and then renaming it to something like TransitionUserPage?
Fun fact, all of the logic for transitions used to be in one component called LoginWithShortLivedTokenPage
. We separated them into different pages and one action in this PR #8855 and it makes it a lot easier to reason about, because you know roughly what state you are in and each piece of the code only has one concern. I'm open to other ideas about how to simplify it of course. These transitions are a messy beast 👹 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll draft what a combination of App.setUpPoliciesAndNavigate()
and LogOutPreviousUserPage
could look like. I think it'll be a bit easier to follow and maintain for our authenticated transitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels like
App.setUpPoliciesAndNavigate()
is basically a bigif
statement and theelse
statement is just nested down inLogOutPreviousUserPage
for some reason.
Kind of, but only when we need to log out a previous user. Sometimes we just log in the transitioning user. I like App.setUpPoliciesAndNavigate()
because it has once specific focus.
It's especially awkward because
AuthScreens
andLogOutPreviousUserPage
are mounted at the same time even when we don't want to log the user out.
LoginWithShortLivedTokenPage
is also mounted even when the user is already signed in, but for both cases there's no problem with the page mounting and doing nothing. It will be unmounted when we navigate to the exit route.
I'm quite happy with the current structure of this flow, so if we are going to change it I really want to understand why it's needed. I think there was some talk of making this whole flow into an API command which could be a great way to clean it up. cc @marcaaron because I think you brought this up at one point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full disclosure I skimmed this thread (sorry I am pre-coffee) but does sound like we could be moving backwards and should maybe create a summary of what the problem is and go from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this issue has the most context from what I can tell. Maybe one of you can post a summary / plan there and we can get some more thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since fixing these transitions is fairly high priority, I'll update this PR to do that with our current approach and then I'll make a follow up GH to investigate maybe improving the way we handle transitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good except for navigating on the LogOutPreviousUserPage. That page should only have one function, which is logging out the previous user and showing a loading spinner in the meantime. Try removing that and see if there's still a problem with the "not found" workspace page.
@@ -670,7 +670,6 @@ const CONST = { | |||
FREE: 'free', | |||
PERSONAL: 'personal', | |||
CORPORATE: 'corporate', | |||
TEAM: 'team', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POLICY.TYPE.TEAM
isn't on main, so if you pull main I think this will go away. We don't use it anywhere.
src/pages/LogOutPreviousUserPage.js
Outdated
} else { | ||
const exitTo = lodashGet(this.props, 'route.params.exitTo'); | ||
Navigation.dismissModal(); | ||
Navigation.navigate(exitTo || ROUTES.HOME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Navigation to the exit to route when not logging in a new user is handled here, which might not have been triggered previously with the componentShouldUpdate
implementation.
Lines 183 to 187 in d312075
if (!isLoggingInAsNewUser && exitTo) { | |
// We must call dismissModal() to remove the /transition route from history | |
Navigation.dismissModal(); | |
Navigation.navigate(exitTo); | |
} |
This reverts commit 8f4ec05.
I added in changes to re-introduce our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool, from my testing it looks like this fixes the issue at least when logged out of NewDot. Please update your testing steps to test when the user is logged out of NewDot and when logged into a different account. Also, please add tests steps for this same flow from the OldDot mobile app, because that's where #10271 was reported.
After you fix the merge conflicts I'll test again and assuming it all works this will be good to go from me.
Code looks good to me! I can test once the test steps are ready to go (it looks like you were literally just editing them so I thought I would wait). |
Yup just fixed the merge conflict and updated the testing steps and all looks good on my end though I can't get OldDot running at the moment. |
Tested on web, this worked great! For the last set of tests (signing in as a different user), I did have to validate my email prior to opening NewDot or else I was greeted with the sign in screen, but I think that's probably expected behavior? Not sure how to test on OldDot Mobile and have it direct to my local vm, but I'm happy to do so if there are instructions and it would be useful! |
After step 8 add a step: Otherwise you don't see the created workspace. We could fix this on OldDot if we want... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good ✅
One thing I noticed is that the workspace naming is a bit off, for example when transitioning with "Get started" as a new user, NewDot created a workspace name "Expensifail's Workspace 2" when it should have been "Expensifail's Workspace". Would you be willing to create a separate issue and PR for that @arosiclair?
Oh also, the mobile transition tests can only be tested on staging/prod so they should be QA steps instead. Sorry, I forgot about that. |
I modified the QA steps / tests for you based on my last comment. Here's the checklist, then I'll merge!
|
@neil-marcellini looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
All checks passed, that's a bug. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Thanks!
Yeah I've noticed this too. I think we're still incrementing the number even after policies that were deleted. Created an issue here. |
🚀 Deployed to staging by @neil-marcellini in version: 1.2.5-0 🚀
|
🚀 Deployed to production by @luacmartins in version: 1.2.5-2 🚀
|
Details
Reinstates improved functionality from this PR for creating a workspace from OldDot and ensures the
NavigationContainer
is ready when doing OldDot to NewDot transitionsFixed Issues
$ https://github.com/Expensify/Expensify/issues/227553
$ #10271
Tests
From Web
Creating a workspace while signed out
Opening an existing workspace
Creating a workspace while signed in
Creating a workspace while signed into another account
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Same as Tests but from Web and OldDot Mobile
Screenshots
Web
Screen.Recording.2022-09-21.at.5.47.07.PM.mov
Mobile Web
Desktop
iOS
Android