From eb1a4c04e30681f5064b5be59dbf67f7acfcbfcf Mon Sep 17 00:00:00 2001 From: Jacek Ablewicz Date: Mon, 24 Feb 2014 17:37:43 +0100 Subject: [PATCH] Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete Batch patron deletion/anonymization tool has some issues: 1) If 'dateformat' setting in I18N/L10N system preferences is set to anything other then 'iso' (eg. 'metric'), bulk deletion when using 'expiration date is before' criterion is not working properly. Date entered in this field will be efectivelly ignored (or possibly sometimes wrongly interpreted as different date, in other format) on the final patron deletion stage. This may result in deleting (or moving to trash) more borrower records then intended. 2) Bulk/batch patron deletion should skip borrowers with nonzero account balance (ones with oustanding fines or credits) 3) This tool shouldn't offer to choose as deletion criterion those patron categories which have category_type set to 'S' (= staff patron categories) This patch fixes abovementioned problems. It also adds an option to "test run" patron batch deletion, and makes this option the default choice in "warning" stage. Test plan: - prepare test database with some patron records (at least 2, the more the better) set up in such a way that they will be vulnerable to issues 1 & 2 - confirm issues 1,2 - restore test database - apply patch - ensure issues 1 & 2 are no longer present - first by using new "test run" option: for #1, record counts in "warning" stage and "final" stage should be now the same; for #2, observe that patron records with nonzero balance are now excluded from deletion - redo the tests, this time choosing "delete permanently" and "move to trash" instead of "test run" - test #3 by changing "Category type" to "S" in some test patron categories - after that, those categories should no longer be choosable as deletion criteria. --- .../prog/en/modules/tools/cleanborrowers.tt | 27 ++++++++++------ tools/cleanborrowers.pl | 33 +++++++++++--------- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt index 7e1fc97..8a14187 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt @@ -76,7 +76,7 @@ - +
- + +
+ [% END %] @@ -148,15 +151,19 @@
- - [% IF ( do_delete ) %] - [% IF ( trash ) %] -

[% TotalDel %] patrons have been successfully moved to trash

+ [% IF ( testrun ) %] +

[% TotalDel %] patrons would have been removed (if it wasn't a test run)

+

No patron records have been actually removed

+ [% ELSE %] + [% IF ( do_delete ) %] + [% IF ( trash ) %] +

[% TotalDel %] patrons have been successfully moved to trash

+ [% ELSE %] +

[% TotalDel %] patrons have been successfully deleted

+ [% END %] [% ELSE %] -

[% TotalDel %] patrons have been successfully deleted

+

No patron records have been removed

[% END %] - [% ELSE %] -

No patron records have been removed

[% END %] [% IF ( do_anonym ) %]

All patrons with checkouts older than [% filterdate1 %] have been anonymized

diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index 0d48b99..9c1977b 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -79,6 +79,7 @@ if ( $params->{'step2'} ) { if ( $checkboxes{borrower} ) { $membersToDelete = GetBorrowersToExpunge( { not_borrowered_since => $filterdate1, expired_before => $borrower_dateexpiry, category_code => $borrower_categorycode } ); + _skip_borrowers_with_nonzero_balance( $membersToDelete ); $totalDel = scalar @$membersToDelete; } @@ -97,7 +98,7 @@ if ( $params->{'step2'} ) { memberstoanonymize_list => $membersToAnonymize, filterdate1 => format_date($filterdate1), filterdate2 => format_date($filterdate2), - borrower_dateexpiry => $borrower_dateexpiry, + borrower_dateexpiry => format_date($borrower_dateexpiry), borrower_categorycode => $borrower_categorycode, ); @@ -121,21 +122,15 @@ if ( $params->{'step3'} ) { if ($do_delete) { my $membersToDelete = GetBorrowersToExpunge( { not_borrowered_since => $filterdate1, expired_before => $borrower_dateexpiry, category_code => $borrower_categorycode } ); + _skip_borrowers_with_nonzero_balance( $membersToDelete ); $totalDel = scalar(@$membersToDelete); $radio = $params->{'radio'}; - if ( $radio eq 'trash' ) { - my $i; - for ( $i = 0 ; $i < $totalDel ; $i++ ) { - MoveMemberToDeleted( $membersToDelete->[$i]->{'borrowernumber'} ); - C4::VirtualShelves::HandleDelBorrower( $membersToDelete->[$i]->{'borrowernumber'} ); - DelMember( $membersToDelete->[$i]->{'borrowernumber'} ); - } - } else { # delete completly. - my $i; - for ( $i = 0 ; $i < $totalDel ; $i++ ) { - C4::VirtualShelves::HandleDelBorrower( $membersToDelete->[$i]->{'borrowernumber'} ); - DelMember( $membersToDelete->[$i]->{'borrowernumber'} ); - } + for ( my $i = 0 ; $i < $totalDel ; $i++ ) { + $radio eq 'testrun' && last; + my $borrowernumber = $membersToDelete->[$i]->{'borrowernumber'}; + $radio eq 'trash' && MoveMemberToDeleted( $borrowernumber ); + C4::VirtualShelves::HandleDelBorrower( $borrowernumber ); + DelMember( $borrowernumber ); } $template->param( do_delete => '1', @@ -156,6 +151,7 @@ if ( $params->{'step3'} ) { $template->param( step3 => '1', trash => ( $radio eq "trash" ) ? (1) : (0), + testrun => ( $radio eq "testrun" ) ? 1: 0, ); #writing the template @@ -172,3 +168,12 @@ $template->param( #writing the template output_html_with_http_headers $cgi, $cookie, $template->output; + +sub _skip_borrowers_with_nonzero_balance { + my $borrowers = shift; + my $balance; + @$borrowers = map { + (undef, undef, $balance) = GetMemberIssuesAndFines( $_->{borrowernumber} ); + ($balance != 0) ? (): ($_); + } @$borrowers; +} -- 1.7.10.4