gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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>
This commit is contained in:
Magnus Enger 2023-06-15 12:21:19 +00:00 committed by Tomas Cohen Arazi
parent 965797092e
commit 6fec4ea0df
No known key found for this signature in database
GPG Key ID: 0A272EA1B2F3C15F
14 changed files with 126 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
C4/Members.pm
View File

@ -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 "" ) {

4
Koha/Exceptions/Patron.pm
View File

@ -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)',

14
Koha/Patron.pm
View File

@ -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 });

7
Koha/REST/V1/Patrons.pm
View File

@ -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} ) {

4
api/v1/swagger/definitions/patron.yaml
View File

@ -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

6
api/v1/swagger/paths/patrons.yaml
View File

@ -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":

6
koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
View File

@ -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 %]

23
koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
View File

@ -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>

8
koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
View File

@ -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">

6
koha-tmpl/intranet-tmpl/prog/js/members-menu.js
View File

@ -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 ){

10
t/db_dependent/Koha/Patron.t
View File

@ -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;

19
t/db_dependent/Koha/Patrons.t
View File

@ -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 ] );

26
t/db_dependent/Members.t
View File

@ -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 = ''|);

1
t/lib/TestBuilder.pm
View File

@ -582,6 +582,7 @@ sub _gen_default_values {
password_expiration_date => undef,
anonymized => 0,
protected => 0,
},
Item => {
notforloan => 0,