Add a config option to change whether unread push notification counts…

… are per-message or per-room (#8820)

This PR adds a new config option to the `push` section of the homeserver config, `group_unread_count_by_room`. By default Synapse will group push notifications by room (so if you have 1000 unread messages, if they lie in 55 rooms, you'll see an unread count on your phone of 55).

However, it is also useful to be able to send out the true count of unread messages if desired. If `group_unread_count_by_room` is set to `false`, then with the above example, one would see an unread count of 1000 (email anyone?).

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)