Implement MSC4133 to support custom profile fields. #17488
Conversation
synapse/handlers/profile.py
Outdated
| "displayname": profileinfo.display_name, | ||
| "avatar_url": profileinfo.avatar_url, | ||
| } | ||
| # TODO Should this strip out empty values? |
There was a problem hiding this comment.
I would think not - that could result in users having fields stay around forever in the DB, yet never visible to users.
| @@ -813,7 +813,7 @@ def is_allowed_mime_type(content_type: str) -> bool: | |||
|
|
|||
| # bail if user already has the same avatar | |||
| profile = await self._profile_handler.get_profile(user_id) | |||
| if profile["avatar_url"] is not None: | |||
| if "avatar_url" in profile: | |||
There was a problem hiding this comment.
Do we still need to check whether profile["avatar_url"] is None before attempting to .split on it?
There was a problem hiding this comment.
I suspect I need to split this to a separate PR to make it clear what the change is.
| async def on_PUT( | ||
| self, request: SynapseRequest, user_id: str, field_name: str | ||
| ) -> Tuple[int, JsonDict]: |
There was a problem hiding this comment.
I think this endpoint may currently accept an empty string as a field name?
i.e. if the request path was /_matrix/client/unstable/uk.tcpip.msc4133/profile/@bob:example.com/
Writing a unit test for this could be useful.
There was a problem hiding this comment.
Right, I guess that's invalid since it could be confused with returning the whole profile. The MSC should clarify this too.
There was a problem hiding this comment.
Is this a suitable clarification? matrix-org/matrix-spec-proposals@8ebe1b1
There was a problem hiding this comment.
Yes, though it's quite strongly worded with all those asterisks 😋 But yeah, the meaning is there.
Though if I were being pedantic, I would say "at least one byte", instead of one character.
There was a problem hiding this comment.
As the key is required to be a string, I thought requiring 1 character would be simpler... though now I guess someone's going to use a single space 🤔
There was a problem hiding this comment.
Or an emoji with a single codepoint, but made up of multiple bytes etc. etc. 👨👧👦
There was a problem hiding this comment.
Aye, I guess my concern was that a single byte of 🌎 shouldn't be valid, whereas setting an emoji as a key name should probably be allowed if someone used 📧 for email, for example?
| @@ -173,6 +174,45 @@ async def get_profile_avatar_url(self, user_id: UserID) -> Optional[str]: | |||
| desc="get_profile_avatar_url", | |||
| ) | |||
|
|
|||
| async def get_profile_field( | |||
There was a problem hiding this comment.
It may be worth putting a cache on this method? In case a user posts in a large room and lots of people go to view their profile.
There was a problem hiding this comment.
Would it make more sense to cache get_profile_fields?
synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
|
I pushed some changes to update / flesh out the rest of the MSC, but I haven't yet gone through @anoadragon453's comments so not worth another review yet. (If folks want to test it that would be reasonable...I'm sure it'll break.) |
clokep
changed the title
Implementation of MSC4133 to support custom profile fields. It is behind an experimental flag and includes tests.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)