Clean up and simplify login flow

[taoeffect]

Problem

The way logging in right now works is extraordinarily confusing and complicated.

There's events being thrown here and there, event handlers across multiple files, and the whole process involves lots of code in lots of places across main.js, controller/actions/identity.js, and controller/app/identity.js.

Solution

• Clean up the login flow, especially in main.js.
• Cease the use of string literals like 'CHELONIA_STATE' and be consistent with the rest of the code (define it as a constant somewhere)
• Shouldn't sbp('okTurtles.events/emit', CONTRACTS_MODIFIED, Array.from(this.subscriptionSet)) be done at the end of 'chelonia/reset' (after newState is applied)?
• Remove the use of _private selectors, e.g. 'gi.app/identity/_private/login' etc. Either they are not necessary at all, or they are proven necessary, in which case an anonymous function inside the original selector is preferred.

[corrideat]

Shouldn't sbp('okTurtles.events/emit', CONTRACTS_MODIFIED, Array.from(this.subscriptionSet)) be done at the end of 'chelonia/reset' (after newState is applied)?

I don't think it makes a difference as that entire block is synchronous. However, the answer would be no, not necessarily. CONTRACTS_MODIFIED should be emitted every time the subscriptionSet changes.

Remove the use of _private selectors, e.g. 'gi.app/identity/_private/login' etc. Either they are not necessary at all, or they are proven necessary, in which case an anonymous function inside the original selector is preferred.

They may look ugly, but I think they're necessary. Besides keeping things in sync, they also help debug issues, as the relevant call is logged this way.

Even if the selector is removed (which would remove some debug information), I'd use a named constant and not sbp('queue', 'foo', () => {}), because that'd unnecessarily use indentation (but this is mostly an aesthetic concern).

[corrideat]

Clean up the login flow, especially in main.js.

Cease the use of string literals like 'CHELONIA_STATE' and be consistent with the rest of the code (define it as a constant somewhere)

Those seem reasonable

[corrideat]

Clean up the login flow, especially in main.js.

I've been inspecting the code, and took a look especially at main.js. Although main.js is relatively complex, and more so than it should be, I'm not convinced that the situation is that bad.

The main area to improve in main.js is in the mounted method. I propose moving as much as possible of that to a different file and further breaking that up into smaller and simpler functions.

Then, outside of main.js, I think that the complexity of the login process is that it's now all over the place. There are some things in main.js, some in app and some in actions. The separation between app, main.js and actions is necessary. However, I think that this fact in addition to it being event-driven makes it difficult to follow, even for someone familiar with the codebase.

Unfortunately, I'm not sure there is much to be simplified here (not to say that those opportunities shouldn't be explored, there could be a few), but as an alternative, I propose adding some additional comments and creating a brief LOGIN-PROCESS.md file that explains in simple terms how login works and how the different parts fit together.

[taoeffect]

They may look ugly, but I think they're necessary. Besides keeping things in sync, they also help debug issues, as the relevant call is logged this way.

I'm not convinced they're necessary. After #2294 is merged we should be able to verify this more readily (since the tests will be more stable). I'm also not sure what you mean by "relevant call is logged this way", as the selector will still be logged when you remove the wrapper (sbp logs all these selectors, and right now there are two log statements for the same login action, one for the _private wrapper, and one for the actual selector).

Then, outside of main.js, I think that the complexity of the login process is that it's now all over the place.

💯

Unfortunately, I'm not sure there is much to be simplified here (not to say that those opportunities shouldn't be explored, there could be a few), but as an alternative, I propose adding some additional comments and creating a brief LOGIN-PROCESS.md file that explains in simple terms how login works and how the different parts fit together.

That would be OK, but really would should find a way to reduce the amount of jumping around files that happens, even if it's moving some code from different files into the same file. It's extraordinarily difficult to follow the login flow right now. We need to find a way to simplify this so that we don't intimidate developers (too much).

[corrideat]

I'm not convinced they're necessary.

They're (they = the queued invocations, not the specific decision to use selectors) necessary because they put a barrier that forces the order of login and logout events. These are external events from the perspective of contracts.

I'm also not sure what you mean by "relevant call is logged this way",

I mean that they'll be logged in order of execution. For example, without using selectors you might see login or logout in the logs, but that's only when they were put in the queue, not when they were executed (however, using a regular function and adding a log statement at the beginning thereof would work too)

really would should find a way to reduce the amount of jumping around files that happens

Agreed, but as I tried to mention earlier, a large part of the 'code' complexity is because the process itself is complex, with several things happening in different locations, and a large part of that is because of SW support, which means the session state goes from the app to the SW and then is broadcasted from the SW to the app.

This is not to say that the code couldn't be organised differently, but rather to say that the complexity stems from the parts needed to make it work.

[taoeffect]

They're (they = the queued invocations, not the specific decision to use selectors) necessary because they put a barrier that forces the order of login and logout events. These are external events from the perspective of contracts.

When you phrase it this way I see your point and especially how this could be useful for the Cypress tests where logins and logouts happen relatively quickly. I agree that at the very least it can't hurt, and in some rare instance might help.

(however, using a regular function and adding a log statement at the beginning thereof would work too)

👍

This is not to say that the code couldn't be organised differently, but rather to say that the complexity stems from the parts needed to make it work.

Yes, makes sense.