From 127239b57f33c357416e6da41746eb382e4776cb Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 24 Jan 2025 12:02:05 -0500 Subject: [PATCH 1/2] home test [nfc]: Extract helpers to be shared Signed-off-by: Zixuan James Li --- test/widgets/home_test.dart | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/test/widgets/home_test.dart b/test/widgets/home_test.dart index 2939358bb8..69598d93f3 100644 --- a/test/widgets/home_test.dart +++ b/test/widgets/home_test.dart @@ -49,6 +49,23 @@ void main () { await tester.pump(); } + void checkOnLoadingPage() { + check(find.byType(CircularProgressIndicator).hitTestable()).findsOne(); + check(find.byType(ChooseAccountPage)).findsNothing(); + check(find.byType(HomePage)).findsNothing(); + } + + ModalRoute? getRouteOf(WidgetTester tester, Finder finder) => + ModalRoute.of(tester.element(finder)); + + void checkOnHomePage(WidgetTester tester, {required Account expectedAccount}) { + check(find.byType(CircularProgressIndicator)).findsNothing(); + check(find.byType(ChooseAccountPage)).findsNothing(); + check(find.byType(HomePage).hitTestable()).findsOne(); + check(getRouteOf(tester, find.byType(HomePage))) + .isA().accountId.equals(expectedAccount.id); + } + group('bottom nav navigation', () { testWidgets('preserve states when switching between views', (tester) async { await prepare(tester); @@ -255,12 +272,6 @@ void main () { const loadPerAccountDuration = Duration(seconds: 30); assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod); - void checkOnLoadingPage() { - check(find.byType(CircularProgressIndicator).hitTestable()).findsOne(); - check(find.byType(ChooseAccountPage)).findsNothing(); - check(find.byType(HomePage)).findsNothing(); - } - void checkOnChooseAccountPage() { // Ignore the possible loading page in the background. check(find.byType(CircularProgressIndicator).hitTestable()).findsNothing(); @@ -268,17 +279,6 @@ void main () { check(find.byType(HomePage)).findsNothing(); } - ModalRoute? getRouteOf(WidgetTester tester, Finder finder) => - ModalRoute.of(tester.element(finder)); - - void checkOnHomePage(WidgetTester tester, {required Account expectedAccount}) { - check(find.byType(CircularProgressIndicator)).findsNothing(); - check(find.byType(ChooseAccountPage)).findsNothing(); - check(find.byType(HomePage).hitTestable()).findsOne(); - check(getRouteOf(tester, find.byType(HomePage))) - .isA().accountId.equals(expectedAccount.id); - } - Future prepare(WidgetTester tester) async { addTearDown(testBinding.reset); await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); From 1c684a4c4a48d47e7e4fa95ae779083d8ff3871c Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Sun, 29 Dec 2024 20:02:59 -0500 Subject: [PATCH 2/2] 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 be blank 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 blank 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 | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/lib/widgets/home.dart b/lib/widgets/home.dart index 6b5a497928..ad70b57c32 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 SizedBox.shrink()); + } 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 69598d93f3..5bb789727f 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'; @@ -442,4 +444,39 @@ void main () { checkOnHomePage(tester, expectedAccount: eg.selfAccount); }); }); + + testWidgets('logging out while still loading', (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 + checkOnLoadingPage(); + + final element = tester.element(find.byType(MaterialApp)); + final future = logOutAccount(element, eg.selfAccount.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + // No error expected from briefly not having + // access to the account being logged out. + check(testBinding.globalStore).accountIds.isEmpty(); + }); + + testWidgets('logging out after fully loaded', (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 + checkOnHomePage(tester, expectedAccount: eg.selfAccount); + + 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 briefly not having + // access to the account being logged out. + check(testBinding.globalStore).accountIds.isEmpty(); + }); }