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
+58 −10
Merged

Beta race fixes #4205

Show file tree
Hide file tree
Changes from 4 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
17 changes: 14 additions & 3 deletions src/Expensify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import React, {PureComponent} from 'react';
import {View, AppState} from 'react-native';
import Onyx, {withOnyx} from 'react-native-onyx';
import _ from 'underscore';

import BootSplash from './libs/BootSplash';
import listenToStorageEvents from './libs/listenToStorageEvents';
Expand All @@ -21,6 +22,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 +71,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 +84,7 @@ const defaultProps = {
},
updateAvailable: false,
initialReportDataLoaded: false,
betas: [],
};

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

if (this.getAuthToken() && this.props.initialReportDataLoaded) {
Expand Down Expand Up @@ -188,6 +196,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,
};
17 changes: 15 additions & 2 deletions src/pages/ValidateLogin2FANewWorkspacePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ 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} 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 +31,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 +42,7 @@ const defaultProps = {
params: {},
},
session: {},
betas: [],
};
class ValidateLogin2FANewWorkspacePage extends Component {
constructor(props) {
Expand All @@ -62,7 +67,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 (_.isEmpty(this.props.betas)) {
setRedirectToWorkspaceNewAfterSignIn(true);
} else {
Navigation.navigate(ROUTES.WORKSPACE_NEW);
}
}
}

Expand Down Expand Up @@ -144,5 +154,8 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
betas: {
key: ONYXKEYS.BETAS,
},
}),
)(ValidateLogin2FANewWorkspacePage);
17 changes: 15 additions & 2 deletions src/pages/ValidateLoginNewWorkspacePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import {Component} from 'react';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import validateLinkPropTypes from './validateLinkPropTypes';
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 +22,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: [],
};
class ValidateLoginNewWorkspacePage extends Component {
componentDidMount() {
Expand All @@ -39,7 +45,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 (_.isEmpty(this.props.betas)) {
setRedirectToWorkspaceNewAfterSignIn(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@chiragsalian I'm not quite understanding this, so can you explain it to me? If this logic runs, there is already an authToken, so why is the user being redirected to the new workspace page after sign in, when they are already signed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the method name is not the best. Atm i just copied pasted the param i.e., redirectToWorkspaceNewAfterSignIn. The functionality of this is more like redirectAfterSignInAndHaveNecessaryParams. So yeah even though we have the authToken we don't have the betas loaded yet.
So if we go to Navigation.navigate(ROUTES.WORKSPACE_NEW) without the betas it will fail and not show the component, which is the issue.

The betas take a little longer to load after being signed in which is why we save the redirect in onyx and then check if we have everything here before we can navigate to the component. Does that make sense?

As for the method name. Atm i just renamed it exactly to the variable. I thought once I continue with the QR code redirect project of mine I'll discuss the method name closely with you to make it more generic.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I understand about needing to wait for betas to load, but I'm still confused why setRedirectToWorkspaceNewAfterSignIn is necessary. It wasn't in the original code, so it's being added for some reason, but I can't tell what that reason is.

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 wasn't in the original code, so it's being added for some reason, but I can't tell what that reason is.

Do you mean you can't tell easily tell what the reason is in the code and hence want me to add comments or you can't see the reason in general. Cause the reason its added is to fix the GH issue. Since its possible to sign in faster than getting betas, if we redirect to Navigation.navigate(ROUTES.WORKSPACE_NEW) too early then the new workspace screen won't show up. You can easily reproduce the same on dev too with my test steps (Test step - Testing pricing redirect while being signed out hits this flow). Let me know if that makes sense.

Copy link
Contributor Author

@chiragsalian chiragsalian Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Discussed 1:1 and landing on a resolution to be made in my follow up QR scanning project work.

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

Expand All @@ -63,5 +73,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