Bugzilla – Attachment 26488 Details for
Bug 11352
Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete
Bug-11352---Batch-Patron-DeletionAnonmyzation-dele.patch (text/plain), 8.85 KB, created by
Jonathan Druart
on 2014-03-20 15:38:49 UTC
(
hide
)
Description:
Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2014-03-20 15:38:49 UTC
Size:
8.85 KB
patch
obsolete
>From 0c6370d20a0c6c76d7f46c92f58c42598295f59d Mon Sep 17 00:00:00 2001 >From: Jacek Ablewicz <abl@biblos.pk.edu.pl> >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. > >Signed-off-by: Magnus Enger <digitalutvikling@gmail.com> >Tested with dateformat = dd/mm/yyyy. I tested with two expired patrons, >one with fines and one without. Before the patch a lot of unexpected >patrons were deleted along with the expected ones. After >applying the patch only the expired patron was deleted, not the >one with fines. The test run and the "real" run reported correct numbers. > >The patch also makes sure no patron categories with category_type = S >are suggested for batch deletion. > >Note: The ergonomics of the "Batch delete/anonymize" tool is hardly >optimal, but this patch fixes a real, data-loosing bug, so let's >deal with the ergonomics later. > >Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> >--- > .../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 @@ > <select id="borrower_categorycode" name="borrower_categorycode"> > <option value="" selected="selected">Any</option> > [% FOREACH bc IN borrower_categorycodes %] >- [% UNLESS bc.categorycode == 'S' %] >+ [% UNLESS ( bc.categorycode == 'S' || bc.category_type == 'S' ) %] > <option value="[% bc.categorycode %]">[% bc.description %]</option> > [% END %] > [% END %] >@@ -119,11 +119,14 @@ > <br /> > [% IF ( totalToDelete ) %] > <fieldset><legend>What do you want to do for deleted patrons?</legend> >+ <input id="delete" type="radio" name="radio" value="delete" /> > <label for="delete">Permanently delete these patrons</label> >- <input id="delete" type="radio" name="radio" value="delete" checked="checked" /> > >+ <br /><input id="trash" type="radio" name="radio" value="trash" /> > <label for="trash">Move these patrons to the trash</label> >- <input id="trash" type="radio" name="radio" value="trash" /> >+ >+ <br /><input id="testrun" type="radio" name="radio" value="testrun" checked="checked" /> >+ <label for="testrun">Do not remove any patrons (test run)</label> > <input type="hidden" name="do_delete" value="[% totalToDelete %]" /></fieldset> > > [% END %] >@@ -148,15 +151,19 @@ > <!-- Step 3 START --> > > <div id="step3"> >- >- [% IF ( do_delete ) %] >- [% IF ( trash ) %] >- <h4>[% TotalDel %] patrons have been successfully moved to trash</h4> >+ [% IF ( testrun ) %] >+ <h4>[% TotalDel %] patrons would have been removed (if it wasn't a test run)</h4> >+ <h4>No patron records have been actually removed</h4> >+ [% ELSE %] >+ [% IF ( do_delete ) %] >+ [% IF ( trash ) %] >+ <h4>[% TotalDel %] patrons have been successfully moved to trash</h4> >+ [% ELSE %] >+ <h4>[% TotalDel %] patrons have been successfully deleted</h4> >+ [% END %] > [% ELSE %] >- <h4>[% TotalDel %] patrons have been successfully deleted</h4> >+ <h4>No patron records have been removed</h4> > [% END %] >- [% ELSE %] >- <h4>No patron records have been removed</h4> > [% END %] > [% IF ( do_anonym ) %] > <h4>All patrons with checkouts older than [% filterdate1 %] have been anonymized</h4> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 11352
:
23346
|
25594
|
26047
| 26488