Beta race fixes #4205

chiragsalian · 2021-07-23T23:11:37Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/171345

Setup

We'll want this to behave just like stag/prod so there are some setup steps.
Remove isDevelopment from here. That line returns true really fast with isDevelopment so you can't test race conditions.
Go to https://www.expensify.com.dev/_support/beta and add a test account to freePlan.
Delete this block.
If you really want to stretch the race condition update this line with,

setTimeout(() => Onyx.set(ONYXKEYS.BETAS, response.betas), 5000);

which is what I did while working on the code.

Tests

Testing workspace/new

Go to Expensify App. While signed out visit http://localhost:8080/workspace/new.
Enter your credentials.
Once logged in confirm you see the workplace modal

Testing pricing redirect.

Logout of the Expensify App.
Go to expensify.com.dev. Log into your test account then go to https://www.expensify.com.dev/pricing
Select Free and confirm you see the workplace modal again

QA Steps

Same steps as above except that you'll need to use an email that's on free plan beta.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Beamanator · 2021-07-26T13:16:33Z

Testing steps for "Testing workspace/new" worked well 👍

~~While doing "Testing pricing redirect.", step 3 "Select Free and confirm you see the workplace modal again" I got this: [redacted image]~~

Rebuilt web-e w/ grunt and works well! 👍

Beamanator

A few comments, nothing life-changing. Overall looks great 👍

src/Expensify.js

src/pages/ValidateLoginNewWorkspacePage.js

Beamanator · 2021-07-26T13:40:50Z

src/pages/ValidateLoginNewWorkspacePage.js

@@ -39,7 +43,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 (this.props.betas) {


Similar to comment in other file - how do you feel about using lodashGet here (like below)?

No reason to use lodashGet here imo since the proptype and default are provided. The other example of lodashGet makes sense because accountID and validateCode are not defined in the defaultParams.

True, but I did mention how session.authToken and session.accountID are in defaultProps and defaulted via lodashGet in the other component. But still I don't mind not using lodashGet here since this.props.betas is not nested in another onyx key 👍

Yeah i'm not going to add it atm since I don't see its value.

alex-mechler

Code LGTM, but I see a comment about potentially changing this up in the issue, so holding off on an approve

TomatoToaster

I think this PR is necessary and I left a comment in the original issue here why I think so.

Also had some suggestions for this PR lmk what you think.

src/Expensify.js

TomatoToaster · 2021-07-26T18:19:00Z

src/pages/ValidateLogin2FANewWorkspacePage.js

@@ -62,7 +66,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 (this.props.betas) {


If we are saving this behavior for the beta, why wouldn't we use

Suggested change

if (this.props.betas) {

if (Permissions.canUseFreePlan(this.props.betas)) {

And similarily for other parts referencing this.props.betas

Otherwise, aren't we saying this can be used by anyone? When betas load they'll be [] for those who aren't in any beta so the else part below here wouldn't run.

Well ROUTES.WORKSPACE_NEW already does the check for Permissions.canUseFreePlan so to keep the code DRY i think its best that the check for canUseFreePlan happens only in NewWorkSpace.js

Otherwise, aren't we saying this can be used by anyone? When betas load they'll be [] for those who aren't in any beta so the else part below here wouldn't run.

I didn't follow your question. Maybe this helps. The if condition determines if newWorkSpace component should render now or later. This take care of race conditions. If we have loaded betas, even if its empty, then we call Navigation.navigate(ROUTES.WORKSPACE_NEW); to render NewWorkSpace since there is no reason to wait. If betas is null, that means we're still waiting and hence we call setRedirectToWorkspaceNewAfterSignIn(true) so that once the betas are loaded (even if its empty) we call Navigation.navigate(ROUTES.WORKSPACE_NEW); at a later time over here. Does that make sense?

Ahh yes that totally makes sense. Thanks for exposing that!

TomatoToaster · 2021-07-26T19:32:43Z

There's one line that's too long but this LGTM, don't block on me 👍 (aaaand you just got it lol)

chiragsalian · 2021-07-26T19:37:29Z

Updated code to address comments 👍 Ready for review.

Beamanator

LGTM!

src/pages/ValidateLoginNewWorkspacePage.js

OSBotify · 2021-07-27T09:43:05Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

tgolen · 2021-07-27T16:38:13Z

src/pages/ValidateLoginNewWorkspacePage.js

@@ -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);


@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?

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.

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.

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.

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

OSBotify · 2021-07-28T21:35:33Z

🚀 Deployed to staging in version: 1.0.80-3🚀

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

OSBotify · 2021-07-30T20:04:00Z

🚀 Deployed to production in version: 1.0.81-4🚀

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

kevinksullivan · 2021-08-02T12:53:15Z

@chiragsalian if this deployed to production, why hasn't it closed?

chiragsalian · 2021-08-03T21:48:10Z

oh good question, looks like some bug that didn't close the parent issue. I'll manually close the parent issue.

chiragsalian added 2 commits July 23, 2021 16:09

Beta race fixes

0b75a8b

revert test changes.

9318e5d

chiragsalian self-assigned this Jul 23, 2021

Merge branch 'main' into chirag-beta-race-fixes

8fda0e3

chiragsalian requested review from Beamanator and tgolen July 23, 2021 23:39

chiragsalian marked this pull request as ready for review July 23, 2021 23:40

chiragsalian requested a review from a team as a code owner July 23, 2021 23:40

MelvinBot requested review from alex-mechler and removed request for a team July 23, 2021 23:40

Beamanator reviewed Jul 26, 2021

View reviewed changes

alex-mechler reviewed Jul 26, 2021

View reviewed changes

TomatoToaster suggested changes Jul 26, 2021

View reviewed changes

chiragsalian added 2 commits July 26, 2021 12:27

setting betas as empty array and using lodashGet

0b6b408

warning cleanup

ce7b7e7

TomatoToaster approved these changes Jul 26, 2021

View reviewed changes

alex-mechler approved these changes Jul 26, 2021

View reviewed changes

Beamanator approved these changes Jul 27, 2021

View reviewed changes

src/pages/ValidateLoginNewWorkspacePage.js Show resolved Hide resolved

Beamanator merged commit 30242db into main Jul 27, 2021

Beamanator deleted the chirag-beta-race-fixes branch July 27, 2021 09:42

chiragsalian mentioned this pull request Jul 27, 2021

Style fix #4257

Merged

5 tasks

tgolen reviewed Jul 27, 2021

View reviewed changes

OSBotify mentioned this pull request Jul 28, 2021

Deploy Checklist: Expensify.cash 2021-07-28 #4279

Closed

71 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beta race fixes #4205

Beta race fixes #4205

chiragsalian commented Jul 23, 2021 •

edited

Loading

Beamanator commented Jul 26, 2021 •

edited

Loading

Beamanator left a comment

Beamanator Jul 26, 2021

chiragsalian Jul 26, 2021

Beamanator Jul 26, 2021

chiragsalian Jul 26, 2021

alex-mechler left a comment

TomatoToaster left a comment

TomatoToaster Jul 26, 2021 •

edited

Loading

chiragsalian Jul 26, 2021

TomatoToaster Jul 26, 2021

TomatoToaster commented Jul 26, 2021 •

edited

Loading

chiragsalian commented Jul 26, 2021

Beamanator left a comment

OSBotify commented Jul 27, 2021

tgolen Jul 27, 2021

chiragsalian Jul 28, 2021

tgolen Jul 28, 2021

chiragsalian Jul 28, 2021

chiragsalian Jul 28, 2021 •

edited

Loading

OSBotify commented Jul 28, 2021

OSBotify commented Jul 30, 2021

kevinksullivan commented Aug 2, 2021

chiragsalian commented Aug 3, 2021

	if (this.props.betas) {
	if (Permissions.canUseFreePlan(this.props.betas)) {

Beta race fixes #4205

Beta race fixes #4205

Conversation

chiragsalian commented Jul 23, 2021 • edited Loading

Details

Fixed Issues

Setup

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Beamanator commented Jul 26, 2021 • edited Loading

Beamanator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-mechler left a comment

Choose a reason for hiding this comment

TomatoToaster left a comment

Choose a reason for hiding this comment

TomatoToaster Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomatoToaster commented Jul 26, 2021 • edited Loading

chiragsalian commented Jul 26, 2021

Beamanator left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragsalian Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

OSBotify commented Jul 28, 2021

OSBotify commented Jul 30, 2021

kevinksullivan commented Aug 2, 2021

chiragsalian commented Aug 3, 2021

chiragsalian commented Jul 23, 2021 •

edited

Loading

Beamanator commented Jul 26, 2021 •

edited

Loading

TomatoToaster Jul 26, 2021 •

edited

Loading

TomatoToaster commented Jul 26, 2021 •

edited

Loading

chiragsalian Jul 28, 2021 •

edited

Loading