-
Notifications
You must be signed in to change notification settings - Fork 119
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
Admin API implementation #253
base: master
Are you sure you want to change the base?
Conversation
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.
Cool PR! I took a first look at it and left some thoughts. I'm not done since I still need to read the Synapse bits it is based on, try out the code, and look at the tests.
Other general thoughts:
I think the filename admin_api.py
should be changed to something indicating that it concerns Synapse (or else what to do when it'll come to implement the admin endpoints of another HS?).
Maybe it could even live in a submodule, by putting the file in synapse/admin_api.py
, as it is maybe too specific to live directly in matrix_client
.
Last thing, the documentation you added will not be present in the Sphinx rendering automatically. You have to do something like this c95c67c.
) | ||
""" | ||
def purge_history(self, room_id, event_id): | ||
"""Perform /admin/purge_hostory. |
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.
typo
Admin api part. | ||
Args: | ||
room_id (str): Room_id to purge. | ||
event_id (str or int): Event_id or ts to purge before. |
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.
Why not simply have two optional arguments? It would look clearer to me, and simplify the code.
room_id (str): Room_id to purge. | ||
event_id (str or int): Event_id or ts to purge before. | ||
""" | ||
if isinstance(event_id, basestring): |
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.
Not relevant if you choose to follow the comment above, but it looks like swapping the if/else
blocks here would allow to test for isinstance(event_id, int)
and get rid of the ugly basestring
hack.
else: | ||
content = { | ||
"delete_local_events": True, | ||
"purge_up_to_ts": int(event_id) |
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.
According to the docstring, this is already an int
. I don't think we want to try to catch user misuses, especially in API level methods.
return self._send("POST", "/admin/purge_history/%s" % room_id, content) | ||
|
||
def purge_history_status(self, purge_id): | ||
"""Perform /admin/purge_history_status. |
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 should be a newline after this one (applies to most of the docstrings here).
|
||
def purge_history_status(self, purge_id): | ||
"""Perform /admin/purge_history_status. | ||
Admin api part. |
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 doubt this line is useful, but if you want to keep it it needs a newline after (applies to every docstrings).
return self._send("POST", "/admin/reset_password/%s" % user_id, content) | ||
|
||
def quarantine_media(self, room_id): | ||
"""Quarantine all media in room so that no one can download it via thi 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.
typo
return self._send("POST", "/admin/quarantine_media/%s" % room_id) | ||
|
||
def shutdown_room(self, room_id, new_room_user_id, room_name=False, message=False): | ||
"""Shuts down a room by removing all local users from the room and blocking |
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 first line of a docstring should be a single sentence, ie not split across multiple lines.
This PR is pretty old. Is anyone still working on it or are there any chances that this will be merged soon? |
Admin API implementation in separate module.
Implemented methods:
Based on https://github.com/matrix-org/synapse/tree/master/docs/admin_api docs
And synapse sources: https://github.com/matrix-org/synapse/blob/master/synapse/rest/client/v1/admin.py
Signed-off-by: Pavel Kardash [email protected]