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

Handle unauthenticated users when creating a new workspace #3578

Merged
merged 6 commits into from
Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
9 changes: 9 additions & 0 deletions src/Expensify.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import UpdateAppModal from './components/UpdateAppModal';
import Visibility from './libs/Visibility';
import GrowlNotification from './components/GrowlNotification';
import {growlRef} from './libs/Growl';
import Navigation from './libs/Navigation/Navigation';
import ROUTES from './ROUTES';

// Initialize the store when the app loads for the first time
Onyx.init({
Expand Down Expand Up @@ -54,6 +56,9 @@ const propTypes = {

/** Currently logged in user accountID */
accountID: PropTypes.number,

/** Should app immediately redirect to new workspace route once authenticated */
redirectToWorkspaceNewAfterSignIn: PropTypes.bool,
}),

/** Whether a new update is available and ready to install. */
Expand All @@ -67,6 +72,7 @@ const defaultProps = {
session: {
authToken: null,
accountID: null,
redirectToWorkspaceNewAfterSignIn: false,
},
updateAvailable: false,
initialReportDataLoaded: false,
Expand Down Expand Up @@ -113,6 +119,9 @@ class Expensify extends PureComponent {
const previousAuthToken = lodashGet(prevProps, 'session.authToken', null);
if (this.getAuthToken() && !previousAuthToken) {
BootSplash.show({fade: true});
if (lodashGet(this.props, 'session.redirectToWorkspaceNewAfterSignIn', false)) {
Navigation.navigate(ROUTES.WORKSPACE_NEW);
}
}

if (this.getAuthToken() && this.props.initialReportDataLoaded) {
Expand Down
5 changes: 5 additions & 0 deletions src/libs/Navigation/AppNavigator/PublicScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import {createStackNavigator} from '@react-navigation/stack';
import SignInPage from '../../../pages/signin/SignInPage';
import SetPasswordPage from '../../../pages/SetPasswordPage';
import PublicWorkspaceNewView from '../../../pages/workspace/PublicWorkspaceNewView';
import ValidateLoginPage from '../../../pages/ValidateLoginPage';
import SCREENS from '../../../SCREENS';

Expand Down Expand Up @@ -31,5 +32,9 @@ export default () => (
options={defaultScreenOptions}
component={SetPasswordPage}
/>
<RootStack.Screen
name="NewWorkspace"
component={PublicWorkspaceNewView}
/>
</RootStack.Navigator>
);
8 changes: 3 additions & 5 deletions src/libs/actions/Session.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,9 @@ Onyx.connect({
*/
function setSuccessfulSignInData(data) {
PushNotification.register(data.accountID);
Onyx.multiSet({
[ONYXKEYS.SESSION]: {
shouldShowComposeInput: true,
..._.pick(data, 'authToken', 'accountID', 'email'),
},
Onyx.merge(ONYXKEYS.SESSION, {
shouldShowComposeInput: true,
..._.pick(data, 'authToken', 'accountID', 'email'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see why this needed to be switched from multiSet() to merge(), but curious if there are reasons why we were using the set behavior over the merge behavior and if anything unexpected will come from this change (I'm not really sure so maybe it's a NAB - but sort of unclear why we used one over the other in the first place).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was thinking about that, my assumption is that since it was done 7 months ago, we just didn't "need to" do .merge yet. I can keep an eye on issues related to Session storage in the next few weeks just in case

});
}

Expand Down
28 changes: 28 additions & 0 deletions src/pages/workspace/PublicWorkspaceNewView.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';
import Onyx from 'react-native-onyx';
import PropTypes from 'prop-types';
import ONYXKEYS from '../../ONYXKEYS';
import SCREENS from '../../SCREENS';

const propTypes = {
/** react-navigation navigation object available to screen components */
navigation: PropTypes.shape({
/** Method used to navigate to a new page and not keep the current route in the history */
replace: PropTypes.func.isRequired,
}).isRequired,
};

class PublicWorkspaceNewView extends React.PureComponent {
componentDidMount() {
Onyx.merge(ONYXKEYS.SESSION, {redirectToWorkspaceNewAfterSignIn: true});
this.props.navigation.replace(SCREENS.HOME);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Had to use this.props.navigation instead of src/libs/Navigation/Navigation -> navigate because navigationRef didn't exist immediately when navigating to /workspace/new

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, that's interesting and I'd be curious to know more about why that is so we can standardize on this.props.navigtation or Navigation.navigate() and not have to use a combination of them. But this works ok for now I think. Maybe we can create a follow up to figure it out at some point.

}

render() {
return null;
}
}

PublicWorkspaceNewView.propTypes = propTypes;

export default PublicWorkspaceNewView;