Check authToken and always redirect somewhere on init #239

marcaaron · 2020-08-19T01:15:14Z

Fixes #229
Fixes #235

cc @tgolen @AndrewGable in case you have a better idea about how to do this... but here's my current reasoning...

Presently, when the app inits we always navigate to the homepage by default because there is little to stop up from navigating there and redirectTo is always undefined on the very first render (technically all state is undefined on the first render).

Since the presence of an authToken should tell us if we are logged in or not I enforced that when the app inits (or we are redirecting to '/' - which we don't ever do) then we will check for the presence of an authToken and redirect to the root if we have one. If we don't have one then we'll redirect to the /signin. And when we have a redirectTo then we'll just use that.

Lmk if I am holding this completely wrong here...

Tests:

Start out logged out with no authToken
Navigate to the site root by entering the url directly into the browser http://localhost:8082/
Verify the homepage does not momentarily flash
Verify that you are able to log in and get redirected to the homepage
Enter the site root into the url browser bar again
Verify that you are still logged in
Log out
Verify you are back on the sign in page

bonus: post-prototype thoughts (for my reference, but feel free to read on)

Would love to talk about possibly leaning on React Router more for all of our navigation needs and making site navigation something that Ion doesn't really need much awareness of. There's the HOC withRouter that can be used just about anywhere to find out where we are or redirect us to someplace else.

AndrewGable

This tests well for me across clients 👍

Jag96 · 2020-08-19T01:52:36Z

src/Expensify.js


        this.recordCurrentRoute = this.recordCurrentRoute.bind(this);
+
+        this.state = {};


Why is this necessary?

When this component first renders it has an undefined state. After withIon mounts (i.e. post render cycle of the Expensify component) only then do we look at the prefillWithKey and set it to this components state. Before that it had no state at all.

Which is why we needed to check for this.state && this.state.redirectTo before. If we know this is always at least going to be {} then we can safely access this.state.redirectTo.

Jag96 · 2020-08-19T01:54:20Z

src/Expensify.js

    }

    render() {
+        const redirectTo = this.state.redirectTo || (!this.state.authToken && '/signin') || '/';


I think if we are going to add this, you should add a comment with some of the info you added in the description to explain why this is necessary

Yep that's a good call.

Jag96 · 2020-08-19T02:08:54Z

Looks like this PR also fixes #235, I tested by:

Merging master into this branch
Deleting the Chat app on my Android emu
Running the app on this branch
Confirming I can sign in (on master you can't)

Jag96 · 2020-08-19T02:12:31Z

Can't reproduce #230 either on this branch 🎉

AndrewGable · 2020-08-19T02:26:30Z

Ok one minor thing I see on this PR, if you are on /36 and refresh, it takes you to / then to a different report (e.g. 17). I don't think it's a blocker, but just a small regression from master

AndrewGable · 2020-08-19T02:53:07Z

This bug also happens if you click sign out on /36, you sign back into /17, I believe the expected behavior there is to go back to /36 as the URL is set to /signin/exitTo/36

This was added in #155 and #179

marcaaron · 2020-08-19T03:18:03Z

Updated. And the bug you found should be fixed now 🤞

AndrewGable

Fixed all little buggies I saw on the previous commits 👍

Jag96 · 2020-08-19T03:37:38Z

I'm still seeing #239 (comment) on this branch, if I signout while on a reportID and sign in again, I'm taken to the first reportID in the list instead of the one in the exitTo. Can you confirm?

AndrewGable · 2020-08-19T03:53:48Z

After double checking, I just realized that the exitTo bug is also happening on master, meaning I think we can ignore it for this PR. Thoughts?

cead22 · 2020-08-19T05:23:51Z

src/Expensify.js

@@ -1,4 +1,6 @@
 import React, {Component} from 'react';
+import {View} from 'react-native';
+import get from 'lodash.get';


This was imported somewhere else as lodashGet, maybe we should try to import these using a standard naming convention, for easy grepping

I want to propose something even crazier...

Let's just ditch underscore already and use lodash for everything.
What does underscore have that lodash doesn't?

https://lodash.com/docs/4.17.15#map
https://lodash.com/docs/4.17.15#chain
https://lodash.com/docs/4.17.15#forEach
https://lodash.com/docs/4.17.15#size
https://lodash.com/docs/4.17.15#find
https://lodash.com/docs/4.17.15#uniqueId

the list goes on and on...

I mean heck it even has _.template (but where we're going we don't need templates)

I've definitely thought of that in the past as well, and I keep coming back to this:

There is no problem using underscore

Lodash handles some functionality slightly differently than underscore, and if an engineer isn't aware of that, it can lead to difficult to find bugs (this was more of a historical issue and it's possible the APIs are more aligned today than they have been in the past)

Some underscore functionality we use doesn't exist in lodash (eg. findWhere() indexBy() pluck())

Lodash methods can be imported individually for functionality that underscore doesn't have (but the vice versa is not true)

Lodash is 24kb vs underscore 6kb and includes a lot of stuff that would never be used

Ok, it was worth a shot 😛

cead22 · 2020-08-19T05:25:08Z

src/lib/Network.js

                        .then(redirectToSignIn);
                }
-                return setSuccessfulSignInData(response, command.exitTo);
+                return setSuccessfulSignInData(response, data.exitTo);


I wonder how much this tripped me up

tgolen

Would love to talk about possibly leaning on React Router more for all of our navigation needs

Yes, I totally agree and would love to explore possible solutions and paths for this. Can you make a GH to discuss? One thing I find difficult about Router is that its primary use is with UI components, and that makes it hard to interact with it inside of just a normal JS lib. Hopefully we can figure out some good solutions for that.

tgolen · 2020-08-19T14:24:07Z

src/Expensify.js

+        // redirect to the signin page if we do not have one. Otherwise when the app inits
+        // we will fall through to the homepage and the user will see a brief flash of the main
+        // app experience.
+        Ion.get(IONKEYS.SESSION)


This can be simplified a little bit to pass back just the authToken:

Ion.get(IONKEYS.SESSION, 'authToken', false)

Check authToken and always redirect somewhere on init

6de7009

marcaaron self-assigned this Aug 19, 2020

marcaaron requested a review from AndrewGable August 19, 2020 01:15

AndrewGable approved these changes Aug 19, 2020

View reviewed changes

Jag96 reviewed Aug 19, 2020

View reviewed changes

add comment

d379717

marcaaron added 2 commits August 18, 2020 20:16

add a generic loader to provide time to load the authToken

7541f7e

woops

f4017c9

undo these weird changes

a770a75

AndrewGable approved these changes Aug 19, 2020

View reviewed changes

Jag96 approved these changes Aug 19, 2020

View reviewed changes

fix exitTo

9da16d5

AndrewGable merged commit 3e376de into master Aug 19, 2020

AndrewGable deleted the marcaaron-hamflash branch August 19, 2020 04:08

This was referenced Aug 19, 2020

First login on mobile web fails, need to retry after refresh #230

Closed

[HOLD] Re-authenticate only if we have stored credentials #232

Closed

cead22 reviewed Aug 19, 2020

View reviewed changes

tgolen reviewed Aug 19, 2020

View reviewed changes

marcaaron mentioned this pull request Aug 20, 2020

Follow up comments. Standardize lodash imports. #263

Merged

snyk-bot mentioned this pull request May 14, 2021

[Snyk] Upgrade @react-native-picker/picker from 1.9.11 to 1.15.0 #2940

Closed

snyk-bot mentioned this pull request Sep 6, 2021

[Snyk] Upgrade @react-native-picker/picker from 1.9.11 to 1.16.5 #5110

Closed


		this.recordCurrentRoute = this.recordCurrentRoute.bind(this);

		this.state = {};

Check authToken and always redirect somewhere on init #239

Check authToken and always redirect somewhere on init #239

Uh oh!

Conversation

marcaaron commented Aug 19, 2020 • edited by Jag96 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewGable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jag96 commented Aug 19, 2020

Uh oh!

Jag96 commented Aug 19, 2020

Uh oh!

AndrewGable commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewGable commented Aug 19, 2020

Uh oh!

marcaaron commented Aug 19, 2020

Uh oh!

AndrewGable left a comment

Choose a reason for hiding this comment

Uh oh!

Jag96 commented Aug 19, 2020

Uh oh!

AndrewGable commented Aug 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marcaaron commented Aug 19, 2020 •

edited by Jag96

Loading

AndrewGable commented Aug 19, 2020 •

edited

Loading