From 9b4b43656ebe693b1445611d4d80b808cb7c6263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Sat, 11 Jun 2022 10:50:06 +0000 Subject: [PATCH] Bug 30947: Simplify CanBookBeIssued date handling 1) This removes support for passing string dates to CanBookBeIssued. The function didn't publicly even document support for string dates, only DateTime objects. 2) We get a $duedate always at least from CalcDateDue so having $issuingimpossible{INVALID_DATE} = output_pref($duedate); was unneccesary and thus removed. 3) The check "duedate cannot be before now" was needlessly complex: if the due date really cannot be before now we should check seconds too and warn the librarian! Thus the truncation to minutes can be dropped safely. To test: 1) prove t/db_dependent/Circulation.t 2) prove t/db_dependent/Illrequests.t 3) Enable OnSiteCheckouts and disable SpecifyDueDate syspref. Create on-site checkout for any patron and verify the due date is your current date at 23:59, you can check the exact minute with sql: > select * from issues Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- C4/Circulation.pm | 27 ++++----------------------- Koha/Illrequest.pm | 2 +- circ/circulation.pl | 5 +++-- t/db_dependent/Circulation.t | 8 ++++---- 4 files changed, 12 insertions(+), 30 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index f5df973d1d..27ba1ee06c 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -794,20 +794,11 @@ sub CanBookBeIssued { my $patron_unblessed = $patron->unblessed; my $circ_library = Koha::Libraries->find( _GetCircControlBranch($item_unblessed, $patron_unblessed) ); - # - # DUE DATE is OK ? -- should already have checked. - # - if ($duedate && ref $duedate ne 'DateTime') { - $duedate = dt_from_string($duedate); - } + my $now = dt_from_string(); - unless ( $duedate ) { - my $issuedate = $now->clone(); - - $duedate = CalcDateDue( $issuedate, $effective_itemtype, $circ_library->branchcode, $patron_unblessed ); - - # Offline circ calls AddIssue directly, doesn't run through here - # So issuingimpossible should be ok. + $duedate ||= CalcDateDue( $now->clone(), $effective_itemtype, $circ_library->branchcode, $patron_unblessed ); + if (DateTime->compare($duedate,$now) == -1 ) { # duedate cannot be before now + $needsconfirmation{INVALID_DATE} = output_pref($duedate); } my $fees = Koha::Charges::Fees->new( @@ -819,16 +810,6 @@ sub CanBookBeIssued { } ); - if ($duedate) { - my $today = $now->clone(); - $today->truncate( to => 'minute'); - if (DateTime->compare($duedate,$today) == -1 ) { # duedate cannot be before now - $needsconfirmation{INVALID_DATE} = output_pref($duedate); - } - } else { - $issuingimpossible{INVALID_DATE} = output_pref($duedate); - } - # # BORROWER STATUS # diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm index 6d26418cd9..a04ea5a553 100644 --- a/Koha/Illrequest.pm +++ b/Koha/Illrequest.pm @@ -1222,7 +1222,7 @@ sub check_out { scalar $target_item->barcode ); if ($params->{duedate} && length $params->{duedate} > 0) { - push @issue_args, $params->{duedate}; + push @issue_args, dt_from_string($params->{duedate}); } # Check if we can check out my ( $error, $confirm, $alerts, $messages ) = diff --git a/circ/circulation.pl b/circ/circulation.pl index 614547b65a..2265ffe2c7 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -196,8 +196,9 @@ my ($datedue,$invalidduedate); my $duedatespec_allow = C4::Context->preference('SpecifyDueDate'); if( $onsite_checkout && !$duedatespec_allow ) { - $datedue = output_pref({ dt => dt_from_string, dateonly => 1, dateformat => 'iso' }); - $datedue .= ' 23:59:00'; + $datedue = dt_from_string()->truncate(to => 'day'); + $datedue->set_hour(23); + $datedue->set_minute(59); } elsif( $duedatespec_allow ) { if ( $duedatespec ) { $datedue = eval { dt_from_string( $duedatespec ) }; diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 274ba2d2f8..53a3285c6d 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -3837,9 +3837,9 @@ subtest 'CanBookBeIssued | is_overdue' => sub { } ); - my $now = dt_from_string; - my $five_days_go = output_pref({ dt => $now->clone->add( days => 5 ), dateonly => 1}); - my $ten_days_go = output_pref({ dt => $now->clone->add( days => 10), dateonly => 1 }); + my $now = dt_from_string()->truncate( to => 'day' ); + my $five_days_go = $now->clone->add( days => 5 ); + my $ten_days_go = $now->clone->add( days => 10); my $library = $builder->build( { source => 'Branch' } ); my $patron = $builder->build_object( { class => 'Koha::Patrons', value => { categorycode => $patron_category->{categorycode} } } ); @@ -3851,7 +3851,7 @@ subtest 'CanBookBeIssued | is_overdue' => sub { my $issue = AddIssue( $patron->unblessed, $item->barcode, $five_days_go ); # date due was 10d ago my $actualissue = Koha::Checkouts->find( { itemnumber => $item->itemnumber } ); - is( output_pref({ str => $actualissue->date_due, dateonly => 1}), $five_days_go, "First issue works"); + is( output_pref({ str => $actualissue->date_due, dateonly => 1}), output_pref({ str => $five_days_go, dateonly => 1}), "First issue works"); my ($issuingimpossible, $needsconfirmation) = CanBookBeIssued($patron,$item->barcode,$ten_days_go, undef, undef, undef); is( $needsconfirmation->{RENEW_ISSUE}, 1, "This is a renewal"); is( $needsconfirmation->{TOO_MANY}, undef, "Not too many, is a renewal");