Bug 23517 - Add /holds/{hold_id}/priority route (PUT)
Summary: Add /holds/{hold_id}/priority route (PUT)
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-28 18:40 UTC by Owen Leonard
Modified: 2019-09-19 19:51 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority (6.49 KB, patch)
2019-09-04 17:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23517: Add the controller method for holds priorities handling (1.72 KB, patch)
2019-09-04 17:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23517: (follow-up) Tidy tests (14.08 KB, patch)
2019-09-04 17:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority (6.56 KB, patch)
2019-09-11 15:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23517: Add the controller method for holds priorities handling (1.81 KB, patch)
2019-09-11 15:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23517: (follow-up) Tidy tests (14.13 KB, patch)
2019-09-11 15:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23517: (follow-up) More test cases (1.41 KB, patch)
2019-09-16 13:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority (6.60 KB, patch)
2019-09-18 04:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23517: Add the controller method for holds priorities handling (1.86 KB, patch)
2019-09-18 04:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23517: (follow-up) Tidy tests (14.19 KB, patch)
2019-09-18 04:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23517: (follow-up) More test cases (1.46 KB, patch)
2019-09-18 04:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23517: Add a spec for PUT /holds/{hold_id}/priority (6.65 KB, patch)
2019-09-19 19:29 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23517: Add the controller method for holds priorities handling (1.91 KB, patch)
2019-09-19 19:29 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23517: (follow-up) Tidy tests (14.23 KB, patch)
2019-09-19 19:29 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23517: (follow-up) More test cases (1.51 KB, patch)
2019-09-19 19:29 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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>