notif: Ignore notifications for logged out accounts

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<void> 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);
 }
 

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';
@@ -217,10 +217,12 @@ class NotificationChannelManager {
 /// Service for managing the notifications shown to the user.
 class NotificationDisplayManager {
   static Future<void> init() async {
+    assert(defaultTargetPlatform == TargetPlatform.android);
     await NotificationChannelManager.ensureChannel();
   }
 
   static void onFcmMessage(FcmMessage data, Map<String, dynamic> dataJson) {
+    assert(defaultTargetPlatform == TargetPlatform.android);
     switch (data) {
       case MessageFcmMessage(): _onMessageFcmMessage(data, dataJson);
       case RemoveFcmMessage(): _onRemoveFcmMessage(data);
@@ -231,9 +233,21 @@ class NotificationDisplayManager {
   static Future<void> _onMessageFcmMessage(MessageFcmMessage data, Map<String, dynamic> 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 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);
 
@@ -365,7 +379,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
@@ -421,6 +435,20 @@ class NotificationDisplayManager {
     }
   }
 
+  static Future<void> removeNotificationsForAccount(Uri realmUrl, int userId) async {
+    assert(defaultTargetPlatform == TargetPlatform.android);
+
+    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.
   ///
@@ -445,10 +473,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";
@@ -464,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'));
@@ -492,6 +522,7 @@ class NotificationDisplayManager {
   /// generated with [NotificationOpenPayload.buildUrl] while creating
   /// the notification.
   static Future<void> navigateForNotification(Uri url) async {
+    assert(defaultTargetPlatform == TargetPlatform.android);
     assert(debugLog('opened notif: url: $url'));
 
     NavigatorState navigator = await ZulipApp.navigator;

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>(
+    T Function() callback, {
+    http.Client Function()? httpClientFactory,
+  }) {
+    return http.runWithClient(callback, httpClientFactory ?? () => fakeHttpClientGivingSuccess);
+  }
+
   Future<void> 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', () {

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<void> init() async {
+  Future<void> 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<void> 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);