From 21a347c70a3e392c3e93b3ba01832f59e3e8dfda Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Sun, 29 Dec 2024 20:02:59 -0500 Subject: [PATCH 1/4] home: Stop assuming account existence from loading page We could pass realmUrl when initializing the `_LoadingPlaceholderPage`, but that will still require a check for the existence of the account. The loading page will contain a CircularLoadingIndicator when the account does not exist. The user can't easily reach this page because they can only logout from `ChooseAccountPage`, until we start invalidating API keys. Even then, they will only see the page briefly before getting navigated, so we should not show any text at all. Fixes: #1219 Signed-off-by: Zixuan James Li --- lib/widgets/home.dart | 19 +++++++++++++++---- test/widgets/home_test.dart | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/lib/widgets/home.dart b/lib/widgets/home.dart index 6b5a497928..fed17bc434 100644 --- a/lib/widgets/home.dart +++ b/lib/widgets/home.dart @@ -151,6 +151,11 @@ const kTryAnotherAccountWaitPeriod = Duration(seconds: 5); class _LoadingPlaceholderPage extends StatefulWidget { const _LoadingPlaceholderPage({required this.accountId}); + /// The relevant account for this page. + /// + /// The account is not guaranteed to exist in the global store. This can + /// happen briefly when the account is removed from the database for logout, + /// but before [PerAccountStoreWidget.routeToRemoveOnLogout] is processed. final int accountId; @override @@ -180,9 +185,15 @@ class _LoadingPlaceholderPageState extends State<_LoadingPlaceholderPage> { @override Widget build(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); - final realmUrl = GlobalStoreWidget.of(context) - // TODO(#1219) `!` is incorrect - .getAccount(widget.accountId)!.realmUrl; + final account = GlobalStoreWidget.of(context).getAccount(widget.accountId); + + if (account == null) { + // We should only reach this state very briefly. + // See [_LoadingPlaceholderPage.accountId]. + return Scaffold( + appBar: AppBar(), + body: const CircularProgressIndicator()); + } return Scaffold( appBar: AppBar(), @@ -201,7 +212,7 @@ class _LoadingPlaceholderPageState extends State<_LoadingPlaceholderPage> { child: Column( children: [ const SizedBox(height: 16), - Text(zulipLocalizations.tryAnotherAccountMessage(realmUrl.toString())), + Text(zulipLocalizations.tryAnotherAccountMessage(account.realmUrl.toString())), const SizedBox(height: 8), ElevatedButton( onPressed: () => Navigator.push(context, diff --git a/test/widgets/home_test.dart b/test/widgets/home_test.dart index 2939358bb8..b1bda73f90 100644 --- a/test/widgets/home_test.dart +++ b/test/widgets/home_test.dart @@ -6,6 +6,7 @@ import 'package:zulip/api/model/events.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'; @@ -21,6 +22,7 @@ import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; +import '../model/store_checks.dart'; import '../model/test_store.dart'; import '../test_navigation.dart'; import 'message_list_checks.dart'; @@ -441,5 +443,23 @@ void main () { // No additional wait for loadPerAccount. checkOnHomePage(tester, expectedAccount: eg.selfAccount); }); + + testWidgets('logging out', (tester) async { + // Regression test for: https://github.com/zulip/zulip-flutter/issues/1219 + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await tester.pumpWidget(const ZulipApp()); + await tester.pump(); // wait for the loading page + await tester.pump(); // wait for store + + final element = tester.element(find.byType(HomePage)); + final future = logOutAccount(element, eg.selfAccount.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + // No error expected from [_LoadingPlaceholderPage] briefly not having + // access to the account being logged out. + checkOnLoadingPage(); + check(testBinding.globalStore).accountIds.isEmpty(); + }); }); } From ee004fa04005ee80c3d65f9be22e53803e7d6399 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 7 Jan 2025 17:20:48 +0800 Subject: [PATCH 2/4] action test [nfc]: Remove irrelevant issue reference Coming up with a realistic test case doesn't actually require invalidating API key. Signed-off-by: Zixuan James Li --- test/widgets/actions_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart index 4a4698a175..8b53e34b45 100644 --- a/test/widgets/actions_test.dart +++ b/test/widgets/actions_test.dart @@ -182,7 +182,7 @@ void main() { final pushedRoutes = >[]; testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); - // TODO(#737): switch to a realistic setup: + // TODO: switch to a realistic setup: // https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363 final account1Route = MaterialAccountWidgetRoute( accountId: account1.id, page: const InboxPageBody()); From 8ab7f7420c801387cb572447a36e7a381494d88a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 16 Jan 2025 17:32:15 -0500 Subject: [PATCH 3/4] notif test [nfc]: Make openNotification public Signed-off-by: Zixuan James Li --- test/notifications/display_test.dart | 31 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 32d8254d6d..400df9d81a 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -92,6 +92,22 @@ RemoveFcmMessage removeFcmMessage(List zulipMessages, {Account? account }) as RemoveFcmMessage; } +Future openNotification(WidgetTester tester, Account account, Message message) async { + final data = messageFcmMessage(message, account: account); + final intentDataUrl = NotificationOpenPayload( + realmUrl: data.realmUrl, + userId: data.userId, + narrow: switch (data.recipient) { + FcmMessageChannelRecipient(:var streamId, :var topic) => + TopicNarrow(streamId, topic), + FcmMessageDmRecipient(:var allRecipientIds) => + DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId), + }).buildUrl(); + unawaited( + WidgetsBinding.instance.handlePushRoute(intentDataUrl.toString())); + await tester.idle(); // let navigateForNotification find navigator +} + void main() { TestZulipBinding.ensureInitialized(); final zulipLocalizations = GlobalLocalizations.zulipLocalizations; @@ -991,21 +1007,6 @@ void main() { check(pushedRoutes).isEmpty(); } - Future openNotification(WidgetTester tester, Account account, Message message) async { - final data = messageFcmMessage(message, account: account); - final intentDataUrl = NotificationOpenPayload( - realmUrl: data.realmUrl, - userId: data.userId, - narrow: switch (data.recipient) { - FcmMessageChannelRecipient(:var streamId, :var topic) => - TopicNarrow(streamId, topic), - FcmMessageDmRecipient(:var allRecipientIds) => - DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId), - }).buildUrl(); - unawaited( - WidgetsBinding.instance.handlePushRoute(intentDataUrl.toString())); - await tester.idle(); // let navigateForNotification find navigator - } void matchesNavigation(Subject> route, Account account, Message message) { route.isA() From 4feb3e8f8704566412ef182436b2c79e26912eab Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 7 Jan 2025 18:31:22 +0800 Subject: [PATCH 4/4] actions test: Use a more realistic setup for logOutAccount This rewrite ended being a bit more substantial than just switching both accounts initial routes to the result of MessageListPage.buildRoute(). We do not use popUtil because the navigator stack normally should not become empty, so the HomePage route for account1 stays. Additionally, because we are pushing a different page route, we no longer need to set up different messages as the discriminator, further simplifying the test. See also: https://github.com/zulip/zulip-flutter/pull/1183#discussion_r1902218275 Signed-off-by: Zixuan James Li --- test/widgets/actions_test.dart | 71 +++++++++++++++++----------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart index 8b53e34b45..b776ec67fc 100644 --- a/test/widgets/actions_test.dart +++ b/test/widgets/actions_test.dart @@ -1,4 +1,3 @@ -import 'dart:async'; import 'dart:convert'; import 'package:checks/checks.dart'; @@ -18,7 +17,8 @@ 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/home.dart'; +import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; import '../api/fake_api.dart'; @@ -27,9 +27,11 @@ import '../model/binding.dart'; import '../model/store_checks.dart'; import '../model/test_store.dart'; import '../model/unreads_checks.dart'; +import '../notifications/display_test.dart'; import '../stdlib_checks.dart'; import '../test_navigation.dart'; import 'dialog_checks.dart'; +import 'page_checks.dart'; import 'test_app.dart'; void main() { @@ -156,68 +158,67 @@ void main() { }); testWidgets("logged-out account's routes removed from nav; other accounts' remain", (tester) async { - Future makeUnreadTopicInInbox(int accountId, String topic) async { + Future prepareStoreWithMessage(int accountId, User user, 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 store.addUser(user); await tester.pump(); + return message; } addTearDown(testBinding.reset); - final account1 = eg.account(id: 1, user: eg.user()); - final account2 = eg.account(id: 2, user: eg.user()); + final account1 = eg.selfAccount; + final account2 = eg.otherAccount; 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 = >[]; + final pushedRoutes = >[]; testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); - // TODO: 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); + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); + final account1Route = pushedRoutes.single; + check(account1Route).isA().page.isA(); + + final account2Connection = + (await testBinding.globalStore.perAccount(account2.id)).connection as FakeApiConnection; + account2Connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: []).toJson()); + final message = await prepareStoreWithMessage( + account2.id, eg.otherUser, 'topic'); + await openNotification(tester, account2, message); + await tester.pump(); - final findLoadingPage = find.byType(LoadingPlaceholderPage, skipOffstage: false); + final findAccount1PageContent = find.byType(HomePage, skipOffstage: false); + final findAccount2PageContent = find.byType(MessageListPage, skipOffstage: false); + check(pushedRoutes).deepEquals([ + account1Route, + (Subject it) => it.isA() + ..accountId.equals(eg.otherAccount.id) + ..page.isA(), + ]); check(findAccount1PageContent).findsOne(); - check(findLoadingPage).findsNothing(); + check(findAccount2PageContent).findsOne(); - final removedRoutes = >[]; + 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; + await tester.pumpAndSettle(); // wait for animations, if any check(removedRoutes).single.identicalTo(account1Route); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(account1.id); check(findAccount1PageContent).findsNothing(); - check(findLoadingPage).findsOne(); - - await tester.pump(); - check(findAccount1PageContent).findsNothing(); - check(findLoadingPage).findsNothing(); check(findAccount2PageContent).findsOne(); }); });