From 27633df98df530e5cf459c5b04d045542b277458 Mon Sep 17 00:00:00 2001 From: Magnus Enger 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 -v --confirm (Replace 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! --- 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 +++++++ t/db_dependent/Koha/Patron.t | 10 +++++++- t/db_dependent/Koha/Patrons.t | 16 +++++++++++-- t/db_dependent/Members.t | 22 +++++++++++++++++- t/lib/TestBuilder.pm | 2 +- 13 files changed, 111 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 %] -
  • Delete
  • + [% IF ( patron.protected == 1 ) %] +
  • Delete
  • + [% ELSE %] +
  • Delete
  • + [% END %] [% ELSE %]
  • Delete
  • [% 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 { [% END %] +
  • + + [% IF ( patron.protected == 1 ) %] + + + [% ELSE %] + + + [% END %] +
  • + [% IF ( Koha.Preference('CheckPrevCheckout') == 'softyes' || Koha.Preference('CheckPrevCheckout') == 'softno' ) %]
  • 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 639d2a6547..26e28d1ea9 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ -471,6 +471,14 @@ [% END %]
  • [% END %] +
  • + Protected: + [% IF ( patron.protected == 1 ) %] + Yes + [% ELSE %] + No + [% END %] +
  • [% IF ( patron.borrowernotes ) %]
  • diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index 44c1726a5d..ecda49f0d8 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 7dceafd1bb..b323b380eb 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; @@ -349,6 +349,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..b8ba43489c 100644 --- a/t/lib/TestBuilder.pm +++ b/t/lib/TestBuilder.pm @@ -579,9 +579,9 @@ sub _gen_default_values { debarred => undef, borrowernotes => '', secret => undef, - password_expiration_date => undef, anonymized => 0, + protected => 0, }, Item => { notforloan => 0, -- 2.34.1