@@ -, +, @@ - Koha::Patron::delete - Koha::Patron::merge_with - Koha::Patron::safe_to_delete - C4::Members::GetBorrowersToExpunge - This does not intend to protect patrons from being edited, only from being deleted - Run the affected tests: prove t/db_dependent/Members.t prove t/db_dependent/Koha/Patrons.t - 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 - 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 - 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". - "No valid patrons to merge were found" should be shown - Repeat this with the other patron as the "patron to keep" - 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 - 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"} --- 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 | 1 + 13 files changed, 111 insertions(+), 5 deletions(-) --- a/C4/Members.pm +++ a/C4/Members.pm @@ -399,6 +399,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 "" ) { --- a/Koha/Exceptions/Patron.pm +++ a/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)', --- a/Koha/Patron.pm +++ a/Koha/Patron.pm @@ -395,6 +395,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 @@ -624,6 +627,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 @@ -642,6 +648,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; @@ -2257,6 +2266,8 @@ This method tells if the Koha:Patron object can be deleted. Possible return valu =item 'is_anonymous_patron' +=item 'is_protected' + =back =cut @@ -2280,6 +2291,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 }); --- a/Koha/REST/V1/Patrons.pm +++ a/Koha/REST/V1/Patrons.pm @@ -366,6 +366,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} ) { --- a/api/v1/swagger/definitions/patron.yaml +++ a/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 --- a/api/v1/swagger/paths/patrons.yaml +++ a/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" @@ -593,6 +598,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": --- a/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc +++ a/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 %] --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -1025,6 +1025,29 @@ legend:hover { [% END %] +
  • + + [% IF ( patron.protected == 1 ) %] + + + [% ELSE %] + + + [% END %] +
  • + [% IF ( Koha.Preference('CheckPrevCheckout') == 'softyes' || Koha.Preference('CheckPrevCheckout') == 'softno' ) %]
  • --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ -468,6 +468,14 @@ [% END %]
  • [% END %] +
  • + Protected: + [% IF ( patron.protected == 1 ) %] + Yes + [% ELSE %] + No + [% END %] +
  • [% IF ( patron.borrowernotes ) %]
  • --- a/t/db_dependent/Koha/Patron.t +++ a/t/db_dependent/Koha/Patron.t @@ -1063,7 +1063,7 @@ subtest 'password expiration tests' => sub { subtest 'safe_to_delete() tests' => sub { - plan tests => 14; + plan tests => 17; $schema->storage->txn_begin; @@ -1117,6 +1117,14 @@ subtest 'safe_to_delete() tests' => sub { # cleanup $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; --- a/t/db_dependent/Koha/Patrons.t +++ a/t/db_dependent/Koha/Patrons.t @@ -1614,7 +1614,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(); @@ -1626,6 +1626,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 ); @@ -1648,7 +1655,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 = @@ -1659,6 +1670,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 ] ); --- a/t/db_dependent/Members.t +++ a/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 53; +use Test::More tests => 55; use Test::MockModule; use Test::Exception; @@ -368,6 +368,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 = ''|); --- a/t/lib/TestBuilder.pm +++ a/t/lib/TestBuilder.pm @@ -580,6 +580,7 @@ sub _gen_default_values { borrowernotes => '', secret => undef, password_expiration_date => undef, + protected => 0, }, Item => { notforloan => 0, --