From 207ecf355c87bcc3988912807da0720dcd7cd233 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulip.com>
Date: Mon, 23 Oct 2023 21:52:01 -0700
Subject: [PATCH] wip rearrange ensure channel: just once at startup

---
 lib/notifications.dart       | 34 +++++++++++++++++++++++++++-------
 test/notifications_test.dart |  8 --------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/lib/notifications.dart b/lib/notifications.dart
index 67f5dcb1d1..c188a3517e 100644
--- a/lib/notifications.dart
+++ b/lib/notifications.dart
@@ -47,11 +47,10 @@ class NotificationService {
     // TODO(#324) defer notif setup if user not logged into any accounts
     //   (in order to avoid calling for permissions)
 
+    await NotificationDisplayManager._init();
     ZulipBinding.instance.firebaseMessagingOnMessage.listen(_onForegroundMessage);
     ZulipBinding.instance.firebaseMessagingOnBackgroundMessage(_onBackgroundMessage);
 
-    NotificationDisplayManager._init();
-
     // Get the FCM registration token, now and upon changes.  See FCM API docs:
     //   https://firebase.google.com/docs/cloud-messaging/android/client#sample-register
     ZulipBinding.instance.firebaseMessaging.onTokenRefresh.listen(_onTokenRefresh);
@@ -94,7 +93,7 @@ class NotificationService {
       return true;
     }());
     LiveZulipBinding.ensureInitialized();
-    NotificationDisplayManager._init();
+    NotificationDisplayManager._init(); // TODO call this just once per isolate
 
     assert(debugLog("notif message in background: ${message.data}"));
     _onRemoteMessage(message);
@@ -122,7 +121,28 @@ class NotificationChannelManager {
   @visibleForTesting
   static final kVibrationPattern = Int64List.fromList([0, 125, 100, 450]);
 
-  static void _ensureChannel() async { // TODO "ensure"
+  /// Create our notification channel, if it doesn't already exist.
+  //
+  // NOTE when changing anything here: the changes will not take effect
+  // for existing installs of the app!  That's because we'll have already
+  // created the channel with the old settings, and they're in the user's
+  // hands from there.  Our choices are:
+  //
+  //  * Leave the old settings in place for existing installs, so the
+  //    changes only apply to new installs.
+  //
+  //  * Change `kChannelId`, so that we abandon the old channel and use
+  //    a new one.  Existing installs will get the new settings.
+  //
+  //    This also means that if the user has changed any of the notification
+  //    settings for the channel -- like "override Do Not Disturb", or "use
+  //    a different sound", or "don't pop on screen" -- their changes get
+  //    reset.  So this has to be done sparingly.
+  //
+  //    If we do this, we should also look for any channel with the old
+  //    channel ID and delete it.  See zulip-mobile's `createNotificationChannel`
+  //    in android/app/src/main/java/com/zulipmobile/notifications/NotificationChannelManager.kt .
+  static Future<void> _ensureChannel() async {
     final plugin = ZulipBinding.instance.notifications;
     await plugin.resolvePlatformSpecificImplementation<AndroidFlutterLocalNotificationsPlugin>()
       ?.createNotificationChannel(AndroidNotificationChannel(
@@ -141,17 +161,17 @@ class NotificationDisplayManager {
   // We rely on the tag instead.
   static const _kNotificationId = 0;
 
-  static void _init() {
-    ZulipBinding.instance.notifications.initialize(
+  static Future<void> _init() async {
+    await ZulipBinding.instance.notifications.initialize(
       const InitializationSettings(
         android: AndroidInitializationSettings('zulip_notification'),
       ),
       onDidReceiveNotificationResponse: _onNotificationOpened,
     );
+    await NotificationChannelManager._ensureChannel();
   }
 
   static void _onMessageFcmMessage(MessageFcmMessage data, Map<String, dynamic> dataJson) {
-    NotificationChannelManager._ensureChannel();
     assert(debugLog('notif message content: ${data.content}'));
     final title = switch (data.recipient) {
       FcmMessageStreamRecipient(:var streamName?, :var topic) =>
diff --git a/test/notifications_test.dart b/test/notifications_test.dart
index 142d9b069c..56c428a633 100644
--- a/test/notifications_test.dart
+++ b/test/notifications_test.dart
@@ -3,7 +3,6 @@ import 'dart:typed_data';
 import 'dart:ui';
 
 import 'package:checks/checks.dart';
-import 'package:firebase_messaging/firebase_messaging.dart';
 import 'package:flutter_local_notifications/flutter_local_notifications.dart';
 import 'package:test/scaffolding.dart';
 import 'package:zulip/api/notifications.dart';
@@ -45,13 +44,6 @@ void main() {
       addTearDown(NotificationService.debugReset);
       await NotificationService.instance.start();
 
-      // On just starting the app, we don't eagerly try to create a channel.
-      check(notifAndroid.takeCreatedChannels()).isEmpty();
-
-      // When a message arrives that causes a notification, we create our channel.
-      testBinding.firebaseMessaging.onMessage.add(
-        RemoteMessage(data: egMessage.toJson()));
-      await null;
       check(notifAndroid.takeCreatedChannels()).single
         ..id.equals(NotificationChannelManager.kChannelId)
         ..name.equals('Messages')