Summary: | Club holds can't be placed without modify_holds_priority permission | ||
---|---|---|---|
Product: | Koha | Reporter: | George Williams (NEKLS) <george> |
Component: | Hold requests | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | Pushed to main --- | QA Contact: | Pedro Amorim <pedro.amorim> |
Severity: | normal | ||
Priority: | P5 - low | CC: | ayoung, bugzilla, cbrannon, david, gmcharlt, jrobb, lisette, lucas, nick, paul.derscheid, pedro.amorim, trevor.diamond |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39872 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39873 |
||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Small patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: |
This fixes placing holds for clubs. Placing holds for a club now only requires the place_holds permission.
|
|
Version(s) released in: |
25.05.00
|
Circulation function: | |
Bug Depends on: | |||
Bug Blocks: | 39869 | ||
Attachments: |
Bug 25787: Update club holds to only require place_holds permission
Bug 25787: Add unit tests Bug 25787: Update club holds to only require place_holds permission Bug 25787: Add unit tests Bug 25787: Update club holds to only require place_holds permission Bug 25787: Add unit tests |
Description
George Williams (NEKLS)
2020-06-17 18:37:23 UTC
That's no good. I've also found that staff can't see the button for the tools page if the clubs is the only tool they have permission too. But that is a different bug. This definitely needs to be fixed. I don't like loop holes in permissions. Still valid in 21.11.11. Technically place_holds is also required to get that far so it may be that the overarching reserveforothers permission is what's truly required to successfully place a club hold. The ability to place club holds should really be its own separate user permission nested under the reserveforothers (or possibly clubs?) permission group. In our club usage scenario, a mistaken club hold can lead to hundreds of a holds on a record which puts many items in motion to patrons that don't actually want them. I wrote some JS to hide the "Clubs" tab at request.pl with exception for the select staff users in charge of placing club holds. It also changes the label wording from "Search patrons or clubs" to just "Search patrons". var loggedUser = $("#logged-in-info-full .loggedinusername").text(); var clubbers = ["staffusername1", "staffusername2"]; //show for these users if ($.inArray(loggedUser, clubbers) == -1) { $('#circ_request [href="#holds_clubsearch_pane"]').closest('li').hide(); $('#circ_request label:contains("Search patrons or clubs")').text("Search patrons"); } This bizarre requirement is still around in 24.05. I had to ask around to even figure out this permission was related as the name and the function we're attempting to do (place club holds) aren't related on the surface. If it happens to be fixed in 24.11 I will come back and update this bug (I'll find out in June 2025) Hi Trevor, commenting on the bug and confirming it#s still an issue in a current version is very helpful. I can't promise you a fix, but I'll highlight it in the list on the dashboard with my keyword. As George suggested, the API route expects full reserveforothers api/v1/swagger/paths/clubs.yaml: 94 x-koha-authorization: 95 permissions: 96 reserveforothers: "1" Created attachment 179633 [details] [review] Bug 25787: Update club holds to only require place_holds permission This moves the permission check from top level to the specific place_hold permission To test: 1 - Create a club 2 - Enroll some patrons 3 - Find or create a staff user with 'place_holds' permission and catalogue permission 4 - Log in as that staff 5 - Find a book and go to holds tab' 6 - Search for your club 7 - Try to place hold 8 - Nothing happens FIXME we should alert of the 403 error or other 9 - Apply patch, yanr build, restart_all 10 - Place hold 11 - Success! Partial patch attached, needs unit tests Created attachment 181831 [details] [review] Bug 25787: Add unit tests prove -v t/db_dependent/api/v1/clubs_holds.t Created attachment 181841 [details] [review] Bug 25787: Update club holds to only require place_holds permission This moves the permission check from top level to the specific place_hold permission To test: 1 - Create a club 2 - Enroll some patrons 3 - Find or create a staff user with 'place_holds' permission and catalogue permission 4 - Log in as that staff 5 - Find a book and go to holds tab' 6 - Search for your club 7 - Try to place hold 8 - Nothing happens FIXME we should alert of the 403 error or other 9 - Apply patch, yanr build, restart_all 10 - Place hold 11 - Success! Signed-off-by: David Nind <david@davidnind.com> Created attachment 181842 [details] [review] Bug 25787: Add unit tests prove -v t/db_dependent/api/v1/clubs_holds.t Signed-off-by: David Nind <david@davidnind.com> One thing I noticed on the holds page (before and after the patch) is that the autocomplete for the club ID or partial name on the holds page doesn't work for me. Most likely, another bug that needs fixing! Testing notes (suing KTD): 1. To enroll patrons in a club: 1.1 Go to a patron's page in the staff interface 1.2 On either their check out or details page, select the "Clubs" tab 1.3 Click "Enroll" action to enroll them in the club Created attachment 181985 [details] [review] Bug 25787: Update club holds to only require place_holds permission This moves the permission check from top level to the specific place_hold permission To test: 1 - Create a club 2 - Enroll some patrons 3 - Find or create a staff user with 'place_holds' permission and catalogue permission 4 - Log in as that staff 5 - Find a book and go to holds tab' 6 - Search for your club 7 - Try to place hold 8 - Nothing happens FIXME we should alert of the 403 error or other 9 - Apply patch, yanr build, restart_all 10 - Place hold 11 - Success! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk> Created attachment 181986 [details] [review] Bug 25787: Add unit tests prove -v t/db_dependent/api/v1/clubs_holds.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk> Passing QA here. (In reply to Jason Robb from comment #2) > The ability to place club holds should really be its own separate user > permission nested under the reserveforothers (or possibly clubs?) permission > group. This is a valid point, and likely worth to open a follow-up bug for it (enhancement). (In reply to David Nind from comment #11) > One thing I noticed on the holds page (before and after the patch) is that > the autocomplete for the club ID or partial name on the holds page doesn't > work for me. > > Most likely, another bug that needs fixing! I also noticed this, but I'm not sure it's a bug. I think it's a feature that's missing! I may be wrong, but it appears this club input was not designed to have autocomplete in the first place! (In reply to Pedro Amorim from comment #14) > Passing QA here. > > (In reply to Jason Robb from comment #2) > > The ability to place club holds should really be its own separate user > > permission nested under the reserveforothers (or possibly clubs?) permission > > group. > > This is a valid point, and likely worth to open a follow-up bug for it > (enhancement). > > (In reply to David Nind from comment #11) > > One thing I noticed on the holds page (before and after the patch) is that > > the autocomplete for the club ID or partial name on the holds page doesn't > > work for me. > > > > Most likely, another bug that needs fixing! > > I also noticed this, but I'm not sure it's a bug. I think it's a feature > that's missing! I may be wrong, but it appears this club input was not > designed to have autocomplete in the first place! Can you please open the bugs mentioned here? Thanks! Pushed for 25.05! Well done everyone, thank you! I believe this broke the test on D12: t_db_dependent_api_v1_clubs_holds_t Can you please double check and provide a fix? (In reply to Katrin Fischer from comment #17) > I believe this broke the test on D12: > > t_db_dependent_api_v1_clubs_holds_t > > Can you please double check and provide a fix? I can't reproduce this failure locally. Hm, this one here is another candidate: 15:52:20 koha_1 | # Looks like you failed 1 test of 3. 15:52:20 koha_1 | [13:50:45] t/db_dependent/api/v1/clubs_holds.t Can't reproduce this failure locally either. Fixed the club related tests now it looks like! (In reply to Katrin Fischer from comment #15) > (In reply to Pedro Amorim from comment #14) > > Passing QA here. > > > > (In reply to Jason Robb from comment #2) > > > The ability to place club holds should really be its own separate user > > > permission nested under the reserveforothers (or possibly clubs?) permission > > > group. > > > > This is a valid point, and likely worth to open a follow-up bug for it > > (enhancement). > > > > (In reply to David Nind from comment #11) > > > One thing I noticed on the holds page (before and after the patch) is that > > > the autocomplete for the club ID or partial name on the holds page doesn't > > > work for me. > > > > > > Most likely, another bug that needs fixing! > > > > I also noticed this, but I'm not sure it's a bug. I think it's a feature > > that's missing! I may be wrong, but it appears this club input was not > > designed to have autocomplete in the first place! > > Can you please open the bugs mentioned here? Thanks! Bugs created: 1. Bug 39873 - Patron club holds - separate permission for ability to place club holds 2. Bug 39872 - Patron club holds - autocomplete does not work as expected (like patron name autocomplete) |