Attachment 109668 Details for Bug 25758 – Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

[patch] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug-25758-Return-onreserve-over-toosoon-when-not-c.patch (text/plain), 7.09 KB, created by Martin Renvoize (ashimema) on 2020-09-04 15:05:28 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2020-09-04 15:05:28 UTC

Size: 7.09 KB

patch

obsolete

>From e3c978b3b16f52cace2596a7cc8b2bdeef4383c3 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Fri, 4 Sep 2020 12:47:58 +0000
>Subject: [PATCH] Bug 25758: Return on_reserve over too_soon when not calling
> from automatic_renewals cron
>
>Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve
>
>For cron purposes this is the correct behaviour.
>
>For display purposes we wish to see on_reserve over too_soon
>
>This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses
>
>To test:
> 1 - set NoRenewalBeforePrecision to date only
> 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
> 3 - check item out
> 4 - confirm item shows Scheduled For Automatic Renewal in issues table
> 5 - place a hold on the item for another patron
> 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
> 7 - change No Renewal Before value to 7
> 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
> 9 - Apply patch
>10 - restart_all
>11 - Reload the issues table - confirm 'on_hold' still shows
>12 - Change No Renewal Before to 0
>13 - Refresh issues table, still shows 'On hold'
>14 - perl misc/cronjobs/automatic_renewals.pl -v
>15 - Result shows 'auto_too_soon'
>16 - prove -v t/db_dependent/Circulation.t
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> C4/Circulation.pm                   | 26 ++++++++++++++++----------
> misc/cronjobs/automatic_renewals.pl |  2 +-
> t/db_dependent/Circulation.t        | 10 ++++++++--
> 3 files changed, 25 insertions(+), 13 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 3571051e84..830f53b961 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2637,11 +2637,11 @@ already renewed the loan. $error will contain the reason the renewal can not pro
> =cut
> 
> sub CanBookBeRenewed {
>-    my ( $borrowernumber, $itemnumber, $override_limit ) = @_;
>+    my ( $borrowernumber, $itemnumber, $override_limit, $cron ) = @_;
> 
>     my $dbh    = C4::Context->dbh;
>     my $renews = 1;
>-    my $auto_renew = 0;
>+    my $auto_renew = "no";
> 
>     my $item      = Koha::Items->find($itemnumber)      or return ( 0, 'no_item' );
>     my $issue = $item->checkout or return ( 0, 'no_checkout' );
>@@ -2739,22 +2739,21 @@ sub CanBookBeRenewed {
> 
>             if ( $soonestrenewal > dt_from_string() )
>             {
>-                return ( 0, "auto_too_soon" ) if $issue->auto_renew && $patron->autorenew_checkouts;
>-                return ( 0, "too_soon" );
>+                $auto_renew = ($issue->auto_renew && $patron->autorenew_checkouts) ? "auto_too_soon" : "too_soon";
>             }
>             elsif ( $issue->auto_renew && $patron->autorenew_checkouts ) {
>-                $auto_renew = 1;
>+                $auto_renew = "ok";
>             }
>         }
> 
>         # Fallback for automatic renewals:
>         # If norenewalbefore is undef, don't renew before due date.
>-        if ( $issue->auto_renew && !$auto_renew && $patron->autorenew_checkouts ) {
>+        if ( $issue->auto_renew && $auto_renew eq "no" && $patron->autorenew_checkouts ) {
>             my $now = dt_from_string;
>             if ( $now >= dt_from_string( $issue->date_due, 'sql' ) ){
>-                $auto_renew = 1;
>+                $auto_renew = "ok";
>             } else {
>-                return ( 0, "auto_too_soon" );
>+                $auto_renew = "auto_too_soon";
>             }
>         }
>     }
>@@ -2833,8 +2832,15 @@ sub CanBookBeRenewed {
>             }
>         }
>     }
>-    return ( 0, "on_reserve" ) if $resfound;    # '' when no hold was found
>-    return ( 0, "auto_renew" ) if $auto_renew && !$override_limit; # 0 if auto-renewal should not succeed
>+    if( $cron ) { #The cron wants to return 'too_soon' over 'on_reserve'
>+        return ( 0, $auto_renew  ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok";
>+        return ( 0, "on_reserve" ) if $resfound;    # '' when no hold was found
>+    } else { # For other purposes we want 'on_reserve' before 'too_soon'
>+        return ( 0, "on_reserve" ) if $resfound;    # '' when no hold was found
>+        return ( 0, $auto_renew  ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok";
>+    }
>+
>+    return ( 0, "auto_renew" ) if $auto_renew eq "ok" && !$override_limit; # 0 if auto-renewal should not succeed
> 
>     return ( 1, undef );
> }
>diff --git a/misc/cronjobs/automatic_renewals.pl b/misc/cronjobs/automatic_renewals.pl
>index a76f7dfb35..1c47cfd680 100755
>--- a/misc/cronjobs/automatic_renewals.pl
>+++ b/misc/cronjobs/automatic_renewals.pl
>@@ -88,7 +88,7 @@ print "Test run only\n" unless $confirm;
> while ( my $auto_renew = $auto_renews->next ) {
> 
>     # CanBookBeRenewed returns 'auto_renew' when the renewal should be done by this script
>-    my ( $ok, $error ) = CanBookBeRenewed( $auto_renew->borrowernumber, $auto_renew->itemnumber );
>+    my ( $ok, $error ) = CanBookBeRenewed( $auto_renew->borrowernumber, $auto_renew->itemnumber, undef, 1 );
>     if ( $error eq 'auto_renew' ) {
>         if ($verbose) {
>             say sprintf "Issue id: %s for borrower: %s and item: %s ". ( $confirm ? 'will' : 'would') . " be renewed.",
>diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
>index f26f2f3255..8ea7ac50eb 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -278,7 +278,7 @@ Koha::CirculationRules->set_rules(
> 
> my ( $reused_itemnumber_1, $reused_itemnumber_2 );
> subtest "CanBookBeRenewed tests" => sub {
>-    plan tests => 83;
>+    plan tests => 87;
> 
>     C4::Context->set_preference('ItemsDeniedRenewal','');
>     # Generate test biblio
>@@ -649,10 +649,16 @@ subtest "CanBookBeRenewed tests" => sub {
>     );
>     ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'Still should not be able to renew' );
>-    is( $error, 'auto_too_soon', 'returned code is auto_too_soon, reserve not checked' );
>+    is( $error, 'on_reserve', 'returned code is on_reserve, reserve checked when not checking for cron' );
>+    ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, undef, 1 );
>+    is( $renewokay, 0, 'Still should not be able to renew' );
>+    is( $error, 'auto_too_soon', 'returned code is auto_too_soon, reserve not checked when checking for cron' );
>     ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, 1 );
>     is( $renewokay, 0, 'Still should not be able to renew' );
>     is( $error, 'on_reserve', 'returned code is on_reserve, auto_too_soon limit is overridden' );
>+    ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, 1, 1 );
>+    is( $renewokay, 0, 'Still should not be able to renew' );
>+    is( $error, 'on_reserve', 'returned code is on_reserve, auto_too_soon limit is overridden' );
>     $dbh->do('UPDATE circulation_rules SET rule_value = 0 where rule_name = "norenewalbefore"');
>     ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, 1 );
>     is( $renewokay, 0, 'Still should not be able to renew' );
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 25758: 106229 | 107409 | 107495 | 107496 | 107504 | 107879 | 107894 | 109139 | 109658 | 109668 | 109672 | 109680 | 110008 | 110009 | 110010