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.

Sign up for GitHub

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

Jump to bottom

Fix transitions to existing workspaces from OldDot #10949

Merged
merged 9 commits into from
Sep 22, 2022
Merged

Fix transitions to existing workspaces from OldDot #10949

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
17394da
Revert "partial revert"
arosiclair Sep 12, 2022
8f4ec05
navigate to exitTo when we're already logged in
arosiclair Sep 12, 2022
9c2f0ea
Merge branch 'main' into arosiclair-transition-fix
arosiclair Sep 16, 2022
94ddbe0
Revert "navigate to exitTo when we're already logged in"
arosiclair Sep 19, 2022
bf19d4b
ensure the NavigationContainer is ready when navigating after transit…
arosiclair Sep 19, 2022
08e2218
dismiss transition route for transitions
arosiclair Sep 19, 2022
b4c2972
Merge branch 'main' into arosiclair-transition-fix
arosiclair Sep 19, 2022
1c752eb
Merge branch 'main' into arosiclair-transition-fix
arosiclair Sep 21, 2022
83a011f
fix bad merge
arosiclair Sep 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,6 @@ const CONST = {
FREE: 'free',
PERSONAL: 'personal',
CORPORATE: 'corporate',
TEAM: 'team',
Copy link
Contributor Author

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?

Copy link
Contributor

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.

},
ROLE: {
ADMIN: 'admin',
Expand Down
9 changes: 1 addition & 8 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import Onyx, {withOnyx} from 'react-native-onyx';
import moment from 'moment';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import * as StyleUtils from '../../../styles/StyleUtils';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import CONST from '../../../CONST';
Expand Down Expand Up @@ -87,9 +86,6 @@ const modalScreenListeners = {

const propTypes = {
...windowDimensionsPropTypes,

/** The current path as reported by the NavigationContainer */
currentPath: PropTypes.string.isRequired,
};

class AuthScreens extends React.Component {
Expand All @@ -116,6 +112,7 @@ class AuthScreens extends React.Component {
// Listen for report changes and fetch some data we need on initialization
UnreadIndicatorUpdater.listenForReportChanges();
App.openApp();
App.setUpPoliciesAndNavigate(this.props.session);
Timing.end(CONST.TIMING.HOMEPAGE_INITIAL_RENDER);

const searchShortcutConfig = CONST.KEYBOARD_SHORTCUTS.SEARCH;
Expand All @@ -133,10 +130,6 @@ class AuthScreens extends React.Component {
}

shouldComponentUpdate(nextProps) {
// we perform this check here instead of componentDidUpdate to skip an unnecessary re-render
if (this.props.currentPath !== nextProps.currentPath) {
App.setUpPoliciesAndNavigate(nextProps.session, nextProps.currentPath);
}
return nextProps.isSmallScreenWidth !== this.props.isSmallScreenWidth;
}

Expand Down
5 changes: 1 addition & 4 deletions src/libs/Navigation/AppNavigator/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,14 @@ import AuthScreens from './AuthScreens';
const propTypes = {
/** If we have an authToken this is true */
authenticated: PropTypes.bool.isRequired,

/** The current path as reported by the NavigationContainer */
currentPath: PropTypes.string.isRequired,
};

const AppNavigator = props => (
props.authenticated
? (

// These are the protected screens and only accessible when an authToken is present
<AuthScreens currentPath={props.currentPath} />
<AuthScreens />
)
: (
<PublicScreens />
Expand Down
14 changes: 1 addition & 13 deletions src/libs/Navigation/NavigationRoot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,6 @@ const propTypes = {
};

class NavigationRoot extends Component {
constructor(props) {
super(props);

this.state = {
currentPath: '',
};

this.parseAndLogRoute = this.parseAndLogRoute.bind(this);
}

/**
* Intercept navigation state changes and log it
* @param {NavigationState} state
Expand All @@ -57,8 +47,6 @@ class NavigationRoot extends Component {
}

UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount();

this.setState({currentPath});
}

render() {
Expand All @@ -79,7 +67,7 @@ class NavigationRoot extends Component {
enabled: false,
}}
>
<AppNavigator authenticated={this.props.authenticated} currentPath={this.state.currentPath} />
<AppNavigator authenticated={this.props.authenticated} />
</NavigationContainer>
);
}
Expand Down
6 changes: 6 additions & 0 deletions src/pages/LogOutPreviousUserPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import ONYXKEYS from '../ONYXKEYS';
import * as Session from '../libs/actions/Session';
import FullScreenLoadingIndicator from '../components/FullscreenLoadingIndicator';
import * as SessionUtils from '../libs/SessionUtils';
import Navigation from '../libs/Navigation/Navigation';
import ROUTES from '../ROUTES';

const propTypes = {
/** The data about the current session which will be set once the user is authenticated and we return to this component as an AuthScreen */
Expand All @@ -24,6 +26,10 @@ class LogOutPreviousUserPage extends Component {
const isLoggingInAsNewUser = SessionUtils.isLoggingInAsNewUser(transitionURL, sessionEmail);
if (isLoggingInAsNewUser) {
Session.signOutAndRedirectToSignIn();
} else {
const exitTo = lodashGet(this.props, 'route.params.exitTo');
Navigation.dismissModal();
Navigation.navigate(exitTo || ROUTES.HOME);
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

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'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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

}
});
}
Expand Down