Bug 13903

Summary: Add API routes to list, create, update, delete holds
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Web servicesAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andreas.hedstrom.mace, arve.soreide, benjamin.rokseth, dcook, ere.maijala, jonathan.druart, jweaver, katrin.fischer, kyle, lari.taskula, olli-antti.kivilahti, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15126    
Bug Blocks: 15566, 15567, 13927, 16213, 16271, 16652, 16815, 17032, 17042, 17086, 25662    
Attachments: Bug 13903: Add API routes to list, create, update, delete reserves
Bug 13903: 6. Add API routes to list, create, update, delete reserves
Bug 13903: 6. Add API routes to list, create, update, delete reserves
Bug 13903: 6. Add API routes to list, create, update, delete reserves
Bug 13903: Add API routes to list, create, update, delete reserves
Bug 13903: Add API routes to list, create, update, delete reserves
Bug 13903: Add API routes to list, create, update, delete reserves
Bug 13903: (QA followup) change routes to /holds
Bug 13903: (QA followup) change routes to /holds

Description Julian Maurice 2015-03-25 11:33:05 UTC
Add API routes to list, create, update, delete reserves
Comment 1 Julian Maurice 2015-03-25 11:34:06 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2015-03-25 17:58:35 UTC
Hi there, just wondering: should we use holds instead of reserves? Reserves is in the code, but all the gui uses the term holds.
Comment 3 Julian Maurice 2015-03-26 09:01:06 UTC
Hi Katrin,

I started this by using "holds", but since the ID field is named "reserve_id" and it's returned in some responses, I think it's more consistent to use "reserves".
Comment 4 Tomás Cohen Arazi 2015-03-30 12:45:59 UTC
(In reply to Katrin Fischer from comment #2)
> Hi there, just wondering: should we use holds instead of reserves? Reserves
> is in the code, but all the gui uses the term holds.

+1 for "holds". And I don't fear refactoring the code a bit to make it more consistent.
Comment 5 Olli-Antti Kivilahti 2015-06-22 12:55:11 UTC
I did some testing and found the following issues:

POST-request

-There should be an exception for non-existent branchcode? So we pass a branchcode as a param which doesn't match any branch we have.
-expirationdate-parameter is mandatory. Eg. it must be given as a parameter with a value of null or something sensible.
-itemnumber is mandatory? How about biblio-level holds?
-C4::Reserves::AddReserves() modification out of scope.
-Repeatedly placing a reserve places multiple holds for the same itemnumber. Etag/Timestamp needed to prevent accepting the same request many times. Also I think this is against Koha business rules.

PUT-request

-Optional fields mandatory. Can be circumvented by using parameters "" and "0000-00-00"

DELETE-request

-Deletes OK, but returns a "page not found"-error.
Comment 6 Olli-Antti Kivilahti 2015-06-24 15:12:43 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2015-07-24 12:12:18 UTC
Tested that the parameter definition "optional" actually works, and they just aren't used.

       "properties": {
         "biblionumber": {
           "description": "Biblio internal identifier",
-          "type": "integer"
+          "type": "integer",
+          "required": true
         },
         "itemnumber": {
           "description": "Item internal identifier",
-          "type": "integer"
+          "type": "integer",
+          "required": true
         },
         "branchcode": {
           "description": "Pickup location",
@@ -413,7 +415,8 @@
         "expirationdate": {
           "description": "Reserve end date",
           "type": "string",
-          "format": "date"
+          "format": "date",
+          "required": false


Using these Swagger2Validator no longer complains about missing parameters and stuff just works !
This resolves the OPTIONAL/MANDATORY-issue.
Comment 8 Olli-Antti Kivilahti 2015-07-24 14:06:51 UTC Comment hidden (obsolete)
Comment 9 Olli-Antti Kivilahti 2015-08-17 12:18:23 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-08-21 10:37:27 UTC
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
Comment 11 Jonathan Druart 2015-08-21 10:40:54 UTC
I meant bug 9809.
Comment 12 Julian Maurice 2015-12-18 15:31:46 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2015-12-18 15:33:36 UTC
Rebased on top of bug 15126
Comment 14 Benjamin Rokseth 2016-04-04 10:10:47 UTC Comment hidden (obsolete)
Comment 15 Benjamin Rokseth 2016-04-04 10:36:05 UTC
Rebased again on top of #15126, since #15126 had to be rebased on top of changes pushed to master.

Tests updated and passing. API routes working as notified.

Two small comments:

- edit reserve requires both priority AND branchcode to update holding branch. This is probably some issue in C4::Reserves::ModReserve rather.
- it would be nice to be able to reserve from barcode as alternative, but I see this needs support in modules first

Setting to signed off as it works according to plan.
Comment 16 Jesse Weaver 2016-04-22 21:56:37 UTC
Testing this, and so far all seems kosher.

I'd like to add a strong +1 to changing the routes (if nothing else) to holds, though; the rest of the existing routes use the GUI names, and we should set a good precedent from the beginning.

I'd be willing to do this myself; all this requires is changing the route names in swagger.json and the test file, correct?
Comment 17 Benjamin Rokseth 2016-04-24 19:03:29 UTC
(In reply to Jesse Weaver from comment #16)
> Testing this, and so far all seems kosher.
> 
> I'd like to add a strong +1 to changing the routes (if nothing else) to
> holds, though; the rest of the existing routes use the GUI names, and we
> should set a good precedent from the beginning.
> 
> I'd be willing to do this myself; all this requires is changing the route
> names in swagger.json and the test file, correct?

Sounds reasonable to me, please go ahead, and please push for this (and dependendt #15126) to get into 3.24! That would be a great step forward for the Rest API.
Comment 18 Jesse Weaver 2016-04-27 22:08:17 UTC
Created attachment 50852 [details] [review]
Bug 13903: Add API routes to list, create, update, delete reserves

GET    /reserves?borrowernumber=X (list)
POST   /reserves                  (create)
PUT    /reserves/{reserve_id}     (update)
DELETE /reserves/{reserve_id}     (delete)

Unit tests in t/db_dependent/api/v1/reserves.t

Test plan:
1. Apply patch
2. Run unit tests
3. Play with the API with your favorite REST client, using documentation
   in the swagger.json file
4. Try to make reserves until the maximum number of reserves for a user
   is reached (you should have a 403 error)

Depends on bug 15126

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 19 Jesse Weaver 2016-04-27 22:08:44 UTC Comment hidden (obsolete)
Comment 20 Jesse Weaver 2016-04-28 22:00:52 UTC
Created attachment 50959 [details] [review]
Bug 13903: (QA followup) change routes to /holds

GET    /holds?borrowernumber=X (list)
POST   /holds                  (create)
PUT    /holds/{reserve_id}     (update)
DELETE /holds/{reserve_id}     (delete)

Unit tests in t/db_dependent/api/v1/holds.t

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 21 Kyle M Hall 2016-05-04 13:54:31 UTC
Pushed to master for Koha 16.05, thanks Julian!
Comment 22 Lari Taskula 2016-06-22 14:59:39 UTC
There seems to be no requirement for permissions to use these routes?