Hi there, just wondering: should we use holds instead of reserves? Reserves is in the code, but all the gui uses the term holds.

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

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

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.

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.

Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).

I meant bug 9809.

Rebased on top of bug 15126

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.

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?

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

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>

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>

Pushed to master for Koha 16.05, thanks Julian!

There seems to be no requirement for permissions to use these routes?