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

Server notices: add an autojoin setting for the notices room #16699

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Nov 28, 2023

Signed-off-by: Mathieu Velten [email protected]

Pull Request Checklist

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this is against the spec:

Servers should invite the target user rather than automatically join them to the server notice room.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, TIL server notices is specced :)

It's a SHOULD and not a MUST so I think it should be fine to have a config flag defaulting to false for that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's probably fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I don't understand the spec's rationale and I think that auto-joining makes for a better UX.

@clokep clokep requested a review from a team November 28, 2023 16:20
Comment on lines +236 to +247
if self._config.servernotices.server_notices_auto_join:
user_requester = create_requester(
user_id, authenticated_entity=self._server_name
)
await self._room_member_handler.update_membership(
requester=user_requester,
target=user_id_obj,
room_id=room_id,
action="join",
ratelimit=False,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit alarmed by this, because it looks like maybe_invite_user_to_room is a generic function. But I see from its docstring that it is specifically intended for server notices.

I also wondered if having the initial membership be invite might be confusing. But the room's join rules presumably require invites, so I think the invite->join transition is inevitable.

On reflection: LGTM.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks Mathieu, I think this is sensible.

@DMRobertson DMRobertson merged commit 9e7f800 into matrix-org:develop Dec 4, 2023
38 of 40 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants