-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
MSC4151: Reporting rooms #1938
MSC4151: Reporting rooms #1938
Conversation
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@@ -5,9 +5,6 @@ Users may encounter content which they find inappropriate and should be | |||
able to report it to the server administrators or room moderators for | |||
review. This module defines a way for users to report content. | |||
|
|||
Content is reported based upon a negative score, where -100 is "most | |||
offensive" and 0 is "inoffensive". | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this here because it only applies to event reporting and is repeated in the corresponding endpoint's documentation.
properties: | ||
reason: | ||
type: string | ||
description: The reason the room is being reported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the event reporting endpoint already has the parameters marked as optional, and MSC2414 was trying to match that endpoint, making them optional here too looks like the right thing to me.
@@ -29,7 +92,7 @@ paths: | |||
will require the homeserver to check whether a user is joined to | |||
the room. To combat this, homeserver implementations should add | |||
a random delay when generating a response. | |||
operationId: reportContent | |||
operationId: reportEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure what consequences changing this has but the old ID felt unsuitable now that there are two endpoints in the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's used to generate bindings, and as such we probably shouldn't change it...
matrix-spec/meta/documentation_style.rst
Lines 113 to 114 in 2cbf606
* ``operationId``: a camelCased unique identifier for this endpoint. This will | |
be used to automatically generate bindings for the endpoint. |
@turt2live curious if you happen to know more about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose if we want to be safe, keeping operationId: reportContent
on the existing endpoint and operationId: reportRoom
on the new one would not be the end of the world. So maybe reverting back would be best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, operationId
usually ends up as the method or class name in generated APIs, and therefore, may (actually will, in the vast majority of cases) break the generated code. However, given that even more fine-grained changes (e.g. adding a field in a request body) that are API-compatible in JSON may be breaking in the generated code (depending on the target language and/or the shape of the API) authors producing auto-generated code have to be careful with any upgrades of the API definitions. In particular, 1.12 has already introduced breaking changes elsewhere from the Quotient perspective. So I wouldn't be too concerned around it, just be cognisant that not everything back-compatible in JSON remains such in a native API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We concluded in #matrix-docs:matrix.org that changing the operation ID is not ideal but acceptable in this case as otherwise it'll be even harder to fix in future.
@@ -62,7 +125,7 @@ paths: | |||
and 0 is inoffensive. | |||
reason: | |||
type: string | |||
description: The reason the content is being reported. May be blank. | |||
description: The reason the content is being reported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed this as proposed in MSC2414 and thought it aligns well enough with the rest of this pull request to sneak in. 😇
@@ -29,7 +92,7 @@ paths: | |||
will require the homeserver to check whether a user is joined to | |||
the room. To combat this, homeserver implementations should add | |||
a random delay when generating a response. | |||
operationId: reportContent | |||
operationId: reportEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's used to generate bindings, and as such we probably shouldn't change it...
matrix-spec/meta/documentation_style.rst
Lines 113 to 114 in 2cbf606
* ``operationId``: a camelCased unique identifier for this endpoint. This will | |
be used to automatically generate bindings for the endpoint. |
@turt2live curious if you happen to know more about this?
properties: | ||
reason: | ||
type: string | ||
description: The reason the room is being reported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the event reporting endpoint already has the parameters marked as optional, and MSC2414 was trying to match that endpoint, making them optional here too looks like the right thing to me.
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
@anoadragon453 is there anything else you'd like me to add or change to help this land? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. Note that if I'm not requested for review and there's no comments happening I won't get notified about a PR or see it in my review queue, so I'll forget about it 🙃
Thanks for the poke. This LGTM now!
See #1938 where they were incorrectly marked as 1.12 instead of 1.13. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
As per matrix-org/matrix-spec-proposals#4151.
The MSC proposed that clients should hide reported rooms. However, there isn't any API for ignoring rooms yet. So I left that part out of the spec as otherwise clients would have to invent a system for doing this across devices.
Pull Request Checklist
Preview: https://pr1938--matrix-spec-previews.netlify.app