From 96159442418779d90b2b9e87065c52eb42ec25aa Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Tue, 1 Mar 2022 13:30:15 +0100
Subject: [PATCH 1/8] Update server notice profile in room if changed

Signed-off-by: Jorge Florian <jafn28@gmail.com>
---
 .../resource_limits_server_notices.py         |   5 +-
 .../server_notices/server_notices_manager.py  |  68 +++++-
 .../test_resource_limits_server_notices.py    |   2 +-
 .../test_server_notices_manager.py            | 195 ++++++++++++++++++
 4 files changed, 261 insertions(+), 9 deletions(-)
 create mode 100644 tests/server_notices/test_server_notices_manager.py

diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py
index 015dd08f05e4..76a0272961b7 100644
--- a/synapse/server_notices/resource_limits_server_notices.py
+++ b/synapse/server_notices/resource_limits_server_notices.py
@@ -71,7 +71,10 @@ async def maybe_send_server_notice_to_user(self, user_id: str) -> None:
             # In practice, not sure we can ever get here
             return
 
-        room_id = await self._server_notices_manager.get_or_create_notice_room_for_user(
+        (
+            room_id,
+            _,
+        ) = await self._server_notices_manager.get_or_create_notice_room_for_user(
             user_id
         )
 
diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index 7b4814e04994..32e8793854f0 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -12,11 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Optional
+from typing import TYPE_CHECKING, Optional, Tuple
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
 from synapse.events import EventBase
-from synapse.types import UserID, create_requester
+from synapse.types import Requester, UserID, create_requester
 from synapse.util.caches.descriptors import cached
 
 if TYPE_CHECKING:
@@ -35,6 +35,7 @@ def __init__(self, hs: "HomeServer"):
         self._room_creation_handler = hs.get_room_creation_handler()
         self._room_member_handler = hs.get_room_member_handler()
         self._event_creation_handler = hs.get_event_creation_handler()
+        self._message_handler = hs.get_message_handler()
         self._is_mine_id = hs.is_mine_id
         self._server_name = hs.hostname
 
@@ -64,7 +65,7 @@ async def send_notice(
             is_state_event: Is the event a state event
             txn_id: The transaction ID.
         """
-        room_id = await self.get_or_create_notice_room_for_user(user_id)
+        (room_id, room_created) = await self.get_or_create_notice_room_for_user(user_id)
         await self.maybe_invite_user_to_room(user_id, room_id)
 
         assert self.server_notices_mxid is not None
@@ -72,6 +73,14 @@ async def send_notice(
             self.server_notices_mxid, authenticated_entity=self._server_name
         )
 
+        if not room_created:
+            await self._update_notice_user_profile_if_changed(
+                requester,
+                room_id,
+                self._config.servernotices.server_notices_mxid_display_name,
+                self._config.servernotices.server_notices_mxid_avatar_url,
+            )
+
         logger.info("Sending server notice to %s", user_id)
 
         event_dict = {
@@ -90,7 +99,9 @@ async def send_notice(
         return event
 
     @cached()
-    async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
+    async def get_or_create_notice_room_for_user(
+        self, user_id: str
+    ) -> Tuple[str, bool]:
         """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
@@ -100,7 +111,7 @@ async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
             user_id: complete user id for the user we want a room for
 
         Returns:
-            room id of notice room.
+            room id of notice room and flag indicating weather room was created.
         """
         if self.server_notices_mxid is None:
             raise Exception("Server notices not enabled")
@@ -125,7 +136,7 @@ async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
                     room.room_id,
                     user_id,
                 )
-                return room.room_id
+                return room.room_id, False
 
         # apparently no existing notice room: create a new one
         logger.info("Creating server notices room for %s", user_id)
@@ -164,7 +175,7 @@ async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
         self._notifier.on_new_event("account_data_key", max_id, users=[user_id])
 
         logger.info("Created server notices room %s for %s", room_id, user_id)
-        return room_id
+        return room_id, True
 
     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
@@ -194,3 +205,46 @@ async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None:
             room_id=room_id,
             action="invite",
         )
+
+    async def _update_notice_user_profile_if_changed(
+        self,
+        requester: Requester,
+        room_id: str,
+        display_name: Optional[str],
+        avatar_url: Optional[str],
+    ) -> None:
+        """
+        Updates notice user profile if it's different from what it's saved in the room.
+
+        Args:
+            requester: The user who is performing the update.
+            room_id: The ID of the server notice room
+            display_name: The displayname of the server notice user
+            avatar_url: The avatar url of the server notice user
+        """
+        logger.info("Checking whether notice user profile has changed", room_id)
+
+        assert self.server_notices_mxid is not None
+
+        notice_user_data_in_room = await self._message_handler.get_room_data(
+            self.server_notices_mxid,
+            room_id,
+            EventTypes.Member,
+            self.server_notices_mxid,
+        )
+
+        assert notice_user_data_in_room is not None
+
+        notice_user_profile_changed = (
+            display_name != notice_user_data_in_room.content.get("displayname")
+            or avatar_url != notice_user_data_in_room.content.get("avatar_url")
+        )
+        if notice_user_profile_changed:
+            logger.info("Updating notice user profile in room %s", room_id)
+            await self._room_member_handler.update_membership(
+                requester=requester,
+                target=UserID.from_string(self.server_notices_mxid),
+                room_id=room_id,
+                action="join",
+                content={"displayname": display_name, "avatar_url": avatar_url},
+            )
diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index 02b96c9e6ecf..7d7b115384a6 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -75,7 +75,7 @@ def prepare(self, reactor, clock, hs):
         self.user_id = "@user_id:test"
 
         self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock(
-            return_value=defer.succeed("!something:localhost")
+            return_value=defer.succeed(("!something:localhost", False))
         )
         self._rlsn._store.add_tag_to_room = Mock(return_value=defer.succeed(None))
         self._rlsn._store.get_tags_for_room = Mock(return_value=make_awaitable({}))
diff --git a/tests/server_notices/test_server_notices_manager.py b/tests/server_notices/test_server_notices_manager.py
new file mode 100644
index 000000000000..a9de5e1f719a
--- /dev/null
+++ b/tests/server_notices/test_server_notices_manager.py
@@ -0,0 +1,195 @@
+# Copyright 2018, 2019 New Vector Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# 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 cast
+from unittest.mock import Mock
+
+from twisted.internet import defer
+
+from synapse.api.constants import EventTypes, Membership
+from synapse.api.room_versions import RoomVersions
+from synapse.events import FrozenEvent
+from synapse.server_notices.server_notices_manager import ServerNoticesManager
+from synapse.storage.roommember import RoomsForUser
+
+from tests import unittest
+from tests.unittest import override_config
+from tests.utils import default_config
+
+server_notice_config = {
+    "system_mxid_localpart": "server",
+    "system_mxid_display_name": "display name",
+    "system_mxid_avatar_url": "test/url",
+    "room_name": "Server Notices",
+}
+
+
+class TestServiceNoticeManager(unittest.HomeserverTestCase):
+    def default_config(self):
+        config = cast(dict, default_config("test"))
+        config.update({"server_notices": server_notice_config})
+
+        # apply any additional config which was specified via the override_config
+        # decorator.
+        if self._extra_config is not None:
+            config.update(self._extra_config)
+
+        return config
+
+    def prepare(self, reactor, clock, hs):
+        self.server_notices_manager = self.hs.get_server_notices_manager()
+
+        self.server_notices_manager._event_creation_handler.create_and_send_nonmember_event = Mock(
+            return_value=defer.succeed(
+                (FrozenEvent({"event_id": "$notice_event123"}, RoomVersions.V1), 0)
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership = Mock(
+            return_value=defer.succeed(("$updated_membership123", 1))
+        )
+
+        self.user_id = "@user_id:test"
+        self.notice_message = {"content": "a new message from server", type: "m.text"}
+
+    @override_config(
+        {
+            "server_notices": {
+                **server_notice_config,
+                "system_mxid_display_name": "new display name",
+            }
+        }
+    )
+    def test_update_notice_user_name_when_changed(self):
+        """
+        Tests that existing server notices user name in room is updated when is
+        different from the one in homeserver config.
+        """
+        self._mock_existing_notice_room(
+            self.server_notices_manager, server_notice_config
+        )
+
+        self.get_success(
+            self.server_notices_manager.send_notice(
+                self.user_id, self.notice_message, EventTypes.Message
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
+        call_args_content = self.server_notices_manager._room_member_handler.update_membership.call_args.kwargs[
+            "content"
+        ]
+
+        assert call_args_content == {
+            "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
+            "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
+        }
+
+    @override_config(
+        {
+            "server_notices": {
+                **server_notice_config,
+                "system_mxid_avatar_url": "test/new-url",
+            }
+        }
+    )
+    def test_update_notice_user_avatar_when_changed(self):
+        """
+        Tests that existing server notices user avatar in room is updated when is
+        different from the one in homeserver config.
+        """
+        self._mock_existing_notice_room(
+            self.server_notices_manager, server_notice_config
+        )
+
+        self.get_success(
+            self.server_notices_manager.send_notice(
+                self.user_id, self.notice_message, EventTypes.Message
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
+        call_args_content = self.server_notices_manager._room_member_handler.update_membership.call_args.kwargs[
+            "content"
+        ]
+
+        assert call_args_content == {
+            "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
+            "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
+        }
+
+    def test_doesnt_update_notice_user_profile_when_not_changed(self):
+        """
+        Tests that existing server notices profile in room is not updated when is
+        equal to the one in homeserver config.
+        """
+        self._mock_existing_notice_room(
+            self.server_notices_manager, server_notice_config
+        )
+
+        self.get_success(
+            self.server_notices_manager.send_notice(
+                self.user_id, self.notice_message, EventTypes.Message
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership.assert_not_called()
+
+    def _mock_existing_notice_room(
+        self, server_notices_manager: ServerNoticesManager, server_notice_config: dict
+    ):
+        """
+        Mocks ServerNoticesManager dependencies used for reading info
+        about existing server notice room.
+        """
+        # Ignored type error: Cannot assign to a method
+        #  https://github.com/python/mypy/issues/2427
+        server_notices_manager._store.get_rooms_for_local_user_where_membership_is = Mock(  # type: ignore
+            return_value=defer.succeed(
+                [
+                    RoomsForUser(
+                        room_id="!something:test",
+                        event_id="$abc123",
+                        membership=Membership.JOIN,
+                        sender="@server:test",
+                        room_version_id="1",
+                        stream_ordering=0,
+                    )
+                ]
+            )
+        )
+
+        # Ignored type error: Cannot assign to a method
+        #  https://github.com/python/mypy/issues/2427
+        server_notices_manager._store.get_users_in_room = Mock(  # type: ignore
+            return_value=defer.succeed([server_notices_manager.server_notices_mxid])
+        )
+
+        notice_user_profile_in_room = {
+            "displayname": server_notice_config["system_mxid_display_name"],
+            "avatar_url": server_notice_config["system_mxid_avatar_url"],
+        }
+        # Ignored type error: Cannot assign to a method
+        #  https://github.com/python/mypy/issues/2427
+        server_notices_manager._message_handler.get_room_data = Mock(  # type: ignore
+            return_value=defer.succeed(
+                FrozenEvent(
+                    {
+                        "event_id": "$notice_user_state_123",
+                        "content": notice_user_profile_in_room,
+                    },
+                    RoomVersions.V1,
+                )
+            )
+        )

From 809483d9fe8acae66c714717d556415237130c43 Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Tue, 1 Mar 2022 14:45:06 +0100
Subject: [PATCH 2/8] add Changelog

---
 changelog.d/12115.bugfix | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 changelog.d/12115.bugfix

diff --git a/changelog.d/12115.bugfix b/changelog.d/12115.bugfix
new file mode 100644
index 000000000000..764d59920a2d
--- /dev/null
+++ b/changelog.d/12115.bugfix
@@ -0,0 +1,2 @@
+Fix bug: Server notices user profile (display_name/avatar_url) are not updated when changed after server restart.
+Contributed by Jorge Florian.

From d953606b5d924b8ba0607c6818d8b6286ebef437 Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Wed, 2 Mar 2022 22:29:25 +0100
Subject: [PATCH 3/8] fix failing test_resource_limits_server_notices

Signed-off-by: Jorge Florian <jafn28@gmail.com>
---
 tests/server_notices/test_resource_limits_server_notices.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index 7d7b115384a6..f522234516f5 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -270,7 +270,7 @@ def test_server_notice_only_sent_once(self):
 
         # Now lets get the last load of messages in the service notice room and
         # check that there is only one server notice
-        room_id = self.get_success(
+        (room_id, _) = self.get_success(
             self.server_notices_manager.get_or_create_notice_room_for_user(self.user_id)
         )
 

From 4a7a6fa6606400474b001afe37c188f4bd0057be Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Fri, 4 Mar 2022 10:20:36 +0100
Subject: [PATCH 4/8] Remove unsupported kwargs call_args uses in Python 3.7

---
 .../test_server_notices_manager.py             | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tests/server_notices/test_server_notices_manager.py b/tests/server_notices/test_server_notices_manager.py
index a9de5e1f719a..5c209ce537b2 100644
--- a/tests/server_notices/test_server_notices_manager.py
+++ b/tests/server_notices/test_server_notices_manager.py
@@ -87,11 +87,12 @@ def test_update_notice_user_name_when_changed(self):
         )
 
         self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
-        call_args_content = self.server_notices_manager._room_member_handler.update_membership.call_args.kwargs[
-            "content"
-        ]
+        (
+            _,
+            kwargs,
+        ) = self.server_notices_manager._room_member_handler.update_membership.call_args
 
-        assert call_args_content == {
+        assert kwargs["content"] == {
             "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
             "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
         }
@@ -120,11 +121,12 @@ def test_update_notice_user_avatar_when_changed(self):
         )
 
         self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
-        call_args_content = self.server_notices_manager._room_member_handler.update_membership.call_args.kwargs[
-            "content"
-        ]
+        (
+            _,
+            kwargs,
+        ) = self.server_notices_manager._room_member_handler.update_membership.call_args
 
-        assert call_args_content == {
+        assert kwargs["content"] == {
             "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
             "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
         }

From 2b41d294d4b0b4e9c896372e7d03ea8cc4024cf6 Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Thu, 17 Mar 2022 23:32:46 +0100
Subject: [PATCH 5/8] address review comments

---
 changelog.d/12115.bugfix                      |   3 +-
 .../resource_limits_server_notices.py         |   5 +-
 .../server_notices/server_notices_manager.py  |  35 ++--
 tests/rest/admin/test_server_notice.py        | 137 +++++++++++-
 .../test_server_notices_manager.py            | 197 ------------------
 5 files changed, 153 insertions(+), 224 deletions(-)
 delete mode 100644 tests/server_notices/test_server_notices_manager.py

diff --git a/changelog.d/12115.bugfix b/changelog.d/12115.bugfix
index 764d59920a2d..eff842b2a518 100644
--- a/changelog.d/12115.bugfix
+++ b/changelog.d/12115.bugfix
@@ -1,2 +1 @@
-Fix bug: Server notices user profile (display_name/avatar_url) are not updated when changed after server restart.
-Contributed by Jorge Florian.
+Fix a long-standing bug that updating the server notices user profile (display name/avatar URL) in the configuration would not be applied to pre-existing rooms. Contributed by Jorge Florian.
diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py
index 76a0272961b7..015dd08f05e4 100644
--- a/synapse/server_notices/resource_limits_server_notices.py
+++ b/synapse/server_notices/resource_limits_server_notices.py
@@ -71,10 +71,7 @@ async def maybe_send_server_notice_to_user(self, user_id: str) -> None:
             # In practice, not sure we can ever get here
             return
 
-        (
-            room_id,
-            _,
-        ) = await self._server_notices_manager.get_or_create_notice_room_for_user(
+        room_id = await self._server_notices_manager.get_or_create_notice_room_for_user(
             user_id
         )
 
diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index 32e8793854f0..bbac76efb454 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Optional, Tuple
+from typing import TYPE_CHECKING, Optional
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
 from synapse.events import EventBase
@@ -65,7 +65,7 @@ async def send_notice(
             is_state_event: Is the event a state event
             txn_id: The transaction ID.
         """
-        (room_id, room_created) = await self.get_or_create_notice_room_for_user(user_id)
+        room_id = await self.get_or_create_notice_room_for_user(user_id)
         await self.maybe_invite_user_to_room(user_id, room_id)
 
         assert self.server_notices_mxid is not None
@@ -73,14 +73,6 @@ async def send_notice(
             self.server_notices_mxid, authenticated_entity=self._server_name
         )
 
-        if not room_created:
-            await self._update_notice_user_profile_if_changed(
-                requester,
-                room_id,
-                self._config.servernotices.server_notices_mxid_display_name,
-                self._config.servernotices.server_notices_mxid_avatar_url,
-            )
-
         logger.info("Sending server notice to %s", user_id)
 
         event_dict = {
@@ -99,9 +91,7 @@ async def send_notice(
         return event
 
     @cached()
-    async def get_or_create_notice_room_for_user(
-        self, user_id: str
-    ) -> Tuple[str, bool]:
+    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
@@ -111,13 +101,17 @@ async def get_or_create_notice_room_for_user(
             user_id: complete user id for the user we want a room for
 
         Returns:
-            room id of notice room and flag indicating weather room was created.
+            room id of notice room.
         """
         if self.server_notices_mxid is None:
             raise Exception("Server notices not enabled")
 
         assert self._is_mine_id(user_id), "Cannot send server notices to remote users"
 
+        requester = create_requester(
+            self.server_notices_mxid, authenticated_entity=self._server_name
+        )
+
         rooms = await self._store.get_rooms_for_local_user_where_membership_is(
             user_id, [Membership.INVITE, Membership.JOIN]
         )
@@ -136,7 +130,13 @@ async def get_or_create_notice_room_for_user(
                     room.room_id,
                     user_id,
                 )
-                return room.room_id, False
+                await self._update_notice_user_profile_if_changed(
+                    requester,
+                    room.room_id,
+                    self._config.servernotices.server_notices_mxid_display_name,
+                    self._config.servernotices.server_notices_mxid_avatar_url,
+                )
+                return room.room_id
 
         # apparently no existing notice room: create a new one
         logger.info("Creating server notices room for %s", user_id)
@@ -154,9 +154,6 @@ async def get_or_create_notice_room_for_user(
                 "avatar_url": self._config.servernotices.server_notices_mxid_avatar_url,
             }
 
-        requester = create_requester(
-            self.server_notices_mxid, authenticated_entity=self._server_name
-        )
         info, _ = await self._room_creation_handler.create_room(
             requester,
             config={
@@ -175,7 +172,7 @@ async def get_or_create_notice_room_for_user(
         self._notifier.on_new_event("account_data_key", max_id, users=[user_id])
 
         logger.info("Created server notices room %s for %s", room_id, user_id)
-        return room_id, True
+        return room_id
 
     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
diff --git a/tests/rest/admin/test_server_notice.py b/tests/rest/admin/test_server_notice.py
index 2c855bff99c7..60b97e8122a7 100644
--- a/tests/rest/admin/test_server_notice.py
+++ b/tests/rest/admin/test_server_notice.py
@@ -91,7 +91,14 @@ def test_user_does_not_exist(self) -> None:
         self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, msg=channel.json_body)
         self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"])
 
-    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "new display name",
+            }
+        }
+    )
     def test_user_is_not_local(self) -> None:
         """
         Tests that a lookup for a user that is not a local returns a HTTPStatus.BAD_REQUEST
@@ -242,7 +249,14 @@ def test_send_server_notice(self) -> None:
         self.assertEqual(messages[1]["content"]["body"], "test msg two")
         self.assertEqual(messages[1]["sender"], "@notices:test")
 
-    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "display name",
+            }
+        }
+    )
     def test_send_server_notice_leave_room(self) -> None:
         """
         Tests that sending a server notices is successfully.
@@ -415,6 +429,110 @@ def test_send_server_notice_delete_room(self) -> None:
         # second room has new ID
         self.assertNotEqual(first_room_id, second_room_id)
 
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "display name",
+                "system_mxid_avatar_url": "test/url",
+            }
+        }
+    )
+    def test_update_notice_user_name_when_changed(self) -> None:
+        """
+        Tests that existing server notices user name in room is updated after
+        server notice config changes.
+        """
+        server_notice_request_content = {
+            "user_id": self.other_user,
+            "content": {"msgtype": "m.text", "body": "test msg one"},
+        }
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        # simulates a change in server config after a server restart.
+        new_display_name = "new display name"
+        self.server_notices_manager._config.servernotices.server_notices_mxid_display_name = (
+            new_display_name
+        )
+        self.server_notices_manager.get_or_create_notice_room_for_user.cache.invalidate_all()
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        invited_rooms = self._check_invite_and_join_status(self.other_user, 1, 0)
+        notice_room_id = invited_rooms[0].room_id
+        self.helper.join(
+            room=notice_room_id, user=self.other_user, tok=self.other_user_token
+        )
+
+        member_states = self._sync_and_get_member_state(
+            notice_room_id, self.other_user_token, "@notices:test"
+        )
+
+        self.assertEqual(member_states[0]["content"]["displayname"], new_display_name)
+
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "display name",
+                "system_mxid_avatar_url": "test/url",
+            }
+        }
+    )
+    def test_update_notice_user_avatar_when_changed(self) -> None:
+        """
+        Tests that existing server notices user avatar in room is updated when is
+        different from the one in homeserver config.
+        """
+        server_notice_request_content = {
+            "user_id": self.other_user,
+            "content": {"msgtype": "m.text", "body": "test msg one"},
+        }
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        # simulates a change in server config after a server restart.
+        new_avatar_url = "test/new-url"
+        self.server_notices_manager._config.servernotices.server_notices_mxid_avatar_url = (
+            new_avatar_url
+        )
+        self.server_notices_manager.get_or_create_notice_room_for_user.cache.invalidate_all()
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        invited_rooms = self._check_invite_and_join_status(self.other_user, 1, 0)
+        notice_room_id = invited_rooms[0].room_id
+        self.helper.join(
+            room=notice_room_id, user=self.other_user, tok=self.other_user_token
+        )
+
+        member_states = self._sync_and_get_member_state(
+            notice_room_id, self.other_user_token, "@notices:test"
+        )
+
+        self.assertEqual(member_states[0]["content"]["avatar_url"], new_avatar_url)
+
     def _check_invite_and_join_status(
         self, user_id: str, expected_invites: int, expected_memberships: int
     ) -> List[RoomsForUser]:
@@ -460,3 +578,18 @@ def _sync_and_get_messages(self, room_id: str, token: str) -> List[JsonDict]:
             x for x in room["timeline"]["events"] if x["type"] == "m.room.message"
         ]
         return messages
+
+    def _sync_and_get_member_state(
+        self, room_id: str, token: str, member_id: str
+    ) -> List[JsonDict]:
+        channel = self.make_request(
+            "GET", "/_matrix/client/r0/sync", access_token=token
+        )
+        self.assertEqual(channel.code, HTTPStatus.OK)
+
+        room = channel.json_body["rooms"]["join"][room_id]
+        return [
+            x
+            for x in room["timeline"]["events"]
+            if x["type"] == "m.room.member" and x["state_key"] == member_id
+        ]
diff --git a/tests/server_notices/test_server_notices_manager.py b/tests/server_notices/test_server_notices_manager.py
deleted file mode 100644
index 5c209ce537b2..000000000000
--- a/tests/server_notices/test_server_notices_manager.py
+++ /dev/null
@@ -1,197 +0,0 @@
-# Copyright 2018, 2019 New Vector Ltd
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# 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 cast
-from unittest.mock import Mock
-
-from twisted.internet import defer
-
-from synapse.api.constants import EventTypes, Membership
-from synapse.api.room_versions import RoomVersions
-from synapse.events import FrozenEvent
-from synapse.server_notices.server_notices_manager import ServerNoticesManager
-from synapse.storage.roommember import RoomsForUser
-
-from tests import unittest
-from tests.unittest import override_config
-from tests.utils import default_config
-
-server_notice_config = {
-    "system_mxid_localpart": "server",
-    "system_mxid_display_name": "display name",
-    "system_mxid_avatar_url": "test/url",
-    "room_name": "Server Notices",
-}
-
-
-class TestServiceNoticeManager(unittest.HomeserverTestCase):
-    def default_config(self):
-        config = cast(dict, default_config("test"))
-        config.update({"server_notices": server_notice_config})
-
-        # apply any additional config which was specified via the override_config
-        # decorator.
-        if self._extra_config is not None:
-            config.update(self._extra_config)
-
-        return config
-
-    def prepare(self, reactor, clock, hs):
-        self.server_notices_manager = self.hs.get_server_notices_manager()
-
-        self.server_notices_manager._event_creation_handler.create_and_send_nonmember_event = Mock(
-            return_value=defer.succeed(
-                (FrozenEvent({"event_id": "$notice_event123"}, RoomVersions.V1), 0)
-            )
-        )
-
-        self.server_notices_manager._room_member_handler.update_membership = Mock(
-            return_value=defer.succeed(("$updated_membership123", 1))
-        )
-
-        self.user_id = "@user_id:test"
-        self.notice_message = {"content": "a new message from server", type: "m.text"}
-
-    @override_config(
-        {
-            "server_notices": {
-                **server_notice_config,
-                "system_mxid_display_name": "new display name",
-            }
-        }
-    )
-    def test_update_notice_user_name_when_changed(self):
-        """
-        Tests that existing server notices user name in room is updated when is
-        different from the one in homeserver config.
-        """
-        self._mock_existing_notice_room(
-            self.server_notices_manager, server_notice_config
-        )
-
-        self.get_success(
-            self.server_notices_manager.send_notice(
-                self.user_id, self.notice_message, EventTypes.Message
-            )
-        )
-
-        self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
-        (
-            _,
-            kwargs,
-        ) = self.server_notices_manager._room_member_handler.update_membership.call_args
-
-        assert kwargs["content"] == {
-            "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
-            "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
-        }
-
-    @override_config(
-        {
-            "server_notices": {
-                **server_notice_config,
-                "system_mxid_avatar_url": "test/new-url",
-            }
-        }
-    )
-    def test_update_notice_user_avatar_when_changed(self):
-        """
-        Tests that existing server notices user avatar in room is updated when is
-        different from the one in homeserver config.
-        """
-        self._mock_existing_notice_room(
-            self.server_notices_manager, server_notice_config
-        )
-
-        self.get_success(
-            self.server_notices_manager.send_notice(
-                self.user_id, self.notice_message, EventTypes.Message
-            )
-        )
-
-        self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
-        (
-            _,
-            kwargs,
-        ) = self.server_notices_manager._room_member_handler.update_membership.call_args
-
-        assert kwargs["content"] == {
-            "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
-            "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
-        }
-
-    def test_doesnt_update_notice_user_profile_when_not_changed(self):
-        """
-        Tests that existing server notices profile in room is not updated when is
-        equal to the one in homeserver config.
-        """
-        self._mock_existing_notice_room(
-            self.server_notices_manager, server_notice_config
-        )
-
-        self.get_success(
-            self.server_notices_manager.send_notice(
-                self.user_id, self.notice_message, EventTypes.Message
-            )
-        )
-
-        self.server_notices_manager._room_member_handler.update_membership.assert_not_called()
-
-    def _mock_existing_notice_room(
-        self, server_notices_manager: ServerNoticesManager, server_notice_config: dict
-    ):
-        """
-        Mocks ServerNoticesManager dependencies used for reading info
-        about existing server notice room.
-        """
-        # Ignored type error: Cannot assign to a method
-        #  https://github.com/python/mypy/issues/2427
-        server_notices_manager._store.get_rooms_for_local_user_where_membership_is = Mock(  # type: ignore
-            return_value=defer.succeed(
-                [
-                    RoomsForUser(
-                        room_id="!something:test",
-                        event_id="$abc123",
-                        membership=Membership.JOIN,
-                        sender="@server:test",
-                        room_version_id="1",
-                        stream_ordering=0,
-                    )
-                ]
-            )
-        )
-
-        # Ignored type error: Cannot assign to a method
-        #  https://github.com/python/mypy/issues/2427
-        server_notices_manager._store.get_users_in_room = Mock(  # type: ignore
-            return_value=defer.succeed([server_notices_manager.server_notices_mxid])
-        )
-
-        notice_user_profile_in_room = {
-            "displayname": server_notice_config["system_mxid_display_name"],
-            "avatar_url": server_notice_config["system_mxid_avatar_url"],
-        }
-        # Ignored type error: Cannot assign to a method
-        #  https://github.com/python/mypy/issues/2427
-        server_notices_manager._message_handler.get_room_data = Mock(  # type: ignore
-            return_value=defer.succeed(
-                FrozenEvent(
-                    {
-                        "event_id": "$notice_user_state_123",
-                        "content": notice_user_profile_in_room,
-                    },
-                    RoomVersions.V1,
-                )
-            )
-        )

From d423e75ea77e4926500b382259d79006ac39c250 Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Thu, 17 Mar 2022 23:40:37 +0100
Subject: [PATCH 6/8] fix tests

---
 tests/server_notices/test_resource_limits_server_notices.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index f522234516f5..02b96c9e6ecf 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -75,7 +75,7 @@ def prepare(self, reactor, clock, hs):
         self.user_id = "@user_id:test"
 
         self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock(
-            return_value=defer.succeed(("!something:localhost", False))
+            return_value=defer.succeed("!something:localhost")
         )
         self._rlsn._store.add_tag_to_room = Mock(return_value=defer.succeed(None))
         self._rlsn._store.get_tags_for_room = Mock(return_value=make_awaitable({}))
@@ -270,7 +270,7 @@ def test_server_notice_only_sent_once(self):
 
         # Now lets get the last load of messages in the service notice room and
         # check that there is only one server notice
-        (room_id, _) = self.get_success(
+        room_id = self.get_success(
             self.server_notices_manager.get_or_create_notice_room_for_user(self.user_id)
         )
 

From 48cab46c20e86944d1a002f18b26378785f2de7e Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Thu, 7 Apr 2022 17:27:57 +0200
Subject: [PATCH 7/8] address review comments

Signed-off-by: Jorge Florian <jafn28@gmail.com>
---
 .../server_notices/server_notices_manager.py  |  4 +-
 tests/rest/admin/test_server_notice.py        | 53 ++++++-------------
 2 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index bbac76efb454..48eae5fa062a 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -211,7 +211,7 @@ async def _update_notice_user_profile_if_changed(
         avatar_url: Optional[str],
     ) -> None:
         """
-        Updates notice user profile if it's different from what it's saved in the room.
+        Updates the notice user's profile if it's different from what is in the room.
 
         Args:
             requester: The user who is performing the update.
@@ -219,7 +219,7 @@ async def _update_notice_user_profile_if_changed(
             display_name: The displayname of the server notice user
             avatar_url: The avatar url of the server notice user
         """
-        logger.info("Checking whether notice user profile has changed", room_id)
+        logger.debug("Checking whether notice user profile has changed for %s", room_id)
 
         assert self.server_notices_mxid is not None
 
diff --git a/tests/rest/admin/test_server_notice.py b/tests/rest/admin/test_server_notice.py
index 60b97e8122a7..3f3e4b944605 100644
--- a/tests/rest/admin/test_server_notice.py
+++ b/tests/rest/admin/test_server_notice.py
@@ -91,14 +91,7 @@ def test_user_does_not_exist(self) -> None:
         self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, msg=channel.json_body)
         self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"])
 
-    @override_config(
-        {
-            "server_notices": {
-                "system_mxid_localpart": "notices",
-                "system_mxid_display_name": "new display name",
-            }
-        }
-    )
+    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
     def test_user_is_not_local(self) -> None:
         """
         Tests that a lookup for a user that is not a local returns a HTTPStatus.BAD_REQUEST
@@ -253,7 +246,6 @@ def test_send_server_notice(self) -> None:
         {
             "server_notices": {
                 "system_mxid_localpart": "notices",
-                "system_mxid_display_name": "display name",
             }
         }
     )
@@ -433,8 +425,6 @@ def test_send_server_notice_delete_room(self) -> None:
         {
             "server_notices": {
                 "system_mxid_localpart": "notices",
-                "system_mxid_display_name": "display name",
-                "system_mxid_avatar_url": "test/url",
             }
         }
     )
@@ -455,7 +445,7 @@ def test_update_notice_user_name_when_changed(self) -> None:
             content=server_notice_request_content,
         )
 
-        # simulates a change in server config after a server restart.
+        # simulate a change in server config after a server restart.
         new_display_name = "new display name"
         self.server_notices_manager._config.servernotices.server_notices_mxid_display_name = (
             new_display_name
@@ -475,18 +465,18 @@ def test_update_notice_user_name_when_changed(self) -> None:
             room=notice_room_id, user=self.other_user, tok=self.other_user_token
         )
 
-        member_states = self._sync_and_get_member_state(
-            notice_room_id, self.other_user_token, "@notices:test"
+        notice_user_state_in_room = self.helper.get_state(
+            notice_room_id,
+            "m.room.member",
+            self.other_user_token,
+            state_key="@notices:test",
         )
-
-        self.assertEqual(member_states[0]["content"]["displayname"], new_display_name)
+        self.assertEqual(notice_user_state_in_room["displayname"], new_display_name)
 
     @override_config(
         {
             "server_notices": {
                 "system_mxid_localpart": "notices",
-                "system_mxid_display_name": "display name",
-                "system_mxid_avatar_url": "test/url",
             }
         }
     )
@@ -507,7 +497,7 @@ def test_update_notice_user_avatar_when_changed(self) -> None:
             content=server_notice_request_content,
         )
 
-        # simulates a change in server config after a server restart.
+        # simulate a change in server config after a server restart.
         new_avatar_url = "test/new-url"
         self.server_notices_manager._config.servernotices.server_notices_mxid_avatar_url = (
             new_avatar_url
@@ -527,11 +517,13 @@ def test_update_notice_user_avatar_when_changed(self) -> None:
             room=notice_room_id, user=self.other_user, tok=self.other_user_token
         )
 
-        member_states = self._sync_and_get_member_state(
-            notice_room_id, self.other_user_token, "@notices:test"
+        notice_user_state = self.helper.get_state(
+            notice_room_id,
+            "m.room.member",
+            self.other_user_token,
+            state_key="@notices:test",
         )
-
-        self.assertEqual(member_states[0]["content"]["avatar_url"], new_avatar_url)
+        self.assertEqual(notice_user_state["avatar_url"], new_avatar_url)
 
     def _check_invite_and_join_status(
         self, user_id: str, expected_invites: int, expected_memberships: int
@@ -578,18 +570,3 @@ def _sync_and_get_messages(self, room_id: str, token: str) -> List[JsonDict]:
             x for x in room["timeline"]["events"] if x["type"] == "m.room.message"
         ]
         return messages
-
-    def _sync_and_get_member_state(
-        self, room_id: str, token: str, member_id: str
-    ) -> List[JsonDict]:
-        channel = self.make_request(
-            "GET", "/_matrix/client/r0/sync", access_token=token
-        )
-        self.assertEqual(channel.code, HTTPStatus.OK)
-
-        room = channel.json_body["rooms"]["join"][room_id]
-        return [
-            x
-            for x in room["timeline"]["events"]
-            if x["type"] == "m.room.member" and x["state_key"] == member_id
-        ]

From 910b858244faf211773cdbc71c9a6676b6ebea2c Mon Sep 17 00:00:00 2001
From: Jorge Florian <jafn28@gmail.com>
Date: Thu, 7 Apr 2022 21:51:00 +0200
Subject: [PATCH 8/8] format test_server_notice.py file

Signed-off-by: Jorge Florian <jafn28@gmail.com>
---
 tests/rest/admin/test_server_notice.py | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/tests/rest/admin/test_server_notice.py b/tests/rest/admin/test_server_notice.py
index 2f2ac63ae85c..dbcba2663c5a 100644
--- a/tests/rest/admin/test_server_notice.py
+++ b/tests/rest/admin/test_server_notice.py
@@ -240,13 +240,7 @@ def test_send_server_notice(self) -> None:
         self.assertEqual(messages[1]["content"]["body"], "test msg two")
         self.assertEqual(messages[1]["sender"], "@notices:test")
 
-    @override_config(
-        {
-            "server_notices": {
-                "system_mxid_localpart": "notices",
-            }
-        }
-    )
+    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
     def test_send_server_notice_leave_room(self) -> None:
         """
         Tests that sending a server notices is successfully.
@@ -415,13 +409,7 @@ def test_send_server_notice_delete_room(self) -> None:
         # second room has new ID
         self.assertNotEqual(first_room_id, second_room_id)
 
-    @override_config(
-        {
-            "server_notices": {
-                "system_mxid_localpart": "notices",
-            }
-        }
-    )
+    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
     def test_update_notice_user_name_when_changed(self) -> None:
         """
         Tests that existing server notices user name in room is updated after
@@ -467,13 +455,7 @@ def test_update_notice_user_name_when_changed(self) -> None:
         )
         self.assertEqual(notice_user_state_in_room["displayname"], new_display_name)
 
-    @override_config(
-        {
-            "server_notices": {
-                "system_mxid_localpart": "notices",
-            }
-        }
-    )
+    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
     def test_update_notice_user_avatar_when_changed(self) -> None:
         """
         Tests that existing server notices user avatar in room is updated when is