notif: Ignore notifications for logged out accounts #1349

tomlin7 · 2025-02-12T19:15:29Z

Fixes: #1264

Changes

Ignores notifications received for accounts that were logged out while the app was offline
Removes notifications (if there exist) of the user when the user logs out

gnprice · 2025-02-12T19:25:19Z

Thanks @tomlin7 for the contribution! Before we can review this, it will need a test — see the repo's README file.

chrisbobbe

Thanks! Comments below from a brief review.

lib/model/actions.dart

lib/notifications/display.dart

chrisbobbe · 2025-02-25T00:06:26Z

I believe the two questions above that were open now have answers:

So you should be able to proceed, incorporating those.

tomlin7 · 2025-02-25T20:25:51Z

@chrisbobbe I have made the requested changes, please review again!

chrisbobbe · 2025-03-04T03:36:26Z

Quoting my comment #1349 (comment):

Following that discussion, yeah, we will need to put the new call inside an Android platform conditional.

You haven't done this in this revision; please do that. In logOutAccount, we should only call NotificationDisplayManager .removeNotificationsForAccount if the platform is Android.

Then as an optional, independent improvement, you could add a commit that does this:

Could be helpful to put asserts at the top of [NotificationDisplayManager's] public methods, checking the target platform is indeed Android — because that's basically an assumption the implementation is making

I see some added asserts, but this needs to be done in a separate commit. The current revision only has one commit.

tomlin7 · 2025-03-05T14:54:46Z

@chrisbobbe I have implemented all the requested changes. Let me know if there are any further improvements needed.

chrisbobbe

Thanks! Comments below.

Also, have you tested this with an Android device or emulator?

lib/model/actions.dart

lib/notifications/display.dart

test/model/actions_test.dart

test/notifications/display_test.dart

lib/notifications/display.dart

lib/model/actions.dart

tomlin7 · 2025-03-16T00:20:52Z

@chrisbobbe I have tested this with an Android device (both debug and release).

chrisbobbe

Thanks! A few comments from a brief review—also, some tests are failing; please fix those.

lib/notifications/display.dart

test/model/actions_test.dart

test/notifications/display_test.dart

lib/notifications/display.dart

The `_groupKey` method currently takes an `FcmMessageWithIdentity` object as an argument. However, this method only requires the realm URL and user ID from that object. This change modifies the method signature to directly accept `realmUrl` and `userId` as arguments. This change is made to facilitate calling `_groupKey` from contexts where an `FcmMessageWithIdentity` object is not available. Discussion: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Removing.20notifications.20for.20logged.20out.20account/near/2088813

gnprice

Thanks @tomlin7 for building this, and @chrisbobbe for the previous reviews!

The implementation looks correct to me; just small comments below. There's a few more things I'd like to see in the tests.

lib/notifications/display.dart

test/notifications/display_test.dart

test/model/actions_test.dart

gnprice · 2025-03-25T05:43:48Z

@rajveermalviya, would you take a look at this and

check for any interactions with your upcoming notif ios: Handle opening of conversation on tap; take 2 #1379 which might mean something should be done differently here (as well as any other points that jump out at you);
do a round of manual testing to confirm this behaves correctly? (I think the implementation is already very close to what we'll merge; the more substantive comments above are on the tests.)

rajveermalviya

I tested the following cases and verified they work correctly:

The notifications being removed when logging-out of the account.
Receiving a notification from FCM but not displaying it when the account wasn't found in GlobalStore (simulating that the API call to unregisterToken failed, by commenting it out).

Also since this PR removes the notification by matching groupKey, it should also include the group summary notification and remove it, avoiding a lingering summary notification.

As for the interaction with #1379, apart from little conflicts, there shouldn't be anything that requires changing here, as this PR only checks for the notification group key (to find and remove notifications of the specific account) and there's no change in regarding to that in #1379. And for iOS this shouldn't matter for now (but remains unresolved), as we don't have a way to control displaying notifications there yet (#1265), and also because #1264 is only about Android.

gnprice · 2025-03-25T16:56:49Z

Thanks @rajveermalviya! Sounds good.

gnprice

Thanks for the revision! These tests look complete now. Comments below.

lib/notifications/display.dart

test/model/actions_test.dart

test/notifications/display_test.dart

gnprice

Thanks for the revision! This is very close now. Comments below.

Some of these comments are additional cases of things that I mentioned in the previous review. When you get code-review feedback on a PR, one key thing you can do to help keep the PR moving swiftly along to merge is to look for other places where the same comments would apply, and act on the feedback in those places too.

test/model/actions_test.dart

test/notifications/display_test.dart

Fixes: zulip#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.

…ethods NotificationDisplayManager's public methods only get invoked on Android, and their implementations basically assume the platform is Android. So, add these asserts, to make that assumption clearer and to fail early if we accidentally call one of these on iOS. When we eventually take more hands-on control of notifications on iOS, we might either adapt this class to support iOS too, or make a new class for iOS and make this one's name and interface clearer that it's Android-only. Discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/NotificationDisplayManager.20Android-only/near/2099196

tomlin7 · 2025-04-12T05:22:28Z

Thanks for the suggestions! It's improving my code quality significiantly. I made sure to run the suggestions through all changes made.

gnprice

Thanks for the revision! Just a few nits remain.

gnprice · 2025-04-15T00:40:55Z

test/notifications/display_test.dart

+      final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
+
+      receiveFcmMessage(async, messageFcmMessage(message));
+      check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty();


nit: these are closely tied, so group together in one stanza:

Suggested change

final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);

receiveFcmMessage(async, messageFcmMessage(message));

check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty();

final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);

receiveFcmMessage(async, messageFcmMessage(message));

check(testBinding.androidNotificationHost.activeNotifications).isNotEmpty();

(e.g., that message local exists only to be used on that next line, so those two lines belong in the same stanza)

gnprice · 2025-04-15T00:42:14Z

test/notifications/display_test.dart

+        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'));


nit: the key info here (the parts of the realm URLs that differ) is still past 80 columns

gnprice · 2025-04-15T00:44:05Z

test/notifications/display_test.dart

+      await NotificationDisplayManager.removeNotificationsForAccount(realmUrl, account1.userId);
+      check(testBinding.androidNotificationHost.activeNotifications)
+        ..length.equals(2)
+        ..first.notification.group.equals('$realmUrl|${account2.userId}');


nit:

Suggested change

await NotificationDisplayManager.removeNotificationsForAccount(realmUrl, account1.userId);

check(testBinding.androidNotificationHost.activeNotifications)

..length.equals(2)

..first.notification.group.equals('$realmUrl|${account2.userId}');

await NotificationDisplayManager.removeNotificationsForAccount(

realmUrl, account1.userId);

check(testBinding.androidNotificationHost.activeNotifications)

..length.equals(2)

..first.notification.group.equals('$realmUrl|${account2.userId}');

otherwise "account1.userId" is hidden off past 80 columns, and that's the key information for interpreting the check at the end of the test

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Feb 14, 2025

tomlin7 force-pushed the ignore-notification-removed-account branch 4 times, most recently from 6c31bb2 to 51c72de Compare February 15, 2025 20:27

chrisbobbe reviewed Feb 19, 2025

View reviewed changes

lib/model/actions.dart Outdated Show resolved Hide resolved

lib/notifications/display.dart Outdated Show resolved Hide resolved

lib/notifications/display.dart Outdated Show resolved Hide resolved

tomlin7 force-pushed the ignore-notification-removed-account branch 3 times, most recently from 90daa5f to bbd7f14 Compare February 21, 2025 20:01

chrisbobbe self-assigned this Feb 21, 2025

tomlin7 force-pushed the ignore-notification-removed-account branch 2 times, most recently from 610d6d7 to 004951d Compare February 25, 2025 19:57

tomlin7 force-pushed the ignore-notification-removed-account branch 2 times, most recently from aba2741 to 1405cc1 Compare March 5, 2025 14:28

chrisbobbe reviewed Mar 12, 2025

View reviewed changes

tomlin7 force-pushed the ignore-notification-removed-account branch 6 times, most recently from b49b478 to b88bb4d Compare March 16, 2025 00:17

chrisbobbe reviewed Mar 17, 2025

View reviewed changes

tomlin7 force-pushed the ignore-notification-removed-account branch from b88bb4d to 27bcdc2 Compare March 22, 2025 09:56

chrisbobbe requested a review from gnprice March 24, 2025 21:49

chrisbobbe assigned gnprice and unassigned chrisbobbe Mar 24, 2025

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 Mar 24, 2025

gnprice reviewed Mar 25, 2025

View reviewed changes

gnprice assigned rajveermalviya Mar 25, 2025

gnprice added the maintainer review PR ready for review by Zulip maintainers label Mar 25, 2025

rajveermalviya reviewed Mar 25, 2025

View reviewed changes

gnprice unassigned rajveermalviya Mar 25, 2025

gnprice removed the maintainer review PR ready for review by Zulip maintainers label Mar 25, 2025

tomlin7 force-pushed the ignore-notification-removed-account branch 5 times, most recently from e2cf339 to 4dafcb9 Compare March 31, 2025 12:23

tomlin7 requested a review from gnprice March 31, 2025 12:26

gnprice reviewed Mar 31, 2025

View reviewed changes

tomlin7 force-pushed the ignore-notification-removed-account branch 2 times, most recently from 1d43bd8 to ad49b0b Compare April 9, 2025 19:43

tomlin7 requested a review from gnprice April 9, 2025 19:44

gnprice reviewed Apr 12, 2025

View reviewed changes

tomlin7 added 2 commits April 12, 2025 10:49

tomlin7 force-pushed the ignore-notification-removed-account branch from ad49b0b to 5f52b54 Compare April 12, 2025 05:20

tomlin7 requested a review from gnprice April 12, 2025 05:22

gnprice reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: Ignore notifications for logged out accounts #1349

notif: Ignore notifications for logged out accounts #1349

tomlin7 commented Feb 12, 2025 •

edited

Loading

gnprice commented Feb 12, 2025

chrisbobbe left a comment

chrisbobbe commented Feb 25, 2025

tomlin7 commented Feb 25, 2025

chrisbobbe commented Mar 4, 2025

tomlin7 commented Mar 5, 2025

chrisbobbe left a comment

tomlin7 commented Mar 16, 2025 •

edited

Loading

chrisbobbe left a comment

gnprice left a comment

gnprice commented Mar 25, 2025

rajveermalviya left a comment

gnprice commented Mar 25, 2025

gnprice left a comment

gnprice left a comment

tomlin7 commented Apr 12, 2025

gnprice left a comment

gnprice Apr 15, 2025

gnprice Apr 15, 2025

gnprice Apr 15, 2025

notif: Ignore notifications for logged out accounts #1349

Are you sure you want to change the base?

notif: Ignore notifications for logged out accounts #1349

Conversation

tomlin7 commented Feb 12, 2025 • edited Loading

Changes

gnprice commented Feb 12, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe commented Feb 25, 2025

tomlin7 commented Feb 25, 2025

chrisbobbe commented Mar 4, 2025

tomlin7 commented Mar 5, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

tomlin7 commented Mar 16, 2025 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice commented Mar 25, 2025

rajveermalviya left a comment

Choose a reason for hiding this comment

gnprice commented Mar 25, 2025

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

tomlin7 commented Apr 12, 2025

gnprice left a comment

Choose a reason for hiding this comment

gnprice Apr 15, 2025

Choose a reason for hiding this comment

gnprice Apr 15, 2025

Choose a reason for hiding this comment

gnprice Apr 15, 2025

Choose a reason for hiding this comment

tomlin7 commented Feb 12, 2025 •

edited

Loading

tomlin7 commented Mar 16, 2025 •

edited

Loading