diff --git a/lib/model/actions.dart b/lib/model/actions.dart index a88eea0777..3869faae03 100644 --- a/lib/model/actions.dart +++ b/lib/model/actions.dart @@ -1,5 +1,8 @@ import 'dart:async'; +import 'package:flutter/foundation.dart'; + +import '../notifications/display.dart'; import '../notifications/receive.dart'; import 'store.dart'; @@ -11,6 +14,10 @@ Future logOutAccount(GlobalStore globalStore, int accountId) async { // Unawaited, to not block removing the account on this request. unawaited(unregisterToken(globalStore, accountId)); + if (defaultTargetPlatform == TargetPlatform.android) { + unawaited(NotificationDisplayManager.removeNotificationsForAccount(account.realmUrl, account.userId)); + } + await globalStore.removeAccount(accountId); } diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 1d74db6854..00a5f6fda5 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -1,10 +1,10 @@ import 'dart:async'; import 'dart:io'; -import 'package:http/http.dart' as http; import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/widgets.dart' hide Notification; +import 'package:http/http.dart' as http; import '../api/model/model.dart'; import '../api/notifications.dart'; @@ -217,10 +217,12 @@ class NotificationChannelManager { /// Service for managing the notifications shown to the user. class NotificationDisplayManager { static Future init() async { + assert(defaultTargetPlatform == TargetPlatform.android); await NotificationChannelManager.ensureChannel(); } static void onFcmMessage(FcmMessage data, Map dataJson) { + assert(defaultTargetPlatform == TargetPlatform.android); switch (data) { case MessageFcmMessage(): _onMessageFcmMessage(data, dataJson); case RemoveFcmMessage(): _onRemoveFcmMessage(data); @@ -231,9 +233,21 @@ class NotificationDisplayManager { static Future _onMessageFcmMessage(MessageFcmMessage data, Map dataJson) async { assert(debugLog('notif message content: ${data.content}')); final zulipLocalizations = GlobalLocalizations.zulipLocalizations; - final groupKey = _groupKey(data); + final groupKey = _groupKey(data.realmUrl, data.userId); final conversationKey = _conversationKey(data, groupKey); + final globalStore = await ZulipBinding.instance.getGlobalStore(); + final account = globalStore.accounts.firstWhereOrNull((account) => + account.realmUrl.origin == data.realmUrl.origin && account.userId == data.userId); + + // Skip showing notifications for a logged-out account. This can occur if + // the unregisterToken request failed previously. It would be annoying + // to the user if notifications keep showing up after they've logged out. + // (Also alarming: it suggests the logout didn't fully work.) + if (account == null) { + return; + } + final oldMessagingStyle = await _androidHost .getActiveNotificationMessagingStyleByTag(conversationKey); @@ -365,7 +379,7 @@ class NotificationDisplayManager { // There may be a lot of messages mentioned here, across a lot of // conversations. But they'll all be for one account, so they'll // fall under one notification group. - final groupKey = _groupKey(data); + final groupKey = _groupKey(data.realmUrl, data.userId); // Find any conversations we can cancel the notification for. // The API doesn't lend itself to removing individual messages as @@ -421,6 +435,20 @@ class NotificationDisplayManager { } } + static Future removeNotificationsForAccount(Uri realmUrl, int userId) async { + assert(defaultTargetPlatform == TargetPlatform.android); + + final groupKey = _groupKey(realmUrl, userId); + final activeNotifications = await _androidHost.getActiveNotifications( + desiredExtras: []); + for (final statusBarNotification in activeNotifications) { + if (statusBarNotification.notification.group == groupKey) { + await _androidHost.cancel( + tag: statusBarNotification.tag, id: statusBarNotification.id); + } + } + } + /// The constant numeric "ID" we use for all non-test notifications, /// along with unique tags. /// @@ -445,10 +473,10 @@ class NotificationDisplayManager { return '$groupKey|$conversation'; } - static String _groupKey(FcmMessageWithIdentity data) { + static String _groupKey(Uri realmUrl, int userId) { // The realm URL can't contain a `|`, because `|` is not a URL code point: // https://url.spec.whatwg.org/#url-code-points - return "${data.realmUrl}|${data.userId}"; + return "$realmUrl|$userId"; } static String _personKey(Uri realmUrl, int userId) => "$realmUrl|$userId"; @@ -464,6 +492,8 @@ class NotificationDisplayManager { required BuildContext context, required Uri url, }) { + assert(defaultTargetPlatform == TargetPlatform.android); + final globalStore = GlobalStoreWidget.of(context); assert(debugLog('got notif: url: $url')); @@ -492,6 +522,7 @@ class NotificationDisplayManager { /// generated with [NotificationOpenPayload.buildUrl] while creating /// the notification. static Future navigateForNotification(Uri url) async { + assert(defaultTargetPlatform == TargetPlatform.android); assert(debugLog('opened notif: url: $url')); NavigatorState navigator = await ZulipApp.navigator; diff --git a/test/model/actions_test.dart b/test/model/actions_test.dart index ab97625c98..e7ba1339dd 100644 --- a/test/model/actions_test.dart +++ b/test/model/actions_test.dart @@ -1,7 +1,11 @@ +import 'dart:io'; + import 'package:checks/checks.dart'; +import 'package:firebase_messaging/firebase_messaging.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; +import 'package:http/testing.dart' as http_testing; import 'package:zulip/model/actions.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/receive.dart'; @@ -12,7 +16,9 @@ import '../fake_async.dart'; import '../model/binding.dart'; import '../model/store_checks.dart'; import '../model/test_store.dart'; +import '../notifications/display_test.dart'; import '../stdlib_checks.dart'; +import '../test_images.dart'; import 'store_test.dart'; void main() { @@ -21,6 +27,24 @@ void main() { late PerAccountStore store; late FakeApiConnection connection; + http.Client makeFakeHttpClient({http.Response? response, Exception? exception}) { + return http_testing.MockClient((request) async { + assert((response != null) ^ (exception != null)); + if (exception != null) throw exception; + return response!; // TODO return 404 on non avatar urls + }); + } + + final fakeHttpClientGivingSuccess = makeFakeHttpClient( + response: http.Response.bytes(kSolidBlueAvatar, HttpStatus.ok)); + + T runWithHttpClient( + T Function() callback, { + http.Client Function()? httpClientFactory, + }) { + return http.runWithClient(callback, httpClientFactory ?? () => fakeHttpClientGivingSuccess); + } + Future prepare({String? ackedPushToken = '123'}) async { addTearDown(testBinding.reset); final selfAccount = eg.selfAccount.copyWith(ackedPushToken: Value(ackedPushToken)); @@ -121,6 +145,24 @@ void main() { async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration); check(newConnection.isOpen).isFalse(); })); + + test('notifications are removed after logout', () => awaitFakeAsync((async) async { + await prepare(); + testBinding.firebaseMessagingInitialToken = '123'; + addTearDown(NotificationService.debugReset); + NotificationService.debugBackgroundIsolateIsLive = false; + await runWithHttpClient(NotificationService.instance.start); + + // Create a notification to check that it's removed after logout + final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); + testBinding.firebaseMessaging.onMessage.add( + RemoteMessage(data: messageFcmMessage(message).toJson())); + async.flushMicrotasks(); + check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty(); + + await logOutAccount(testBinding.globalStore, eg.selfAccount.id); + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + })); }); group('unregisterToken', () { diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index b1c56b55b1..ccba7e24cc 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -26,9 +26,9 @@ import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; import 'package:zulip/widgets/theme.dart'; +import '../example_data.dart' as eg; import '../fake_async.dart'; import '../model/binding.dart'; -import '../example_data.dart' as eg; import '../model/narrow_checks.dart'; import '../stdlib_checks.dart'; import '../test_images.dart'; @@ -114,7 +114,10 @@ void main() { return http.runWithClient(callback, httpClientFactory ?? () => fakeHttpClientGivingSuccess); } - Future init() async { + Future init({bool addSelfAccount = true}) async { + if (addSelfAccount) { + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + } addTearDown(testBinding.reset); testBinding.firebaseMessagingInitialToken = '012abc'; addTearDown(NotificationService.debugReset); @@ -872,7 +875,8 @@ void main() { }))); test('remove: different realm URLs but same user-ids and same message-ids', () => runWithHttpClient(() => awaitFakeAsync((async) async { - await init(); + await init(addSelfAccount: false); + final stream = eg.stream(); const topic = 'Some Topic'; final conversationKey = 'stream:${stream.streamId}:some topic'; @@ -881,6 +885,7 @@ void main() { realmUrl: Uri.parse('https://1.chat.example'), id: 1001, user: eg.user(userId: 1001)); + await testBinding.globalStore.add(account1, eg.initialSnapshot()); final message1 = eg.streamMessage(id: 1000, stream: stream, topic: topic); final data1 = messageFcmMessage(message1, account: account1, streamName: stream.name); @@ -890,6 +895,7 @@ void main() { realmUrl: Uri.parse('https://2.chat.example'), id: 1002, user: eg.user(userId: 1001)); + await testBinding.globalStore.add(account2, eg.initialSnapshot()); final message2 = eg.streamMessage(id: 1000, stream: stream, topic: topic); final data2 = messageFcmMessage(message2, account: account2, streamName: stream.name); @@ -917,19 +923,21 @@ void main() { }))); test('remove: different user-ids but same realm URL and same message-ids', () => runWithHttpClient(() => awaitFakeAsync((async) async { - await init(); + await init(addSelfAccount: false); final realmUrl = eg.realmUrl; final stream = eg.stream(); const topic = 'Some Topic'; final conversationKey = 'stream:${stream.streamId}:some topic'; final account1 = eg.account(id: 1001, user: eg.user(userId: 1001), realmUrl: realmUrl); + await testBinding.globalStore.add(account1, eg.initialSnapshot()); final message1 = eg.streamMessage(id: 1000, stream: stream, topic: topic); final data1 = messageFcmMessage(message1, account: account1, streamName: stream.name); final groupKey1 = '${account1.realmUrl}|${account1.userId}'; final account2 = eg.account(id: 1002, user: eg.user(userId: 1002), realmUrl: realmUrl); + await testBinding.globalStore.add(account2, eg.initialSnapshot()); final message2 = eg.streamMessage(id: 1000, stream: stream, topic: topic); final data2 = messageFcmMessage(message2, account: account2, streamName: stream.name); @@ -955,6 +963,76 @@ void main() { receiveFcmMessage(async, removeFcmMessage([message2], account: account2)); check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); }))); + + test('removeNotificationsForAccount: removes notifications', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(); + final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); + receiveFcmMessage(async, messageFcmMessage(message)); + check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty(); + + await NotificationDisplayManager.removeNotificationsForAccount( + eg.selfAccount.realmUrl, eg.selfAccount.userId); + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + }))); + + test('removeNotificationsForAccount: leaves notifications for other accounts (same realm URL)', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(addSelfAccount: false); + + final realmUrl = eg.realmUrl; + final account1 = eg.account(id: 1001, user: eg.user(userId: 1001), realmUrl: realmUrl); + final account2 = eg.account(id: 1002, user: eg.user(userId: 1002), realmUrl: realmUrl); + await testBinding.globalStore.add(account1, eg.initialSnapshot()); + await testBinding.globalStore.add(account2, eg.initialSnapshot()); + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + + final message1 = eg.streamMessage(); + final message2 = eg.streamMessage(); + receiveFcmMessage(async, messageFcmMessage(message1, account: account1)); + receiveFcmMessage(async, messageFcmMessage(message2, account: account2)); + check(testBinding.androidNotificationHost.activeNotifications) + .length.equals(4); + + await NotificationDisplayManager.removeNotificationsForAccount( + realmUrl, account1.userId); + check(testBinding.androidNotificationHost.activeNotifications) + ..length.equals(2) + ..first.notification.group.equals('$realmUrl|${account2.userId}'); + }))); + + test('removeNotificationsForAccount leaves notifications for other accounts (same user-ids)', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(addSelfAccount: false); + + final userId = 1001; + final account1 = eg.account( + id: 1001, user: eg.user(userId: userId), + realmUrl: Uri.parse('https://realm1.example')); + final account2 = eg.account( + id: 1002, user: eg.user(userId: userId), + realmUrl: Uri.parse('https://realm2.example')); + await testBinding.globalStore.add(account1, eg.initialSnapshot()); + await testBinding.globalStore.add(account2, eg.initialSnapshot()); + + final message1 = eg.streamMessage(); + final message2 = eg.streamMessage(); + receiveFcmMessage(async, messageFcmMessage(message1, account: account1)); + receiveFcmMessage(async, messageFcmMessage(message2, account: account2)); + check(testBinding.androidNotificationHost.activeNotifications) + .length.equals(4); + + await NotificationDisplayManager.removeNotificationsForAccount(account1.realmUrl, userId); + check(testBinding.androidNotificationHost.activeNotifications) + ..length.equals(2) + ..first.notification.group.equals('${account2.realmUrl}|$userId'); + }))); + + test('removeNotificationsForAccount does nothing if there are no notifications', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(); + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + + await NotificationDisplayManager.removeNotificationsForAccount(eg.selfAccount.realmUrl, eg.selfAccount.userId); + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + }))); }); group('NotificationDisplayManager open', () { @@ -976,7 +1054,7 @@ void main() { Future prepare(WidgetTester tester, {bool early = false, bool withAccount = true}) async { - await init(); + await init(addSelfAccount: false); pushedRoutes = []; final testNavObserver = TestNavigatorObserver() ..onPushed = (route, prevRoute) => pushedRoutes.add(route);