Created attachment 144843 [details] [review] Bug 31797: Add DELETE /items/:item_id endpoint This patch adds the mentioned endpoint. The controller relies on Koha::Item->safe_to_delete for checks and uses `safe_delete` as additem.pl does. The required permissions are edit_catalogue. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/items.t => SUCCESS: Tests pass! 3. Play with item deletion using a REST tool like Postman => SUCCESS: All works as expected 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Playing around with ajax(), this seems to work well: $.ajax({ url: '/api/v1/items/307', method: 'DELETE', contentType: 'application/json', success: function(result) { console.log('deleted') }, });
Created attachment 144892 [details] [review] Bug 31797: Add DELETE /items/:item_id endpoint This patch adds the mentioned endpoint. The controller relies on Koha::Item->safe_to_delete for checks and uses `safe_delete` as additem.pl does. The required permissions are edit_catalogue. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/items.t => SUCCESS: Tests pass! 3. Play with item deletion using a REST tool like Postman => SUCCESS: All works as expected 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Code generally looks solid.. but I have 2 minor queries. 1. I believe we decided it was OK sticking to english for error descriptions in the API and that code could easily be used by end users for translations.. though I can't find reference to that conversation right now. 2. I find it a little annoying that we only send the first error instead of an array of all errors that apply. Perhaps we should think about that here?
(In reply to Martin Renvoize from comment #4) > Code generally looks solid.. but I have 2 minor queries. > > 1. I believe we decided it was OK sticking to english for error descriptions > in the API and that code could easily be used by end users for > translations.. though I can't find reference to that conversation right now. When we started the API, we only had a textual description of the error, in the 'error' key. Then bug 28020 introduced error codes. It included the introduction of a markup notation to describe such possible error codes for rendering in api.koha-community.org > 2. I find it a little annoying that we only send the first error instead of > an array of all errors that apply. Perhaps we should think about that here? Yeah. Unfortunately: - The API would require a refactoring to allow that (i.e. all endpoints return one error...) - It would imply refactoring Koha::Item->safe_to_delete as well, which I'm not against (provided we have a switch to avoid performance penalties in things like batch deletes), but that's a major change for a more complete error response. I don't feel it is worth right now.
Created attachment 145743 [details] [review] Bug 31797: Add DELETE /items/:item_id endpoint This patch adds the mentioned endpoint. The controller relies on Koha::Item->safe_to_delete for checks and uses `safe_delete` as additem.pl does. The required permissions are edit_catalogue. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/items.t => SUCCESS: Tests pass! 3. Play with item deletion using a REST tool like Postman => SUCCESS: All works as expected 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Whilst I'd love to see an 'errors' array at some point, we discussed it and decided that was a feature for a v2 api if we want to do it cleanly. As such, Passing QA
Pushed to master for 23.05. Nice work everyone, thanks!