Fix #2288 - lastLoggedIn update

[cypress[bot]]

group-income    Run #3122

Run Properties:   Passed #3122  •  be4ad846f0 ℹ️: Merge 7c58edff5119ffe12964d7893bfb0b86cef4a816 into a0124826f54ee669053c86387470...

Project	group-income
Branch Review	2288-lastloggedin-value-is-still-not-being-updated-properly
Run status	Passed #3122
Run duration	09m 11s
Commit	be4ad846f0 ℹ️: Merge 7c58edff5119ffe12964d7893bfb0b86cef4a816 into a0124826f54ee669053c86387470...
Committer	Ricardo Iván Vieitez Parra
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	10
Skipped	0
Passing	111
View all changes introduced in this branch ↗︎

[corrideat]

So while they're logged in it should be updated as well. It should be updated: [...]

In that case, I'm thinking of reverting some of the changes made.

Part of the reason of moving the location for calling gi.actions/group/kv/updateLastLoggedIn into app/identity.js is that it more closely reflects when a user has last logged in or been active (e.g., if you never log out, having this call in actions/identity.js means it'll basically never be called). However, if now it's a throttled periodic thing it might make more sense for it to live in the SW.

I'm thinking then maybe of this:

1. Putting it back in actions/identity.js.
2. Add a periodic call to it that only runs iff the value is too old.
3. Adding another call (in addition to the periodic call above) in app/identity.js, which uses the same logic as the periodic call.

More detailed description of 2 and 3 above (code examples not to be taken literally).

1. Let's say I add a parameter to gi.actions/group/kv/updateLastLoggedIn called throttle. The logic looks like if (throttle && lastLoggedIn > Date.now() - 30 * 60e3) return.
2. For 2, something like setInterval(() => sbp('gi.actions/group/kv/updateLastLoggedIn', { throttle: true }), 300e3)
3. For 3, something like sbp('gi.actions/group/kv/updateLastLoggedIn', { throttle: true }).

How does that sound?

[taoeffect]

@corrideat I'm not sure I follow, sorry. I don't understand why 2 periodic calls would be needed.

Also, ideally we would leverage some existing periodic logic we have, like the code @SebinSong wrote for gi.periodicNotifications, and/or the code that @snowteamer wrote for chelonia.persistentActions

[corrideat]

I don't understand why 2 periodic calls would be needed.

Maybe because they wouldn't? Only a single periodic call is needed. However, gi.actions/group/kv/updateLastLoggedIn needs to be called from three locations.

1. actions/identity.js[login]: This is when you actually log in (what master does). If you refresh the page or open a new tab, or re-open an active session, this does not get called (which is why now it doesn't work). In this PR, for this reason, I moved it to app. Now, after your feedback, I think that I could put it back where it was so that when you actually login, the value is set, regardless of how frequently it happens (or, alternatively, this particular call could be skipped).
2. Periodic call (yes, it's possible we could re-use something else and it doesn't necessarily need to be setInterval, I'll look into that). This would be what refreshes the value. I'm proposing to call this relatively frequently but have it throttled so that it's only updated once
3. app/identity.js[login]: This is when you load the app (which could be a new or an existing session). In this PR, you need this to happen for refreshing the lastLoggedIn attribute (currently). I'm proposing to keep this call but throttle it. The effect of this is that opening the app immediately updates lastLoggedIn if it's too old.

Worked example.

Let's say lastLoggedIn is 1970-01-01 00:00 and no current sessions exist. With the proposed changes ([x] means which part makes the call):

1. You log in from a new device at 00:05. lastLoggedIn gets set to 00:05. [actions]
2. You close that tab after logging in.
3. You open a new tab at 00:20. Nothing happens. [app]
4. You log out and log in again at 00:30. lastLoggedIn gets set to 00:30. [actions]
5. You close that tab after logging in.
6. You open a new tab at 00:45. Nothing happens. [app]
7. You close that tab.
8. You open a new tab at 01:01. lastLoggedIn gets set to 01:01. [app].
9. You leave that tab open.
10. 01:06: Nothing happens [periodic].
11. 01:11: Nothing happens [periodic].
12. etc.
13. 01:31: lastLoggedIn gets set to 00:31. [periodic].

[taoeffect]

OK, thanks for the clarification. Regarding the periodic call, one thing to watch out for is if there are X tabs open, if this would cause it to get called X times as frequently. If so, maybe the periodic logic needs to be moved to the SW.

[corrideat]

Does the example look right, or do you think that the call from actions should be skipped?

if there are X tabs open, if this would cause it to get called X times as frequently. If so, maybe the periodic logic needs to be moved to the SW.

You're right, but then the SW can be suspended. I had some logic for the SW to be kept alive while tabs are open, so in practice I think it could be done either way.

In either case, the calls should be infrequent (I suggested 5 minutes), so largely I don't think this is a concern either way. Even if you have 100 tabs open, you'd be calling this on average once every 3s, which, while not ideal, should also not affect performance much (since also those calls will be no-ops most of the time).

[corrideat]

If skipping the call from actions, the example would look like this:

1. You log in from a new device at 00:05. ~lastLoggedIn gets set to 00:05. [actions]~
2. You close that tab after logging in.
3. You open a new tab at 00:20. Nothing happens. [app]
4. You log out and log in again at 00:30. lastLoggedIn gets set to 00:30. ~[actions]~ [app]
5. You close that tab after logging in.
6. You open a new tab at 00:45. Nothing happens. [app]
7. You close that tab.
8. You open a new tab at 01:01. lastLoggedIn gets set to 01:01. [app].
9. You leave that tab open.
10. 01:06: Nothing happens [periodic].
11. 01:11: Nothing happens [periodic].
12. etc.
13. 01:31: lastLoggedIn gets set to 00:31. [periodic].

[taoeffect]

In either case, the calls should be infrequent (I suggested 5 minutes), so largely I don't think this is a concern either way. Even if you have 100 tabs open, you'd be calling this on average once every 3s, which, while not ideal, should also not affect performance much (since also those calls will be no-ops most of the time).

Well, we also multiply whatever this value is by the number of active users and it can become an unnecessary strain on the server. So we need to make sure the period is faithful to the number of browsers (not tabs or windows) open.

Does the example look right, or do you think that the call from actions should be skipped?

If the actions is the SW, then it shouldn't be skipped per above, right?

I don't quite following your example above, as it shows a periodic invocation in which nothing happens even though the interval that you set (5 min) was exceeded.

[corrideat]

Well, we also multiply whatever this value is by the number of active users and it can become an unnecessary strain on the server. So we need to make sure the period is faithful to the number of browsers (not tabs or windows) open.

Well, no. I'm maybe not explaining the proposal and example clearly.

See my comment where I mention that calls from 2 (periodic) & 3 (actions) are throttled (with a code snippet to show the basic logic). This means that no matter how often you call them, the server is not involved unless the value is too old.

I don't quite following your example above, as it shows a periodic invocation in which nothing happens even though the interval that you set (5 min) was exceeded.

Nothing happens because the throttle parameter only updates the value when it's too old (30 minutes). So, while the periodic call is made every 5 minutes, since there's nothing to update, nothing gets updated.

The reason to have a lower window for the period is to ensure that the value is kept updated.

[taoeffect]

Oh ok, now I get what you mean. 👍

Alright, sounds good to me!

[corrideat]

You didn't answer whether logging in to a fresh session should immediately update the lastLoggedIn value or not.

[taoeffect]

You didn't answer whether logging in to a fresh session should immediately update the lastLoggedIn value or not.

Sorry, I missed that! Yes, if by "fresh session" you mean logging in after being logged out (or waking it up on another device), definitely it should be called.

[corrideat]

It'll be called either way, but the question is whether to make an immediate unthrottled call (see the two different examples).

lastLoggedIn = 00:00:00

You log in by entering your password at 00:05:00.

Scenario A.

Since you logged in, we set lastLoggedIn = 00:05:00

Scenario B.

Since you logged in, we call the throttled call. However, since 0 - 30 < 5, we don't update anything at this point.

[taoeffect]

Yeah, make an immediate, unthrottled call when you've logged in.

[corrideat]

Feedback is implemented. The remaining issue is fixing the tests. Feel free to review.

[corrideat]

Merged master and resolved conflicts