From e0b65bf159019b0618cf6c4bd66836156f35bb4a Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 24 Feb 2017 13:26:29 +0100 Subject: [PATCH] Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history Content-Type: text/plain; charset=utf-8 From opac-privacy.pl: # delete all reading records for items returned # uses a hardcoded date ridiculously far in the future my $rows = eval { Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history( { before => '2999-12-12' } ); }; It sounds better to make this before parameter not mandatory, and remove the condition from the sql query if it is not passed. Test plan: 1. Anonymise your reading history at the OPAC. 2. Confirm that all your reading history has been anonymised Signed-off-by: Marcel de Rooy --- Koha/Patrons.pm | 19 +++++++++++++----- opac/opac-privacy.pl | 4 +--- t/db_dependent/Koha/Patrons.t | 46 +++++++++++++++++++++++++++++++++---------- 3 files changed, 51 insertions(+), 18 deletions(-) diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index 068c4fb..6548f02 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -216,9 +216,9 @@ sub search_patrons_to_anonymise { =head3 anonymise_issue_history - Koha::Patrons->search->anonymise_issue_history( { before => $older_than_date } ); + Koha::Patrons->search->anonymise_issue_history( { [ before => $older_than_date ] } ); -Anonymise issue history (old_issues) for all patrons older than the given date. +Anonymise issue history (old_issues) for all patrons older than the given date (optional). 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 @@ -229,14 +229,23 @@ sub anonymise_issue_history { my $older_than_date = $params->{before}; - return unless $older_than_date; - $older_than_date = dt_from_string $older_than_date; + $older_than_date = dt_from_string $older_than_date if $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 $old_issues_to_anonymise = $self->search_related( + 'old_issues', + { + ( + $older_than_date + ? ( 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 } ); } diff --git a/opac/opac-privacy.pl b/opac/opac-privacy.pl index 622b9f5..ff0d328 100755 --- a/opac/opac-privacy.pl +++ b/opac/opac-privacy.pl @@ -60,10 +60,8 @@ if ( $op eq "update_privacy" ) { elsif ( $op eq "delete_record" ) { # delete all reading records for items returned - # uses a hardcoded date ridiculously far in the future - my $rows = eval { - Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history( { before => '2999-12-12' } ); + Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history; }; $rows = $@ ? 0 : int($rows); $template->param( 'deleted' => $rows ); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 06d4d86..7fac24e 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -688,7 +688,7 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub { t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous->{borrowernumber} ); subtest 'patron privacy is 1 (default)' => sub { - plan tests => 4; + plan tests => 6; t::lib::Mocks::mock_preference('IndependentBranches', 0); my $patron = $builder->build( @@ -696,7 +696,7 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub { value => { privacy => 1, } } ); - my $item = $builder->build( + my $item_1 = $builder->build( { source => 'Item', value => { itemlost => 0, @@ -704,29 +704,55 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub { }, } ); - my $issue = $builder->build( + my $issue_1 = $builder->build( { source => 'Issue', value => { borrowernumber => $patron->{borrowernumber}, - itemnumber => $item->{itemnumber}, + itemnumber => $item_1->{itemnumber}, + }, + } + ); + my $item_2 = $builder->build( + { source => 'Item', + value => { + itemlost => 0, + withdrawn => 0, + }, + } + ); + my $issue_2 = $builder->build( + { source => 'Issue', + value => { + borrowernumber => $patron->{borrowernumber}, + itemnumber => $item_2->{itemnumber}, }, } ); - my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' ); - is( $returned, 1, 'The item should have been returned' ); + my ( $returned_1, undef, undef ) = C4::Circulation::AddReturn( $item_1->{barcode}, undef, undef, undef, '2010-10-10' ); + my ( $returned_2, undef, undef ) = C4::Circulation::AddReturn( $item_2->{barcode}, undef, undef, undef, '2011-11-11' ); + is( $returned_1 && $returned_2, 1, 'The items should have been returned' ); my $patrons_to_anonymise = Koha::Patrons->search_patrons_to_anonymise( { before => '2010-10-11' } )->search( { 'me.borrowernumber' => $patron->{borrowernumber} } ); is( ref($patrons_to_anonymise), 'Koha::Patrons', 'search_patrons_to_anonymise should return Koha::Patrons' ); - my $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => '2010-10-11' } )->anonymise_issue_history( { before => '2010-10-11' } ); + my $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => '2011-11-12' } )->anonymise_issue_history( { before => '2010-10-11' } ); ok( $rows_affected > 0, 'AnonymiseIssueHistory should affect at least 1 row' ); my $dbh = C4::Context->dbh; - my ($borrowernumber_used_to_anonymised) = $dbh->selectrow_array(q| - SELECT borrowernumber FROM old_issues where itemnumber = ? - |, undef, $item->{itemnumber}); + my $sth = $dbh->prepare(q|SELECT borrowernumber FROM old_issues where itemnumber = ?|); + $sth->execute($item_1->{itemnumber}); + my ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; is( $borrowernumber_used_to_anonymised, $anonymous->{borrowernumber}, 'With privacy=1, the issue should have been anonymised' ); + $sth->execute($item_2->{itemnumber}); + ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; + is( $borrowernumber_used_to_anonymised, $patron->{borrowernumber}, 'The issue should not have been anonymised, the returned date is later' ); + + $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => '2011-11-12' } )->anonymise_issue_history; + $sth->execute($item_2->{itemnumber}); + ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; + is( $borrowernumber_used_to_anonymised, $anonymous->{borrowernumber}, 'The issue should have been anonymised, the returned date is before' ); + Koha::Patrons->find( $patron->{borrowernumber})->delete; }; -- 2.1.4