Fix transitions to existing workspaces from OldDot #10949

arosiclair · 2022-09-12T22:28:00Z

Details

Reinstates improved functionality from this PR for creating a workspace from OldDot and ensures the NavigationContainer is ready when doing OldDot to NewDot transitions

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/227553
$ #10271

Tests

From Web

Creating a workspace while signed out

Create a new user
Log into OldDot
Use the "Get started with our free plan" inbox task to create a Workspace
Verify the workspace is successfully created in NewDot

Opening an existing workspace

Close NewDot
Return to OldDot
Navigate to Settings > Policies > Group
Select the previously created workspace
Verify the workspace is re-opened in NewDot without issue

Creating a workspace while signed in

Close NewDot
Return to Old Dot
Navigate to Settings > Policies > Group
Create another free plan group policy
Verify another new workspace is successfully created in NewDot

Creating a workspace while signed into another account

Close NewDot
Create another new user
Log into OldDot
Use the "Get started with our free plan" inbox task to create a Workspace
Verify you are signed into the new user and the workspace is successfully created

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Same as Tests but from Web and OldDot Mobile

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-09-21.at.5.47.07.PM.mov

Mobile Web

Desktop

iOS

Android

This reverts commit 46c7c43.

arosiclair · 2022-09-13T16:00:44Z

src/CONST.js

@@ -670,7 +670,6 @@ const CONST = {
            FREE: 'free',
            PERSONAL: 'personal',
            CORPORATE: 'corporate',
-            TEAM: 'team',


@chiragsalian Undoing your revert removed this policy type. Do we actually want this?

POLICY.TYPE.TEAM isn't on main, so if you pull main I think this will go away. We don't use it anywhere.

arosiclair · 2022-09-13T16:19:59Z

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.mov

EDIT: Since we're navigating to the exitTo route and this is set to workspace/new (a fake route) for creating a new workspace it momentarily leads to our not found page. I think the solution is to move away from creating workspaces using exitTo=workspace/new and instead use something like createWorkspace=true. This'll let us continue using exitTo without having to add a bunch of special cases to it.

@neil-marcellini thoughts on this?

neil-marcellini

I'll give a more thorough review later on.

neil-marcellini · 2022-09-15T15:47:39Z

src/pages/LogOutPreviousUserPage.js

+                } else {
+                    const exitTo = lodashGet(this.props, 'route.params.exitTo');
+                    Navigation.dismissModal();
+                    Navigation.navigate(exitTo || ROUTES.HOME);


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));

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).

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.

App/src/libs/actions/App.js

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);

}

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?

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 👹 😂

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

it feels like App.setUpPoliciesAndNavigate() is basically a big if statement and the else statement is just nested down in LogOutPreviousUserPage 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 and LogOutPreviousUserPage 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.

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.

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.

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.

neil-marcellini

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.

neil-marcellini · 2022-09-15T18:14:32Z

src/CONST.js

@@ -670,7 +670,6 @@ const CONST = {
            FREE: 'free',
            PERSONAL: 'personal',
            CORPORATE: 'corporate',
-            TEAM: 'team',


POLICY.TYPE.TEAM isn't on main, so if you pull main I think this will go away. We don't use it anywhere.

neil-marcellini · 2022-09-15T18:16:25Z

src/pages/LogOutPreviousUserPage.js

+                } else {
+                    const exitTo = lodashGet(this.props, 'route.params.exitTo');
+                    Navigation.dismissModal();
+                    Navigation.navigate(exitTo || ROUTES.HOME);


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.

App/src/libs/actions/App.js

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.

…ions

arosiclair · 2022-09-20T17:38:34Z

I added in changes to re-introduce our isNavigationReady promise and used that to fix navigations for transitions. I also needed to add Navigation.dismissModal() call that we had before to clear our /transition spinner page.

neil-marcellini

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.

dangrous · 2022-09-21T21:59:18Z

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).

arosiclair · 2022-09-21T22:39:10Z

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.

dangrous · 2022-09-21T23:16:27Z

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!

neil-marcellini · 2022-09-22T16:20:06Z

Opening an existing workspace

Close NewDot

Return to OldDot

Navigate to Settings > Policies > Group

Select the previously created workspace

Verify the workspace is re-opened in NewDot without issue

After step 8 add a step:
9: Refresh the page

Otherwise you don't see the created workspace. We could fix this on OldDot if we want...

neil-marcellini

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?

neil-marcellini · 2022-09-22T16:33:15Z

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.

neil-marcellini · 2022-09-22T16:59:56Z

I modified the QA steps / tests for you based on my last comment. Here's the checklist, then I'll merge!

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

melvin-bot · 2022-09-22T17:00:50Z

@neil-marcellini looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

neil-marcellini · 2022-09-22T17:01:08Z

All checks passed, that's a bug.

OSBotify · 2022-09-22T17:06:34Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

arosiclair · 2022-09-22T17:52:33Z

I modified the QA steps / tests for you based on my last comment.

Thanks!

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?

Yeah I've noticed this too. I think we're still incrementing the number even after policies that were deleted. Created an issue here.

OSBotify · 2022-09-22T19:17:10Z

🚀 Deployed to staging by @neil-marcellini in version: 1.2.5-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-09-23T22:47:34Z

🚀 Deployed to production by @luacmartins in version: 1.2.5-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

arosiclair added 2 commits September 12, 2022 18:05

Revert "partial revert"

17394da

This reverts commit 46c7c43.

navigate to exitTo when we're already logged in

8f4ec05

arosiclair self-assigned this Sep 12, 2022

arosiclair requested a review from a team as a code owner September 12, 2022 22:28

melvin-bot bot requested review from dangrous and removed request for a team September 12, 2022 22:28

arosiclair commented Sep 13, 2022

View reviewed changes

neil-marcellini requested changes Sep 15, 2022

View reviewed changes

arosiclair added 5 commits September 16, 2022 17:31

Merge branch 'main' into arosiclair-transition-fix

9c2f0ea

Revert "navigate to exitTo when we're already logged in"

94ddbe0

This reverts commit 8f4ec05.

ensure the NavigationContainer is ready when navigating after transit…

bf19d4b

…ions

dismiss transition route for transitions

08e2218

Merge branch 'main' into arosiclair-transition-fix

b4c2972

arosiclair changed the title ~~[WIP] Fix transitions to existing workspaces from OldDot~~ Fix transitions to existing workspaces from OldDot Sep 20, 2022

arosiclair requested review from neil-marcellini and marcaaron September 20, 2022 17:38

neil-marcellini reviewed Sep 21, 2022

View reviewed changes

Merge branch 'main' into arosiclair-transition-fix

1c752eb

fix bad merge

83a011f

neil-marcellini approved these changes Sep 22, 2022

View reviewed changes

neil-marcellini merged commit 86cae9b into main Sep 22, 2022

neil-marcellini deleted the arosiclair-transition-fix branch September 22, 2022 17:00

melvin-bot bot added the Emergency label Sep 22, 2022

neil-marcellini removed the Emergency label Sep 22, 2022

OSBotify mentioned this pull request Sep 22, 2022

Deploy Checklist: New Expensify 2022-09-22 #11215

Closed

20 tasks

kavimuru mentioned this pull request Sep 23, 2022

mWEB/Safari - Nothing happens when tap "Set up my company for free" #11245

Closed

aimane-chnaif mentioned this pull request Oct 3, 2022

[$250] Web - Endless loading appears clicking on https://staging. new.expensify.com/details in concierge #10966

Closed

eVoloshchak mentioned this pull request Nov 4, 2022

[$1000] The transition of opening a new workspace at the creation doesn't match to other pages reported by @Puneet-here #12246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix transitions to existing workspaces from OldDot #10949

Fix transitions to existing workspaces from OldDot #10949

arosiclair commented Sep 12, 2022 •

edited by neil-marcellini

Loading

arosiclair Sep 13, 2022

neil-marcellini Sep 15, 2022

arosiclair commented Sep 13, 2022 •

edited

Loading

neil-marcellini left a comment

neil-marcellini Sep 15, 2022

arosiclair Sep 15, 2022

neil-marcellini Sep 15, 2022

arosiclair Sep 15, 2022

neil-marcellini Sep 16, 2022

arosiclair Sep 16, 2022

neil-marcellini Sep 19, 2022

marcaaron Sep 19, 2022

marcaaron Sep 19, 2022

arosiclair Sep 19, 2022

neil-marcellini left a comment

neil-marcellini Sep 15, 2022

neil-marcellini Sep 15, 2022

arosiclair commented Sep 20, 2022

neil-marcellini left a comment

dangrous commented Sep 21, 2022

arosiclair commented Sep 21, 2022

dangrous commented Sep 21, 2022

neil-marcellini commented Sep 22, 2022

neil-marcellini left a comment

neil-marcellini commented Sep 22, 2022

neil-marcellini commented Sep 22, 2022

PR Reviewer Checklist

melvin-bot bot commented Sep 22, 2022

neil-marcellini commented Sep 22, 2022

OSBotify commented Sep 22, 2022

arosiclair commented Sep 22, 2022

OSBotify commented Sep 22, 2022

OSBotify commented Sep 23, 2022

	if (!isLoggingInAsNewUser && exitTo) {
	// We must call dismissModal() to remove the /transition route from history
	Navigation.dismissModal();
	Navigation.navigate(exitTo);
	}

Fix transitions to existing workspaces from OldDot #10949

Fix transitions to existing workspaces from OldDot #10949

Conversation

arosiclair commented Sep 12, 2022 • edited by neil-marcellini Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arosiclair commented Sep 13, 2022 • edited Loading

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arosiclair commented Sep 20, 2022

neil-marcellini left a comment

Choose a reason for hiding this comment

dangrous commented Sep 21, 2022

arosiclair commented Sep 21, 2022

dangrous commented Sep 21, 2022

neil-marcellini commented Sep 22, 2022

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini commented Sep 22, 2022

neil-marcellini commented Sep 22, 2022

PR Reviewer Checklist

melvin-bot bot commented Sep 22, 2022

neil-marcellini commented Sep 22, 2022

OSBotify commented Sep 22, 2022

arosiclair commented Sep 22, 2022

OSBotify commented Sep 22, 2022

OSBotify commented Sep 23, 2022

arosiclair commented Sep 12, 2022 •

edited by neil-marcellini

Loading

arosiclair commented Sep 13, 2022 •

edited

Loading