diff --git a/changelog.d/20111.bugfix b/changelog.d/20111.bugfix new file mode 100644 index 0000000000..13ebff2eae --- /dev/null +++ b/changelog.d/20111.bugfix @@ -0,0 +1 @@ +Fix notification counts for the main timeline sometimes being attributed to a thread. diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 9c5fd35906..86af1ac449 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -704,21 +704,14 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas if thread_id not in summarised_threads: continue - if thread_id == MAIN_TIMELINE: - counts.notify_count += notif_count - counts.unread_count += unread_count - elif thread_id in thread_counts: - thread_counts[thread_id].notify_count += notif_count - thread_counts[thread_id].unread_count += unread_count - else: - # Previous thread summaries of 0 are discarded above. - # - # TODO If empty summaries are deleted this can be removed. - thread_counts[thread_id] = NotifCounts( - notify_count=notif_count, - unread_count=unread_count, - highlight_count=0, - ) + # Note that previous thread summaries of 0 are discarded above, so this + # may create a new entry. + # + # TODO If empty summaries are deleted this can use `thread_counts` + # directly. + counts = _get_thread(thread_id) + counts.notify_count += notif_count + counts.unread_count += unread_count # Finally we need to count push actions that aren't included in the # summary returned above. This might be due to recent events that haven't diff --git a/tests/storage/test_event_push_actions.py b/tests/storage/test_event_push_actions.py index bfe7049fb2..a5285ea2c5 100644 --- a/tests/storage/test_event_push_actions.py +++ b/tests/storage/test_event_push_actions.py @@ -472,6 +472,71 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): self.get_success(self.store._rotate_notifs()) _assert_badge(1) + def _send_message( + self, + room_id: str, + tok: str, + thread_root: str | None = None, + highlight_for: str | None = None, + ) -> str: + """Send a message, optionally into a thread and/or mentioning a user.""" + content: JsonDict = { + "msgtype": "m.text", + "body": highlight_for if highlight_for is not None else "msg", + } + if thread_root is not None: + content["m.relates_to"] = { + "rel_type": RelationTypes.THREAD, + "event_id": thread_root, + } + return self.helper.send_event( + room_id, type="m.room.message", content=content, tok=tok + )["event_id"] + + def test_count_aggregation_main_timeline_top_up_stays_on_main_timeline( + self, + ) -> None: + """ + Regression test: the top up of un-rotated push actions for the main timeline + must be added to the main timeline, not to whichever thread the summary query + happened to return last. + """ + user_id, _, _, other_token, room_id = self._create_users_and_room() + + self._send_message(room_id, other_token) + self.get_success(self.store._rotate_notifs()) + + # A summary row for a thread whose ID sorts after "main", so that it, rather + # than the main timeline, is the last row the summary query returns. + self.get_success( + self.store.db_pool.simple_insert( + table="event_push_summary", + values={ + "user_id": user_id, + "room_id": room_id, + "thread_id": "zzz-thread", + "notif_count": 1, + "unread_count": 0, + "stream_ordering": 100000, + "last_receipt_stream_ordering": None, + }, + ) + ) + + # A main timeline notification which has not been rotated yet. + self._send_message(room_id, other_token) + + counts = self.get_success( + self.store.db_pool.runInteraction( + "get-unread-counts", + self.store._get_unread_counts_by_receipt_txn, + room_id, + user_id, + ) + ) + self.assertEqual(counts.main_timeline, NotifCounts(notify_count=2)) + self.assertEqual(counts.threads, {"zzz-thread": NotifCounts(notify_count=1)}) + def test_count_aggregation_threads(self) -> None: """ This is essentially the same test as test_count_aggregation, but adds