Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Prevent duplicate push notifications for room reads #11835

Merged
merged 9 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11835.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make a `POST` to `/rooms/<room_id>/receipt/m.read/<event_id>` only trigger a push notification if the count of unread messages is different to the one in the last successfully sent push.
7 changes: 6 additions & 1 deletion synapse/push/httppusher.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ def __init__(self, hs: "HomeServer", pusher_config: PusherConfig):
self.data_minus_url = {}
self.data_minus_url.update(self.data)
del self.data_minus_url["url"]
self.badge_count_last_call: Optional[int] = None

def on_started(self, should_check_for_notifs: bool) -> None:
"""Called when this pusher has been started.
Expand Down Expand Up @@ -136,7 +137,9 @@ async def _update_badge(self) -> None:
self.user_id,
group_by_room=self._group_unread_count_by_room,
)
await self._send_badge(badge)
if self.badge_count_last_call is None or self.badge_count_last_call != badge:
self.badge_count_last_call = badge
await self._send_badge(badge)

def on_timer(self) -> None:
self._start_processing()
Expand Down Expand Up @@ -402,6 +405,8 @@ async def dispatch_push(
rejected = []
if "rejected" in resp:
rejected = resp["rejected"]
else:
self.badge_count_last_call = badge
return rejected

async def _send_badge(self, badge: int) -> None:
Expand Down
28 changes: 9 additions & 19 deletions tests/push/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ def test_push_unread_count_group_by_room(self):
#
# This push should still only contain an unread count of 1 (for 1 unread room)
self.assertEqual(
self.push_attempts[5][2]["notification"]["counts"]["unread"], 1
self.push_attempts[4][2]["notification"]["counts"]["unread"], 1
)

@override_config({"push": {"group_unread_count_by_room": False}})
Expand All @@ -588,7 +588,7 @@ def test_push_unread_count_message_count(self):
# We're counting every unread message, so there should now be 4 since the
# last read receipt
self.assertEqual(
self.push_attempts[5][2]["notification"]["counts"]["unread"], 4
self.push_attempts[4][2]["notification"]["counts"]["unread"], 4
)

def _test_push_unread_count(self):
Expand Down Expand Up @@ -673,30 +673,20 @@ def _test_push_unread_count(self):
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a comment just above 'This will actually trigger a new notification to be sent out so that even if the user does not receive another message, their unread count goes down'...

It doesn't make much sense to me though because it was already at zero right before.

I would say 'I think it may be worth removing the comment', but this test is trying to test that pushes go out when you read something. As it stands, we're not testing that anymore.
Do you think you can adapt the test so that it will actually get the chance to send a push after a read receipt?

The test currently looks like:

  • send message → send count of 0 (because we haven't got a read receipt in the room yet)
  • add read receipt → send count of 0 (because we sent a read receipt) — your changes stop this from happening, so we don't actually end up testing that read receipts do anything
  • send message → send count of 1

I wonder if it should look like this:

  • send message → send count of 0 (because we haven't got a read receipt in the room yet)
  • add read receipt → make sure nothing new has been sent (that's still a valuable piece of test information) — do it with an assert on len(push_attempts)
  • send message → send count of 1
  • add read receipt (to the second message) → make sure we send a count of 0
  • send message → send count of 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I will start working on that issue by the beginning of next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reivilibre: I still refactored the test a bit. If you do not like it, the commit before preserves
the old coding style.

self.assertEqual(channel.code, 200, channel.json_body)

# Advance time and make the push succeed
self.push_attempts[1][0].callback({})
self.pump()

# Unread count is still zero as we've read the only message in the room
self.assertEqual(len(self.push_attempts), 2)
self.assertEqual(
self.push_attempts[1][2]["notification"]["counts"]["unread"], 0
)

# Send another message
self.helper.send(
room_id, body="How's the weather today?", tok=other_access_token
)

# Advance time and make the push succeed
self.push_attempts[2][0].callback({})
self.push_attempts[1][0].callback({})
self.pump()

# This push should contain an unread count of 1 as there's now been one
# message since our last read receipt
self.assertEqual(len(self.push_attempts), 3)
self.assertEqual(len(self.push_attempts), 2)
self.assertEqual(
self.push_attempts[2][2]["notification"]["counts"]["unread"], 1
self.push_attempts[1][2]["notification"]["counts"]["unread"], 1
)

# Since we're grouping by room, sending more messages shouldn't increase the
Expand All @@ -705,18 +695,18 @@ def _test_push_unread_count(self):

# Advance time and make the push succeed
self.pump()
self.push_attempts[3][0].callback({})
self.push_attempts[2][0].callback({})

self.helper.send(room_id, body="Hello??", tok=other_access_token)

# Advance time and make the push succeed
self.pump()
self.push_attempts[4][0].callback({})
self.push_attempts[3][0].callback({})

self.helper.send(room_id, body="HELLO???", tok=other_access_token)

# Advance time and make the push succeed
self.pump()
self.push_attempts[5][0].callback({})
self.push_attempts[4][0].callback({})

self.assertEqual(len(self.push_attempts), 6)
self.assertEqual(len(self.push_attempts), 5)