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??).
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?
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
(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
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>
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>
Created attachment 92590 [details] [review] Bug 23517: (follow-up) Tidy tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
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>
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>
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>
Created attachment 92836 [details] [review] Bug 23517: (follow-up) More test cases Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
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>
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>
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>
Can we add updating https://wiki.koha-community.org/wiki/REST_api_RFCs to the workflow once this is pushed? :)
(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.
Nice work! Pushed to master for 19.11.00
I've tried to apply on 19.05.x but UT does not work. Can anyone confirm ?
Created attachment 93143 [details] [review] Bug 23517: (follow-up) AddReserve expects a priority parameter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>