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,13 @@ 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);
+    if (account == null) {
+      return;
+    }
+
     final oldMessagingStyle = await _androidHost
       .getActiveNotificationMessagingStyleByTag(conversationKey);
 
@@ -518,6 +525,16 @@ class NotificationDisplayManager {
     }
     return null;
   }
+
+  static Future<void> removeNotificationsForAccount(Uri realmUri, int userId) async {
+    final groupKey = _groupKey(realmUri, userId);
+    final activeNotifications = await _androidHost.getActiveNotifications(desiredExtras: [kExtraLastZulipMessageId]);
+    for (final statusBarNotification in activeNotifications) {
+      if (statusBarNotification.notification.group == groupKey) {
+        await _androidHost.cancel(tag: statusBarNotification.tag, id: statusBarNotification.id);
+      }
+    }
+  }
 }
 
 /// The information contained in 'zulip://notification/…' internal

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,12 +27,35 @@ 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));
     await testBinding.globalStore.add(selfAccount, eg.initialSnapshot());
     store = await testBinding.globalStore.perAccount(selfAccount.id);
     connection = store.connection as FakeApiConnection;
+
+    testBinding.firebaseMessagingInitialToken = '123';
+    addTearDown(NotificationService.debugReset);
+    NotificationService.debugBackgroundIsolateIsLive = false;
+    await NotificationService.instance.start();
   }
 
   /// Creates and caches a new [FakeApiConnection] in [TestGlobalStore].
@@ -71,14 +100,23 @@ 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);
       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 that notifications were created
+      check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty();
+
       final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
       // Unregister-token request and account removal dispatched together
       checkSingleUnregisterRequest(newConnection);
@@ -94,9 +132,12 @@ void main() {
 
       async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
       check(newConnection.isOpen).isFalse();
-    }));
 
-    test('unregister request has an error', () => awaitFakeAsync((async) async {
+      // Check that notifications were removed
+      check(testBinding.androidNotificationHost.activeNotifications).isEmpty();
+    })));
+
+    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);
@@ -105,6 +146,15 @@ void main() {
       final newConnection = separateConnection()
         ..prepare(delay: unregisterDelay, apiException: exception);
 
+      // 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 that notifications were created
+      check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty();
+
       final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
       // Unregister-token request and account removal dispatched together
       checkSingleUnregisterRequest(newConnection);
@@ -120,7 +170,10 @@ void main() {
 
       async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
       check(newConnection.isOpen).isFalse();
-    }));
+
+      // Check that notifications were removed
+      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);
@@ -481,7 +484,8 @@ void main() {
       await init();
       final stream = eg.stream();
       final message = eg.streamMessage(stream: stream);
-      await checkNotifications(async, messageFcmMessage(message, streamName: stream.name),
+      await checkNotifications(async,
+        messageFcmMessage(message, streamName: stream.name),
         expectedIsGroupConversation: true,
         expectedTitle: '#${stream.name} > ${message.topic}',
         expectedTagComponent: 'stream:${message.streamId}:${message.topic}');
@@ -613,7 +617,8 @@ void main() {
       await init();
       final stream = eg.stream();
       final message = eg.streamMessage(stream: stream);
-      await checkNotifications(async, messageFcmMessage(message, streamName: null),
+      await checkNotifications(async,
+        messageFcmMessage(message, streamName: null),
         expectedIsGroupConversation: true,
         expectedTitle: '#(unknown channel) > ${message.topic}',
         expectedTagComponent: 'stream:${message.streamId}:${message.topic}');
@@ -730,6 +735,7 @@ void main() {
         await init();
         final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
         final data = messageFcmMessage(message);
+
         receiveFcmMessage(async, data);
         checkNotification(data,
           messageStyleMessages: [data],
@@ -746,6 +752,7 @@ void main() {
         await init();
         final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
         final data = messageFcmMessage(message);
+
         receiveFcmMessage(async, data);
         checkNotification(data,
           messageStyleMessages: [data],
@@ -872,7 +879,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 +889,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 +899,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 +927,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 +967,21 @@ 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]);
+
+      await checkNotifications(async, messageFcmMessage(message),
+        expectedIsGroupConversation: false,
+        expectedTitle: eg.otherUser.fullName,
+        expectedTagComponent: 'dm:${message.allRecipientIds.join(",")}');
+
+      check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty();
+      await NotificationDisplayManager.removeNotificationsForAccount(
+        eg.selfAccount.realmUrl, eg.selfAccount.userId);
+      check(testBinding.androidNotificationHost.activeNotifications).isEmpty();
+    })));
   });
 
   group('NotificationDisplayManager open', () {