Bug 23517

Summary: Add API route to update a holds priority
Product: Koha Reporter: Owen Leonard <oleonard>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: new feature    
Priority: P5 - low CC: fridolin.somers, josef.moravec, katrin.fischer, martin.renvoize, sally.healey, tomascohen, wizzyrea
Version: Main   
Hardware: All   
OS: All   
URL: https://wiki.koha-community.org/wiki/Holds_priority_endpoint_RFC
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00
Attachments: Bug 23517: Add a spec for PUT /holds/{hold_id}/priority
Bug 23517: Add the controller method for holds priorities handling
Bug 23517: (follow-up) Tidy tests
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority
Bug 23517: Add the controller method for holds priorities handling
Bug 23517: (follow-up) Tidy tests
Bug 23517: (follow-up) More test cases
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority
Bug 23517: Add the controller method for holds priorities handling
Bug 23517: (follow-up) Tidy tests
Bug 23517: (follow-up) More test cases
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority
Bug 23517: Add the controller method for holds priorities handling
Bug 23517: (follow-up) Tidy tests
Bug 23517: (follow-up) More test cases
Bug 23517: (follow-up) AddReserve expects a priority parameter

Description Owen Leonard 2019-08-28 18:40:07 UTC
When using the staff client, the holds modification page requires "reserveforothers => 'place_holds'" permission. This is the minimum required permission to modify a hold's pickup location, suspended status, or to delete it.

The API requires "reserveforothers" permission (both "place_holds" AND "modify_holds_priority") to delete a hold. This means that operations which a user could perform in the staff client are forbidden by the API.

Add and update operations are also problematic because of the granularity of the sub-permissions:

 - Someone with only "place_holds" permission should be able to add holds and modify every aspect of the hold EXCEPT priority.

 - Someone with only "modify_holds_priority" permission should be able to modify only the priority of a hold (I guess??).
Comment 1 Tomás Cohen Arazi 2019-08-29 15:33:47 UTC
Owen, would you say we should better redefine the holds permissions in a more fine-grained way and then adjust the API to reflect that?

Hm, maybe we can set 'priority' as read-only and change 'reserveforothers' for 'place_holds' for the first problem.

And for the second one, add a new endpoint like PUT /holds/:hold_id/priority
requiring the 'modify_holds_priority'. What do you think?
Comment 2 Owen Leonard 2019-08-30 13:10:43 UTC
I think we can work within the permissions definitions the staff client enforces right now, and I think you've got it right:

 - 'place_holds' allows changes to any hold detail except priority
 - 'modify_holds_priority' allows changes to priority independently of any other detail
Comment 3 Katrin Fischer 2019-09-01 22:10:35 UTC
(In reply to Owen Leonard from comment #2)
> I think we can work within the permissions definitions the staff client
> enforces right now, and I think you've got it right:
> 
>  - 'place_holds' allows changes to any hold detail except priority
>  - 'modify_holds_priority' allows changes to priority independently of any
> other detail

+1
Comment 4 Tomás Cohen Arazi 2019-09-04 17:10:06 UTC
Created attachment 92588 [details] [review]
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority

This patch adds the OpenAPI spec for the endpoint, and tests for the
desired behaviour.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2019-09-04 17:10:09 UTC
Created attachment 92589 [details] [review]
Bug 23517: Add the controller method for holds priorities handling

This patch implements the PUT /holds/:hold_id/priority endpoint
To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2019-09-04 17:10:13 UTC
Created attachment 92590 [details] [review]
Bug 23517: (follow-up) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2019-09-04 17:12:11 UTC
My implementation adds the:
PUT /holds/{hold_id}/priority endpoint for setting the priority of a hold. It uses the same approach rotas endpoint does. And it checks for the very specific permissions.
Comment 8 Owen Leonard 2019-09-11 15:24:48 UTC
Created attachment 92710 [details] [review]
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority

This patch adds the OpenAPI spec for the endpoint, and tests for the
desired behaviour.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2019-09-11 15:24:53 UTC
Created attachment 92711 [details] [review]
Bug 23517: Add the controller method for holds priorities handling

This patch implements the PUT /holds/:hold_id/priority endpoint
To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Owen Leonard 2019-09-11 15:24:56 UTC
Created attachment 92712 [details] [review]
Bug 23517: (follow-up) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Tomás Cohen Arazi 2019-09-16 13:43:08 UTC
Created attachment 92836 [details] [review]
Bug 23517: (follow-up) More test cases

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Josef Moravec 2019-09-18 04:41:24 UTC
Created attachment 92927 [details] [review]
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority

This patch adds the OpenAPI spec for the endpoint, and tests for the
desired behaviour.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2019-09-18 04:41:28 UTC
Created attachment 92928 [details] [review]
Bug 23517: Add the controller method for holds priorities handling

This patch implements the PUT /holds/:hold_id/priority endpoint
To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2019-09-18 04:41:33 UTC
Created attachment 92929 [details] [review]
Bug 23517: (follow-up) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2019-09-18 04:41:37 UTC
Created attachment 92930 [details] [review]
Bug 23517: (follow-up) More test cases

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Liz Rea 2019-09-19 19:29:18 UTC
Created attachment 92986 [details] [review]
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority

This patch adds the OpenAPI spec for the endpoint, and tests for the
desired behaviour.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 17 Liz Rea 2019-09-19 19:29:29 UTC
Created attachment 92987 [details] [review]
Bug 23517: Add the controller method for holds priorities handling

This patch implements the PUT /holds/:hold_id/priority endpoint
To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 18 Liz Rea 2019-09-19 19:29:34 UTC
Created attachment 92988 [details] [review]
Bug 23517: (follow-up) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 19 Liz Rea 2019-09-19 19:29:38 UTC
Created attachment 92989 [details] [review]
Bug 23517: (follow-up) More test cases

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 20 Katrin Fischer 2019-09-22 10:27:12 UTC
Can we add updating https://wiki.koha-community.org/wiki/REST_api_RFCs to the workflow once this is pushed? :)
Comment 21 Tomás Cohen Arazi 2019-09-22 16:40:12 UTC
(In reply to Katrin Fischer from comment #20)
> Can we add updating https://wiki.koha-community.org/wiki/REST_api_RFCs to
> the workflow once this is pushed? :)

Sure! I try to keep this up to date. I missed the voting meeting, but will add that as well.
Comment 22 Martin Renvoize 2019-09-23 10:55:15 UTC
Nice work!

Pushed to master for 19.11.00
Comment 23 Fridolin Somers 2019-09-24 07:18:50 UTC
I've tried to apply on 19.05.x but UT does not work.
Can anyone confirm ?
Comment 24 Tomás Cohen Arazi 2019-09-24 14:40:12 UTC
Created attachment 93143 [details] [review]
Bug 23517: (follow-up) AddReserve expects a priority parameter

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>