refactor for readability, and reuse caching for setting tags

synapse/server_notices/resource_limits_server_notices.py

@@ -18,6 +18,7 @@ from twisted.internet import defer
 
 from synapse.api.constants import EventTypes
 from synapse.api.errors import AuthError, SynapseError
+from synapse.server_notices.server_notices_manager import SERVER_NOTICE_ROOM_TAG
 
 logger = logging.getLogger(__name__)
 
@@ -57,20 +58,100 @@ class ResourceLimitsServerNotices(object):
         if self._hs_disabled is True:
             return
 
-        if self._limit_usage_by_mau is True:
+        if self._limit_usage_by_mau is False:
+            return
+
         timestamp = yield self._store.user_last_seen_monthly_active(user_id)
         if timestamp is None:
             # This user will be blocked from receiving the notice anyway.
             # In practice, not sure we can ever get here
             return
 
+        # Determine current state of room
+
         room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id)
 
-            # Need to set tag here because room may have been created prior to
-            # tagging being set on creation. Ideally would set something to read
-            # room tags first, and cache that aggressively/
-            yield self._store.add_tag_to_room(user_id, room_id, 'm.server_notice', None)
+        yield self._check_and_set_tags(user_id, room_id)
+        currently_blocked, ref_events = yield self._is_room_currently_blocked(room_id)
 
+        try:
+            # Normally should always pass in user_id if you have it, but in
+            # this case are checking what would happen to other users if they
+            # were to arrive.
+            yield self.auth.check_auth_blocking()
+            if currently_blocked:
+                # Room is notifying of a block, when it ought not to be.
+                # Remove block notification
+                content = {
+                    "pinned": ref_events
+                }
+                yield self._server_notices_manager.send_notice(
+                    user_id, content, EventTypes.Pinned, '',
+                )
+
+        except AuthError as e:
+
+            try:
+                if not currently_blocked:
+                    # Room is not notifying of a block, when it ought to be.
+                    # Add block notification
+                    content = {
+                        'body': e.msg,
+                        'admin_uri': self._admin_uri,
+                    }
+                    event = yield self._server_notices_manager.send_notice(
+                        user_id, content, EventTypes.ServerNoticeLimitReached
+                    )
+
+                    content = {
+                        "pinned": [
+                            event.event_id,
+                        ]
+                    }
+                    yield self._server_notices_manager.send_notice(
+                        user_id, content, EventTypes.Pinned, '',
+                    )
+
+            except SynapseError as e:
+                logger.error("Error sending resource limits server notice: %s", e)
+
+    @defer.inlineCallbacks
+    def _check_and_set_tags(self, user_id, room_id):
+        """
+        Since server notices rooms were originally not with tags,
+        important to check that tags have been set correctly
+        Args:
+            user_id(str): the user in question
+            room_id(str): the server notices room for that user
+        """
+        tags = yield self._store.get_tags_for_user(user_id)
+        server_notices_tags = tags.get(room_id)
+        need_to_set_tag = True
+        if server_notices_tags:
+            if server_notice_tags.get(SERVER_NOTICE_ROOM_TAG):
+                # tag already present, nothing to do here
+                need_to_set_tag = False
+        if need_to_set_tag:
+            yield self._store.add_tag_to_room(
+                user_id, room_id, SERVER_NOTICE_ROOM_TAG, None
+            )
+
+    @defer.inlineCallbacks
+    def _is_room_currently_blocked(self, room_id):
+        """
+        Determines if the room is currently blocked
+
+        Args:
+            room_id(str): The room id of the server notices room
+
+        Returns:
+
+            bool: Is the room currently blocked
+            list: The list of pinned events 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
+        """
         currently_blocked = False
         pinned_state_event = None
         try:
@@ -86,51 +167,12 @@ class ResourceLimitsServerNotices(object):
             referenced_events = pinned_state_event.content.get('pinned')
 
         events = yield self._store.get_events(referenced_events)
+        event_to_remove = None
         for event_id, event in events.items():
             if event.type == EventTypes.ServerNoticeLimitReached:
                 currently_blocked = True
-            try:
-                # Normally should always pass in user_id if you have it, but in
-                # this case are checking what would happen to other users if they
-                # were to arrive.
-                yield self.auth.check_auth_blocking()
+                # remove event in case we need to disable blocking later on.
+                if event_id in referenced_events:
+                    referenced_events.remove(event.event_id)
 
-                # Need to start removing notices
-                # if user_id in self._notified_of_blocking:
-                if currently_blocked:
-                    # Send message to remove warning
-                    # send state event here
-                    # How do I do this? if drop the id, how to refer to it?
-                    content = {
-                        "pinned": []
-                    }
-                    yield self._server_notices_manager.send_notice(
-                        user_id, content, EventTypes.Pinned, '',
-                    )
-
-            except AuthError as e:
-                # Need to start notifying of blocking
-                try:
-                    if not currently_blocked:
-                        # TODO use admin email contained in error once PR lands
-                        content = {
-                            'body': e.msg,
-                            'admin_uri': self._admin_uri,
-                        }
-                        event = yield self._server_notices_manager.send_notice(
-                            user_id, content, EventTypes.ServerNoticeLimitReached
-                        )
-
-                        # send server notices state event here
-                        # TODO Over writing pinned events
-                        content = {
-                            "pinned": [
-                                event.event_id,
-                            ]
-                        }
-                        yield self._server_notices_manager.send_notice(
-                            user_id, content, EventTypes.Pinned, '',
-                        )
-
-                except SynapseError as e:
-                    logger.error("Error sending resource limits server notice: %s", e)
+        defer.returnValue((currently_blocked, referenced_events))

synapse/server_notices/server_notices_manager.py

@@ -22,6 +22,8 @@ from synapse.util.caches.descriptors import cachedInlineCallbacks
 
 logger = logging.getLogger(__name__)
 
+SERVER_NOTICE_ROOM_TAG = "m.server_notice"
+
 
 class ServerNoticesManager(object):
     def __init__(self, hs):
@@ -151,7 +153,9 @@ class ServerNoticesManager(object):
             creator_join_profile=join_profile,
         )
         room_id = info['room_id']
-        yield self._store.add_tag_to_room(user_id, room_id, 'm.server_notice', None)
+        yield self._store.add_tag_to_room(
+            user_id, room_id, SERVER_NOTICE_ROOM_TAG, None
+        )
 
         logger.info("Created server notices room %s for %s", room_id, user_id)
         defer.returnValue(room_id)

tests/server_notices/test_resource_limits_server_notices.py

@@ -54,6 +54,7 @@ class TestResourceLimitsServerNotices(unittest.TestCase):
         self._rlsn._server_notices_manager.get_notice_room_for_user = Mock(
             returnValue=""
         )
+        self._rlsn._store.add_tag_to_room = Mock()
         self.hs.config.admin_uri = "mailto:user@test.com"
 
     @defer.inlineCallbacks