diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index ba02a819b1..0764d87955 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -206,8 +206,8 @@ "url": {"type": "String", "example": "http://example.com/"} } }, - "errorLoginCouldNotConnectTitle": "Could not connect", - "@errorLoginCouldNotConnectTitle": { + "errorCouldNotConnectTitle": "Could not connect", + "@errorCouldNotConnectTitle": { "description": "Error title when the app could not connect to the server." }, "errorMessageDoesNotSeemToExist": "That message does not seem to exist.", @@ -515,6 +515,13 @@ "@topicValidationErrorMandatoryButEmpty": { "description": "Topic validation error when topic is required but was empty." }, + "errorInvalidApiKeyMessage": "Your account at {url} could not be authenticated. Please try logging in again or use another account.", + "@errorInvalidApiKeyMessage": { + "description": "Error message in the dialog for invalid API key.", + "placeholders": { + "url": {"type": "String", "example": "http://chat.example.com/"} + } + }, "errorInvalidResponse": "The server sent an invalid response", "@errorInvalidResponse": { "description": "Error message when an API call returned an invalid response." diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index 9be039ef72..453ae35357 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -391,7 +391,7 @@ abstract class ZulipLocalizations { /// /// In en, this message translates to: /// **'Could not connect'** - String get errorLoginCouldNotConnectTitle; + String get errorCouldNotConnectTitle; /// Error message when loading a message that does not exist. /// @@ -795,6 +795,12 @@ abstract class ZulipLocalizations { /// **'Topics are required in this organization.'** String get topicValidationErrorMandatoryButEmpty; + /// Error message in the dialog for invalid API key. + /// + /// In en, this message translates to: + /// **'Your account at {url} could not be authenticated. Please try logging in again or use another account.'** + String errorInvalidApiKeyMessage(String url); + /// Error message when an API call returned an invalid response. /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index 2e59dae8af..28e5d4c973 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsAr extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Could not connect'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index a0e86b7d35..2987799e77 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsEn extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Could not connect'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index 6e7f1559ac..93c49994fd 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsJa extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Could not connect'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index cabd4897e9..a04de46a73 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsNb extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Could not connect'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index 0a2a2bf1d7..1c30ed0291 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsPl extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Nie można połączyć'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'Taka wiadomość raczej nie istnieje.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Wątki są wymagane przez tę organizację.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index 99bd72c62f..3d5e7fa6fb 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsRu extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Не удалось подключиться'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'Это сообщение, похоже, отсутствует.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Темы обязательны в этой организации.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'Получен недопустимый ответ сервера'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index 13646eafd5..ea1929139b 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -174,7 +174,7 @@ class ZulipLocalizationsSk extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Nepodarilo sa pripojiť'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'Správa zrejme neexistuje.'; @@ -399,6 +399,11 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'Server poslal nesprávnu odpoveď'; diff --git a/lib/log.dart b/lib/log.dart index e3261f8cba..31b9469e8f 100644 --- a/lib/log.dart +++ b/lib/log.dart @@ -31,7 +31,8 @@ bool debugLog(String message) { return true; } -typedef ReportErrorCallback = void Function(String? message, {String? details}); +typedef ReportErrorCancellablyCallback = void Function(String? message, {String? details}); +typedef ReportErrorCallback = void Function(String message, {String? details}); /// Show the user an error message, without requiring them to interact with it. /// @@ -48,9 +49,20 @@ typedef ReportErrorCallback = void Function(String? message, {String? details}); // This gets set in [ZulipApp]. We need this indirection to keep `lib/log.dart` // from importing widget code, because the file is a dependency for the rest of // the app. -ReportErrorCallback reportErrorToUserBriefly = defaultReportErrorToUserBriefly; +ReportErrorCancellablyCallback reportErrorToUserBriefly = reportErrorToConsole; -void defaultReportErrorToUserBriefly(String? message, {String? details}) { +/// Show the user a dismissable error message in a modal popup. +/// +/// Typically this shows an [AlertDialog] containing the message. +/// If called before the app's widget tree is ready (see [ZulipApp.ready]), +/// then we give up on showing the message to the user, +/// and just log the message to the console. +// This gets set in [ZulipApp]. We need this indirection to keep `lib/log.dart` +// from importing widget code, because the file is a dependency for the rest of +// the app. +ReportErrorCallback reportErrorToUserModally = reportErrorToConsole; + +void reportErrorToConsole(String? message, {String? details}) { // Error dismissing is a no-op to the default handler. if (message == null) return; // If this callback is still in place, then the app's widget tree diff --git a/lib/model/actions.dart b/lib/model/actions.dart new file mode 100644 index 0000000000..76d88d250b --- /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/store.dart b/lib/model/store.dart index 7603c7f452..6838028933 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -19,6 +19,7 @@ import '../api/backoff.dart'; import '../api/route/realm.dart'; import '../log.dart'; import '../notifications/receive.dart'; +import 'actions.dart'; import 'autocomplete.dart'; import 'database.dart'; import 'emoji.dart'; @@ -149,7 +150,29 @@ abstract class GlobalStore extends ChangeNotifier { /// and/or [perAccountSync]. Future loadPerAccount(int accountId) async { assert(_accounts.containsKey(accountId)); - final store = await doLoadPerAccount(accountId); + final realmUrl = getAccount(accountId)!.realmUrl; + final PerAccountStore store; + try { + store = await doLoadPerAccount(accountId); + } catch (e) { + switch (e) { + case HttpException(httpStatus: 401): + // The API key is invalid and the store can never be loaded + // unless the user retries manually. + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + reportErrorToUserModally( + zulipLocalizations.errorCouldNotConnectTitle, + details: zulipLocalizations.errorInvalidApiKeyMessage( + realmUrl.toString())); + // The account could be logged out from the choose-account page. + if (_accounts.containsKey(accountId)) { + await logOutAccount(this, accountId); + } + throw AccountNotFoundException(); + default: + rethrow; + } + } if (!_accounts.containsKey(accountId)) { // [removeAccount] was called during [doLoadPerAccount]. store.dispose(); @@ -913,12 +936,19 @@ class UpdateMachine { try { return await registerQueue(connection); } catch (e, s) { - assert(debugLog('Error fetching initial snapshot: $e')); - // Print stack trace in its own log entry; log entries are truncated - // at 1 kiB (at least on Android), and stack can be longer than that. - assert(debugLog('Stack:\n$s')); + // TODO(#890): tell user if initial-fetch errors persist, or look non-transient + switch (e) { + case HttpException(httpStatus: 401): + // We cannot recover from this error through retrying. + // Leave it to [GlobalStore.loadPerAccount]. + rethrow; + default: + assert(debugLog('Error fetching initial snapshot: $e')); + // Print stack trace in its own log entry; log entries are truncated + // at 1 kiB (at least on Android), and stack can be longer than that. + assert(debugLog('Stack:\n$s')); + } assert(debugLog('Backing off, then will retry…')); - // TODO tell user if initial-fetch errors persist, or look non-transient await (backoffMachine ??= BackoffMachine()).wait(); assert(debugLog('… Backoff wait complete, retrying initial fetch.')); } @@ -1177,6 +1207,7 @@ class UpdateMachine { store.isLoading = true; bool isUnexpected; + // TODO(#1054): handle auth failure switch (error) { case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'): assert(debugLog('Lost event queue for $store. Replacing…')); @@ -1218,8 +1249,17 @@ class UpdateMachine { if (_disposed) return; } - await store._globalStore._reloadPerAccount(store.accountId); - assert(_disposed); + try { + await store._globalStore._reloadPerAccount(store.accountId); + } on AccountNotFoundException { + // The account was logged out while we retry to replace the store, + // but that's OK as long as other code will be removing it from the UI + // (usually by using [routeToRemoveOnLogout]). + assert(debugLog('… Event queue not replaced; account logged out.')); + return; + } finally { + assert(_disposed); + } assert(debugLog('… Event queue replaced.')); } 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 9525dffdfe..0608866972 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'; @@ -84,7 +84,8 @@ class ZulipApp extends StatefulWidget { @visibleForTesting static void debugReset() { _snackBarCount = 0; - reportErrorToUserBriefly = defaultReportErrorToUserBriefly; + reportErrorToUserBriefly = reportErrorToConsole; + reportErrorToUserModally = reportErrorToConsole; _ready.dispose(); _ready = ValueNotifier(false); } @@ -128,10 +129,21 @@ class ZulipApp extends StatefulWidget { newSnackBar.closed.whenComplete(() => _snackBarCount--); } + /// The callback we normally use as [reportErrorToUserModally]. + static void _reportErrorToUserModally(String message, {String? details}) { + assert(_ready.value); + + showErrorDialog( + context: navigatorKey.currentContext!, + title: message, + message: details); + } + void _declareReady() { assert(navigatorKey.currentContext != null); _ready.value = true; reportErrorToUserBriefly = _reportErrorToUserBriefly; + reportErrorToUserModally = _reportErrorToUserModally; } @override @@ -187,7 +199,13 @@ class _ZulipAppState extends State with WidgetsBindingObserver { theme: themeData, navigatorKey: ZulipApp.navigatorKey, - navigatorObservers: widget.navigatorObservers ?? const [], + navigatorObservers: [ + if (widget.navigatorObservers != null) + ...widget.navigatorObservers!, + // This must be the last item to maintain the invariant + // that the navigator stack is always non-empty. + _EmptyStackNavigatorObserver(), + ], builder: (BuildContext context, Widget? child) { if (!ZulipApp.ready.value) { SchedulerBinding.instance.addPostFrameCallback( @@ -218,6 +236,39 @@ class _ZulipAppState extends State with WidgetsBindingObserver { } } +/// Pushes a route whenever the observed navigator stack becomes empty. +class _EmptyStackNavigatorObserver extends NavigatorObserver { + void _pushRouteIfEmptyStack() async { + final navigator = await ZulipApp.navigator; + bool isEmptyStack = true; + // TODO: find a better way to inspect the navigator stack + navigator.popUntil((route) { + isEmptyStack = false; + return true; // never actually pops + }); + if (isEmptyStack) { + unawaited(navigator.push( + MaterialWidgetRoute(page: const ChooseAccountPage()))); + } + } + + @override + void didRemove(Route route, Route? previousRoute) async { + if (previousRoute == null) { + // The route removed is the bottom-most one. + _pushRouteIfEmptyStack(); + } + } + + @override + void didPop(Route route, Route? previousRoute) async { + if (previousRoute == null) { + // The route popped is the bottom-most one. + _pushRouteIfEmptyStack(); + } + } +} + class ChooseAccountPage extends StatelessWidget { const ChooseAccountPage({super.key}); @@ -249,7 +300,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/login.dart b/lib/widgets/login.dart index ce1cf09438..64a0210525 100644 --- a/lib/widgets/login.dart +++ b/lib/widgets/login.dart @@ -176,7 +176,7 @@ class _AddAccountPageState extends State { // TODO(#105) give more helpful feedback; see `fetchServerSettings` // in zulip-mobile's src/message/fetchActions.js. showErrorDialog(context: context, - title: zulipLocalizations.errorLoginCouldNotConnectTitle, + title: zulipLocalizations.errorCouldNotConnectTitle, message: zulipLocalizations.errorLoginCouldNotConnect(url.toString())); return; } 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..5b192a7c55 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -7,13 +7,13 @@ import 'package:flutter/foundation.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/core.dart'; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; 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/model/actions.dart'; import 'package:zulip/log.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/receive.dart'; @@ -122,6 +122,39 @@ void main() { check(completers(1)).length.equals(1); }); + test('GlobalStore.perAccount loading fails with HTTP status code 401', () => awaitFakeAsync((async) async { + addTearDown(testBinding.reset); + + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.loadPerAccountException = ZulipApiException( + routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401, + data: {}, message: ''); + await check(testBinding.globalStore.perAccount(eg.selfAccount.id)) + .throws(); + + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + })); + + test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async { + addTearDown(testBinding.reset); + + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + + testBinding.globalStore.loadPerAccountDuration = Duration(seconds: 2); + testBinding.globalStore.loadPerAccountException = ZulipApiException( + routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401, + data: {}, message: ''); + final future = testBinding.globalStore.perAccount(eg.selfAccount.id); + check(testBinding.globalStore.takeDoRemoveAccountCalls()).isEmpty(); + + await logOutAccount(testBinding.globalStore, eg.selfAccount.id); + check(testBinding.globalStore.takeDoRemoveAccountCalls()).single; + + await check(future).throws(); + check(testBinding.globalStore.takeDoRemoveAccountCalls()).isEmpty(); + })); + // TODO test insertAccount group('GlobalStore.updateAccount', () { @@ -824,23 +857,23 @@ 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 + test('unexpected poll error, but reload fails with HTTP status code 401; log out', () => awaitFakeAsync((async) async { 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(); + prepareUnexpectedLoopError(); updateMachine.debugAdvanceLoop(); async.elapse(Duration.zero); + check(store).isLoading.isTrue(); + + globalStore.loadPerAccountException = ZulipApiException( + routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401, + data: {}, message: ''); + // The reload doesn't happen immediately; there's a timer. + check(async.pendingTimers).length.equals(1); + async.flushTimers(); - // The old store's [MessageListView]s have been disposed. - // (And no exception was thrown; that was #810.) - check(store.debugMessageListViews).isEmpty(); + check(globalStore.takeDoRemoveAccountCalls().single) + .equals(eg.selfAccount.id); })); group('report error', () { @@ -858,7 +891,7 @@ void main() { Future prepare() async { reportErrorToUserBriefly = logReportedError; - addTearDown(() => reportErrorToUserBriefly = defaultReportErrorToUserBriefly); + addTearDown(() => reportErrorToUserBriefly = reportErrorToConsole); await preparePoll(lastEventId: 1); } diff --git a/test/model/test_store.dart b/test/model/test_store.dart index b6887cbed7..534a6003b5 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -129,6 +129,7 @@ class TestGlobalStore extends GlobalStore { static const Duration removeAccountDuration = Duration(milliseconds: 1); Duration? loadPerAccountDuration; + Object? loadPerAccountException; /// Consume the log of calls made to [doRemoveAccount]. List takeDoRemoveAccountCalls() { @@ -150,6 +151,9 @@ class TestGlobalStore extends GlobalStore { if (loadPerAccountDuration != null) { await Future.delayed(loadPerAccountDuration!); } + if (loadPerAccountException != null) { + throw loadPerAccountException!; + } final initialSnapshot = _initialSnapshots[accountId]!; final store = PerAccountStore.fromInitialSnapshot( globalStore: this, diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart index 5a957c44e0..d125170258 100644 --- a/test/widgets/actions_test.dart +++ b/test/widgets/actions_test.dart @@ -12,6 +12,7 @@ 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'; @@ -20,6 +21,7 @@ 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 'package:zulip/widgets/store.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; @@ -109,7 +111,7 @@ void main() { final newConnection = separateConnection() ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'}); - final future = logOutAccount(context, eg.selfAccount.id); + final future = logOutAccount(GlobalStoreWidget.of(context), eg.selfAccount.id); // Unregister-token request and account removal dispatched together checkSingleUnregisterRequest(newConnection); check(testBinding.globalStore.takeDoRemoveAccountCalls()) @@ -141,7 +143,7 @@ void main() { final newConnection = separateConnection() ..prepare(delay: unregisterDelay, exception: exception); - final future = logOutAccount(context, eg.selfAccount.id); + final future = logOutAccount(GlobalStoreWidget.of(context), eg.selfAccount.id); // Unregister-token request and account removal dispatched together checkSingleUnregisterRequest(newConnection); check(testBinding.globalStore.takeDoRemoveAccountCalls()) @@ -185,7 +187,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()); @@ -211,7 +213,7 @@ void main() { testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); final context = tester.element(find.byType(MaterialApp)); - final future = logOutAccount(context, account1.id); + final future = logOutAccount(GlobalStoreWidget.of(context), account1.id); await tester.pump(TestGlobalStore.removeAccountDuration); await future; check(removedRoutes).single.identicalTo(account1Route); diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index af386bfdf1..8bceef9c35 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -3,7 +3,9 @@ import 'dart:async'; import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/exception.dart'; import 'package:zulip/log.dart'; +import 'package:zulip/model/actions.dart'; import 'package:zulip/model/database.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/home.dart'; @@ -57,6 +59,99 @@ void main() { }); }); + group('_EmptyStackNavigatorObserver', () { + late List> pushedRoutes; + late List> removedRoutes; + + Future prepare(WidgetTester tester) async { + addTearDown(testBinding.reset); + + pushedRoutes = []; + removedRoutes = []; + final testNavObserver = TestNavigatorObserver(); + testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); + testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); + + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); // start to load account + check(pushedRoutes).single.isA().page.isA(); + pushedRoutes.clear(); + } + + testWidgets('do not push route to non-empty navigator stack', (tester) async { + const loadPerAccountDuration = Duration(seconds: 30); + assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod); + testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration; + testBinding.globalStore.loadPerAccountException = ZulipApiException( + routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401, + data: {}, message: ''); + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + await prepare(tester); + + await tester.pump(kTryAnotherAccountWaitPeriod); + await tester.tap(find.text('Try another account')); + await tester.pump(); // tap the button + check(pushedRoutes).single.isA().page.isA(); + pushedRoutes.clear(); + + await tester.pump(loadPerAccountDuration); // got the error + await tester.pump(TestGlobalStore.removeAccountDuration); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + check(removedRoutes).single.isA().page.isA(); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Could not connect', + expectedMessage: + 'Your account at https://chat.example/ could not be authenticated.' + ' Please try logging in again or use another account.'))); + // No more routes are pushed after dismissing the error dialog, + // because the navigator stack was non-empty. + check(pushedRoutes).isEmpty(); + }); + + testWidgets('push route when popping last route on stack', (tester) async { + testBinding.globalStore.loadPerAccountDuration = Duration.zero; + testBinding.globalStore.loadPerAccountException = ZulipApiException( + routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401, + data: {}, message: ''); + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + await prepare(tester); + + await tester.pump(Duration.zero); // got the error + await tester.pump(TestGlobalStore.removeAccountDuration); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + check(removedRoutes).single.isA().page.isA(); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Could not connect', + expectedMessage: + 'Your account at https://chat.example/ could not be authenticated.' + ' Please try logging in again or use another account.'))); + // The navigator stack became empty after dismissing the error dialog, + // so a choose-account page route was pushed. + check(pushedRoutes).single.isA().page.isA(); + }); + + testWidgets('push route when removing last route on stack', (tester) async { + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await prepare(tester); + + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + check(pushedRoutes).single.isA().page.isA(); + check(removedRoutes).single.isA().page.isA(); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + }); + }); + group('ChooseAccountPage', () { Future setupChooseAccountPage(WidgetTester tester, { required List accounts, @@ -361,5 +456,24 @@ void main() { await tester.pumpAndSettle(); check(findSnackBarByText('unrelated').evaluate()).single; }); + + testWidgets('reportErrorToUserModally', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(const ZulipApp()); + const message = 'test error message'; + const details = 'details'; + + // Prior to app startup, reportErrorToUserModally only logs. + reportErrorToUserModally(message, details: details); + check(ZulipApp.ready).value.isFalse(); + await tester.pump(); + checkNoErrorDialog(tester); + + check(ZulipApp.ready).value.isTrue(); + // After app startup, reportErrorToUserModally displays an [AlertDialog]. + reportErrorToUserModally(message, details: details); + await tester.pump(); + checkErrorDialog(tester, expectedTitle: message, expectedMessage: details); + }); }); } diff --git a/test/widgets/home_test.dart b/test/widgets/home_test.dart index 5bb789727f..15730dc53a 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'; @@ -15,6 +15,7 @@ import 'package:zulip/widgets/inbox.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; import 'package:zulip/widgets/profile.dart'; +import 'package:zulip/widgets/store.dart'; import 'package:zulip/widgets/subscription_list.dart'; import 'package:zulip/widgets/theme.dart'; @@ -454,7 +455,7 @@ void main () { checkOnLoadingPage(); final element = tester.element(find.byType(MaterialApp)); - final future = logOutAccount(element, eg.selfAccount.id); + final future = logOutAccount(GlobalStoreWidget.of(element), eg.selfAccount.id); await tester.pump(TestGlobalStore.removeAccountDuration); await future; // No error expected from briefly not having @@ -472,7 +473,7 @@ void main () { checkOnHomePage(tester, expectedAccount: eg.selfAccount); final element = tester.element(find.byType(HomePage)); - final future = logOutAccount(element, eg.selfAccount.id); + final future = logOutAccount(GlobalStoreWidget.of(element), 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..204993cc76 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -12,6 +12,7 @@ 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'; @@ -130,6 +131,26 @@ void main() { final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); check(state.composeBoxController).isNull(); }); + + testWidgets('dispose MessageListView when logged out', (tester) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + (store.connection as FakeApiConnection).prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: [eg.streamMessage()]).toJson()); + await tester.pumpWidget(TestZulipApp( + accountId: eg.selfAccount.id, + skipAssertAccountExists: true, + child: MessageListPage(initNarrow: const CombinedFeedNarrow()))); + await tester.pump(); + await tester.pump(); + 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', () {