From 7247ef11aca1327215387e231c04485f61d59e93 Mon Sep 17 00:00:00 2001 From: Billy Date: Sun, 16 Mar 2025 05:20:21 +0530 Subject: [PATCH 1/3] notif [nfc]: Change `_groupKey` method to take a realm URL and user ID The `_groupKey` method currently takes an `FcmMessageWithIdentity` object as an argument. However, this method only requires the realm URL and user ID from that object. This change modifies the method signature to directly accept `realmUrl` and `userId` as arguments. This change is made to facilitate calling `_groupKey` from contexts where an `FcmMessageWithIdentity` object is not available. Discussion: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Removing.20notifications.20for.20logged.20out.20account/near/2088813 --- lib/notifications/display.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 1d74db6854..9be1364ffb 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'; @@ -231,7 +231,7 @@ 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 oldMessagingStyle = await _androidHost @@ -365,7 +365,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 @@ -445,10 +445,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"; From 2fd60985dec03c63c6ddf19726489986a5a92494 Mon Sep 17 00:00:00 2001 From: Billy Date: Sun, 16 Mar 2025 05:21:02 +0530 Subject: [PATCH 2/3] notif: Ignore notifications for logged out accounts Fixes: #1264 Notifications are ignored for accounts which were logged out but the request to stop receiving notifications failed due to some reason. Also when an account is logged out, any existing notifications for that account is removed from the UI. --- lib/model/actions.dart | 7 +++ lib/notifications/display.dart | 24 ++++++++ test/model/actions_test.dart | 42 +++++++++++++ test/notifications/display_test.dart | 88 ++++++++++++++++++++++++++-- 4 files changed, 156 insertions(+), 5 deletions(-) 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 9be1364ffb..4d777bdf72 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -234,6 +234,18 @@ class NotificationDisplayManager { 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); @@ -421,6 +433,18 @@ class NotificationDisplayManager { } } + static Future removeNotificationsForAccount(Uri realmUrl, int userId) async { + 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. /// 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); From 378596390a42173299447155dc6f9478000469e3 Mon Sep 17 00:00:00 2001 From: Billy Date: Wed, 5 Mar 2025 19:55:48 +0530 Subject: [PATCH 3/3] notif [nfc]: Add Android-only asserts in NotificationDisplayManager methods NotificationDisplayManager's public methods only get invoked on Android, and their implementations basically assume the platform is Android. So, add these asserts, to make that assumption clearer and to fail early if we accidentally call one of these on iOS. When we eventually take more hands-on control of notifications on iOS, we might either adapt this class to support iOS too, or make a new class for iOS and make this one's name and interface clearer that it's Android-only. Discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/NotificationDisplayManager.20Android-only/near/2099196 --- lib/notifications/display.dart | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 4d777bdf72..00a5f6fda5 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.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); @@ -434,6 +436,8 @@ 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: []); @@ -488,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')); @@ -516,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;