-
Notifications
You must be signed in to change notification settings - Fork 583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send mark-read-generic
notification on updateSeen
#2567
Conversation
add `reason` add `recipientDid` push `mark-read-generic` notification on `updateSeen` add `client_controlled`
822dba2
to
cb3fc14
Compare
await ctx.courierClient.pushNotifications({ | ||
notifications: [ | ||
{ | ||
id: 'mark-read-generic', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Courier wont send a notification with the same id twice, so we'll need to make this something unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, in that case we could maybe just do this
id: 'mark-read-generic', | |
id: `${viewer}-${seenAt}`, |
Think I have an idea here of how we can handle this nicely. Requires something in courier so I'll PR there as well. |
await ctx.courierClient.pushNotifications({ | ||
notifications: [ | ||
{ | ||
id: `${viewer}-${seenAt}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review here! Would prefer to avoid accepting user input as the id and make it something more deterministic. All the other notif ids are murmur hashes with parts separated by double-colons, may make sense to recreate that here.
import murmur from 'murmurhash'
const getNotifId = (viewer: string, seenAt: Date) => {
const key = ['mark-read-generic', viewer, seenAt.getTime().toString()].join(
'::',
)
return murmur.v3(key).toString(16)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, lovely. thanks for pointing in the right direction!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the tests just need some love—my guess is that the courier call is failing in the tests since there's no service running there. We could either make the courier config and client optional, or mock out courier in the tests. The former sounds a bit more straightforward to me.
Why
We've added support for incrementing the badge properly on iOS whenever push notifications are received for regular notifications (i.e. like, repost, etc.) However, there is no logic for decrementing the badge whenever notifications are read. We need to send an additional notification to those clients whenever they mark their notifications as read.
How
@devinivy added support for "client controlled" notifications in Courier for the chat service (though it looks like we needed to add that value here which I think I did correctly). We can therefore send one of those "client controlled" notifications to the recipient to tell the client to decrement the badge.
Since we currently mark all notifications as "read" based on the "last seen" value, we do not need to send a specific number of notifications to mark as read (though it looks like we maybe could do that by getting the unread notification count from the dataplane?) Therefore, I think the best move for now would be to just have the client reset the "generic" notification count to zero (separate from the DMs notification count) whenever a notification with reason
mark-read-generic
is received by the client.Test Plan
We'll actually need to deploy this change to test it unfortunately, since the notifications need to come from Apple's server to test the Notification Service Extension on iOS. The good news is that since in this case we do not need to supply a
title
ormessage
, these notifications will not appear for users anyway. This means that we can deploy this change whenever we are comfortable doing so and test the updated client internally.