From b542c614c8226253cf0a2a66b9a9532250e4204a Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Sun, 11 Aug 2024 22:19:45 +0530 Subject: [PATCH] notif: Handle remove notification message Fixes: #341 Co-authored-by: Greg Price --- lib/notifications/display.dart | 54 ++++++++- test/model/binding.dart | 38 +++++-- test/notifications/display_test.dart | 162 ++++++++++++++++++++++++++- 3 files changed, 238 insertions(+), 16 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index c0aa0ca831..0b4ba0c4c1 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -84,7 +84,7 @@ class NotificationDisplayManager { static void onFcmMessage(FcmMessage data, Map dataJson) { switch (data) { case MessageFcmMessage(): _onMessageFcmMessage(data, dataJson); - case RemoveFcmMessage(): break; // TODO(#341) handle + case RemoveFcmMessage(): _onRemoveFcmMessage(data); case UnexpectedFcmMessage(): break; // TODO(log) } } @@ -155,6 +155,9 @@ class NotificationDisplayManager { messagingStyle: messagingStyle, number: messagingStyle.messages.length, + extras: { + kExtraZulipMessageId: data.zulipMessageId.toString(), + }, contentIntent: PendingIntent( // TODO make intent URLs distinct, instead of requestCode @@ -202,6 +205,55 @@ class NotificationDisplayManager { ); } + static void _onRemoveFcmMessage(RemoveFcmMessage data) async { + assert(debugLog('notif remove zulipMessageIds: ${data.zulipMessageIds}')); + + final groupKey = _groupKey(data); + + var haveRemaining = false; + final activeNotifications = await ZulipBinding.instance.androidNotificationHost + .getActiveNotifications(desiredExtras: [kExtraZulipMessageId]); + for (final statusBarNotification in activeNotifications) { + if (statusBarNotification == null) continue; // TODO(pigeon) eliminate this case + final notification = statusBarNotification.notification; + + // Sadly we don't get toString on Pigeon data classes: flutter#59027 + assert(debugLog(' existing notif' + ' id: ${statusBarNotification.id}, tag: ${statusBarNotification.tag},' + ' notification: (group: ${notification.group}, extras: ${notification.extras}))')); + + // Don't act on notifications that are for other Zulip accounts/identities. + if (notification.group != groupKey) continue; + + // Don't act on the summary notification for the group. + if (statusBarNotification.tag == groupKey) continue; + + final lastMessageIdStr = notification.extras[kExtraZulipMessageId]; + if (lastMessageIdStr == null) continue; + final lastMessageId = int.parse(lastMessageIdStr, radix: 10); + if (data.zulipMessageIds.contains(lastMessageId)) { + // The latest Zulip message in this conversation was read. + // That's our cue to cancel the notification for the conversation. + await ZulipBinding.instance.androidNotificationHost + .cancel(tag: statusBarNotification.tag, id: statusBarNotification.id); + } else { + // This notification is for another conversation that's still unread. + // We won't cancel the summary notification. + haveRemaining = true; + } + } + + if (!haveRemaining) { + // The notification group is now empty; it had no notifications we didn't + // just cancel, except the summary notification. Cancel that one too. + await ZulipBinding.instance.androidNotificationHost + .cancel(tag: groupKey, id: notificationIdAsHashOf(groupKey)); + } + } + + @visibleForTesting + static const kExtraZulipMessageId = 'zulipMessageId'; + /// A notification ID, derived as a hash of the given string key. /// /// The result fits in 31 bits, the size of a nonnegative Java `int`, diff --git a/test/model/binding.dart b/test/model/binding.dart index 9152fbaa1e..d69c31bff2 100644 --- a/test/model/binding.dart +++ b/test/model/binding.dart @@ -555,10 +555,13 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi { } List _notifyCalls = []; + Iterable get activeNotifications => _activeNotifications.values; + final Map<(int, String?), StatusBarNotification> _activeNotifications = {}; + final Map _activeNotificationsMessagingStyle = {}; - /// Clears all active notifications that have been created via [notify]. - void clearActiveNotifications() { + /// Clears all messaginging style of active notifications that have been created via [notify]. + void clearActiveNotificationsMessagingStyle() { _activeNotificationsMessagingStyle.clear(); } @@ -599,6 +602,11 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi { )); if (tag != null) { + _activeNotifications[(id, tag)] = StatusBarNotification( + id: id, + notification: Notification(group: groupKey ?? '', extras: extras ?? {}), + tag: tag); + _activeNotificationsMessagingStyle[tag] = messagingStyle == null ? null : MessagingStyle( @@ -613,24 +621,30 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi { key: message.person.key, name: message.person.name, iconBitmap: null)), - ).toList()); + ).toList(growable: false)); } } @override - Future getActiveNotificationMessagingStyleByTag(String tag) async => - _activeNotificationsMessagingStyle[tag]; + Future> getActiveNotifications({required List desiredExtras}) async { + return _activeNotifications.values.map((statusNotif) { + final notificationExtras = statusNotif.notification.extras; + statusNotif.notification.extras = Map.fromEntries( + desiredExtras + .map((key) => MapEntry(key, notificationExtras[key])) + .where((entry) => entry.value != null) + ); + return statusNotif; + }).toList(growable: false); + } @override - Future> getActiveNotifications({required List desiredExtras}) { - // TODO: implement getActiveNotifications - throw UnimplementedError(); - } + Future getActiveNotificationMessagingStyleByTag(String tag) async => + _activeNotificationsMessagingStyle[tag]; @override - Future cancel({String? tag, required int id}) { - // TODO: implement cancel - throw UnimplementedError(); + Future cancel({String? tag, required int id}) async { + _activeNotifications.remove((id, tag)); } } diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 16f50fdc63..5657f0316b 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -6,7 +6,7 @@ import 'package:checks/checks.dart'; import 'package:collection/collection.dart'; import 'package:fake_async/fake_async.dart'; import 'package:firebase_messaging/firebase_messaging.dart'; -import 'package:flutter/widgets.dart'; +import 'package:flutter/widgets.dart' hide Notification; import 'package:flutter_local_notifications/flutter_local_notifications.dart' hide Message, Person; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; @@ -74,6 +74,20 @@ MessageFcmMessage messageFcmMessage( }) as MessageFcmMessage; } +RemoveFcmMessage removeFcmMessage(List zulipMessages, {Account? account}) { + account ??= eg.selfAccount; + return FcmMessage.fromJson({ + "event": "remove", + + "server": "zulip.example.cloud", + "realm_id": "4", + "realm_uri": account.realmUrl.toString(), + "user_id": account.userId.toString(), + + "zulip_message_ids": zulipMessages.map((e) => e.id).join(','), + }) as RemoveFcmMessage; +} + void main() { TestZulipBinding.ensureInitialized(); final zulipLocalizations = GlobalLocalizations.zulipLocalizations; @@ -171,7 +185,10 @@ void main() { ..number.equals(messageStyleMessages.length) ..color.equals(kZulipBrandColor.value) ..smallIconResourceName.equals('zulip_notification') - ..extras.isNull() + ..extras.which((it) => it.isNotNull() + ..deepEquals({ + NotificationDisplayManager.kExtraZulipMessageId: data.zulipMessageId.toString(), + })) ..groupKey.equals(expectedGroupKey) ..isGroupSummary.isNull() ..inboxStyle.isNull() @@ -215,7 +232,7 @@ void main() { expectedIsGroupConversation: expectedIsGroupConversation, expectedTitle: expectedTitle, expectedTagComponent: expectedTagComponent); - testBinding.androidNotificationHost.clearActiveNotifications(); + testBinding.androidNotificationHost.clearActiveNotificationsMessagingStyle(); testBinding.firebaseMessaging.onBackgroundMessage.add( RemoteMessage(data: data.toJson())); @@ -233,6 +250,28 @@ void main() { async.flushMicrotasks(); } + Condition conditionActiveNotif(MessageFcmMessage data, String tagComponent) { + final expectedGroupKey = '${data.realmUri}|${data.userId}'; + final expectedTag = '$expectedGroupKey|$tagComponent'; + return (it) => it.isA() + ..id.equals(NotificationDisplayManager.notificationIdAsHashOf(expectedTag)) + ..notification.which((it) => it + ..group.equals(expectedGroupKey) + ..extras.deepEquals({ + NotificationDisplayManager.kExtraZulipMessageId: data.zulipMessageId.toString(), + })) + ..tag.equals(expectedTag); + } + + Condition conditionSummaryActiveNotif(String expectedGroupKey) { + return (it) => it.isA() + ..id.equals(NotificationDisplayManager.notificationIdAsHashOf(expectedGroupKey)) + ..notification.which((it) => it + ..group.equals(expectedGroupKey) + ..extras.isEmpty()) + ..tag.equals(expectedGroupKey); + } + test('stream message', () => runWithHttpClient(() => awaitFakeAsync((async) async { await init(); final stream = eg.stream(); @@ -495,6 +534,112 @@ void main() { expectedTitle: eg.selfUser.fullName, expectedTagComponent: 'dm:${message.allRecipientIds.join(",")}'); }))); + + test('remove: smoke', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(); + final message = eg.streamMessage(); + final data = messageFcmMessage(message); + final expectedGroupKey = '${data.realmUri}|${data.userId}'; + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + await receiveFcmMessage(async, data); + check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ + conditionActiveNotif(data, 'stream:${message.streamId}:${message.topic}'), + conditionSummaryActiveNotif(expectedGroupKey), + ]); + testBinding.firebaseMessaging.onMessage.add( + RemoteMessage(data: removeFcmMessage([message]).toJson())); + async.flushMicrotasks(); + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + await receiveFcmMessage(async, data); + check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ + conditionActiveNotif(data, 'stream:${message.streamId}:${message.topic}'), + conditionSummaryActiveNotif(expectedGroupKey), + ]); + testBinding.firebaseMessaging.onBackgroundMessage.add( + RemoteMessage(data: removeFcmMessage([message]).toJson())); + async.flushMicrotasks(); + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + }))); + + test('remove: clears conversation only if the removal event is for the last message', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(); + final stream = eg.stream(); + const topicA = 'Topic A'; + final message1 = eg.streamMessage(stream: stream, topic: topicA); + final data1 = messageFcmMessage(message1, streamName: stream.name); + final message2 = eg.streamMessage(stream: stream, topic: topicA); + final data2 = messageFcmMessage(message2, streamName: stream.name); + final message3 = eg.streamMessage(stream: stream, topic: topicA); + final data3 = messageFcmMessage(message3, streamName: stream.name); + final expectedGroupKey = '${data1.realmUri}|${data1.userId}'; + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + + await receiveFcmMessage(async, data1); + await receiveFcmMessage(async, data2); + await receiveFcmMessage(async, data3); + + check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ + conditionActiveNotif(data3, 'stream:${stream.streamId}:$topicA'), + conditionSummaryActiveNotif(expectedGroupKey), + ]); + + testBinding.firebaseMessaging.onMessage.add( + RemoteMessage(data: removeFcmMessage([message1, message2]).toJson())); + async.flushMicrotasks(); + + check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ + conditionActiveNotif(data3, 'stream:${stream.streamId}:$topicA'), + conditionSummaryActiveNotif(expectedGroupKey), + ]); + + testBinding.firebaseMessaging.onMessage.add( + RemoteMessage(data: removeFcmMessage([message3]).toJson())); + async.flushMicrotasks(); + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + }))); + + test('remove: clears summary notification only if all conversation notifications are cleared', () => runWithHttpClient(() => awaitFakeAsync((async) async { + await init(); + final stream = eg.stream(); + const topicA = 'Topic A'; + final message1 = eg.streamMessage(stream: stream, topic: topicA); + final data1 = messageFcmMessage(message1, streamName: stream.name); + const topicB = 'Topic B'; + final message2 = eg.streamMessage(stream: stream, topic: topicB); + final data2 = messageFcmMessage(message2, streamName: stream.name); + final expectedGroupKey = '${data1.realmUri}|${data1.userId}'; + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + + await receiveFcmMessage(async, data1); + await receiveFcmMessage(async, data2); + + check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ + conditionActiveNotif(data1, 'stream:${stream.streamId}:$topicA'), + conditionSummaryActiveNotif(expectedGroupKey), + conditionActiveNotif(data2, 'stream:${stream.streamId}:$topicB'), + ]); + + testBinding.firebaseMessaging.onMessage.add( + RemoteMessage(data: removeFcmMessage([message1]).toJson())); + async.flushMicrotasks(); + + check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ + conditionSummaryActiveNotif(expectedGroupKey), + conditionActiveNotif(data2, 'stream:${stream.streamId}:$topicB'), + ]); + + testBinding.firebaseMessaging.onMessage.add( + RemoteMessage(data: removeFcmMessage([message2]).toJson())); + async.flushMicrotasks(); + + check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); + }))); }); group('NotificationDisplayManager open', () { @@ -700,3 +845,14 @@ extension on Subject { Subject get timestampMs => has((x) => x.timestampMs, 'timestampMs'); Subject get person => has((x) => x.person, 'person'); } + +extension on Subject { + Subject get group => has((x) => x.group, 'group'); + Subject> get extras => has((x) => x.extras, 'extras'); +} + +extension on Subject { + Subject get id => has((x) => x.id, 'id'); + Subject get notification => has((x) => x.notification, 'notification'); + Subject get tag => has((x) => x.tag, 'tag'); +}