Conversation
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
2700-improve-logging-in-process
|
| Run status |
|
| Run duration | 13m 11s |
| Commit |
|
| Committer | Ricardo Iván Vieitez Parra |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
116
|
| View all changes introduced in this branch ↗︎ | |
corrideat
force-pushed
the
2700-improve-logging-in-process
branch
2 times, most recently
from
e79edbe to
7aef971
Compare
taoeffect
left a comment
taoeffect
left a comment
There was a problem hiding this comment.
I'm not sure about this PR...
| await sbp('gi.db/settings/save', SETTING_CURRENT_USER, identityContractID) | ||
| sbp('okTurtles.events/emit', LOGIN, { identityContractID, encryptionParams, state }) |
There was a problem hiding this comment.
Why is this being moved here?
The point of LOGIN, as far as I recall, was to indicate that we've logged in (at least partially).
This event is being emitted before that's happened, as the identity contract has not been loaded. It should at least be emitted after the identity contract has been loaded.
There was a problem hiding this comment.
But we have partially logged in at this point, in the sense of verifying credentials. This change is necessary:
- To allow for resumable login (as the issue says). Waiting for the entire identity contract means that logging in is an 'all or nothing' approach
- For offline use. If you see Offline cache #2508, you'll notice a similar change there. This is because in the case of an offline session, we obviously can't wait for
syncto complete.
The LOGIN event also isn't to indicate that we've logged in, that's LOGIN_COMPLETE. See the LOGIN_FLOW.md file:
LOGIN: TheLOGINevent represents an active login intent, which is to
say, it represents that there is an active login process, which may either
fail or succeed. This event can originate in the service worker (when
establishing a new session) or in a local browsing context (when there is
an active sesion).
LOGIN_COMPLETE: TheLOGIN_COMPLETEis emitted following aLOGINevent
and signifies that the login process completed successfully. This event is
only local, which means that it doesn't originate on the service worker. Its
purpose is to let the app know that an active user session exists and has been
initialised.
There was a problem hiding this comment.
This is the change I was talking about, which is uglier than this because it's testing for a condition that's difficult to detect (come to think of it, that might be why that PR is working during dev testing but not under real conditions).
There was a problem hiding this comment.
The Join.vue page listens for LOGIN as well. If there's an issue syncing the identity contract, it might think everything worked out well. When you return to this PR, can you please double-check that as well?
There was a problem hiding this comment.
Also, worth testing this to see what happens if syncing throws, and how recoverable the situation is.
There was a problem hiding this comment.
What about renaming LOGIN to LOGIN_START, LOGIN_INIT, or LOGIN_BEGIN to mitigate the confusion?
There was a problem hiding this comment.
What about renaming LOGIN to LOGIN_START, LOGIN_INIT, or LOGIN_BEGIN to mitigate the confusion?
That sounds pretty reasonable
corrideat
force-pushed
the
2700-improve-logging-in-process
branch
from
7aef971 to
71ccebb
Compare
3 participants
Closes #2700