diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 319c9e9dbc..118ab83c70 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -214,8 +214,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.", @@ -523,6 +523,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 b3a8752ba1..9579683908 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -397,7 +397,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. /// @@ -801,6 +801,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 967c7fb33c..71bf06d8ce 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -179,7 +179,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.'; @@ -404,6 +404,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 83d2af10b6..7a33e33567 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -179,7 +179,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.'; @@ -404,6 +404,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 034bcd17d0..137883e5e9 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -179,7 +179,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.'; @@ -404,6 +404,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 6416f59b08..3dec7d9b5a 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -179,7 +179,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.'; @@ -404,6 +404,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 cab571c163..83a777bfc4 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -179,7 +179,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.'; @@ -404,6 +404,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 babbc976fd..827aaf0155 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -179,7 +179,7 @@ class ZulipLocalizationsRu extends ZulipLocalizations { } @override - String get errorLoginCouldNotConnectTitle => 'Не удалось подключиться'; + String get errorCouldNotConnectTitle => 'Could not connect'; @override String get errorMessageDoesNotSeemToExist => 'Это сообщение, похоже, отсутствует.'; @@ -404,6 +404,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 ac3b93b024..38a3f8a240 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -179,7 +179,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.'; @@ -404,6 +404,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..60bcfb637b 100644 --- a/lib/log.dart +++ b/lib/log.dart @@ -31,7 +31,12 @@ bool debugLog(String message) { return true; } -typedef ReportErrorCallback = void Function(String? message, {String? details}); +// This should only be used for error reporting functions that allow the error +// to be cancelled programmatically. The implementation is expected to handle +// `null` for the `message` parameter and promptly dismiss the reported errors. +typedef ReportErrorCancellablyCallback = void Function(String? message, {String? details}); + +typedef ReportErrorCallback = void Function(String title, {String? message}); /// Show the user an error message, without requiring them to interact with it. /// @@ -48,10 +53,29 @@ 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 = defaultReportErrorToUserBriefly; + +/// Show the user a dismissable error message in a modal popup. +/// +/// Typically this shows an [AlertDialog] with `title` as the title, `message` +/// as the body. 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 = defaultReportErrorToUserModally; void defaultReportErrorToUserBriefly(String? message, {String? details}) { - // Error dismissing is a no-op to the default handler. + _reportErrorToConsole(message, details); +} + +void defaultReportErrorToUserModally(String title, {String? message}) { + _reportErrorToConsole(title, message); +} + +void _reportErrorToConsole(String? message, String? details) { + // Error dismissing is a no-op for the console. if (message == null) return; // If this callback is still in place, then the app's widget tree // hasn't mounted yet even as far as the [Navigator]. diff --git a/lib/model/store.dart b/lib/model/store.dart index 7603c7f452..611494cc8f 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,8 +150,36 @@ abstract class GlobalStore extends ChangeNotifier { /// and/or [perAccountSync]. Future loadPerAccount(int accountId) async { assert(_accounts.containsKey(accountId)); - final store = await doLoadPerAccount(accountId); + 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 account = getAccount(accountId); + if (account == null) { + // The account was logged out during `await doLoadPerAccount`. + // Here, that seems possible only by the user's own action; + // the logout can't have been done programmatically. + // Even if it were, it would have come with its own UI feedback. + // Anyway, skip showing feedback, to not be confusing or repetitive. + throw AccountNotFoundException(); + } + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + reportErrorToUserModally( + zulipLocalizations.errorCouldNotConnectTitle, + message: zulipLocalizations.errorInvalidApiKeyMessage( + account.realmUrl.toString())); + await logOutAccount(this, accountId); + throw AccountNotFoundException(); + default: + rethrow; + } + } if (!_accounts.containsKey(accountId)) { + // TODO(#1354): handle this earlier // [removeAccount] was called during [doLoadPerAccount]. store.dispose(); throw AccountNotFoundException(); @@ -913,12 +942,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 +1213,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 +1255,14 @@ class UpdateMachine { if (_disposed) return; } - await store._globalStore._reloadPerAccount(store.accountId); - assert(_disposed); + try { + await store._globalStore._reloadPerAccount(store.accountId); + } on AccountNotFoundException { + assert(debugLog('… Event queue not replaced; account was logged out.')); + return; + } finally { + assert(_disposed); + } assert(debugLog('… Event queue replaced.')); } diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 2ad35e3e53..73a4e5f232 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -85,6 +85,7 @@ class ZulipApp extends StatefulWidget { static void debugReset() { _snackBarCount = 0; reportErrorToUserBriefly = defaultReportErrorToUserBriefly; + reportErrorToUserModally = defaultReportErrorToUserModally; _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 title, {String? message}) { + assert(_ready.value); + + showErrorDialog( + context: navigatorKey.currentContext!, + title: title, + message: message); + } + void _declareReady() { assert(navigatorKey.currentContext != null); _ready.value = true; reportErrorToUserBriefly = _reportErrorToUserBriefly; + reportErrorToUserModally = _reportErrorToUserModally; } @override @@ -211,7 +223,11 @@ class _ZulipAppState extends State with WidgetsBindingObserver { theme: themeData, navigatorKey: ZulipApp.navigatorKey, - navigatorObservers: widget.navigatorObservers ?? const [], + navigatorObservers: [ + if (widget.navigatorObservers != null) + ...widget.navigatorObservers!, + _PreventEmptyStack(), + ], builder: (BuildContext context, Widget? child) { if (!ZulipApp.ready.value) { SchedulerBinding.instance.addPostFrameCallback( @@ -234,6 +250,33 @@ class _ZulipAppState extends State with WidgetsBindingObserver { } } +/// Pushes a route whenever the observed navigator stack becomes empty. +class _PreventEmptyStack 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 { + _pushRouteIfEmptyStack(); + } + + @override + void didPop(Route route, Route? previousRoute) async { + _pushRouteIfEmptyStack(); + } +} + class ChooseAccountPage extends StatelessWidget { const ChooseAccountPage({super.key}); diff --git a/lib/widgets/login.dart b/lib/widgets/login.dart index 195bf75e62..504289adc1 100644 --- a/lib/widgets/login.dart +++ b/lib/widgets/login.dart @@ -190,7 +190,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/example_data.dart b/test/example_data.dart index 116a9a1bd8..a758481f52 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -39,6 +39,18 @@ ZulipApiException apiBadRequest({ data: {}, message: message); } +/// The error for the "events" route when the target event queue has been +/// garbage collected. +/// +/// https://zulip.com/api/get-events#bad_event_queue_id-errors +ZulipApiException apiExceptionBadEventQueueId({ + String queueId = 'fb67bf8a-c031-47cc-84cf-ed80accacda8', +}) { + return ZulipApiException( + routeName: 'events', httpStatus: 400, code: 'BAD_EVENT_QUEUE_ID', + data: {'queue_id': queueId}, message: 'Bad event queue ID: $queueId'); +} + /// The error the server gives when the client's credentials /// (API key together with email and realm URL) are no longer valid. /// @@ -867,6 +879,7 @@ ChannelUpdateEvent channelUpdateEvent( TestGlobalStore globalStore({List accounts = const []}) { return TestGlobalStore(accounts: accounts); } +const _globalStore = globalStore; InitialSnapshot initialSnapshot({ String? queueId, @@ -937,10 +950,14 @@ InitialSnapshot initialSnapshot({ } const _initialSnapshot = initialSnapshot; -PerAccountStore store({Account? account, InitialSnapshot? initialSnapshot}) { +PerAccountStore store({ + GlobalStore? globalStore, + Account? account, + InitialSnapshot? initialSnapshot, +}) { final effectiveAccount = account ?? selfAccount; return PerAccountStore.fromInitialSnapshot( - globalStore: globalStore(accounts: [effectiveAccount]), + globalStore: globalStore ?? _globalStore(accounts: [effectiveAccount]), accountId: effectiveAccount.id, initialSnapshot: initialSnapshot ?? _initialSnapshot(), ); diff --git a/test/model/store_test.dart b/test/model/store_test.dart index ad80a6263d..99235dbe67 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -13,6 +13,7 @@ import 'package:zulip/api/route/events.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/realm.dart'; import 'package:zulip/log.dart'; +import 'package:zulip/model/actions.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/receive.dart'; @@ -120,6 +121,29 @@ void main() { check(completers(1)).length.equals(1); }); + test('GlobalStore.perAccount loading fails with HTTP status code 401', () => awaitFakeAsync((async) async { + final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]); + final future = globalStore.perAccount(eg.selfAccount.id); + + globalStore.completers[eg.selfAccount.id]! + .single.completeError(eg.apiExceptionUnauthorized()); + await check(future).throws(); + })); + + test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async { + final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]); + final future = globalStore.perAccount(eg.selfAccount.id); + + await logOutAccount(globalStore, eg.selfAccount.id); + check(globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + globalStore.completers[eg.selfAccount.id]! + .single.completeError(eg.apiExceptionUnauthorized()); + await check(future).throws(); + check(globalStore.takeDoRemoveAccountCalls()).isEmpty(); + })); + // TODO test insertAccount group('GlobalStore.updateAccount', () { @@ -757,11 +781,8 @@ void main() { } void prepareExpiredEventQueue() { - connection.prepare(httpStatus: 400, json: { - 'result': 'error', 'code': 'BAD_EVENT_QUEUE_ID', - 'queue_id': updateMachine.queueId, - 'msg': 'Bad event queue ID: ${updateMachine.queueId}', - }); + connection.prepare(apiException: eg.apiExceptionBadEventQueueId( + queueId: updateMachine.queueId)); } Future prepareHandleEventError() async { @@ -972,6 +993,73 @@ void main() { }); }); + group('UpdateMachine.poll reload failure', () { + late LoadingTestGlobalStore globalStore; + + List> completers() => + globalStore.completers[eg.selfAccount.id]!; + + Future prepareReload(FakeAsync async) async { + globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]); + final future = globalStore.perAccount(eg.selfAccount.id); + final store = eg.store(globalStore: globalStore, account: eg.selfAccount); + completers().single.complete(store); + await future; + completers().clear(); + final updateMachine = globalStore.updateMachines[eg.selfAccount.id] = + UpdateMachine.fromInitialSnapshot( + store: store, initialSnapshot: eg.initialSnapshot()); + updateMachine.debugPauseLoop(); + updateMachine.poll(); + + (store.connection as FakeApiConnection).prepare( + apiException: eg.apiExceptionBadEventQueueId()); + updateMachine.debugAdvanceLoop(); + async.elapse(Duration.zero); + check(store).isLoading.isTrue(); + } + + void checkReloadFailure({ + required FutureOr Function() completeLoading, + }) { + awaitFakeAsync((async) async { + await prepareReload(async); + check(completers()).single.isCompleted.isFalse(); + + await completeLoading(); + check(completers()).single.isCompleted.isTrue(); + check(globalStore.takeDoRemoveAccountCalls()).single.equals(eg.selfAccount.id); + + async.elapse(TestGlobalStore.removeAccountDuration); + check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); + + async.flushTimers(); + // Reload never succeeds and there are no unhandled errors. + check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); + }); + } + + Future logOutAndCompleteWithNewStore() async { + // [PerAccountStore.fromInitialSnapshot] requires the account + // to be in the global store when called; do so before logging out. + final newStore = eg.store(globalStore: globalStore, account: eg.selfAccount); + await logOutAccount(globalStore, eg.selfAccount.id); + completers().single.complete(newStore); + } + + test('user logged out before new store is loaded', () => awaitFakeAsync((async) async { + checkReloadFailure(completeLoading: logOutAndCompleteWithNewStore); + })); + + void completeWithApiExceptionUnauthorized() { + completers().single.completeError(eg.apiExceptionUnauthorized()); + } + + test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async { + checkReloadFailure(completeLoading: completeWithApiExceptionUnauthorized); + })); + }); + group('UpdateMachine.registerNotificationToken', () { late UpdateMachine updateMachine; late FakeApiConnection connection; 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/stdlib_checks.dart b/test/stdlib_checks.dart index 792588ee75..8bfaea54fd 100644 --- a/test/stdlib_checks.dart +++ b/test/stdlib_checks.dart @@ -5,6 +5,7 @@ /// part of the Dart standard library. library; +import 'dart:async'; import 'dart:convert'; import 'package:checks/checks.dart'; @@ -74,6 +75,10 @@ Object? deepToJson(Object? object) { return (result, true); } +extension CompleterChecks on Subject> { + Subject get isCompleted => has((x) => x.isCompleted, 'isCompleted'); +} + extension JsonChecks on Subject { /// Expects that the value is deeply equal to [expected], /// after calling [deepToJson] on both. diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index af386bfdf1..7b1388dbec 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -4,6 +4,7 @@ import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.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 +58,124 @@ void main() { }); }); + group('_PreventEmptyStack', () { + late List> pushedRoutes; + late List> removedRoutes; + late List> poppedRoutes; + + Future prepare(WidgetTester tester) async { + addTearDown(testBinding.reset); + + pushedRoutes = []; + removedRoutes = []; + poppedRoutes = []; + final testNavObserver = TestNavigatorObserver(); + testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); + testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); + testNavObserver.onPopped = (route, prevRoute) => poppedRoutes.add(route); + + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); // start to load account + check(pushedRoutes).single.isA().page.isA(); + pushedRoutes.clear(); + } + + testWidgets('push route when removing last route on stack', (tester) async { + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await prepare(tester); + // The navigator stack should contain only a home page route. + + // Log out, causing the home page to be removed from the stack. + final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + // The choose-account page should appear. + check(removedRoutes).single.isA().page.isA(); + check(pushedRoutes).single.isA().page.isA(); + }); + + testWidgets('push route when popping last route on stack', (tester) async { + // Set up the loading of per-account data to fail. + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.loadPerAccountDuration = Duration.zero; + testBinding.globalStore.loadPerAccountException = eg.apiExceptionUnauthorized(); + await prepare(tester); + // The navigator stack should contain only a home page route. + + // Await the failed load, causing the home page to be removed + // and an error dialog pushed in its place. + await tester.pump(Duration.zero); + await tester.pump(TestGlobalStore.removeAccountDuration); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + check(removedRoutes).single.isA().page.isA(); + check(poppedRoutes).isEmpty(); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + + // Dismiss the error dialog, causing it to be popped from the stack. + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Could not connect', + expectedMessage: + 'Your account at ${eg.selfAccount.realmUrl} could not be authenticated.' + ' Please try logging in again or use another account.'))); + // The choose-account page should appear, because the error dialog + // was the only route remaining. + check(poppedRoutes).single.isA>(); + check(pushedRoutes).single.isA().page.isA(); + }); + + testWidgets('do not push route to non-empty navigator stack', (tester) async { + // Set up the loading of per-account data to fail, but only after a + // long enough time for the "Try another account" button to appear. + const loadPerAccountDuration = Duration(seconds: 30); + assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod); + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration; + testBinding.globalStore.loadPerAccountException = eg.apiExceptionUnauthorized(); + await prepare(tester); + // The navigator stack should contain only a home page route. + + // Await the "Try another account" button, and tap it. + await tester.pump(kTryAnotherAccountWaitPeriod); + await tester.tap(find.text('Try another account')); + await tester.pump(); + // The navigator stack should contain the home page route + // and a choose-account page route. + check(removedRoutes).isEmpty(); + check(poppedRoutes).isEmpty(); + check(pushedRoutes).single.isA().page.isA(); + pushedRoutes.clear(); + + // Now await the failed load, causing the home page to be removed + // and an error dialog pushed, while the choose-account page remains. + await tester.pump(loadPerAccountDuration); + await tester.pump(TestGlobalStore.removeAccountDuration); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + check(removedRoutes).single.isA().page.isA(); + check(poppedRoutes).isEmpty(); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + // The navigator stack should now contain the choose-account page route + // and the dialog route. + + // Dismiss the error dialog, causing it to be popped from the stack. + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Could not connect', + expectedMessage: + 'Your account at ${eg.selfAccount.realmUrl} could not be authenticated.' + ' Please try logging in again or use another account.'))); + // No routes should be pushed after dismissing the error dialog, + // because there was already another route remaining on the stack + // (namely the choose-account page route). + check(poppedRoutes).single.isA>(); + check(pushedRoutes).isEmpty(); + }); + }); + group('ChooseAccountPage', () { Future setupChooseAccountPage(WidgetTester tester, { required List accounts, @@ -245,7 +364,9 @@ void main() { check(ZulipApp.scaffoldMessenger).isNotNull(); check(ZulipApp.ready).value.isTrue(); }); + }); + group('error reporting', () { Finder findSnackBarByText(String text) => find.descendant( of: find.byType(SnackBar), matching: find.text(text)); @@ -307,7 +428,7 @@ void main() { check(findSnackBarByText(message).evaluate()).single; } - testWidgets('reportErrorToUser dismissing SnackBar', (tester) async { + testWidgets('reportErrorToUserBriefly dismissing SnackBar', (tester) async { const message = 'test error message'; const details = 'error details'; await prepareSnackBarWithDetails(tester, message, details); @@ -361,5 +482,24 @@ void main() { await tester.pumpAndSettle(); check(findSnackBarByText('unrelated').evaluate()).single; }); + + testWidgets('reportErrorToUserModally', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(const ZulipApp()); + const title = 'test title'; + const message = 'test message'; + + // Prior to app startup, reportErrorToUserModally only logs. + reportErrorToUserModally(title, message: message); + check(ZulipApp.ready).value.isFalse(); + await tester.pump(); + checkNoErrorDialog(tester); + + check(ZulipApp.ready).value.isTrue(); + // After app startup, reportErrorToUserModally displays an [AlertDialog]. + reportErrorToUserModally(title, message: message); + await tester.pump(); + checkErrorDialog(tester, expectedTitle: title, expectedMessage: message); + }); }); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 58782a30ec..1aa1af81c5 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -7,7 +7,6 @@ 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'; @@ -143,9 +142,8 @@ void main() { 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.debugPrepareLoopError( + eg.apiExceptionBadEventQueueId(queueId: updateMachine.queueId)); updateMachine.debugAdvanceLoop(); await tester.pump(); // Event queue has been replaced; but the [MessageList] hasn't been diff --git a/test/widgets/store_test.dart b/test/widgets/store_test.dart index 7e70f01a19..dc67647eb7 100644 --- a/test/widgets/store_test.dart +++ b/test/widgets/store_test.dart @@ -203,7 +203,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());