Skip to content

Commit a271863

Browse files
chrisbobbechrisbobbe
committed
store: Implement removeAccount
1 parent 5e7d2f5 commit a271863

File tree

4 files changed

+160
-9
lines changed

4 files changed

+160
-9
lines changed

‎lib/model/store.dart

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ abstract class GlobalStore extends ChangeNotifier {
7878
}
7979

8080
final Map<int, PerAccountStore> _perAccountStores = {};
81+
82+
int get debugNumPerAccountStoresLoading => _perAccountStoresLoading.length;
8183
final Map<int, Future<PerAccountStore>> _perAccountStoresLoading = {};
8284

8385
/// The store's per-account data for the given account, if already loaded.
@@ -144,8 +146,16 @@ abstract class GlobalStore extends ChangeNotifier {
144146
/// This method should be called only by the implementation of [perAccount].
145147
/// Other callers interested in per-account data should use [perAccount]
146148
/// and/or [perAccountSync].
147-
Future<PerAccountStore> loadPerAccount(int accountId) {
148-
return doLoadPerAccount(accountId);
149+
Future<PerAccountStore> loadPerAccount(int accountId) async {
150+
assert(_accounts.containsKey(accountId));
151+
final store = await doLoadPerAccount(accountId);
152+
if (!_accounts.containsKey(accountId)) {
153+
// [removeAccount] was called during [doLoadPerAccount].
154+
// TODO close connection inside `.dispose` instead (once tests can adapt)
155+
store..dispose()..connection.close();
156+
throw AccountNotFoundException();
157+
}
158+
return store;
149159
}
150160

151161
/// Load per-account data for the given account, unconditionally.
@@ -199,10 +209,27 @@ abstract class GlobalStore extends ChangeNotifier {
199209
/// Update an account in the underlying data store.
200210
Future<void> doUpdateAccount(int accountId, AccountsCompanion data);
201211

212+
/// Remove an account from the store.
213+
Future<void> removeAccount(int accountId) async {
214+
await doRemoveAccount(accountId);
215+
assert(_accounts.containsKey(accountId));
216+
_accounts.remove(accountId);
217+
_perAccountStores.remove(accountId)
218+
// TODO close connection inside `.dispose` instead (once tests can adapt)
219+
?..dispose()..connection.close();
220+
unawaited(_perAccountStoresLoading.remove(accountId));
221+
notifyListeners();
222+
}
223+
224+
/// Remove an account from the underlying data store.
225+
Future<void> doRemoveAccount(int accountId);
226+
202227
@override
203228
String toString() => '${objectRuntimeType(this, 'GlobalStore')}#${shortHash(this)}';
204229
}
205230

231+
class AccountNotFoundException implements Exception {}
232+
206233
/// Store for the user's data for a given Zulip account.
207234
///
208235
/// This should always have a consistent snapshot of the state on the server,
@@ -375,6 +402,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
375402
// Data attached to the self-account on the realm.
376403

377404
final int accountId;
405+
406+
/// The [Account] this store belongs to.
407+
///
408+
/// Will throw if called after [dispose] has been called.
378409
Account get account => _globalStore.getAccount(accountId)!;
379410

380411
/// Always equal to `account.userId`.
@@ -444,6 +475,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
444475
// End of data.
445476
////////////////////////////////////////////////////////////////
446477
478+
bool _disposed = false;
479+
447480
/// Called when the app is reassembled during debugging, e.g. for hot reload.
448481
///
449482
/// This will redo from scratch any computations we can, such as parsing
@@ -461,9 +494,12 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
461494
typingStatus.dispose();
462495
updateMachine?.dispose(); // TODO is updateMachine ever null except in tests?
463496
super.dispose();
497+
_disposed = true;
464498
}
465499

466500
Future<void> handleEvent(Event event) async {
501+
assert(!_disposed);
502+
467503
switch (event) {
468504
case HeartbeatEvent():
469505
assert(debugLog("server event: heartbeat"));
@@ -604,10 +640,12 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
604640
}
605641
}
606642

607-
Future<void> sendMessage({required MessageDestination destination, required String content}) {
643+
Future<void> sendMessage({required MessageDestination destination, required String content}) async {
644+
assert(!_disposed);
645+
608646
// TODO implement outbox; see design at
609647
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739
610-
return _apiSendMessage(connection,
648+
await _apiSendMessage(connection,
611649
destination: destination,
612650
content: content,
613651
readBySender: true,
@@ -723,6 +761,14 @@ class LiveGlobalStore extends GlobalStore {
723761
assert(rowsAffected == 1);
724762
}
725763

764+
@override
765+
Future<void> doRemoveAccount(int accountId) async {
766+
final rowsAffected = await (_db.delete(_db.accounts)
767+
..where((a) => a.id.equals(accountId))
768+
).go();
769+
assert(rowsAffected == 1);
770+
}
771+
726772
@override
727773
String toString() => '${objectRuntimeType(this, 'LiveGlobalStore')}#${shortHash(this)}';
728774
}
@@ -789,6 +835,8 @@ class UpdateMachine {
789835
final String queueId;
790836
int lastEventId;
791837

838+
bool _disposed = false;
839+
792840
static Future<InitialSnapshot> _registerQueueWithRetry(
793841
ApiConnection connection) async {
794842
BackoffMachine? backoffMachine;
@@ -875,11 +923,15 @@ class UpdateMachine {
875923
}());
876924
}
877925

926+
if (_disposed) return;
927+
878928
final GetEventsResult result;
879929
try {
880930
result = await getEvents(store.connection,
881931
queueId: queueId, lastEventId: lastEventId);
882932
} catch (e) {
933+
if (_disposed) return;
934+
883935
store.isLoading = true;
884936
switch (e) {
885937
case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
@@ -907,6 +959,8 @@ class UpdateMachine {
907959
}
908960
}
909961

962+
if (_disposed) return;
963+
910964
// After one successful request, we reset backoff to its initial state.
911965
// That way if the user is off the network and comes back on, the app
912966
// doesn't wind up in a state where it's slow to recover the next time
@@ -928,6 +982,7 @@ class UpdateMachine {
928982
final events = result.events;
929983
for (final event in events) {
930984
await store.handleEvent(event);
985+
if (_disposed) return;
931986
}
932987
if (events.isNotEmpty) {
933988
lastEventId = events.last.id;
@@ -943,6 +998,8 @@ class UpdateMachine {
943998
// TODO(#322) save acked token, to dedupe updating it on the server
944999
// TODO(#323) track the registerFcmToken/etc request, warn if not succeeding
9451000
Future<void> registerNotificationToken() async {
1001+
if (_disposed) return;
1002+
9461003
if (!debugEnableRegisterNotificationToken) {
9471004
return;
9481005
}
@@ -958,6 +1015,7 @@ class UpdateMachine {
9581015

9591016
void dispose() { // TODO abort long-poll and close ApiConnection
9601017
NotificationService.instance.token.removeListener(_registerNotificationToken);
1018+
_disposed = true;
9611019
}
9621020

9631021
/// In debug mode, controls whether [fetchEmojiData] should

‎lib/widgets/store.dart

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -202,11 +202,21 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
202202
_setStore(store);
203203
} else {
204204
// If we don't already have data, request it.
205-
206-
// If this succeeds, globalStore will notify listeners, and
207-
// [didChangeDependencies] will run again, this time in the
208-
// `store != null` case above.
209-
globalStore.perAccount(widget.accountId);
205+
() async {
206+
try {
207+
// If this succeeds, globalStore will notify listeners, and
208+
// [didChangeDependencies] will run again, this time in the
209+
// `store != null` case above.
210+
await globalStore.perAccount(widget.accountId);
211+
} on AccountNotFoundException {
212+
// The account was logged out while its store was loading.
213+
// This widget will be showing [placeholder] perpetually,
214+
// but that's OK as long as other code will be removing it from the UI
215+
// (for example by removing a per-account route from the nav).
216+
} catch (e) {
217+
// TODO(log)
218+
}
219+
}();
210220
}
211221
}
212222

‎test/model/store_test.dart

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,84 @@ void main() {
166166
// TODO test database gets updated correctly (an integration test with sqlite?)
167167
});
168168

169+
group('GlobalStore.removeAccount', () {
170+
void checkGlobalStore(GlobalStore store, int accountId, {
171+
required bool expectAccount,
172+
required bool expectStore,
173+
}) {
174+
expectAccount
175+
? check(store.getAccount(accountId)).isNotNull()
176+
: check(store.getAccount(accountId)).isNull();
177+
expectStore
178+
? check(store.perAccountSync(accountId)).isNotNull()
179+
: check(store.perAccountSync(accountId)).isNull();
180+
}
181+
182+
test('when store loaded', () async {
183+
final globalStore = eg.globalStore();
184+
await globalStore.add(eg.selfAccount, eg.initialSnapshot());
185+
await globalStore.perAccount(eg.selfAccount.id);
186+
187+
checkGlobalStore(globalStore, eg.selfAccount.id,
188+
expectAccount: true, expectStore: true);
189+
int notifyCount = 0;
190+
globalStore.addListener(() => notifyCount++);
191+
192+
await globalStore.removeAccount(eg.selfAccount.id);
193+
194+
// TODO test that the removed store got disposed and its connection closed
195+
checkGlobalStore(globalStore, eg.selfAccount.id,
196+
expectAccount: false, expectStore: false);
197+
check(notifyCount).equals(1);
198+
});
199+
200+
test('when store not loaded', () async {
201+
final globalStore = eg.globalStore();
202+
await globalStore.add(eg.selfAccount, eg.initialSnapshot());
203+
204+
checkGlobalStore(globalStore, eg.selfAccount.id,
205+
expectAccount: true, expectStore: false);
206+
int notifyCount = 0;
207+
globalStore.addListener(() => notifyCount++);
208+
209+
await globalStore.removeAccount(eg.selfAccount.id);
210+
211+
checkGlobalStore(globalStore, eg.selfAccount.id,
212+
expectAccount: false, expectStore: false);
213+
check(notifyCount).equals(1);
214+
});
215+
216+
test('when store loading', () async {
217+
final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
218+
checkGlobalStore(globalStore, eg.selfAccount.id,
219+
expectAccount: true, expectStore: false);
220+
221+
// don't await; we'll complete/await it manually after removeAccount
222+
final loadingFuture = globalStore.perAccount(eg.selfAccount.id);
223+
224+
checkGlobalStore(globalStore, eg.selfAccount.id,
225+
expectAccount: true, expectStore: false);
226+
int notifyCount = 0;
227+
globalStore.addListener(() => notifyCount++);
228+
229+
await globalStore.removeAccount(eg.selfAccount.id);
230+
231+
checkGlobalStore(globalStore, eg.selfAccount.id,
232+
expectAccount: false, expectStore: false);
233+
check(notifyCount).equals(1);
234+
235+
globalStore.completers[eg.selfAccount.id]!.single
236+
.complete(eg.store(account: eg.selfAccount, initialSnapshot: eg.initialSnapshot()));
237+
// TODO test that the never-used store got disposed and its connection closed
238+
await check(loadingFuture).throws<AccountNotFoundException>();
239+
checkGlobalStore(globalStore, eg.selfAccount.id,
240+
expectAccount: false, expectStore: false);
241+
check(notifyCount).equals(1); // no extra notify
242+
243+
check(globalStore.debugNumPerAccountStoresLoading).equals(0);
244+
});
245+
});
246+
169247
group('PerAccountStore.handleEvent', () {
170248
// Mostly this method just dispatches to ChannelStore and MessageStore etc.,
171249
// and so most of the tests live in the test files for those

‎test/model/test_store.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,11 @@ class TestGlobalStore extends GlobalStore {
106106
// Nothing to do.
107107
}
108108

109+
@override
110+
Future<void> doRemoveAccount(int accountId) async {
111+
// Nothing to do.
112+
}
113+
109114
@override
110115
Future<PerAccountStore> doLoadPerAccount(int accountId) {
111116
final initialSnapshot = _initialSnapshots[accountId]!;

0 commit comments

Comments
 (0)