diff --git a/changelog.d/15345.feature b/changelog.d/15345.feature index ebb3170961..4662bb0fd3 100644 --- a/changelog.d/15345.feature +++ b/changelog.d/15345.feature @@ -1,3 +1,3 @@ Follow-up to adding experimental feature flags per-user (#15344) which moves experimental features MSC3026 (busy presence), -MSC2654 (unread counts), MSC3881 (remotely toggle push notifications for another client), and -MSC3967 (Do not require UIA when first uploading cross signing keys) from the experimental config to per-user flags. +MSC3881 (remotely toggle push notifications for another client), and MSC3967 (Do not require UIA when first uploading +cross signing keys) from the experimental config to per-user flags. diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index ec78e77c11..320084f5f5 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -27,7 +27,6 @@ from typing import ( Union, ) -import attr from prometheus_client import Counter from synapse.api.constants import ( @@ -108,17 +107,6 @@ def _should_count_as_unread(event: EventBase, context: EventContext) -> bool: return False -@attr.s(slots=True, auto_attribs=True) -class ActionsForUser: - """ - A class to hold the actions for a given event and whether the event should - increment the unread count. - """ - - actions: Collection[Union[Mapping, str]] - count_as_unread: bool - - class BulkPushRuleEvaluator: """Calculates the outcome of push rules for an event for all users in the room at once. @@ -348,8 +336,15 @@ class BulkPushRuleEvaluator: # (historical messages persisted in reverse-chronological order). return + # Disable counting as unread unless the experimental configuration is + # enabled, as it can cause additional (unwanted) rows to be added to the + # event_push_actions table. + count_as_unread = False + if self.hs.config.experimental.msc2654_enabled: + count_as_unread = _should_count_as_unread(event, context) + rules_by_user = await self._get_rules_for_event(event) - actions_by_user: Dict[str, ActionsForUser] = {} + actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {} room_member_count = await self.store.get_number_joined_users_in_room( event.room_id @@ -434,22 +429,17 @@ class BulkPushRuleEvaluator: if not isinstance(display_name, str): display_name = None + if count_as_unread: + # Add an element for the current user if the event needs to be marked as + # unread, so that add_push_actions_to_staging iterates over it. + # If the event shouldn't be marked as unread but should notify the + # current user, it'll be added to the dict later. + actions_by_user[uid] = [] + actions = evaluator.run(rules, uid, display_name) - - # check whether unread counts are enabled for this user - unread_enabled = await self.store.get_feature_enabled(uid, "msc2654") - if not unread_enabled: - unread_enabled = self.hs.config.experimental.msc2654_enabled - - if unread_enabled: - count_as_unread = _should_count_as_unread(event, context) - else: - count_as_unread = False - - if "notify" in actions or count_as_unread: - # Push rules say we should notify the user of this event or the event should - # increment the unread count - actions_by_user[uid] = ActionsForUser(actions, count_as_unread) + if "notify" in actions: + # Push rules say we should notify the user of this event + actions_by_user[uid] = actions # If there aren't any actions then we can skip the rest of the # processing. @@ -477,6 +467,7 @@ class BulkPushRuleEvaluator: await self.store.add_push_actions_to_staging( event.event_id, actions_by_user, + count_as_unread, thread_id, ) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 0a5e08875f..cbcfacad90 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -100,6 +100,7 @@ class SyncRestServlet(RestServlet): self.presence_handler = hs.get_presence_handler() self._server_notices_sender = hs.get_server_notices_sender() self._event_serializer = hs.get_event_client_serializer() + self._msc2654_enabled = hs.config.experimental.msc2654_enabled self._msc3773_enabled = hs.config.experimental.msc3773_enabled async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -260,7 +261,7 @@ class SyncRestServlet(RestServlet): ) joined = await self.encode_joined( - sync_result.joined, time_now, serialize_options, requester + sync_result.joined, time_now, serialize_options ) invited = await self.encode_invited( @@ -272,7 +273,7 @@ class SyncRestServlet(RestServlet): ) archived = await self.encode_archived( - sync_result.archived, time_now, serialize_options, requester + sync_result.archived, time_now, serialize_options ) logger.debug("building sync response dict") @@ -343,7 +344,6 @@ class SyncRestServlet(RestServlet): rooms: List[JoinedSyncResult], time_now: int, serialize_options: SerializeEventConfig, - requester: Requester, ) -> JsonDict: """ Encode the joined rooms in a sync result @@ -352,7 +352,6 @@ class SyncRestServlet(RestServlet): rooms: list of sync results for rooms this user is joined to time_now: current time - used as a baseline for age calculations serialize_options: Event serializer options - requester: The requester of the sync Returns: The joined rooms list, in our response format """ @@ -363,7 +362,6 @@ class SyncRestServlet(RestServlet): time_now, joined=True, serialize_options=serialize_options, - requester=requester, ) return joined @@ -454,7 +452,6 @@ class SyncRestServlet(RestServlet): rooms: List[ArchivedSyncResult], time_now: int, serialize_options: SerializeEventConfig, - requester: Requester, ) -> JsonDict: """ Encode the archived rooms in a sync result @@ -463,7 +460,6 @@ class SyncRestServlet(RestServlet): rooms: list of sync results for rooms this user is joined to time_now: current time - used as a baseline for age calculations serialize_options: Event serializer options - requester: the requester of the sync Returns: The archived rooms list, in our response format """ @@ -474,7 +470,6 @@ class SyncRestServlet(RestServlet): time_now, joined=False, serialize_options=serialize_options, - requester=requester, ) return joined @@ -485,7 +480,6 @@ class SyncRestServlet(RestServlet): time_now: int, joined: bool, serialize_options: SerializeEventConfig, - requester: Requester, ) -> JsonDict: """ Args: @@ -498,7 +492,6 @@ class SyncRestServlet(RestServlet): only_fields: Optional. The list of event fields to include. event_formatter: function to convert from federation format to client format - Requester: The requester of the sync Returns: The room, encoded in our response format """ @@ -552,14 +545,7 @@ class SyncRestServlet(RestServlet): "org.matrix.msc3773.unread_thread_notifications" ] = room.unread_thread_notifications result["summary"] = room.summary - - msc2654_enabled = await self.hs.get_datastores().main.get_feature_enabled( - requester.user.to_string(), "msc2654" - ) - if not msc2654_enabled: - msc2654_enabled = self.hs.config.experimental.msc2654_enabled - - if msc2654_enabled: + if self._msc2654_enabled: result["org.matrix.msc2654.unread_count"] = room.unread_count return result diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 63f5a96809..eeccf5db24 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -105,7 +105,6 @@ from synapse.util import json_encoder from synapse.util.caches.descriptors import cached if TYPE_CHECKING: - from synapse.push.bulk_push_rule_evaluator import ActionsForUser from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -1216,7 +1215,8 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas async def add_push_actions_to_staging( self, event_id: str, - user_id_actions: Dict[str, "ActionsForUser"], + user_id_actions: Dict[str, Collection[Union[Mapping, str]]], + count_as_unread: bool, thread_id: str, ) -> None: """Add the push actions for the event to the push action staging area. @@ -1234,19 +1234,17 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas # This is a helper function for generating the necessary tuple that # can be used to insert into the `event_push_actions_staging` table. def _gen_entry( - user_id: str, actions_by_user: "ActionsForUser" + user_id: str, actions: Collection[Union[Mapping, str]] ) -> Tuple[str, str, str, int, int, int, str, int]: - is_highlight = 1 if _action_has_highlight(actions_by_user.actions) else 0 - notif = 1 if "notify" in actions_by_user.actions else 0 + is_highlight = 1 if _action_has_highlight(actions) else 0 + notif = 1 if "notify" in actions else 0 return ( event_id, # event_id column user_id, # user_id column - _serialize_action( - actions_by_user.actions, bool(is_highlight) - ), # actions column + _serialize_action(actions, bool(is_highlight)), # actions column notif, # notif column is_highlight, # highlight column - int(actions_by_user.count_as_unread), # unread column + int(count_as_unread), # unread column thread_id, # thread_id column self._clock.time_msec(), # inserted_ts column ) @@ -1264,8 +1262,8 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas "inserted_ts", ), values=[ - _gen_entry(user_id, actions_by_user) - for user_id, actions_by_user in user_id_actions.items() + _gen_entry(user_id, actions) + for user_id, actions in user_id_actions.items() ], desc="add_push_actions_to_staging", ) diff --git a/tests/replication/slave/storage/test_events.py b/tests/replication/slave/storage/test_events.py index aea25ed6df..b2125b1fea 100644 --- a/tests/replication/slave/storage/test_events.py +++ b/tests/replication/slave/storage/test_events.py @@ -24,7 +24,6 @@ from synapse.api.room_versions import RoomVersions from synapse.events import EventBase, _EventInternalMetadata, make_event_from_dict from synapse.events.snapshot import EventContext from synapse.handlers.room import RoomEventSource -from synapse.push.bulk_push_rule_evaluator import ActionsForUser from synapse.server import HomeServer from synapse.storage.databases.main.event_push_actions import ( NotifCounts, @@ -413,10 +412,8 @@ class EventsWorkerStoreTestCase(BaseSlavedStoreTestCase): self.get_success( self.master_store.add_push_actions_to_staging( event.event_id, - { - user_id: ActionsForUser(actions, False) - for user_id, actions in push_actions - }, + dict(push_actions), + False, "main", ) ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 49cd4cee60..9c876c7a32 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -28,7 +28,6 @@ from synapse.api.constants import ( ReceiptTypes, RelationTypes, ) -from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -539,12 +538,14 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): def default_config(self) -> JsonDict: config = super().default_config() + config["experimental_features"] = { + "msc2654_enabled": True, + } return config def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.url = "/sync?since=%s" self.next_batch = "s0" - self.hs = hs # Register the first user (used to check the unread counts). self.user_id = self.register_user("kermit", "monkey") @@ -587,12 +588,6 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" - # add per-user flag to the DB - self.get_success( - self.hs.get_datastores().main.set_features_for_user( - self.user_id, {ExperimentalFeature.MSC2654: True} - ) - ) # Check that our own messages don't increase the unread count. self.helper.send(self.room_id, "hello", tok=self.tok)