From 6fec4ea0df18669f0a14d9edafbf63d25099aa45 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

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
---
 C4/Members.pm                                 |  1 +
 Koha/Exceptions/Patron.pm                     |  4 +++
 Koha/Patron.pm                                | 14 ++++++++++
 Koha/REST/V1/Patrons.pm                       |  7 ++---
 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                 | 19 ++++++++++++--
 t/db_dependent/Members.t                      | 26 ++++++++++++++++++-
 t/lib/TestBuilder.pm                          |  1 +
 14 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/C4/Members.pm b/C4/Members.pm
index e6a2e2a0ca..8ecdad3e73 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -282,6 +282,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 b466276ff0..e5a2798bc1 100644
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -401,6 +401,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
@@ -630,6 +633,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
@@ -648,6 +654,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;
 
@@ -2491,6 +2500,8 @@ This method tells if the Koha:Patron object can be deleted. Possible return valu
 
 =item 'is_anonymous_patron'
 
+=item 'is_protected'
+
 =back
 
 =cut
@@ -2514,6 +2525,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 903467f8ba..24aba7fa9f 100644
--- a/Koha/REST/V1/Patrons.pm
+++ b/Koha/REST/V1/Patrons.pm
@@ -403,10 +403,11 @@ sub delete {
             }
 
             my $error_descriptions = {
-                has_checkouts  => 'Pending checkouts prevent deletion',
-                has_debt       => 'Pending debts prevent deletion',
-                has_guarantees => 'Patron is a guarantor and it prevents deletion',
+                has_checkouts       => 'Pending checkouts prevent deletion',
+                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 8374ee944e..007e101206 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 f070c3eb97..3a1278e3cf 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 c19650b6a9..5554a11413 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 aea6e5b87c..12879efe7c 100755
--- a/t/db_dependent/Koha/Patron.t
+++ b/t/db_dependent/Koha/Patron.t
@@ -1446,7 +1446,7 @@ subtest 'password expiration tests' => sub {
 
 subtest 'safe_to_delete() tests' => sub {
 
-    plan tests => 14;
+    plan tests => 17;
 
     $schema->storage->txn_begin;
 
@@ -1502,6 +1502,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 5cf513c2d4..a9df49df55 100755
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -1631,7 +1631,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();
 
@@ -1643,6 +1643,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 );
@@ -1665,7 +1672,14 @@ 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 =
@@ -1676,6 +1690,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 ad679c67a5..70f4403cd5 100755
--- a/t/db_dependent/Members.t
+++ b/t/db_dependent/Members.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 49;
+use Test::More tests => 51;
 use Test::MockModule;
 use Test::Exception;
 
@@ -352,6 +352,30 @@ $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..91fb518494 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.39.5