From 25bb40848590f48d9db17d85a48e7f57bb5612f4 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 22 Jul 2016 17:25:24 +0100 Subject: [PATCH] Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise Content-Type: text/plain; charset=utf-8 The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed to return the patrons with an issue history older than a given date. It would make more sense to return a list of Koha::Patrons. On the way, the code from AnonymiseIssueHistory will be moved as well to anonymise_issue_history. Note that these 2 subroutines are strongly linked: one is used to know the number of patrons we will anonymise the history, the other one is used to anonymise the issues history. The problem is that the first one is not used to do the action, but only for displayed purpose. In some cases, these 2 values can differ, which could be confusing. Case 1: The logged in librarian is not superlibrarian and IndependentBranches is set: if 2+ patrons from different libraries match the date parameter, the interface will display "Checkout history for 2 patrons will be anonymized", when actually only 1 will be. Case 2: If 2+ patrons match the date parameter but one of them has his privacy set to forever (privacy=0), the same issue will appear. This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons->search_patrons_to_anonymise and from C4::Circulation::AnonymiseIssueHistory to Koha::Patrons->anonymise_issue_history Test plan: 1/ Confirm the 2 issues and make sure they are fixed using the Batch patron anonymization tool (tools/cleanborrowers.pl) 2/ At the OPAC, use the 'Immediate deletion' button to delete all your reading history (regardless the setting of the privacy rule) 3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to anonymise patrons. Signed-off-by: Josef Moravec Signed-off-by: Kyle M Hall Signed-off-by: Marcel de Rooy --- C4/Circulation.pm | 44 ---------------- C4/Members.pm | 46 ----------------- Koha/Patrons.pm | 58 +++++++++++++++++++++- .../prog/en/modules/tools/cleanborrowers.tt | 4 +- .../opac-tmpl/bootstrap/en/modules/opac-privacy.tt | 6 +-- misc/cronjobs/batch_anonymise.pl | 7 ++- opac/opac-privacy.pl | 15 ++---- tools/cleanborrowers.pl | 23 ++++----- 8 files changed, 81 insertions(+), 122 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index e2cd626..ff05ebc 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -94,7 +94,6 @@ BEGIN { &GetBranchItemRule &GetBiblioIssues &GetOpenIssue - &AnonymiseIssueHistory &CheckIfIssuedToPatron &IsItemIssued GetTopIssues @@ -3271,49 +3270,6 @@ sub DeleteTransfer { return $sth->execute($itemnumber); } -=head2 AnonymiseIssueHistory - - ($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>. - -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 and a value that evaluates to true if an error occurred deleting the history. - -=cut - -sub AnonymiseIssueHistory { - my $date = shift; - my $borrowernumber = shift; - my $dbh = C4::Context->dbh; - my $query = " - UPDATE old_issues - SET borrowernumber = ? - WHERE returndate < ? - AND borrowernumber IS NOT NULL - "; - - # The default of 0 does not work due to foreign key constraints - # The anonymisation should not fail quietly if AnonymousPatron is not a valid entry - # Set it to undef (NULL) - my $anonymouspatron = C4::Context->preference('AnonymousPatron') || undef; - my @bind_params = ($anonymouspatron, $date); - if (defined $borrowernumber) { - $query .= " AND borrowernumber = ?"; - push @bind_params, $borrowernumber; - } else { - $query .= " AND (SELECT privacy FROM borrowers WHERE borrowers.borrowernumber=old_issues.borrowernumber) <> 0"; - } - 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, $anonymisation_err); -} - =head2 SendCirculationAlert Send out a C or C alert using the messaging system. diff --git a/C4/Members.pm b/C4/Members.pm index a501041..8e73872 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -72,7 +72,6 @@ BEGIN { &GetBorNotifyAcctRecord &GetBorrowersToExpunge - &GetBorrowersWithIssuesHistoryOlderThan &IssueSlip GetBorrowersWithEmail @@ -1113,51 +1112,6 @@ sub GetBorrowersToExpunge { return \@results; } -=head2 GetBorrowersWithIssuesHistoryOlderThan - - $results = &GetBorrowersWithIssuesHistoryOlderThan($date) - -this function get all borrowers who has an issue history older than I<$date> given on input arg. - -I<$result> is a ref to an array which all elements are a hashref. -This hashref is containt the number of time this borrowers has borrowed before I<$date> and the borrowernumber. - -=cut - -sub GetBorrowersWithIssuesHistoryOlderThan { - my $dbh = C4::Context->dbh; - my $date = shift ||POSIX::strftime("%Y-%m-%d",localtime()); - my $filterbranch = shift || - ((C4::Context->preference('IndependentBranches') - && C4::Context->userenv - && !C4::Context->IsSuperLibrarian() - && C4::Context->userenv->{branch}) - ? C4::Context->userenv->{branch} - : ""); - my $query = " - SELECT count(borrowernumber) as n,borrowernumber - FROM old_issues - WHERE returndate < ? - AND borrowernumber IS NOT NULL - "; - my @query_params; - push @query_params, $date; - if ($filterbranch){ - $query.=" AND branchcode = ?"; - push @query_params, $filterbranch; - } - $query.=" GROUP BY borrowernumber "; - warn $query if $debug; - my $sth = $dbh->prepare($query); - $sth->execute(@query_params); - my @results; - - while ( my $data = $sth->fetchrow_hashref ) { - push @results, $data; - } - return \@results; -} - =head2 IssueSlip IssueSlip($branchcode, $borrowernumber, $quickslip) diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index d079df0..60e1a2c 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -1,6 +1,7 @@ package Koha::Patrons; -# Copyright ByWater Solutions 2014 +# Copyright 2014 ByWater Solutions +# Copyright 2016 Koha Development Team # # This file is part of Koha. # @@ -181,6 +182,61 @@ sub article_requests_finished { return $self->{_article_requests_finished}; } +=head3 search_patrons_to_anonymise + + my $patrons = Koha::Patrons->search_patrons_to_anonymise( $date ); + +This method returns all patrons who has an issue history older than a given date. + +=cut + +sub search_patrons_to_anonymise { + my ( $class, $older_than_date, $library ) = @_; + $older_than_date = $older_than_date ? dt_from_string($older_than_date) : dt_from_string; + $library ||= + ( C4::Context->preference('IndependentBranches') && C4::Context->userenv && !C4::Context->IsSuperLibrarian() && C4::Context->userenv->{branch} ) + ? C4::Context->userenv->{branch} + : undef; + + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my $rs = $class->search( + { returndate => { '<' => $dtf->format_datetime($older_than_date), }, + 'old_issues.borrowernumber' => { 'not' => undef }, + privacy => { '<>' => 0 }, # Keep forever + ( $library ? ( 'old_issues.branchcode' => $library ) : () ), + }, + { join => ["old_issues"], + group_by => 'borrowernumber' + } + ); + return Koha::Patrons->_new_from_dbic($rs); +} + +=head3 anonymise_issue_history + + Koha::Patrons->search->anonymise_issue_history( $older_than_date ); + +Anonymise issue history (old_issues) for all patrons older than the given date. +To make sure all the conditions are met, the caller has the responsability to +call search_patrons_to_anonymise to filter the Koha::Patrons set + +=cut + +sub anonymise_issue_history { + my ( $self, $older_than_date ) = @_; + + return unless $older_than_date; + $older_than_date = dt_from_string $older_than_date; + + # The default of 0 does not work due to foreign key constraints + # The anonymisation should not fail quietly if AnonymousPatron is not a valid entry + # Set it to undef (NULL) + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my $old_issues_to_anonymise = $self->search_related( 'old_issues', { returndate => { '<' => $dtf->format_datetime($older_than_date) } } ); + my $anonymous_patron = C4::Context->preference('AnonymousPatron') || undef; + $old_issues_to_anonymise->update( { 'old_issues.borrowernumber' => $anonymous_patron } ); +} + =head3 type =cut 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 e47a85e..e5bf24d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt @@ -217,8 +217,8 @@

No patron records have been removed

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

All checkouts older than [% last_issue_date | $KohaDates %] have been anonymized

+ [% IF do_anonym %] +

All checkouts ([% do_anonym %]) older than [% last_issue_date | $KohaDates %] have been anonymized

[% ELSE %]

No patron records have been anonymized

[% END %] diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-privacy.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-privacy.tt index 38ca4fa..98346f6 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-privacy.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-privacy.tt @@ -25,10 +25,10 @@

Your privacy management

- [% IF ( deleted ) %] + [% IF deleted %]
Your reading history has been deleted.
- [% ELSIF ( err_history_not_deleted ) %] -
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.
+ [% ELSE %] +
No reading history to delete
[% END %] [% IF ( privacy_updated ) %] diff --git a/misc/cronjobs/batch_anonymise.pl b/misc/cronjobs/batch_anonymise.pl index c90842f..7197603 100755 --- a/misc/cronjobs/batch_anonymise.pl +++ b/misc/cronjobs/batch_anonymise.pl @@ -30,7 +30,7 @@ BEGIN { } use C4::Context; -use C4::Circulation; +use Koha::Patrons; use Date::Calc qw( Today Add_Delta_Days @@ -74,8 +74,7 @@ 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, $err_history_not_deleted) = AnonymiseIssueHistory($formatdate); -carp "Anonymisation of reading history failed." if ($err_history_not_deleted); -$verbose and print "$rows checkouts anonymised.\n"; +my $rows = Koha::Patrons->search_patrons_to_anonymise( $formatdate )->anonymise_issue_history( $formatdate ); +$verbose and print int($rows) . " checkouts anonymised.\n"; exit(0); diff --git a/opac/opac-privacy.pl b/opac/opac-privacy.pl index 969c46a..8b9caf8 100755 --- a/opac/opac-privacy.pl +++ b/opac/opac-privacy.pl @@ -21,7 +21,6 @@ use CGI qw ( -utf8 ); use C4::Auth; # checkauth, getborrowernumber. use C4::Context; -use C4::Circulation; use C4::Members; use C4::Output; use Koha::Patrons; @@ -62,16 +61,12 @@ elsif ( $op eq "delete_record" ) { # delete all reading records for items returned # uses a hardcoded date ridiculously far in the future - my ( $rows, $err_history_not_deleted ) = - AnonymiseIssueHistory( '2999-12-12', $borrowernumber ); - # confirm the user the deletion has been done - if ( !$err_history_not_deleted ) { - $template->param( 'deleted' => 1 ); - } - else { - $template->param( 'err_history_not_deleted' => 1 ); - } + my $rows = eval { + Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history( '2999-12-12' ); + }; + $rows = $@ ? 0 : int($rows); + $template->param( 'deleted' => $rows ); } # get borrower privacy .... diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index dfc47ff..e0d3b4e 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -42,6 +42,7 @@ use Koha::DateUtils qw( dt_from_string output_pref ); use Koha::Patron::Categories; use Koha::Patrons; use Date::Calc qw/Today Add_Delta_YM/; +use Koha::Patrons; use Koha::List::Patron; my $cgi = new CGI; @@ -106,19 +107,17 @@ if ( $step == 2 ) { } _skip_borrowers_with_nonzero_balance($patrons_to_delete); - my $members_to_anonymize; - if ( $checkboxes{issue} ) { - if ( $branch eq '*' ) { - $members_to_anonymize = GetBorrowersWithIssuesHistoryOlderThan($last_issue_date); - } else { - $members_to_anonymize = GetBorrowersWithIssuesHistoryOlderThan($last_issue_date, $branch); - } - } + my $patrons_to_anonymize = + $checkboxes{issue} + ? $branch eq '*' + ? Koha::Patrons->search_patrons_to_anonymise($last_issue_date) + : Koha::Patrons->search_patrons_to_anonymise( $last_issue_date, $branch ) + : undef; $template->param( patrons_to_delete => $patrons_to_delete, - patrons_to_anonymize => $members_to_anonymize, - patron_list_id => $patron_list_id + patrons_to_anonymize => $patrons_to_anonymize, + patron_list_id => $patron_list_id, ); } @@ -160,9 +159,9 @@ elsif ( $step == 3 ) { # Anonymising all members if ($do_anonym) { #FIXME: anonymisation errors are not handled - ($totalAno,my $anonymisation_error) = AnonymiseIssueHistory($last_issue_date); + my $rows = Koha::Patrons->search_patrons_to_anonymise( $last_issue_date )->anonymise_issue_history( $last_issue_date ); $template->param( - do_anonym => '1', + do_anonym => $rows, ); } -- 2.1.4