This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Admin API for reported events #8217
Admin API for reported events #8217
Changes from 2 commits
11105b7
f3a10fc
66563b1
860bc4b
610bed3
503539e
286c84f
72cdbc1
d60c707
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 may want to do some basic error checking for negative
from
values. SQLite3 won't mind (will treat it as0
, but Postgres will raise an error).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.
The default for
default
isNone
.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.
len(event_reports)
should hopefully never be greater thanlimit
. Perhaps you meantif start + len(event_reports) < total:
?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.
You can find the same code here:
synapse/synapse/rest/admin/users.py
Lines 100 to 102 in 97962ad
If
len(event_reports)
equalslimit
you will get more results on next page.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 see, this was a bit confusing.
==
is really the case we care about here, but we're doing>=
for safety.So... does this mean if there are 100 reports, and we set a limit of 100, we'll get a
next_token
that doesn't actually show any more reports?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.
Oh. There is a bug. You are right. I will add a test.
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 not sure. Should be
next_token
an integer or string?In the user api it is a string:
synapse/synapse/rest/admin/users.py
Line 102 in babc027
In room api an integer:
synapse/synapse/rest/admin/rooms.py
Line 205 in babc027
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.
Generally we prefer pagination tokens to be strings, so that we can encode extra data into them if necessary.
However, considering we're not very likely to do that here, and it would be unfortunate to have one admin API with a string pagination token, and another with an int one: I'd keep it as int for now.
In the future if we need to encode extra data for some reason, we'll just bump the endpoint version.
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.
Could you put this in a
Returns:
block instead belowArgs:
?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.
name?