Bug 31797 - Add DELETE endpoint for Items
Summary: Add DELETE endpoint for Items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature (vote)
Assignee: Agustín Moyano
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 31789 31798
  Show dependency treegraph
 
Reported: 2022-10-14 14:38 UTC by Nick Clemens
Modified: 2023-12-28 20:45 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00


Attachments
Bug 31797: Add DELETE /items/:item_id endpoint (8.74 KB, patch)
2022-12-26 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31797: Add DELETE /items/:item_id endpoint (8.78 KB, patch)
2022-12-30 15:41 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 31797: Add DELETE /items/:item_id endpoint (8.85 KB, patch)
2023-01-27 13:32 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-10-14 14:38:02 UTC

    
Comment 1 Tomás Cohen Arazi 2022-12-26 19:05:35 UTC
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>
Comment 2 Lucas Gass 2022-12-30 15:40:19 UTC
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')
    },
});
Comment 3 Lucas Gass 2022-12-30 15:41:05 UTC
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>
Comment 4 Martin Renvoize 2023-01-27 12:42:21 UTC
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?
Comment 5 Tomás Cohen Arazi 2023-01-27 13:15:20 UTC
(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.
Comment 6 Martin Renvoize 2023-01-27 13:32:42 UTC
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>
Comment 7 Martin Renvoize 2023-01-27 13:33:38 UTC
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
Comment 8 Tomás Cohen Arazi 2023-01-30 18:10:46 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!