Bug 27117 - Staff without modify_holds_priority permission can't update hold pick-up from biblio
Summary: Staff without modify_holds_priority permission can't update hold pick-up from...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 18729 27201
  Show dependency treegraph
 
Reported: 2020-11-30 17:09 UTC by Martha Fuerst
Modified: 2020-12-28 18:57 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.07


Attachments
screenshot of error when trying to open the drop-down menu for pick-up location on record details page (66.20 KB, image/png)
2020-11-30 17:09 UTC, Martha Fuerst
Details
Bug 27117 : Only place_holds permission is needed to adjust pickup locations (1.25 KB, patch)
2020-12-01 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27117 : Only place_holds permission is needed to adjust pickup locations (2.24 KB, patch)
2020-12-01 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27117: Only place_holds permission is needed to adjust pickup locations (2.29 KB, patch)
2020-12-10 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27117: Only place_holds permission is needed to adjust pickup locations (2.36 KB, patch)
2020-12-16 15:43 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martha Fuerst 2020-11-30 17:09:31 UTC
Created attachment 114060 [details]
screenshot of error when trying to open the drop-down menu for pick-up location on record details page

This is perhaps related to Bug 18729.

Prior to 20.05.05, staff without the modify_holds_priority permission were able to update the pick-up location for a hold from the record details page, but not the patron's details or checkouts pages.

Staff now see an error (see attached screenshot) when they open the drop-down menu, and when they click "close", the dialog box keeps popping up. I counted 30 times before I was able to (I think?) click away so that it didn't regenerate.

This needs to be fixed, and ultimately, Bug 18729 really needs to be addressed.
Comment 1 Jonathan Druart 2020-12-01 10:50:21 UTC
Created attachment 114063 [details] [review]
Bug 27117 : Only place_holds permission is needed to adjust pickup locations

The GET /pickup_locations route is requesting the whole reserveforothers
permission whereas only the subpermission place_holds is needed.

Test plan:
0. Don't apply this patch
1. Set the subpermission place_holds but modify_holds_priority
2. Edit a hold and click the pickup library dropdown list
3. You get a JS alert and log displays
  GET /api/v1/app.pl/api/v1/holds/5/pickup_locations
  403 Forbidden
4. Apply this patch
5. Reload the page, click the dropdown list, modify the pickup location
and save
=> Success!
Comment 2 Jonathan Druart 2020-12-01 10:54:52 UTC
Created attachment 114064 [details] [review]
Bug 27117 : Only place_holds permission is needed to adjust pickup locations

The GET /pickup_locations route is requesting the whole reserveforothers
permission whereas only the subpermission place_holds is needed.

Test plan:
0. Don't apply this patch
1. Set the subpermission place_holds but modify_holds_priority
2. Edit a hold and click the pickup library dropdown list
3. You get a JS alert and log displays
  GET /api/v1/app.pl/api/v1/holds/5/pickup_locations
  403 Forbidden
4. Apply this patch
5. Reload the page, click the dropdown list, modify the pickup location
and save
=> Success!
Comment 3 Tomás Cohen Arazi 2020-12-09 21:49:13 UTC
I agree with the solution and also with the follow-up bug on 18729.

This is giving a weird error if the user doesn't have permissions. The right thing would be to completely disable the dropdown IMHO.
Comment 4 Tomás Cohen Arazi 2020-12-10 14:56:19 UTC
Created attachment 114310 [details] [review]
Bug 27117: Only place_holds permission is needed to adjust pickup locations

The GET /pickup_locations route is requesting the whole reserveforothers
permission whereas only the subpermission place_holds is needed.

Test plan:
0. Don't apply this patch
1. Set the subpermission place_holds but modify_holds_priority
2. Edit a hold and click the pickup library dropdown list
3. You get a JS alert and log displays
  GET /api/v1/app.pl/api/v1/holds/5/pickup_locations
  403 Forbidden
4. Apply this patch
5. Reload the page, click the dropdown list, modify the pickup location
and save
=> Success!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2020-12-10 14:57:41 UTC
This works as expected (today), not sure what mistake I made yesterday.

The QA script is happy, and the changes to the tests make sense (and they pass).

Thanks Jonathan!
Comment 6 Kyle M Hall 2020-12-16 15:43:56 UTC
Created attachment 114451 [details] [review]
Bug 27117: Only place_holds permission is needed to adjust pickup locations

The GET /pickup_locations route is requesting the whole reserveforothers
permission whereas only the subpermission place_holds is needed.

Test plan:
0. Don't apply this patch
1. Set the subpermission place_holds but modify_holds_priority
2. Edit a hold and click the pickup library dropdown list
3. You get a JS alert and log displays
  GET /api/v1/app.pl/api/v1/holds/5/pickup_locations
  403 Forbidden
4. Apply this patch
5. Reload the page, click the dropdown list, modify the pickup location
and save
=> Success!

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Jonathan Druart 2020-12-21 09:20:20 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 8 Fridolin Somers 2020-12-28 13:48:28 UTC
Pushed to 20.11.x for 20.11.01
Comment 9 Andrew Fuerste-Henry 2020-12-28 15:27:25 UTC
Pushed to 20.05.x for 20.05.07
Comment 10 Victor Grousset/tuxayo 2020-12-28 18:57:41 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.