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.

Sign up for GitHub

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

Jump to bottom

Initial room_type work, MSC3288 implementation #375

Merged
merged 12 commits into from
Sep 3, 2021
Merged

Initial room_type work, MSC3288 implementation #375

merged 12 commits into from
Sep 3, 2021

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jul 20, 2021 •
edited
Loading

Related matrix-org/synapse#10435
Implementation of matrix-org/matrix-spec-proposals#3288

Pull Request Checklist

  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fix a bug that prevented receiving messages from other servers." instead of "Move 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.
  • Pull request includes a sign off

@t3chguy t3chguy mentioned this pull request Jul 20, 2021
Merged
4 tasks
@t3chguy t3chguy changed the title Initial room_type work, MSC pending Initial room_type work, MSC3288 implementation Jul 21, 2021
@t3chguy t3chguy marked this pull request as ready for review July 26, 2021 20:27
@clokep clokep requested a review from a team July 29, 2021 13:16
@clokep clokep self-assigned this Jul 30, 2021
clokep
clokep previously requested changes Jul 30, 2021
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Unfortunately due to the l10n concern I think we need to think through this a bit more.

There might be other ways to solve it, but I gave a suggestion on the comment.

sydent/http/servlets/store_invite_servlet.py Outdated
Comment on lines 129 to 131
substitutions["room_type_name"] = (
"space" if substitutions["room_type"] == "m.space" else "room"
)
Copy link
Member

Choose a reason for hiding this comment

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

The issue with this approach is that it is not localizable -- if someone updates their templates to be in a different language the replacements would still be in English here.

I think the solution to this is to replace our home-grown templates with Jinja2 templates (probably in a separate PR). It may or may not be feasible to continue supporting the current templates as an upgrade path. Alternately we could require updating the templates at the same time.

Then we'll want to rebase this PR and do the logic of whether to include space or room in the template itself.

Copy link
Member

Choose a reason for hiding this comment

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

See #376 for some work towards this.

Copy link
Member

Choose a reason for hiding this comment

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

#376 was merged and I've incorporated the changes from that in there. I've applied the changes only to the Jinja templates, I think it is OK for the legacy templates to only say "room". Does that sound reasonable @t3chguy?

@t3chguy t3chguy mentioned this pull request Aug 10, 2021
Merged
2 tasks
@turt2live turt2live mentioned this pull request Aug 25, 2021
Merged
@clokep clokep requested a review from a team September 2, 2021 11:19
@clokep
Copy link
Member

clokep commented Sep 2, 2021

Could I get a second pair of eyes on this since I've now done a bunch of work to it?

@reivilibre reivilibre self-assigned this Sep 3, 2021
reivilibre
reivilibre approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Is it worth calling out (in the changelog?) that only Jinja2 templates can properly customise themselves for room types / spaces?

sydent/http/servlets/store_invite_servlet.py
Comment on lines +128 to +131
# MSC3288
substitutions["room_type"] = substitutions.pop(
"org.matrix.msc3288.room_type", ""
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing these lines will go away when we move out of unstable

Copy link
Member

Choose a reason for hiding this comment

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

Something like that. We'll probably have Synapse send both for a while and update Sydent to use just room_type? Or fallback to org.matrix.msc3288.room_type. 🤷

clokep
clokep reviewed Sep 3, 2021
View reviewed changes
changelog.d/375.feature Outdated Show resolved Hide resolved
@clokep clokep dismissed their stale review September 3, 2021 11:31

Dismissing my own review

@clokep clokep merged commit e50f5d0 into matrix-org:main Sep 3, 2021
@reivilibre reivilibre mentioned this pull request Sep 10, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clokep clokep clokep left review comments

@reivilibre reivilibre reivilibre approved these changes

Assignees

@clokep clokep

@reivilibre reivilibre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t3chguy @clokep @reivilibre