Nothing Special   »   [go: up one dir, main page]

Skip to content
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

Multiple removal of testcases #785

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kgolezardi
Copy link
Contributor
@kgolezardi kgolezardi commented Jul 17, 2017

Admin can select multiple testcases and delete them all at once


This change is Reviewable

@lw
Copy link
Member
lw commented Aug 6, 2017

I think this is a change we want, but I have a few comments.

For a series of reasons I think having a server-side DELETE handler that deletes multiple objects is weird. I would prefer if you kept it as it was and just had the client-side JS code call the handler multiple time. This would have a performance impact but given the rarity of the operation this shouldn't be a big issue.

Also, in general, if one commit of your PR fixes a previous commit it's common practice to rebase the PR and squash them together. It makes the PR easier to review.

@akmohtashami
Copy link
Contributor

If I am not mistaken, calling a delete handler multiple times will not only have performance impact but it also will break atomicity. AFAIK, it is not unusual to have such handler which supports multiple deletes. Could you please share some of your reasons not to have such handler as well?

@lw
Copy link
Member
lw commented Aug 6, 2017

I would expect a properly designed HTTP API to have every path represent an entity (or a collection of entities) and to react to a DELETE on a path by removing that entity (or collection), meaning that a subsequent GET on it would give a 404. This wouldn't be the case here.

I have additional concerns about that specific code. First, it's requiring the use of a query parameter, which should generally be intended as optional. Also it uses JSON-encoding when there is no need for it (a comma-separated list or even just repeating the argument would be more idiomatic).

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

Successfully merging this pull request may close these issues.

3 participants