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

Allow users to login with validate code and be redirected to Workspace/Card modal #4519

Merged
merged 36 commits into from
Aug 20, 2021
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c64aeff
Add new routes
Aug 9, 2021
d5b58c1
Add workspace_card route to be available when validating login and re…
Aug 10, 2021
38d8a24
remove console statements and add missing import
Aug 10, 2021
11145dd
Merge branch 'main' of github.com:Expensify/App into amal-validate-co…
Aug 10, 2021
ee2e220
Update uses to remove VALIDATE_ since we aren't validating logins
Aug 10, 2021
0bec935
Rename to redirectAfterSignIn so flag is generic to all validateCodeL…
Aug 10, 2021
6d20074
Rename component and constants to match generic functionality of logg…
Aug 10, 2021
7ca2bb3
Rename files to fit new generic functionality
Aug 10, 2021
e395f45
Merge branch 'main' of github.com:Expensify/App into amal-validate-co…
Aug 11, 2021
b4e635e
Remove uneeded line
Aug 11, 2021
f47c728
Clearer comment
Aug 11, 2021
1b0607b
Fix missing parentheses
Aug 11, 2021
1c330be
Merge branch 'main' of github.com:Expensify/App into amal-validate-co…
Aug 12, 2021
5ca0808
Merge branch 'main' of github.com:Expensify/App into amal-validate-co…
Aug 12, 2021
4b5c433
Remove unused public workspace route
Aug 12, 2021
223c4be
rename to setRedirectAfterSignIn
Aug 12, 2021
ed2a67e
consolidated beta loading logic in LoginWith components
Aug 13, 2021
f0ffacf
Make sure new policy is autocreated only after all policies have been…
Aug 13, 2021
8115017
remove unused proptype
Aug 13, 2021
8cceef2
revert change to permissions
Aug 13, 2021
d154f44
fix comment typo
Aug 13, 2021
90b2c21
Show loader to prevent user from being immediately rerouted to the ho…
Aug 13, 2021
876fc46
Fix 2fa page's styles
Aug 13, 2021
cd50b67
Merge branch 'main' of github.com:Expensify/App into amal-validate-co…
Aug 13, 2021
ec60643
Fix missing React import needed for FullScreenLoadingIndicator
Aug 13, 2021
501dd95
Use existing collection to determine if policies have been loaded
Aug 16, 2021
786b57e
Use switch and add comments for clarity
Aug 17, 2021
96b4a4a
Use existing policies collection to determine if policies have loaded
Aug 17, 2021
cc0c2b8
Use screens relevant to validateCode not 2fa
Aug 17, 2021
fac742e
Remove unused Onyx key
Aug 17, 2021
fcbeadf
Clarify when this logic can be removed
Aug 17, 2021
b40cabf
Add todo to comment so it's clearer theres some action to be done in …
Aug 17, 2021
20ee86a
Add missing doc
Aug 18, 2021
23798c7
Use Policy to make code more contextual
Aug 18, 2021
b4d779c
Use @
Aug 18, 2021
3e6fae6
remove @TODO since workspace has already been created
Aug 19, 2021
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
21 changes: 0 additions & 21 deletions src/Expensify.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import PropTypes from 'prop-types';
import React, {PureComponent} from 'react';
import {View, AppState} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';

import BootSplash from './libs/BootSplash';
import * as ActiveClientManager from './libs/ActiveClientManager';
Expand All @@ -17,8 +16,6 @@ import Visibility from './libs/Visibility';
import GrowlNotification from './components/GrowlNotification';
import {growlRef} from './libs/Growl';
import StartupTimer from './libs/StartupTimer';
import {setRedirectToWorkspaceNewAfterSignIn} from './libs/actions/Session';
import {create} from './libs/actions/Policy';

const propTypes = {
/* Onyx Props */
Expand All @@ -31,9 +28,6 @@ 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 @@ -44,21 +38,16 @@ const propTypes = {

/** Tells us if the sidebar has rendered */
isSidebarLoaded: PropTypes.bool,

/** List of betas */
betas: PropTypes.arrayOf(PropTypes.string),
};

const defaultProps = {
session: {
authToken: null,
accountID: null,
redirectToWorkspaceNewAfterSignIn: false,
},
updateAvailable: false,
initialReportDataLoaded: false,
isSidebarLoaded: false,
betas: [],
};

class Expensify extends PureComponent {
Expand Down Expand Up @@ -111,13 +100,6 @@ class Expensify extends PureComponent {
BootSplash.show({fade: true});
}

if (this.getAuthToken()
&& !_.isEmpty(this.props.betas)
&& lodashGet(this.props, 'session.redirectToWorkspaceNewAfterSignIn', false)) {
setRedirectToWorkspaceNewAfterSignIn(false);
create();
}

if (this.getAuthToken() && this.props.initialReportDataLoaded && this.props.isSidebarLoaded) {
BootSplash.getVisibilityStatus()
.then((value) => {
Expand Down Expand Up @@ -172,9 +154,6 @@ export default withOnyx({
session: {
key: ONYXKEYS.SESSION,
},
betas: {
key: ONYXKEYS.BETAS,
},
updateAvailable: {
key: ONYXKEYS.UPDATE_AVAILABLE,
initWithStoredValues: false,
Expand Down
6 changes: 4 additions & 2 deletions src/ROUTES.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ export default {

// This is a special validation URL that will take the user to /workspace/new after validation. This is used
// when linking users from e.com in order to share a session in this app.
VALIDATE_LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE: 'v/:accountID/:validateCode/new-workspace',
VALIDATE_LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE: 'v/:accountID/:validateCode/2fa/new-workspace',
LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE: 'v/:accountID/:validateCode/new-workspace',
Copy link
Contributor

Choose a reason for hiding this comment

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

🙇 thank you for the name updates

LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE: 'v/:accountID/:validateCode/2fa/new-workspace',
LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD: 'v/:accountID/:validateCode/workspace/:policyID/card',
LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD: 'v/:accountID/:validateCode/2fa/workspace/:policyID/card',
ENABLE_PAYMENTS: 'enable-payments',
WORKSPACE: 'workspace',
WORKSPACE_CARD: ':policyID/card',
Expand Down
6 changes: 4 additions & 2 deletions src/SCREENS.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ export default {
HOME: 'Home',
LOADING: 'Loading',
REPORT: 'Report',
VALIDATE_LOGIN_NEW_WORKSPACE: 'ValidateLoginNewWorkspace',
VALIDATE_LOGIN_2FA_NEW_WORKSPACE: 'ValidateLogin2FANewWorkspace',
LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE: 'LoginWithValidateCodeNewWorkspace',
LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE: 'LoginWithValidateCode2FANewWorkspace',
LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD: 'LoginWithValidateCodeWorkspaceCard',
LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD: 'LoginWithValidateCode2FAWorkspaceCard',
};
22 changes: 16 additions & 6 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ import {
} from './ModalStackNavigators';
import SCREENS from '../../../SCREENS';
import Timers from '../../Timers';
import ValidateLoginNewWorkspacePage from '../../../pages/ValidateLoginNewWorkspacePage';
import ValidateLogin2FANewWorkspacePage from '../../../pages/ValidateLogin2FANewWorkspacePage';
import LoginWithValidateCodePage from '../../../pages/LoginWithValidateCodePage';
import LoginWithValidateCode2FAPage from '../../../pages/LoginWithValidateCode2FAPage';
import WorkspaceSettingsDrawerNavigator from './WorkspaceSettingsDrawerNavigator';
import spacing from '../../../styles/utilities/spacing';
import CardOverlay from '../../../components/CardOverlay';
Expand Down Expand Up @@ -298,14 +298,24 @@ class AuthScreens extends React.Component {
component={ValidateLoginPage}
/>
<RootStack.Screen
name={SCREENS.VALIDATE_LOGIN_NEW_WORKSPACE}
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE}
options={defaultScreenOptions}
component={ValidateLoginNewWorkspacePage}
component={LoginWithValidateCodePage}
/>
<RootStack.Screen
name={SCREENS.VALIDATE_LOGIN_2FA_NEW_WORKSPACE}
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE}
options={defaultScreenOptions}
component={ValidateLogin2FANewWorkspacePage}
component={LoginWithValidateCode2FAPage}
/>
<RootStack.Screen
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD}
options={defaultScreenOptions}
component={LoginWithValidateCodePage}
/>
<RootStack.Screen
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD}
options={defaultScreenOptions}
component={LoginWithValidateCode2FAPage}
/>

{/* These are the various modal routes */}
Expand Down
23 changes: 14 additions & 9 deletions src/libs/Navigation/AppNavigator/PublicScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ 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';
import ValidateLoginNewWorkspacePage from '../../../pages/ValidateLoginNewWorkspacePage';
import ValidateLogin2FANewWorkspacePage from '../../../pages/ValidateLogin2FANewWorkspacePage';
import LoginWithValidateCodePage from '../../../pages/LoginWithValidateCodePage';
import LoginWithValidateCode2FAPage from '../../../pages/LoginWithValidateCode2FAPage';
import defaultScreenOptions from './defaultScreenOptions';

const RootStack = createStackNavigator();
Expand All @@ -29,18 +28,24 @@ export default () => (
component={SetPasswordPage}
/>
<RootStack.Screen
name="NewWorkspace"
component={PublicWorkspaceNewView}
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE}
options={defaultScreenOptions}
component={LoginWithValidateCodePage}
/>
<RootStack.Screen
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE}
options={defaultScreenOptions}
component={LoginWithValidateCode2FAPage}
/>
<RootStack.Screen
name={SCREENS.VALIDATE_LOGIN_NEW_WORKSPACE}
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD}
options={defaultScreenOptions}
component={ValidateLoginNewWorkspacePage}
component={LoginWithValidateCodePage}
/>
<RootStack.Screen
name={SCREENS.VALIDATE_LOGIN_2FA_NEW_WORKSPACE}
name={SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD}
options={defaultScreenOptions}
component={ValidateLogin2FANewWorkspacePage}
component={LoginWithValidateCode2FAPage}
/>
</RootStack.Navigator>
);
6 changes: 4 additions & 2 deletions src/libs/Navigation/linkingConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ export default {
// Public Routes
SetPassword: ROUTES.SET_PASSWORD_WITH_VALIDATE_CODE,
ValidateLogin: ROUTES.VALIDATE_LOGIN_WITH_VALIDATE_CODE,
[SCREENS.VALIDATE_LOGIN_NEW_WORKSPACE]: ROUTES.VALIDATE_LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE,
[SCREENS.VALIDATE_LOGIN_2FA_NEW_WORKSPACE]: ROUTES.VALIDATE_LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE,
[SCREENS.LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE]: ROUTES.LOGIN_WITH_VALIDATE_CODE_NEW_WORKSPACE,
[SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE]: ROUTES.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE,
[SCREENS.LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD]: ROUTES.LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD,
[SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD]: ROUTES.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD,

// Modal Screens
Settings: {
Expand Down
10 changes: 0 additions & 10 deletions src/libs/actions/Session.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,15 +303,6 @@ function continueSessionFromECom(accountID, validateCode, twoFactorAuthCode) {
});
}

/**
* Sets the redirectToWorkspaceNewAfterSignIn flag in the session variable
*
* @param {Boolean} shouldRedirect
*/
function setRedirectToWorkspaceNewAfterSignIn(shouldRedirect) {
Onyx.merge(ONYXKEYS.SESSION, {redirectToWorkspaceNewAfterSignIn: shouldRedirect});
}

export {
continueSessionFromECom,
fetchAccountDetails,
Expand All @@ -322,5 +313,4 @@ export {
resendValidationLink,
resetPassword,
restartSignin,
setRedirectToWorkspaceNewAfterSignIn,
};
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ import {TextInput, View} from 'react-native';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import validateLinkPropTypes from './validateLinkPropTypes';
import {continueSessionFromECom, setRedirectToWorkspaceNewAfterSignIn} from '../libs/actions/Session';
import {continueSessionFromECom} from '../libs/actions/Session';
import styles from '../styles/styles';
import ExpensifyCashLogo from '../components/ExpensifyCashLogo';
import variables from '../styles/variables';
Expand All @@ -17,7 +16,11 @@ import Text from '../components/Text';
import compose from '../libs/compose';
import ONYXKEYS from '../ONYXKEYS';
import Navigation from '../libs/Navigation/Navigation';
import ROUTES from '../ROUTES';
import SCREENS from '../SCREENS';
import {create} from '../libs/actions/Policy';
import Permissions from '../libs/Permissions';
import FullScreenLoadingIndicator from '../components/FullscreenLoadingIndicator';

const propTypes = {
/* Onyx Props */
Expand All @@ -28,7 +31,7 @@ const propTypes = {
authToken: PropTypes.string,
}),

/** The accountID and validateCode are passed via the URL */
/** The route name, accountID, and validateCode are passed via the URL */
route: validateLinkPropTypes,

/** List of betas */
Expand All @@ -44,7 +47,7 @@ const defaultProps = {
session: {},
betas: [],
};
class ValidateLogin2FANewWorkspacePage extends Component {
class LoginWithValidateCode2FAPage extends Component {
constructor(props) {
super(props);

Expand All @@ -58,21 +61,44 @@ class ValidateLogin2FANewWorkspacePage extends Component {
}

componentDidMount() {
// If the user has an active session already, they need to be redirected straight to the new workspace page
// If the user has an active session already, they need to be redirected straight to the relevant page
if (this.props.session.authToken) {
// 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 they user cancels
// out of the new workspace modal, then they will be routed back to
// /v/<accountID>/<validateCode>/new-workspace and we don't want that. We want them to go back to `/` and
// by calling dismissModal(), the /v/... route is removed from history so the user will get taken to `/`
// modal you say? I know, it confuses me too. Without dismissing the current modal, if the user cancels out
// of the new workspace modal, then they will be routed back to
// /v/<accountID>/<validateCode>/workspace/123/card and we don't want that. We want them to go back to `/`
// and by calling dismissModal(), the /v/... route is removed from history so the user will get taken to `/`
// if they cancel out of the new workspace modal.
Navigation.dismissModal();
if (Permissions.canUseFreePlan(this.props.betas)) {
this.rerouteToRelevantPage();
}
}
}

if (_.isEmpty(this.props.betas)) {
setRedirectToWorkspaceNewAfterSignIn(true);
} else {
componentDidUpdate() {
// Betas can be loaded a little after a user is authenticated, so check again if the betas have been updated
if (this.props.session.authToken && Permissions.canUseFreePlan(this.props.betas)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also check that this.props.betas exists before calling canUseFreePlan()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will always exist because of the default props of this component having it set to []. We could check if it's empty, but Permissions.canUseFreePlan doesn't need the argument to be a non-empty array so I feel like that check might be redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a better description of why I thought this could have been a problem:

  1. Component mounts
  2. Component is updated with a session auth token, but betas haven't loaded yet
  3. Permission check happens with an empty beta list
  4. Betas finish loading
  5. Now the beta check from 3 was stale and wrong because of the race condition

It's possible this isn't actually a problem because the first permission check would always return false if the beta array is empty, and there would be no redirect. Then, once the betas finish loading, there would be another component update with the proper beta data and the permission check would run and return true and then the redirect happens.

I think we are OK? It's quite the "gotcha" though and relies on the first permission check returning false with an empty beta array. If someone were to ever implement a permission check that did opposite logic (like cannotUseFreePlan, then the race condition would happen).

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 think we are ok! And ahh yeah I haven't seen the cannotUse type of beta checks, so I didn't consider them. Your reasoning around the beta checks is right and the component loads endlessly Until we CAN prove the user is in the beta. Hypothetically a user who isn't in any betas who landed on this page would see an endless loader, but such a user shouldn't be able to access the link that brings them here in the first place.

this.rerouteToRelevantPage();
}
}

rerouteToRelevantPage() {
// Since all 2FA validate code login routes lead to this component, redirect to the appropriate page based on
// the original route.
switch (this.props.route.name) {
case SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD:
Navigation.navigate(ROUTES.getWorkspaceCardRoute(this.props.route.params.policyID));
break;

case SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE:
// Creating a policy will reroute the user to the settings page afterwards
create();
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this in the surrounding context, it's hard to know what this does. I would suggest changing the import to be import * as Policy from.... then change this reference to Policy.create() so that it helps add context about what's getting created without having to check the imports.

}
break;

default:
Navigation.navigate(ROUTES.HOME);
break;
}
}

Expand All @@ -93,18 +119,17 @@ class ValidateLogin2FANewWorkspacePage extends Component {
}

render() {
// If the user is already logged in, don't need to display anything because they will get redirected to the
// new workspace page in componentDidMount
// Show a loader so that the user isn't immediately kicked to the home page before rerouteToRelevantPage runs
if (this.props.session.authToken) {
return null;
return <FullScreenLoadingIndicator />;
}

return (
<View style={[styles.signInPageInnerNative]}>
<View style={[styles.signInPageInnerNative, styles.alignItemsCenter, styles.mt6]}>
<View style={[styles.componentHeightLarge]}>
<ExpensifyCashLogo width={variables.componentSizeLarge} height={variables.componentSizeLarge} />
</View>
<View style={[styles.mb6, styles.alignItemsCenter]}>
<View style={[styles.mb6]}>
<Text style={[styles.h1]}>
{this.props.translate('signInPage.expensifyDotCash')}
</Text>
Expand Down Expand Up @@ -145,8 +170,8 @@ class ValidateLogin2FANewWorkspacePage extends Component {
}
}

ValidateLogin2FANewWorkspacePage.propTypes = propTypes;
ValidateLogin2FANewWorkspacePage.defaultProps = defaultProps;
LoginWithValidateCode2FAPage.propTypes = propTypes;
LoginWithValidateCode2FAPage.defaultProps = defaultProps;

export default compose(
withLocalize,
Expand All @@ -158,4 +183,4 @@ export default compose(
key: ONYXKEYS.BETAS,
},
}),
)(ValidateLogin2FANewWorkspacePage);
)(LoginWithValidateCode2FAPage);
Loading