Skip to content

login: Support logging out of an account (take 2) #1010

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

Merged
merged 7 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,22 @@
"@chooseAccountPageTitle": {
"description": "Title for ChooseAccountPage"
},
"chooseAccountPageLogOutButton": "Log out",
"@chooseAccountPageLogOutButton": {
"description": "Label for the 'Log out' button for an account on the choose-account page"
},
"logOutConfirmationDialogTitle": "Log out?",
"@logOutConfirmationDialogTitle": {
"description": "Title for a confirmation dialog for logging out."
},
"logOutConfirmationDialogMessage": "To use this account in the future, you will have to re-enter the URL for your organization and your account information.",
"@logOutConfirmationDialogMessage": {
"description": "Message for a confirmation dialog for logging out."
},
"logOutConfirmationDialogConfirmButton": "Log out",
"@logOutConfirmationDialogConfirmButton": {
"description": "Label for the 'Log out' button on a confirmation dialog for logging out."
},
"chooseAccountButtonAddAnAccount": "Add an account",
"@chooseAccountButtonAddAnAccount": {
"description": "Label for ChooseAccountPage button to add an account"
Expand Down
19 changes: 18 additions & 1 deletion lib/api/route/notifications.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

import '../core.dart';

/// https://zulip.com/api/add-fcm-token
Expand All @@ -10,6 +9,15 @@ Future<void> addFcmToken(ApiConnection connection, {
});
}

/// https://zulip.com/api/remove-fcm-token
Future<void> removeFcmToken(ApiConnection connection, {
required String token,
}) {
return connection.delete('removeFcmToken', (_) {}, 'users/me/android_gcm_reg_id', {
'token': RawParameter(token),
});
}

/// https://zulip.com/api/add-apns-token
Future<void> addApnsToken(ApiConnection connection, {
required String token,
Expand All @@ -20,3 +28,12 @@ Future<void> addApnsToken(ApiConnection connection, {
'appid': RawParameter(appid),
});
}

/// https://zulip.com/api/remove-apns-token
Future<void> removeApnsToken(ApiConnection connection, {
required String token,
}) {
return connection.delete('removeApnsToken', (_) {}, 'users/me/apns_device_token', {
'token': RawParameter(token),
});
}
16 changes: 16 additions & 0 deletions lib/notifications/receive.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,22 @@ class NotificationService {
}
}

static Future<void> unregisterToken(ApiConnection connection, {required String token}) async {
switch (defaultTargetPlatform) {
case TargetPlatform.android:
await removeFcmToken(connection, token: token);

case TargetPlatform.iOS:
await removeApnsToken(connection, token: token);

case TargetPlatform.linux:
case TargetPlatform.macOS:
case TargetPlatform.windows:
case TargetPlatform.fuchsia:
assert(false);
}
}

static void _onForegroundMessage(FirebaseRemoteMessage message) {
assert(debugLog("notif message: ${message.data}"));
_onRemoteMessage(message);
Expand Down
37 changes: 37 additions & 0 deletions lib/widgets/actions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,53 @@
/// in order to present success or error feedback to the user through the UI.
library;

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';

import '../api/model/model.dart';
import '../api/model/narrow.dart';
import '../api/route/messages.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import '../notifications/receive.dart';
import 'dialog.dart';
import 'store.dart';

Future<void> 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<void> 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<void> markNarrowAsRead(BuildContext context, Narrow narrow) async {
final store = PerAccountStoreWidget.of(context);
final connection = store.connection;
Expand Down
68 changes: 56 additions & 12 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import '../log.dart';
import '../model/localizations.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import '../notifications/display.dart';
import 'about_zulip.dart';
import 'actions.dart';
import 'app_bar.dart';
import 'dialog.dart';
import 'inbox.dart';
Expand Down Expand Up @@ -232,11 +234,45 @@ class ChooseAccountPage extends StatelessWidget {
required Widget title,
Widget? subtitle,
}) {
final designVariables = DesignVariables.of(context);
final zulipLocalizations = ZulipLocalizations.of(context);
final materialLocalizations = MaterialLocalizations.of(context);
return Card(
clipBehavior: Clip.hardEdge,
child: ListTile(
title: title,
subtitle: subtitle,
trailing: MenuAnchor(
menuChildren: [
MenuItemButton(
onPressed: () {
showSuggestedActionDialog(context: context,
title: zulipLocalizations.logOutConfirmationDialogTitle,
message: zulipLocalizations.logOutConfirmationDialogMessage,
// TODO(#1032) "destructive" style for action button
actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton,
onActionButtonPress: () {
// TODO error handling if db write fails?
logOutAccount(context, accountId);
});
},
child: Text(zulipLocalizations.chooseAccountPageLogOutButton)),
],
builder: (BuildContext context, MenuController controller, Widget? child) {
return IconButton(
tooltip: materialLocalizations.showMenuTooltip, // "Show menu"
onPressed: () {
if (controller.isOpen) {
controller.close();
} else {
controller.open();
}
},
icon: Icon(Icons.adaptive.more, color: designVariables.icon));
}),
// The default trailing padding with M3 is 24px. Decrease by 12 because
// IconButton (the "…" button) comes with 12px padding on all sides.
contentPadding: const EdgeInsetsDirectional.only(start: 16, end: 12),
onTap: () => Navigator.push(context,
HomePage.buildRoute(accountId: accountId))));
}
Expand Down Expand Up @@ -275,24 +311,32 @@ class ChooseAccountPage extends StatelessWidget {
}
}

enum ChooseAccountPageOverflowMenuItem { aboutZulip }

class ChooseAccountPageOverflowButton extends StatelessWidget {
const ChooseAccountPageOverflowButton({super.key});

@override
Widget build(BuildContext context) {
return PopupMenuButton<ChooseAccountPageOverflowMenuItem>(
itemBuilder: (BuildContext context) => const [
PopupMenuItem(
value: ChooseAccountPageOverflowMenuItem.aboutZulip,
child: Text('About Zulip')),
],
onSelected: (item) {
switch (item) {
case ChooseAccountPageOverflowMenuItem.aboutZulip:
final designVariables = DesignVariables.of(context);
final materialLocalizations = MaterialLocalizations.of(context);
return MenuAnchor(
menuChildren: [
MenuItemButton(
onPressed: () {
Navigator.push(context, AboutZulipPage.buildRoute(context));
}
},
child: const Text('About Zulip')), // TODO(i18n)
],
builder: (BuildContext context, MenuController controller, Widget? child) {
return IconButton(
tooltip: materialLocalizations.showMenuTooltip, // "Show menu"
onPressed: () {
if (controller.isOpen) {
controller.close();
} else {
controller.open();
}
},
icon: Icon(Icons.adaptive.more, color: designVariables.icon));
Comment on lines +330 to +339
Copy link
Member

Choose a reason for hiding this comment

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

Seems annoying that with the new API we have to recite all this standard stuff. But so be it. If we find this coming up more, we can abstract it out.

});
}
}
Expand Down
5 changes: 4 additions & 1 deletion lib/widgets/dialog.dart
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ void showSuggestedActionDialog({
onPressed: () => Navigator.pop(context),
child: _dialogActionText(zulipLocalizations.dialogCancel)),
TextButton(
onPressed: onActionButtonPress,
onPressed: () {
onActionButtonPress();
Navigator.pop(context);
},
child: _dialogActionText(actionButtonText ?? zulipLocalizations.dialogContinue)),
]));
}
1 change: 1 addition & 0 deletions lib/widgets/page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ mixin AccountPageRouteMixin<T extends Object?> on PageRoute<T> {
return PerAccountStoreWidget(
accountId: accountId,
placeholder: const LoadingPlaceholderPage(),
routeToRemoveOnLogout: this,
child: super.buildPage(context, animation, secondaryAnimation));
}
}
Expand Down
24 changes: 22 additions & 2 deletions lib/widgets/store.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import 'package:flutter/material.dart';
import 'package:flutter/scheduler.dart';

import '../model/binding.dart';
import '../model/store.dart';
import 'page.dart';

/// Provides access to the app's data.
///
Expand Down Expand Up @@ -112,11 +114,19 @@ class PerAccountStoreWidget extends StatefulWidget {
super.key,
required this.accountId,
this.placeholder = const LoadingPlaceholder(),
this.routeToRemoveOnLogout,
required this.child,
});

final int accountId;
final Widget placeholder;

/// A per-account [Route] that should be removed on logout.
///
/// Use this when the widget is a page on a route that should go away
/// when the account is logged out, instead of lingering with [placeholder].
final AccountPageRouteMixin? routeToRemoveOnLogout;

final Widget child;

/// The user's data for the relevant Zulip account for this widget.
Expand Down Expand Up @@ -195,6 +205,16 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
void didChangeDependencies() {
super.didChangeDependencies();
final globalStore = GlobalStoreWidget.of(context);
final accountExists = globalStore.getAccount(widget.accountId) != null;
if (!accountExists) {
// logged out
_setStore(null);
if (widget.routeToRemoveOnLogout != null) {
SchedulerBinding.instance.addPostFrameCallback(
(_) => Navigator.of(context).removeRoute(widget.routeToRemoveOnLogout!));
}
return;
}
// If we already have data, get it immediately. This avoids showing one
// frame of loading indicator each time we have a new PerAccountStoreWidget.
final store = globalStore.perAccountSync(widget.accountId);
Expand All @@ -212,13 +232,13 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
// 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
// (for example by removing a per-account route from the nav).
// (usually by using [routeToRemoveOnLogout]).
}
}();
}
}

void _setStore(PerAccountStore store) {
void _setStore(PerAccountStore? store) {
if (store != this.store) {
setState(() {
this.store = store;
Expand Down
42 changes: 42 additions & 0 deletions test/api/route/notifications_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ void main() {
});
});

group('removeFcmToken', () {
Future<void> checkRemoveFcmToken(FakeApiConnection connection, {
required String token,
}) async {
connection.prepare(json: {});
await removeFcmToken(connection, token: token);
check(connection.lastRequest).isA<http.Request>()
..method.equals('DELETE')
..url.path.equals('/api/v1/users/me/android_gcm_reg_id')
..bodyFields.deepEquals({
'token': token,
});
}

test('smoke', () {
return FakeApiConnection.with_((connection) async {
await checkRemoveFcmToken(connection, token: 'asdf');
});
});
});

group('addApnsToken', () {
Future<void> checkAddApnsToken(FakeApiConnection connection, {
required String token,
Expand All @@ -50,4 +71,25 @@ void main() {
});
});
});

group('removeApnsToken', () {
Future<void> checkRemoveApnsToken(FakeApiConnection connection, {
required String token,
}) async {
connection.prepare(json: {});
await removeApnsToken(connection, token: token);
check(connection.lastRequest).isA<http.Request>()
..method.equals('DELETE')
..url.path.equals('/api/v1/users/me/apns_device_token')
..bodyFields.deepEquals({
'token': token,
});
}

test('smoke', () {
return FakeApiConnection.with_((connection) async {
await checkRemoveApnsToken(connection, token: 'asdf');
});
});
});
}
Loading