Allow e.com to link straight to e.cash and share sessions #3715

tgolen · 2021-06-21T23:47:51Z

Fixed Issues

Done along with: https://github.com/Expensify/Web-Expensify/pull/31302
Fixes https://github.com/Expensify/Expensify/issues/162554

Tests / QA

Be sure you're signed out of e.cash
Sign into e.com with a brand new account on a domain WITHOUT two-factor authentication
Complete the inbox task "What will you use Expensify for? Choose as many as you'd like!" and ONLY choose the expensify card
Verify that you are automatically logged into e.cash and are seeing the page to create a new workspace

Now, test it out with 2FA

Be sure you're signed out of e.cash
Sign into e.com with a brand new account on a domain WITH two-factor authentication enabled
Complete the inbox task "What will you use Expensify for? Choose as many as you'd like!" and ONLY choose the expensify card
Verify that you are redirected to e.cash to ever a 2FA code
After the code is entered, verify you are automatically logged into e.cash and are seeing the page to create a new workspace

Tested On

No other platforms need to be tested because this flow only works on web (the inbox task isn't shown on mobile at all)

Screenshots

Web

cc @marcaaron @TomatoToaster

tgolen · 2021-07-02T18:14:18Z

This is all working, but the redirect to the new-workspace page is causing several JS console warnings:

They don't appear to be affecting anything, but I haven't been able to figure out what the deal is. My guess is that it's related to running these back-to-back:

            Navigation.dismissModal();
            Navigation.navigate(ROUTES.WORKSPACE_NEW);

and I think there is a race condition because I think part of the code inside dismissModal() is async, but it's hard to figure out when exactly dismissModal() is finished doing its thing. There is one existing flow that uses this same dismiss->navigate pattern here: https://github.com/Expensify/Expensify.cash/blob/099457ec41f238be8103d953e4c8b50ea2b030f1/src/libs/actions/Policy.js#L201-L202

I'm not sure what to do about this. We need to have this code deployed by Jul 9, so I am thinking that we merge this as-is, and then open up an issue for improving this dismiss->navigate pattern.

marcaaron

These changes LGTM! I'm not sure whether there is any problem with the dismiss + navigate pattern (it seems kosher to me). But I agree that the react warning is not a blocker. Could be an issue is withOnyx() not having any awareness that its children have unmounted - I've observed similar issues before, but hard to say without debugging.

marcaaron · 2021-07-02T18:40:57Z

src/pages/ValidateLogin2FANewWorkspacePage.js

+        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
+            // modal you say? I know, it confuses me too. Without dismissing the current modal, if they user cancels
+            // out of the new workspace modal, then they will be routed back to


But there is no current modal you say? I know, it confuses me too.

NAB, I can see how this could be confusing (because the modal is not visible), but I think maybe it's not that mysterious. Curious if we should replace this commentary with a better explanation and have an idea about what's happening here...

ValidateLogin2FANewWorkspacePage is the modal we are dismissing, it "exists", but the page is returning null. So that's why we need to remove it from the history stack or else when we "goBack()" we will navigate to it.

Yeah, I'm all in favor of updating this comment to make more sense. If the real problem is that ValidateLogin2FANewWorkspacePage is in the history stack, maybe there is a better way for us to remove it from the stack without needing to call dismissModal()?

maybe there is a better way for us to remove it from the stack without needing to call dismissModal()

I'm having trouble working out a better way. It feels like there are contexts where you'd want the default navigate() method to preserve the underlying modal in the stack e.g. moving from /settings to /settings/payments and contexts where you'll want a different behavior. We could maybe add a parameter to navigate() that forces all other modals to close?

All right, I was able to fix this. Turns out, I was navigating to that modal twice (once in Session, and again from ComponentDidMount (from when the same page was remounted: once in public screens and then again in auth screens).

Once I removed the navigation in the Session action, it all worked like a charm.

tgolen · 2021-07-02T20:03:43Z

I removed the hold because this actually needs to be deployed first, before the web-e change.

tgolen · 2021-07-06T18:23:58Z

All right, thanks for the reviews! I updated it and addressed everything as well as fixing the initial warnings that I was struggling with. Should be ready for another review.

marcaaron · 2021-07-06T18:26:29Z

src/libs/API.js

+ * @param {String} parameters.accountID
+ * @param {String} parameters.validateCode
+ * @param {String} [parameters.twoFactorAuthCode]
+ * @returns {Promise<unknown>}


NAB / just curious but why Promise<unknown>?

I think PHPStorm did that automatically, so I'm not sure why! I'll remove it

marcaaron · 2021-07-06T18:28:36Z

src/libs/Navigation/AppNavigator/AuthScreens.js

+    },
+    headerShown: false,
+    animationTypeForReplace: 'pop',
+};


Seems like defaultScreenOptions is also declared in PublicScreens.js with the same values. Maybe they should be moved into a separate config file so we don't have to remember to update things twice in the future when applying a change?

Sure, I can do that.

tgolen · 2021-07-06T20:29:32Z

Updated to DRY up the default screen options.

alex-mechler · 2021-07-06T22:43:47Z

I'm trying to test this out, and Web-E redirects me to the correct link, but then I am redirect to the homepage without any error messages. Is there any additional setup I have to do that is not mentioned in the testing steps?

tgolen · 2021-07-07T19:26:51Z

@alex-mechler I don't think there is anything special you need to do, no.

Just so I am clear, the test steps work for except that instead of seeing the new-workspace page, you see the home page?

alex-mechler · 2021-07-07T19:37:06Z

Correct. I am not automatically logged into e.cash, and do not see the new workspace page. I checked out the branch from https://github.com/Expensify/Web-Expensify/pull/31302 and this branch. Is there any other branches I need to checkout? Was there any auth changes that I might be missing?

alex-mechler

Turns out my auth was out of date 😅

Works well, two NABs, feel free to merge if you don't think they are worth fixing

alex-mechler · 2021-07-07T21:38:32Z

src/languages/en.js

@@ -268,6 +268,7 @@ export default {
    },
    passwordForm: {
        pleaseFillOutAllFields: 'Please fill out all fields',
+        enterATwoFactorAuthenticationCodeToContinue: 'Enter a two factor authentication code to continue',


NAB: I feel like your reads better than a here

OK, I can change this.

alex-mechler · 2021-07-07T21:40:00Z

src/pages/ValidateLogin2FANewWorkspacePage.js

+                            success
+                            style={[styles.mb2]}
+                            text={this.props.translate('common.continue')}
+                            isLoading={this.state.loading}


NAB: We never set this anywhere but the constructor, meaning the user has no feedback as to when we are actually loading

I'll see about cleaning this up.

tgolen · 2021-07-07T21:53:14Z

OK, I updated to change the language on the form prompt and to display the loading indicator when the 2FA is being validated.

tgolen · 2021-07-08T14:08:39Z

I'm gonna go ahead and merge since the other reviewers approved already and the changes since then were minor.

tgolen · 2021-07-08T14:09:10Z

Once this goes to production, we can merge Expensify/Web-Expensify#31302

OSBotify · 2021-07-08T14:09: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.

kavimuru · 2021-07-11T03:30:56Z

User is not automatically logged into e.cash after completing the inbox task

Expected Result:

Scenario 1: Verify that you are automatically logged into e.cash and are seeing the page to create a new workspace
Scenario 2: Verify that you are redirected to e.cash to ever a 2FA code

Actual Result:

Not automatically logged into e.cash and the page to create a new workspace isn't shown. User is redirected to sign in page for both the scenarios

Actions Performed:

Scenario1:

Be sure you're signed out of e.cash
Sign into e.com with a brand new account on a domain WITHOUT two-factor authentication
Complete the inbox task "What will you use Expensify for? Choose as many as you'd like!" and ONLY choose the expensify card

Scenario2:

Be sure you're signed out of e.cash
Sign into e.com with a brand new account on a domain WITH two-factor authentication enabled
Complete the inbox task "What will you use Expensify for? Choose as many as you'd like!" and ONLY choose the expensify card

Platform:

Web ✔️
iOS
mWeb
Android

Build:

1.0.77-0

Notes/Images/Video:

tgolen · 2021-07-12T15:40:56Z

Ah, I think that's expected for now. Sorry, this issue should be production QA and internal QA (because it requires being in a beta for it to work). It also needs to have https://github.com/Expensify/Web-Expensify/pull/31302 deployed, and it looks like that still hasn't been.

isagoico · 2021-07-12T20:39:34Z

Perfect! will check the PR off the list then. Thanks 🙇

OSBotify · 2021-07-14T19:33:08Z

🚀 Deployed to production in version: 1.0.77-5🚀

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

TomatoToaster · 2021-07-15T18:08:59Z

ITS WORKING!!
I got redirected from expensify after choosing just expensify card on the business inbox task.

tgolen · 2021-07-15T19:28:27Z

Yeah! Noice

…

On Thu, Jul 15, 2021 at 12:09 PM Amal Nazeem ***@***.***> wrote: ITS WORKING!! I got redirected from expensify after gettting the new task [image: image] <https://user-images.githubusercontent.com/19364431/125834154-2c196589-a829-4269-8d6b-f7fcf87e99f6.png> — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#3715 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB2LEGKRHYBFGDHV7O3TX4P4NANCNFSM47CSENHA> .

tgolen added 10 commits June 21, 2021 13:25

Add new validation routes

e40fe94

Use a constant for the screen names

c6047a2

Add the route to public screens as well

27ddb14

Add a comment for the route

c4738e8

Move variable to a class property

90fc9dc

Create separate pages for the separate routes

9790dfd

WIP on redirecting to new workspace page

d8906d6

Get redirection to the new workspace modal working

fbafc61

Add a user action for getting an authToken

2819a23

Lint and show proper error message

a4d43e2

tgolen self-assigned this Jun 21, 2021

tgolen added 7 commits June 22, 2021 09:16

Rename action method

6eac5bb

Move method to Session action and implement API method

09bded2

Pass the 2FA code to action method

a0bb25d

Remove unneeded data from request

af4cfeb

Merge branch 'main' into tgolen-validate-redirect

d32f02c

Improve comment, redirect in the action file

3880ba8

Merge branch 'main' into tgolen-validate-redirect

0d5d4f3

tgolen changed the title ~~Allow e.com to link straight to e.cash and share sessions~~ [HOLD Web-E #31302] Allow e.com to link straight to e.cash and share sessions Jul 2, 2021

tgolen requested a review from a team July 2, 2021 18:07

MelvinBot requested review from Dal-Papa and removed request for a team July 2, 2021 18:07

tgolen marked this pull request as ready for review July 2, 2021 18:14

tgolen requested a review from a team as a code owner July 2, 2021 18:14

MelvinBot requested review from alex-mechler and removed request for a team July 2, 2021 18:14

marcaaron reviewed Jul 2, 2021

View reviewed changes

tgolen changed the title ~~[HOLD Web-E #31302] Allow e.com to link straight to e.cash and share sessions~~ Allow e.com to link straight to e.cash and share sessions Jul 2, 2021

Upper case API method.

636ad0b

marcaaron reviewed Jul 6, 2021

View reviewed changes

Make default screen options into a reusable module

134564e

Dal-Papa previously approved these changes Jul 7, 2021

View reviewed changes

alex-mechler previously approved these changes Jul 7, 2021

View reviewed changes

tgolen added 2 commits July 7, 2021 15:49

Reword form prompt

a94f7ab

Set the loading state to true

f9f3c61

tgolen dismissed stale reviews from alex-mechler and Dal-Papa via f9f3c61 July 7, 2021 21:52

alex-mechler approved these changes Jul 7, 2021

View reviewed changes

tgolen merged commit 2bda7d6 into main Jul 8, 2021

tgolen deleted the tgolen-validate-redirect branch July 8, 2021 14:08

This was referenced Jul 8, 2021

Deploy Checklist: Expensify.cash 2021-07-07 #3905

Closed

Deploy Checklist: Expensify.cash 2021-07-09 #3958

Closed

tgolen added the InternalQA label Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow e.com to link straight to e.cash and share sessions #3715

Allow e.com to link straight to e.cash and share sessions #3715

tgolen commented Jun 21, 2021 •

edited

Loading

tgolen commented Jul 2, 2021

marcaaron left a comment

marcaaron Jul 2, 2021

tgolen Jul 2, 2021

marcaaron Jul 2, 2021

tgolen Jul 6, 2021

tgolen commented Jul 2, 2021

tgolen commented Jul 6, 2021

marcaaron Jul 6, 2021

tgolen Jul 6, 2021

marcaaron Jul 6, 2021

tgolen Jul 6, 2021

tgolen commented Jul 6, 2021

alex-mechler commented Jul 6, 2021

tgolen commented Jul 7, 2021

alex-mechler commented Jul 7, 2021

alex-mechler left a comment

alex-mechler Jul 7, 2021

tgolen Jul 7, 2021

alex-mechler Jul 7, 2021

tgolen Jul 7, 2021

tgolen commented Jul 7, 2021

tgolen commented Jul 8, 2021

tgolen commented Jul 8, 2021

OSBotify commented Jul 8, 2021

kavimuru commented Jul 11, 2021

tgolen commented Jul 12, 2021

isagoico commented Jul 12, 2021

OSBotify commented Jul 14, 2021

TomatoToaster commented Jul 15, 2021 •

edited

Loading

tgolen commented Jul 15, 2021 via email

Allow e.com to link straight to e.cash and share sessions #3715

Allow e.com to link straight to e.cash and share sessions #3715

Conversation

tgolen commented Jun 21, 2021 • edited Loading

Fixed Issues

Tests / QA

Tested On

Screenshots

Web

tgolen commented Jul 2, 2021

marcaaron 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

tgolen commented Jul 2, 2021

tgolen commented Jul 6, 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

tgolen commented Jul 6, 2021

alex-mechler commented Jul 6, 2021

tgolen commented Jul 7, 2021

alex-mechler commented Jul 7, 2021

alex-mechler 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

tgolen commented Jul 7, 2021

tgolen commented Jul 8, 2021

tgolen commented Jul 8, 2021

OSBotify commented Jul 8, 2021

kavimuru commented Jul 11, 2021

User is not automatically logged into e.cash after completing the inbox task

Expected Result:

Actual Result:

Actions Performed:

Platform:

Build:

Notes/Images/Video:

tgolen commented Jul 12, 2021

isagoico commented Jul 12, 2021

OSBotify commented Jul 14, 2021

TomatoToaster commented Jul 15, 2021 • edited Loading

tgolen commented Jul 15, 2021 via email

tgolen commented Jun 21, 2021 •

edited

Loading

TomatoToaster commented Jul 15, 2021 •

edited

Loading