From fb70ce5af105c488040f778d65140e7383e62ad5 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 9 Jul 2015 09:52:28 +0100 Subject: [PATCH] Bug 14494: Prevent slow checkout if the patron does not have an expiry date If a patron has a expiry date set to 9999-12-31 (for organizations for instance), the checkouts are very slow. It's caused by 2 different calls to DateTime in CanBookBeIssued: 1/ DateTime->new( year => 9999, month => 12, day => 31, time_zone => C4::Context->tz ); The time_zone should not be set (as it's done in Koha::DateUtils), set to UTC or floating tz. 2/ DateTime->compare($today, $expiry_dt) The comparaison of 2 DT with 1 related to 9999 is very slow, as you can imagine. For 1/ we need to call Koha::DateUtils::dt_from_string (actually, we should never call DateTime directly). For 2/ we just need to test if the date is != 9999, no need to compare it in this case. Test plan: Before this patch, confirm that the checkouts are slow if the patron has a dateexpiry set to 9999-12-31. update borrowers set dateexpiry="9999-12-31" where borrowernumber=42; After this patch, you should not see any regression when checking out items to an expired patron and to a valid patron. --- C4/Circulation.pm | 21 ++++++--------------- t/db_dependent/Circulation.t | 41 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index de4b299..aac82bc 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -767,24 +767,15 @@ sub CanBookBeIssued { if ( !defined $borrower->{dateexpiry} || $borrower->{'dateexpiry'} eq '0000-00-00') { $issuingimpossible{EXPIRED} = 1; } else { - my ($y, $m, $d) = split /-/,$borrower->{'dateexpiry'}; - if ($y && $m && $d) { # are we really writing oinvalid dates to borrs - my $expiry_dt = DateTime->new( - year => $y, - month => $m, - day => $d, - time_zone => C4::Context->tz, - ); - $expiry_dt->truncate( to => 'day'); - my $today = $now->clone()->truncate(to => 'day'); - if (DateTime->compare($today, $expiry_dt) == 1) { - $issuingimpossible{EXPIRED} = 1; - } - } else { - carp("Invalid expity date in borr"); + my $expiry_dt = dt_from_string( $borrower->{dateexpiry}, 'sql' ); + $expiry_dt->truncate( to => 'day'); + my $today = $now->clone()->truncate(to => 'day'); + + if ($expiry_dt->year < 9999 && DateTime->compare($today, $expiry_dt) == 1) { $issuingimpossible{EXPIRED} = 1; } } + # # BORROWER STATUS # diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 30a5fe7..bb8b3bf 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,6 +18,7 @@ use Modern::Perl; use DateTime; +use Time::HiRes qw( gettimeofday ); use C4::Biblio; use C4::Branch; use C4::Items; @@ -26,8 +27,9 @@ use C4::Reserves; use C4::Overdues qw(UpdateFine); use Koha::DateUtils; use Koha::Database; +use t::lib::TestBuilder; -use Test::More tests => 61; +use Test::More tests => 65; BEGIN { use_ok('C4::Circulation'); @@ -594,6 +596,43 @@ C4::Context->dbh->do("DELETE FROM accountlines"); is ( $count, 0, "Calling UpdateFine on non-existant fine with an amount of 0 does not result in an empty fine" ); } +{ + my $builder = t::lib::TestBuilder->new(); + my $item = $builder->build( { source => 'Item' } ); + my $patron = $builder->build( + { source => 'Borrower', + value => { dateexpiry => '9999-12-31' } + } + ); + $patron->{flags} = C4::Members::patronflags($borrower); + my $duration = gettimeofday(); + my ( $issuingimpossible, $needsconfirmation ) = C4::Circulation::CanBookBeIssued( $patron, $item->{barcode} ); + $duration = gettimeofday() - $duration; + cmp_ok $duration, '<', 1, "CanBookBeIssued should not be take more than 1s if the patron is expired"; + is( not( exists $issuingimpossible->{EXPIRED} ), 1, 'The patron should not be considered as expired if dateexpiry is 9999-*' ); + + $item = $builder->build( { source => 'Item' } ); + $patron = $builder->build( + { source => 'Borrower', + value => { dateexpiry => '0000-00-00' } + } + ); + $patron->{flags} = C4::Members::patronflags($borrower); + ( $issuingimpossible, $needsconfirmation ) = C4::Circulation::CanBookBeIssued( $patron, $item->{barcode} ); + is( $issuingimpossible->{EXPIRED}, 1, 'The patron should be considered as expired if dateexpiry is 0000-00-00' ); + + my $tomorrow = dt_from_string->add_duration( DateTime::Duration->new( days => 1 ) ); + $item = $builder->build( { source => 'Item' } ); + $patron = $builder->build( + { source => 'Borrower', + value => { dateexpiry => output_pref( { dt => $tomorrow, dateonly => 1, dateformat => 'sql' } ) }, + } + ); + $patron->{flags} = C4::Members::patronflags($borrower); + ( $issuingimpossible, $needsconfirmation ) = C4::Circulation::CanBookBeIssued( $patron, $item->{barcode} ); + is( not( exists $issuingimpossible->{EXPIRED} ), 1, 'The patron should not be considered as expired if dateexpiry is tomorrow' ); +} + $dbh->rollback; 1; -- 2.1.0