26170 – Add protected status for patrons

Bug 26170 - Add protected status for patrons

Summary: Add protected status for patrons

Status:	RESOLVED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Patrons (show other bugs)
Version:	Main
Hardware:	All All

Importance:	P5 - low enhancement
Assignee:	Magnus Enger
QA Contact:	Martin Renvoize (ashimema)

URL:
Keywords:

Duplicates (1):	22398 (view as bug list)
Depends on:
Blocks:	35474 36085 35264
	Show dependency tree / graph

Reported:	2020-08-07 07:30 UTC by David Cook
Modified:	2024-05-16 18:03 UTC (History)
CC List:	15 users (show)

See Also:	14708 23634 33965 22577
Change sponsored?:	---
Patch complexity:	Medium patch
Documentation contact:	Caroline Cyr La Rose
Documentation submission:	https://gitlab.com/koha-community/koha-manual/-/merge_requests/754
Text to go in the release notes:	This enhancement makes it possible to protect patrons from being accidentally deleted or merged with other patrons, from the UI and from (well behaved) cron jobs. It adds a 'Protected' field (with Yes and No options) in the library use section for a patron's record.
Version(s) released in:	23.11.00
Circulation function:

Attachments
Bug 26170: Database update (3.10 KB, patch) 2023-08-18 08:30 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (18.92 KB, patch) 2023-08-18 08:31 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.45 KB, patch) 2023-08-18 08:31 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.45 KB, patch) 2023-10-17 13:32 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Database update (3.10 KB, patch) 2023-10-17 13:34 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (19.06 KB, patch) 2023-10-17 13:34 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.45 KB, patch) 2023-10-17 13:34 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Database update (3.10 KB, patch) 2023-10-18 12:44 UTC, Emily-Rose Francoeur	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (19.06 KB, patch) 2023-10-18 12:44 UTC, Emily-Rose Francoeur	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (2.97 KB, patch) 2023-10-18 12:44 UTC, Emily-Rose Francoeur	Details \| Diff \| Splinter Review
Bug 26170: Database update (3.10 KB, patch) 2023-10-19 08:30 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (19.87 KB, patch) 2023-10-19 08:30 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.45 KB, patch) 2023-10-19 08:30 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Database update (3.10 KB, patch) 2023-10-19 08:43 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (19.92 KB, patch) 2023-10-19 08:43 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.45 KB, patch) 2023-10-19 08:43 UTC, Magnus Enger	Details \| Diff \| Splinter Review
Bug 26170: Database update (3.15 KB, patch) 2023-10-19 12:00 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (19.97 KB, patch) 2023-10-19 12:00 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.50 KB, patch) 2023-10-19 12:00 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 26170: Database update (3.29 KB, patch) 2023-10-23 08:14 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 26170: Add protected status for patrons (20.49 KB, patch) 2023-10-23 08:14 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 26170: Update Schema - DO NOT PUSH (3.56 KB, patch) 2023-10-23 08:14 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Show Obsolete (19) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description David Cook 2020-08-07 07:30:56 UTC

At the moment, the only way to create OAuth2 or Basic Auth credentials is to generate a user.

This user can be deleted by a librarian by accident, which will break your API integration with Koha. 

I propose that we make some system users that cannot be deleted from the Web UI. (Or users that can be deleted by a superlibrarian, but need to be "unlocked" first or maybe have their category changed, and a warning splashed that this will likely break integrations with other systems.)

Comment 1 Magnus Enger 2020-08-07 08:27:25 UTC

+1! Also relevant for SIP2 users.

Comment 2 Lisette Scheer 2020-08-07 16:32:27 UTC

+1 This would be very helpful.

Comment 3 David Cook 2020-08-09 23:38:11 UTC

Oh self-checkout as well! I've had people break that by accidentally deleting the autologin user.

Comment 4 David Cook 2020-08-09 23:39:00 UTC

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.)

Comment 5 Katrin Fischer 2020-08-16 11:09:55 UTC

Adding a see also on bug 14708 - Anonymouspatron would be another candidate.

Comment 6 David Cook 2020-09-01 00:03:34 UTC

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.

Comment 7 Liz Rea 2022-07-13 18:25:44 UTC

Maybe a default attribute of "undeletable," just to be general

Comment 8 Magnus Enger 2023-06-12 11:47:12 UTC

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?

Comment 9 David Cook 2023-06-12 23:33:35 UTC

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 :/.

Comment 10 Magnus Enger 2023-06-13 06:19:23 UTC

(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?

Comment 11 David Cook 2023-06-13 23:35:35 UTC

(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.

Comment 12 Magnus Enger 2023-06-14 06:39:20 UTC

(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?

Comment 13 David Cook 2023-06-14 23:24:56 UTC

(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.

Comment 14 Magnus Enger 2023-06-15 08:02:09 UTC

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

Comment 15 Katrin Fischer 2023-06-15 08:06:13 UTC

MVP and small steps are great. Breaking this up into multiple bugs and keeping the first steps as simple as possible is best practice.

Comment 16 Sally 2023-06-15 08:13:51 UTC

This bug might also be of interest:  https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33965

Comment 17 David Cook 2023-06-16 00:54:52 UTC

(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]﷑

Comment 18 Magnus Enger 2023-08-18 08:30:40 UTC Comment hidden (obsolete)

Created attachment 154602 [details] [review]
Bug 26170: Database update

Comment 19 Magnus Enger 2023-08-18 08:31:24 UTC Comment hidden (obsolete)

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!

Comment 20 Magnus Enger 2023-08-18 08:31:43 UTC Comment hidden (obsolete)

Created attachment 154604 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Comment 21 Katrin Fischer 2023-09-23 10:27:03 UTC

*** Bug 22398 has been marked as a duplicate of this bug. ***

Comment 22 David Nind 2023-09-25 00:08:03 UTC Comment hidden (obsolete)

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

Comment 23 Magnus Enger 2023-10-17 13:32:33 UTC Comment hidden (obsolete)

Created attachment 157247 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Comment 24 Magnus Enger 2023-10-17 13:34:09 UTC Comment hidden (obsolete)

Created attachment 157248 [details] [review]
Bug 26170: Database update

Comment 25 Magnus Enger 2023-10-17 13:34:13 UTC Comment hidden (obsolete)

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!

Comment 26 Magnus Enger 2023-10-17 13:34:17 UTC Comment hidden (obsolete)

Created attachment 157250 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Comment 27 David Nind 2023-10-17 18:09:51 UTC

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

Comment 28 Emily-Rose Francoeur 2023-10-18 12:44:17 UTC Comment hidden (obsolete)

Created attachment 157309 [details] [review]
Bug 26170: Database update

Comment 29 Emily-Rose Francoeur 2023-10-18 12:44:19 UTC Comment hidden (obsolete)

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!

Comment 30 Emily-Rose Francoeur 2023-10-18 12:44:22 UTC Comment hidden (obsolete)

Created attachment 157311 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Comment 31 Emily-Rose Francoeur 2023-10-18 12:47:07 UTC

The patch has been rebased

Comment 32 Magnus Enger 2023-10-19 08:30:11 UTC Comment hidden (obsolete)

Created attachment 157391 [details] [review]
Bug 26170: Database update

Comment 33 Magnus Enger 2023-10-19 08:30:15 UTC Comment hidden (obsolete)

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.

Comment 34 Magnus Enger 2023-10-19 08:30:19 UTC Comment hidden (obsolete)

Created attachment 157393 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Comment 35 Magnus Enger 2023-10-19 08:43:28 UTC Comment hidden (obsolete)

Created attachment 157395 [details] [review]
Bug 26170: Database update

Comment 36 Magnus Enger 2023-10-19 08:43:32 UTC Comment hidden (obsolete)

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

Comment 37 Magnus Enger 2023-10-19 08:43:36 UTC Comment hidden (obsolete)

Created attachment 157397 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Comment 38 Magnus Enger 2023-10-19 08:45:39 UTC

(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.

Comment 39 David Nind 2023-10-19 12:00:12 UTC Comment hidden (obsolete)

Created attachment 157426 [details] [review]
Bug 26170: Database update

Signed-off-by: David Nind <david@davidnind.com>

Comment 40 David Nind 2023-10-19 12:00:16 UTC Comment hidden (obsolete)

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>

Comment 41 David Nind 2023-10-19 12:00:19 UTC Comment hidden (obsolete)

Created attachment 157428 [details] [review]
Bug 26170: Update Schema - DO NOT PUSH

Signed-off-by: David Nind <david@davidnind.com>

Comment 42 Martin Renvoize (ashimema) 2023-10-23 07:57:14 UTC

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.

Comment 43 Martin Renvoize (ashimema) 2023-10-23 08:14:19 UTC

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>

Comment 44 Martin Renvoize (ashimema) 2023-10-23 08:14:22 UTC

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>

Comment 45 Martin Renvoize (ashimema) 2023-10-23 08:14:26 UTC

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>

Comment 46 Martin Renvoize (ashimema) 2023-10-23 08:14:54 UTC

All looking good, tests passing, QA scripts happy.

Passing QA

Comment 47 Tomás Cohen Arazi (tcohen) 2023-10-26 19:40:07 UTC

Hi, I have some doubts about this 'feature' and the implementation. Will do my best to review it and provide feedback ASAP.

Comment 48 Tomás Cohen Arazi (tcohen) 2023-11-01 20:26:16 UTC

Pushed to master for 23.11.

Nice work everyone, thanks!

Comment 49 Jonathan Druart 2023-11-03 13:09:00 UTC

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.

Comment 50 Katrin Fischer 2023-11-04 12:55:48 UTC

I had a look, but it's not obvious to me how to fix :(

Comment 51 David Cook 2023-11-06 05:08:17 UTC

(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...

Comment 52 David Cook 2023-11-06 05:23:00 UTC

(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...

Comment 53 David Cook 2023-11-06 05:38:08 UTC

(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.)

Comment 54 Martin Renvoize (ashimema) 2023-11-06 09:42:56 UTC

Confirmed 35264 is the solution here.  PQA on that followup.

Comment 55 Tomás Cohen Arazi (tcohen) 2023-11-06 11:15:49 UTC

Thanks everyone.

Comment 56 Fridolin Somers 2023-11-08 08:38:10 UTC

Enhancement not pushed to 23.05.x