Bug 31797

Summary: Add DELETE endpoint for Items
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: REST APIAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: new feature    
Priority: P5 - low CC: lucas, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
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
Bug Depends on:    
Bug Blocks: 31789, 31798    
Attachments: Bug 31797: Add DELETE /items/:item_id endpoint
Bug 31797: Add DELETE /items/:item_id endpoint
Bug 31797: Add DELETE /items/:item_id endpoint

Description Nick Clemens (kidclamp) 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!