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

Jump to bottom

Only create 1 free workspace when transitioning #8611

Closed
wants to merge 4 commits into from
Closed

Only create 1 free workspace when transitioning #8611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc2f02d
Prevent navigating to workspace/new on update
neil-marcellini Apr 11, 2022
604bd2e
Don't create a free policy while creating one
neil-marcellini Apr 11, 2022
a00a3a1
Mark the workspace as created after navigating
neil-marcellini Apr 11, 2022
b2b2eec
Merge branch 'neil-transition-flows' into neil-get-started
neil-marcellini Apr 26, 2022
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
20 changes: 20 additions & 0 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,30 @@ const modalScreenListeners = {
};

const propTypes = {

/** Is a workspace currently being created for the user? */
isCreatingWorkspace: PropTypes.bool,

/** Information about the network */
network: PropTypes.shape({
/** Is the network currently offline or not */
isOffline: PropTypes.bool,
}),

/** Session info for the currently logged in user. */
session: PropTypes.shape({

/** Currently logged in user email */
email: PropTypes.string,
}),

...windowDimensionsPropTypes,
};

const defaultProps = {
isCreatingWorkspace: false,
network: {isOffline: true},
session: {email: null},
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is there a reason why we are using null vs. just defaulting session to an empty object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we use _.isNull which will return false if email is undefined, so it could determine that we are logging in as a new user when we are not. I think that's why I did this

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Line 215 in 604bd2e

const isLoggingInAsNewUser = !_.isNull(this.props.session.email) && (email !== this.props.session.email);

};

class AuthScreens extends React.Component {
Expand Down Expand Up @@ -191,6 +204,10 @@ class AuthScreens extends React.Component {
return false;
}

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

Choose a reason for hiding this comment

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

@neil-marcellini Can you provide please just a quick explanation of why we need this?

It seems like the AuthScreens must be mounting twice for us to need this, but I'm unsure which events/states/user actions cause it to mount twice.

Copy link
Contributor Author

@neil-marcellini neil-marcellini Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. I put up this PR before I had figured out that Onyx.clear() was resetting the session silently. I will run the tests with #8736 merged into the feature branch instead of this PR, and if it works then we won't need these changes.

return false;
}

const path = new URL(url).pathname;
const params = new URLSearchParams(url);
const exitTo = params.get('exitTo');
Expand Down Expand Up @@ -370,6 +387,9 @@ AuthScreens.defaultProps = defaultProps;
export default compose(
withWindowDimensions,
withOnyx({
isCreatingWorkspace: {
key: ONYXKEYS.IS_CREATING_WORKSPACE,
},
network: {
key: ONYXKEYS.NETWORK,
},
Expand Down
8 changes: 5 additions & 3 deletions src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ function create(name = '') {
let res = null;
return API.Policy_Create({type: CONST.POLICY.TYPE.FREE, policyName: name})
.then((response) => {
Onyx.set(ONYXKEYS.IS_CREATING_WORKSPACE, false);
if (response.jsonCode !== 200) {
// Show the user feedback
const errorMessage = Localize.translateLocal('workspace.new.genericFailureMessage');
Expand Down Expand Up @@ -157,7 +156,10 @@ function navigateToPolicy(policyID) {
* @param {String} [name]
*/
function createAndNavigate(name = '') {
create(name).then(navigateToPolicy);
create(name).then(() => {
navigateToPolicy();
Onyx.set(ONYXKEYS.IS_CREATING_WORKSPACE, false);
Copy link
Contributor

@marcaaron marcaaron Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

question: Can we leave IS_CREATING_WORKSPACE in the create() call (maybe in a .finally() block)? That seems like a better place for it because anyone calling that method must remember to set IS_CREATING_WORKSPACE to false now.

suggestion: Put it back the way it was if possible or if not make the functions that call create() each set the IS_CREATING_WORKSPACE: true flag.

});
}

/**
Expand Down Expand Up @@ -228,6 +230,7 @@ function createAndGetPolicyList() {
.then(() => {
Navigation.dismissModal();
navigateToPolicy(newPolicyID);
Onyx.set(ONYXKEYS.IS_CREATING_WORKSPACE, false);
});
}

Expand Down Expand Up @@ -509,7 +512,6 @@ export {
removeMembers,
invite,
isAdminOfFreePolicy,
create,
uploadAvatar,
update,
setWorkspaceErrors,
Expand Down
7 changes: 7 additions & 0 deletions src/pages/LogInWithShortLivedTokenPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,14 @@ class LogInWithShortLivedTokenPage extends Component {
}

Log.info('[LoginWithShortLivedTokenPage] User is signed in');
this.navigateToExitRoute();
}

componentDidUpdate() {
this.navigateToExitRoute();
}

navigateToExitRoute() {
// 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 (exitTo === ROUTES.WORKSPACE_NEW) {
Expand Down