store: Implement removeAccount

Author: chrisbobbe

lib/model/store.dart

@@ -78,6 +78,8 @@ abstract class GlobalStore extends ChangeNotifier {
   }
 
   final Map<int, PerAccountStore> _perAccountStores = {};
+
+  int get debugNumPerAccountStoresLoading => _perAccountStoresLoading.length;
   final Map<int, Future<PerAccountStore>> _perAccountStoresLoading = {};
 
   /// The store's per-account data for the given account, if already loaded.
@@ -144,8 +146,15 @@ 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].
+      store.dispose();
+      throw AccountNotFoundException();
+    }
+    return store;
   }
 
   /// Load per-account data for the given account, unconditionally.
@@ -199,10 +208,26 @@ 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 {
+    assert(_accounts.containsKey(accountId));
+    await doRemoveAccount(accountId);
+    if (!_accounts.containsKey(accountId)) return; // Already removed.
+    _accounts.remove(accountId);
+    _perAccountStores.remove(accountId)?.dispose();
+    unawaited(_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,
@@ -376,6 +401,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
   // Data attached to the self-account on the realm.
 
   final int accountId;
+
+  /// The [Account] this store belongs to.
+  ///
+  /// Will throw if called after [dispose] has been called.
   Account get account => _globalStore.getAccount(accountId)!;
 
   /// Always equal to `account.userId`.
@@ -733,6 +762,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)}';
 }

lib/widgets/store.dart

@@ -202,11 +202,19 @@ 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).
+        }
+      }();
     }
   }
 

test/model/store_test.dart

@@ -166,6 +166,84 @@ 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,
+    }) {
+      expectAccount
+        ? check(store.getAccount(accountId)).isNotNull()
+        : check(store.getAccount(accountId)).isNull();
+      expectStore
+        ? check(store.perAccountSync(accountId)).isNotNull()
+        : check(store.perAccountSync(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);
+
+      // 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);
+      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);
+
+      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
+      await check(loadingFuture).throws<AccountNotFoundException>();
+      checkGlobalStore(globalStore, eg.selfAccount.id,
+        expectAccount: false, expectStore: false);
+      check(notifyCount).equals(1); // no extra notify
+
+      check(globalStore.debugNumPerAccountStoresLoading).equals(0);
+    });
+  });
+
   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

@@ -119,6 +119,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]!;