login: Support logging out of an account #948

chrisbobbe · 2024-09-17T22:00:03Z

Screenshots coming soon. :)

Fixes: #463

chrisbobbe · 2024-09-17T22:04:25Z

chrisbobbe · 2024-09-17T22:05:44Z

This implements the logic where per-account routes on the nav are removed on logout, but it doesn't animate their exits; they just disappear suddenly. I think that's probably fine, and I didn't find a way to add the animation in.

To test a scenario where there are per-account routes when the logout happens, you can add a button to the "under construction" screen and copy-paste _logOutAccount into its press handler.

chrisbobbe · 2024-09-17T22:07:02Z

Marked as a draft because in particular I think this may be lacking some test coverage that we'll want; for example, of the various if (maybeAccount == null) return;s added in PerAccountStore. But in that example, at least, I'm not sure if there might be a nicer way to write the app code. I added those early returns to avoid having ApiConnection.send get called after the ApiConnection was closed, which is a goal, as I think we discussed in the office on Friday.

gnprice

Thanks! Looking forward to having this.

Comments below on reading mainly the logic asked about in your last comment above.

gnprice · 2024-09-20T23:44:54Z

lib/model/store.dart

+      // Abort if the account has been logged out.
+      if (store.maybeAccount == null) return;


(In the office we discussed some changes to this part: have a bool disposed on UpdateMachine, so this is if (disposed) return;; check just after each await in this method; and handleEvent and sendMessage don't need the check, but perhaps should get an assert for it instead.)

gnprice · 2024-09-20T23:53:17Z

test/model/store_test.dart

+      check(notifyCount).equals(1); // no extra notify
+    });
+
+    // TODO test database gets updated correctly (an integration test with sqlite?)


I think we're fine without this.

Probably the right thing in principle is that the contents of LiveGlobalStore.doRemoveAccount should go into a removeAccount method on AppDatabase, and similarly doUpdateAccount and doInsertAccount should be reduced to one-line wrappers around AppDatabase methods. Then we can write tests for those in database_test.dart so those tests run against actual SQLite, like the existing tests in that file.

That'll become important if we start having more logic in those methods — like for example in a future where we cache all the server data locally, and so have a much more complex data model in the database.

But for now while these methods are so trivial, I think we're fine without such tests.

gnprice · 2024-09-20T23:53:55Z

lib/api/route/notifications.dart

+// https://zulip.com/api/add-fcm-token
 Future<void> registerFcmToken(ApiConnection connection, {


nit: should be doc comment

gnprice · 2024-09-20T23:57:22Z

lib/model/store.dart

@@ -75,7 +75,10 @@ abstract class GlobalStore extends ChangeNotifier {
      email: account.email, apiKey: account.apiKey);
  }

+  Map<int, PerAccountStore> get debugPerAccountStores => _perAccountStores;


How about like this?

Suggested change

Map<int, PerAccountStore> get debugPerAccountStores => _perAccountStores;

UnmodifiableMapView<int, PerAccountStore> get debugPerAccountStores => UnmodifiableMapView(_perAccountStores);

That just makes a bit clearer that this is only meant for inspection (and that one doesn't have to wonder if the maps might get modified through these).

Oh, interesting. Sure.

gnprice · 2024-09-21T00:03:29Z

lib/model/store.dart

    final updateMachine = UpdateMachine.fromInitialSnapshot(
      store: store, initialSnapshot: initialSnapshot);
+    store.updateMachine = updateMachine;


Let's put this assignment inside the UpdateMachine constructor. That way it's more crisply an invariant that the UpdateMachine's store will always point back to it.

Then relatedly (also about making invariants more explicit), PerAccountStore.updateMachine can get a setter that asserts the old value was null, so we're not redirecting the same store to start pointing to a different update machine.

gnprice · 2024-09-21T00:05:53Z

lib/model/store.dart

+    _perAccountStores.remove(accountId)
+      // TODO close connection inside `.dispose` instead (once tests can adapt)
+      ?..dispose()..connection.close();
+    _perAccountStoresLoading.remove(accountId);


Even though this is logic I largely suggested a week or two ago, I found it kind of confusing to read. 🙂 I think the logic is good but would benefit from some comments.

I can take a pass at writing those comments in the next round.

Cool; thanks.

gnprice · 2024-09-21T00:07:32Z

test/model/store_test.dart

+      expectStore
+        ? check(store.debugPerAccountStores[accountId]).isNotNull()
+        : check(store.debugPerAccountStores[accountId]).isNull();
+      expectStoreLoading
+        ? check(store.debugPerAccountStoresLoading[accountId]).isNotNull()
+        : check(store.debugPerAccountStoresLoading[accountId]).isNull();


Ideally these tests would be written without this inspection of the internal state of GlobalStore, and instead in terms of its outward-facing behavior seen by its callers. Ultimately all the behavior we care about should be observable that way.

gnprice · 2024-09-21T00:14:11Z

test/model/store_test.dart

+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: false, expectStore: false, expectStoreLoading: false);


For example here, I think the fact that we remove the account from _perAccountStoresLoading can be observed by checking:

A fresh perAccount call at this point throws directly, rather than returning a Future that throws. (Though this fact isn't that important — maybe shouldn't even get a test, since it'd be fine if it changed.)

A fresh perAccount call after the old loadingFuture has completed will throw directly, rather than returning a Future that throws. This basically expresses the fact that we don't have an entry in _perAccountStoresLoading sticking around indefinitely after the whole interaction is done.

Or thinking again, maybe the only fact we really want here about _perAccountStoresLoading is:

At the end of the whole story, _perAccountStoresLoading is empty. (Could have a debug getter that just reports its length.)

In other words that means we aren't leaking a reference to an old Future that's no longer relevant.

The difference between throwing immediately and returning a Future that throws is maybe nice but isn't really fundamental — it's not like we need to have an extra fast answer when looking for an account that no longer exists.

At the end of the whole story, _perAccountStoresLoading is empty. (Could have a debug getter that just reports its length.)

Cool; sure, I'll try this for the next revision.

gnprice · 2024-09-21T00:15:19Z

test/model/store_test.dart

+        ? check(store.debugPerAccountStores[accountId]).isNotNull()
+        : check(store.debugPerAccountStores[accountId]).isNull();


These can just use perAccountSync, right?

chrisbobbe · 2024-10-12T01:57:33Z

Thanks for the review! Revision pushed.

gnprice · 2024-10-15T23:57:15Z

lib/api/route/notifications.dart

+/// https://zulip.com/api/add-fcm-token
 Future<void> registerFcmToken(ApiConnection connection, {


After reading these API docs against our code, I just sent #998 to more fully update to what the docs say. I think that replaces part of this commit from this PR:
e20fa5e api: Add unregister{Apns,Fcm}Token; link to new docs for register-token fns

gnprice

Thanks! Generally this looks good. Comments below, having read completely through the first 7 commits:
1147845 test: Assert isOpen on FakeApiConnection.prepare
e20fa5e api: Add unregister{Apns,Fcm}Token; link to new docs for register-token fns
de0213e store [nfc]: Add indirection doLoadPerAccount
a73e929 store [nfc]: Comment in _PerAccountStoreWidgetState.didChangeDependencies
5e7d2f5 store: Add nullable [PerAccountStore.updateMachine]; dispose in [dispose]
a271863 store: Implement removeAccount
3e11686 notif: Implement NotificationService.unregisterToken

and skimmed the last one:
054da31 login: Support logging out of an account

I also sent #998 as mentioned above; and I have a draft locally of the comments I mentioned at #948 (comment) last time.

gnprice · 2024-10-15T23:59:38Z

lib/model/store.dart

+  set updateMachine(UpdateMachine? value) {
+    assert(_updateMachine == null);


Suggested change

set updateMachine(UpdateMachine? value) {

assert(_updateMachine == null);

set updateMachine(UpdateMachine? value) {

assert(_updateMachine == null);

assert(value != null);

That way it's clear this only ever gets set to non-null once for a given PerAccountStore.

gnprice · 2024-10-16T00:06:36Z

lib/model/store.dart

@@ -445,10 +492,14 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
    unreads.dispose();
    _messages.dispose();
    typingStatus.dispose();
+    updateMachine?.dispose(); // TODO is updateMachine ever null except in tests?


No, only in tests.

I think there's nothing actionable for a TODO here, though.

gnprice · 2024-10-16T00:13:51Z

lib/model/store.dart

+    await doRemoveAccount(accountId);
+    assert(_accounts.containsKey(accountId));


Suggested change

await doRemoveAccount(accountId);

assert(_accounts.containsKey(accountId));

assert(_accounts.containsKey(accountId));

await doRemoveAccount(accountId);

if (!_accounts.containsKey(accountId)) return; // Already removed.

Otherwise I think there's a race here if we try to remove the same account twice concurrently.

gnprice · 2024-10-16T00:50:01Z

lib/model/store.dart

+      // TODO close connection inside `.dispose` instead (once tests can adapt)
+      store..dispose()..connection.close();


What's the obstacle to doing this?

(I think we've discussed this before, but I've forgotten.)

Thanks for going back over this with me in the office! :)

gnprice · 2024-10-16T00:52:24Z

lib/model/store.dart

    super.dispose();
+    _disposed = true;
  }


I think this order feels more logical:

Suggested change

super.dispose();

_disposed = true;

}

_disposed = true;

super.dispose();

}

The _disposed flag is part of this subclass's data; so we set it to the "yes, disposed" state before calling the base class's implementation to tear down the base class's data.

gnprice · 2024-10-16T00:53:14Z

lib/model/store.dart

@@ -890,6 +959,8 @@ class UpdateMachine {
        }
      }

+      if (_disposed) return;


I think the _disposed flag, together with all these conditions checking for it, would be good as its own commit.

nit: let's put these checks consistently right after each await.

That's where we want them semantically; so arranging it that way overtly in the source code is I think the only reasonable way to be sure we're doing that.

So e.g. this one moves to right after result = await getEvents(…. (And we'll still need one at the top of the catch block, which is the other place that's "right after" that await.)

And the one toward the top of the loop moves further up, to right after await _debugLoopSignal….

Then for completeness we can also put an assert at the top of the method itself, like we're doing on other methods.

gnprice · 2024-10-16T00:54:48Z

lib/model/store.dart

-    return _apiSendMessage(connection,
+    await _apiSendMessage(connection,


nit: can stick with return here (and no async) — that also matches how the underlying route bindings are written, which end like return connection.post(…);

gnprice · 2024-10-16T01:02:37Z

lib/widgets/store.dart

+        } catch (e) {
+          // TODO(log)
+        }


Suggested change

} catch (e) {

// TODO(log)

}

}

If we don't catch the exception, it'll become an unhandled exception (because nothing awaits the future returned by this IIFE, and the exception will cause that future to complete with an error). That'll automatically cause it to get logged, once we have any kind of generic logging in place.

Could also say something like

rethrow; // TODO(log)

but, apart from playing host to the comment, that has exactly the same effect as not having the catch block in the first place. Because I think we don't need the TODO, that makes it redundant.

gnprice · 2024-10-16T01:12:08Z

lib/widgets/app.dart

+    // Async IIFE to not block removing the account on the unregister-token request.
+    unawaited(() async {


nit: move this anonymous function to be its own private method with a name; it's 17 lines long, so I think that'll help make the overall structure easier to see

gnprice · 2024-10-16T01:20:14Z

lib/widgets/app.dart

+    // TODO error handling? disable logout button during this?
+    await globalStore.removeAccount(accountId);


Eh — this is only talking to the local database, not the network, so it should pretty consistently be fast and reliable. I'd guess p50 of <1ms, p99 of <50ms.

(That's jargon/shorthand for "50th percentile" and "99th percentile" respectively.)

And this is an uncommon operation. So it isn't worth building extra UI for the small fraction of users where it takes more than a frame for the operation to complete.

It's no longer true that these endpoints are undocumented, so, link to them.

…cies

…ose]

We'll add more to this soon.

…alse

This seems hygienic. It makes sense to explicitly clean up HTTP client resources when we know we'll be setting up a new client instance to replace the old one. (The dead-queue reload is currently the only path where PerAccountStore.dispose is called.) It will also make sense to clean up these resources when an account is logged out. That's a feature we'll be implementing soon, with PerAccountStore.dispose in that new path as well. The "TODO abort long-poll and close ApiConnection" was on UpdateMachine's dispose method, not PerAccountStore's. But since PerAccountStore is what owns the connection, its dispose method seemed like the more appropriate place to close the connection.

Fixes: zulip#463

chrisbobbe · 2024-10-17T23:32:10Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Generally this looks great; comments below.

I haven't yet read fully through the final commit:
6bc8bcc login: Support logging out of an account

In the interest of getting things merged efficiently, maybe let's split this PR up — let's have this first PR cover the commits leading up to removeAccount:
ed1adbe test: Assert isOpen on FakeApiConnection.prepare
18914a4 store [nfc]: Add indirection doLoadPerAccount
6cfe900 store [nfc]: Comment in _PerAccountStoreWidgetState.didChangeDependencies
2609910 store: Add nullable [PerAccountStore.updateMachine]; dispose in [dispose]
b2cedd5 store: Add dartdoc on UpdateMachine.dispose
3e0d499 store: Add and check _disposed on PerAccountStore and UpdateMachine
7aff4d1 store test: Expose TestGlobalStore.useCachedApiConnections; default false
f3a3361 store: Call connection.close in PerAccountStore.dispose
ab9ce6d store: Implement removeAccount

and then a follow-up PR can cover the remaining commits:
325c663 api: Add remove{Apns,Fcm}Token
b4b9569 notif: Implement NotificationService.unregisterToken
6bc8bcc login: Support logging out of an account

gnprice · 2024-10-18T00:43:50Z

lib/api/route/notifications.dart

+/// https://zulip.com/api/remove-fcm-token
+Future<void> removeFcmToken(ApiConnection connection, {


nit: paragraph in commit message is no longer relevant in this revision:

api: Add remove{Apns,Fcm}Token It's no longer true that these endpoints are undocumented, so, link to them.

gnprice · 2024-10-18T00:51:21Z

lib/model/store.dart

@@ -430,6 +474,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
  // End of data.
  ////////////////////////////////////////////////////////////////

+  bool _disposed = false;


nit: put this just above dispose

(Alternatively it could go up in the first section of data fields, after isLoading. But if it's outside the "data" section anyway, there's no benefit from having reassemble separate it from dispose.)

gnprice · 2024-10-18T00:56:58Z

lib/model/store.dart

@@ -890,6 +959,8 @@ class UpdateMachine {
        }
      }

+      if (_disposed) return;


nit: let's put these checks consistently right after each await.

That's where we want them semantically; so arranging it that way overtly in the source code is I think the only reasonable way to be sure we're doing that.

So e.g. this one moves to right after result = await getEvents(…. (And we'll still need one at the top of the catch block, which is the other place that's "right after" that await.)

And the one toward the top of the loop moves further up, to right after await _debugLoopSignal….

Then for completeness we can also put an assert at the top of the method itself, like we're doing on other methods.

gnprice · 2024-10-18T00:57:33Z

lib/model/store.dart

  Future<void> registerNotificationToken() async {
+    if (_disposed) return;


Should be an assert, I think — seems like a bug if we get here when it's already disposed.

gnprice · 2024-10-18T01:04:12Z

lib/model/store.dart

    recentDmConversationsView.dispose();
    unreads.dispose();
    _messages.dispose();
    typingStatus.dispose();
+    updateMachine?.dispose();
+    connection.close();


nit:

This seems hygienic. It makes sense to explicitly clean up HTTP client resources when we know we'll be setting up a new client instance to replace the old one. (The dead-queue reload is currently the only path where PerAccountStore.dispose is called.)

In this situation it's not that we will be setting up a new HTTP client instance; rather we already have, which was passed to the new PerAccountStore (and even before that, was used for fetching the new store's initial snapshot). The new store gets constructed before the old one is disposed.

chrisbobbe requested a review from gnprice September 17, 2024 22:00

gnprice reviewed Sep 21, 2024

View reviewed changes

chrisbobbe force-pushed the pr-logout-account branch from 60515c1 to 054da31 Compare October 12, 2024 01:57

chrisbobbe marked this pull request as ready for review October 12, 2024 01:57

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Oct 15, 2024

chrisbobbe assigned gnprice Oct 15, 2024

gnprice mentioned this pull request Oct 15, 2024

api [nfc]: Update notif-token routes based on API docs #998

Merged

gnprice reviewed Oct 15, 2024

View reviewed changes

gnprice reviewed Oct 16, 2024

View reviewed changes

chrisbobbe added 12 commits October 17, 2024 15:03

test: Assert isOpen on FakeApiConnection.prepare

ed1adbe

api: Add remove{Apns,Fcm}Token

325c663

It's no longer true that these endpoints are undocumented, so, link to them.

store [nfc]: Add indirection doLoadPerAccount

18914a4

store [nfc]: Comment in _PerAccountStoreWidgetState.didChangeDependen…

6cfe900

…cies

store: Add nullable [PerAccountStore.updateMachine]; dispose in [disp…

2609910

…ose]

store: Add dartdoc on UpdateMachine.dispose

b2cedd5

We'll add more to this soon.

store: Add and check _disposed on PerAccountStore and UpdateMachine

3e0d499

store test: Expose TestGlobalStore.useCachedApiConnections; default f…

7aff4d1

…alse

store: Implement removeAccount

ab9ce6d

notif: Implement NotificationService.unregisterToken

b4b9569

login: Support logging out of an account

6bc8bcc

Fixes: zulip#463

chrisbobbe force-pushed the pr-logout-account branch from 054da31 to 6bc8bcc Compare October 17, 2024 23:30

gnprice reviewed Oct 18, 2024

View reviewed changes

gnprice mentioned this pull request Oct 18, 2024

[Draft] login: ability to remove logged in account #559

Closed

		// Abort if the account has been logged out.
		if (store.maybeAccount == null) return;

		// https://zulip.com/api/add-fcm-token
		Future<void> registerFcmToken(ApiConnection connection, {

	Map<int, PerAccountStore> get debugPerAccountStores => _perAccountStores;
	UnmodifiableMapView<int, PerAccountStore> get debugPerAccountStores => UnmodifiableMapView(_perAccountStores);

		checkGlobalStore(globalStore, eg.selfAccount.id,
		expectAccount: false, expectStore: false, expectStoreLoading: false);

		? check(store.debugPerAccountStores[accountId]).isNotNull()
		: check(store.debugPerAccountStores[accountId]).isNull();

		/// https://zulip.com/api/add-fcm-token
		Future<void> registerFcmToken(ApiConnection connection, {

		set updateMachine(UpdateMachine? value) {
		assert(_updateMachine == null);

		await doRemoveAccount(accountId);
		assert(_accounts.containsKey(accountId));

		// TODO close connection inside `.dispose` instead (once tests can adapt)
		store..dispose()..connection.close();

		return _apiSendMessage(connection,
		await _apiSendMessage(connection,

		// Async IIFE to not block removing the account on the unregister-token request.
		unawaited(() async {

		// TODO error handling? disable logout button during this?
		await globalStore.removeAccount(accountId);

		/// https://zulip.com/api/remove-fcm-token
		Future<void> removeFcmToken(ApiConnection connection, {

		Future<void> registerNotificationToken() async {
		if (_disposed) return;

login: Support logging out of an account #948

Are you sure you want to change the base?

login: Support logging out of an account #948

Conversation

chrisbobbe commented Sep 17, 2024

chrisbobbe commented Sep 17, 2024

chrisbobbe commented Sep 17, 2024 • edited Loading

chrisbobbe commented Sep 17, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 12, 2024

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 17, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Sep 17, 2024 •

edited

Loading

chrisbobbe commented Sep 17, 2024 •

edited

Loading