Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement MSC4133 to support custom profile fields. #17488

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Jul 25, 2024

Implementation of MSC4133 to support custom profile fields. It is behind an experimental flag and includes tests.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

@clokep clokep marked this pull request as ready for review July 26, 2024 00:00
@clokep clokep requested a review from a team as a code owner July 26, 2024 00:00
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

A few comments, but this is definitely on the right track. Thanks for implementing this!

"displayname": profileinfo.display_name,
"avatar_url": profileinfo.avatar_url,
}
# TODO Should this strip out empty values?
Copy link
Member

Choose a reason for hiding this comment

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

I would think not - that could result in users having fields stay around forever in the DB, yet never visible to users.

synapse/handlers/profile.py Outdated Show resolved Hide resolved
synapse/handlers/profile.py Outdated Show resolved Hide resolved
synapse/handlers/profile.py Outdated Show resolved Hide resolved
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to check whether profile["avatar_url"] is None before attempting to .split on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect I need to split this to a separate PR to make it clear what the change is.

synapse/rest/client/profile.py Show resolved Hide resolved
synapse/storage/databases/main/profile.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/profile.py Outdated Show resolved Hide resolved
tests/rest/client/test_profile.py Outdated Show resolved Hide resolved
clokep and others added 4 commits July 31, 2024 07:25
Co-authored-by: Andrew Morgan <[email protected]>
Co-authored-by: Andrew Morgan <[email protected]>
Co-authored-by: Andrew Morgan <[email protected]>
@clokep
Copy link
Contributor Author

clokep commented Aug 9, 2024

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 clokep changed the title Partially implement MSC4133 to support custom profile fields. Implement MSC4133 to support custom profile fields. Sep 9, 2024
@CLAassistant
Copy link

CLAassistant commented Oct 14, 2024

CLA assistant check
All committers have signed the CLA.

@clokep
Copy link
Contributor Author

clokep commented Nov 7, 2024

I think this is close to ready for another review. There's a few open questions on the MSC that will affect this still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants