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

Beta race fixes #4205

Merged
merged 5 commits into from
Jul 27, 2021
Merged

Beta race fixes #4205

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b75a8b
Beta race fixes
chiragsalian Jul 23, 2021
9318e5d
revert test changes.
chiragsalian Jul 23, 2021
8fda0e3
Merge branch 'main' into chirag-beta-race-fixes
chiragsalian Jul 23, 2021
0b6b408
setting betas as empty array and using lodashGet
chiragsalian Jul 26, 2021
ce7b7e7
warning cleanup
chiragsalian Jul 26, 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
16 changes: 13 additions & 3 deletions src/Expensify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {growlRef} from './libs/Growl';
import Navigation from './libs/Navigation/Navigation';
import ROUTES from './ROUTES';
import StartupTimer from './libs/StartupTimer';
import {setRedirectToWorkspaceNewAfterSignIn} from './libs/actions/Session';

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

/** Whether the initial data needed to render the app is ready */
initialReportDataLoaded: PropTypes.bool,

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

const defaultProps = {
Expand All @@ -79,6 +83,7 @@ const defaultProps = {
},
updateAvailable: false,
initialReportDataLoaded: false,
betas: null,
};

class Expensify extends PureComponent {
Expand Down Expand Up @@ -129,9 +134,11 @@ 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.betas && this.props.session.redirectToWorkspaceNewAfterSignIn) {
setRedirectToWorkspaceNewAfterSignIn(false);
Navigation.navigate(ROUTES.WORKSPACE_NEW);
}

if (this.getAuthToken() && this.props.initialReportDataLoaded) {
Expand Down Expand Up @@ -188,6 +195,9 @@ export default withOnyx({
session: {
key: ONYXKEYS.SESSION,
},
betas: {
key: ONYXKEYS.BETAS,
},
updateAvailable: {
key: ONYXKEYS.UPDATE_AVAILABLE,
initWithStoredValues: false,
Expand Down
10 changes: 10 additions & 0 deletions src/libs/actions/Session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,15 @@ 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 @@ -309,4 +318,5 @@ export {
resendValidationLink,
resetPassword,
restartSignin,
setRedirectToWorkspaceNewAfterSignIn,
};
16 changes: 14 additions & 2 deletions src/pages/ValidateLogin2FANewWorkspacePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import validateLinkPropTypes from './validateLinkPropTypes';
import {continueSessionFromECom} from '../libs/actions/Session';
import {continueSessionFromECom, setRedirectToWorkspaceNewAfterSignIn} from '../libs/actions/Session';
import styles from '../styles/styles';
import ExpensifyCashLogo from '../components/ExpensifyCashLogo';
import variables from '../styles/variables';
Expand All @@ -30,6 +30,9 @@ const propTypes = {
/** The accountID and validateCode are passed via the URL */
route: validateLinkPropTypes,

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

...withLocalizePropTypes,
};

Expand All @@ -38,6 +41,7 @@ const defaultProps = {
params: {},
},
session: {},
betas: null,
};
class ValidateLogin2FANewWorkspacePage extends Component {
constructor(props) {
Expand All @@ -62,7 +66,12 @@ class ValidateLogin2FANewWorkspacePage extends Component {
// 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();
Navigation.navigate(ROUTES.WORKSPACE_NEW);

if (this.props.betas) {
Copy link
Contributor

@TomatoToaster TomatoToaster Jul 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If we are saving this behavior for the beta, why wouldn't we use

Suggested change
if (this.props.betas) {
if (Permissions.canUseFreePlan(this.props.betas)) {

And similarily for other parts referencing this.props.betas

Otherwise, aren't we saying this can be used by anyone? When betas load they'll be [] for those who aren't in any beta so the else part below here wouldn't run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well ROUTES.WORKSPACE_NEW already does the check for Permissions.canUseFreePlan so to keep the code DRY i think its best that the check for canUseFreePlan happens only in NewWorkSpace.js

Otherwise, aren't we saying this can be used by anyone? When betas load they'll be [] for those who aren't in any beta so the else part below here wouldn't run.

I didn't follow your question. Maybe this helps. The if condition determines if newWorkSpace component should render now or later. This take care of race conditions. If we have loaded betas, even if its empty, then we call Navigation.navigate(ROUTES.WORKSPACE_NEW); to render NewWorkSpace since there is no reason to wait. If betas is null, that means we're still waiting and hence we call setRedirectToWorkspaceNewAfterSignIn(true) so that once the betas are loaded (even if its empty) we call Navigation.navigate(ROUTES.WORKSPACE_NEW); at a later time over here. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yes that totally makes sense. Thanks for exposing that!

Navigation.navigate(ROUTES.WORKSPACE_NEW);
} else {
setRedirectToWorkspaceNewAfterSignIn(true);
}
}
}

Expand Down Expand Up @@ -144,5 +153,8 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
betas: {
key: ONYXKEYS.BETAS,
},
}),
)(ValidateLogin2FANewWorkspacePage);
15 changes: 13 additions & 2 deletions src/pages/ValidateLoginNewWorkspacePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import compose from '../libs/compose';
import ONYXKEYS from '../ONYXKEYS';
import Navigation from '../libs/Navigation/Navigation';
import ROUTES from '../ROUTES';
import {continueSessionFromECom} from '../libs/actions/Session';
import {continueSessionFromECom, setRedirectToWorkspaceNewAfterSignIn} from '../libs/actions/Session';

const propTypes = {
/* Onyx Props */
Expand All @@ -20,13 +20,17 @@ const propTypes = {

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

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

const defaultProps = {
route: {
params: {},
},
session: {},
betas: null,
};
class ValidateLoginNewWorkspacePage extends Component {
componentDidMount() {
Expand All @@ -39,7 +43,11 @@ class ValidateLoginNewWorkspacePage extends Component {
// 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();
Navigation.navigate(ROUTES.WORKSPACE_NEW);
if (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.

Similar to comment in other file - how do you feel about using lodashGet here (like below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason to use lodashGet here imo since the proptype and default are provided. The other example of lodashGet makes sense because accountID and validateCode are not defined in the defaultParams.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, but I did mention how session.authToken and session.accountID are in defaultProps and defaulted via lodashGet in the other component. But still I don't mind not using lodashGet here since this.props.betas is not nested in another onyx key 👍

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'm not going to add it atm since I don't see its value.

Navigation.navigate(ROUTES.WORKSPACE_NEW);
} else {
setRedirectToWorkspaceNewAfterSignIn(true);
}
return;
}

Expand All @@ -63,5 +71,8 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
betas: {
key: ONYXKEYS.BETAS,
},
}),
)(ValidateLoginNewWorkspacePage);
5 changes: 2 additions & 3 deletions src/pages/workspace/PublicWorkspaceNewView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import React from 'react';
import Onyx from 'react-native-onyx';
import PropTypes from 'prop-types';
import ONYXKEYS from '../../ONYXKEYS';
import SCREENS from '../../SCREENS';
import {setRedirectToWorkspaceNewAfterSignIn} from '../../libs/actions/Session';

const propTypes = {
/** react-navigation navigation object available to screen components */
Expand All @@ -14,7 +13,7 @@ const propTypes = {

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

Expand Down