Attachment 136093 Details for Bug 30947 – Bug 30947: Simplify CanBookBeIssued date handling

[patch] Bug 30947: Simplify CanBookBeIssued date handling

Bug-30947-Simplify-CanBookBeIssued-date-handling.patch (text/plain), 5.78 KB, created by Nick Clemens (kidclamp) on 2022-06-15 12:25:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2022-06-15 12:25:19 UTC

Size: 5.78 KB

patch

obsolete

>From 6884ace6c065539c70729e8003120cbbe0a47b86 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
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> 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.
>+    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(
>@@ -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");
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 30947: 135959 | 135960 | 136092 | 136093 | 136158 | 136159 | 136329