From b97c38b527e5584c7de6e1343942ca7baf6c5eef Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 13 Oct 2023 15:23:35 +0000 Subject: [PATCH] Bug 23260: (follow-up) Tidy --- Koha/Patrons.pm | 66 ++++++++++----------- misc/cronjobs/anonymise_last_borrowers.pl | 9 +-- t/db_dependent/Koha/Patrons.t | 72 +++++++++++++---------- 3 files changed, 77 insertions(+), 70 deletions(-) diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index d2fd2980fb..f596b55944 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -159,6 +159,7 @@ sub search_patrons_to_anonymise { return Koha::Patrons->_new_from_dbic($rs); } + =head3 anonymise_issue_history Koha::Patrons->search->anonymise_issue_history( { [ before => $older_than_date ] } ); @@ -179,18 +180,17 @@ sub anonymise_issue_history { # 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 $dtf = Koha::Database->new->schema->storage->datetime_parser; my $nb_rows = 0; while ( my $patron = $self->next ) { my $old_issues_to_anonymise = $patron->old_checkouts->search( - { - ( - $older_than_date - ? ( returndate => - { '<' => $dtf->format_datetime($older_than_date) } ) - : () - ) - } + { + ( + $older_than_date + ? ( returndate => { '<' => $dtf->format_datetime($older_than_date) } ) + : () + ) + } ); my $anonymous_patron = C4::Context->preference('AnonymousPatron') || undef; $nb_rows += $old_issues_to_anonymise->update( { 'old_issues.borrowernumber' => $anonymous_patron } ); @@ -198,7 +198,6 @@ sub anonymise_issue_history { return $nb_rows; } - =head3 anonymise_last_borrowers Koha::Patrons->search->anonymise_last_borrowers(); @@ -213,19 +212,21 @@ sub anonymise_last_borrowers { return unless C4::Context->preference("AnonymiseLastBorrower"); my $days = C4::Context->preference("AnonymiseLastBorrowerDays") || 0; - my ($year,$month,$day) = Today(); - my ($newyear,$newmonth,$newday) = Add_Delta_Days ($year,$month,$day,(-1)*$days); - my $older_than_date = dt_from_string(sprintf "%4d-%02d-%02d",$newyear,$newmonth,$newday); + my ( $year, $month, $day ) = Today(); + my ( $newyear, $newmonth, $newday ) = Add_Delta_Days( $year, $month, $day, (-1) * $days ); + my $older_than_date = dt_from_string( sprintf "%4d-%02d-%02d", $newyear, $newmonth, $newday ); my $anonymous_patron = C4::Context->preference('AnonymousPatron') || undef; my $dtf = Koha::Database->new->schema->storage->datetime_parser; - my $rs = $self->_resultset->search( - { created_on => { '<' => $dtf->format_datetime($older_than_date), }, - 'items_last_borrowers.borrowernumber' => { 'not' => undef }, # Keep forever + my $rs = $self->_resultset->search( + { + created_on => { '<' => $dtf->format_datetime($older_than_date), }, + 'items_last_borrowers.borrowernumber' => { 'not' => undef }, # Keep forever ( $anonymous_patron ? ( 'items_last_borrowers.borrowernumber' => { '!=' => $anonymous_patron } ) : () ), }, - { join => ["items_last_borrowers"], + { + join => ["items_last_borrowers"], distinct => 1, } ); @@ -233,24 +234,21 @@ sub anonymise_last_borrowers { my $nb_rows = 0; while ( my $patron = $patrons->next ) { - my $last_borrowers_to_anonymise = - $patron->_result->items_last_borrowers->search( - { - ( - $older_than_date - ? ( created_on => - { '<' => $dtf->format_datetime($older_than_date) } ) - : (), - "itemnumber.damaged" => 0, - "itemnumber.itemlost" => 0, - "itemnumber.withdrawn" => 0, - ), - }, - { - join => ['itemnumber'] - } + my $last_borrowers_to_anonymise = $patron->_result->items_last_borrowers->search( + { + ( + $older_than_date + ? ( created_on => { '<' => $dtf->format_datetime($older_than_date) } ) + : (), + "itemnumber.damaged" => 0, + "itemnumber.itemlost" => 0, + "itemnumber.withdrawn" => 0, + ), + }, + { join => ['itemnumber'] } ); - $nb_rows += $last_borrowers_to_anonymise->update( { 'items_last_borrower.borrowernumber' => $anonymous_patron } ); + $nb_rows += + $last_borrowers_to_anonymise->update( { 'items_last_borrower.borrowernumber' => $anonymous_patron } ); } return $nb_rows; diff --git a/misc/cronjobs/anonymise_last_borrowers.pl b/misc/cronjobs/anonymise_last_borrowers.pl index 7f73794196..6eb47976cb 100755 --- a/misc/cronjobs/anonymise_last_borrowers.pl +++ b/misc/cronjobs/anonymise_last_borrowers.pl @@ -1,4 +1,3 @@ - #!/usr/bin/perl # Copyright 2011, ByWater Solutions. @@ -36,7 +35,6 @@ use Koha::Patrons; use Getopt::Long; use C4::Log; - sub usage { print STDERR < \$help, - 'v|verbose' => \$verbose, + 'h|help' => \$help, + 'v|verbose' => \$verbose, ) || usage(1); if ($help) { @@ -61,8 +59,7 @@ if ($help) { my $pref = C4::Context->preference("AnonymiseLastBorrower"); - -if( !$pref ) { +if ( !$pref ) { print "Preference 'AnonymiseLastBorrower' must be enabled to anonymise item's last borrower\n\n"; usage(1); } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 299b76f681..943845451b 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1289,14 +1289,14 @@ subtest 'anonymise items_last_borrower' => sub { my $anonymous = $builder->build_object( { class => 'Koha::Patrons', }, ); - t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous->borrowernumber ); - t::lib::Mocks::mock_preference('StoreLastBorrower', 1); - t::lib::Mocks::mock_preference('AnonymiseLastBorrower', 1); + t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous->borrowernumber ); + t::lib::Mocks::mock_preference( 'StoreLastBorrower', 1 ); + t::lib::Mocks::mock_preference( 'AnonymiseLastBorrower', 1 ); subtest 'anonymise items_last_borrower by days' => sub { plan tests => 4; - t::lib::Mocks::mock_preference('AnonymiseLastBorrowerDays', 5); + t::lib::Mocks::mock_preference( 'AnonymiseLastBorrowerDays', 5 ); my $patron = $builder->build_object( { @@ -1304,7 +1304,7 @@ subtest 'anonymise items_last_borrower' => sub { value => { privacy => 1, } } ); - my $item_1 = $builder->build_sample_item; + my $item_1 = $builder->build_sample_item; my $issue_1 = $builder->build_object( { class => 'Koha::Checkouts', @@ -1314,7 +1314,7 @@ subtest 'anonymise items_last_borrower' => sub { }, } ); - my $item_2 = $builder->build_sample_item; + my $item_2 = $builder->build_sample_item; my $issue_2 = $builder->build_object( { class => 'Koha::Checkouts', @@ -1326,29 +1326,35 @@ subtest 'anonymise items_last_borrower' => sub { ); my $dbh = C4::Context->dbh; - my ( $returned_1 ) = C4::Circulation::AddReturn( $item_1->barcode, undef, undef, dt_from_string ); - my ( $returned_2 ) = C4::Circulation::AddReturn( $item_2->barcode, undef, undef, dt_from_string ); + my ($returned_1) = C4::Circulation::AddReturn( $item_1->barcode, undef, undef, dt_from_string ); + my ($returned_2) = C4::Circulation::AddReturn( $item_2->barcode, undef, undef, dt_from_string ); is( $returned_1 && $returned_2, 1, 'The items should have been returned' ); my $stu = $dbh->prepare(q|UPDATE items_last_borrower set created_on = ? where itemnumber = ?|); - $stu->execute(DateTime->today(time_zone => C4::Context->tz())->add( days => -6 ), $item_2->itemnumber); + $stu->execute( DateTime->today( time_zone => C4::Context->tz() )->add( days => -6 ), $item_2->itemnumber ); my $nb_rows = Koha::Patrons->anonymise_last_borrowers(); - is( $nb_rows, 1, 'anonymise_issue_history should have returned the number of last borrowers anonymised'); + is( $nb_rows, 1, 'anonymise_issue_history should have returned the number of last borrowers anonymised' ); my $sth = $dbh->prepare(q|SELECT borrowernumber FROM items_last_borrower where itemnumber = ?|); - $sth->execute($item_1->itemnumber); + $sth->execute( $item_1->itemnumber ); my ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; - is( $borrowernumber_used_to_anonymised, $patron->borrowernumber, 'Last borrower younger than 5 days are not anonymised' ); - $sth->execute($item_2->itemnumber); + is( + $borrowernumber_used_to_anonymised, $patron->borrowernumber, + 'Last borrower younger than 5 days are not anonymised' + ); + $sth->execute( $item_2->itemnumber ); ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; - is( $borrowernumber_used_to_anonymised, $anonymous->borrowernumber, 'Last borrower older than 5 days are anonymised' ); + is( + $borrowernumber_used_to_anonymised, $anonymous->borrowernumber, + 'Last borrower older than 5 days are anonymised' + ); }; subtest 'withrawn, lost and damaged items' => sub { plan tests => 6; - t::lib::Mocks::mock_preference('AnonymiseLastBorrowerDays', 0); + t::lib::Mocks::mock_preference( 'AnonymiseLastBorrowerDays', 0 ); my $patron = $builder->build_object( { @@ -1356,7 +1362,7 @@ subtest 'anonymise items_last_borrower' => sub { value => { privacy => 1, } } ); - my $item_1 = $builder->build_sample_item; + my $item_1 = $builder->build_sample_item; my $issue_1 = $builder->build_object( { class => 'Koha::Checkouts', @@ -1366,7 +1372,7 @@ subtest 'anonymise items_last_borrower' => sub { }, } ); - my $item_2 = $builder->build_sample_item; + my $item_2 = $builder->build_sample_item; my $issue_2 = $builder->build_object( { class => 'Koha::Checkouts', @@ -1376,7 +1382,7 @@ subtest 'anonymise items_last_borrower' => sub { }, } ); - my $item_3 = $builder->build_sample_item; + my $item_3 = $builder->build_sample_item; my $issue_3 = $builder->build_object( { class => 'Koha::Checkouts', @@ -1386,7 +1392,7 @@ subtest 'anonymise items_last_borrower' => sub { }, } ); - my $item_4 = $builder->build_sample_item; + my $item_4 = $builder->build_sample_item; my $issue_4 = $builder->build_object( { class => 'Koha::Checkouts', @@ -1397,10 +1403,10 @@ subtest 'anonymise items_last_borrower' => sub { } ); - my ( $returned_1 ) = C4::Circulation::AddReturn( $item_1->barcode, undef, undef, dt_from_string('2010-10-11') ); - my ( $returned_2 ) = C4::Circulation::AddReturn( $item_2->barcode, undef, undef, dt_from_string('2010-10-11') ); - my ( $returned_3 ) = C4::Circulation::AddReturn( $item_3->barcode, undef, undef, dt_from_string('2010-10-11') ); - my ( $returned_4 ) = C4::Circulation::AddReturn( $item_4->barcode, undef, undef, dt_from_string('2010-10-11') ); + my ($returned_1) = C4::Circulation::AddReturn( $item_1->barcode, undef, undef, dt_from_string('2010-10-11') ); + my ($returned_2) = C4::Circulation::AddReturn( $item_2->barcode, undef, undef, dt_from_string('2010-10-11') ); + my ($returned_3) = C4::Circulation::AddReturn( $item_3->barcode, undef, undef, dt_from_string('2010-10-11') ); + my ($returned_4) = C4::Circulation::AddReturn( $item_4->barcode, undef, undef, dt_from_string('2010-10-11') ); is( $returned_1 && $returned_2 && $returned_3 && $returned_4, 1, 'The items should have been returned' ); $item_1->withdrawn(1)->store; $item_2->itemlost(1)->store; @@ -1408,24 +1414,30 @@ subtest 'anonymise items_last_borrower' => sub { my $dbh = C4::Context->dbh; my $stu = $dbh->prepare(q|UPDATE items_last_borrower set created_on = ? where itemnumber in (?, ?, ?, ?)|); - $stu->execute(DateTime->today(time_zone => C4::Context->tz())->add( days => -1 ), $item_1->itemnumber,$item_2->itemnumber,$item_3->itemnumber,$item_4->itemnumber); + $stu->execute( + DateTime->today( time_zone => C4::Context->tz() )->add( days => -1 ), $item_1->itemnumber, + $item_2->itemnumber, $item_3->itemnumber, $item_4->itemnumber + ); my $nb_rows = Koha::Patrons->anonymise_last_borrowers(); - is( $nb_rows, 1, 'anonymise_issue_history should have returned the number of last borrowers anonymised'); + is( $nb_rows, 1, 'anonymise_issue_history should have returned the number of last borrowers anonymised' ); my $sth = $dbh->prepare(q|SELECT borrowernumber FROM items_last_borrower where itemnumber = ?|); - $sth->execute($item_1->itemnumber); + $sth->execute( $item_1->itemnumber ); my ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; is( $borrowernumber_used_to_anonymised, $patron->borrowernumber, 'withrawn items should not be anonymised' ); - $sth->execute($item_2->itemnumber); + $sth->execute( $item_2->itemnumber ); ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; is( $borrowernumber_used_to_anonymised, $patron->borrowernumber, 'lost items should not be anonymised' ); - $sth->execute($item_3->itemnumber); + $sth->execute( $item_3->itemnumber ); ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; is( $borrowernumber_used_to_anonymised, $patron->borrowernumber, 'damaged items should not be anonymised' ); - $sth->execute($item_4->itemnumber); + $sth->execute( $item_4->itemnumber ); ($borrowernumber_used_to_anonymised) = $sth->fetchrow_array; - is( $borrowernumber_used_to_anonymised, $anonymous->borrowernumber, 'not withdrawn, lost or damaged items are anonymised' ); + is( + $borrowernumber_used_to_anonymised, $anonymous->borrowernumber, + 'not withdrawn, lost or damaged items are anonymised' + ); }; -- 2.30.2