From 869bc0167868086403a140833275e44fc5d94012 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 19 Aug 2021 12:57:32 +0100 Subject: [PATCH 01/12] Include room avatars in email notifications Judging by the template, this was intended ages ago, but we never actually passed an avatar URL to the template. I'm not sure what the best way to test this is. I've written a unit test of sorts, but I'd like to manually verify it by actually seeing an email. Maybe I should setup my own installation of synapse? Closes #1546. --- setup.py | 2 +- synapse/push/mailer.py | 8 +++++- tests/push/test_email.py | 61 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 64 insertions(+), 7 deletions(-) diff --git a/setup.py b/setup.py index c47856351081..22a3d630019f 100755 --- a/setup.py +++ b/setup.py @@ -119,7 +119,7 @@ def exec_file(path_segments): # Tests assume that all optional dependencies are installed. # # parameterized_class decorator was introduced in parameterized 0.7.0 -CONDITIONAL_REQUIREMENTS["test"] = ["parameterized>=0.7.0"] +CONDITIONAL_REQUIREMENTS["test"] = ["parameterized>=0.7.0", "html5-parser>=0.4.6"] setup( name="matrix-synapse", diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index 941fb238b798..c92f616cc01d 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -258,7 +258,7 @@ async def _fetch_room_state(room_id: str) -> None: # actually sort our so-called rooms_in_order list, most recent room first rooms_in_order.sort(key=lambda r: -(notifs_by_room[r][-1]["received_ts"] or 0)) - rooms = [] + rooms: List[Dict[str, Any]] = [] for r in rooms_in_order: roomvars = await self._get_room_vars( @@ -356,12 +356,18 @@ async def _get_room_vars( room_name = await calculate_room_name(self.store, room_state_ids, user_id) + avatar_event = await self.store.get_event( + room_state_ids[(EventTypes.RoomAvatar, "")] + ) + avatar_url = avatar_event.content.get("url") + room_vars: Dict[str, Any] = { "title": room_name, "hash": string_ordinal_total(room_id), # See sender avatar hash "notifs": [], "invite": is_invite, "link": self._make_room_link(room_id), + "avatar_url": avatar_url, } if not is_invite: diff --git a/tests/push/test_email.py b/tests/push/test_email.py index e0a3342088d4..0419ad7cbfa3 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -11,10 +11,14 @@ # 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. - +import email.message +import json import os +from typing import Dict, List, Sequence, Tuple import attr +import html5_parser +import lxml.etree import pkg_resources from twisted.internet.defer import Deferred @@ -33,6 +37,9 @@ class _User: token = attr.ib() +SendMailCall = Tuple[Sequence, Dict] + + class EmailPusherTests(HomeserverTestCase): servlets = [ @@ -70,9 +77,10 @@ def make_homeserver(self, reactor, clock): hs = self.setup_test_homeserver(config=config) # List[Tuple[Deferred, args, kwargs]] - self.email_attempts = [] + self.email_attempts: List[Tuple[Deferred, Sequence, Dict]] = [] def sendmail(*args, **kwargs): + # This mocks out synapse.reactor.send_email._sendmail. d = Deferred() self.email_attempts.append((d, args, kwargs)) return d @@ -253,6 +261,47 @@ def test_multiple_rooms(self): # We should get emailed about those messages self._check_for_mail() + def test_room_notifications_include_avatar(self): + # Create a room and set its avatar. + room = self.helper.create_room_as(self.user_id, tok=self.access_token) + channel = self.make_request( + "PUT", + f"/rooms/{room}/state/m.room.avatar", + content=json.dumps({"url": "mxc://DUMMY_MEDIA_ID"}).encode(), + access_token=self.access_token, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # Invite two other uses. + for other in self.others: + self.helper.invite( + room=room, src=self.user_id, tok=self.access_token, targ=other.id + ) + self.helper.join(room=room, user=other.id, tok=other.token) + + # The other users send some messages. + # TODO It seems that two messages are required to trigger an email? + self.helper.send(room, body="Alpha", tok=self.others[0].token) + self.helper.send(room, body="Beta", tok=self.others[1].token) + + # We should get emailed about those messages + args, kwargs = self._check_for_mail() + + # That email should contain the room's avatar + msg: bytes = args[5] + # Multipart: plain text, base 64 encoded; html, base 64 encoded + html = ( + email.message_from_bytes(msg) + .get_payload()[1] + .get_payload(decode=True) + .decode() + ) + root: lxml.etree.Element = html5_parser.parse(html) + images = root.xpath('.//td[@class="room_avatar"]//img') + self.assertEqual(len(images), 1) + source = images[0].attrib["src"] + self.assertIn("_matrix/media/v1/thumbnail/DUMMY_MEDIA_ID", source) + def test_empty_room(self): """All users leaving a room shouldn't cause the pusher to break.""" # Create a simple room with two users @@ -305,8 +354,8 @@ def test_encrypted_message(self): # We should get emailed about that message self._check_for_mail() - def _check_for_mail(self): - """Check that the user receives an email notification""" + def _check_for_mail(self) -> SendMailCall: + """Check that synapse sends off exactly one email notification. Return it.""" # Get the stream ordering before it gets sent pushers = self.get_success( @@ -330,8 +379,9 @@ def _check_for_mail(self): # One email was attempted to be sent self.assertEqual(len(self.email_attempts), 1) + deferred, sendmail_args, sendmail_kwargs = self.email_attempts[0] # Make the email succeed - self.email_attempts[0][0].callback(True) + deferred.callback(True) self.pump() # One email was attempted to be sent @@ -347,3 +397,4 @@ def _check_for_mail(self): # Reset the attempts. self.email_attempts = [] + return sendmail_args, sendmail_kwargs From 771a6db911a45c4fce2e128b88bf748e5c887fbd Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 19 Aug 2021 13:08:50 +0100 Subject: [PATCH 02/12] Changelog --- changelog.d/10657.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10657.misc diff --git a/changelog.d/10657.misc b/changelog.d/10657.misc new file mode 100644 index 000000000000..dbc6d6d27135 --- /dev/null +++ b/changelog.d/10657.misc @@ -0,0 +1 @@ +Log exceptions in tests to stderr. \ No newline at end of file From dc73a0d10d49873051aca4ee92114109c5e4bf64 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 19 Aug 2021 13:11:57 +0100 Subject: [PATCH 03/12] Correct the changelog durrrr --- changelog.d/10657.misc | 1 - changelog.d/10658.misc | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/10657.misc create mode 100644 changelog.d/10658.misc diff --git a/changelog.d/10657.misc b/changelog.d/10657.misc deleted file mode 100644 index dbc6d6d27135..000000000000 --- a/changelog.d/10657.misc +++ /dev/null @@ -1 +0,0 @@ -Log exceptions in tests to stderr. \ No newline at end of file diff --git a/changelog.d/10658.misc b/changelog.d/10658.misc new file mode 100644 index 000000000000..2e4e49663a27 --- /dev/null +++ b/changelog.d/10658.misc @@ -0,0 +1 @@ +Include room avatars in email notifications. From ee2c75a8d74be25e5c9686e84419310618486d3f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 19 Aug 2021 13:45:37 +0100 Subject: [PATCH 04/12] Changelog type --- changelog.d/{10658.misc => 10658.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{10658.misc => 10658.bugfix} (100%) diff --git a/changelog.d/10658.misc b/changelog.d/10658.bugfix similarity index 100% rename from changelog.d/10658.misc rename to changelog.d/10658.bugfix From 18d00a57261298785c0351e735da8a5f4924ec16 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 19 Aug 2021 13:46:03 +0100 Subject: [PATCH 05/12] Rooms might not have avatars --- synapse/push/mailer.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index c92f616cc01d..753684739db8 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -356,18 +356,13 @@ async def _get_room_vars( room_name = await calculate_room_name(self.store, room_state_ids, user_id) - avatar_event = await self.store.get_event( - room_state_ids[(EventTypes.RoomAvatar, "")] - ) - avatar_url = avatar_event.content.get("url") - room_vars: Dict[str, Any] = { "title": room_name, "hash": string_ordinal_total(room_id), # See sender avatar hash "notifs": [], "invite": is_invite, "link": self._make_room_link(room_id), - "avatar_url": avatar_url, + "avatar_url": await self._get_room_avatar(room_state_ids), } if not is_invite: @@ -399,6 +394,16 @@ async def _get_room_vars( return room_vars + async def _get_room_avatar( + self, + room_state_ids: StateMap[str], + ) -> Optional[str]: + event_id = room_state_ids.get((EventTypes.RoomAvatar, "")) + if event_id: + ev = await self.store.get_event(event_id) + return ev.content["url"] + return None + async def _get_notif_vars( self, notif: Dict[str, Any], From ff8b8a978fd401cea72d16a39702c73b1fc50e8c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 19 Aug 2021 18:12:03 +0100 Subject: [PATCH 06/12] Don't assume "url" is a str or present --- synapse/push/mailer.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index 753684739db8..9d6707d9de6e 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -401,7 +401,9 @@ async def _get_room_avatar( event_id = room_state_ids.get((EventTypes.RoomAvatar, "")) if event_id: ev = await self.store.get_event(event_id) - return ev.content["url"] + url = ev.content.get("url") + if isinstance(url, str): + return url return None async def _get_notif_vars( From d217f19f5ac37cdfaa555f1b32546cb2336cb5c9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 31 Aug 2021 11:02:46 +0100 Subject: [PATCH 07/12] Update changelog.d/10658.bugfix Co-authored-by: Patrick Cloke --- changelog.d/10658.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10658.bugfix b/changelog.d/10658.bugfix index 2e4e49663a27..a59d4029332c 100644 --- a/changelog.d/10658.bugfix +++ b/changelog.d/10658.bugfix @@ -1 +1 @@ -Include room avatars in email notifications. +Fix a long-standing bug where room avatars were not included in email notifications. From 83be2c2cfb7f3419d61313d7b4d6507ddaab87d9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 31 Aug 2021 11:07:47 +0100 Subject: [PATCH 08/12] _get_room_avatar docstring --- synapse/push/mailer.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index 9d6707d9de6e..b0834720ad9a 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -398,6 +398,15 @@ async def _get_room_avatar( self, room_state_ids: StateMap[str], ) -> Optional[str]: + """ + Retrieve the avatar url for this room---if it exists. + + Args: + room_state_ids: The event IDs of the current room state. + + Returns: + room's avatar url if it's present and a string; otherwise None. + """ event_id = room_state_ids.get((EventTypes.RoomAvatar, "")) if event_id: ev = await self.store.get_event(event_id) From be418e1df2e1a4a37866a28b5e1fee137c49e1ed Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 31 Aug 2021 11:21:03 +0100 Subject: [PATCH 09/12] Simplify test body --- tests/push/test_email.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/push/test_email.py b/tests/push/test_email.py index 0419ad7cbfa3..e5c93104db7f 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -264,13 +264,12 @@ def test_multiple_rooms(self): def test_room_notifications_include_avatar(self): # Create a room and set its avatar. room = self.helper.create_room_as(self.user_id, tok=self.access_token) - channel = self.make_request( - "PUT", - f"/rooms/{room}/state/m.room.avatar", - content=json.dumps({"url": "mxc://DUMMY_MEDIA_ID"}).encode(), - access_token=self.access_token, + self.helper.send_state( + room, + "m.room.avatar", + {"url": "mxc://DUMMY_MEDIA_ID"}, + self.access_token ) - self.assertEqual(200, channel.code, msg=channel.json_body) # Invite two other uses. for other in self.others: @@ -296,11 +295,7 @@ def test_room_notifications_include_avatar(self): .get_payload(decode=True) .decode() ) - root: lxml.etree.Element = html5_parser.parse(html) - images = root.xpath('.//td[@class="room_avatar"]//img') - self.assertEqual(len(images), 1) - source = images[0].attrib["src"] - self.assertIn("_matrix/media/v1/thumbnail/DUMMY_MEDIA_ID", source) + self.assertIn("_matrix/media/v1/thumbnail/DUMMY_MEDIA_ID", html) def test_empty_room(self): """All users leaving a room shouldn't cause the pusher to break.""" From cf53466839af3c854946339c6a0801e2bb772754 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 31 Aug 2021 11:21:40 +0100 Subject: [PATCH 10/12] Don't require html5-parser for testing --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 22a3d630019f..c47856351081 100755 --- a/setup.py +++ b/setup.py @@ -119,7 +119,7 @@ def exec_file(path_segments): # Tests assume that all optional dependencies are installed. # # parameterized_class decorator was introduced in parameterized 0.7.0 -CONDITIONAL_REQUIREMENTS["test"] = ["parameterized>=0.7.0", "html5-parser>=0.4.6"] +CONDITIONAL_REQUIREMENTS["test"] = ["parameterized>=0.7.0"] setup( name="matrix-synapse", From 4efff04930671a2f0eb2be6c4b92eefb14820110 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 31 Aug 2021 11:24:40 +0100 Subject: [PATCH 11/12] type hint fixup --- tests/push/test_email.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/push/test_email.py b/tests/push/test_email.py index e5c93104db7f..87df24e162af 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -37,7 +37,6 @@ class _User: token = attr.ib() -SendMailCall = Tuple[Sequence, Dict] class EmailPusherTests(HomeserverTestCase): @@ -349,8 +348,13 @@ def test_encrypted_message(self): # We should get emailed about that message self._check_for_mail() - def _check_for_mail(self) -> SendMailCall: - """Check that synapse sends off exactly one email notification. Return it.""" + def _check_for_mail(self) -> Tuple[Sequence, Dict]: + """Assert that synapse sent off exactly one email notification. + + Returns: + args and kwargs passed to synapse.reactor.send_email._sendmail for + that notification. + """ # Get the stream ordering before it gets sent pushers = self.get_success( From a8ca8074d248c439c5ad20efc1f7dceb5971c76d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 31 Aug 2021 19:12:29 +0100 Subject: [PATCH 12/12] Lint --- tests/push/test_email.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/tests/push/test_email.py b/tests/push/test_email.py index f89dbb4ad4cb..2bed7302cfc8 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -12,13 +12,10 @@ # See the License for the specific language governing permissions and # limitations under the License. import email.message -import json import os from typing import Dict, List, Sequence, Tuple import attr -import html5_parser -import lxml.etree import pkg_resources from twisted.internet.defer import Deferred @@ -37,8 +34,6 @@ class _User: token = attr.ib() - - class EmailPusherTests(HomeserverTestCase): servlets = [ @@ -266,10 +261,7 @@ def test_room_notifications_include_avatar(self): # Create a room and set its avatar. room = self.helper.create_room_as(self.user_id, tok=self.access_token) self.helper.send_state( - room, - "m.room.avatar", - {"url": "mxc://DUMMY_MEDIA_ID"}, - self.access_token + room, "m.room.avatar", {"url": "mxc://DUMMY_MEDIA_ID"}, self.access_token ) # Invite two other uses.