From 4e7e8a5544297aa6d02eb86b4a621e25a19e9e0b Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Thu, 20 Mar 2014 16:41:29 +0100
Subject: [PATCH] Bug 11975: improve the batch patron deletion code

This patch tries to improve the code for the cleanborrowers.pl tool.

- use KohaDates plugin and Koha::DateUtils for date management,
  removing a dependency on C4::Dates
- replace variables step1, step2 and step3 with step
- add a JavaScript check if no action if checked

Test plan:
- Backup your DB
- Play with the Patron deletion/anonymisation tool and try to find
  something inconsistent.
If you don't find something different, it works!

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
---
 .../prog/en/modules/tools/cleanborrowers.tt   |  33 +++--
 tools/cleanborrowers.pl                       | 132 ++++++++++++------
 2 files changed, 110 insertions(+), 55 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 8a141876b2..bd505fbb29 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt
@@ -1,5 +1,6 @@
+[% USE KohaDates %]
 [% INCLUDE 'doc-head-open.inc' %]
-<title>Koha &rsaquo; Tools &rsaquo; Batch patron deletion/anonymization [% IF ( step2 ) %]&rsaquo; Confirm[% END %][% IF ( step3 ) %]&rsaquo; Finished[% END %]</title>
+<title>Koha &rsaquo; Tools &rsaquo; Batch patron deletion/anonymization [% IF step == 2 %]&rsaquo; Confirm[% END %][% IF step == 3 %]&rsaquo; Finished[% END %]</title>
 [% INCLUDE 'doc-head-close.inc' %]
 [% INCLUDE 'calendar.inc' %]
 <script type="text/javascript">
@@ -21,9 +22,13 @@
                       return false;
                   }
               }
+              if(!form.checkbox[0].checked && !form.checkbox[1].checked) {
+                alert( _("Please check at least one action") );
+                return false;
+              }
               return true;
           }
-          
+
         /**
          *  checkForm2(form)
          *  This function check the form2 is correctly filled.
@@ -39,7 +44,7 @@
 [% INCLUDE 'header.inc' %]
 [% INCLUDE 'cat-search.inc' %]
 
-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a>  &rsaquo; [% IF ( step1 ) %]Clean Patron Records[% ELSE %]<a href="/cgi-bin/koha/tools/cleanborrowers.pl">Clean patron records</a> &rsaquo; [% END %][% IF ( step2 ) %]Confirm[% END %][% IF ( step3 ) %]Finished[% END %]</div>
+<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a>  &rsaquo; [% IF step == 1 %]Clean Patron Records[% ELSE %]<a href="/cgi-bin/koha/tools/cleanborrowers.pl">Clean patron records</a> &rsaquo; [% END %][% IF step == 2 %]Confirm[% END %][% IF step == 3 %]Finished[% END %]</div>
 
 <div id="doc3" class="yui-t2">
 
@@ -51,7 +56,7 @@
 <div class="help">
     <p>This tool allows you to delete patrons and anonymize checkout history. For deleting patrons, any combination of limits can be used.</p>
 </div>
-[% IF ( step1 ) %]
+[% IF step == 1 %]
 <!-- step 1 START -->
 <div id="step1">
     <form name="f1" onsubmit="return checkForm(this);" action="/cgi-bin/koha/tools/cleanborrowers.pl" method="post">
@@ -63,7 +68,7 @@
         <ul>
                 <li>
                     <label for="date1">who have not borrowed since:</label>
-                    <input size="10" id="date1" name="filterdate1" type="text" class="datepicker" />
+                    <input size="10" id="date1" name="not_borrowed_since" type="text" class="datepicker" />
                     <span class="hint">[% INCLUDE 'date-format.inc' %]</span>
                 </li>
                 <li>
@@ -91,13 +96,13 @@
         <ul>
             <li>
                 <label for="date2">Permanently delete checkout history older than</label>
-                <input size="10" id="date2" name="filterdate2" type="text" class="datepicker" />
+                <input size="10" id="date2" name="last_issue_date" type="text" class="datepicker" />
                 <span class="hint">[% INCLUDE 'date-format.inc' %]</span>
             </li>
         </ul>
 
             <!-- hidden here -->
-            <input type="hidden" name="step2" value="1" />
+            <input type="hidden" name="step" value="2" />
             </fieldset>
             <fieldset class="action"><input type="submit" value="Next &gt;&gt;" /></fieldset>
     </form>
@@ -105,7 +110,7 @@
 <!-- step 1 END -->
 [% END %]
 
-[% IF ( step2 ) %]
+[% IF step == 2 %]
 <!-- STEP 2 START -->
 <div id="step2">
         <form name="f2" action="/cgi-bin/koha/tools/cleanborrowers.pl" method="post" onsubmit="return checkForm2(this);">
@@ -135,10 +140,10 @@
                 <input type="hidden" name="do_anonym" value="[% totalToAnonymize %]" />
             [% END %]
 
-            <input type="hidden" name="step3" value="1" />
-            <input type="hidden" name="filterdate1" value="[% filterdate1 %]" />
-            <input type="hidden" name="filterdate2" value="[% filterdate2 %]" />
-            <input type="hidden" name="borrower_dateexpiry" value="[% borrower_dateexpiry %]" />
+            <input type="hidden" name="step" value="3" />
+            <input type="hidden" name="not_borrowed_since" value="[% not_borrowed_since | $KohaDates %]" />
+            <input type="hidden" name="last_issue_date" value="[% last_issue_date | $KohaDates %]" />
+            <input type="hidden" name="borrower_dateexpiry" value="[% borrower_dateexpiry | $KohaDates %]" />
             <input type="hidden" name="borrower_categorycode" value="[% borrower_categorycode %]" />
     </fieldset>
     <fieldset class="action"><input type="submit" value="Finish" /> <a class="cancel" href="/cgi-bin/koha/tools/cleanborrowers.pl">Cancel</a></fieldset>
@@ -147,7 +152,7 @@
 <!-- STEP 2 END -->
 [% END %]
 
-[% IF ( step3 ) %]
+[% IF step == 3 %]
 <!-- Step 3 START -->
 
     <div id="step3">
@@ -166,7 +171,7 @@
             [% END %]
         [% END %]
         [% IF ( do_anonym ) %]
-            <h4>All patrons with checkouts older than [% filterdate1 %] have been anonymized</h4>
+            <h4>All patrons with checkouts older than [% last_issue_date | $KohaDates %] have been anonymized</h4>
         [% ELSE %]
             <h4>No patron records have been anonymized</h4>
         [% END %]
diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl
index 9c1977b96b..f6ebcc29ae 100755
--- a/tools/cleanborrowers.pl
+++ b/tools/cleanborrowers.pl
@@ -37,10 +37,10 @@ use strict;
 use CGI;
 use C4::Auth;
 use C4::Output;
-use C4::Dates qw/format_date format_date_in_iso/;
 use C4::Members;        # GetBorrowersWhoHavexxxBorrowed.
 use C4::Circulation;    # AnonymiseIssueHistory.
 use C4::VirtualShelves ();    #no import
+use Koha::DateUtils qw( dt_from_string output_pref );
 use Date::Calc qw/Today Add_Delta_YM/;
 
 my $cgi = new CGI;
@@ -51,10 +51,21 @@ my $cgi = new CGI;
 #  * multivalued CGI paramaters are returned as a packaged string separated by "\0" (null)
 my $params = $cgi->Vars;
 
-my $filterdate1;              # the date which filter on issue history.
-my $filterdate2;              # the date which filter on borrowers last issue.
-my $borrower_dateexpiry;
-my $borrower_categorycode;
+my $step = $params->{step} || 1;
+my $not_borrowered_since =    # the date which filter on issue history.
+  $params->{not_borrowered_since}
+  ? dt_from_string $params->{not_borrowered_since}
+  : undef;
+my $last_issue_date =         # the date which filter on borrowers last issue.
+  $params->{last_issue_date}
+  ? dt_from_string $params->{last_issue_date}
+  : undef;
+my $borrower_dateexpiry =
+  $params->{borrower_dateexpiry}
+  ? dt_from_string $params->{borrower_dateexpiry}
+  : undef;
+
+my $borrower_categorycode = $params->{'borrower_categorycode'} || q{};
 
 # getting the template
 my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
@@ -66,19 +77,48 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
     }
 );
 
-if ( $params->{'step2'} ) {
-    $filterdate1           = format_date_in_iso( $params->{'filterdate1'} );
-    $filterdate2           = format_date_in_iso( $params->{'filterdate2'} );
-    $borrower_dateexpiry   = format_date_in_iso( $params->{'borrower_dateexpiry'} );
-    $borrower_categorycode = $params->{'borrower_categorycode'};
+if ( $step == 2 ) {
 
     my %checkboxes = map { $_ => 1 } split /\0/, $params->{'checkbox'};
 
     my $totalDel;
     my $membersToDelete;
     if ( $checkboxes{borrower} ) {
-        $membersToDelete =
-          GetBorrowersToExpunge( { not_borrowered_since => $filterdate1, expired_before => $borrower_dateexpiry, category_code => $borrower_categorycode } );
+        $membersToDelete = GetBorrowersToExpunge(
+            {
+                (
+                    $not_borrowered_since
+                    ? (
+                        not_borrowered_since => output_pref(
+                            {
+                                dt         => $not_borrowered_since,
+                                dateformat => 'iso',
+                                dateonly   => 1
+                            }
+                        )
+                      )
+                    : ()
+                ),
+                (
+                    $borrower_dateexpiry
+                    ? (
+                        expired_before => output_pref(
+                            {
+                                dt         => $borrower_dateexpiry,
+                                dateformat => 'iso',
+                                dateonly   => 1
+                            }
+                        )
+                      )
+                    : ()
+                ),
+                (
+                    $borrower_categorycode
+                    ? ( category_code => $borrower_categorycode )
+                    : ()
+                ),
+            }
+        );
         _skip_borrowers_with_nonzero_balance( $membersToDelete );
         $totalDel = scalar @$membersToDelete;
 
@@ -86,33 +126,19 @@ if ( $params->{'step2'} ) {
     my $totalAno;
     my $membersToAnonymize;
     if ( $checkboxes{issue} ) {
-        $membersToAnonymize = GetBorrowersWithIssuesHistoryOlderThan($filterdate2);
+        $membersToAnonymize = GetBorrowersWithIssuesHistoryOlderThan($last_issue_date);
         $totalAno           = scalar @$membersToAnonymize;
     }
 
     $template->param(
-        step2                   => 1,
         totalToDelete           => $totalDel,
         totalToAnonymize        => $totalAno,
         memberstodelete_list    => $membersToDelete,
         memberstoanonymize_list => $membersToAnonymize,
-        filterdate1             => format_date($filterdate1),
-        filterdate2             => format_date($filterdate2),
-        borrower_dateexpiry     => format_date($borrower_dateexpiry),
-        borrower_categorycode   => $borrower_categorycode,
     );
-
-    ### TODO : Use GetBorrowersNamesAndLatestIssue function in order to get the borrowers to delete or anonymize.
-    output_html_with_http_headers $cgi, $cookie, $template->output;
-    exit;
 }
 
-if ( $params->{'step3'} ) {
-    $filterdate1           = format_date_in_iso( $params->{'filterdate1'} );
-    $filterdate2           = format_date_in_iso( $params->{'filterdate2'} );
-    $borrower_dateexpiry   = format_date_in_iso( $params->{'borrower_dateexpiry'} );
-    $borrower_categorycode = $params->{'borrower_categorycode'};
-
+elsif ( $step == 3 ) {
     my $do_delete = $params->{'do_delete'};
     my $do_anonym = $params->{'do_anonym'};
 
@@ -120,8 +146,36 @@ if ( $params->{'step3'} ) {
 
     # delete members
     if ($do_delete) {
-        my $membersToDelete =
-          GetBorrowersToExpunge( { not_borrowered_since => $filterdate1, expired_before => $borrower_dateexpiry, category_code => $borrower_categorycode } );
+        my $membersToDelete = GetBorrowersToExpunge(
+            {
+                (
+                    $not_borrowered_since
+                    ? (
+                        not_borrowered_since => output_pref(
+                            {
+                                dt         => $not_borrowered_since,
+                                dateformat => 'iso'
+                            }
+                        )
+                      )
+                    : ()
+                ),
+                (
+                    $borrower_dateexpiry
+                    ? (
+                        expired_before => output_pref(
+                            { dt => $borrower_dateexpiry, dateformat => 'iso' }
+                        )
+                      )
+                    : ()
+                ),
+                (
+                    $borrower_categorycode
+                    ? ( category_code => $borrower_categorycode )
+                    : ()
+                ),
+            }
+        );
         _skip_borrowers_with_nonzero_balance( $membersToDelete );
         $totalDel = scalar(@$membersToDelete);
         $radio    = $params->{'radio'};
@@ -141,29 +195,25 @@ if ( $params->{'step3'} ) {
     # Anonymising all members
     if ($do_anonym) {
         #FIXME: anonymisation errors are not handled
-        ($totalAno,my $anonymisation_error) = AnonymiseIssueHistory($filterdate2);
+        ($totalAno,my $anonymisation_error) = AnonymiseIssueHistory($last_issue_date);
         $template->param(
-            filterdate1 => $filterdate2,
             do_anonym   => '1',
         );
     }
 
     $template->param(
-        step3 => '1',
         trash => ( $radio eq "trash" ) ? (1) : (0),
         testrun => ( $radio eq "testrun" ) ? 1: 0,
     );
-
-    #writing the template
-    output_html_with_http_headers $cgi, $cookie, $template->output;
-    exit;
 }
 
 $template->param(
-    step1                    => '1',
-    filterdate1              => $filterdate1,
-    filterdate2              => $filterdate2,
-    borrower_categorycodes   => GetBorrowercategoryList(),
+    step                   => $step,
+    not_borrowered_since   => $not_borrowered_since,
+    borrower_dateexpiry    => $borrower_dateexpiry,
+    last_issue_date        => $last_issue_date,
+    borrower_categorycodes => GetBorrowercategoryList(),
+    borrower_categorycode  => $borrower_categorycode,
 );
 
 #writing the template
-- 
2.39.5