Bug 18729

Summary: Librarian unable to update hold pickup library from patron pages without "modify_holds_priority" permission
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, george, gmcharlt, jdemuth, jonathan.druart, katrin.fischer, lisettepalouse+koha, martin.renvoize, nick, sally.healey, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11
Bug Depends on: 27117, 27205, 27209    
Bug Blocks: 27201    
Attachments: Bug 18729: Add PUT /holds/{hold_id}/pickup_priority
Bug 18729: Add PUT /holds/{hold_id}/pickup_priority
Bug 18729: Add more tests
Bug 18729: Add PUT /holds/{hold_id}/pickup_priority
Bug 18729: Add more tests
Bug 18729: (follow-up) Adjust API to new spec
Bug 18729: Adapt holds.js
Bug 18729: Add PUT /holds/{hold_id}/pickup_location
Bug 18729: Add more tests
Bug 18729: (follow-up) Adjust API to new spec
Bug 18729: Adapt holds.js
Bug 18729: Add PUT /holds/{hold_id}/pickup_location
Bug 18729: Add more tests
Bug 18729: (follow-up) Adjust API to new spec
Bug 18729: Adapt holds.js
Bug 18729: Add PUT /holds/{hold_id}/pickup_location
Bug 18729: Add more tests
Bug 18729: (follow-up) Adjust API to new spec
Bug 18729: Adapt holds.js

Description Kyle M Hall 2017-06-05 15:08:30 UTC
A librarian can update a hold's pickup library from the record details page, but not from the patron details or checkout pages unless he or she has the "modify_holds_priority". If the librarian does not, an error is displayed instead.

This is do to the fact that on those two pages the API is used and it used the PUT for hold to update all hold fields, which requires this permission. Another endpoint for updating only the pickup location should be added that does not require the "modify_holds_priority" permission.
Comment 1 Tomás Cohen Arazi 2017-06-07 15:10:15 UTC
(In reply to Kyle M Hall from comment #0)
> This is do to the fact that on those two pages the API is used and it used
> the PUT for hold to update all hold fields, which requires this permission.
> Another endpoint for updating only the pickup location should be added that
> does not require the "modify_holds_priority" permission.

Kyle, could you specify what exact permission should be needed?
Comment 2 Kyle M Hall 2017-06-07 18:55:52 UTC
(In reply to Tomás Cohen Arazi from comment #1)
> (In reply to Kyle M Hall from comment #0)
> > This is do to the fact that on those two pages the API is used and it used
> > the PUT for hold to update all hold fields, which requires this permission.
> > Another endpoint for updating only the pickup location should be added that
> > does not require the "modify_holds_priority" permission.
> 
> Kyle, could you specify what exact permission should be needed?

A permission shouldn't be needed, as that is not needed to set the pickup branch on any of the other pages.
Comment 3 Katrin Fischer 2018-05-20 01:29:55 UTC
If the staff user only has 'place_holds' the holds tab in the patron account is broken and gives an error:

DataTables warning: table id=holds-table - Ajax error. For more information about this error, please see http://datatables.net/tn/7

Investigating.
Comment 4 Katrin Fischer 2018-05-20 02:28:37 UTC
(In reply to Katrin Fischer from comment #3)
> If the staff user only has 'place_holds' the holds tab in the patron account
> is broken and gives an error:
> 
> DataTables warning: table id=holds-table - Ajax error. For more information
> about this error, please see http://datatables.net/tn/7
> 
> Investigating.

Forget above, the problem is the hols tab not working if staff only has borrowers, but not circulate_remaining_permissions. I opened a separate bug for this issue: bug 20794
Comment 5 Katrin Fischer 2018-05-20 02:43:47 UTC
Tomas, could you take another look here? I am not sure how to solve this bug.

I think the problem is that we require reserveforothers for updates to holds, but in some cases we only want circulate_remaining_permissions.
Comment 6 Sally 2019-11-04 17:31:03 UTC
This is still valid on master.  It'd be a great addition to Koha if it could be resolved.

We need all of our librarians to be able to make location modifications, but we don't want them to be able to adjust the priority of the holds.  Patrons have a valid reason to say to staff, "I want to pick my hold up from ABC instead XYZ" - but they don't have one to say, "I want to be moved from 174 to 9 in the queue."  

We'd love to be able to take the "modify_holds_priority" feature away from some librarians, but we can't justify losing the location modification feature in the process.
Comment 7 Sally 2019-11-04 17:38:39 UTC
I've just seen:  https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those changes resolve this bug?
Comment 8 Martin Renvoize 2019-12-13 10:08:50 UTC
Tomas, are you still aiming to get to this one, or should we unassign it?
Comment 9 Tomás Cohen Arazi 2019-12-17 02:29:12 UTC
(In reply to Sally from comment #7)
> I've just seen: 
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those
> changes resolve this bug?

That one only deals with priority. Question: what permissions would you require for changing the pickup location? Don't think in terms of the current UI, maybe we can add modify_holds_pickup_location for example.
Comment 10 Sally 2019-12-17 09:54:29 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> (In reply to Sally from comment #7)
> > I've just seen: 
> > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those
> > changes resolve this bug?
> 
> That one only deals with priority. Question: what permissions would you
> require for changing the pickup location? Don't think in terms of the
> current UI, maybe we can add modify_holds_pickup_location for example.

Ah, thanks Tomas - yes, a new modify_holds_pickup_location would work for us.  

The key part for us is separating the permissions between modifying the pickup location and modifying the hold priority, so that suggestion works.
Comment 11 Tomás Cohen Arazi 2019-12-18 13:48:38 UTC
(In reply to Sally from comment #10)
> (In reply to Tomás Cohen Arazi from comment #9)
> > (In reply to Sally from comment #7)
> > > I've just seen: 
> > > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those
> > > changes resolve this bug?
> > 
> > That one only deals with priority. Question: what permissions would you
> > require for changing the pickup location? Don't think in terms of the
> > current UI, maybe we can add modify_holds_pickup_location for example.
> 
> Ah, thanks Tomas - yes, a new modify_holds_pickup_location would work for
> us.  
> 
> The key part for us is separating the permissions between modifying the
> pickup location and modifying the hold priority, so that suggestion works.

Thinking of an upgrade path, would you agree users with modify_holds_priority should inherit the new modify_holds_pickup_location permission?
Comment 12 Katrin Fischer 2019-12-18 22:10:21 UTC
> Thinking of an upgrade path, would you agree users with
> modify_holds_priority should inherit the new modify_holds_pickup_location
> permission?

I think I would even make it simpler and use reserve_for_others or place_holds. As Kyle wrote it's already possible to change the pickup location without the modify_holds_priority permission - just from another page. The API way to do things was added later - so people have probably been doing the other way for a long time with the lower permission. Short: I'd match what the holds page for the record has. And if we add a new permission we should probably change it there as well?

comment#0
>A librarian can update a hold's pickup library from the record details page, >but not from the patron details or checkout pages unless he or she has the >"modify_holds_priority". If the librarian does not, an error is displayed >instead.
Comment 13 Jonathan Druart 2020-05-07 09:28:50 UTC
Tomas, this one seems an important one to have for 20.05
Comment 14 Jonathan Druart 2020-05-07 09:31:18 UTC
(In reply to Jonathan Druart from comment #13)
> Tomas, this one seems an important one to have for 20.05

Hum, I thought it was a regression caused by bug 22284, but looking at the low bug number it should not be related actually.
Comment 15 Jonathan Druart 2020-12-01 11:30:18 UTC
Created attachment 114067 [details] [review]
Bug 18729: Add PUT /holds/{hold_id}/pickup_priority
Comment 16 Jonathan Druart 2020-12-01 11:30:56 UTC
Tomas, can you have a look at this patch please?
I think I am almost there, but the test is failing (500 instead of 200) and the UI is getting a 400.
Comment 17 Jonathan Druart 2020-12-10 15:25:41 UTC
Created attachment 114311 [details] [review]
Bug 18729: Add PUT /holds/{hold_id}/pickup_priority

Bug 18729: Fix return type
Comment 18 Jonathan Druart 2020-12-10 15:25:58 UTC
Thanks Tomas! :)
Comment 19 Tomás Cohen Arazi 2020-12-10 18:43:24 UTC
Hi, I'd like to mention that:

- The idea of having a route to change the pickup location *only*, was meant so we could have a more granular permission set. So I expect this route to require something like "reserveforothers": "modify_pickup_location". That permission should be set on an atomicupdate to all users having place_holds, as per bug 27117.

- I liked the idea of calling them 'pickup locations' instead of the already used 'pickup_library' (see the hold objects definition). I like the fact that we prepare for some future in which there could be other pickup locations (shelves?  special lockers for sanitized materials on this new world?). With this in mind, I have two other comments to make:

- The PUT verb is accepting (and returning) a string, that actually represents what we already call pickup_library_id, but named generically 'pickup location'. I suggest we drop this and in turn expect a JSON object, with a 'pickup_library_id' attribute, replicating the already used terminology. It will also allow us to add new options for the future.

- I regret not proposing this before, but we should really have a class abstracting pickup locations, that could initially just be a wrapper for libraries, but with a ->to_api method that makes it return the pickup_library_id and the name. If we need more things, we can solve that through embedding (e.g. embed the library object altogether). This will allow us to have simpler routes for dropdowns (right now we should take care of 'private' attributes for such dropdowns in the OPAC, for example).

About this last comment, I haven't polished the idea. But the main message is 'do not put any design constraint ourselves at this point'. And I'm willing to have the full discussion when appropriate.
Comment 20 Tomás Cohen Arazi 2020-12-11 12:14:03 UTC
Created attachment 114337 [details] [review]
Bug 18729: Add more tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2020-12-14 15:30:53 UTC
Created attachment 114388 [details] [review]
Bug 18729: Add PUT /holds/{hold_id}/pickup_priority

Bug 18729: Fix return type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2020-12-14 15:30:58 UTC
Created attachment 114389 [details] [review]
Bug 18729: Add more tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2020-12-14 15:31:04 UTC
Created attachment 114390 [details] [review]
Bug 18729: (follow-up) Adjust API to new spec

The previous patch introduces some behavioural changes to the API, as
well as the data types that need to be passed; all happens in the tests.

This patch adapts the route so it complies with those changes:
- JSON object containing pickup_library_id attribute is now passed back
  and forth.
- The controller should take care of checking the pickup location is
  valid, using the available tools.

To test:
1. Apply the patches, up to the tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> ERROR: Tests fail because the controller doesn't implement the
desired behavior
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2020-12-14 15:31:11 UTC
Created attachment 114391 [details] [review]
Bug 18729: Adapt holds.js

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2020-12-14 18:29:34 UTC
Created attachment 114393 [details] [review]
Bug 18729: Add PUT /holds/{hold_id}/pickup_location

This patch adds a route to overwrite the current pickup location.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2020-12-14 18:29:40 UTC
Created attachment 114394 [details] [review]
Bug 18729: Add more tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2020-12-14 18:29:45 UTC
Created attachment 114395 [details] [review]
Bug 18729: (follow-up) Adjust API to new spec

The previous patch introduces some behavioural changes to the API, as
well as the data types that need to be passed; all happens in the tests.

This patch adapts the route so it complies with those changes:
- JSON object containing pickup_library_id attribute is now passed back
  and forth.
- The controller should take care of checking the pickup location is
  valid, using the available tools.

To test:
1. Apply the patches, up to the tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> ERROR: Tests fail because the controller doesn't implement the
desired behavior
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2020-12-14 18:29:50 UTC
Created attachment 114396 [details] [review]
Bug 18729: Adapt holds.js

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 David Nind 2021-03-04 09:46:37 UTC
Created attachment 117656 [details] [review]
Bug 18729: Add PUT /holds/{hold_id}/pickup_location

This patch adds a route to overwrite the current pickup location.

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 30 David Nind 2021-03-04 09:46:41 UTC
Created attachment 117657 [details] [review]
Bug 18729: Add more tests

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 31 David Nind 2021-03-04 09:46:45 UTC
Created attachment 117658 [details] [review]
Bug 18729: (follow-up) Adjust API to new spec

The previous patch introduces some behavioural changes to the API, as
well as the data types that need to be passed; all happens in the tests.

This patch adapts the route so it complies with those changes:
- JSON object containing pickup_library_id attribute is now passed back
  and forth.
- The controller should take care of checking the pickup location is
  valid, using the available tools.

To test:
1. Apply the patches, up to the tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> ERROR: Tests fail because the controller doesn't implement the
desired behavior
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 David Nind 2021-03-04 09:46:49 UTC
Created attachment 117659 [details] [review]
Bug 18729: Adapt holds.js

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 Martin Renvoize 2021-04-16 11:59:04 UTC
Created attachment 119711 [details] [review]
Bug 18729: Add PUT /holds/{hold_id}/pickup_location

This patch adds a route to overwrite the current pickup location.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2021-04-16 11:59:09 UTC
Created attachment 119712 [details] [review]
Bug 18729: Add more tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2021-04-16 11:59:13 UTC
Created attachment 119713 [details] [review]
Bug 18729: (follow-up) Adjust API to new spec

The previous patch introduces some behavioural changes to the API, as
well as the data types that need to be passed; all happens in the tests.

This patch adapts the route so it complies with those changes:
- JSON object containing pickup_library_id attribute is now passed back
  and forth.
- The controller should take care of checking the pickup location is
  valid, using the available tools.

To test:
1. Apply the patches, up to the tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> ERROR: Tests fail because the controller doesn't implement the
desired behavior
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2021-04-16 11:59:18 UTC
Created attachment 119714 [details] [review]
Bug 18729: Adapt holds.js

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2021-04-16 11:59:57 UTC
Works well, is a good enhancement/bugfix and passes the QA scripts.

Passing QA
Comment 38 Jonathan Druart 2021-04-16 14:30:48 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 39 Fridolin Somers 2021-04-22 14:40:45 UTC
Pushed to 20.11.x for 20.11.05
Comment 40 Andrew Fuerste-Henry 2021-04-25 17:07:18 UTC
Pushed to 20.05.x for 20.05.11
Comment 41 Victor Grousset/tuxayo 2021-04-25 20:52:11 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.