notif: Ignore notifications for logged out accounts

Fixes: #1264

Notifications are ignored for accounts which were logged out but the
request to stop receiving notifications failed due to some reason.

Also when an account is logged out, any existing notifications for that
account is removed from the UI.

Author: tomlin7

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

@@ -234,6 +234,18 @@ class NotificationDisplayManager {
     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);
 
@@ -421,6 +433,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.
   ///

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));
@@ -71,7 +95,7 @@ void main() {
   }
 
   group('logOutAccount', () {
-    test('smoke', () => awaitFakeAsync((async) async {
+    test('smoke', () => runWithHttpClient(() => awaitFakeAsync((async) async {
       await prepare();
       check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id);
       const unregisterDelay = Duration(seconds: 5);
@@ -94,9 +118,9 @@ void main() {
 
       async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
       check(newConnection.isOpen).isFalse();
-    }));
+    })));
 
-    test('unregister request has an error', () => awaitFakeAsync((async) async {
+    test('unregister request has an error', () => runWithHttpClient(() => awaitFakeAsync((async) async {
       await prepare();
       check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id);
       const unregisterDelay = Duration(seconds: 5);
@@ -120,7 +144,46 @@ void main() {
 
       async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
       check(newConnection.isOpen).isFalse();
-    }));
+    })));
+
+    test('notifications are removed after logout', () => runWithHttpClient(() => awaitFakeAsync((async) async {
+      await prepare();
+      testBinding.firebaseMessagingInitialToken = '123';
+      addTearDown(NotificationService.debugReset);
+      NotificationService.debugBackgroundIsolateIsLive = false;
+      await NotificationService.instance.start();
+
+      check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id);
+      const unregisterDelay = Duration(seconds: 5);
+      assert(unregisterDelay > TestGlobalStore.removeAccountDuration);
+      final newConnection = separateConnection()
+        ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'});
+
+      // 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();
+
+      final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
+      // Unregister-token request and account removal dispatched together
+      checkSingleUnregisterRequest(newConnection);
+      check(testBinding.globalStore.takeDoRemoveAccountCalls())
+        .single.equals(eg.selfAccount.id);
+
+      async.elapse(TestGlobalStore.removeAccountDuration);
+      await future;
+      // Account removal not blocked on unregister-token response
+      check(testBinding.globalStore).accountIds.isEmpty();
+      check(connection.isOpen).isFalse();
+      check(newConnection.isOpen).isTrue(); // for the unregister-token request
+
+      async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
+      check(newConnection.isOpen).isFalse();
+      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,68 @@ 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);
+      check(testBinding.androidNotificationHost.activeNotifications.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);
+      check(testBinding.androidNotificationHost.activeNotifications.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 +1046,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);