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

Add room version 11 #1604

Merged
merged 20 commits into from
Aug 15, 2023
Merged

Add room version 11 #1604

merged 20 commits into from
Aug 15, 2023

Conversation

clokep
Copy link
Member

@clokep clokep commented Jul 26, 2023

@clokep clokep marked this pull request as ready for review July 26, 2023 14:54
@clokep clokep requested a review from a team as a code owner July 26, 2023 14:54
to the `content` of `m.room.redaction` events in *older* room versions when serving
such events over the Client-Server API.

### Authorization rules
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a copy of the room v10 ones with the references to creator of the m.room.create removed.

I debated about making this a separate file (and the room v10) one so that you can diff them more easily?

content/rooms/v11.md Outdated Show resolved Hide resolved
data/event-schemas/schema/m.room.create.yaml Show resolved Hide resolved
data/event-schemas/schema/m.room.create.yaml Outdated Show resolved Hide resolved
data/event-schemas/schema/m.room.redaction.yaml Outdated Show resolved Hide resolved
data/event-schemas/schema/m.room.redaction.yaml Outdated Show resolved Hide resolved
content/rooms/v11.md Show resolved Hide resolved
content/rooms/v11.md Outdated Show resolved Hide resolved
content/rooms/fragments/v11-redactions.md Outdated Show resolved Hide resolved
content/rooms/fragments/v11-redactions.md Outdated Show resolved Hide resolved
content/rooms/fragments/v11-redactions.md Outdated Show resolved Hide resolved
clokep and others added 4 commits August 8, 2023 12:55
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

generally lgtm - I haven't checked against the MSCs, but assume that's happened in another review (or will happen). This review is primarily for documentation style consistency.

content/rooms/fragments/v11-redactions.md Outdated Show resolved Hide resolved
content/rooms/v11.md Outdated Show resolved Hide resolved
content/rooms/v11.md Show resolved Hide resolved
content/rooms/v11.md Outdated Show resolved Hide resolved
@turt2live turt2live added the release-blocker Blocks the next release from happening label Aug 8, 2023
content/rooms/v11.md Outdated Show resolved Hide resolved
@clokep clokep requested review from richvdh and turt2live August 8, 2023 20:22
@clokep
Copy link
Member Author

clokep commented Aug 8, 2023

I think the remaining bits are if we want to edit the examples here (see #1604 (comment)); if we need to insert the redactions twice (see #1604 (comment)).

From my POV the warning box question (#1604 (comment)) is resolved with keeping it, but shout if there are changes to make.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

It fits the documentation style requirements

Thanks!

@turt2live turt2live mentioned this pull request Aug 8, 2023
13 tasks
data/api/server-server/definitions/pdu_v11.yaml Outdated Show resolved Hide resolved
data/api/server-server/definitions/pdu_v11.yaml Outdated Show resolved Hide resolved
Comment on lines 12 to 21
Clients should no longer depend on the `creator` property in the `content` of
[`m.room.create`](/client-server-api#mroomcreate) events. In all room versions,
clients can rely on `sender` instead to determine a room creator.

Clients should note that the format of [`m.room.redaction`](/client-server-api#mroomredaction)
events has been modified and look for the `redacts` key under `content` instead
of a top-level event property.

Clients should note that the `third_party_invite` key of [`m.room.member`](/client-server-api#mroommember)
events is no longer redacted, *but* will only contain the `signed` key after redaction.
Copy link
Member

Choose a reason for hiding this comment

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

The way this is structured still makes me feel like this is an introduction, and that the real changes are going to be spelt out elsewhere.

Two things would help:

  1. Add an "Event format" subheading.
  2. Delete "Clients should note that".

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to clean this up 951f619.

I'm still not quite happy with the phrasing of the third paragraph (I copied it from the MSC3821). I think it is there because the required display_name field under m.room.member event's third_party_invite won't exist after redaction so clients shouldn't assume it is there.

content/rooms/fragments/v11-redactions.md Outdated Show resolved Hide resolved
data/event-schemas/schema/m.room.create.yaml Show resolved Hide resolved
data/event-schemas/schema/m.room.create.yaml Outdated Show resolved Hide resolved
data/event-schemas/schema/m.room.create.yaml Show resolved Hide resolved
clokep and others added 5 commits August 9, 2023 10:24
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@clokep clokep requested a review from richvdh August 9, 2023 18:59
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise. Thanks!

content/rooms/v11.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocks the next release from happening
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants