Description
Caroline Cyr La Rose
2024-09-30 16:48:08 UTC
Created attachment 173400 [details] [review] Bug 38040: Disabled tables element for staff The test plan is the same has mentioned before: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes' 2. Set canreservefromotherbranches to "Don't allow (with independent branches)" 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Created attachment 176050 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled When IndependentBranches is enabled, any staff member from any library can see and edit the holds for patrons from other libraries. Only superlibrarians should be able to edit all holds, including those from other libraries. Staff patrons should only be able to edit or delete holds for their library. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Signed-off-by: David Nind <david@davidnind.com> I updated the assignee, edited the commit message, and added a release note. 1. is_superlibrarian is already available from every templates Use logged_in_user.is_superlibrarian or CAN_user_superlibrarian 2. Same for the logged in library, use Branches.GetLoggedInBranchcode 3. Useless indentation change in request.pl So basically you don't need any changes from the controller. Is this enforce server-side? Created attachment 177197 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled I made the corrections from comment 4. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Signed-off-by: David Nind <david@davidnind.com> Created attachment 177198 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled Created attachment 177292 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled Rebased Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Signed-off-by: David Nind <david@davidnind.com> I've retested, but for step 8 I can still cancel holds made by other libraries. Please also amend the commit message so that the description describes the problem - similar to comment #2. See the commit message guidelines https://wiki.koha-community.org/wiki/Commit_messages (Please don't put comments in the commit message like "Rebased" - these can be done as normal bug comments.) Created attachment 177472 [details] [review] Bug 38040: Light reformating When IndependentBranches is enabled, any staff member from any library can see and edit the holds for patrons from other libraries. Only superlibrarians should be able to edit all holds, including those from other libraries. Staff patrons should only be able to edit or delete holds for their library. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Hi David, Thank you for the feedback, I had accidentally reverted the changes from previous patches. Hopefully this should work now. This patch implements the corrections from comment 4. Created attachment 177492 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled When IndependentBranches is enabled, any staff member from any library can see and edit the holds for patrons from other libraries. Only superlibrarians should be able to edit all holds, including those from other libraries. Staff patrons should only be able to edit or delete holds for their library. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Signed-off-by: David Nind <david@davidnind.com> (In reply to William Lavoie from comment #10) > Hi David, > > Thank you for the feedback, I had accidentally reverted the changes from > previous patches. Hopefully this should work now. > > This patch implements the corrections from comment 4. Thanks William! I've squashed the patches (not sure whether I should have, but I think that is OK in this instance). I've changed the assignee as well. Good luck with the QA steps! David This patch only deals with the template, I can easily edit the html, remove the disabled class, and still cancel the hold. A check needs to be added into the perl scripts or the modules. It also affects all non-superlibrarians, regardless of independentbranches being enabled/disabled, we need to check that preference first. I think maybe we need an additional permission for reserves here - which says whether you can affect all holds or not. Taking away the ability to modify/cancel holds would be a big change in behavior that I am not sure all libraries using independent branches would agree to. The trend has been to move parts of Independent Branches to more granular system preferences ( IndependentBranchesPatronModifications , IndependentBranchesTransfers ) so maybe we need 'IndependentBranchesReserves'? This current code could be adapted to work in IntranerUserJS if it is sufficient for your library, happy to help with that or any of the changes above if I can Created attachment 178014 [details] [review] Bug 38040: Adding server-side verification Added a system preference 'IndependentBranchesHolds' and server-side verification in the perl scripts Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, IndependentBranchesTransfers and IndependentBranchesHolds to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and update the database 9. Validate that step 7 is not possible now and that holds from the staff's library can be modified or deleted. Created attachment 178015 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled When IndependentBranches is enabled, any staff member from any library can see and edit the holds for patrons from other libraries. Only superlibrarians should be able to edit all holds, including those from other libraries. Staff patrons should only be able to edit or delete holds for their library. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Signed-off-by: David Nind <david@davidnind.com> Created attachment 178016 [details] [review] Bug 38040: Adding server-side verification Added a system preference 'IndependentBranchesHolds' and server-side verification in the perl scripts Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, IndependentBranchesTransfers and IndependentBranchesHolds to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and update the database 9. Validate that step 7 is not possible now and that holds from the staff's library can be modified or deleted. Hi Owen, Thank you for the feedback, I have added checks in the perl scripts for deletion as well as the other operations (changing priority, adding an expiration date, batch deletions, etc). After a discussion with Caroline, we agreed with your solution to add a system preference 'IndependentBranchesHolds' (the Koha terminology favors 'Hold' over 'Reserve'). I have added an atomic update. Additionally, in order to allow users to change priority within their own library, I changed the way holds are displayed so that for non-superlibrarians, when 'IndependentBranchesHolds' is set to 'Yes', the holds are displayed by library, similarly to having 'HoldsSplitQueue' set to 'branch'. (In reply to William Lavoie from comment #17) My apologies, I meant to address Nick. Created attachment 179186 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled When IndependentBranches is enabled, any staff member from any library can see and edit the holds for patrons from other libraries. Only superlibrarians should be able to edit all holds, including those from other libraries. Staff patrons should only be able to edit or delete holds for their library. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, and IndependentBranchesTransfers to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and validate that step 7 is not possible now. Signed-off-by: David Nind <david@davidnind.com> Created attachment 179187 [details] [review] Bug 38040: Adding server-side verification Added a system preference 'IndependentBranchesHolds' and server-side verification in the perl scripts Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, IndependentBranchesTransfers and IndependentBranchesHolds to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and update the database 9. Validate that step 7 is not possible now and that holds from the staff's library can be modified or deleted. Comment on attachment 179186 [details] [review] Bug 38040: Prevent editing other libraries' holds when IndependentBranches is enabled Review of attachment 179186 [details] [review]: ----------------------------------------------------------------- ::: reserve/request.pl @@ +39,4 @@ > use C4::Serials qw( CountSubscriptionFromBiblionumber ); > use C4::Circulation qw( _GetCircControlBranch GetBranchItemRule ); > use Koha::DateUtils qw( dt_from_string ); > +<<<<<<< HEAD This patch contains a git merge error There's a git merge error in the second patch. Can you fix that and squash the patches together? (Then perhaps split out the DB update into a separate patch?) Thanks for your work to-date on this one. I need this fix soon for one of my libraries, as I think it's the last piece of the puzzle of IndependentBranches/LibraryGroups/heaps of my own fixes to get Koha usable for this particular set of libraries... (In reply to David Cook from comment #23) > Thanks for your work to-date on this one. I need this fix soon for one of my > libraries, as I think it's the last piece of the puzzle of > IndependentBranches/LibraryGroups/heaps of my own fixes to get Koha usable > for this particular set of libraries... What I mean is... I can dedicate time to working with inLibro on this. So I Failed QA on the 2nd patch, and now I'm marking "Patch doesn't apply" on the 1st patch too. error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt). error: could not build fake ancestor I've fetched from the Koha upstream, so that suggests that this patch has been based off a local commit that doesn't exist in the Koha upstream. Please rebase. Inspecting the code more closely and looks like you've got your patches out of order as well. Btw in installer/data/mysql/mandatory/sysprefs.sql the syspref needs to be added in alphabetical order by variable name. Lots of unnecessary white space changes as well. Noticing a lot of spaces around the method operator -> as well which might not get caught by a tidy but is atypical for Koha. Since this one is quite important for me, I'm going to try to resurrect it... I was busy today, but taking a quick look at the code. Needed to add the following to request.tt as it wasn't disabling the pickup location dropdown: $(this).find('.pickup_location_dropdown').prop('disabled', true); -- It looks like these patches don't take into account modrequest.pl? That script is invoked by the "Update hold(s)" button. (Holds/reserves code truly is a nightmare, eh?) -- I am tempted to take the frontend shortcut for my current project, since validating server-side is a minefield. I'll think on this more tomorrow... So... I'm a bit confused here. I've followed the test plan and I've reviewed the code, and it seems to me that the text "A patron from library Centerville" comes from the Library Groups feature and not IndependentBranches. Because I can't get "A patron from library Centerville" to appear in main following the test plan, and when I look at the code that makes sense, because it's a Library Groups feature. As a result, it seems to me that the solution should actually be with Library Groups and not IndependentBranches. (In reply to David Cook from comment #30) > As a result, it seems to me that the solution should actually be with > Library Groups and not IndependentBranches. Oh wait a minute...the text "A patron from library Centerville" shows for both features. I was looking for IndependentBranches in Koha/Patron.pm but now I see C4::Context::only_my_library. I made a mistake in my testing as well. Koha::Patron->libraries_where_can_see_things is pretty suboptimal for Library Groups... iterating through every group and subgroup to find the ones that apply to a borrower at the code level rather than SQL... It would be interesting to have a "Limit hold management by group" for Library Groups though... Since the holds in tmpl/intranet-tmpl/prog/en/includes/holds_table.inc are actually hashrefs from reserve/request.pl we can actually do the permission check in the controller, and then we just look for the flag in the template/view so that we can toggle the display accordingly. Something like "$logged_in_user->can_manage_hold($hold->patron)" would be good, because it could apply to both IndependentBranches and Library Groups. Plus we can then use it for validating the actions/ops server-side as well. The only question that remains then is how to gracefully handle errors server-side. Although really I think something like pre-validating before the action/op and throwing a 403 is not a bad idea. Simple and effective. Created attachment 179714 [details] [review] Bug 38040: [alternate] Restrict hold management if cannot see patron data This patch leverages the Koha::Patron->can_see_patron_infos() function to determine whether or not the UI will display hold management functions. If the logged in user cannot see the patron info, it seems to follow that they should not be able to manage the hold either. NOTE: This should work for both IndependentBranches and Library Group functionality. This is only the front-end modification. To be coded: a back-end modification that throws a 403 for stateful actions to holds the logged in user is not allowed to manage. I've uploaded an alternate patch, which hides the hold management controls at the template level if you can't see the patron data (via either IndependentBranches or Library Groups). This serves my immediate purposes, but let me know what you think at the community level, and we can keep working on the feature (ie validating requests to request.pl, modrequest.pl, etc.) -- Note that even with the controls hidden, if your logged in patron has permission to change priority, they can still change their own holds relative to holds from other branches. Btw, if you view my patch with "git show -w", it's much less scary than the Splinter Review. These are the stats without the whitespace change: 2 files changed, 26 insertions(+), 3 deletions(-) Created attachment 180531 [details] [review] Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled Created attachment 180532 [details] [review] Bug 38040: DB Update: Adding IndependentBranchesHolds syspref Created attachment 180581 [details] [review] Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled I added the verification to modrequest.pl as well. Created attachment 180582 [details] [review] Bug 38040: DB Update - Adding IndependentBranchesHolds syspref Hello David, I appreciate the feedback and I tried to address everything you brought up in these patches. I merged the front-end and back-end patches into a single one and separated the DB update, I removed white spaces, I put the syspref in alphabetical order and I added server-side validation in modrequest.pl. If the alternate patch is preferred then I can help with the back-end modifications. Test plan: 1. Set IndependentBranches, IndependentBranchesPatronModifications, IndependentBranchesTransfers and IndependentBranchesHolds to 'Yes'. 2. Set canreservefromotherbranches to "Don't allow (with independent branches)". 3. Give limited permissions to a staff patron - circulate - catalogue - delete_borrowers - edit_borrowers - list_borrowers - send_messages_to_borrowers - reserveforothers - updatecharges 4. As the superlibrarian, place several holds on a record with items from various branches. 5. Log in as the restricted staff 6. View the holds for the record --> Some will say "A patron from library XX" instead of the name -- OK 7. Click the trash bin icon to cancel a hold from another library --> Hold is cancelled -- NOT OK 8. Apply the patch and update the database 9. Validate that step 7 is not possible now and that holds from the staff's library can be modified or deleted. Created attachment 180898 [details] [review] Bug 38040: [alternate] Restrict hold management if cannot see patron data This patch leverages the Koha::Patron->can_see_patron_infos() function to determine whether or not the UI will display hold management functions. If the logged in user cannot see the patron info, it seems to follow that they should not be able to manage the hold either. NOTE: This should work for both IndependentBranches and Library Group functionality. This is only the front-end modification. To be coded: a back-end modification that throws a 403 for stateful actions to holds the logged in user is not allowed to manage. Signed-off-by: Magnus Enger <magnus@libriotech.no> Created attachment 180899 [details] [review] Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled I added the verification to modrequest.pl as well. Signed-off-by: Magnus Enger <magnus@libriotech.no> Created attachment 180900 [details] [review] Bug 38040: DB Update - Adding IndependentBranchesHolds syspref Signed-off-by: Magnus Enger <magnus@libriotech.no> Magnus did you sign off both approaches? Created attachment 182444 [details] [review] Bug 38040: DB Update - Adding IndependentBranchesHolds syspref Signed-off-by: Magnus Enger <magnus@libriotech.no> Rebased to main. Created attachment 182445 [details] [review] Bug 38040: [alternate] Restrict hold management if cannot see patron data This patch leverages the Koha::Patron->can_see_patron_infos() function to determine whether or not the UI will display hold management functions. If the logged in user cannot see the patron info, it seems to follow that they should not be able to manage the hold either. NOTE: This should work for both IndependentBranches and Library Group functionality. This is only the front-end modification. To be coded: a back-end modification that throws a 403 for stateful actions to holds the logged in user is not allowed to manage. Signed-off-by: Magnus Enger <magnus@libriotech.no> Created attachment 182446 [details] [review] Bug 38040: Preventing editing other libraries' holds with IndependentBranches enabled I added the verification to modrequest.pl as well. Signed-off-by: Magnus Enger <magnus@libriotech.no> Created attachment 182447 [details] [review] Bug 38040: DB Update - Adding IndependentBranchesHolds syspref Signed-off-by: Magnus Enger <magnus@libriotech.no> Rebased (In reply to David Cook from comment #45) > Magnus did you sign off both approaches? This looks like a good question to be answered before starting QA. Could you please address it, Magnus ? Thanks. Changing status to reflect need for feedback. |