-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Changes from 4 commits
b2a7370
e8b3c20
df5de8d
8795277
9529c79
8a169f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 */ | ||
|
@@ -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}; | ||
} | ||
|
||
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(); | ||
Julesssss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
|
||
signInWithShortLivedToken(accountID, email, shortLivedToken, encryptedAuthToken); | ||
this.setState({run: true}); | ||
Julesssss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final comment, do we need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the GetBetas API call is now happening twice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fixed that
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() { | ||
|
@@ -84,5 +106,8 @@ export default compose( | |
session: { | ||
key: ONYXKEYS.SESSION, | ||
}, | ||
betas: { | ||
key: ONYXKEYS.BETAS, | ||
}, | ||
}), | ||
)(LogInWithShortLivedTokenPage); |
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 don't really get
run
for the state variable, should it behasRan
?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.
Yes, hasRun would be a better name.
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.
NAB