From aace5d09905ec394846c51c846d3d68290ae76d0 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Thu, 4 Apr 2024 16:26:00 +0200 Subject: [PATCH] Bug 36526: Remove circular dependency from Koha::Objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Koha::Objects depends on Koha::DateUtils, which depends on C4::Context, which depends on Koha::Config::SysPrefs, which depends on Koha::Objects Apart from the circular dependency, the dependency on C4::Context alone is problematic as it loads a bunch of modules that are not needed at all in Koha::Objects (YAML::XS and ZOOM for instance). As Koha::Objects is used as a base for a lot of modules, we should take care to only load the minimum required. This patch removes uses of Koha::DateUtils from Koha::Objects. It was only used in Koha::Objects::filter_by_last_update filter_by_last_update now requires that the 'from' and 'to' parameters must be DateTime objects. Previously it would also allow date and datetime strings. This possibility was only used in two places: * misc/cronjobs/cleanup_database.pl * tools/cleanborrowers.pl Now they call dt_from_string first and pass a DateTime object to filter_by_last_update Test plan: 1. Run `perl -cw Koha/Objects.pm`. It should only say: "Koha/Objects.pm syntax OK" without warnings 2. Run `prove t/db_dependent/Koha/Objects.t` 3. Verify that misc/cronjobs/cleanup_database.pl works as before, especially with the options --pseudo-transactions, --pseudo-transactions-from and --pseudo-transactions-to 4. Go to Tools » Batch patron deletion and anonymization, check "Verify you want to anonymize patron checkout history" and enter a date in the text input below. Then click Next and verify that the correct count of borrowers is shown. Click on the "Finish" button and verify that the circulation history has been correctly anonymized See also bug 36432 Signed-off-by: Tadeusz Sośnierz Signed-off-by: Martin Renvoize Signed-off-by: Katrin Fischer (cherry picked from commit 675c8263b707c8e1987ac6084272b355deb1b05c) Signed-off-by: Fridolin Somers --- Koha/Objects.pm | 28 +++++++++++++++------------- misc/cronjobs/cleanup_database.pl | 3 +++ t/db_dependent/Koha/Objects.t | 25 +++++-------------------- tools/cleanborrowers.pl | 4 +++- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/Koha/Objects.pm b/Koha/Objects.pm index dc3d044bd3..a59e5d5712 100644 --- a/Koha/Objects.pm +++ b/Koha/Objects.pm @@ -25,7 +25,6 @@ use Class::Inspector; use Koha::Database; use Koha::Exceptions::Object; -use Koha::DateUtils qw( dt_from_string ); =head1 NAME @@ -232,10 +231,10 @@ sub update { =head3 filter_by_last_update -my $filtered_objects = $objects->filter_by_last_update({ - from => $date1, to => $date2, - days|older_than => $days, min_days => $days, younger_than => $days, -}); + my $filtered_objects = $objects->filter_by_last_update({ + from => $from_datetime, to => $to_datetime, + days|older_than => $days, min_days => $days, younger_than => $days, + }); You should pass at least one of the parameters: from, to, days|older_than, min_days or younger_than. Make sure that they do not conflict with each other @@ -243,7 +242,7 @@ to get meaningful results. Note: from, to and min_days are inclusive! And by nature days|older_than and younger_than are exclusive. -The from and to parameters can be DateTime objects or date strings. +The from and to parameters must be DateTime objects. =cut @@ -254,21 +253,24 @@ sub filter_by_last_update { Koha::Exceptions::MissingParameter->throw("Please pass: days|from|to|older_than|younger_than") unless grep { exists $params->{$_} } qw/days from to older_than younger_than min_days/; + foreach my $key (qw(from to)) { + if (exists $params->{$key} and ref $params->{$key} ne 'DateTime') { + Koha::Exceptions::WrongParameter->throw("'$key' parameter must be a DateTime object"); + } + } + my $dtf = Koha::Database->new->schema->storage->datetime_parser; foreach my $p ( qw/days older_than younger_than min_days/ ) { next if !exists $params->{$p}; - my $dt = Koha::DateUtils::dt_from_string(); + my $days = $params->{$p}; my $operator = { days => '<', older_than => '<', min_days => '<=' }->{$p} // '>'; - $dt->subtract( days => $params->{$p} )->truncate( to => 'day' ); - $conditions->{$operator} = $dtf->format_datetime( $dt ); + $conditions->{$operator} = \['DATE_SUB(CURDATE(), INTERVAL ? DAY)', $days]; } if ( exists $params->{from} ) { - my $from = ref($params->{from}) ? $params->{from} : dt_from_string($params->{from}); - $conditions->{'>='} = $dtf->format_datetime( $from ); + $conditions->{'>='} = $dtf->format_datetime( $params->{from} ); } if ( exists $params->{to} ) { - my $to = ref($params->{to}) ? $params->{to} : dt_from_string($params->{to}); - $conditions->{'<='} = $dtf->format_datetime( $to ); + $conditions->{'<='} = $dtf->format_datetime( $params->{to} ); } return $self->search( diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index 04cc934b9c..0193018129 100755 --- a/misc/cronjobs/cleanup_database.pl +++ b/misc/cronjobs/cleanup_database.pl @@ -701,6 +701,9 @@ if ( defined $pPseudoTransactions or $pPseudoTransactionsFrom or $pPseudoTransac $pPseudoTransactionsTo->set( hour => 23, minute => 59, second => 59 ); } } + if ($pPseudoTransactionsFrom) { + $pPseudoTransactionsFrom = dt_from_string($pPseudoTransactionsFrom); + } my $anonymized_transactions = Koha::PseudonymizedTransactions->filter_by_last_update( { timestamp_column_name => 'datetime', diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index a868b5c3d0..a5444fc2a8 100755 --- a/t/db_dependent/Koha/Objects.t +++ b/t/db_dependent/Koha/Objects.t @@ -1162,31 +1162,16 @@ subtest "filter_by_last_update" => sub { )->count; is( $count, 3, '3 patrons have been updated between D-4 and D-2' ); - t::lib::Mocks::mock_preference( 'dateformat', 'metric' ); - try { + throws_ok { $count = $patrons->filter_by_last_update( { timestamp_column_name => 'updated_on', from => '1970-12-31' } ) ->count; - } - catch { - ok( - $_->isa( - 'No exception raised, from and to parameters can take an iso formatted date' - ) - ); - }; - try { + } 'Koha::Exceptions::WrongParameter', 'from parameter must be a DateTime object'; + throws_ok { $count = $patrons->filter_by_last_update( - { timestamp_column_name => 'updated_on', from => '31/12/1970' } ) + { timestamp_column_name => 'updated_on', to => '1970-12-31' } ) ->count; - } - catch { - ok( - $_->isa( - 'No exception raised, from and to parameters can take an metric formatted date (depending on dateformat syspref)' - ) - ); - }; + } 'Koha::Exceptions::WrongParameter', 'to parameter must be a DateTime object'; subtest 'Parameters older_than, younger_than' => sub { my $now = dt_from_string(); diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index 98ee38312e..b3b7266aab 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -37,6 +37,8 @@ use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); use C4::Members qw( GetBorrowersToExpunge ); + +use Koha::DateUtils qw( dt_from_string ); use Koha::Old::Checkouts; use Koha::Patron::Categories; use Koha::Patrons; @@ -148,7 +150,7 @@ elsif ( $step == 3 ) { my $rows = Koha::Old::Checkouts ->filter_by_anonymizable ->filter_by_last_update({ - to => $last_issue_date, timestamp_column_name => 'returndate' }) + to => dt_from_string($last_issue_date), timestamp_column_name => 'returndate' }) ->anonymize; $template->param(