From f5775c05a0a21a5488c0fddd63985a3739c0d60f Mon Sep 17 00:00:00 2001
From: Magnus Enger <magnus@libriotech.no>
Date: Thu, 15 Jun 2023 12:21:19 +0000
Subject: [PATCH] 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
---
 C4/Members.pm                                 |  1 +
 Koha/Exceptions/Patron.pm                     |  4 ++++
 Koha/Patron.pm                                | 14 +++++++++++
 Koha/REST/V1/Patrons.pm                       |  1 +
 api/v1/swagger/definitions/patron.yaml        |  4 ++++
 api/v1/swagger/paths/patrons.yaml             |  6 +++++
 .../prog/en/includes/members-toolbar.inc      |  6 ++++-
 .../prog/en/modules/members/memberentrygen.tt | 23 +++++++++++++++++++
 .../prog/en/modules/members/moremember.tt     |  8 +++++++
 .../intranet-tmpl/prog/js/members-menu.js     |  6 ++++-
 t/db_dependent/Koha/Patron.t                  | 10 +++++++-
 t/db_dependent/Koha/Patrons.t                 | 16 +++++++++++--
 t/db_dependent/Members.t                      | 22 +++++++++++++++++-
 t/lib/TestBuilder.pm                          |  1 +
 14 files changed, 116 insertions(+), 6 deletions(-)

diff --git a/C4/Members.pm b/C4/Members.pm
index 919f6c3890..fa23992af9 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -337,6 +337,7 @@ sub GetBorrowersToExpunge {
     $query .= q| WHERE  category_type <> 'S'
         AND ( borrowers.flags IS NULL OR borrowers.flags = 0 )
         AND tmp.guarantor_id IS NULL
+        AND borrowers.protected = 0
     |;
     my @query_params;
     if ( $filterbranch && $filterbranch ne "" ) {
diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm
index 7df1702945..bc52bd8a1e 100644
--- a/Koha/Exceptions/Patron.pm
+++ b/Koha/Exceptions/Patron.pm
@@ -23,6 +23,10 @@ use Exception::Class (
         isa         => 'Koha::Exceptions::Patron',
         description => "Deleting patron failed, AnonymousPatron is not deleteable"
     },
+    'Koha::Exceptions::Patron::FailedDeleteProtectedPatron' => {
+        isa         => 'Koha::Exceptions::Patron',
+        description => "Deleting patron failed, patron is protected"
+    },
     'Koha::Exceptions::Patron::InvalidUserid' => {
         isa         => 'Koha::Exceptions::Patron',
         description => 'Field userid is not valid (probably not unique)',
diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 7321277234..4ffe3df4a3 100644
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -398,6 +398,9 @@ sub delete {
     my $anonymous_patron = C4::Context->preference("AnonymousPatron");
     Koha::Exceptions::Patron::FailedDeleteAnonymousPatron->throw() if $anonymous_patron && $self->id eq $anonymous_patron;
 
+    # Check if patron is protected
+    Koha::Exceptions::Patron::FailedDeleteProtectedPatron->throw() if $self->protected == 1;
+
     $self->_result->result_source->schema->txn_do(
         sub {
             # Cancel Patron's holds
@@ -627,6 +630,9 @@ sub merge_with {
     my $anonymous_patron = C4::Context->preference("AnonymousPatron");
     return if $anonymous_patron && $self->id eq $anonymous_patron;
 
+    # Do not merge other patrons into a protected patron
+    return if $self->protected;
+
     my @patron_ids = @{ $patron_ids };
 
     # Ensure the keeper isn't in the list of patrons to merge
@@ -645,6 +651,9 @@ sub merge_with {
 
             next unless $patron;
 
+            # Do not merge protected patrons into other patrons
+            next if $patron->protected;
+
             # Unbless for safety, the patron will end up being deleted
             $results->{merged}->{$patron_id}->{patron} = $patron->unblessed;
 
@@ -2466,6 +2475,8 @@ This method tells if the Koha:Patron object can be deleted. Possible return valu
 
 =item 'is_anonymous_patron'
 
+=item 'is_protected'
+
 =back
 
 =cut
@@ -2489,6 +2500,9 @@ sub safe_to_delete {
     elsif ( $self->guarantee_relationships->count ) {
         $error = 'has_guarantees';
     }
+    elsif ( $self->protected ) {
+        $error = 'is_protected';
+    }
 
     if ( $error ) {
         return Koha::Result::Boolean->new(0)->add_message({ message => $error });
diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patrons.pm
index d89d47e102..cb5c863afc 100644
--- a/Koha/REST/V1/Patrons.pm
+++ b/Koha/REST/V1/Patrons.pm
@@ -402,6 +402,7 @@ sub delete {
                 has_debt       => 'Pending debts prevent deletion',
                 has_guarantees => 'Patron is a guarantor and it prevents deletion',
                 is_anonymous_patron => 'Anonymous patron cannot be deleted',
+                is_protected => 'Protected patrons cannot be deleted',
             };
 
             if ( any { $error->message eq $_ } keys %{$error_descriptions} ) {
diff --git a/api/v1/swagger/definitions/patron.yaml b/api/v1/swagger/definitions/patron.yaml
index 7967722db7..edcf47beec 100644
--- a/api/v1/swagger/definitions/patron.yaml
+++ b/api/v1/swagger/definitions/patron.yaml
@@ -374,6 +374,10 @@ properties:
       - object
       - "null"
     description: Library of the patron
+  protected:
+    type:
+      - boolean
+    description: Protected status of the patron
 additionalProperties: false
 required:
   - surname
diff --git a/api/v1/swagger/paths/patrons.yaml b/api/v1/swagger/paths/patrons.yaml
index ff64e71bbb..cf8aa09122 100644
--- a/api/v1/swagger/paths/patrons.yaml
+++ b/api/v1/swagger/paths/patrons.yaml
@@ -339,6 +339,11 @@
         description: Search on login_attempts
         required: false
         type: string
+      - name: protected
+        in: query
+        description: Search on protected status
+        required: false
+        type: boolean
       - $ref: "../swagger.yaml#/parameters/match"
       - $ref: "../swagger.yaml#/parameters/order_by"
       - $ref: "../swagger.yaml#/parameters/page"
@@ -603,6 +608,7 @@
             * `has_debt`: The patron has pending debts
             * `has_guarantees`: The patron has guarantees
             * `is_anonymous_patron`: The system-wide anonymous patron cannot be deleted
+            * `is_protected`: Protected patrons cannot be deleted
         schema:
           $ref: "../swagger.yaml#/definitions/error"
       "500":
diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
index 8e277919a6..ddb03e62f3 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
@@ -84,7 +84,11 @@
                 [% END %]
 
                 [% IF CAN_user_borrowers_delete_borrowers %]
-                    <li><a id="deletepatron" href="#">Delete</a></li>
+                    [% IF ( patron.protected == 1 ) %]
+                        <li class="disabled"><a data-toggle="tooltip" data-placement="left" title="Patron is protected" id="deletepatron" href="#">Delete</a></li>
+                    [% ELSE %]
+                        <li><a id="deletepatron" href="#">Delete</a></li>
+                    [% END %]
                 [% ELSE %]
                     <li class="disabled"><a data-toggle="tooltip" data-placement="left" title="You are not authorized to delete patrons" id="deletepatron" href="#">Delete</a></li>
                 [% END %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
index e97a15fdc6..cc92309c94 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ -1027,6 +1027,29 @@ legend:hover {
                                             </li>
                                         [% END %]
 
+                                        <li class="radio">
+                                            <label for="protected">Protected:</label>
+                                            [% IF ( patron.protected == 1 ) %]
+                                                <label for="yes-protected">
+                                                    Yes
+                                                    <input type="radio" id="yes-protected" name="protected" value="1" checked="checked" />
+                                                </label>
+                                                <label for="no-protected">
+                                                    No
+                                                    <input type="radio" id="no-protected" name="protected" value="0" />
+                                                </label>
+                                            [% ELSE %]
+                                                <label for="yes-protected">
+                                                    Yes
+                                                    <input type="radio" id="yes-protected" name="protected" value="1" />
+                                                </label>
+                                                <label for="no-protected">
+                                                    No
+                                                    <input type="radio" id="no-protected" name="protected" value="0" checked="checked" />
+                                                </label>
+                                            [% END %]
+                                        </li>
+
                                         [% IF ( Koha.Preference('CheckPrevCheckout') == 'softyes' || Koha.Preference('CheckPrevCheckout') == 'softno' ) %]
                                             <li>
                                                 <label for="checkprevcheckout">Check for previous checkouts: </label>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
index 6facce6e5c..d0c3d47de0 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
@@ -476,6 +476,14 @@
                                                 [% END %]
                                             </li>
                                         [% END %]
+                                        <li id="patron-protected">
+                                            <span class="label">Protected: </span>
+                                            [% IF ( patron.protected == 1 ) %]
+                                                Yes
+                                            [% ELSE %]
+                                                No
+                                            [% END %]
+                                        </li>
 
                                         [% IF ( patron.borrowernotes ) %]
                                             <li id="patron-borrowernotes">
diff --git a/koha-tmpl/intranet-tmpl/prog/js/members-menu.js b/koha-tmpl/intranet-tmpl/prog/js/members-menu.js
index b020966e58..bb9b19a2b6 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/members-menu.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/members-menu.js
@@ -12,7 +12,11 @@ $(document).ready(function(){
 
     if( CAN_user_borrowers_delete_borrowers ){
         $("#deletepatron").click(function(){
-            window.location='/cgi-bin/koha/members/deletemem.pl?member=' + borrowernumber;
+            if( $(this).data("toggle") == "tooltip"){ // Disabled menu option has tooltip attribute
+                e.preventDefault();
+            } else {
+                window.location='/cgi-bin/koha/members/deletemem.pl?member=' + borrowernumber;
+            }
         });
     }
     if( CAN_user_borrowers_edit_borrowers ){
diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t
index ce59a5583b..2adbc77b6f 100755
--- a/t/db_dependent/Koha/Patron.t
+++ b/t/db_dependent/Koha/Patron.t
@@ -1483,7 +1483,7 @@ subtest 'password expiration tests' => sub {
 
 subtest 'safe_to_delete() tests' => sub {
 
-    plan tests => 14;
+    plan tests => 17;
 
     $schema->storage->txn_begin;
 
@@ -1539,6 +1539,14 @@ subtest 'safe_to_delete() tests' => sub {
     t::lib::Mocks::mock_userenv( { borrowernumber => $manager->id } );
     $patron->account->pay({ amount => 10, debits => [ $debit ] });
 
+    ## Make it protected
+    $patron->protected( 1 );
+    ok( !$patron->safe_to_delete, 'Cannot delete, is protected' );
+    $message = $patron->safe_to_delete->messages->[0];
+    is( $message->type, 'error', 'Type is error' );
+    is( $message->message, 'is_protected', 'Cannot delete, is protected' );
+    $patron->protected( 0 );
+
     ## Happy case :-D
     ok( $patron->safe_to_delete, 'Can delete, all conditions met' );
     my $messages = $patron->safe_to_delete->messages;
diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index 24d4f7a811..243bdf7f81 100755
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -1627,7 +1627,7 @@ subtest 'BorrowersLog tests' => sub {
 $schema->storage->txn_rollback;
 
 subtest 'Test Koha::Patrons::merge' => sub {
-    plan tests => 110;
+    plan tests => 113;
 
     my $schema = Koha::Database->new()->schema();
 
@@ -1639,6 +1639,13 @@ subtest 'Test Koha::Patrons::merge' => sub {
     my $loser_1 = $builder->build({ source => 'Borrower' })->{borrowernumber};
     my $loser_2 = $builder->build({ source => 'Borrower' })->{borrowernumber};
 
+    my $keeper_protected = $builder->build_object({ class => 'Koha::Patrons' });
+    $keeper_protected->protected( 1 )->store;
+
+    my $loser_protected_obj = $builder->build_object({ class => 'Koha::Patrons' });
+    $loser_protected_obj->protected( 1 )->store;
+    my $loser_protected = $loser_protected_obj->borrowernumber;
+
     my $anonymous_patron_orig = C4::Context->preference('AnonymousPatron');
     my $anonymous_patron = $builder->build({ source => 'Borrower' })->{borrowernumber};
     t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous_patron );
@@ -1661,7 +1668,11 @@ subtest 'Test Koha::Patrons::merge' => sub {
         is( $loser_2_rs->count(), 1, "Found 1 $r rows for loser_2" );
     }
 
-    my $results = $keeper->merge_with([ $loser_1, $loser_2 ]);
+    my $results_protected = $keeper_protected->merge_with( [ $loser_1 ] );
+    is( $results_protected, undef, "Protected patrons cannot have other patrons merged into them" );
+    is( Koha::Patrons->search( { borrowernumber => $loser_1 } )->count, 1, "Patron from attempted merge with protected patron still exists" );
+
+    my $results = $keeper->merge_with([ $loser_1, $loser_2, $loser_protected ]);
 
     while (my ($r, $field) = each(%$resultsets)) {
         my $keeper_rs =
@@ -1672,6 +1683,7 @@ subtest 'Test Koha::Patrons::merge' => sub {
     is( Koha::Patrons->find($loser_1), undef, 'Loser 1 has been deleted' );
     is( Koha::Patrons->find($loser_2), undef, 'Loser 2 has been deleted' );
     is( ref Koha::Patrons->find($anonymous_patron), 'Koha::Patron', 'Anonymous Patron was not deleted' );
+    is( ref Koha::Patrons->find($loser_protected), 'Koha::Patron', 'Protected patron was not deleted' );
 
     $anonymous_patron = Koha::Patrons->find($anonymous_patron);
     $results = $anonymous_patron->merge_with( [ $keeper->id ] );
diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t
index 1a314682c1..79aec68dad 100755
--- a/t/db_dependent/Members.t
+++ b/t/db_dependent/Members.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 50;
+use Test::More tests => 52;
 use Test::MockModule;
 use Test::Exception;
 
@@ -354,6 +354,26 @@ $patron->set({ flags => 4 })->store;
 $patstodel = GetBorrowersToExpunge( {category_code => 'SMALLSTAFF' } );
 is( scalar @$patstodel, 0, 'Regular patron with flags>0 can not be deleted' );
 
+# Test GetBorrowersToExpunge and patrons with "protected" status (borrowers.protected = 1)
+$builder->build({
+        source => 'Category',
+        value => {
+            categorycode         => 'PROTECTED',
+            description          => 'Protected',
+            category_type        => 'A',
+        },
+});
+$borrowernumber = Koha::Patron->new({
+    categorycode => 'PROTECTED',
+    branchcode => $library2->{branchcode},
+})->store->borrowernumber;
+$patron = Koha::Patrons->find( $borrowernumber );
+$patstodel = GetBorrowersToExpunge( {category_code => 'PROTECTED' } );
+is( scalar @$patstodel, 1, 'Patron with default protected status can be deleted' );
+$patron->set({ protected => 1 })->store;
+$patstodel = GetBorrowersToExpunge( {category_code => 'PROTECTED' } );
+is( scalar @$patstodel, 0, 'Patron with protected status set can not be deleted' );
+
 # Regression tests for BZ13502
 ## Remove all entries with userid='' (should be only 1 max)
 $dbh->do(q|DELETE FROM borrowers WHERE userid = ''|);
diff --git a/t/lib/TestBuilder.pm b/t/lib/TestBuilder.pm
index ff2450852b..785aad09ad 100644
--- a/t/lib/TestBuilder.pm
+++ b/t/lib/TestBuilder.pm
@@ -582,6 +582,7 @@ sub _gen_default_values {
 
             password_expiration_date => undef,
             anonymized               => 0,
+            protected      => 0,
         },
         Item => {
             notforloan         => 0,
-- 
2.34.1