Bugzilla – Attachment 157310 Details for
Bug 26170
Add protected status for patrons
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26170: Add protected status for patrons
Bug-26170-Add-protected-status-for-patrons.patch (text/plain), 19.06 KB, created by
Emily-Rose Francoeur
on 2023-10-18 12:44:19 UTC
(
hide
)
Description:
Bug 26170: Add protected status for patrons
Filename:
MIME Type:
Creator:
Emily-Rose Francoeur
Created:
2023-10-18 12:44:19 UTC
Size:
19.06 KB
patch
obsolete
>From 3dc85c5060433321b51859ca9df5b487e413441c 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! >--- > 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 %] >- <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 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 %] > </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/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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=157310">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26170
:
154602
|
154603
|
154604
|
157247
|
157248
|
157249
|
157250
|
157309
|
157310
|
157311
|
157391
|
157392
|
157393
|
157395
|
157396
|
157397
|
157426
|
157427
|
157428
|
157629
|
157630
|
157631