Format presence events on the edges instead of reformatting them multiple times #2013
Conversation
synapse/api/filtering.py
Outdated
# account_data has been allowed to have non-dict content, so check type first | ||
if isinstance(content, dict): | ||
sender = content.get("user_id") | ||
if isinstance(event, UserPresenceState): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you comment what's going on here? Just a quick comment to explain why both branches are necessary would be enough.
@@ -979,14 +981,15 @@ def should_notify(old_state, new_state): | |||
return False | |||
|
|||
|
|||
def format_user_presence_state(state, now): | |||
def format_user_presence_state(state, now, include_user_id=True): | |||
"""Convert UserPresenceState to a format that can be sent down to clients | |||
and to other servers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment to explain why the "user_id" is optional? something like
The "user_id" is optional so that this function can be used to format presence
updates for client /sync responses and for federation /send requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(done)
synapse/api/filtering.py
Outdated
sender = event.get("sender", None) | ||
if not sender: | ||
# Presence events have their 'sender' in content.user_id | ||
content = event.get("content") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? It looks like it is presence specific and presence should be handled as UserPresenceState tuples/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does account data have a sender?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember tbh. I think only presence has a content.user_id
though.
a65012c
to
e892457
Compare
Other than updating the comment for #2013 (diff) LGTM |
No description provided.