Log in - "Continue" button remains initially active on the login page when logout of offline mode

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Login with any account
3. Turn off your internet connection.
4. Go to Settings -> Sign out
5. Attempt to logout

Expected Result:

The "Continue" button should be inactive, there should be an inscription "You seem to be offline".

Actual Result:

"Continue" button remains initially active

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App
• Mobile Web

Version Number: 1.2.32.1

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5837443_Recording__3092.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[slafortune]

@yuwenmemon is this something that will be an internal fix?

[marcaaron]

I feel like we have run into a similar issue elsewhere. IIRC there was some discussion about whether the user should even be allowed to "log out" if they are offline. I think we could block them from taking this action and nobody would notice or care.

[yuwenmemon]

Hmm, I feel like we should still let people log out when offline.

Imagine you are on a public computer at a library and your internet goes out and you want to leave. You want to be able to clear your session out from that machine before heading out the door, right?

[marcaaron]

Using a public computer to do anything financial is never a good idea 😄

But, lol yeah! It's not a bad argument at all.

[yuwenmemon]

I'm not sure if this is worth fixing but I have figured out a way we could possibly do it.

So the reason this happens is because when we sign out, we call clearStorageAndRedirect. This ends up setting NETWORK/isOffline to null/false. Then only after that do we preserve the isOffline state from its previous value.

So we render once after clearing and then render again after setting isOffline back to the previous state. Hence why we see the "blip".

Two possible ways around this:

1. We could possibly update the defaultKeyState for isOffline to be true before signing out if we're offline.

2. The other option would be to somehow make this clearing action with preserving certain keys atomic. We'd have to update Onyx, but essentially we'd create some sort of clearWithDefaults method that keeps passed-in keys when clearing out Onyx.

[yuwenmemon]

cc @MariaHCD and @neil-marcellini for your thoughts since you worked on things adjacent to this.

[neil-marcellini]

This is a tricky one. I like the clearWithDefaults idea because right now we are only able to set default key states with Onyx.init and I don't think it would make sense to initialize Onyx again. There might be other use cases where we want to dynamically set an Onyx default.

The other option would be to have a different UX on the sign in page. Instead of disabling the button when offline we could just show an error message after you click it, like "Unable to sign in because you appear to be offline". I think that's what we used to have. I like the first option better because it's consistent with our offline UX.

[yuwenmemon]

Okay so then to formalize what I'm thinking we'd add a clearWithDefaults method to Onyx which would essentially be the following edit to clear:

function clearWithDefaults(defaults) {
    const keyStatesToPreserve = fastMerge(defaultKeyStates, defaults);
    return getAllKeys()
        .then((keys) => {
            _.each(keys, (key) => {
                const resetValue = lodashGet(keyStatesToPreserve, key, null);
                cache.set(key, resetValue);
                notifySubscribersOnNextTick(key, resetValue);
            });
            return Storage.clear();
        });
}

Although I would be curious for why we opted to use the clearStorageAndRedirect wrapper function around clear to essentially do this instead of doing it within clear maybe @marcaaron you would know?

[yuwenmemon]

Tested it locally with the changes and it fixed the issue. The other issue it would also solve is if you sign out with your language set to Spanish right now we flash the English login page to you for a split second as well:

[neil-marcellini]

I like your proposal here #13062 (comment) and that's even better if it can fix another issue.

[marcaaron]

I'm not sure if this is worth fixing

Maybe it would be good to get some consensus on it in Slack?

Although I would be curious for why we opted to use the clearStorageAndRedirect wrapper function around clear to essentially do this instead of doing it within clear maybe @marcaaron you would know?

I think it's just the way we did it. Building some extra functionality into clear() to preserve keys feels like it has been discussed in the past (not sure where exactly) probably as a code cleanup (maybe) but not to solve any specific problem. I'd guess (but can't remember for sure) we avoided it to keep the low level Onyx API relatively simple and free of unnecessary responsibility.

[yuwenmemon]

Maybe it would be good to get some consensus on it in Slack?

Sorry, just saw this now after putting the PRs in - I think considering the language bug that this is worth fixing now, and in hindsight did not take too much time to figure out and code up.

[melvin-bot]

@yuwenmemon, @slafortune Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@yuwenmemon, @slafortune 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[yuwenmemon]

Taking this out of review as we had to go back to the drawing board as per this thread:
https://expensify.slack.com/archives/C01GTK53T8Q/p1671583867011199

My initial thought is that while we wait for react-native-onyx to go to SQLite based, we could try using multiRemove from AsyncStorage in the interim as a stop-gap fix.

[melvin-bot]

@yuwenmemon, @slafortune Huh... This is 4 days overdue. Who can take care of this?

[slafortune]

Looking for volunteers here - https://expensify.slack.com/archives/C01GTK53T8Q/p1672088280027649

[melvin-bot]

Triggered auto assignment to @grgia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[slafortune]

Reapplying the engineering label based on the convo here - https://expensify.slack.com/archives/C01GTK53T8Q/p1672088280027649

[tgolen]

I created a draft PR for this. I'm running into issues trying to get it tested locally though. I should be able to finish this up tomorrow 🤞

[grgia]

@tgolen would you like some help testing it?

[tgolen]

Thanks! I was able to finally get it tested locally (once I learned I had to remove the webpack cache to see the local changes to react-native-onyx). Feel free to jump in on a review of Expensify/react-native-onyx#219. It should be ready to go. I tested it out on all platforms and it seems to work pretty well. It at least doesn't crash iOS anymore :D

[melvin-bot]

@tgolen, @slafortune, @grgia Whoops! This issue is 2 days overdue. Let's get this updated quick!

[tgolen]

Not overdue. This is fixed with Expensify/react-native-onyx#220

[slafortune]

Looks like this was ^ is close to complete

[tgolen]

Yep, it's getting there! Should be ready to merge

[melvin-bot]

@tgolen, @slafortune, @grgia Whoops! This issue is 2 days overdue. Let's get this updated quick!

[yuwenmemon]

This just got deployed to Prod, so I believe we can close this out! Even gave it a quick spin and it works like a charm 👍