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

Fix navigating to workspace by preloading betas #5443

Merged
merged 6 commits into from
Sep 23, 2021
Merged
Changes from 4 commits
Commits
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
49 changes: 37 additions & 12 deletions src/pages/LogInWithShortLivedTokenPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ONYXKEYS from '../ONYXKEYS';
import {signInWithShortLivedToken} from '../libs/actions/Session';
import FullScreenLoadingIndicator from '../components/FullscreenLoadingIndicator';
import Navigation from '../libs/Navigation/Navigation';
import * as User from '../libs/actions/User';

const propTypes = {
/** The parameters needed to authenticate with a short lived token are in the URL */
Expand Down Expand Up @@ -38,37 +39,58 @@ const propTypes = {
/** The authToken for the current session */
email: PropTypes.string,
}),

/** Beta features list */
betas: PropTypes.arrayOf(PropTypes.string),
};

const defaultProps = {
route: {
params: {},
},
session: {},
betas: null,
};

class LogInWithShortLivedTokenPage extends Component {
constructor(props) {
super(props);
this.state = {run: false};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really get run for the state variable, should it be hasRan?

Copy link
Contributor Author

@iwiznia iwiznia Sep 23, 2021

Choose a reason for hiding this comment

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

Yes, hasRun would be a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB

}

componentDidMount() {
const accountID = parseInt(lodashGet(this.props.route.params, 'accountID', ''), 10);
const email = lodashGet(this.props.route.params, 'email', '');
const shortLivedToken = lodashGet(this.props.route.params, 'shortLivedToken', '');
const encryptedAuthToken = lodashGet(this.props.route.params, 'encryptedAuthToken', '');

// 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 the user is revisiting the component authenticated or they were already logged into the right account, we simply redirect them to the exitTo
// If the user is revisiting the component authenticated or they were already logged into
// the right account, we need to ensure they have the betas loaded, then in componentDidUpdate we will
if (this.props.session.authToken && email === this.props.session.email) {
// In order to navigate to a modal, we first have to dismiss the current modal. But there is no current
// modal you say? I know, it confuses me too. 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.
Navigation.dismissModal();
Navigation.navigate(exitTo);
User.getBetas();
return;
}

signInWithShortLivedToken(accountID, email, shortLivedToken, encryptedAuthToken);
this.setState({run: true});
}

componentDidUpdate() {
if (this.state.run || !this.props.betas) {
return;
}

// 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. But there is no current
// modal you say? I know, it confuses me too. 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.
Navigation.dismissModal();
Navigation.navigate(exitTo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Final comment, do we need to call this.setState({run: true}) here too, to prevent other GetBeta API calls from triggering Navigation.navigate a second time?

Copy link
Contributor

Choose a reason for hiding this comment

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

As the GetBetas API call is now happening twice

Copy link
Contributor Author

@iwiznia iwiznia Sep 23, 2021

Choose a reason for hiding this comment

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

As the GetBetas API call is now happening twice

Fixed that

Final comment, do we need to call this.setState({run: true}) here too, to prevent other GetBeta API calls from triggering Navigation.navigate a second time?

We can't since you should not call setState from componentDidUpdate according to linter. In any case it is not needed since once we navigate this component stops existing.

}

render() {
Expand All @@ -84,5 +106,8 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
betas: {
key: ONYXKEYS.BETAS,
},
}),
)(LogInWithShortLivedTokenPage);