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

Add a config option to change whether unread push notification counts are per-message or per-room #8820

Merged
merged 11 commits into from
Nov 30, 2020
2 changes: 2 additions & 0 deletions changelog.d/8820.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "total unread rooms" or "total unread messages".
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

10 changes: 10 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2271,6 +2271,16 @@ push:
#
#include_content: false

# When a push notification is received, a total unread message count is also sent.
# This number can either be calculated as the total number of unread messages
# for the user, or the total number of *rooms* the user has unread messages in.
#
# The default value is "true", meaning push clients will see the total number of
# rooms with unread messages in them. Uncomment to instead send the total number
# of unread messages.
#
#group_unread_count_by_room: false


# Spam checkers are third-party modules that can block specific actions
# of local users, such as creating rooms and registering undesirable
Expand Down
13 changes: 13 additions & 0 deletions synapse/config/push.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ class PushConfig(Config):
def read_config(self, config, **kwargs):
push_config = config.get("push") or {}
self.push_include_content = push_config.get("include_content", True)
self.push_group_unread_count_by_room = push_config.get(
"group_unread_count_by_room", True
)

pusher_instances = config.get("pusher_instances") or []
self.pusher_shard_config = ShardedWorkerHandlingConfig(pusher_instances)
Expand Down Expand Up @@ -68,4 +71,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# include the event ID and room ID in push notification payloads.
#
#include_content: false

# When a push notification is received, a total unread message count is also sent.
# This number can either be calculated as the total number of unread messages
# for the user, or the total number of *rooms* the user has unread messages in.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
#
# The default value is "true", meaning push clients will see the total number of
# rooms with unread messages in them. Uncomment to instead send the total number
# of unread messages.
#
#group_unread_count_by_room: false
"""
15 changes: 13 additions & 2 deletions synapse/push/httppusher.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ def __init__(self, hs, pusherdict):
self.failing_since = pusherdict["failing_since"]
self.timed_call = None
self._is_processing = False
self._push_group_unread_count_by_room = (
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
hs.config.push_group_unread_count_by_room
)

# This is the highest stream ordering we know it's safe to process.
# When new events arrive, we'll be given a window of new events: we
Expand Down Expand Up @@ -136,7 +139,11 @@ def on_new_receipts(self, min_stream_id, max_stream_id):
async def _update_badge(self):
# XXX as per https://github.com/matrix-org/matrix-doc/issues/2627, this seems
# to be largely redundant. perhaps we can remove it.
badge = await push_tools.get_badge_count(self.hs.get_datastore(), self.user_id)
badge = await push_tools.get_badge_count(
self.hs.get_datastore(),
self.user_id,
group_by_room=self._push_group_unread_count_by_room,
)
await self._send_badge(badge)

def on_timer(self):
Expand Down Expand Up @@ -283,7 +290,11 @@ async def _process_one(self, push_action):
return True

tweaks = push_rule_evaluator.tweaks_for_actions(push_action["actions"])
badge = await push_tools.get_badge_count(self.hs.get_datastore(), self.user_id)
badge = await push_tools.get_badge_count(
self.hs.get_datastore(),
self.user_id,
group_by_room=self._push_group_unread_count_by_room,
)

event = await self.store.get_event(push_action["event_id"], allow_none=True)
if event is None:
Expand Down
14 changes: 10 additions & 4 deletions synapse/push/push_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from synapse.storage import Storage


async def get_badge_count(store, user_id):
async def get_badge_count(store, user_id, group_by_room: bool):
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
invites = await store.get_invited_rooms_for_local_user(user_id)
joins = await store.get_rooms_for_user(user_id)

Expand All @@ -34,9 +34,15 @@ async def get_badge_count(store, user_id):
room_id, user_id, last_unread_event_id
)
)
# return one badge count per conversation, as count per
# message is so noisy as to be almost useless
badge += 1 if notifs["notify_count"] else 0
if notifs["notify_count"] == 0:
continue

if group_by_room:
# return one badge count per conversation
badge += 1
else:
# increment the badge count by the number of unread messages in the room
badge += notifs["notify_count"]
return badge


Expand Down
157 changes: 155 additions & 2 deletions tests/push/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,24 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from mock import Mock

from twisted.internet.defer import Deferred

import synapse.rest.admin
from synapse.logging.context import make_deferred_yieldable
from synapse.rest.client.v1 import login, room
from synapse.rest.client.v2_alpha import receipts

from tests.unittest import HomeserverTestCase
from tests.unittest import HomeserverTestCase, override_config


class HTTPPusherTests(HomeserverTestCase):
servlets = [
synapse.rest.admin.register_servlets_for_client_rest_resource,
room.register_servlets,
login.register_servlets,
receipts.register_servlets,
]
user_id = True
hijack_auth = False
Expand Down Expand Up @@ -499,3 +500,155 @@ def test_sends_high_priority_for_atroom(self):

# check that this is low-priority
self.assertEqual(self.push_attempts[1][2]["notification"]["prio"], "low")

def test_push_unread_count_group_by_room(self):
"""
The HTTP pusher will group unread count by number of unread rooms.
"""
self._test_push_unread_count(group_by_room=True)

@override_config({"push": {"group_unread_count_by_room": False}})
def test_push_unread_count_message_count(self):
"""
The HTTP pusher will send the total unread message count.
"""
self._test_push_unread_count(group_by_room=False)

def _test_push_unread_count(self, group_by_room: bool):
Copy link
Member

Choose a reason for hiding this comment

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

Having a test helper like this take a boolean is a bit odd, instead I think it would be clearer to take an expected_count argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, and definitely generally the case. I think though in this case _test_push_unread_count isn't meant to be used as a helper. It's really only intended for saving on code duplication and running the above two tests.

Perhaps I could rename it to something more clear?

Copy link
Member

Choose a reason for hiding this comment

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

isn't meant to be used as a helper. It's really only intended for saving on code duplication and running the above two tests.

Sure, but abstracting via forking the code that way is a bit odd IMO.

Another option is to do the final assertions in the callers instead of in _test_push_unread_count?

Copy link
Member Author

Choose a reason for hiding this comment

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

My worry is mainly with anybody extending these tests in the future (which may not be a necessary worry and one that can be a bridge crossed when we get to it!), but my line of thinking is currently:

  • With expected_count we're toggling on a single variable when multiple may need to be changed in the future. A boolean for group_by_room would then be simpler.
  • Finalizing the code at the end in each function would require possible variables such as event IDs, room IDs etc to be returned by _test_push_unread_count to the calling function, which could be something else that gets a bit messy in the future.

However, I'm probably just making a mountain out of a molehill here, and taking your initial suggestion will likely be just fine for the current requirements of these tests.

Copy link
Member

@clokep clokep Nov 30, 2020

Choose a reason for hiding this comment

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

Finalizing the code at the end in each function would require possible variables such as event IDs, room IDs etc to be returned by _test_push_unread_count to the calling function, which could be something else that gets a bit messy in the future.

I don't follow this, I'm suggesting doing something like this:

def test_push_unread_count_group_by_room(self):
    self._test_push_unread_count()

   # 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
   )

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, well I meant in terms of wanting to do more requests, however I forgot that everything we need to assert is stored in self.push_attempts anyways huh!

That's quite elegant to check in a separate function. I think I'll give that a shot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in c05b7ed!

"""
Tests that the correct unread count appears in sent push notifications

Note that:
* Sending messages will cause push notifications to go out to relevant users
* Sending a read receipt will cause a "badge update" notification to go out to
the user that sent the receipt
"""
# Register the user who gets notified
user_id = self.register_user("user", "pass")
access_token = self.login("user", "pass")

# Register the user who sends the message
other_user_id = self.register_user("other_user", "pass")
other_access_token = self.login("other_user", "pass")

# Create a room (as other_user)
room_id = self.helper.create_room_as(other_user_id, tok=other_access_token)

# The user to get notified joins
self.helper.join(room=room_id, user=user_id, tok=access_token)

# Register the pusher
user_tuple = self.get_success(
self.hs.get_datastore().get_user_by_access_token(access_token)
)
token_id = user_tuple.token_id

self.get_success(
self.hs.get_pusherpool().add_pusher(
user_id=user_id,
access_token=token_id,
kind="http",
app_id="m.http",
app_display_name="HTTP Push Notifications",
device_display_name="pushy push",
pushkey="a@example.com",
lang=None,
data={"url": "example.com"},
)
)

# Send a message
response = self.helper.send(
room_id, body="Hello there!", tok=other_access_token
)
# To get an unread count, the user who is getting notified has to have a read
# position in the room. We'll set the read position to this event in a moment
first_message_event_id = response["event_id"]

# Advance time a bit (so the pusher will register something has happened) and
# make the push succeed
self.push_attempts[0][0].callback({})
self.pump()

# Check our push made it
self.assertEqual(len(self.push_attempts), 1)
self.assertEqual(self.push_attempts[0][1], "example.com")

# Check that the unread count for the room is 0
#
# The unread count is zero as the user has no read receipt in the room yet
self.assertEqual(
self.push_attempts[0][2]["notification"]["counts"]["unread"], 0
)

# Now set the user's read receipt position to the first event
#
# 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
request, channel = self.make_request(
"POST",
"/rooms/%s/receipt/m.read/%s" % (room_id, first_message_event_id),
{},
access_token=access_token,
)
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.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(
self.push_attempts[2][2]["notification"]["counts"]["unread"], 1
)

# Since we're grouping by room, sending more messages shouldn't increase the
# unread count, as they're all being sent in the same room
self.helper.send(room_id, body="Hello?", tok=other_access_token)

# Advance time and make the push succeed
self.pump()
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[4][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.assertEqual(len(self.push_attempts), 6)
if group_by_room:
# 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
)
else:
# We're counting every unread message, so there should now be 4 since our
# last read receipt
self.assertEqual(
self.push_attempts[5][2]["notification"]["counts"]["unread"], 4
)