-
Notifications
You must be signed in to change notification settings - Fork 397
MSC4277: Harmonizing the reporting endpoints #4277
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
base: main
Are you sure you want to change the base?
MSC4277: Harmonizing the reporting endpoints #4277
Conversation
Signed-off-by: Johannes Marbach <[email protected]>
42df948
to
0f9c41e
Compare
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.
Implementation requirements:
- Server
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.
200 regardless of event / room existence: element-hq/synapse#18263
I'm unsure if the random delay / constant time part requires an implementation. The spec already mentions this for the event reporting endpoint but Synapse doesn't appear to implement it and it's not part of element-hq/synapse#18120 either.
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've added the removal of score
into element-hq/synapse#18263. Synapse didn't do anything else than assert the type anyway.
|
||
## Potential issues | ||
|
||
The `reason` field on the room and user reporting endpoints is currently |
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.
Honestly, I find it bizarre that reason is required but may be empty string. I suppose this might not be the MSC to change it, but I'd rather we just made it optional.
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 think I understand the value of requiring it to be filled for spam protection. The combination of required but allowed to be empty strikes me as slightly odd, too, but it's hard to judge without knowing T&S's future plans for the field.
Generally though, I suppose if we later go from required but possibly empty to required and not empty, it'll be just as much a breaking change for clients as if we went from optional to required and not empty.
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.
It's hard to say. Even if the field was required people could just keyboard smash the field. I'm not really going to die on this hill though, I just think we should unify it in one direction.
My feeling for T&S that any report is a useful report, and that tooling is smart enough these days that we can make use of the room_id / event_id to do preliminary investigations even if the reason
context isn't useful.
Rendered
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am a Systems Architect at gematik, Software Engineer at Filament and Unomed, Matrix community member and former Element employee. This proposal was written and published with my gematik hat on.