Fix various post sign out requests + bugs by marcaaron · Pull Request #1075 · Expensify/App

marcaaron · 2020-12-23T16:50:29Z

@Julesssss @tgolen

Details

There are a few login bugs that I noticed while testing the sign in / sign out flows and I'm addressing them all here.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/148468

Tests

Log in
Quickly log out
Verify you can safely log in again and there are no endless requests being made on a loop in the JS console

Screenshots

❌

github-actions · 2020-12-23T16:50:44Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

src/libs/API.js

marcaaron · 2020-12-23T16:57:01Z

src/libs/Network.js

+            // make this request at all and clear the request queue
+            networkRequestQueue = [];
+            return;
+        }


If we don't do this we'll allow a request to be made that can't possibly succeed so it's better that we never make it at all.

If someone adds another throw inside enhanceParameters, or there is some kind of bug that gets introduced that throws an unexpected error, it's not clear that this will cause the network queue to be cleared. I'd rather be very explicit about how this work by doing maybe one of these solutions:

Have a method Network.clearRequestQueue() that is called in place of the throw new Error('A request was made without an authToken');. I'd prefer this one because it's very clear what the intention of the code is.

Have specific checking here on err to only clear the request queue when the A request was made without an authToken error is thrown. I don't like this as much because if I were just looking at addAuthTokenToParameters I wouldn't know that clearing the network queue could be a side-effect of throwing an error.

Ok, that's a valid concern. The problem with 1 if that we probably don't need to make any request if we have no authToken. Clearing the network request queue alone doesn't achieve this since we are already making this request on the next line.

Will think about this some more.

Ok I got a better solution. We will:

Clear and pause the request queue via addAuthTokenToParameters()

Check to see if calling enhanceParameters() paused the queue and return early

src/libs/NetworkConnection.js

marcaaron · 2020-12-23T16:58:49Z

src/libs/actions/Report.js

    // A report can be missing a name if a comment is received via pusher event
    // and the report does not yet exist in Onyx (eg. a new DM created with the logged in person)
-    if (report.reportName === undefined) {
+    if (report.reportID && report.reportName === undefined) {


In some cases, report data will show up first without a reportID and no reportName. In those cases, we should not attempt to fetch a report that has no reportID

Should we attempt to fetch data with ReportID but undefined reportName?

Yeah so, it's completely OK to do that (see the comment above) and in fact we want to because we might be handling a report comment from Pusher and we need to download it. Reports that we've downloaded already should have some kind of report name in storage. This is a little weird but it's just how it works for now. Maybe in the future we will fix this to make it more obvious that a report has not been fetched from the server and won't use the reportName implementation detail.

How would it be possible for there to be a report without a reportID? Especially since the Onyx key uses the report ID in the name of the key?

In theory, this could happen anytime we call merge() for a report that doesn't exist without also including the reportID. I'm not exactly sure what situation leads to that happening here, but this seems like a safe way to avoid calling an API that is guaranteed to fail.

marcaaron · 2020-12-23T16:59:20Z

src/libs/actions/Session.js

                    // If we have an old login for some reason, we should delete it before storing the new details
                    if (credentials.login) {
                        API.DeleteLogin({
+                            authToken: authenticateResponse.authToken,


This seemed to be failing due to not having an authToken despite authenticating just prior.

I think this should be avoided and I would rather dig into why the authToken sent with the DeleteLogin request is incorrect. Maybe this is what we are discussing in this slack thread today?

src/pages/home/report/ReportActionsView.js

Julesssss · 2020-12-23T17:30:40Z

Ran out of time today, will have to review tomorrow morning.

Gonals

Looks good, just one question!

Gonals · 2020-12-28T10:58:15Z

@tgolen All yours!

src/libs/API.js

tgolen · 2020-12-28T21:32:25Z

src/libs/Network.js

+            // make this request at all and clear the request queue
+            networkRequestQueue = [];
+            return;
+        }


If someone adds another throw inside enhanceParameters, or there is some kind of bug that gets introduced that throws an unexpected error, it's not clear that this will cause the network queue to be cleared. I'd rather be very explicit about how this work by doing maybe one of these solutions:

Have a method Network.clearRequestQueue() that is called in place of the throw new Error('A request was made without an authToken');. I'd prefer this one because it's very clear what the intention of the code is.

Have specific checking here on err to only clear the request queue when the A request was made without an authToken error is thrown. I don't like this as much because if I were just looking at addAuthTokenToParameters I wouldn't know that clearing the network queue could be a side-effect of throwing an error.

tgolen · 2020-12-28T21:35:44Z

src/libs/actions/Report.js

    // A report can be missing a name if a comment is received via pusher event
    // and the report does not yet exist in Onyx (eg. a new DM created with the logged in person)
-    if (report.reportName === undefined) {
+    if (report.reportID && report.reportName === undefined) {


How would it be possible for there to be a report without a reportID? Especially since the Onyx key uses the report ID in the name of the key?

tgolen · 2020-12-28T21:37:38Z

src/libs/actions/Session.js

                    // If we have an old login for some reason, we should delete it before storing the new details
                    if (credentials.login) {
                        API.DeleteLogin({
+                            authToken: authenticateResponse.authToken,


I think this should be avoided and I would rather dig into why the authToken sent with the DeleteLogin request is incorrect. Maybe this is what we are discussing in this slack thread today?

src/pages/home/report/ReportActionsView.js

marcaaron · 2020-12-29T00:42:14Z

Updated this one.

tgolen

Cool, I like how you solved that. It's a little awkward to see a double-check for the queue being paused, but the comment you added clarifies why it's necessary and I don't see any other way around it.

src/libs/API.js

marcaaron · 2020-12-29T19:52:26Z

Updated

marcaaron added 2 commits December 23, 2020 06:38

Fix various post sign out requests + bugs

6b5213e

clear timers

218d0c2

marcaaron requested a review from a team as a code owner December 23, 2020 16:50

marcaaron self-assigned this Dec 23, 2020

botify requested review from Gonals and removed request for a team December 23, 2020 16:50

marcaaron added 2 commits December 23, 2020 06:51

fix comment

a852fef

remove clear queue method

2b2bb4a

marcaaron commented Dec 23, 2020

View reviewed changes

marcaaron requested a review from tgolen December 23, 2020 21:22

Julesssss previously approved these changes Dec 24, 2020

View reviewed changes

Gonals requested changes Dec 24, 2020

View reviewed changes

marcaaron requested a review from Gonals December 24, 2020 19:31

Gonals previously approved these changes Dec 28, 2020

View reviewed changes

tgolen requested changes Dec 28, 2020

View reviewed changes

fix conflicts

75fbb0f

marcaaron dismissed stale reviews from Gonals and Julesssss via 75fbb0f December 29, 2020 00:10

Add clearRequestQueue() and clear the queue when there is no authToken

ce167cb

marcaaron requested a review from tgolen December 29, 2020 00:42

tgolen requested changes Dec 29, 2020

View reviewed changes

src/libs/API.js Outdated Show resolved Hide resolved

marcaaron added 3 commits December 29, 2020 08:50

Fix up comment

82e21d0

fix comment again

9f08e55

last one

e97a947

marcaaron requested a review from tgolen December 29, 2020 18:51

tgolen approved these changes Dec 29, 2020

View reviewed changes

marcaaron merged commit 53148ca into master Dec 30, 2020

marcaaron deleted the marcaaron-preventQueued branch December 30, 2020 22:31

github-actions bot locked and limited conversation to collaborators Dec 30, 2020

Conversation

marcaaron commented Dec 23, 2020

Details

Fixed Issues

Tests

Screenshots

Uh oh!

github-actions bot commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Julesssss commented Dec 23, 2020

Uh oh!

Gonals left a comment

Choose a reason for hiding this comment

Uh oh!

Gonals commented Dec 28, 2020

Uh oh!

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!

Uh oh!

Uh oh!

marcaaron commented Dec 29, 2020

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron commented Dec 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Dec 23, 2020 •

edited

Loading