Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Include room avatars in email notifications #10658

Merged
DMRobertson merged 13 commits into from
Sep 1, 2021
+71 −6
Merged

Include room avatars in email notifications #10658

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
869bc01
Include room avatars in email notifications
Aug 19, 2021
771a6db
Changelog
Aug 19, 2021
dc73a0d
Correct the changelog durrrr
Aug 19, 2021
ee2c75a
Changelog type
Aug 19, 2021
18d00a5
Rooms might not have avatars
Aug 19, 2021
ff8b8a9
Don't assume "url" is a str or present
Aug 19, 2021
d217f19
Update changelog.d/10658.bugfix
Aug 31, 2021
83be2c2
_get_room_avatar docstring
Aug 31, 2021
be418e1
Simplify test body
Aug 31, 2021
cf53466
Don't require html5-parser for testing
Aug 31, 2021
4efff04
type hint fixup
Aug 31, 2021
0d2c15d
Merge branch 'develop' of github.com:matrix-org/synapse into dmr/room…
Aug 31, 2021
a8ca807
Lint
Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 changelog.d/10658.misc → changelog.d/10658.bugfix
View file
Open in desktop
File renamed without changes.
17 changes: 11 additions & 6 deletions synapse/push/mailer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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"]
Copy link
Copy Markdown
Member

@clokep clokep Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is untrusted, so there's no guarantee that this will have a url (or that it will be a string).

I also wonder if it makes sense to use the thumbnail_url instead?

Regardless, we'll need to convert the MXC URL to an HTTP one, I believe.

Copy link
Copy Markdown
Contributor Author

@DMRobertson DMRobertson Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, we'll need to convert the MXC URL to an HTTP one, I believe.

I think this is already done in the jinja template room.html:

                <img alt="" src="{{ room.avatar_url|mxc_to_http(48,48) }}" />

where I'm guessing that pipe will feed the LHS into the mxc_to_http_filter defined in synapse/util/templates.py:85

Copy link
Copy Markdown
Member

@clokep clokep Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah-ha! I forgot we did this in the template!

Copy link
Copy Markdown
Contributor Author

@DMRobertson DMRobertson Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is untrusted, so there's no guarantee that this will have a url (or that it will be a string).

The spec says that "url" is required---but I guess we don't enforce that at present? Can do an isintance(..., str) check on the URL though.

I also wonder if it makes sense to use the thumbnail_url instead?

Spec says that this is "Only present if the thumbnail is unencrypted.". But I'm not sure if the main avatar could be encrypted too?

Copy link
Copy Markdown
Member

@clokep clokep Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is untrusted, so there's no guarantee that this will have a url (or that it will be a string).

The spec says that "url" is required---but I guess we don't enforce that at present? Can do an isintance(..., str) check on the URL though.

In general, event content can be whatever. So I don't think we can assume it is there / the right type. Even if we enforce it, an event could come over federation that has the incorrect shape. (Or it could have been redacted so all event content was removed.)

return None

async def _get_notif_vars(
self,
notif: Dict[str, Any],
Expand Down