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

changelog.d/8820.feature

@@ -0,0 +1 @@
+Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "the number of rooms with unread messages" or "total unread messages".

docs/sample_config.yaml

@@ -2271,6 +2271,16 @@ push:
   #
   #include_content: false
 
+  # When a push notification is received, an unread count is also sent.
+  # This number can either be calculated as the number of unread messages
+  # for the user, or the number of *rooms* the user has unread messages in.
+  #
+  # The default value is "true", meaning push clients will see the number of
+  # rooms with unread messages in them. Uncomment to instead send the 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

synapse/config/push.py

@@ -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)
@@ -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, an unread count is also sent.
+          # This number can either be calculated as the number of unread messages
+          # for the user, or the number of *rooms* the user has unread messages in.
+          #
+          # The default value is "true", meaning push clients will see the number of
+          # rooms with unread messages in them. Uncomment to instead send the number
+          # of unread messages.
+          #
+          #group_unread_count_by_room: false
         """

synapse/push/httppusher.py

@@ -75,6 +75,7 @@ def __init__(self, hs, pusherdict):
         self.failing_since = pusherdict["failing_since"]
         self.timed_call = None
         self._is_processing = False
+        self._group_unread_count_by_room = 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
@@ -136,7 +137,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._group_unread_count_by_room,
+        )
         await self._send_badge(badge)
 
     def on_timer(self):
@@ -283,7 +288,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._group_unread_count_by_room,
+        )
 
         event = await self.store.get_event(push_action["event_id"], allow_none=True)
         if event is None:

synapse/push/push_tools.py

@@ -12,12 +12,12 @@
 # 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 synapse.push.presentable_names import calculate_room_name, name_from_member_event
 from synapse.storage import Storage
+from synapse.storage.databases.main import DataStore
 
 
-async def get_badge_count(store, user_id):
+async def get_badge_count(store: DataStore, user_id: str, group_by_room: bool) -> int:
     invites = await store.get_invited_rooms_for_local_user(user_id)
     joins = await store.get_rooms_for_user(user_id)
 
@@ -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
 
 

tests/push/test_http.py

@@ -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
@@ -499,3 +500,161 @@ 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.
+        """
+        # Carry out common push count tests and setup
+        self._test_push_unread_count()
+
+        # Carry out our option-value specific test
+        #
+        # 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
+        )
+
+    @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.
+        """
+        # Carry out common push count tests and setup
+        self._test_push_unread_count()
+
+        # Carry out our option-value specific test
+        #
+        # 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
+        )
+
+    def _test_push_unread_count(self):
+        """
+        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)