Allow users to login with validate code and be redirected to Workspace/Card modal #4519

TomatoToaster · 2021-08-10T01:04:48Z

@mountiny, please review when you get the chance
CC: @timszot, @chiragsalian, @tgolen since you've all recently worked on this (no need to review, just in case you have strong opinions on what I changed here)

Details

Makes this "Login With Validate Code" generic so other routes (in this case, workspace/card) can also use the same component to do the login and redirect logic. Also updated some constant's names to be in line with this.

I've also introduced a areAllPoliciesLoaded onyx key. In the current freePlan beta, we only load policies after we have loaded the betas and can make sure the user is in the freePlan beta. This becomes an issue for logging in with a validateCode and then being redirected to workspace/card because that component would be loaded before all the user's policies have been loaded. This makes it seem like the policy we're looking for in src/pages/workspace/WorkspaceSidebar.js will almost never be loaded in time and will always create a new policy. To prevent this, we'll only create a new policy there after we're SURE all the policies a user has is already loaded.

Also as pointed out here in this comment "Validate Login" is kind of inaccurate, so I've renamed the components to LoginWithValidateCodePage and LoginWithValidateCode2FAPage accordingly.

HOLD on

Always auto name and create new workspaces from oldDot #4533

Fixed Issues

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

Tests

Web QA done locally. Make sure you are on the latest for Web-E

If you would like to test the beta properly remove the isDevelopment() || in this line:

App/src/libs/Permissions.js

Line 11 in b2d03e1

return isDevelopment() || _.contains(betas, CONST.BETAS.ALL);

This shows why the areAllPoliciesLoaded is relevant. The betas will have to be loaded from the api as ['all'] like it would be in production and and replicates the behavior where policies are loaded after the betas have been loaded.

QA Steps

With an account in the Free Plan beta, create a workspace in NewDot:
Log out of NewDot or open an incognito window and log into OldDot with the same account.
Go to Settings -> Policies and verify you see the Workspace there:
Click on that policy's name and verify that a new tab opens up in NewDot where you are logged in automatically and you are redirected to your workspace's settings page like this:
Go back to the OldDot Settings -> Policy tab and click it again. Verify another tab is opened to the same Workspace.
Test steps 1-5 again with an account that has 2FA enabled as well (make sure to log out of NewDot first!)
On the NewDot tab, look at the url for the Workspace/Card page. It should be something like staging.new.expensify.com/workspace/71648CB0DB905D52/card. Change the number (i.e. 71648CB0DB905D52) a little bit and go to that url. Verify that you see the error growl and a new policy is created.

Tested On

Screenshots

N/A Screenshots for web (only relevant platform) in issue.

…directing

…de-reuse

…ogin routes

…ing with validate code

…de-reuse

… loaded

mountiny · 2021-08-13T11:58:23Z

@TomatoToaster I am not sure what I am doing wrong here when testing this locally, but I have made sure to get up-to-date Web-E and run composer there. If I click on the newly created workspace name in OldDot I get to NewDot sign in page :/ I dont see anything relevant in console.

Is there not any other step I need to do for testing?

TomatoToaster · 2021-08-17T01:35:25Z

This is ready for another review @tgolen @mountiny. Let me know if the comments I've added to the rerouteToRelevantPage component are clear enough. I tried to incorporate "why are we doing this" in the comments, and it reads clear to me, but I have also been looking at this for a while 😅 .

Also I got rid of the are_all_policies_loaded onyx key and used the existing collection as suggested. I left a comment so that when the freeplan beta is done, that allPolicies !== null logic can be removed because there won't be any delayed loading of policies. I think we should eventually do that since it's kind of expensive to load all the policies for that component when we don't need them anymore. Let me know if you think that should be altered or maybe I should do something else.

mountiny · 2021-08-17T14:22:44Z

src/pages/workspace/WorkspaceSidebar.js

-    }, [policy]);
+    // After all the policies have loaded, we can know if the given policyID points to a nonexistant workspace
+    // When free plan is out of beta and Permissions.canUseFreePlan() gets removed,
+    // all code involving 'allPolicies' can be removed since policy loading will no longer be delayed on login.


NAB: I feel like it would be nice to use dome keyword like TODO: in places where we know for certain we will make a change in future to maybe keep track of it and find it more easily in future. WDYT?

Of course, if we would decide to use some keyword, then it will be properly useful only if we use it in all the places like this. Let me know what you think about this?

I like TODO because if we miss it, someone will definitely notice it's off when they see that comment still there.

Yes, let's add a // @TODO (this is our standard format) and then also create a GH issue to do the removal and make sure it happens.

I created a GH issue and also made a note to reexamine if the two components LoginWith can be consolidated. I think they can be and it'll be good to reexamine when this is out of beta.

Issue here: https://github.com/Expensify/Expensify/issues/174109

I'm not sure if everyone still agrees with this but we've historically discouraged the use of TODO comments and just use GH issues. TODO comments tend to live forever in the code, and pollute the code as they become outdated, or because there's usually no timeline for when they need to be done, etc.

Is there anything that the GH issue doesn't solve, that we need the TODO comment for?

Wow, interesting, did not realize this at all. If there is such an experience with using it, it might good decision to just omit it this time and we will make the change needed once the time comes.

Ah I wish github would bump down these comments so they're easier to see. Since we have the issue created I don't feel strongly about keepign it as as TODO. I'll remove it.

…remove this

mountiny

Great job, Amal!

tgolen

Thanks for the changes! I think that helps. I'm still a little confused on the race condition with the betas loaded, so maybe my comments about checking that the betas have been loaded is too paranoid.

tgolen · 2021-08-18T15:02:10Z

src/pages/LoginWithValidateCode2FAPage.js

-            } else {
+    componentDidUpdate() {
+        // Betas can be loaded a little after a user is authenticated, so check again if the betas have been updated
+        if (this.props.session.authToken && Permissions.canUseFreePlan(this.props.betas)) {


Should this also check that this.props.betas exists before calling canUseFreePlan()?

It will always exist because of the default props of this component having it set to []. We could check if it's empty, but Permissions.canUseFreePlan doesn't need the argument to be a non-empty array so I feel like that check might be redundant.

Here is a better description of why I thought this could have been a problem:

Component mounts

Component is updated with a session auth token, but betas haven't loaded yet

Permission check happens with an empty beta list

Betas finish loading

Now the beta check from 3 was stale and wrong because of the race condition

It's possible this isn't actually a problem because the first permission check would always return false if the beta array is empty, and there would be no redirect. Then, once the betas finish loading, there would be another component update with the proper beta data and the permission check would run and return true and then the redirect happens.

I think we are OK? It's quite the "gotcha" though and relies on the first permission check returning false with an empty beta array. If someone were to ever implement a permission check that did opposite logic (like cannotUseFreePlan, then the race condition would happen).

I think we are ok! And ahh yeah I haven't seen the cannotUse type of beta checks, so I didn't consider them. Your reasoning around the beta checks is right and the component loads endlessly Until we CAN prove the user is in the beta. Hypothetically a user who isn't in any betas who landed on this page would see an endless loader, but such a user shouldn't be able to access the link that brings them here in the first place.

tgolen · 2021-08-18T15:03:48Z

src/pages/LoginWithValidateCode2FAPage.js

+                break;
+
+            case SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE:
+                // Creating a policy will reroute the user to the settings page afterwards
                create();


Seeing this in the surrounding context, it's hard to know what this does. I would suggest changing the import to be import * as Policy from.... then change this reference to Policy.create() so that it helps add context about what's getting created without having to check the imports.

tgolen · 2021-08-18T15:05:00Z

src/pages/LoginWithValidateCodePage.js

+    componentDidMount() {
+        // If the user has an active session already, they need to be redirected straight to the relevant page
+        if (this.props.session.authToken) {
+            // In order to navigate to a modal, we first have to dismiss the current modal. But there is no current


NAB At some point, I think we need to DRY up these two components since they share about 90% of the same code.

tgolen · 2021-08-18T15:05:27Z

src/pages/LoginWithValidateCodePage.js

+            // and 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();
+            if (Permissions.canUseFreePlan(this.props.betas)) {


Same here, do we need to check that betas exist first before checking permissions?

tgolen · 2021-08-18T15:05:43Z

src/pages/LoginWithValidateCodePage.js

+
+    componentDidUpdate() {
+        // Betas can be loaded a little after a user is authenticated, so check again if the betas have been updated
+        if (this.props.session.authToken && Permissions.canUseFreePlan(this.props.betas)) {


src/pages/workspace/WorkspaceSidebar.js

TomatoToaster · 2021-08-18T15:59:56Z

@tgolen, addressed some comments and I think this is ready for another look!

TomatoToaster · 2021-08-19T15:37:15Z

TODO has been removed since the issue has been already created to take care of this later.

tgolen

Just this one little thing and we're good to merge.

src/pages/workspace/WorkspaceSidebar.js

tgolen · 2021-08-19T15:42:48Z

src/pages/LoginWithValidateCode2FAPage.js

-            } else {
+    componentDidUpdate() {
+        // Betas can be loaded a little after a user is authenticated, so check again if the betas have been updated
+        if (this.props.session.authToken && Permissions.canUseFreePlan(this.props.betas)) {


Here is a better description of why I thought this could have been a problem:

Component mounts

Component is updated with a session auth token, but betas haven't loaded yet

Permission check happens with an empty beta list

Betas finish loading

Now the beta check from 3 was stale and wrong because of the race condition

It's possible this isn't actually a problem because the first permission check would always return false if the beta array is empty, and there would be no redirect. Then, once the betas finish loading, there would be another component update with the proper beta data and the permission check would run and return true and then the redirect happens.

I think we are OK? It's quite the "gotcha" though and relies on the first permission check returning false with an empty beta array. If someone were to ever implement a permission check that did opposite logic (like cannotUseFreePlan, then the race condition would happen).

TomatoToaster · 2021-08-19T18:17:47Z

I think I got the changes you requested (just in the time you were doing the review!) @tgolen, could you take another look?

OSBotify · 2021-08-20T14:06:52Z

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

OSBotify · 2021-08-30T17:38:36Z

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

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

Amal Nazeem added 2 commits August 9, 2021 12:59

Add new routes

c64aeff

Add workspace_card route to be available when validating login and re…

d5b58c1

…directing

TomatoToaster self-assigned this Aug 10, 2021

TomatoToaster changed the title ~~Amal validate code reuse~~ Allow users to login with validate code and be redirected to workspace_card page Aug 10, 2021

TomatoToaster changed the title ~~Allow users to login with validate code and be redirected to workspace_card page~~ Allow users to login with validate code and be redirected to Workspace/Card modal Aug 10, 2021

Amal Nazeem added 6 commits August 9, 2021 21:13

remove console statements and add missing import

38d8a24

Merge branch 'main' of github.com:Expensify/App into amal-validate-co…

11145dd

…de-reuse

Update uses to remove VALIDATE_ since we aren't validating logins

ee2e220

Rename to redirectAfterSignIn so flag is generic to all validateCodeL…

0bec935

…ogin routes

Rename component and constants to match generic functionality of logg…

6d20074

…ing with validate code

Rename files to fit new generic functionality

7ca2bb3

TomatoToaster changed the title ~~Allow users to login with validate code and be redirected to Workspace/Card modal~~ [HOLD] Allow users to login with validate code and be redirected to Workspace/Card modal Aug 10, 2021

TomatoToaster changed the title ~~[HOLD] Allow users to login with validate code and be redirected to Workspace/Card modal~~ Allow users to login with validate code and be redirected to Workspace/Card modal Aug 11, 2021

Amal Nazeem added 7 commits August 11, 2021 14:32

Merge branch 'main' of github.com:Expensify/App into amal-validate-co…

e395f45

…de-reuse

Remove uneeded line

b4e635e

Clearer comment

f47c728

Fix missing parentheses

1b0607b

Merge branch 'main' of github.com:Expensify/App into amal-validate-co…

1c330be

…de-reuse

Merge branch 'main' of github.com:Expensify/App into amal-validate-co…

5ca0808

…de-reuse

Remove unused public workspace route

4b5c433

TomatoToaster force-pushed the amal-validate-code-reuse branch from 79d5e77 to 4b5c433 Compare August 12, 2021 22:18

Amal Nazeem added 3 commits August 12, 2021 18:26

rename to setRedirectAfterSignIn

223c4be

consolidated beta loading logic in LoginWith components

ed2a67e

Make sure new policy is autocreated only after all policies have been…

f0ffacf

… loaded

TomatoToaster marked this pull request as ready for review August 13, 2021 02:33

TomatoToaster requested a review from a team as a code owner August 13, 2021 02:33

MelvinBot requested review from mountiny and removed request for a team August 13, 2021 02:33

remove unused proptype

8115017

Amal Nazeem added 2 commits August 16, 2021 21:21

Use existing policies collection to determine if policies have loaded

96b4a4a

Use screens relevant to validateCode not 2fa

cc0c2b8

TomatoToaster changed the title ~~[WIP]Allow users to login with validate code and be redirected to Workspace/Card modal~~ Allow users to login with validate code and be redirected to Workspace/Card modal Aug 17, 2021

Amal Nazeem added 2 commits August 16, 2021 21:29

Remove unused Onyx key

fac742e

Clarify when this logic can be removed

fcbeadf

mountiny self-requested a review August 17, 2021 10:11

mountiny previously approved these changes Aug 17, 2021

View reviewed changes

Add todo to comment so it's clearer theres some action to be done in …

b40cabf

…remove this

TomatoToaster dismissed mountiny’s stale review via b40cabf August 17, 2021 18:32

mountiny previously approved these changes Aug 17, 2021

View reviewed changes

tgolen requested changes Aug 18, 2021

View reviewed changes

Amal Nazeem added 3 commits August 18, 2021 11:45

Add missing doc

20ee86a

Use Policy to make code more contextual

23798c7

Use @

b4d779c

TomatoToaster dismissed mountiny’s stale review via b4d779c August 18, 2021 15:59

remove @todo since workspace has already been created

3e6fae6

mountiny self-requested a review August 19, 2021 15:43

tgolen requested changes Aug 19, 2021

View reviewed changes

mountiny approved these changes Aug 19, 2021

View reviewed changes

tgolen approved these changes Aug 20, 2021

View reviewed changes

tgolen merged commit cefb82a into main Aug 20, 2021

tgolen deleted the amal-validate-code-reuse branch August 20, 2021 14:06

This was referenced Aug 20, 2021

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

Deploy Checklist: New Expensify 2021-08-25 #4828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to login with validate code and be redirected to Workspace/Card modal #4519

Allow users to login with validate code and be redirected to Workspace/Card modal #4519

TomatoToaster commented Aug 10, 2021 •

edited

Loading

mountiny commented Aug 13, 2021

TomatoToaster commented Aug 17, 2021

mountiny Aug 17, 2021

TomatoToaster Aug 17, 2021 •

edited

Loading

tgolen Aug 18, 2021

TomatoToaster Aug 18, 2021 •

edited

Loading

cead22 Aug 18, 2021

cead22 Aug 18, 2021

mountiny Aug 19, 2021

TomatoToaster Aug 19, 2021

mountiny left a comment

tgolen left a comment

tgolen Aug 18, 2021

TomatoToaster Aug 18, 2021

tgolen Aug 19, 2021

TomatoToaster Aug 19, 2021

tgolen Aug 18, 2021

tgolen Aug 18, 2021

tgolen Aug 18, 2021

tgolen Aug 18, 2021

TomatoToaster commented Aug 18, 2021

TomatoToaster commented Aug 19, 2021

tgolen left a comment

tgolen Aug 19, 2021

TomatoToaster commented Aug 19, 2021

OSBotify commented Aug 20, 2021

OSBotify commented Aug 30, 2021

Allow users to login with validate code and be redirected to Workspace/Card modal #4519

Allow users to login with validate code and be redirected to Workspace/Card modal #4519

Conversation

TomatoToaster commented Aug 10, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

mountiny commented Aug 13, 2021

TomatoToaster commented Aug 17, 2021

Choose a reason for hiding this comment

TomatoToaster Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomatoToaster Aug 18, 2021 • edited Loading

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

mountiny left a comment

Choose a reason for hiding this comment

tgolen 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

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

TomatoToaster commented Aug 18, 2021

TomatoToaster commented Aug 19, 2021

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomatoToaster commented Aug 19, 2021

OSBotify commented Aug 20, 2021

OSBotify commented Aug 30, 2021

TomatoToaster commented Aug 10, 2021 •

edited

Loading

TomatoToaster Aug 17, 2021 •

edited

Loading

TomatoToaster Aug 18, 2021 •

edited

Loading