notif: Handle remove notification message #878

rajveermalviya · 2024-08-11T16:53:53Z

Fixes: #341

sm-sayedi

Thanks @rajveermalviya for working on this nice feature. Did a manual testing of the feature which works fine. LGTM. Moving on to the mentor review! cc @sumanthvrao @kenclary

kenclary

lgtm too :)

chrisbobbe

Thanks! Here's a light review, and I expect Greg may have more to say since he knows the Android-notifications system better than I do.

chrisbobbe · 2024-08-14T22:10:23Z

test/model/binding.dart

@@ -560,8 +560,8 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi {

  final Map<String, MessagingStyle?> _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].


nit: spelling; also, instead of "messaging style", maybe this should say [MessagingStyle]? It's not intuitively clear what a "messaging style" is, and maybe Android could have chosen a better name for it, but using the identifier should at least shorten the reader's path to looking up what it means in the Android documentation.

chrisbobbe · 2024-08-14T22:11:28Z

lib/notifications/display.dart

+      // Don't act on the summary notification for the group.
+      if (statusBarNotification.tag == groupKey) continue;
+
+      final lastMessageIdStr = notification.extras[kExtraZulipMessageId];


As an independent improvement, could you write a dartdoc for kExtraZulipMessageId? I found myself wondering what it was, while reading this.

rajveermalviya · 2024-08-15T10:50:51Z

Thanks for the reviews @kenclary and @chrisbobbe!
Pushed a new revision @chrisbobbe, PTAL.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2024-08-15T19:54:16Z

lib/notifications/display.dart

+  /// The identifier key for the message-id of embedded in the
+  /// notification's extras bundle.
+  @visibleForTesting
+  static const kExtraZulipMessageId = 'zulipMessageId';


Thanks for adding this dartdoc. Does it have a missing word or something? I'm not parsing "[…] the message-id of embedded […]".

Also, "the notification" makes it sound like I should already know what kind of thing—and also which specific thing of that kind—is being referred to. Is it an instance of StatusBarNotification, or of Notification? Looking through the code, I think this string is used as a key in the Notification.extras map, so it seems helpful to include a reference to [Notification.extras].

I'm not as familiar with the Android-notifications code as you and Greg are, and one question I had was whether this constant's value is part of the Zulip API, or if it's purely internal to our code. It seems like the latter, but I think if the dartdoc were fleshed out a bit more to answer that question, I think it would be helpful.

chrisbobbe · 2024-08-15T19:55:58Z

test/model/binding.dart

-  /// Clears all active notifications that have been created via [notify].
-  void clearActiveNotifications() {
+  /// Clears all [MessagingStyle] of active notifications that have been created via [notify].
+  void clearActiveNotificationsMessagingStyle() {


_activeNotificationsMessagingStyle is a collection, so do we want to say [MessagingStyle]s, plural?

gnprice

Thanks @rajveermalviya, and thanks @sm-sayedi and @chrisbobbe for the previous reviews!

Generally this looks great. I like these test cases. Various small comments below, and I see there are a couple open from Chris's review today above.

Let's add one other test case: the RemoveFcmMessage shouldn't affect any notifications for other accounts. To make the test as sharp as possible, let's exercise the case where by sheer bad luck the other account's notification has the same message ID, and is to the same user ID — the only difference is the realm URL. (This isn't even totally improbable: if the accounts are in two different very small or new servers, our user could easily have the same very small user ID in both and then the two servers could even hit the same small message ID around the same time.)

I guess to be thorough let's make that two test cases (or perhaps two steps in one test case): different realm URL but same user ID, and same realm URL but different user ID. And in both cases the same message ID. (For the same realm and different user IDs, that's even less improbable: our user has two accounts in the realm, both got notified by some message, and now they've read the message from just one account. I guess for that scenario it would actually be a defensible product choice to remove both notifications, since it's truly the same message and not two unrelated messages that happen to have the same ID… but I like the behavior we have, so let's test it.)

I'll also want to look closer at the implementation of _onRemoveFcmMessage in comparison with the zulip-mobile version — it looks right, and most of the comments even look familiar, but I'll want to do a careful line-by-line comparison to be sure we understand any differences. (Skipping that in this round only because it's late evening here.)

gnprice · 2024-08-16T06:49:25Z

pigeon/notifications.dart

+  final String tag;
+}


My draft had some comments here and on Notification about other properties that were included or not included, and why. Those comments were intended to include in the final commits (and I now realize that was ambiguous when they appeared in the context of a draft commit) — I think they'll be useful if we later return to these bindings and consider extending them, for implementing some future change to notifications.

gnprice · 2024-08-16T06:53:32Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+            StatusBarNotification(
+                it.id.toLong(),
+                Notification(
+                    it.notification.group,
+                    desiredExtras
+                        .associateWith { key -> it.notification.extras.getString(key) }
+                        .filter { entry -> entry.value != null }
+                ),
+                it.tag


nit: put id and tag before the body of the notification — in general, short arguments that identify the name or location of something go before long multi-line arguments that provide its content

(probably it'd be good to reorder them that way in the class declaration, too)

gnprice · 2024-08-16T06:58:57Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+                        .associateWith { key -> it.notification.extras.getString(key) }
+                        .filter { entry -> entry.value != null }


This semantics works fine since it covers the one use case we actually have — it's definitely simpler to think about than the one in my draft, and that's a good improvement.

It needs to be described in the method's doc, though (in pigeon/notification.dart). It looks to be:

Each requested extra is included if it's present with type string.

If it's absent, or not a string, it's skipped.

If not written down explicitly, that's the sort of thing that could lead to some confused time spent debugging when someone in the future tries to add a number or a boolean or something, and then it just doesn't show up — they'd wonder if they typoed the key, or if the extra didn't make it onto the notification for some reason, or what.

gnprice · 2024-08-16T07:01:59Z

lib/notifications/display.dart

+      if (statusBarNotification.tag == groupKey) continue;
+
+      final lastMessageIdStr = notification.extras[kExtraZulipMessageId];
+      if (lastMessageIdStr == null) continue;


This case shouldn't happen, right? It'd be a bug within this file, or at least in our understanding of how the Android SDK features we're using are supposed to work.

So let's make that explicit:

Suggested change

if (lastMessageIdStr == null) continue;

assert(lastMessageIdStr != null);

if (lastMessageIdStr == null) continue; // TODO(log)

gnprice · 2024-08-16T07:02:29Z

lib/notifications/display.dart

+
+      final lastMessageIdStr = notification.extras[kExtraZulipMessageId];
+      if (lastMessageIdStr == null) continue;
+      final lastMessageId = int.parse(lastMessageIdStr, radix: 10);


Cool, good use of radix: 10

gnprice · 2024-08-16T07:16:21Z

test/notifications/display_test.dart

+      testBinding.firebaseMessaging.onMessage.add(
+        RemoteMessage(data: removeFcmMessage([message]).toJson()));
+      async.flushMicrotasks();


nit: this can use receiveFcmMessage, right?

(and a few more places below)

gnprice · 2024-08-16T07:17:27Z

test/notifications/display_test.dart

+      check(testBinding.androidNotificationHost.activeNotifications).isEmpty();
+
+      check(testBinding.androidNotificationHost.activeNotifications).isEmpty();


nit:

Suggested change

check(testBinding.androidNotificationHost.activeNotifications).isEmpty();

check(testBinding.androidNotificationHost.activeNotifications).isEmpty();

check(testBinding.androidNotificationHost.activeNotifications).isEmpty();

these are close enough together the reader can be expected to remember the previous one 🙂

gnprice · 2024-08-16T07:19:36Z

test/notifications/display_test.dart

+
+      check(testBinding.androidNotificationHost.activeNotifications).isEmpty();
+      await receiveFcmMessage(async, data);
+      check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[
+        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();


hmm but maybe just delete this whole stanza? seems like it's a copy of the one above. I don't think it's expected to get a MessageFcmMessage for the same Zulip message twice. If that's intentional and you think it's a scenario which should be tested, it needs at least a comment explaining the narrative of it.

gnprice · 2024-08-16T07:22:25Z

test/notifications/display_test.dart

+      await receiveFcmMessage(async, data1);
+      await receiveFcmMessage(async, data2);
+      await receiveFcmMessage(async, data3);
+
+      check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[


nit:

Suggested change

await receiveFcmMessage(async, data1);

await receiveFcmMessage(async, data2);

await receiveFcmMessage(async, data3);

check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[

await receiveFcmMessage(async, data1);

await receiveFcmMessage(async, data2);

await receiveFcmMessage(async, data3);

check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[

Generally when a test has code like "make this change, this change, this change; then check the state of the world", I like to group those into one stanza. Then a blank line separates the checks from the next round of making changes.

That way the stanzas, or the blank lines separating them, serve to group together those change/check pairs.

gnprice · 2024-08-16T07:26:15Z

test/notifications/display_test.dart

+      testBinding.firebaseMessaging.onMessage.add(
+        RemoteMessage(data: removeFcmMessage([message1, message2]).toJson()));
+      async.flushMicrotasks();
+
+      check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[


This could benefit from a brief comment telling the story of the test:

Suggested change

testBinding.firebaseMessaging.onMessage.add(

RemoteMessage(data: removeFcmMessage([message1, message2]).toJson()));

async.flushMicrotasks();

check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[

// A RemoveFcmMessage for the first two messages; the notification stays.

testBinding.firebaseMessaging.onMessage.add(

RemoteMessage(data: removeFcmMessage([message1, message2]).toJson()));

async.flushMicrotasks();

check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[

(and then a couple of nits above apply to other aspects of these lines)

Similarly later in this test case:

// Then a RemoveFcmMessage for the last message; clear the notification.

The test name above already gives a good summary of the plot. But I think these are helpful to make sure the reader sees the point, and how the details connect to the idea that's being tested.

rajveermalviya · 2024-08-16T23:49:08Z

Thanks for the reviews @chrisbobbe and @gnprice! Pushed a new revision, PTAL.

chrisbobbe · 2024-08-17T01:24:20Z

Cool, thanks! I see my review has been addressed, so I'll remove the maintainer-review label. 🙂

gnprice

Thanks for the revision! This looks great.

One nit below, and then I've pushed a few added commits on top:
b9addf2 notif [nfc]: Make shorter local alias for ZulipBinding.instance.androidNotificationHost
f2c8e40 notif [nfc]: Keep more comments in remove-notif from zulip-mobile implementation
aa5c31d notif [nfc]: Clarify meaning of last-message-ID extra

I did that comparison to the zulip-mobile code which I mentioned above, and all the logic looks like a match but there were more comments I felt should come over too; that's the second commit above.

I also fixed the one nit. @chrisbobbe would you take a look at the new commits? In particular see if they answer the questions you had at #878 (comment) above. Then I think this is ready to merge.

gnprice · 2024-08-19T23:42:19Z

pigeon/notifications.dart

+  final Map<String?, String?> extras;
+}


Suggested change

final Map<String?, String?> extras;

}

final Map<String?, String?> extras;

// Various other properties too; add them if needed.

}

(part of #878 (comment) )

gnprice · 2024-08-19T23:48:39Z

test/notifications/display_test.dart

+      // Check on background event; onBackgroundMessage
+      await receiveFcmMessage(async, data);
+      check(testBinding.androidNotificationHost.activeNotifications).deepEquals(<Condition<Object?>>[
+        conditionActiveNotif(data, 'stream:${message.streamId}:${message.topic}'),
+        conditionSummaryActiveNotif(expectedGroupKey),
+      ]);
+      testBinding.firebaseMessaging.onBackgroundMessage.add(


Ah, I see the difference from the previous stanza now. 🙂 (continuing from #878 (comment) )

Having a test for this makes sense, good idea.

chrisbobbe · 2024-08-21T22:44:01Z

I also fixed the one nit. @chrisbobbe would you take a look at the new commits? In particular see if they answer the questions you had at #878 (comment) above. Then I think this is ready to merge.

Yep, it's clear now, thanks! Please merge at will.

See: https://developer.android.com/reference/kotlin/androidx/core/app/NotificationManagerCompat?hl=en#getActiveNotifications() Co-authored-by: Greg Price <greg@zulip.com>

See: https://developer.android.com/reference/kotlin/androidx/core/app/NotificationManagerCompat?hl=en#cancel(java.lang.String,int) Co-authored-by: Greg Price <greg@zulip.com>

This change is similar to existing implementation in zulip-mobile: https://github.com/zulip/zulip-mobile/blob/2217c858e207f9f092651dd853051843c3f04422/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt#L116-L168 Fixes: zulip#341 Co-authored-by: Greg Price <greg@zulip.com>

…onMessagingStyleByTag` non-growable This better simulates real bindings behaviour.

…idNotificationHost This is quite a mouthful and we repeat it a lot in this file. A nice short local name also helps things fit more easily on a line.

…lementation These comments in the corresponding code in zulip-mobile: https://github.com/zulip/zulip-mobile/blob/2217c858e207f9f092651dd853051843c3f04422/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt#L116-L168 are explaining the same logic we've copied into this code. So the comments are equally relevant for explaining the code here too.

Each notification can be showing several Zulip messages. This extra carries the message ID of specifically the latest one.

gnprice · 2024-08-21T22:58:07Z

Thanks! Merged.

rajveermalviya force-pushed the pr-notif-remove branch from b542c61 to 68d0e57 Compare August 11, 2024 17:02

rajveermalviya added the buddy review GSoC buddy review needed. label Aug 11, 2024

rajveermalviya requested a review from sm-sayedi August 11, 2024 17:04

sm-sayedi approved these changes Aug 12, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Aug 12, 2024

sm-sayedi requested review from sumanthvrao and kenclary August 12, 2024 07:26

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Aug 13, 2024

rajveermalviya requested a review from chrisbobbe August 13, 2024 08:16

kenclary approved these changes Aug 13, 2024

View reviewed changes

chrisbobbe reviewed Aug 14, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-remove branch 2 times, most recently from 21c13ff to f5d05a1 Compare August 15, 2024 10:48

rajveermalviya added the integration review Added by maintainers when PR may be ready for integration label Aug 15, 2024

rajveermalviya requested a review from gnprice August 15, 2024 10:51

chrisbobbe reviewed Aug 15, 2024

View reviewed changes

gnprice reviewed Aug 16, 2024

View reviewed changes

gnprice mentioned this pull request Aug 16, 2024

Support polls (read-only) #885

Merged

rajveermalviya force-pushed the pr-notif-remove branch 2 times, most recently from 65f9094 to 1152d94 Compare August 16, 2024 23:39

rajveermalviya requested a review from gnprice August 16, 2024 23:49

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Aug 17, 2024

gnprice force-pushed the pr-notif-remove branch from 1152d94 to aa5c31d Compare August 20, 2024 17:56

gnprice reviewed Aug 20, 2024

View reviewed changes

gnprice added the maintainer review PR ready for review by Zulip maintainers label Aug 20, 2024

rajveermalviya requested a review from chrisbobbe August 20, 2024 22:18

rajveermalviya force-pushed the pr-notif-remove branch from aa5c31d to 7cd237a Compare August 20, 2024 22:19

rajveermalviya and others added 7 commits August 21, 2024 15:53

pigeon: Add binding for getActiveNotifications

a859e8a

See: https://developer.android.com/reference/kotlin/androidx/core/app/NotificationManagerCompat?hl=en#getActiveNotifications() Co-authored-by: Greg Price <greg@zulip.com>

pigeon: Add binding for cancel

f230423

See: https://developer.android.com/reference/kotlin/androidx/core/app/NotificationManagerCompat?hl=en#cancel(java.lang.String,int) Co-authored-by: Greg Price <greg@zulip.com>

notif test [nfc]: Make messages list returned by `getActiveNotificati…

1d65da7

…onMessagingStyleByTag` non-growable This better simulates real bindings behaviour.

notif [nfc]: Make shorter local alias for ZulipBinding.instance.andro…

d062713

…idNotificationHost This is quite a mouthful and we repeat it a lot in this file. A nice short local name also helps things fit more easily on a line.

notif [nfc]: Clarify meaning of last-message-ID extra

b521c45

Each notification can be showing several Zulip messages. This extra carries the message ID of specifically the latest one.

gnprice force-pushed the pr-notif-remove branch from 7cd237a to b521c45 Compare August 21, 2024 22:58

gnprice merged commit b521c45 into zulip:main Aug 21, 2024

gnprice mentioned this pull request Aug 21, 2024

Sketch removing notification when message read #864

Closed

rajveermalviya deleted the pr-notif-remove branch August 22, 2024 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: Handle remove notification message #878

notif: Handle remove notification message #878

rajveermalviya commented Aug 11, 2024

sm-sayedi left a comment •

edited

Loading

kenclary left a comment

chrisbobbe left a comment

chrisbobbe Aug 14, 2024

chrisbobbe Aug 14, 2024

rajveermalviya commented Aug 15, 2024

chrisbobbe left a comment

chrisbobbe Aug 15, 2024

chrisbobbe Aug 15, 2024

gnprice left a comment

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

gnprice Aug 16, 2024

rajveermalviya commented Aug 16, 2024

chrisbobbe commented Aug 17, 2024

gnprice left a comment

gnprice Aug 19, 2024

gnprice Aug 19, 2024

chrisbobbe commented Aug 21, 2024

gnprice commented Aug 21, 2024

		.associateWith { key -> it.notification.extras.getString(key) }
		.filter { entry -> entry.value != null }

	if (lastMessageIdStr == null) continue;
	assert(lastMessageIdStr != null);
	if (lastMessageIdStr == null) continue; // TODO(log)

		check(testBinding.androidNotificationHost.activeNotifications).isEmpty();

		check(testBinding.androidNotificationHost.activeNotifications).isEmpty();

notif: Handle remove notification message #878

notif: Handle remove notification message #878

Conversation

rajveermalviya commented Aug 11, 2024

sm-sayedi left a comment • edited Loading

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Aug 15, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Aug 16, 2024

chrisbobbe commented Aug 17, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 21, 2024

gnprice commented Aug 21, 2024

sm-sayedi left a comment •

edited

Loading