store: Implement removeAccount

Author: chrisbobbe

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;
   final Map<int, PerAccountStore> _perAccountStores = {};
+
+  Map<int, Future<PerAccountStore>> get debugPerAccountStoresLoading => _perAccountStoresLoading;
   final Map<int, Future<PerAccountStore>> _perAccountStoresLoading = {};
 
   /// The store's per-account data for the given account, if already loaded.
@@ -142,8 +145,16 @@ abstract class GlobalStore extends ChangeNotifier {
   /// This method should be called only by the implementation of [perAccount].
   /// Other callers interested in per-account data should use [perAccount]
   /// and/or [perAccountSync].
-  Future<PerAccountStore> loadPerAccount(int accountId) {
-    return doLoadPerAccount(accountId);
+  Future<PerAccountStore> loadPerAccount(int accountId) async {
+    assert(_accounts.containsKey(accountId));
+    final store = await doLoadPerAccount(accountId);
+    if (!_accounts.containsKey(accountId)) {
+      // [removeAccount] was called during [doLoadPerAccount].
+      // TODO close connection inside `.dispose` instead (once tests can adapt)
+      store..dispose()..connection.close();
+      throw AccountNotFoundException();
+    }
+    return store;
   }
 
   /// Load per-account data for the given account, unconditionally.
@@ -197,10 +208,27 @@ abstract class GlobalStore extends ChangeNotifier {
   /// Update an account in the underlying data store.
   Future<void> doUpdateAccount(int accountId, AccountsCompanion data);
 
+  /// Remove an account from the store.
+  Future<void> removeAccount(int accountId) async {
+    await doRemoveAccount(accountId);
+    assert(_accounts.containsKey(accountId));
+    _accounts.remove(accountId);
+    _perAccountStores.remove(accountId)
+      // TODO close connection inside `.dispose` instead (once tests can adapt)
+      ?..dispose()..connection.close();
+    _perAccountStoresLoading.remove(accountId);
+    notifyListeners();
+  }
+
+  /// Remove an account from the underlying data store.
+  Future<void> doRemoveAccount(int accountId);
+
   @override
   String toString() => '${objectRuntimeType(this, 'GlobalStore')}#${shortHash(this)}';
 }
 
+class AccountNotFoundException implements Exception {}
+
 /// Store for the user's data for a given Zulip account.
 ///
 /// This should always have a consistent snapshot of the state on the server,
@@ -337,8 +365,17 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
   // Data attached to the self-account on the realm.
 
   final int accountId;
+
+  /// The [Account] this store belongs to.
+  ///
+  /// Assumes the account hasn't been logged out.
+  /// If the account may have been logged out, don't use this;
+  /// use [maybeAccount] instead.
   Account get account => _globalStore.getAccount(accountId)!;
 
+  /// Like [account], but for contexts where the account might be logged out.
+  Account? get maybeAccount => _globalStore.getAccount(accountId);
+
   /// Always equal to `account.userId`.
   final int selfUserId;
 
@@ -426,6 +463,9 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
   }
 
   Future<void> handleEvent(Event event) async {
+    // Do nothing if the account has been logged out.
+    if (maybeAccount == null) return;
+
     switch (event) {
       case HeartbeatEvent():
         assert(debugLog("server event: heartbeat"));
@@ -566,10 +606,13 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
     }
   }
 
-  Future<void> sendMessage({required MessageDestination destination, required String content}) {
+  Future<void> sendMessage({required MessageDestination destination, required String content}) async {
+    // Do nothing if the account has been logged out.
+    if (maybeAccount == null) return;
+
     // TODO implement outbox; see design at
     //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739
-    return _apiSendMessage(connection,
+    await _apiSendMessage(connection,
       destination: destination,
       content: content,
       readBySender: true,
@@ -685,6 +728,14 @@ class LiveGlobalStore extends GlobalStore {
     assert(rowsAffected == 1);
   }
 
+  @override
+  Future<void> doRemoveAccount(int accountId) async {
+    final rowsAffected = await (_db.delete(_db.accounts)
+      ..where((a) => a.id.equals(accountId))
+    ).go();
+    assert(rowsAffected == 1);
+  }
+
   @override
   String toString() => '${objectRuntimeType(this, 'LiveGlobalStore')}#${shortHash(this)}';
 }
@@ -793,11 +844,17 @@ class UpdateMachine {
         }());
       }
 
+      // Abort if the account has been logged out.
+      if (store.maybeAccount == null) return;
+
       final GetEventsResult result;
       try {
         result = await getEvents(store.connection,
           queueId: queueId, lastEventId: lastEventId);
       } catch (e) {
+        // Abort if the account has been logged out.
+        if (store.maybeAccount == null) return;
+
         store.isLoading = true;
         switch (e) {
           case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
@@ -825,6 +882,9 @@ class UpdateMachine {
         }
       }
 
+      // Abort if the account has been logged out.
+      if (store.maybeAccount == null) return;
+
       // After one successful request, we reset backoff to its initial state.
       // That way if the user is off the network and comes back on, the app
       // doesn't wind up in a state where it's slow to recover the next time
@@ -847,6 +907,10 @@ class UpdateMachine {
       for (final event in events) {
         await store.handleEvent(event);
       }
+
+      // Abort if the account has been logged out.
+      if (store.maybeAccount == null) return;
+
       if (events.isNotEmpty) {
         lastEventId = events.last.id;
       }
@@ -881,6 +945,9 @@ class UpdateMachine {
   // TODO(#322) save acked token, to dedupe updating it on the server
   // TODO(#323) track the registerFcmToken/etc request, warn if not succeeding
   Future<void> registerNotificationToken() async {
+    // Abort if the account has been logged out.
+    if (store.maybeAccount == null) return;
+
     if (!debugEnableRegisterNotificationToken) {
       return;
     }

lib/widgets/store.dart

@@ -202,11 +202,21 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
       _setStore(store);
     } else {
       // If we don't already have data, request it.
-
-      // If this succeeds, globalStore will notify listeners, and
-      // [didChangeDependencies] will run again, this time in the
-      // `store != null` case above.
-      globalStore.perAccount(widget.accountId);
+      () async {
+        try {
+          // If this succeeds, globalStore will notify listeners, and
+          // [didChangeDependencies] will run again, this time in the
+          // `store != null` case above.
+          await globalStore.perAccount(widget.accountId);
+        } on AccountNotFoundException {
+          // The account was logged out while its store was loading.
+          // This widget will be showing [placeholder] perpetually,
+          // but that's OK as long as other code will be removing it from the UI
+          // (for example by removing a per-account route from the nav).
+        } catch (e) {
+          // TODO(log)
+        }
+      }();
     }
   }
 

test/model/store_test.dart

@@ -164,6 +164,88 @@ void main() {
     // TODO test database gets updated correctly (an integration test with sqlite?)
   });
 
+  group('GlobalStore.removeAccount', () {
+    void checkGlobalStore(GlobalStore store, int accountId, {
+      required bool expectAccount,
+      required bool expectStore,
+      bool expectStoreLoading = false,
+    }) {
+      expectAccount
+        ? check(store.getAccount(accountId)).isNotNull()
+        : check(store.getAccount(accountId)).isNull();
+      expectStore
+        ? check(store.debugPerAccountStores[accountId]).isNotNull()
+        : check(store.debugPerAccountStores[accountId]).isNull();
+      expectStoreLoading
+        ? check(store.debugPerAccountStoresLoading[accountId]).isNotNull()
+        : check(store.debugPerAccountStoresLoading[accountId]).isNull();
+    }
+
+    test('when store loaded', () async {
+      final globalStore = eg.globalStore();
+      await globalStore.add(eg.selfAccount, eg.initialSnapshot());
+      await globalStore.perAccount(eg.selfAccount.id);
+
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: true, expectStore: true);
+      int notifyCount = 0;
+      globalStore.addListener(() => notifyCount++);
+
+      await globalStore.removeAccount(eg.selfAccount.id);
+
+      // TODO test that the removed store got disposed and its connection closed
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: false, expectStore: false);
+      check(notifyCount).equals(1);
+    });
+
+    test('when store not loaded', () async {
+      final globalStore = eg.globalStore();
+      await globalStore.add(eg.selfAccount, eg.initialSnapshot());
+
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: true, expectStore: false);
+      int notifyCount = 0;
+      globalStore.addListener(() => notifyCount++);
+
+      await globalStore.removeAccount(eg.selfAccount.id);
+
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: false, expectStore: false);
+      check(notifyCount).equals(1);
+    });
+
+    test('when store loading', () async {
+      final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: true, expectStore: false, expectStoreLoading: false);
+
+      // don't await; we'll complete/await it manually after removeAccount
+      final loadingFuture = globalStore.perAccount(eg.selfAccount.id);
+
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: true, expectStore: false, expectStoreLoading: true);
+      int notifyCount = 0;
+      globalStore.addListener(() => notifyCount++);
+
+      await globalStore.removeAccount(eg.selfAccount.id);
+
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: false, expectStore: false, expectStoreLoading: false);
+      check(notifyCount).equals(1);
+
+      globalStore.completers[eg.selfAccount.id]!.single
+        .complete(eg.store(account: eg.selfAccount, initialSnapshot: eg.initialSnapshot()));
+      // TODO test that the never-used store got disposed and its connection closed
+      check(loadingFuture).throws<AccountNotFoundException>();
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: false, expectStore: false, expectStoreLoading: false);
+      check(notifyCount).equals(1); // no extra notify
+    });
+
+    // TODO test database gets updated correctly (an integration test with sqlite?)
+  });
+
   group('PerAccountStore.handleEvent', () {
     // Mostly this method just dispatches to ChannelStore and MessageStore etc.,
     // and so most of the tests live in the test files for those

test/model/test_store.dart

@@ -105,6 +105,11 @@ class TestGlobalStore extends GlobalStore {
     // Nothing to do.
   }
 
+  @override
+  Future<void> doRemoveAccount(int accountId) async {
+    // Nothing to do.
+  }
+
   @override
   Future<PerAccountStore> doLoadPerAccount(int accountId) {
     final initialSnapshot = _initialSnapshots[accountId]!;