Skip to content

Commit

Permalink
store: Handle invalid API key on register-queue
Browse files Browse the repository at this point in the history
This is near term fix for a user-reported issue:
  https://chat.zulip.org/#narrow/channel/48-mobile/topic/0.2E0.2E19.20Flutter.20.3A.20Cant.20connect.20to.20self.20hosted.20instance/near/2004042

It is not intended to be the full fix.  With a better UX, we would
bring the user back to the choose-account page without them manually
doing so.  That's covered by #737 but out-of-scope for this commit.

Signed-off-by: Zixuan James Li <[email protected]>
  • Loading branch information
PIG208 committed Dec 21, 2024
1 parent eb3e2aa commit 4df9d15
Show file tree
Hide file tree
Showing 15 changed files with 158 additions and 18 deletions.
11 changes: 9 additions & 2 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,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.",
Expand Down Expand Up @@ -458,6 +458,13 @@
"@topicValidationErrorMandatoryButEmpty": {
"description": "Topic validation error when topic is required but was empty."
},
"errorInvalidApiKeyMessage": "Your account at {url} cannot be authenticated. Please logout and try again.",
"@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."
Expand Down
8 changes: 7 additions & 1 deletion lib/generated/l10n/zulip_localizations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,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.
///
Expand Down Expand Up @@ -721,6 +721,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} cannot be authenticated. Please logout and try again.'**
String errorInvalidApiKeyMessage(String url);

/// Error message when an API call returned an invalid response.
///
/// In en, this message translates to:
Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_ar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,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.';
Expand Down Expand Up @@ -357,6 +357,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 cannot be authenticated. Please logout and try again.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,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.';
Expand Down Expand Up @@ -357,6 +357,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 cannot be authenticated. Please logout and try again.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_fr.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class ZulipLocalizationsFr 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.';
Expand Down Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url cannot be authenticated. Please logout and try again.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_ja.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,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.';
Expand Down Expand Up @@ -357,6 +357,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 cannot be authenticated. Please logout and try again.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_pl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,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.';
Expand Down Expand Up @@ -357,6 +357,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 cannot be authenticated. Please logout and try again.';
}

@override
String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_ru.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class ZulipLocalizationsRu 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.';
Expand Down Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url cannot be authenticated. Please logout and try again.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
16 changes: 14 additions & 2 deletions lib/log.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -48,7 +49,18 @@ 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 a [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 = defaultReportErrorToUserBriefly;

void defaultReportErrorToUserBriefly(String? message, {String? details}) {
// Error dismissing is a no-op to the default handler.
Expand Down
11 changes: 10 additions & 1 deletion lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,16 @@ class UpdateMachine {
// 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
// TODO(#890): tell user if initial-fetch errors persist, or look non-transient
switch (e) {
case ZulipApiException(code: 'INVALID_API_KEY'):
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
reportErrorToUserModally(
zulipLocalizations.errorCouldNotConnectTitle,
details: zulipLocalizations.errorInvalidApiKeyMessage(
connection.realmUrl.toString()));
rethrow;
}
await (backoffMachine ??= BackoffMachine()).wait();
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
}
Expand Down
12 changes: 12 additions & 0 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class ZulipApp extends StatefulWidget {
static void debugReset() {
_snackBarCount = 0;
reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
reportErrorToUserModally = defaultReportErrorToUserBriefly;
_ready.dispose();
_ready = ValueNotifier(false);
}
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/widgets/login.dart
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ class _AddAccountPageState extends State<AddAccountPage> {
// 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;
}
Expand Down
23 changes: 18 additions & 5 deletions lib/widgets/store.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:flutter/material.dart';
import 'package:flutter/scheduler.dart';

import '../api/exception.dart';
import '../model/binding.dart';
import '../model/store.dart';
import 'page.dart';
Expand Down Expand Up @@ -245,11 +246,23 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
// [didChangeDependencies] will run again, this time in the
// `store != null` case above.
await globalStore.perAccount(widget.accountId);
} on AccountNotFoundException {
// The account was logged out while its store was loading.
// This widget will be showing [placeholder] perpetually,
// but that's OK as long as other code will be removing it from the UI
// (usually by using [routeToRemoveOnLogout]).
} catch (e) {
switch (e) {
case AccountNotFoundException():
// The account was logged out while its store was loading.
// This widget will be showing [placeholder] perpetually,
// but that's OK as long as other code will be removing it from the UI
// (usually by using [routeToRemoveOnLogout]).
return;
case ZulipApiException(code: 'INVALID_API_KEY'):
// The API key is invalid and the store can never be loaded
// unless the user retries manually.
// TODO(#737): Reset the navigator stack and bring the user back
// to the choose-account page.
return;
default:
rethrow;
}
}
}();
}
Expand Down
31 changes: 31 additions & 0 deletions test/model/store_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ 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';
Expand Down Expand Up @@ -500,6 +501,36 @@ void main() {
users.map((expected) => (it) => it.fullName.equals(expected.fullName)));
}));

String? lastReportedError;
String? takeLastReportedError() {
final result = lastReportedError;
lastReportedError = null;
return result;
}

Future<void> logReportedError(String message, {String? details}) async {
lastReportedError = '$message\n$details';
}

test('does not retry registerQueue on invalid API key', () => awaitFakeAsync((async) async {
reportErrorToUserModally = logReportedError;
addTearDown(() => reportErrorToUserModally = defaultReportErrorToUserBriefly);
await prepareStore();

// Try to load, but the API key is told to be invalid.
globalStore.useCachedApiConnections = true;
connection.prepare(httpStatus: 400, json: {
'result': 'error', 'code': 'INVALID_API_KEY',
'msg': 'Invalid API key',
});
await check(UpdateMachine.load(globalStore, eg.selfAccount.id))
.throws<ZulipApiException>();

check(takeLastReportedError()).isNotNull().equals(
'Could not connect\n'
'Your account at https://chat.example/ cannot be authenticated. Please logout and try again.');
}));

// TODO test UpdateMachine.load starts polling loop
// TODO test UpdateMachine.load calls registerNotificationToken
});
Expand Down
20 changes: 20 additions & 0 deletions test/widgets/app_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import 'dart:async';

import 'package:checks/checks.dart';
import 'package:flutter/material.dart';
import 'package:flutter_checks/flutter_checks.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/log.dart';
import 'package:zulip/model/database.dart';
Expand Down Expand Up @@ -361,5 +362,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();
check(find.byType(AlertDialog)).findsNothing();

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);
});
});
}

0 comments on commit 4df9d15

Please sign in to comment.