notif: Support migration of Android notification channels #981

rajveermalviya · 2024-10-04T19:17:09Z

(Split from #717)

Needed for #340, when updating notification channels to use a custom notification sound.

chrisbobbe

Thanks! Small comments below, and marking for Greg's review.

chrisbobbe · 2024-10-05T02:27:37Z

test/model/binding.dart

+  /// Consumes the log of sequence of calls made to [getNotificationChannels],
+  /// [deleteNotificationChannel] and [createNotificationChannel].


"log of sequence of calls" -> "log of calls"?

chrisbobbe · 2024-10-05T02:34:10Z

lib/notifications/display.dart

+      if (channel!.id == kChannelId) {
+        found = true;
+      } else {
+        await _androidHost.deleteNotificationChannel(channel.id);


It sounds like it would be helpful to mention this in the dartdoc; something like:

/// Create our notification channel, if it doesn't already exist. /// /// Deletes obsolete channels, if present, from old versions of the app.

chrisbobbe · 2024-10-05T02:46:26Z

lib/notifications/display.dart

+  // TODO(?) at launch make sure this channel-id doesn't collide with the one
+  // present in zulip-mobile.


More accurately, I think, the collision to avoid is with a channel ID that's active in the installed app the user will be upgrading from. In theory, that could be different from the one zulip-mobile has in main, right, because the user could be upgrading from a very old zulip-mobile straight to zulip-flutter?

I see in main zulip-mobile has "messages-3". There's a handy list of previous values in a code comment on it:

/** The channel ID we use for our one notification channel, which we use for all notifications. */ // Previous values: "default", "messages-1", (alpha-only: "messages-2") val CHANNEL_ID = "messages-3"

which…hmm, that list does include "messages-1". I'm not sure how old that one is, and how many of our users will still have that as their active channel.

Rather than tracking that down, maybe simplest to be conservative and just say something like

// TODO(launch) check this doesn't match zulip-mobile's current or previous channel IDs

rajveermalviya · 2024-10-05T03:29:51Z

Thanks for the review @chrisbobbe! Pushed a new revision PTAL.

chrisbobbe · 2024-10-07T21:30:58Z

Thanks! LGTM, over to @gnprice.

gnprice

Thanks @rajveermalviya, and thanks @chrisbobbe for the previous review!

Generally this looks great. Various comments below.

gnprice · 2024-10-07T23:51:56Z

test/model/binding.dart

+  }
+
+  @override
+  Future<void> deleteNotificationChannel(String channelId) async {


nit: avoid the intermediate state with

// TODO: implement deleteNotificationChannel throw UnimplementedError();

by adding the implementation here in the same commit that adds the binding

gnprice · 2024-10-07T23:55:10Z

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

+            .map { NotificationChannel(
+                it.id,
+                it.importance.toLong(),
+                it.name?.toString(),
+                it.shouldShowLights()


nit: use named parameters for these — otherwise it's impossible to tell locally if some of the parameters have been accidentally swapped

gnprice · 2024-10-07T23:59:22Z

test/model/binding.dart

+  Future<List<NotificationChannel?>> getNotificationChannels() async {
+    _channelMethodCallLogs.add('getNotificationChannels');
+    return _createdChannels.toList(growable: false);


This simulation leaves out one important aspect of how getNotificationChannels can behave: it can return channels that weren't created by this run of the app, but by some previous run (possibly years in the past).

If the tests you're currently writing don't need that feature, then it's fine to leave it out. But let's leave a comment here mentioning that gap. (Partly that serves as an invitation to a future developer to add that feature, if they find themself needing it.)

I see, what the tests effectively do is call createNotificationChannel in order to simulate a channel having been created in the distant past.

That works fine. But this approach changes the semantics of _createdChannels — particularly because deleteNotificationChannel now removes channels from that list. Instead of a log of what was created, it's really serving now as a list of the channels that currently exist. The name, and the "take"-based API which is suited for a log, are no longer a fit.

So let's have a prep commit that replaces takeCreatedChannels with a getter like:

Iterable<NotificationChannel> get createdChannels => _createdChannels;

Then that's well adapted for the direction it gets extended in the rest of this PR.

gnprice · 2024-10-07T23:59:51Z

test/model/binding.dart

+  /// [deleteNotificationChannel] and [createNotificationChannel].
+  ///
+  /// Returns a list of function names in the order they were invoked.
+  List<String> takeChannelMethodCallLogs() {


nit: "log" singular, since this is really just one log

gnprice · 2024-10-08T00:01:09Z

lib/notifications/display.dart

+  // TODO(launch) check this doesn't match zulip-mobile's current or previous
+  // channel IDs


nit: in a TODO comment (or other labeled comment), indent any continuation lines to keep them grouped under the heading:

Suggested change

// TODO(launch) check this doesn't match zulip-mobile's current or previous

// channel IDs

// TODO(launch) check this doesn't match zulip-mobile's current or previous

// channel IDs

gnprice · 2024-10-08T00:10:53Z

test/notifications/display_test.dart

+    test('channel is not recreated if one with same id already exists', () async {
+      await NotificationChannelManager.ensureChannel();


It looks like these tests affect the state in the binding, and then leak that state. To fix that, they should start with addTearDown(testBinding.reset) (like the existing smoke test does via init).

gnprice · 2024-10-08T00:19:49Z

test/notifications/display_test.dart

+      await NotificationChannelManager.ensureChannel();
+      check(testBinding.androidNotificationHost.takeChannelMethodCallLogs())
+        .deepEquals(['getNotificationChannels', 'createNotificationChannel']);
+
+      await NotificationChannelManager.ensureChannel();


It looks like the first ensureChannel here is to simulate a channel having gotten created in the past, and then the second is the call that we're testing the behavior of.

I think it'd be cleaner to do the setup more directly, without ensureChannel. Using ensureChannel will only produce a channel that's exactly the way the current code sets it up; this breaks down in the next test case, where we want to simulate some previous version of the app having set it up, and it also breaks down with the possibility that the user may have altered the settings on the channel. (If they have, we should leave the existing channel in place; only the ID counts for saying it's the same channel.)

gnprice · 2024-10-08T00:30:01Z

test/notifications/display_test.dart

+      check(testBinding.androidNotificationHost.takeChannelMethodCallLogs())
+        .deepEquals(['getNotificationChannels']);


I'm generally not a fan of having "get" calls in a log that a test checks like this — it feels too far into testing the implementation details (cf https://zulip.readthedocs.io/en/stable/testing/philosophy.html#integration-testing-or-unit-testing ).

(We do routinely include the equivalent in our API tests — we check what HTTP requests the app made to the simulated server. But because these queries are local to the device, I'd like to try to treat them more as internal.)

Can we leave these out of the log, and have the tests just look at what the code does with the information? I.e., the create and delete calls.

Needed for zulip#340, when updating notification channels to use a custom notification sound.

rajveermalviya · 2024-10-08T13:01:06Z

Thanks for the reviews @chrisbobbe and @gnprice!
@gnprice pushed a new revision with update test bindings. PTAL.

gnprice · 2024-10-08T22:13:35Z

Thanks for the revision! All looks good — merging.

This was referenced Oct 4, 2024

notif: Use Zulip's custom notification sound on Android #982

Merged

notif: Use Zulip's notification sound on Android #717

Closed

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Oct 4, 2024

chrisbobbe reviewed Oct 5, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-channel-migrations branch from 8afb983 to c85adc9 Compare October 5, 2024 03:28

rajveermalviya requested a review from chrisbobbe October 5, 2024 03:29

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Oct 7, 2024

chrisbobbe requested a review from gnprice October 7, 2024 21:31

chrisbobbe assigned gnprice Oct 7, 2024

gnprice reviewed Oct 8, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-channel-migrations branch from c85adc9 to b3a5b2a Compare October 8, 2024 09:50

rajveermalviya added 3 commits October 8, 2024 17:31

pigeon: Add binding for getNotificationChannels

d6626a8

pigeon: Add binding for deleteNotificationChannel

5bdd0aa

notif: Support migration of Android notification channels

00deaa6

Needed for zulip#340, when updating notification channels to use a custom notification sound.

rajveermalviya force-pushed the pr-notif-channel-migrations branch from b3a5b2a to 00deaa6 Compare October 8, 2024 12:33

rajveermalviya requested a review from gnprice October 8, 2024 13:01

gnprice merged commit 00deaa6 into zulip:main Oct 8, 2024
1 check passed

rajveermalviya deleted the pr-notif-channel-migrations branch October 9, 2024 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: Support migration of Android notification channels #981

notif: Support migration of Android notification channels #981

rajveermalviya commented Oct 4, 2024

chrisbobbe left a comment •

edited

Loading

chrisbobbe Oct 5, 2024

chrisbobbe Oct 5, 2024

chrisbobbe Oct 5, 2024

rajveermalviya commented Oct 5, 2024

chrisbobbe commented Oct 7, 2024

gnprice left a comment

gnprice Oct 7, 2024

gnprice Oct 7, 2024

gnprice Oct 7, 2024

gnprice Oct 8, 2024

gnprice Oct 7, 2024

gnprice Oct 8, 2024

gnprice Oct 8, 2024

gnprice Oct 8, 2024

gnprice Oct 8, 2024

rajveermalviya commented Oct 8, 2024

gnprice commented Oct 8, 2024

		/// Consumes the log of sequence of calls made to [getNotificationChannels],
		/// [deleteNotificationChannel] and [createNotificationChannel].

		// TODO(?) at launch make sure this channel-id doesn't collide with the one
		// present in zulip-mobile.

		// TODO(launch) check this doesn't match zulip-mobile's current or previous
		// channel IDs

		test('channel is not recreated if one with same id already exists', () async {
		await NotificationChannelManager.ensureChannel();

		check(testBinding.androidNotificationHost.takeChannelMethodCallLogs())
		.deepEquals(['getNotificationChannels']);

notif: Support migration of Android notification channels #981

notif: Support migration of Android notification channels #981

Conversation

rajveermalviya commented Oct 4, 2024

chrisbobbe left a comment • edited Loading

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 Oct 5, 2024

chrisbobbe commented Oct 7, 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

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 Oct 8, 2024

gnprice commented Oct 8, 2024

chrisbobbe left a comment •

edited

Loading