-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Only create 1 free workspace when transitioning #8611
Conversation
Still a draft PR since I'm working on resolving this bug with flow C. Screen.Recording.2022-04-12.at.1.54.52.PM.mov |
It looks to me like some action is finishing and probably calling |
@roryabraham thanks for taking a look. I added logs for all calls to Navigation.navigate and I did not see any of my log lines in the output for this flow. I believe that the problem is really the authToken getting set to null as I mentioned in my comment on the issue. I want to figure out why that is happening. I'll try looking into the internals of Onyx. |
4f594d1
to
a00a3a1
Compare
Ready for review! Please let me know if you have questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple questions. Looks like there's some lint errors.
network: {isOffline: true}, | ||
session: {email: null}, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
const isLoggingInAsNewUser = !_.isNull(this.props.session.email) && (email !== this.props.session.email); |
@@ -191,6 +204,10 @@ class AuthScreens extends React.Component { | |||
return false; | |||
} | |||
|
|||
if (this.props.isCreatingWorkspace) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
create(name).then(navigateToPolicy); | ||
create(name).then(() => { | ||
navigateToPolicy(); | ||
Onyx.set(ONYXKEYS.IS_CREATING_WORKSPACE, false); |
There was a problem hiding this comment.
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.
Details
Related Issue
$ #7617
Tests
Below are tests for all of the transition flows from Web-Expensify to NewDot. This PR specifically fixes flows A, B, and D. Once all the PRs have been merged into the feature branch we can test all of the flows, so ignore the rest for now.
@gmail.com
addressE. Pricing select free
E.1 Creating a free policy
@gmail.com
addressPR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android