From 8052ba9717d3dbee3bfb48cc22b8782f57b233ff Mon Sep 17 00:00:00 2001 From: Agustin Moyano Date: Mon, 4 Nov 2019 16:36:25 -0300 Subject: [PATCH] Bug 23260: (follow-up) optimize anonymise_items_last_borrower Sponsored-by: Northeast Kansas Library System Signed-off-by: Maryse Simard --- Koha/Item/LastPatrons.pm | 41 +++++++++++++++ Koha/Patrons.pm | 87 +++++--------------------------- misc/cronjobs/batch_anonymise.pl | 3 +- t/db_dependent/Koha/Patrons.t | 44 +++------------- 4 files changed, 62 insertions(+), 113 deletions(-) diff --git a/Koha/Item/LastPatrons.pm b/Koha/Item/LastPatrons.pm index 3a91b92f94..7ed7c8fd5d 100644 --- a/Koha/Item/LastPatrons.pm +++ b/Koha/Item/LastPatrons.pm @@ -22,6 +22,7 @@ use Carp; use Koha::Item::LastPatron; use Koha::Database; +use Koha::DateUtils; use base qw(Koha::Objects); @@ -35,6 +36,46 @@ Koha::Item::LastPatrons - Koha Item LastPatron Object set class =cut +=head3 anonymise_items_last_borrower + + Koha::Patrons->search->anonymise_items_last_borrower( { [ before => $older_than_date ] } ); + +Anonymise last borrower history (items_last_borrower) for all patrons older than the given date (optional). +To make sure all the conditions are met, the caller has the responsibility to +call search_patrons_to_anonymise to filter the Koha::Patrons set + +=cut + +sub anonymise_items_last_borrower { + my ( $self, $params ) = @_; + + my $older_than_date = $params->{before}; + + $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 $nb_rows = 0; + my $last_borrowers = $self->search( + { + ( + $older_than_date + ? ( created_on => + { '<' => $dtf->format_datetime($older_than_date) } ) + : () + ) + } + ); + my $anonymous_patron = C4::Context->preference('AnonymousPatron') || undef; + while(my $last_borrower = $last_borrowers->next) { + $last_borrower->borrowernumber( $anonymous_patron )->store; + $nb_rows++; + } + return $nb_rows; +} + =head3 type =cut diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index 8f2ffe42f1..1935dd8d48 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -130,7 +130,7 @@ sub search_upcoming_membership_expires { =head3 search_patrons_to_anonymise - my $patrons = Koha::Patrons->search_patrons_to_anonymise( { before => $older_than_date, [ library => $library, for => $table_to_anonymize ] } ); + my $patrons = Koha::Patrons->search_patrons_to_anonymise( { before => $older_than_date, [ library => $library ] } ); This method returns all patrons who has an issue history older than a given date. @@ -140,7 +140,7 @@ sub search_patrons_to_anonymise { my ( $class, $params ) = @_; my $older_than_date = $params->{before}; my $library = $params->{library}; - my $for = $params->{for}; + $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} ) @@ -149,35 +149,17 @@ sub search_patrons_to_anonymise { my $anonymous_patron = C4::Context->preference('AnonymousPatron') || undef; my $dtf = Koha::Database->new->schema->storage->datetime_parser; - my $rs; - if($for && $for eq 'items_last_borrower') { - $rs = $class->_resultset->search( - { 'items_last_borrowers.created_on' => { '<' => $dtf->format_datetime($older_than_date), }, - 'items_last_borrowers.borrowernumber' => { 'not' => undef }, - 'itemnumber.damaged' => 0, - 'itemnumber.itemlost' => 0, - 'itemnumber.withdrawn' => 0, - ( $library ? ( 'itemnumber.homebranch' => $library ) : () ), - ( $anonymous_patron ? ( 'items_last_borrowers.borrowernumber' => { '!=' => $anonymous_patron } ) : () ), - }, - { join => { "items_last_borrowers" => "itemnumber" }, - distinct => 1, - } - ); - } else { - #for old_issues - $rs = $class->_resultset->search( - { returndate => { '<' => $dtf->format_datetime($older_than_date), }, - 'old_issues.borrowernumber' => { 'not' => undef }, - privacy => { '<>' => 0 }, # Keep forever - ( $library ? ( 'old_issues.branchcode' => $library ) : () ), - ( $anonymous_patron ? ( 'old_issues.borrowernumber' => { '!=' => $anonymous_patron } ) : () ), - }, - { join => ["old_issues"], - distinct => 1, - } - ); - } + my $rs = $class->_resultset->search( + { returndate => { '<' => $dtf->format_datetime($older_than_date), }, + 'old_issues.borrowernumber' => { 'not' => undef }, + privacy => { '<>' => 0 }, # Keep forever + ( $library ? ( 'old_issues.branchcode' => $library ) : () ), + ( $anonymous_patron ? ( 'old_issues.borrowernumber' => { '!=' => $anonymous_patron } ) : () ), + }, + { join => ["old_issues"], + distinct => 1, + } + ); return Koha::Patrons->_new_from_dbic($rs); } @@ -220,49 +202,6 @@ sub anonymise_issue_history { return $nb_rows; } -=head3 anonymise_items_last_borrower - - Koha::Patrons->search->anonymise_items_last_borrower( { [ before => $older_than_date ] } ); - -Anonymise last borrower history (items_last_borrower) for all patrons older than the given date (optional). -To make sure all the conditions are met, the caller has the responsibility to -call search_patrons_to_anonymise to filter the Koha::Patrons set - -=cut - -sub anonymise_items_last_borrower { - my ( $self, $params ) = @_; - - my $older_than_date = $params->{before}; - - $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 $nb_rows = 0; - while ( my $patron = $self->next ) { - my $last_borrowers = Koha::Item::LastPatrons->search( - { - borrowernumber => $patron->borrowernumber, - ( - $older_than_date - ? ( created_on => - { '<' => $dtf->format_datetime($older_than_date) } ) - : () - ) - } - ); - my $anonymous_patron = C4::Context->preference('AnonymousPatron') || undef; - while(my $last_borrower = $last_borrowers->next) { - $last_borrower->borrowernumber( $anonymous_patron )->store; - $nb_rows++; - } - } - return $nb_rows; -} - =head3 delete Koha::Patrons->search({ some filters here })->delete({ move => 1, verbose => 1 }); diff --git a/misc/cronjobs/batch_anonymise.pl b/misc/cronjobs/batch_anonymise.pl index 36c05a5068..5094ac9c0d 100755 --- a/misc/cronjobs/batch_anonymise.pl +++ b/misc/cronjobs/batch_anonymise.pl @@ -32,6 +32,7 @@ BEGIN { use Koha::Script -cron; use C4::Context; use Koha::Patrons; +use Koha::Item::LastPatrons; use Date::Calc qw( Today Add_Delta_Days @@ -78,7 +79,7 @@ $verbose and print "Checkouts and items last borrowers before $formatdate will b my $rows = Koha::Patrons->search_patrons_to_anonymise( { before => $formatdate } )->anonymise_issue_history( { before => $formatdate } ); $verbose and print int($rows) . " checkouts anonymised.\n"; -$rows = Koha::Patrons->search_patrons_to_anonymise( { before => $formatdate , for => 'items_last_borrower'} )->anonymise_items_last_borrower( { before => $formatdate } ); +$rows = Koha::Item::LastPatrons->anonymise_items_last_borrower( { before => $formatdate } ); $verbose and print int($rows) . " items last borrower anonymised.\n"; exit(0); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 3e2e76c451..c4c774ad78 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -36,6 +36,7 @@ use Koha::ActionLogs; use Koha::Holds; use Koha::Old::Holds; use Koha::Patrons; +use Koha::Item::LastPatrons; use Koha::Patron::Categories; use Koha::Patron::Relationship; use Koha::Database; @@ -1181,7 +1182,7 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub { }; subtest 'search_patrons_to_anonymise & anonymise_items_last_borrower' => sub { - plan tests => 3; + plan tests => 2; my $branch = $builder->build({ source => 'Branch' }); my $userenv_patron = $builder->build_object({ @@ -1223,8 +1224,8 @@ subtest 'search_patrons_to_anonymise & anonymise_items_last_borrower' => sub { my $now = DateTime->now; my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, $now ); is( $returned, 1, 'The item should have been returned' ); - my $before = $now + DateTime::Duration->new( days => 1 ); - my $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => $before->ymd, for => 'items_last_borrower' } )->anonymise_items_last_borrower( { before => $before->ymd } ); + my $before = DateTime->now->add( days => 1 ); + my $rows_affected = Koha::Item::LastPatrons->anonymise_items_last_borrower( { before => $before->ymd } ); 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| @@ -1266,8 +1267,8 @@ subtest 'search_patrons_to_anonymise & anonymise_items_last_borrower' => sub { my $now = DateTime->now; my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, $now ); is( $returned, 1, 'The item should have been returned' ); - my $before = $now + DateTime::Duration->new( days => 1 ); - my $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => $before->ymd, for => 'items_last_borrower' } )->anonymise_items_last_borrower( { before => $before->ymd } ); + my $before = DateTime->now->add( days => 1 ); + my $rows_affected = Koha::Item::LastPatrons->anonymise_items_last_borrower( { before => $before->ymd } ); 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| @@ -1277,39 +1278,6 @@ subtest 'search_patrons_to_anonymise & anonymise_items_last_borrower' => sub { Koha::Patrons->find( $patron->{borrowernumber})->delete; }; - subtest 'Logged in librarian is not superlibrarian & IndependentBranches' => sub { - plan tests => 1; - t::lib::Mocks::mock_preference( 'IndependentBranches', 1 ); - my $patron = $builder->build( - { source => 'Borrower', - value => { privacy => 1 } # Another branchcode than the logged in librarian - } - ); - my $item = $builder->build( - { source => 'Item', - value => { - itemlost => 0, - withdrawn => 0, - damaged => 0 - }, - } - ); - my $issue = $builder->build( - { source => 'Issue', - value => { - borrowernumber => $patron->{borrowernumber}, - itemnumber => $item->{itemnumber}, - }, - } - ); - - my $now = DateTime->now; - my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, $now ); - my $before = $now + DateTime::Duration->new( days => 1 ); - is( Koha::Patrons->search_patrons_to_anonymise( { before => $before->ymd, for => 'items_last_borrower' } )->count, 0 ); - Koha::Patrons->find( $patron->{borrowernumber})->delete; - }; - $userenv_patron->delete; t::lib::Mocks::mock_preference('StoreLastBorrower', 0); t::lib::Mocks::mock_preference('IndependentBranches', 0); -- 2.17.1