Bug 6506: When AnonymousPatron not set, deletion of issue history silently failed.

Remedied by:
- in Circulation.pm changing AnonymiseIssueHistory so that it returns ($rows, $err_history_not_deleted) instead of $rows
- consequential change to misc/cronjobs/batch_anonymise.pl to handle updated return value, and fail if there is an error
- consequential change to tools/cleanborrowers.pl although this still fails silently (raised as bug 9944)
- update of opac-privacy.pl to check return value and pass on error
- update of opac-privacy.tt to display error if appropriate

Note bug 9942 remains unfixed, which is a similar issue upon issue return.

To test:
1. OPAC
- enable privacy mode (preference OpacPrivacy)
- leave anonymous patron set to zero (preference AnonymousPatron)
- attempt to delete user history
- observe error
- check history - still there
- change anonymous patron to a valid user
- attempt to delete user history
- observe success message
- check history - gone

2. cleanborrowers.pl
- test it functions as before.  bug 9944 has been raised for it continuing to silently fail.

3. batch_anonymise.pl
- enable privacy mode (preference OpacPrivacy)
- leave anonymous patron set to zero (preference AnonymousPatron)
- run script (I use --days -1 for testing)
- script should fail with a Carp message
- change anonymous patron to a valid user
- run script as before
- script returns quietly
- check history - gone

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 3fc124f..1de40e8 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1967,6 +1967,7 @@ sub MarkIssueReturned {
     if ( $privacy == 2) {
         # The default of 0 does not work due to foreign key constraints
         # The anonymisation will fail quietly if AnonymousPatron is not a valid entry
+        # FIXME the above is unacceptable - bug 9942 relates
         my $anonymouspatron = (C4::Context->preference('AnonymousPatron')) ? C4::Context->preference('AnonymousPatron') : 0;
         my $sth_ano = $dbh->prepare("UPDATE old_issues SET borrowernumber=?
                                   WHERE borrowernumber = ?
@@ -2821,7 +2822,7 @@ sub DeleteTransfer {
 
 =head2 AnonymiseIssueHistory
 
-  $rows = AnonymiseIssueHistory($date,$borrowernumber)
+  ($rows,$err_history_not_deleted) = AnonymiseIssueHistory($date,$borrowernumber)
 
 This function write NULL instead of C<$borrowernumber> given on input arg into the table issues.
 if C<$borrowernumber> is not set, it will delete the issue history for all borrower older than C<$date>.
@@ -2829,7 +2830,7 @@ if C<$borrowernumber> is not set, it will delete the issue history for all borro
 If c<$borrowernumber> is set, it will delete issue history for only that borrower, regardless of their opac privacy
 setting (force delete).
 
-return the number of affected rows.
+return the number of affected rows and a value that evaluates to true if an error occurred deleting the history.
 
 =cut
 
@@ -2856,8 +2857,9 @@ sub AnonymiseIssueHistory {
     }
     my $sth = $dbh->prepare($query);
     $sth->execute(@bind_params);
+    my $anonymisation_err = $dbh->err;
     my $rows_affected = $sth->rows;  ### doublecheck row count return function
-    return $rows_affected;
+    return ($rows_affected, $anonymisation_err);
 }
 
 =head2 SendCirculationAlert

diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-privacy.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-privacy.tt
index 5245fdf..233222b 100644 (file)
--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-privacy.tt
+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-privacy.tt
@@ -13,9 +13,11 @@
 
     [% IF ( deleted ) %]
         <div class="dialog message">Your reading history has been deleted.</div>
+    [% ELSIF ( err_history_not_deleted ) %]
+        <div class="dialog error">The deletion of your reading history failed, because there is a problem with the configuration of this feature. Please help to fix the system by informing your library of this error.</div>
     [% END %]
     [% IF ( privacy_updated ) %]
-        <div class="dialog message">Your privacy rules have been updated</div>
+        <div class="dialog message">Your privacy rules have been updated.</div>
     [% END %]
 
     <h2>Privacy rule</h2>

diff --git a/misc/cronjobs/batch_anonymise.pl b/misc/cronjobs/batch_anonymise.pl
index 857171b..48a58c6 100755 (executable)
--- a/misc/cronjobs/batch_anonymise.pl
+++ b/misc/cronjobs/batch_anonymise.pl
@@ -19,6 +19,7 @@
 
 use strict;
 use warnings;
+use Carp;
 
 BEGIN {
 
@@ -70,7 +71,8 @@ my ($newyear,$newmonth,$newday) = Add_Delta_Days ($year,$month,$day,(-1)*$days);
 my $formatdate = sprintf "%4d-%02d-%02d",$newyear,$newmonth,$newday;
 $verbose and print "Checkouts before $formatdate will be anonymised.\n";
 
-my $rows = AnonymiseIssueHistory($formatdate);
+my ($rows, $err_history_not_deleted) = AnonymiseIssueHistory($formatdate);
+carp "Anonymisation of reading history failed." if ($err_history_not_deleted);
 $verbose and print "$rows checkouts anonymised.\n";
 
 exit(0);

diff --git a/opac/opac-privacy.pl b/opac/opac-privacy.pl
index bbfb8fc..22fe694 100755 (executable)
--- a/opac/opac-privacy.pl
+++ b/opac/opac-privacy.pl
@@ -51,9 +51,14 @@ if ($op eq "update_privacy")
 if ($op eq "delete_record") {
     # delete all reading records for items returned
     # uses a hardcoded date ridiculously far in the future
-    AnonymiseIssueHistory('2999-12-12',$borrowernumber);
+    my ($rows,$err_history_not_deleted) = AnonymiseIssueHistory('2999-12-12',$borrowernumber);
     # confirm the user the deletion has been done
-    $template->param('deleted' => 1);
+    if ( !$err_history_not_deleted ) {
+        $template->param( 'deleted' => 1 );
+    }
+    else {
+        $template->param( 'err_history_not_deleted' => 1 );
+    }
 }
 # get borrower privacy ....
 my ( $borr ) = GetMemberDetails( $borrowernumber );

diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl
index 65ce813..0d48b99 100755 (executable)
--- a/tools/cleanborrowers.pl
+++ b/tools/cleanborrowers.pl
@@ -145,7 +145,8 @@ if ( $params->{'step3'} ) {
 
     # Anonymising all members
     if ($do_anonym) {
-        $totalAno = AnonymiseIssueHistory($filterdate2);
+        #FIXME: anonymisation errors are not handled
+        ($totalAno,my $anonymisation_error) = AnonymiseIssueHistory($filterdate2);
         $template->param(
             filterdate1 => $filterdate2,
             do_anonym   => '1',