Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store: Handle invalid API key on register-queue #1183

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
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": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l10n [nfc]: Use a generalize name for errorCouldNotConnectTitle

commit-message nit: "generalized"

"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 try again or use another account.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my comment in CZO still applies:

[…] the message looks like it can't be literally true. If an action really "cannot" be done, then trying the same action again won't work.

Or, from another angle: the server hasn't said that the account can't be authenticated, so we shouldn't tell the user that. That would be a pretty pathological case: I guess in theory it would be accurate if the database were corrupted in a way that made it impossible to know if any API key was valid.

What we know from the INVALID_API_KEY error is that the current attempt to authenticate failed, because we used the wrong API key. Here's a proposal:

Your account at {url} could not be authenticated. Please try logging in again or use another account.

cc @alya for thoughts on this message.

"@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 try 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:
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 try again or use another account.';
}

@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 try again or use another account.';
}

@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 try again or use another account.';
}

@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 try again or use another account.';
}

@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 try again or use another account.';
}

@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 try again or use another account.';
}

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

Expand Down
18 changes: 15 additions & 3 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,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 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 = 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
Expand Down
7 changes: 6 additions & 1 deletion lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,12 @@ 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'):
// See also: [_PerAccountStoreWidgetState.didChangeDependencies]
rethrow;
}
await (backoffMachine ??= BackoffMachine()).wait();
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
}
Expand Down
22 changes: 21 additions & 1 deletion lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
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 Expand Up @@ -221,6 +233,14 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
class ChooseAccountPage extends StatelessWidget {
const ChooseAccountPage({super.key});

/// Navigate to [ChooseAccountPage], ensuring that its route is at the root level.
static void navigate(BuildContext context) {
final navigator = Navigator.of(context);
navigator.popUntil((route) => route.isFirst);
unawaited(navigator.pushReplacement(
MaterialWidgetRoute(page: const ChooseAccountPage())));
}
Comment on lines +236 to +242
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this isn't used anywhere.


Widget _buildAccountItem(
BuildContext context, {
required int accountId,
Expand Down
59 changes: 33 additions & 26 deletions lib/widgets/home.dart
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ const kTryAnotherAccountWaitPeriod = Duration(seconds: 5);
class _LoadingPlaceholderPage extends StatefulWidget {
const _LoadingPlaceholderPage({required this.accountId});

/// The relevant account for this page.
///
/// The account is not guaranteed to exist in the global store. This can
/// happen briefly when the account is removed from the database for logout,
/// but before [PerAccountStoreWidget.routeToRemoveOnLogout] is processed.
final int accountId;

@override
Expand Down Expand Up @@ -180,35 +185,37 @@ class _LoadingPlaceholderPageState extends State<_LoadingPlaceholderPage> {
@override
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
final realmUrl = GlobalStoreWidget.of(context)
// TODO(#1219) `!` is incorrect
.getAccount(widget.accountId)!.realmUrl;
final account = GlobalStoreWidget.of(context).getAccount(widget.accountId);

return Scaffold(
appBar: AppBar(),
body: Center(
child: Column(
mainAxisSize: MainAxisSize.min,
children: [
const CircularProgressIndicator(),
Visibility(
visible: showTryAnotherAccount,
maintainSize: true,
maintainAnimation: true,
maintainState: true,
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: 8),
child: Column(
children: [
const SizedBox(height: 16),
Text(zulipLocalizations.tryAnotherAccountMessage(realmUrl.toString())),
const SizedBox(height: 8),
ElevatedButton(
onPressed: () => Navigator.push(context,
MaterialWidgetRoute(page: const ChooseAccountPage())),
child: Text(zulipLocalizations.tryAnotherAccountButton)),
]))),
])));
body: (account == null)
// We should only reach this state very briefly.
// See [_LoadingPlaceholderPage.accountId].
? const SizedBox.shrink()

: Center(child: Column(
mainAxisSize: MainAxisSize.min,
children: [
const CircularProgressIndicator(),
Visibility(
visible: showTryAnotherAccount,
maintainSize: true,
maintainAnimation: true,
maintainState: true,
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: 8),
child: Column(
children: [
const SizedBox(height: 16),
Text(zulipLocalizations.tryAnotherAccountMessage(account.realmUrl.toString())),
const SizedBox(height: 8),
ElevatedButton(
onPressed: () => Navigator.push(context,
MaterialWidgetRoute(page: const ChooseAccountPage())),
child: Text(zulipLocalizations.tryAnotherAccountButton)),
]))),
])));
}
}

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
Loading