Skip to content

Commit 8bcae42

Browse files
PIG208PIG208
committed
store: Handle invalid API key on register-queue
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 zulip#737 but out-of-scope for this commit. Signed-off-by: Zixuan James Li <[email protected]>
1 parent 6a83136 commit 8bcae42

17 files changed

+203
-18
lines changed

‎assets/l10n/app_en.arb

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,8 +186,8 @@
186186
"url": {"type": "String", "example": "http://example.com/"}
187187
}
188188
},
189-
"errorLoginCouldNotConnectTitle": "Could not connect",
190-
"@errorLoginCouldNotConnectTitle": {
189+
"errorCouldNotConnectTitle": "Could not connect",
190+
"@errorCouldNotConnectTitle": {
191191
"description": "Error title when the app could not connect to the server."
192192
},
193193
"errorMessageDoesNotSeemToExist": "That message does not seem to exist.",
@@ -458,6 +458,13 @@
458458
"@topicValidationErrorMandatoryButEmpty": {
459459
"description": "Topic validation error when topic is required but was empty."
460460
},
461+
"errorInvalidApiKeyMessage": "Your account at {url} cannot be authenticated. Please try again or use another account.",
462+
"@errorInvalidApiKeyMessage": {
463+
"description": "Error message in the dialog for invalid API key.",
464+
"placeholders": {
465+
"url": {"type": "String", "example": "http://chat.example.com/"}
466+
}
467+
},
461468
"errorInvalidResponse": "The server sent an invalid response",
462469
"@errorInvalidResponse": {
463470
"description": "Error message when an API call returned an invalid response."

‎lib/generated/l10n/zulip_localizations.dart

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ abstract class ZulipLocalizations {
359359
///
360360
/// In en, this message translates to:
361361
/// **'Could not connect'**
362-
String get errorLoginCouldNotConnectTitle;
362+
String get errorCouldNotConnectTitle;
363363

364364
/// Error message when loading a message that does not exist.
365365
///
@@ -721,6 +721,12 @@ abstract class ZulipLocalizations {
721721
/// **'Topics are required in this organization.'**
722722
String get topicValidationErrorMandatoryButEmpty;
723723

724+
/// Error message in the dialog for invalid API key.
725+
///
726+
/// In en, this message translates to:
727+
/// **'Your account at {url} cannot be authenticated. Please try again or use another account.'**
728+
String errorInvalidApiKeyMessage(String url);
729+
724730
/// Error message when an API call returned an invalid response.
725731
///
726732
/// In en, this message translates to:

‎lib/generated/l10n/zulip_localizations_ar.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
159159
}
160160

161161
@override
162-
String get errorLoginCouldNotConnectTitle => 'Could not connect';
162+
String get errorCouldNotConnectTitle => 'Could not connect';
163163

164164
@override
165165
String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -357,6 +357,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

‎lib/generated/l10n/zulip_localizations_en.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
159159
}
160160

161161
@override
162-
String get errorLoginCouldNotConnectTitle => 'Could not connect';
162+
String get errorCouldNotConnectTitle => 'Could not connect';
163163

164164
@override
165165
String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -357,6 +357,11 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

‎lib/generated/l10n/zulip_localizations_fr.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
159159
}
160160

161161
@override
162-
String get errorLoginCouldNotConnectTitle => 'Could not connect';
162+
String get errorCouldNotConnectTitle => 'Could not connect';
163163

164164
@override
165165
String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -357,6 +357,11 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

‎lib/generated/l10n/zulip_localizations_ja.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
159159
}
160160

161161
@override
162-
String get errorLoginCouldNotConnectTitle => 'Could not connect';
162+
String get errorCouldNotConnectTitle => 'Could not connect';
163163

164164
@override
165165
String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -357,6 +357,11 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

‎lib/generated/l10n/zulip_localizations_pl.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
159159
}
160160

161161
@override
162-
String get errorLoginCouldNotConnectTitle => 'Nie można połączyć';
162+
String get errorCouldNotConnectTitle => 'Could not connect';
163163

164164
@override
165165
String get errorMessageDoesNotSeemToExist => 'Taka wiadomość raczej nie istnieje.';
@@ -357,6 +357,11 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Wątki są wymagane przez tę organizację.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera';
362367

‎lib/generated/l10n/zulip_localizations_ru.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
159159
}
160160

161161
@override
162-
String get errorLoginCouldNotConnectTitle => 'Could not connect';
162+
String get errorCouldNotConnectTitle => 'Could not connect';
163163

164164
@override
165165
String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -357,6 +357,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

‎lib/log.dart

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ bool debugLog(String message) {
3131
return true;
3232
}
3333

34-
typedef ReportErrorCallback = void Function(String? message, {String? details});
34+
typedef ReportErrorCancellablyCallback = void Function(String? message, {String? details});
35+
typedef ReportErrorCallback = void Function(String message, {String? details});
3536

3637
/// Show the user an error message, without requiring them to interact with it.
3738
///
@@ -48,7 +49,18 @@ typedef ReportErrorCallback = void Function(String? message, {String? details});
4849
// This gets set in [ZulipApp]. We need this indirection to keep `lib/log.dart`
4950
// from importing widget code, because the file is a dependency for the rest of
5051
// the app.
51-
ReportErrorCallback reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
52+
ReportErrorCancellablyCallback reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
53+
54+
/// Show the user a dismissable error message in a modal popup.
55+
///
56+
/// Typically this shows a [AlertDialog] containing the message.
57+
/// If called before the app's widget tree is ready (see [ZulipApp.ready]),
58+
/// then we give up on showing the message to the user,
59+
/// and just log the message to the console.
60+
// This gets set in [ZulipApp]. We need this indirection to keep `lib/log.dart`
61+
// from importing widget code, because the file is a dependency for the rest of
62+
// the app.
63+
ReportErrorCallback reportErrorToUserModally = defaultReportErrorToUserBriefly;
5264

5365
void defaultReportErrorToUserBriefly(String? message, {String? details}) {
5466
// Error dismissing is a no-op to the default handler.

‎lib/model/store.dart

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -912,7 +912,11 @@ class UpdateMachine {
912912
// at 1 kiB (at least on Android), and stack can be longer than that.
913913
assert(debugLog('Stack:\n$s'));
914914
assert(debugLog('Backing off, then will retry…'));
915-
// TODO tell user if initial-fetch errors persist, or look non-transient
915+
// TODO(#890): tell user if initial-fetch errors persist, or look non-transient
916+
switch (e) {
917+
case ZulipApiException(code: 'INVALID_API_KEY'):
918+
rethrow;
919+
}
916920
await (backoffMachine ??= BackoffMachine()).wait();
917921
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
918922
}

‎lib/widgets/app.dart

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ class ZulipApp extends StatefulWidget {
8585
static void debugReset() {
8686
_snackBarCount = 0;
8787
reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
88+
reportErrorToUserModally = defaultReportErrorToUserBriefly;
8889
_ready.dispose();
8990
_ready = ValueNotifier(false);
9091
}
@@ -128,10 +129,21 @@ class ZulipApp extends StatefulWidget {
128129
newSnackBar.closed.whenComplete(() => _snackBarCount--);
129130
}
130131

132+
/// The callback we normally use as [reportErrorToUserModally].
133+
static void _reportErrorToUserModally(String message, {String? details}) {
134+
assert(_ready.value);
135+
136+
showErrorDialog(
137+
context: navigatorKey.currentContext!,
138+
title: message,
139+
message: details);
140+
}
141+
131142
void _declareReady() {
132143
assert(navigatorKey.currentContext != null);
133144
_ready.value = true;
134145
reportErrorToUserBriefly = _reportErrorToUserBriefly;
146+
reportErrorToUserModally = _reportErrorToUserModally;
135147
}
136148

137149
@override

‎lib/widgets/login.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ class _AddAccountPageState extends State<AddAccountPage> {
176176
// TODO(#105) give more helpful feedback; see `fetchServerSettings`
177177
// in zulip-mobile's src/message/fetchActions.js.
178178
showErrorDialog(context: context,
179-
title: zulipLocalizations.errorLoginCouldNotConnectTitle,
179+
title: zulipLocalizations.errorCouldNotConnectTitle,
180180
message: zulipLocalizations.errorLoginCouldNotConnect(url.toString()));
181181
return;
182182
}

‎lib/widgets/store.dart

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
1+
import 'dart:async';
2+
13
import 'package:flutter/material.dart';
24
import 'package:flutter/scheduler.dart';
35

6+
import '../api/exception.dart';
7+
import '../log.dart';
48
import '../model/binding.dart';
9+
import '../model/localizations.dart';
510
import '../model/store.dart';
11+
import 'actions.dart';
12+
import 'app.dart';
613
import 'page.dart';
714

815
/// Provides access to the app's data.
@@ -245,11 +252,32 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
245252
// [didChangeDependencies] will run again, this time in the
246253
// `store != null` case above.
247254
await globalStore.perAccount(widget.accountId);
248-
} on AccountNotFoundException {
249-
// The account was logged out while its store was loading.
250-
// This widget will be showing [placeholder] perpetually,
251-
// but that's OK as long as other code will be removing it from the UI
252-
// (usually by using [routeToRemoveOnLogout]).
255+
} catch (e) {
256+
switch (e) {
257+
case AccountNotFoundException():
258+
// The account was logged out while its store was loading.
259+
// This widget will be showing [placeholder] perpetually,
260+
// but that's OK as long as other code will be removing it from the UI
261+
// (usually by using [routeToRemoveOnLogout]).
262+
return;
263+
case ZulipApiException(code: 'INVALID_API_KEY'):
264+
// The API key is invalid and the store can never be loaded
265+
// unless the user retries manually.
266+
// TODO(#737): Reset the navigator stack and bring the user back
267+
// to the choose-account page.
268+
if (!mounted) return;
269+
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
270+
reportErrorToUserModally(
271+
zulipLocalizations.errorCouldNotConnectTitle,
272+
details: zulipLocalizations.errorInvalidApiKeyMessage(
273+
globalStore.getAccount(widget.accountId)!.realmUrl.toString()));
274+
unawaited(
275+
Navigator.push(context, MaterialWidgetRoute(page: const ChooseAccountPage())));
276+
await logOutAccount(context, widget.accountId);
277+
return;
278+
default:
279+
rethrow;
280+
}
253281
}
254282
}();
255283
}

‎test/model/store_test.dart

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'package:flutter/foundation.dart';
77
import 'package:http/http.dart' as http;
88
import 'package:test/scaffolding.dart';
99
import 'package:zulip/api/core.dart';
10+
import 'package:zulip/api/exception.dart';
1011
import 'package:zulip/api/model/events.dart';
1112
import 'package:zulip/api/model/model.dart';
1213
import 'package:zulip/api/route/events.dart';
@@ -500,6 +501,19 @@ void main() {
500501
users.map((expected) => (it) => it.fullName.equals(expected.fullName)));
501502
}));
502503

504+
test('does not retry registerQueue on invalid API key', () => awaitFakeAsync((async) async {
505+
await prepareStore();
506+
507+
// Try to load, but the API key is told to be invalid.
508+
globalStore.useCachedApiConnections = true;
509+
connection.prepare(httpStatus: 400, json: {
510+
'result': 'error', 'code': 'INVALID_API_KEY',
511+
'msg': 'Invalid API key',
512+
});
513+
await check(UpdateMachine.load(globalStore, eg.selfAccount.id))
514+
.throws<ZulipApiException>();
515+
}));
516+
503517
// TODO test UpdateMachine.load starts polling loop
504518
// TODO test UpdateMachine.load calls registerNotificationToken
505519
});

‎test/model/test_store.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ class TestGlobalStore extends GlobalStore {
126126

127127
static const Duration removeAccountDuration = Duration(milliseconds: 1);
128128
Duration? loadPerAccountDuration;
129+
Object? loadPerAccountException;
129130

130131
/// Consume the log of calls made to [doRemoveAccount].
131132
List<int> takeDoRemoveAccountCalls() {
@@ -147,6 +148,9 @@ class TestGlobalStore extends GlobalStore {
147148
if (loadPerAccountDuration != null) {
148149
await Future<void>.delayed(loadPerAccountDuration!);
149150
}
151+
if (loadPerAccountException != null) {
152+
throw loadPerAccountException!;
153+
}
150154
final initialSnapshot = _initialSnapshots[accountId]!;
151155
final store = PerAccountStore.fromInitialSnapshot(
152156
globalStore: this,

‎test/widgets/app_test.dart

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import 'dart:async';
22

33
import 'package:checks/checks.dart';
44
import 'package:flutter/material.dart';
5+
import 'package:flutter_checks/flutter_checks.dart';
56
import 'package:flutter_test/flutter_test.dart';
67
import 'package:zulip/log.dart';
78
import 'package:zulip/model/database.dart';
@@ -361,5 +362,24 @@ void main() {
361362
await tester.pumpAndSettle();
362363
check(findSnackBarByText('unrelated').evaluate()).single;
363364
});
365+
366+
testWidgets('reportErrorToUserModally', (tester) async {
367+
addTearDown(testBinding.reset);
368+
await tester.pumpWidget(const ZulipApp());
369+
const message = 'test error message';
370+
const details = 'details';
371+
372+
// Prior to app startup, reportErrorToUserModally only logs.
373+
reportErrorToUserModally(message, details: details);
374+
check(ZulipApp.ready).value.isFalse();
375+
await tester.pump();
376+
check(find.byType(AlertDialog)).findsNothing();
377+
378+
check(ZulipApp.ready).value.isTrue();
379+
// After app startup, reportErrorToUserModally displays an [AlertDialog].
380+
reportErrorToUserModally(message, details: details);
381+
await tester.pump();
382+
checkErrorDialog(tester, expectedTitle: message, expectedMessage: details);
383+
});
364384
});
365385
}

0 commit comments

Comments
 (0)