Add shouldCreateNewPolicy param to getPolicySummaries action #5706

roryabraham · 2021-10-06T23:08:06Z

Details

This PR fixes two related race conditions:

`LoginWithShortLivedTokenPage` race condition explanation

Page loads, beta/session props are not present yet.
Calls Session.signInWithShortLivedToken
Before call to API.createLogin or betas load, LogInWithShortLivedTokenPage immediately runs this.setState({hasRun: true});
In componentDidUpdate, this.state.hasRun is true, and then we are stuck in LogInWithShortLivedTokenPage / infinite spinner forever.

`LoginWithShortLivedTokenPage` race condition fix

Get rid of hasRun state just navigate to exitTo once betas are loaded and we are logged into the correct account.

Policy load/creation race condition explanation

We send an API request to create the policy
We send API requests to fetch existing policies.
First API request returns and policy is added to Onyx.
Second API request(s) return, but at the time when the API was collecting the list of policies to send in the response, the new policy hadn't been created yet. So it's missing from this response and we remove it from Onyx.

Policy load/creation race condition fix

Have AuthScreens determine if we are on /transition + if we are going to exitTo /workspace/new. Those conditions tell us if we should create a new policy.
Create a new action that will optionally create a new policy before loading the full policy list and updating Onyx.
- Note: we were previously loading policy summaries and full policies back-to-back. Instead, we just load full policies and use that to save a simplified object to Onyx. We can revisit later to load full policies only when absolutely needed, but what we have here seems no worse than what we had before.
Once the route is loaded in AuthScreens, we will call the new API with shouldCreateFreePolicy.

Fixed Issues

$ #5518

Tests / QA Steps

Sign into a new account on OldDot mWeb
Click Set up my company for free
Verify that:
1. you are taken to NewDot
2. logged into the new account
3. a workspace has been created
4. you are navigated to the page for the new workspace.

Tested On

marcaaron

Looks great just left a couple minor comments.

src/libs/actions/Policy.js

src/pages/LogInWithShortLivedTokenPage.js

src/libs/actions/Policy.js

roryabraham · 2021-10-08T00:50:37Z

Updated!

marcaaron · 2021-10-08T00:58:55Z

src/libs/actions/Policy.js

+        }).then(() => {
+            if (shouldAutomaticallyReroute) {
+                Navigation.dismissModal();
+                Navigation.navigate(ROUTES.getWorkspaceCardRoute(res.policyID));


Oh this is also accessing the policyID

mountiny

Looks great Rory! +1 for checking if the policyID is set as Marc mentioned above.

# Conflicts: # src/libs/actions/Policy.js

roryabraham · 2021-10-08T17:22:16Z

Thanks for the reviews @marcaaron and @mountiny! I made some updates to this PR (bringing back the Get?returnValueList=policySummaryList API call) such that the getPolicyList action is now an all-in-one that does the following:

Optionally creates a new policy
Loads policy summaries
Then optionally redirects to the new policy
Then loads full policies.

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.

marcaaron

LGTM. Left some non blocker comments.

marcaaron · 2021-10-08T17:40:12Z

src/libs/actions/Policy.js

+
+            if (shouldCreateNewPolicy) {
+                Navigation.dismissModal();
+                Navigation.navigate(newPolicyID ? ROUTES.getWorkspaceCardRoute(newPolicyID) : ROUTES.HOME);


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.

marcaaron · 2021-10-08T17:41:52Z

src/libs/actions/Policy.js

+ * More specifically, this action will:
+ * 1. Optionally create a new policy.
+ * 2. Fetch policy summaries.
+ * 3. Optionally navigate to the new policy.


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.

OSBotify · 2021-10-08T18:14:59Z

✋ 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)

OSBotify · 2021-10-14T22:17:05Z

🚀 Cherry-picked to staging by @AndrewGable in version: 1.1.7-18 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

isagoico · 2021-10-15T02:46:16Z

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

We were trying to test this #5706 and I think this might be internal QA. I'm not able to reach the option to "Click Set up my company for free " using a gmail account. If I use a domain account it will always ask me to validate before logging in.

roryabraham · 2021-10-15T05:05:33Z

Verified and checked off

OSBotify · 2021-10-15T05:47:23Z

🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-10-15T06:41:19Z

🚀 Deployed to staging by @marcaaron in version: 1.1.7-25 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-10-23T00:43:41Z

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham added 3 commits October 6, 2021 14:14

Add shouldCreateNewPolicy param to getPolicySummaries action

1725ba0

Get rid of policySummaryList API call, fix race condition

6ea4750

Fix authentication race condition in LogInWithShortLivedTokenPage

27a0d69

roryabraham self-assigned this Oct 6, 2021

Deprecate WorkspaceNew page

12a2930

roryabraham mentioned this pull request Oct 6, 2021

Fix infinite spinner on OldDot transition #5552

Closed

5 tasks

roryabraham added 2 commits October 7, 2021 17:08

Use Linking.getInitialURL instead of Navigation.getCurrentURL

da3ea68

Remove console log

b4c0f71

roryabraham marked this pull request as ready for review October 8, 2021 00:16

roryabraham requested a review from a team as a code owner October 8, 2021 00:16

MelvinBot requested review from mountiny and removed request for a team October 8, 2021 00:16

roryabraham requested a review from marcaaron October 8, 2021 00:16

marcaaron reviewed Oct 8, 2021

View reviewed changes

src/libs/actions/Policy.js Outdated Show resolved Hide resolved

src/pages/LogInWithShortLivedTokenPage.js Show resolved Hide resolved

src/pages/LogInWithShortLivedTokenPage.js Outdated Show resolved Hide resolved

src/libs/actions/Policy.js Outdated Show resolved Hide resolved

Cleanup per review comments

8327635

roryabraham requested a review from marcaaron October 8, 2021 00:50

marcaaron reviewed Oct 8, 2021

View reviewed changes

mountiny reviewed Oct 8, 2021

View reviewed changes

roryabraham added 3 commits October 8, 2021 09:49

Safe-access policyID

cf625d1

Merge branch 'main' into Rory-FixLoadingSpinnerRaceCondition

7846d77

# Conflicts: # src/libs/actions/Policy.js

Load policy summaries first, then full policies

49878f1

Fix semicolon typo

3fec339

roryabraham requested review from marcaaron and mountiny October 8, 2021 17:24

marcaaron approved these changes Oct 8, 2021

View reviewed changes

marcaaron merged commit 30ce47c into main Oct 8, 2021

marcaaron deleted the Rory-FixLoadingSpinnerRaceCondition branch October 8, 2021 18:14

AndrewGable mentioned this pull request Oct 14, 2021

🍒 Cherry pick PR #5747 to staging 🍒 #5845

Closed

github-actions bot pushed a commit that referenced this pull request Oct 14, 2021

Merge pull request #5706 from Expensify/Rory-FixLoadingSpinnerRaceCon…

7adf523

…dition Add shouldCreateNewPolicy param to getPolicySummaries action (cherry picked from commit 30ce47c)

This was referenced Oct 14, 2021

🍒 Cherry pick PR #5706 to staging 🍒 #5847

Merged

Deploy Checklist: New Expensify 2021-10-07 #5718

Closed

OSBotify mentioned this pull request Oct 15, 2021

Deploy Checklist: New Expensify 2021-10-15 #5879

Closed

neil-marcellini mentioned this pull request May 23, 2022

Create separate transition pages to sign the user in / out #8855

Merged

95 tasks

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shouldCreateNewPolicy param to getPolicySummaries action #5706

Add shouldCreateNewPolicy param to getPolicySummaries action #5706

roryabraham commented Oct 6, 2021 •

edited

Loading

marcaaron left a comment

roryabraham commented Oct 8, 2021

marcaaron Oct 8, 2021

mountiny left a comment

roryabraham commented Oct 8, 2021 •

edited

Loading

marcaaron left a comment

marcaaron Oct 8, 2021

marcaaron Oct 8, 2021

OSBotify commented Oct 8, 2021

OSBotify commented Oct 14, 2021

isagoico commented Oct 15, 2021

roryabraham commented Oct 15, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 23, 2021

Add shouldCreateNewPolicy param to getPolicySummaries action #5706

Add shouldCreateNewPolicy param to getPolicySummaries action #5706

Conversation

roryabraham commented Oct 6, 2021 • edited Loading

Details

LoginWithShortLivedTokenPage race condition explanation

LoginWithShortLivedTokenPage race condition fix

Policy load/creation race condition explanation

Policy load/creation race condition fix

Fixed Issues

Tests / QA Steps

Tested On

marcaaron left a comment

Choose a reason for hiding this comment

roryabraham commented Oct 8, 2021

marcaaron Oct 8, 2021

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

roryabraham commented Oct 8, 2021 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Oct 8, 2021

Choose a reason for hiding this comment

marcaaron Oct 8, 2021

Choose a reason for hiding this comment

OSBotify commented Oct 8, 2021

OSBotify commented Oct 14, 2021

isagoico commented Oct 15, 2021

roryabraham commented Oct 15, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 23, 2021

roryabraham commented Oct 6, 2021 •

edited

Loading

`LoginWithShortLivedTokenPage` race condition explanation

`LoginWithShortLivedTokenPage` race condition fix

roryabraham commented Oct 8, 2021 •

edited

Loading