Page MenuHomePhabricator

[native] Don't skip `updateBadgeCount` for `deviceTokens` handled by `rescindPushNotifs`
ClosedPublic

Authored by atul on Nov 10 2022, 1:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 6:33 AM
Unknown Object (File)
Sun, Nov 24, 6:33 AM
Unknown Object (File)
Nov 21 2024, 5:17 AM
Unknown Object (File)
Nov 21 2024, 5:17 AM
Unknown Object (File)
Nov 3 2024, 11:49 PM
Unknown Object (File)
Nov 3 2024, 11:49 PM
Unknown Object (File)
Nov 3 2024, 11:49 PM
Unknown Object (File)
Nov 3 2024, 11:38 PM
Subscribers

Details

Summary

Context: https://linear.app/comm/issue/ENG-2078/notification-rescinds-dont-appear-to-be-working-as-expected

In rescindAndUpdateBadgeCounts we were previously skipping updateBadgeCount for deviceTokens "handled" by rescindPushNotifs. I haven't dug too deeply into the git history yet, but my hunch is that this was because rescindPushNotifs previously handled updating the badge count by including the badge entry in the APS dictionary.

However, the badge entry in the APS dictionary of the rescind (background) notif isn't updating the badge count (at least anymore). We have a separate alert-type notification that handles updating the badge count.

This alert notification is already being sent for the "mark as unread" and "mark as read" actions, but isn't being sent when a thread is marked as read after being focused (because that results in a rescind as well that excludes the deviceToken from being handled by updateBadgeCount).

Because rescinding and updating badge count must always happen separately, there's no reason for a rescind to affect whether a badge count update notif is sent. This diff removes the deviceToken exclusion behavior from rescindAndUpdateBadgeCounts and updates updateBadgeCount to remove the (now irrelevant) excludeDeviceTokens argument.


Will handle updating rescindPushNotifs in next diff.

Test Plan

Video of things behaving as expected (you can see old behavior on Linear issue):

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul edited the test plan for this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul published this revision for review.Nov 10 2022, 1:22 PM
atul edited the summary of this revision. (Show Details)

Preempting review since I discussed and reviewed this with @atul in real life

This revision is now accepted and ready to land.Nov 10 2022, 1:44 PM