Fix some comments and types in service notices

changelog.d/7996.bugfix

@@ -0,0 +1 @@
+Fix various comments and minor discrepencies in server notices code.

synapse/server_notices/consent_server_notices.py

@@ -13,6 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
+from typing import Any
 
 from synapse.api.errors import SynapseError
 from synapse.api.urls import ConsentURIBuilder
@@ -55,14 +56,11 @@ def __init__(self, hs):
 
             self._consent_uri_builder = ConsentURIBuilder(hs.config)
 
-    async def maybe_send_server_notice_to_user(self, user_id):
+    async def maybe_send_server_notice_to_user(self, user_id: str) -> None:
         """Check if we need to send a notice to this user, and does so if so
 
         Args:
-            user_id (str): user to check
-
-        Returns:
-            Deferred
+            user_id: user to check
         """
         if self._server_notice_content is None:
             # not enabled
@@ -105,7 +103,7 @@ async def maybe_send_server_notice_to_user(self, user_id):
             self._users_in_progress.remove(user_id)
 
 
-def copy_with_str_subst(x, substitutions):
+def copy_with_str_subst(x: Any, substitutions: Any) -> Any:
     """Deep-copy a structure, carrying out string substitions on any strings
 
     Args:
@@ -121,7 +119,7 @@ def copy_with_str_subst(x, substitutions):
     if isinstance(x, dict):
         return {k: copy_with_str_subst(v, substitutions) for (k, v) in x.items()}
     if isinstance(x, (list, tuple)):
-        return [copy_with_str_subst(y) for y in x]
+        return [copy_with_str_subst(y, substitutions) for y in x]
 
     # assume it's uninterested and can be shallow-copied.
     return x

synapse/server_notices/resource_limits_server_notices.py

@@ -13,6 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
+from typing import List, Tuple
 
 from synapse.api.constants import (
     EventTypes,
@@ -52,18 +53,15 @@ def __init__(self, hs):
             and not hs.config.hs_disabled
         )
 
-    async def maybe_send_server_notice_to_user(self, user_id):
+    async def maybe_send_server_notice_to_user(self, user_id: str) -> None:
         """Check if we need to send a notice to this user, this will be true in
         two cases.
         1. The server has reached its limit does not reflect this
         2. The room state indicates that the server has reached its limit when
         actually the server is fine
 
         Args:
-            user_id (str): user to check
-
-        Returns:
-            Deferred
+            user_id: user to check
         """
         if not self._enabled:
             return
@@ -115,36 +113,38 @@ async def maybe_send_server_notice_to_user(self, user_id):
             elif not currently_blocked and limit_msg:
                 # Room is not notifying of a block, when it ought to be.
                 await self._apply_limit_block_notification(
-                    user_id, limit_msg, limit_type
+                    user_id, limit_msg, limit_type  # type: ignore
                 )
         except SynapseError as e:
             logger.error("Error sending resource limits server notice: %s", e)
 
-    async def _remove_limit_block_notification(self, user_id, ref_events):
+    async def _remove_limit_block_notification(
+        self, user_id: str, ref_events: List[str]
+    ) -> None:
         """Utility method to remove limit block notifications from the server
         notices room.
 
         Args:
-            user_id (str): user to notify
-            ref_events (list[str]): The event_ids of pinned events that are unrelated to
-            limit blocking and need to be preserved.
+            user_id: user to notify
+            ref_events: The event_ids of pinned events that are unrelated to
+                limit blocking and need to be preserved.
         """
         content = {"pinned": ref_events}
         await self._server_notices_manager.send_notice(
             user_id, content, EventTypes.Pinned, ""
         )
 
     async def _apply_limit_block_notification(
-        self, user_id, event_body, event_limit_type
-    ):
+        self, user_id: str, event_body: str, event_limit_type: str
+    ) -> None:
         """Utility method to apply limit block notifications in the server
         notices room.
 
         Args:
-            user_id (str): user to notify
-            event_body(str): The human readable text that describes the block.
-            event_limit_type(str): Specifies the type of block e.g. monthly active user
-            limit has been exceeded.
+            user_id: user to notify
+            event_body: The human readable text that describes the block.
+            event_limit_type: Specifies the type of block e.g. monthly active user
+                limit has been exceeded.
         """
         content = {
             "body": event_body,
@@ -162,7 +162,7 @@ async def _apply_limit_block_notification(
             user_id, content, EventTypes.Pinned, ""
         )
 
-    async def _check_and_set_tags(self, user_id, room_id):
+    async def _check_and_set_tags(self, user_id: str, room_id: str) -> None:
         """
         Since server notices rooms were originally not with tags,
         important to check that tags have been set correctly
@@ -182,17 +182,16 @@ async def _check_and_set_tags(self, user_id, room_id):
             )
             self._notifier.on_new_event("account_data_key", max_id, users=[user_id])
 
-    async def _is_room_currently_blocked(self, room_id):
+    async def _is_room_currently_blocked(self, room_id: str) -> Tuple[bool, List[str]]:
         """
         Determines if the room is currently blocked
 
         Args:
-            room_id(str): The room id of the server notices room
+            room_id: The room id of the server notices room
 
         Returns:
-            Deferred[Tuple[bool, List]]:
             bool: Is the room currently blocked
-            list: The list of pinned events that are unrelated to limit blocking
+            list: The list of pinned event IDs that are unrelated to limit blocking
             This list can be used as a convenience in the case where the block
             is to be lifted and the remaining pinned event references need to be
             preserved
@@ -207,7 +206,7 @@ async def _is_room_currently_blocked(self, room_id):
             # The user has yet to join the server notices room
             pass
 
-        referenced_events = []
+        referenced_events = []  # type: List[str]
         if pinned_state_event is not None:
             referenced_events = list(pinned_state_event.content.get("pinned", []))
 

synapse/server_notices/server_notices_manager.py

@@ -13,8 +13,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
+from typing import Optional
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
+from synapse.events import EventBase
 from synapse.types import UserID, create_requester
 from synapse.util.caches.descriptors import cached
 
@@ -50,20 +52,21 @@ def is_enabled(self):
         return self._config.server_notices_mxid is not None
 
     async def send_notice(
-        self, user_id, event_content, type=EventTypes.Message, state_key=None
-    ):
+        self,
+        user_id: str,
+        event_content: dict,
+        type: str = EventTypes.Message,
+        state_key: Optional[bool] = None,
+    ) -> EventBase:
         """Send a notice to the given user
 
         Creates the server notices room, if none exists.
 
         Args:
-            user_id (str): mxid of user to send event to.
-            event_content (dict): content of event to send
-            type(EventTypes): type of event
-            is_state_event(bool): Is the event a state event
-
-        Returns:
-            Deferred[FrozenEvent]
+            user_id: mxid of user to send event to.
+            event_content: content of event to send
+            type: type of event
+            is_state_event: Is the event a state event
         """
         room_id = await self.get_or_create_notice_room_for_user(user_id)
         await self.maybe_invite_user_to_room(user_id, room_id)
@@ -89,17 +92,17 @@ async def send_notice(
         return event
 
     @cached()
-    async def get_or_create_notice_room_for_user(self, user_id):
+    async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
         """Get the room for notices for a given user
 
         If we have not yet created a notice room for this user, create it, but don't
         invite the user to it.
 
         Args:
-            user_id (str): complete user id for the user we want a room for
+            user_id: complete user id for the user we want a room for
 
         Returns:
-            str: room id of notice room.
+            room id of notice room.
         """
         if not self.is_enabled():
             raise Exception("Server notices not enabled")
@@ -163,7 +166,7 @@ async def get_or_create_notice_room_for_user(self, user_id):
         logger.info("Created server notices room %s for %s", room_id, user_id)
         return room_id
 
-    async def maybe_invite_user_to_room(self, user_id: str, room_id: str):
+    async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None:
         """Invite the given user to the given server room, unless the user has already
         joined or been invited to it.
 

synapse/server_notices/server_notices_sender.py

@@ -12,6 +12,8 @@
 # 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 typing import Iterable, Union
+
 from synapse.server_notices.consent_server_notices import ConsentServerNotices
 from synapse.server_notices.resource_limits_server_notices import (
     ResourceLimitsServerNotices,
@@ -32,22 +34,22 @@ def __init__(self, hs):
         self._server_notices = (
             ConsentServerNotices(hs),
             ResourceLimitsServerNotices(hs),
-        )
+        )  # type: Iterable[Union[ConsentServerNotices, ResourceLimitsServerNotices]]
 
-    async def on_user_syncing(self, user_id):
+    async def on_user_syncing(self, user_id: str) -> None:
         """Called when the user performs a sync operation.
 
         Args:
-            user_id (str): mxid of user who synced
+            user_id: mxid of user who synced
         """
         for sn in self._server_notices:
             await sn.maybe_send_server_notice_to_user(user_id)
 
-    async def on_user_ip(self, user_id):
+    async def on_user_ip(self, user_id: str) -> None:
         """Called on the master when a worker process saw a client request.
 
         Args:
-            user_id (str): mxid
+            user_id: mxid
         """
         # The synchrotrons use a stubbed version of ServerNoticesSender, so
         # we check for notices to send to the user in on_user_ip as well as

synapse/server_notices/worker_server_notices_sender.py

@@ -12,7 +12,6 @@
 # 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 twisted.internet import defer
 
 
 class WorkerServerNoticesSender(object):
@@ -24,24 +23,18 @@ def __init__(self, hs):
             hs (synapse.server.HomeServer):
         """
 
-    def on_user_syncing(self, user_id):
+    async def on_user_syncing(self, user_id: str) -> None:
         """Called when the user performs a sync operation.
 
         Args:
-            user_id (str): mxid of user who synced
-
-        Returns:
-            Deferred
+            user_id: mxid of user who synced
         """
-        return defer.succeed(None)
+        return None
 
-    def on_user_ip(self, user_id):
+    async def on_user_ip(self, user_id: str) -> None:
         """Called on the master when a worker process saw a client request.
 
         Args:
-            user_id (str): mxid
-
-        Returns:
-            Deferred
+            user_id: mxid
         """
         raise AssertionError("on_user_ip unexpectedly called on worker")

tox.ini

@@ -202,6 +202,7 @@ commands = mypy \
             synapse/push/push_rule_evaluator.py \
             synapse/replication \
             synapse/rest \
+            synapse/server_notices \
             synapse/spam_checker_api \
             synapse/storage/data_stores/main/ui_auth.py \
             synapse/storage/database.py \