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

Room avatars are broken on email notifications #1546

Closed
matrixbot opened this issue May 13, 2016 · 5 comments · Fixed by #10658
Closed

Room avatars are broken on email notifications #1546

matrixbot opened this issue May 13, 2016 · 5 comments · Fixed by #10658
Assignees
Labels
A-Email-Push Email notifications S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@matrixbot
Copy link
Member

matrixbot commented May 13, 2016

Submitted by @​matthew:matrix.org

(Imported from https://matrix.org/jira/browse/SYN-695)

Relevant code

@matrixbot matrixbot changed the title room avatars are broken on email notifs (SYN-695) room avatars are broken on email notifs (https://github.com/matrix-org/synapse/issues/1546) Nov 7, 2016
@matrixbot matrixbot changed the title room avatars are broken on email notifs (https://github.com/matrix-org/synapse/issues/1546) room avatars are broken on email notifs (SYN-695) Nov 7, 2016
@MadLittleMods MadLittleMods added the A-Email-Push Email notifications label Aug 17, 2021
@MadLittleMods
Copy link
Contributor

MadLittleMods commented Aug 17, 2021

Unsure in which exact way they were broken before but this looks to still be the case and still reproducible going back to "Riot notifications".

The room avatars are just colored circles. The user avatars work just fine although I have blurred them out in the image.

From the relevant code, synapse/res/templates/room.html#L4-L14, it appears like room.avatar_url isn't being serialized in or passed in to be used in the template.

@MadLittleMods MadLittleMods added T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. S-Tolerable Minor significance, cosmetic issues, low or no impact to users. labels Aug 17, 2021
@MadLittleMods MadLittleMods changed the title room avatars are broken on email notifs (SYN-695) Room avatars are broken on email notifications Aug 17, 2021
@DMRobertson DMRobertson self-assigned this Aug 18, 2021
@DMRobertson
Copy link
Contributor

Took a quick look at this out of curiosity. Looks like we'd need to provide an "avatar_url" key in the dict returned from synapse.push.mailer.Mailer._get_room_vars. Wrote a quick unit test to try and reproduce the error, but I'm having trouble with trial. Will chase down when I next get chance.

@clokep
Copy link
Member

clokep commented Aug 18, 2021

Took a quick look at this out of curiosity. Looks like we'd need to provide an "avatar_url" key in the dict returned from synapse.push.mailer.Mailer._get_room_vars.

That sounds about the right spot to do this! Probably around:

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),
}

We probably just need to check room_state_ids for a m.room.avatar event and pull some info out.

DMRobertson pushed a commit that referenced this issue Aug 19, 2021
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.
@DMRobertson
Copy link
Contributor

Room for improvement here:

  • Groups and users without avatars get a "placeholder" avatar which is just a coloured circle. This doesn't seem to correspond to the colour of the circle seen in element
  • That circle doesn't have the letter like it does in element

Not quite clear on where the boundary between Matrix and Element is here. But it could look nicer.

DMRobertson pushed a commit that referenced this issue Sep 1, 2021
Judging by the template, this was intended ages ago, but we never
actually passed an avatar URL to the template. So let's provide one.

Closes #1546.

Co-authored-by: Patrick Cloke <[email protected]>
@MadLittleMods
Copy link
Contributor

Thanks for fixing this up @DMRobertson 🦓

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Email-Push Email notifications S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants