cms icon indicating copy to clipboard operation
cms copied to clipboard

Multiple removal of testcases

Open kgolezardi opened this issue 8 years ago • 3 comments

Admin can select multiple testcases and delete them all at once


This change is Reviewable

kgolezardi avatar Jul 17 '17 21:07 kgolezardi

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.

lw avatar Aug 06 '17 20:08 lw

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?

akmohtashami avatar Aug 06 '17 20:08 akmohtashami

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).

lw avatar Aug 06 '17 21:08 lw