Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement delete_devices API #1993

Merged
merged 3 commits into from
Mar 14, 2017
Merged

Implement delete_devices API #1993

merged 3 commits into from
Mar 14, 2017

Conversation

lukebarnard1
Copy link
Contributor

@lukebarnard1 lukebarnard1 commented Mar 13, 2017

This implements the proposal here https://docs.google.com/document/d/1C-25Gqz3TXy2jIAoeOKxpNtmme0jI4g3yFGqv5GlAAk for deleting multiple devices using a single request.

This implements the proposal here https://docs.google.com/document/d/1C-25Gqz3TXy2jIAoeOKxpNtmme0jI4g3yFGqv5GlAAk for deleting multiple devices at once in a single request.
def on_POST(self, request):
try:
body = servlet.parse_json_object_from_request(request)

Copy link
Member

Choose a reason for hiding this comment

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

Bit of a spurious newline here.

# the same as those that pass an empty dict
body = {}
else:
raise
Copy link
Member

Choose a reason for hiding this comment

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

Generally its best to do raise e, as if you later add a yield foo then you will re-raise the wrong error

defer.returnValue((401, result))

requester = yield self.auth.get_user_by_req(request)
for d_id in body['devices']:
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for the first cut, but might be nice to change delete_device to delete_devices and then pass around the list through everywhere. This has a number of advantages: a) it reduces work as a lot of things are cheaper to do in bulk, b) it makes it easier to make the API either succeed or fail, rather than partially succeed if something fails halfway through

"""
Args:
hs (synapse.server.HomeServer): server
"""
Copy link
Member

Choose a reason for hiding this comment

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

Pointless docstring is pointless

@@ -45,6 +45,52 @@ def on_GET(self, request):
)
defer.returnValue((200, {"devices": devices}))

class DeleteDevicesRestServlet(servlet.RestServlet):
PATTERNS = client_v2_patterns("/delete_devices", releases=[], v2_alpha=False)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a docstring to say what this and what form the API roughly takes, e.g.:

API for bulk deletion of devices. Accepts a json object with a devices key which lists the device_ids to delete. Requires user interactive auth.

Luke Barnard added 2 commits March 13, 2017 16:45
(But this doesn't implement the same for deleting access tokens or e2e keys.

Also respond to code review.
@lukebarnard1
Copy link
Contributor Author

PTAL @erikjohnston

@ara4n
Copy link
Member

ara4n commented Mar 13, 2017

what is the rationale for this ooi? (as I thought element-hq/element-web#2464 was a sufficient fix...)

@erikjohnston
Copy link
Member

@ara4n Riot web cheekily implements bulk deletion of devices by caching the password temporarily, if i understand correctly, so I think it makes sense to have a bulk deletion API. (It also means we only notify about device changes once than for each individual device).

@erikjohnston
Copy link
Member

lgtm

@ara4n
Copy link
Member

ara4n commented Mar 14, 2017

ok... would be helpful if there was a matching riot bug for this, if nothing else to aid prioritisation relative to the other fires on the riot side (and to ensure the clientside of the api actually gets implemented!)

@lukebarnard1
Copy link
Contributor Author

@ara4n it's on my list of todos, and overlaps nicely with https://github.com/vector-im/riot-web/issues/3268 as it will need a similar multi-select table

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants