From ec3b31a9dccdfa757043593dbe0d187f1cda50bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@iki.fi>
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
---
 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 d92cdcc0b2..f0e04e7112 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -789,20 +789,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.
+    my $now = dt_from_string();
+    $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(
@@ -814,16 +805,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 f0a6bd57a6..31f95b5bcf 100755
--- a/circ/circulation.pl
+++ b/circ/circulation.pl
@@ -197,8 +197,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 1322e54fbb..9ca6dff6ec 100755
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -3832,9 +3832,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} } } );
 
@@ -3846,7 +3846,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");
-- 
2.30.2