-
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
Add shouldCreateNewPolicy param to getPolicySummaries action #5706
Conversation
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.
Looks great just left a couple minor comments.
Updated! |
src/libs/actions/Policy.js
Outdated
}).then(() => { | ||
if (shouldAutomaticallyReroute) { | ||
Navigation.dismissModal(); | ||
Navigation.navigate(ROUTES.getWorkspaceCardRoute(res.policyID)); |
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.
Oh this is also accessing the policyID
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.
Looks great Rory! +1 for checking if the policyID is set as Marc mentioned above.
# Conflicts: # src/libs/actions/Policy.js
Thanks for the reviews @marcaaron and @mountiny! I made some updates to this PR (bringing back the
That way, we're not waiting for full policies to load before rerouting to the new policy (only waiting for policySummaries). We are still loading the full list of full policies, but we could probably stop doing that once https://github.com/Expensify/Auth/pull/6019 is done. It also might not end up being a problem if https://github.com/Expensify/Auth/pull/6002 works out well. |
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.
LGTM. Left some non blocker comments.
|
||
if (shouldCreateNewPolicy) { | ||
Navigation.dismissModal(); | ||
Navigation.navigate(newPolicyID ? ROUTES.getWorkspaceCardRoute(newPolicyID) : ROUTES.HOME); |
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.
NAB, might be good to leave a comment about why we need to navigate to the /
when there is no newPolicyID
- although doesn't seem likely to happen.
* More specifically, this action will: | ||
* 1. Optionally create a new policy. | ||
* 2. Fetch policy summaries. | ||
* 3. Optionally navigate to the new policy. |
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.
NAB, this makes it sound like there is another option to navigate to a created policy. But what actually happens is... if the policy is created we navigate to it.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…dition Add shouldCreateNewPolicy param to getPolicySummaries action (cherry picked from commit 30ce47c)
🚀 Cherry-picked to staging by @AndrewGable in version: 1.1.7-18 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
I think this is not QAble by us and should be internal - Left a comment with more info here https://expensify.slack.com/archives/C9YU7BX5M/p1634265038218100
|
Verified and checked off |
🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀
|
🚀 Deployed to staging by @marcaaron in version: 1.1.7-25 🚀
|
🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀
|
Details
This PR fixes two related race conditions:
LoginWithShortLivedTokenPage
race condition explanationSession.signInWithShortLivedToken
API.createLogin
or betas load, LogInWithShortLivedTokenPage immediately runsthis.setState({hasRun: true});
componentDidUpdate
,this.state.hasRun
is true, and then we are stuck in LogInWithShortLivedTokenPage / infinite spinner forever.LoginWithShortLivedTokenPage
race condition fixGet rid of
hasRun
state just navigate toexitTo
once betas are loaded and we are logged into the correct account.Policy load/creation race condition explanation
Policy load/creation race condition fix
/transition
+ if we are going to exitTo/workspace/new
. Those conditions tell us if we should create a new policy.AuthScreens
, we will call the new API withshouldCreateFreePolicy
.Fixed Issues
$ #5518
Tests / QA Steps
Set up my company for free
Tested On