Description
David Cook
2020-08-07 07:30:56 UTC
+1! Also relevant for SIP2 users. +1 This would be very helpful. Oh self-checkout as well! I've had people break that by accidentally deleting the autologin user. My boss pointed out that we'd also want to "lock" password changes, as that would have consequences for integrations too. (Not to say you can't change the password, but that it would need to be harder and have a warning that it could break integrations.) Adding a see also on bug 14708 - Anonymouspatron would be another candidate. Bug 23634 provides similar logic to what will be needed here. But instead of "$patron->is_superlibrarian && ! $user->is_superlibrarian", it'll be "$patron->is_system_user && ! $user->is_superlibrarian". Now I just need to decide how to denote a system user. On Bug 23634, Martin mentions that we don't have role based access control (RBAC) and that's true, but maybe that should change. Although RBAC can get complicated. While you could calculate and cache the permissions for a logged in user session, that could get more challenging for the $patron that isn't logged in... I suppose maybe it wouldn't be too hard to do $patron->has_role('system'), especially leveraging a L1 cache. Maybe a default attribute of "undeletable," just to be general Has anyone thought any more about this? What are the ways we could mark a patron as protected/undeleteable (or at least harder to delete)? - A simple flag in the borrowers table - A flag in the categories table, to make whole categories protected - A special "extended patron attribute" Other options? It lingers in the back of my mind, but it's not on my list of priorities, so I haven't thought of it much in years :/. (In reply to David Cook from comment #9) > It lingers in the back of my mind, but it's not on my list of priorities, so > I haven't thought of it much in years :/. Any preferences on how to do it? (In reply to Magnus Enger from comment #10) > (In reply to David Cook from comment #9) > > It lingers in the back of my mind, but it's not on my list of priorities, so > > I haven't thought of it much in years :/. > > Any preferences on how to do it? I'd need to think about it more. Reading back through comments, I realize that it's not just "deleting" that we'd want to prevent, but also "modifications" to things like the permissions, password, and userid. In that sense, the most likely scenarios seem to be that you'd create a normal user, set all the details, and then lock/protect them once you've set them up as desired. And only a superlibrarian can lock/unlock or protect/unprotect. (RBAC would make this much easier but alas.) For my purposes, I'd prefer for no one to be able to unlock/unprotect them via the web UI, but that's not practical for people who don't have backend support. Maybe we could include a koha-conf.xml item to also enable/disable the lock/unlock web interface. We could default it to "on", and then backend admins like myself could turn it off where appropriate. This is probably easiest implemented by adding a Boolean column to the borrowers table. A special "extended patron attribute" could be used, but that seems a bit hacky, and we'd then have to look at securing the patron attributes from changes too (on both the staff interface and OPAC). I think better to have a dedicated schema change. (In reply to David Cook from comment #11) > I'd need to think about it more. Yeah, that's my aim here. ;-) > Reading back through comments, I realize that it's not just "deleting" that > we'd want to prevent, but also "modifications" to things like the > permissions, password, and userid. As a start, I'd be happy if we just made it harder to delete some patrons. We are seeing some cases where users related to things like SIP2 and SCO get deleted, and it would be a big gain if we could just make that happen less often. So maybe we could start there and do the other protections later? But even "make it hard to delete" could mean different things: - Just a popup saying "are you really sure you want to delete this patron" - Make it impossible to delete protected patrons, but make it easy to remove the "protected" flag (so an extra step to delete) - Make it really hard to remove the "protected" flag and delete the patron And we need to think about what happens when someone tries to delete a protected patron. A "direct" delete on one patron is probably not too hard, but what should happen when a protected patron is included in a batch delete? Should the batch delete stop, or should we just report in some way that the protected patron was not deleted? Or should we just ignore it? (In reply to Magnus Enger from comment #12) > As a start, I'd be happy if we just made it harder to delete some patrons. > We are seeing some cases where users related to things like SIP2 and SCO get > deleted, and it would be a big gain if we could just make that happen less > often. So maybe we could start there and do the other protections later? Yeah I'd be happy with that too. I've had some cases where they've changed the passwords for those users, but the more common scenario has been deleted ones. > But even "make it hard to delete" could mean different things: > - Just a popup saying "are you really sure you want to delete this patron" > - Make it impossible to delete protected patrons, but make it easy to remove > the "protected" flag (so an extra step to delete) > - Make it really hard to remove the "protected" flag and delete the patron Yeah I was thinking all those could make some sense hehe. > And we need to think about what happens when someone tries to delete a > protected patron. A "direct" delete on one patron is probably not too hard, > but what should happen when a protected patron is included in a batch > delete? Should the batch delete stop, or should we just report in some way > that the protected patron was not deleted? Or should we just ignore it? I've been wondering about that as well. I imagine the batch delete must already have some capacity for showing errors but I haven't checked. I'm in MVP mode here, so my preferred approach would be to do a simple solution that covers the basics. And then we could elaborate later. So my suggestion would be: - A "protected" flag on the patron (borrowers.protected, for example), editable in the same way as other patron information - Disable the More > Delete menu item on the patron detail page, based on the flag - Make sure protected patrons are handled properly by functions such as -- Koha::Patron::delete -- Koha::Patron::merge_with -- Koha::Patron::move_to_deleted -- Koha::Patron::safe_to_delete -- Koha::Patrons::delete -- C4::Members::GetBorrowersToExpunge Then we could enhance this later with things like: - A permission for changing the protected flag on a patron - A syspref for altering what protected means, like enabling More > Delete, but displaying a simple warning or making the protected attribute un-editable once it has been set (and we could discuss these options later, of course). - Enabling batch edit of protected status MVP and small steps are great. Breaking this up into multiple bugs and keeping the first steps as simple as possible is best practice. This bug might also be of interest: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33965 (In reply to Magnus Enger from comment #14) > I'm in MVP mode here You'd be my MVP (Most Valuable Player) if you went ahead with what you propose. I'd be happy to support with a QA stamp [U+1F44D] Created attachment 154602 [details] [review] Bug 26170: Database update Created attachment 154603 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Created attachment 154604 [details] [review] Bug 26170: Update Schema - DO NOT PUSH *** Bug 22398 has been marked as a duplicate of this bug. *** The patch no longer applies 8-(.. git bz apply 26170 Bug 26170 - Add protected status for patrons 154602 - Bug 26170: Database update 154603 - Bug 26170: Add protected status for patrons 154604 - Bug 26170: Update Schema - DO NOT PUSH Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 26170: Database update Applying: Bug 26170: Add protected status for patrons Using index info to reconstruct a base tree... M C4/Members.pm M Koha/Exceptions/Patron.pm M Koha/Patron.pm M Koha/REST/V1/Patrons.pm M api/v1/swagger/paths/patrons.yaml M koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt M t/db_dependent/Koha/Patron.t M t/db_dependent/Koha/Patrons.t M t/db_dependent/Members.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Members.t CONFLICT (content): Merge conflict in t/db_dependent/Members.t Auto-merging t/db_dependent/Koha/Patrons.t Auto-merging t/db_dependent/Koha/Patron.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc Auto-merging api/v1/swagger/paths/patrons.yaml Auto-merging Koha/REST/V1/Patrons.pm Auto-merging Koha/Patron.pm Auto-merging Koha/Exceptions/Patron.pm Auto-merging C4/Members.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 26170: Add protected status for patrons Created attachment 157247 [details] [review] Bug 26170: Update Schema - DO NOT PUSH Created attachment 157248 [details] [review] Bug 26170: Database update Created attachment 157249 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Created attachment 157250 [details] [review] Bug 26170: Update Schema - DO NOT PUSH Everything works as per the test plan. However, while the "More > Delete" option for a patron is greyed out, it is not disabled. If you click on Delete you get: - a message saying "Are you sure you want to delete the patron NAME?", then - if you say Yes, then an error trace: [Deleting patron failed, patron is protected] at /usr/share/perl5/Exception/Class/Base.pm line 88 Some other suggestions (maybe future enhancements): 1. Change "More > Delete" to "More > Delete patron". I'm not sure whether this makes a difference for translation, but is also consistent with "Renew patron" option. 2. Maybe add a hint for the protected file? "Prevents deleting a patron." Testing notes (using KTD): 1. Run the database update after applying the patch: updatedatabase 2. Run dbic: dbic 3. Restart everything: restart_all Created attachment 157309 [details] [review] Bug 26170: Database update Created attachment 157310 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Created attachment 157311 [details] [review] Bug 26170: Update Schema - DO NOT PUSH The patch has been rebased Created attachment 157391 [details] [review] Bug 26170: Database update Created attachment 157392 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Update 2023-10-19: Fix "More > Delete" on patron, so link can not be clicked. Created attachment 157393 [details] [review] Bug 26170: Update Schema - DO NOT PUSH Created attachment 157395 [details] [review] Bug 26170: Database update Created attachment 157396 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Update 2023-10-19: Fix "More > Delete" on patron, so link can not be clicked. Update 2023-10-19: Rebase Created attachment 157397 [details] [review] Bug 26170: Update Schema - DO NOT PUSH (In reply to Émily-Rose Francoeur from comment #31) > The patch has been rebased Thanks!! Sadly, I forgot to apply my changes in response to Comment 27 on top of your rebase. But I did another rebase and the patches should apply now. Created attachment 157426 [details] [review] Bug 26170: Database update Signed-off-by: David Nind <david@davidnind.com> Created attachment 157427 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Update 2023-10-19: Fix "More > Delete" on patron, so link can not be clicked. Update 2023-10-19: Rebase Signed-off-by: David Nind <david@davidnind.com> Created attachment 157428 [details] [review] Bug 26170: Update Schema - DO NOT PUSH Signed-off-by: David Nind <david@davidnind.com> This is a great first step.. I'd love to see some enhancement bugs attached to allow for limiting who can edit the protected status and other things discussed.. But I feel that getting these foundations in is a great first step so I won't be blocking it. Created attachment 157629 [details] [review] Bug 26170: Database update Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157630 [details] [review] Bug 26170: Add protected status for patrons This set of patches makes it possible to protect patrons from being accidetally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. The following subroutines are affected: - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge Please note: - This does not intend to protect patrons from being edited, only from being deleted To test: * Tests - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t * Editing protected status and manual deletion - Add a new user, note the presence of the "Protected" field under "Library management", but leave it at the default "No", for now. - Note that "Protected" is displayed in the "Library use" section of the patron details. - Note that More > Delete is avaiable as an action when the patron is saved - Edit the user and set "Protected" to "Yes" - Note that More > Delete is now disabled, with a note that the patron is protected * Batch patron deletion - Go to Tools > Batch patron deletion and anonymization - Check the box for "Verify you want to delete patrons" - Choose the category of your protected patron for "whose patron category is" and click "Next" to run the actual deletion - Check that your protected patron was not deleted * Merging patrons - Make sure you have two patrons with similar names or the same category, so you can find them with one search. One should be protected, one not. - Search for the patrons, tick their boxes and click on "Merge selected patrons" - Select one of the patrons as the "patron to keep". . Click on "Merge patrons" - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" (A future enhancement could be to not allow a protected patron to be selected for merging in the first place.) * misc/cronjobs/delete_patrons.pl - Make sure you have a protected patron, in a category with at least one more patron. - Run something like this (at least in ktd): $ perl misc/cronjobs/delete_patrons.pl --category_code <code> -v --confirm (Replace <code> with the actual categorycode.) - Make sure the borrowernumber of the protected patron is not mentioned in the output of the script. - Check the protected patron was not deleted - Check the non-protected patrons were deleted * REST API (with ktd) - Make sure you still have a protected patron, and note their borrowernumber - Enable RESTBasicAuth and restart all the things - Run these two commands from the command line on the host: $ curl -u koha:koha --request GET "http://localhost:8081/api/v1/patrons/54" $ curl -u koha:koha --request DELETE "http://localhost:8081/api/v1/patrons/54" (Replace 54 with the actual borrowernumber of your protected patron.) - The first curl command should give you the patron details. The second should give this output: {"error":"Protected patrons cannot be deleted","error_code":"is_protected"} There could be more functions/scripts where patrons are deleted that I have not thought about. Please report them on the bug if you find any! Update 2023-10-19: Fix "More > Delete" on patron, so link can not be clicked. Update 2023-10-19: Rebase Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157631 [details] [review] Bug 26170: Update Schema - DO NOT PUSH Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All looking good, tests passing, QA scripts happy. Passing QA Hi, I have some doubts about this 'feature' and the implementation. Will do my best to review it and provide feedback ASAP. Pushed to master for 23.11. Nice work everyone, thanks! Introduced a failing test, please fix ASAP. t/db_dependent/Koha/Patrons/Import.t .. 2/178 # Failed test 'No warning raised by import_patrons' # at t/db_dependent/Koha/Patrons/Import.t line 174. # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1017, <$handle_3a> line 2. # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1017, <$handle_3a> line 2. # didn't expect to find a warning # Failed test 'No warning raised by import_patrons' # at t/db_dependent/Koha/Patrons/Import.t line 202. # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1017, <$handle_3c> line 2. # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1017, <$handle_3c> line 2. I had a look, but it's not obvious to me how to fix :( (In reply to Katrin Fischer from comment #50) > I had a look, but it's not obvious to me how to fix :( It is a tough one. I'm taking things apart right now, and it's certainly an issue with the "protected" column. Hopefully should have more info soon... (In reply to David Cook from comment #51) > (In reply to Katrin Fischer from comment #50) > > I had a look, but it's not obvious to me how to fix :( > > It is a tough one. I'm taking things apart right now, and it's certainly an > issue with the "protected" column. Hopefully should have more info soon... So I found a change to t/db_dependent/Koha/Patrons/Import.t which makes the test pass, but I don't think it's the right place to change it. Then I figured out a change to Koha/Patrons/Import.pm which actually points out some changes needed for http://localhost:8081/cgi-bin/koha/tools/import_borrowers.pl as well... (In reply to Katrin Fischer from comment #50) > I had a look, but it's not obvious to me how to fix :( I've got a fix on bug 35264. It includes a minor change to the "Import patrons" tool as well. (An alternative is to include "protected" in the test CSV in t/db_dependent/Koha/Patrons/Import.t but that would really just suppress a warning that would appear during normal operations.) Confirmed 35264 is the solution here. PQA on that followup. Thanks everyone. Enhancement not pushed to 23.05.x |