okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
329 stars 43 forks source link

Simplify and explain login flow #2317

Closed corrideat closed 5 days ago

cypress[bot] commented 1 week ago

group-income    Run #3100

Run Properties:  status check passed Passed #3100  •  git commit 0cdc4d2044 ℹ️: Merge 20f94273df2b9e72cdc2d8ac544ad4f0d57a3596 into e341e652a02766143f1a084f3547...
Project group-income
Branch Review 2271-simplify-login-flow
Run status status check passed Passed #3100
Run duration 09m 36s
Commit git commit 0cdc4d2044 ℹ️: Merge 20f94273df2b9e72cdc2d8ac544ad4f0d57a3596 into e341e652a02766143f1a084f3547...
Committer Ricardo Iván Vieitez Parra
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 111
View all changes introduced in this branch ↗︎
corrideat commented 1 week ago

Question: is this PR finished or is there more work to do?

As mentioned on Slack, the only thing missing (other than feedback) is completing the LOGIN_FLOW.md file.

I ask because as far as the jumping around between events related to login that happens when following the logic of actions/identity.js and app/identity.js, that seems unchained. Is there no way to simplify that part / make it clearer?

I'm going to assume you mean unchanged other than unchained. Even then, I'm still unsure what you're asking specifically.

If you're talking about where the event listeners are defined, they are in those files as per your request when those event listeners were defined. There's no particular reason why they couldn't be in a different location.

If you're talking about the events themselves, that's the result of the service worker and the app being different contexts entirely and of both needing to know about the currently logged in user.

taoeffect commented 1 week ago

I'm going to assume you mean unchanged other than unchained.

Yes, that was a typo.

This PR does a great job of simplifying main.js. As-is, it's an improvement over what we have.

What I'm wondering is, is it possible in any way at all, to simplify the logic that's inside of gi.app/identity/login and gi.actions/identity/login (and related code)? Is this something you can take a closer look at? If nothing can be done to simplify it, then I suppose we'll have to live with it as-is, but if something can be done to simplify it, it would be awesome and also this would probably be the time to do it.

corrideat commented 1 week ago

gi.app/identity/login and gi.actions/identity/login

I've taken a look and it looks pretty simple to me already (both about 100 lines with relatively straightforward logic). I'll add some brief additional comments to the uncommented sections.

corrideat commented 1 week ago

I consider this PR complete now unless there are additional requests. I highly recommend taking the time to read through LOGIN_FLOW.md. It's an extensive file that presents in detail the motivations behind everything, documents where things happen and why and walks though specific scenarios.

corrideat commented 1 week ago

Thanks! Updated as requested.