diff --git a/lib/model/actions.dart b/lib/model/actions.dart new file mode 100644 index 0000000000..a88eea0777 --- /dev/null +++ b/lib/model/actions.dart @@ -0,0 +1,36 @@ +import 'dart:async'; + +import '../notifications/receive.dart'; +import 'store.dart'; + +// TODO: Make this a part of GlobalStore +Future logOutAccount(GlobalStore globalStore, int accountId) async { + final account = globalStore.getAccount(accountId); + if (account == null) return; // TODO(log) + + // Unawaited, to not block removing the account on this request. + unawaited(unregisterToken(globalStore, accountId)); + + await globalStore.removeAccount(accountId); +} + +Future unregisterToken(GlobalStore globalStore, int accountId) async { + final account = globalStore.getAccount(accountId); + if (account == null) return; // TODO(log) + + // TODO(#322) use actual acked push token; until #322, this is just null. + final token = account.ackedPushToken + // Try the current token as a fallback; maybe the server has registered + // it and we just haven't recorded that fact in the client. + ?? NotificationService.instance.token.value; + if (token == null) return; + + final connection = globalStore.apiConnectionFromAccount(account); + try { + await NotificationService.unregisterToken(connection, token: token); + } catch (e) { + // TODO retry? handle failures? + } finally { + connection.close(); + } +} diff --git a/lib/model/message.dart b/lib/model/message.dart index f228d6da91..84f3bbc3e5 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -61,12 +61,20 @@ class MessageStoreImpl with MessageStore { } void dispose() { - // When a MessageListView is disposed, it removes itself from the Set - // `MessageStoreImpl._messageListViews`. Instead of iterating on that Set, - // iterate on a copy, to avoid concurrent modifications. - for (final view in _messageListViews.toList()) { - view.dispose(); - } + // Not disposing the [MessageListView]s here, because they are owned by + // (i.e., they get [dispose]d by) the [_MessageListState], including in the + // case where the [PerAccountStore] is replaced. + // + // TODO: Add assertions that the [MessageListView]s are indeed disposed, by + // first ensuring that [PerAccountStore] is only disposed after those with + // references to it are disposed, then reinstating this `dispose` method. + // + // We can't add the assertions as-is because the sequence of events + // guarantees that `PerAccountStore` is disposed (when that happens, + // [GlobalStore] notifies its listeners, causing widgets dependent on the + // [InheritedNotifier] to rebuild in the next frame) before the owner's + // `dispose` or `onNewStore` is called. Discussion: + // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893 } @override diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart index 86c6e44875..2df502ec6c 100644 --- a/lib/widgets/actions.dart +++ b/lib/widgets/actions.dart @@ -15,44 +15,9 @@ import '../api/model/narrow.dart'; import '../api/route/messages.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../model/narrow.dart'; -import '../model/store.dart'; -import '../notifications/receive.dart'; import 'dialog.dart'; import 'store.dart'; -Future logOutAccount(BuildContext context, int accountId) async { - final globalStore = GlobalStoreWidget.of(context); - - final account = globalStore.getAccount(accountId); - if (account == null) return; // TODO(log) - - // Unawaited, to not block removing the account on this request. - unawaited(unregisterToken(globalStore, accountId)); - - await globalStore.removeAccount(accountId); -} - -Future unregisterToken(GlobalStore globalStore, int accountId) async { - final account = globalStore.getAccount(accountId); - if (account == null) return; // TODO(log) - - // TODO(#322) use actual acked push token; until #322, this is just null. - final token = account.ackedPushToken - // Try the current token as a fallback; maybe the server has registered - // it and we just haven't recorded that fact in the client. - ?? NotificationService.instance.token.value; - if (token == null) return; - - final connection = globalStore.apiConnectionFromAccount(account); - try { - await NotificationService.unregisterToken(connection, token: token); - } catch (e) { - // TODO retry? handle failures? - } finally { - connection.close(); - } -} - Future markNarrowAsRead(BuildContext context, Narrow narrow) async { final store = PerAccountStoreWidget.of(context); final connection = store.connection; diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 9fa82144a1..2ad35e3e53 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -6,11 +6,11 @@ import 'package:flutter/scheduler.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../log.dart'; +import '../model/actions.dart'; import '../model/localizations.dart'; import '../model/store.dart'; import '../notifications/display.dart'; import 'about_zulip.dart'; -import 'actions.dart'; import 'dialog.dart'; import 'home.dart'; import 'login.dart'; @@ -265,7 +265,7 @@ class ChooseAccountPage extends StatelessWidget { actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton, onActionButtonPress: () { // TODO error handling if db write fails? - logOutAccount(context, accountId); + logOutAccount(GlobalStoreWidget.of(context), accountId); }); }, child: Text(zulipLocalizations.chooseAccountPageLogOutButton)), diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 57259640b1..4d0ff00072 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -483,6 +483,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat @override void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages + model?.dispose(); _initModel(PerAccountStoreWidget.of(context)); } diff --git a/test/model/actions_test.dart b/test/model/actions_test.dart new file mode 100644 index 0000000000..e24bb3223c --- /dev/null +++ b/test/model/actions_test.dart @@ -0,0 +1,188 @@ +import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; +import 'package:zulip/api/exception.dart'; +import 'package:zulip/model/actions.dart'; +import 'package:zulip/model/store.dart'; +import 'package:zulip/notifications/receive.dart'; + +import '../api/fake_api.dart'; +import '../example_data.dart' as eg; +import '../fake_async.dart'; +import '../model/binding.dart'; +import '../model/store_checks.dart'; +import '../model/test_store.dart'; +import '../stdlib_checks.dart'; +import 'store_test.dart'; + +void main() { + TestZulipBinding.ensureInitialized(); + + late PerAccountStore store; + late FakeApiConnection connection; + + Future prepare({String? ackedPushToken = '123'}) async { + addTearDown(testBinding.reset); + final selfAccount = eg.selfAccount.copyWith(ackedPushToken: Value(ackedPushToken)); + await testBinding.globalStore.add(selfAccount, eg.initialSnapshot()); + store = await testBinding.globalStore.perAccount(selfAccount.id); + connection = store.connection as FakeApiConnection; + } + + /// Creates and caches a new [FakeApiConnection] in [TestGlobalStore]. + /// + /// In live code, [unregisterToken] makes a new [ApiConnection] for the + /// unregister-token request instead of reusing the store's connection. + /// To enable callers to prepare responses for that request, this function + /// creates a new [FakeApiConnection] and caches it in [TestGlobalStore] + /// for [unregisterToken] to pick up. + /// + /// Call this instead of just turning on + /// [TestGlobalStore.useCachedApiConnections] so that [unregisterToken] + /// doesn't try to call `close` twice on the same connection instance, + /// which isn't allowed. (Once by the unregister-token code + /// and once as part of removing the account.) + FakeApiConnection separateConnection() { + testBinding.globalStore + ..clearCachedApiConnections() + ..useCachedApiConnections = true; + return testBinding.globalStore + .apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection; + } + + String unregisterApiPathForPlatform(TargetPlatform platform) { + return switch (platform) { + TargetPlatform.android => '/api/v1/users/me/android_gcm_reg_id', + TargetPlatform.iOS => '/api/v1/users/me/apns_device_token', + _ => throw Error(), + }; + } + + void checkSingleUnregisterRequest( + FakeApiConnection connection, { + String? expectedToken, + }) { + final subject = check(connection.takeRequests()).single.isA() + ..method.equals('DELETE') + ..url.path.equals(unregisterApiPathForPlatform(defaultTargetPlatform)); + if (expectedToken != null) { + subject.bodyFields.deepEquals({'token': expectedToken}); + } + } + + group('logOutAccount', () { + test('smoke', () => awaitFakeAsync((async) async { + await prepare(); + check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); + const unregisterDelay = Duration(seconds: 5); + assert(unregisterDelay > TestGlobalStore.removeAccountDuration); + final newConnection = separateConnection() + ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'}); + + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); + // Unregister-token request and account removal dispatched together + checkSingleUnregisterRequest(newConnection); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + async.elapse(TestGlobalStore.removeAccountDuration); + await future; + // Account removal not blocked on unregister-token response + check(testBinding.globalStore).accountIds.isEmpty(); + check(connection.isOpen).isFalse(); + check(newConnection.isOpen).isTrue(); // still busy with unregister-token + + async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration); + check(newConnection.isOpen).isFalse(); + })); + + test('unregister request has an error', () => awaitFakeAsync((async) async { + await prepare(); + check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); + const unregisterDelay = Duration(seconds: 5); + assert(unregisterDelay > TestGlobalStore.removeAccountDuration); + final exception = ZulipApiException( + httpStatus: 401, + code: 'UNAUTHORIZED', + data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, + routeName: 'removeEtcEtcToken', + message: 'Invalid API key', + ); + final newConnection = separateConnection() + ..prepare(delay: unregisterDelay, exception: exception); + + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); + // Unregister-token request and account removal dispatched together + checkSingleUnregisterRequest(newConnection); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + async.elapse(TestGlobalStore.removeAccountDuration); + await future; + // Account removal not blocked on unregister-token response + check(testBinding.globalStore).accountIds.isEmpty(); + check(connection.isOpen).isFalse(); + check(newConnection.isOpen).isTrue(); // for the unregister-token request + + async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration); + check(newConnection.isOpen).isFalse(); + })); + }); + + group('unregisterToken', () { + testAndroidIos('smoke, happy path', () => awaitFakeAsync((async) async { + await prepare(ackedPushToken: '123'); + + final newConnection = separateConnection() + ..prepare(json: {'msg': '', 'result': 'success'}); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + async.elapse(Duration.zero); + await future; + checkSingleUnregisterRequest(newConnection, expectedToken: '123'); + check(newConnection.isOpen).isFalse(); + })); + + test('fallback to current token if acked is missing', () => awaitFakeAsync((async) async { + await prepare(ackedPushToken: null); + NotificationService.instance.token = ValueNotifier('asdf'); + + final newConnection = separateConnection() + ..prepare(json: {'msg': '', 'result': 'success'}); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + async.elapse(Duration.zero); + await future; + checkSingleUnregisterRequest(newConnection, expectedToken: 'asdf'); + check(newConnection.isOpen).isFalse(); + })); + + test('no error if acked token and current token both missing', () => awaitFakeAsync((async) async { + await prepare(ackedPushToken: null); + NotificationService.instance.token = ValueNotifier(null); + + final newConnection = separateConnection(); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + async.flushTimers(); + await future; + check(newConnection.takeRequests()).isEmpty(); + })); + + test('connection closed if request errors', () => awaitFakeAsync((async) async { + await prepare(ackedPushToken: '123'); + + final newConnection = separateConnection() + ..prepare(exception: ZulipApiException( + httpStatus: 401, + code: 'UNAUTHORIZED', + data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, + routeName: 'removeEtcEtcToken', + message: 'Invalid API key', + )); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + async.elapse(Duration.zero); + await future; + checkSingleUnregisterRequest(newConnection, expectedToken: '123'); + check(newConnection.isOpen).isFalse(); + })); + }); +} diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 43f17be61a..9c9a940d42 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -77,18 +77,6 @@ void main() { checkNotified(count: messageList.fetched ? messages.length : 0); } - test('disposing multiple registered MessageListView instances', () async { - // Regression test for: https://github.com/zulip/zulip-flutter/issues/810 - await prepare(narrow: const MentionsNarrow()); - MessageListView.init(store: store, narrow: const StarredMessagesNarrow()); - check(store.debugMessageListViews).length.equals(2); - - // When disposing, the [MessageListView]s are expected to unregister - // themselves from the message store. - store.dispose(); - check(store.debugMessageListViews).isEmpty(); - }); - group('reconcileMessages', () { test('from empty', () async { await prepare(); diff --git a/test/model/store_test.dart b/test/model/store_test.dart index c8c6b4c266..bc393d6d6f 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -12,8 +12,6 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/events.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/realm.dart'; -import 'package:zulip/model/message_list.dart'; -import 'package:zulip/model/narrow.dart'; import 'package:zulip/log.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/receive.dart'; @@ -824,25 +822,6 @@ void main() { checkReload(prepareHandleEventError); }); - test('expired queue disposes registered MessageListView instances', () => awaitFakeAsync((async) async { - // Regression test for: https://github.com/zulip/zulip-flutter/issues/810 - await preparePoll(); - - // Make sure there are [MessageListView]s in the message store. - MessageListView.init(store: store, narrow: const MentionsNarrow()); - MessageListView.init(store: store, narrow: const StarredMessagesNarrow()); - check(store.debugMessageListViews).length.equals(2); - - // Let the server expire the event queue. - prepareExpiredEventQueue(); - updateMachine.debugAdvanceLoop(); - async.elapse(Duration.zero); - - // The old store's [MessageListView]s have been disposed. - // (And no exception was thrown; that was #810.) - check(store.debugMessageListViews).isEmpty(); - })); - group('report error', () { String? lastReportedError; String? takeLastReportedError() { diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart index 5a957c44e0..290d6f3e53 100644 --- a/test/widgets/actions_test.dart +++ b/test/widgets/actions_test.dart @@ -1,13 +1,9 @@ -import 'dart:async'; import 'dart:convert'; import 'package:checks/checks.dart'; -import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; -import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; -import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; @@ -15,20 +11,13 @@ import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; -import 'package:zulip/notifications/receive.dart'; import 'package:zulip/widgets/actions.dart'; -import 'package:zulip/widgets/app.dart'; -import 'package:zulip/widgets/inbox.dart'; -import 'package:zulip/widgets/page.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; -import '../model/store_checks.dart'; -import '../model/test_store.dart'; import '../model/unreads_checks.dart'; import '../stdlib_checks.dart'; -import '../test_navigation.dart'; import 'dialog_checks.dart'; import 'test_app.dart'; @@ -59,228 +48,6 @@ void main() { context = tester.element(find.byType(Placeholder)); } - /// Creates and caches a new [FakeApiConnection] in [TestGlobalStore]. - /// - /// In live code, [unregisterToken] makes a new [ApiConnection] for the - /// unregister-token request instead of reusing the store's connection. - /// To enable callers to prepare responses for that request, this function - /// creates a new [FakeApiConnection] and caches it in [TestGlobalStore] - /// for [unregisterToken] to pick up. - /// - /// Call this instead of just turning on - /// [TestGlobalStore.useCachedApiConnections] so that [unregisterToken] - /// doesn't try to call `close` twice on the same connection instance, - /// which isn't allowed. (Once by the unregister-token code - /// and once as part of removing the account.) - FakeApiConnection separateConnection() { - testBinding.globalStore - ..clearCachedApiConnections() - ..useCachedApiConnections = true; - return testBinding.globalStore - .apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection; - } - - String unregisterApiPathForPlatform(TargetPlatform platform) { - return switch (platform) { - TargetPlatform.android => '/api/v1/users/me/android_gcm_reg_id', - TargetPlatform.iOS => '/api/v1/users/me/apns_device_token', - _ => throw Error(), - }; - } - - void checkSingleUnregisterRequest( - FakeApiConnection connection, { - String? expectedToken, - }) { - final subject = check(connection.takeRequests()).single.isA() - ..method.equals('DELETE') - ..url.path.equals(unregisterApiPathForPlatform(defaultTargetPlatform)); - if (expectedToken != null) { - subject.bodyFields.deepEquals({'token': expectedToken}); - } - } - - group('logOutAccount', () { - testWidgets('smoke', (tester) async { - await prepare(tester, skipAssertAccountExists: true); - check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); - const unregisterDelay = Duration(seconds: 5); - assert(unregisterDelay > TestGlobalStore.removeAccountDuration); - final newConnection = separateConnection() - ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'}); - - final future = logOutAccount(context, eg.selfAccount.id); - // Unregister-token request and account removal dispatched together - checkSingleUnregisterRequest(newConnection); - check(testBinding.globalStore.takeDoRemoveAccountCalls()) - .single.equals(eg.selfAccount.id); - - await tester.pump(TestGlobalStore.removeAccountDuration); - await future; - // Account removal not blocked on unregister-token response - check(testBinding.globalStore).accountIds.isEmpty(); - check(connection.isOpen).isFalse(); - check(newConnection.isOpen).isTrue(); // still busy with unregister-token - - await tester.pump(unregisterDelay - TestGlobalStore.removeAccountDuration); - check(newConnection.isOpen).isFalse(); - }); - - testWidgets('unregister request has an error', (tester) async { - await prepare(tester, skipAssertAccountExists: true); - check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); - const unregisterDelay = Duration(seconds: 5); - assert(unregisterDelay > TestGlobalStore.removeAccountDuration); - final exception = ZulipApiException( - httpStatus: 401, - code: 'UNAUTHORIZED', - data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, - routeName: 'removeEtcEtcToken', - message: 'Invalid API key', - ); - final newConnection = separateConnection() - ..prepare(delay: unregisterDelay, exception: exception); - - final future = logOutAccount(context, eg.selfAccount.id); - // Unregister-token request and account removal dispatched together - checkSingleUnregisterRequest(newConnection); - check(testBinding.globalStore.takeDoRemoveAccountCalls()) - .single.equals(eg.selfAccount.id); - - await tester.pump(TestGlobalStore.removeAccountDuration); - await future; - // Account removal not blocked on unregister-token response - check(testBinding.globalStore).accountIds.isEmpty(); - check(connection.isOpen).isFalse(); - check(newConnection.isOpen).isTrue(); // for the unregister-token request - - await tester.pump(unregisterDelay - TestGlobalStore.removeAccountDuration); - check(newConnection.isOpen).isFalse(); - }); - - testWidgets("logged-out account's routes removed from nav; other accounts' remain", (tester) async { - Future makeUnreadTopicInInbox(int accountId, String topic) async { - final stream = eg.stream(); - final message = eg.streamMessage(stream: stream, topic: topic); - final store = await testBinding.globalStore.perAccount(accountId); - await store.addStream(stream); - await store.addSubscription(eg.subscription(stream)); - await store.addMessage(message); - await tester.pump(); - } - - addTearDown(testBinding.reset); - - final account1 = eg.account(id: 1, user: eg.user()); - final account2 = eg.account(id: 2, user: eg.user()); - await testBinding.globalStore.add(account1, eg.initialSnapshot()); - await testBinding.globalStore.add(account2, eg.initialSnapshot()); - - final testNavObserver = TestNavigatorObserver(); - await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); - await tester.pump(); - final navigator = await ZulipApp.navigator; - navigator.popUntil((_) => false); // clear starting routes - await tester.pumpAndSettle(); - - final pushedRoutes = >[]; - testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); - // TODO(#737): switch to a realistic setup: - // https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363 - final account1Route = MaterialAccountWidgetRoute( - accountId: account1.id, page: const InboxPageBody()); - final account2Route = MaterialAccountWidgetRoute( - accountId: account2.id, page: const InboxPageBody()); - unawaited(navigator.push(account1Route)); - unawaited(navigator.push(account2Route)); - await tester.pumpAndSettle(); - check(pushedRoutes).deepEquals([account1Route, account2Route]); - - await makeUnreadTopicInInbox(account1.id, 'topic in account1'); - final findAccount1PageContent = find.text('topic in account1', skipOffstage: false); - - await makeUnreadTopicInInbox(account2.id, 'topic in account2'); - final findAccount2PageContent = find.text('topic in account2', skipOffstage: false); - - final findLoadingPage = find.byType(LoadingPlaceholderPage, skipOffstage: false); - - check(findAccount1PageContent).findsOne(); - check(findLoadingPage).findsNothing(); - - final removedRoutes = >[]; - testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); - - final context = tester.element(find.byType(MaterialApp)); - final future = logOutAccount(context, account1.id); - await tester.pump(TestGlobalStore.removeAccountDuration); - await future; - check(removedRoutes).single.identicalTo(account1Route); - check(findAccount1PageContent).findsNothing(); - check(findLoadingPage).findsOne(); - - await tester.pump(); - check(findAccount1PageContent).findsNothing(); - check(findLoadingPage).findsNothing(); - check(findAccount2PageContent).findsOne(); - }); - }); - - group('unregisterToken', () { - testWidgets('smoke, happy path', (tester) async { - await prepare(tester, ackedPushToken: '123'); - - final newConnection = separateConnection() - ..prepare(json: {'msg': '', 'result': 'success'}); - final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); - await tester.pump(Duration.zero); - await future; - checkSingleUnregisterRequest(newConnection, expectedToken: '123'); - check(newConnection.isOpen).isFalse(); - }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); - - testWidgets('fallback to current token if acked is missing', (tester) async { - await prepare(tester, ackedPushToken: null); - NotificationService.instance.token = ValueNotifier('asdf'); - - final newConnection = separateConnection() - ..prepare(json: {'msg': '', 'result': 'success'}); - final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); - await tester.pump(Duration.zero); - await future; - checkSingleUnregisterRequest(newConnection, expectedToken: 'asdf'); - check(newConnection.isOpen).isFalse(); - }); - - testWidgets('no error if acked token and current token both missing', (tester) async { - await prepare(tester, ackedPushToken: null); - NotificationService.instance.token = ValueNotifier(null); - - final newConnection = separateConnection(); - final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); - await tester.pumpAndSettle(); - await future; - check(newConnection.takeRequests()).isEmpty(); - }); - - testWidgets('connection closed if request errors', (tester) async { - await prepare(tester, ackedPushToken: '123'); - - final newConnection = separateConnection() - ..prepare(exception: ZulipApiException( - httpStatus: 401, - code: 'UNAUTHORIZED', - data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, - routeName: 'removeEtcEtcToken', - message: 'Invalid API key', - )); - final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); - await tester.pump(Duration.zero); - await future; - checkSingleUnregisterRequest(newConnection, expectedToken: '123'); - check(newConnection.isOpen).isFalse(); - }); - }); - group('markNarrowAsRead', () { testWidgets('smoke test on modern server', (tester) async { final narrow = TopicNarrow.ofMessage(eg.streamMessage()); diff --git a/test/widgets/home_test.dart b/test/widgets/home_test.dart index 5bb789727f..1525355766 100644 --- a/test/widgets/home_test.dart +++ b/test/widgets/home_test.dart @@ -3,10 +3,10 @@ import 'package:flutter/material.dart'; import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/api/model/events.dart'; +import 'package:zulip/model/actions.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/about_zulip.dart'; -import 'package:zulip/widgets/actions.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/app_bar.dart'; import 'package:zulip/widgets/home.dart'; @@ -453,8 +453,7 @@ void main () { await tester.pump(); // wait for the loading page checkOnLoadingPage(); - final element = tester.element(find.byType(MaterialApp)); - final future = logOutAccount(element, eg.selfAccount.id); + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); await tester.pump(TestGlobalStore.removeAccountDuration); await future; // No error expected from briefly not having @@ -471,8 +470,7 @@ void main () { await tester.pump(); // wait for store checkOnHomePage(tester, expectedAccount: eg.selfAccount); - final element = tester.element(find.byType(HomePage)); - final future = logOutAccount(element, eg.selfAccount.id); + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); await tester.pump(TestGlobalStore.removeAccountDuration); await future; // No error expected from briefly not having diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index b3cc208463..3f79f8cae6 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -7,11 +7,13 @@ import 'package:flutter/rendering.dart'; import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; import 'package:zulip/api/route/messages.dart'; +import 'package:zulip/model/actions.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; @@ -56,6 +58,7 @@ void main() { List? subscriptions, UnreadMessagesSnapshot? unreadMsgs, List navObservers = const [], + bool skipAssertAccountExists = false, }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); @@ -77,6 +80,7 @@ void main() { eg.newestGetMessagesResult(foundOldest: foundOldest, messages: messages).toJson()); await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + skipAssertAccountExists: skipAssertAccountExists, navigatorObservers: navObservers, child: MessageListPage(initNarrow: narrow))); @@ -130,6 +134,47 @@ void main() { final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); check(state.composeBoxController).isNull(); }); + + testWidgets('dispose MessageListView when event queue expired', (tester) async { + final message = eg.streamMessage(); + await setupMessageListPage(tester, messages: [message]); + final oldViewModel = store.debugMessageListViews.single; + final updateMachine = store.updateMachine!; + updateMachine.debugPauseLoop(); + updateMachine.poll(); + + updateMachine.debugPrepareLoopError(ZulipApiException( + routeName: 'events', httpStatus: 400, code: 'BAD_EVENT_QUEUE_ID', + data: {'queue_id': updateMachine.queueId}, message: 'Bad event queue ID.')); + updateMachine.debugAdvanceLoop(); + await tester.pump(); + // Event queue has been replaced; but the [MessageList] hasn't been + // rebuilt yet. + final newStore = testBinding.globalStore.perAccountSync(eg.selfAccount.id)!; + check(connection.isOpen).isFalse(); // indicates that the old store has been disposed + check(store.debugMessageListViews).single.equals(oldViewModel); + check(newStore.debugMessageListViews).isEmpty(); + + (newStore.connection as FakeApiConnection).prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: [message]).toJson()); + await tester.pump(); + await tester.pump(Duration.zero); + // As [MessageList] rebuilds, the old view model gets disposed and + // replaced with a fresh one. + check(store.debugMessageListViews).isEmpty(); + check(newStore.debugMessageListViews).single.not((it) => it.equals(oldViewModel)); + }); + + testWidgets('dispose MessageListView when logged out', (tester) async { + await setupMessageListPage(tester, + messages: [eg.streamMessage()], skipAssertAccountExists: true); + check(store.debugMessageListViews).single; + + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + check(store.debugMessageListViews).isEmpty(); + }); }); group('app bar', () { diff --git a/test/widgets/store_test.dart b/test/widgets/store_test.dart index e2d7821ae3..7e70f01a19 100644 --- a/test/widgets/store_test.dart +++ b/test/widgets/store_test.dart @@ -1,13 +1,22 @@ +import 'dart:async'; + import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; +import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/model/actions.dart'; import 'package:zulip/model/store.dart'; +import 'package:zulip/widgets/app.dart'; +import 'package:zulip/widgets/inbox.dart'; +import 'package:zulip/widgets/page.dart'; import 'package:zulip/widgets/store.dart'; import '../flutter_checks.dart'; import '../model/binding.dart'; import '../example_data.dart' as eg; import '../model/store_checks.dart'; +import '../model/test_store.dart'; +import '../test_navigation.dart'; /// A widget whose state uses [PerAccountStoreAwareStateMixin]. class MyWidgetWithMixin extends StatefulWidget { @@ -167,6 +176,71 @@ void main() { tester.widget(find.text('found store, account: ${eg.selfAccount.id}')); }); + testWidgets("PerAccountStoreWidget.routeToRemoveOnLogout logged-out account's routes removed from nav; other accounts' remain", (tester) async { + Future makeUnreadTopicInInbox(int accountId, String topic) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream, topic: topic); + final store = await testBinding.globalStore.perAccount(accountId); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await store.addMessage(message); + await tester.pump(); + } + + addTearDown(testBinding.reset); + + final account1 = eg.account(id: 1, user: eg.user()); + final account2 = eg.account(id: 2, user: eg.user()); + await testBinding.globalStore.add(account1, eg.initialSnapshot()); + await testBinding.globalStore.add(account2, eg.initialSnapshot()); + + final testNavObserver = TestNavigatorObserver(); + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); + final navigator = await ZulipApp.navigator; + navigator.popUntil((_) => false); // clear starting routes + await tester.pumpAndSettle(); + + final pushedRoutes = >[]; + testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); + // TODO(#737): switch to a realistic setup: + // https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363 + final account1Route = MaterialAccountWidgetRoute( + accountId: account1.id, page: const InboxPageBody()); + final account2Route = MaterialAccountWidgetRoute( + accountId: account2.id, page: const InboxPageBody()); + unawaited(navigator.push(account1Route)); + unawaited(navigator.push(account2Route)); + await tester.pumpAndSettle(); + check(pushedRoutes).deepEquals([account1Route, account2Route]); + + await makeUnreadTopicInInbox(account1.id, 'topic in account1'); + final findAccount1PageContent = find.text('topic in account1', skipOffstage: false); + + await makeUnreadTopicInInbox(account2.id, 'topic in account2'); + final findAccount2PageContent = find.text('topic in account2', skipOffstage: false); + + final findLoadingPage = find.byType(LoadingPlaceholderPage, skipOffstage: false); + + check(findAccount1PageContent).findsOne(); + check(findLoadingPage).findsNothing(); + + final removedRoutes = >[]; + testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); + + final future = logOutAccount(testBinding.globalStore, account1.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + check(removedRoutes).single.identicalTo(account1Route); + check(findAccount1PageContent).findsNothing(); + check(findLoadingPage).findsOne(); + + await tester.pump(); + check(findAccount1PageContent).findsNothing(); + check(findLoadingPage).findsNothing(); + check(findAccount2PageContent).findsOne(); + }); + testWidgets('PerAccountStoreAwareStateMixin', (tester) async { final widgetWithMixinKey = GlobalKey(); final accountId = eg.selfAccount.id;