From 31665e7588118de4ee3c31ecceb8f481f6b4b0e8 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Fri, 4 Sep 2020 12:47:58 +0000
Subject: [PATCH] [19.11] 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 0cedf80d9c..97695c385f 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -2683,11 +2683,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' );
@@ -2778,22 +2778,21 @@ sub CanBookBeRenewed {
 
             if ( $soonestrenewal > dt_from_string() )
             {
-                return ( 0, "auto_too_soon" ) if $issue->auto_renew;
-                return ( 0, "too_soon" );
+                $auto_renew = ($issue->auto_renew) ? "auto_too_soon" : "too_soon";
             }
             elsif ( $issue->auto_renew ) {
-                $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 ) {
+        if ( $issue->auto_renew && $auto_renew eq "no" ) {
             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";
             }
         }
     }
@@ -2863,8 +2862,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 fb1cf03ba9..92ed6f0ad8 100755
--- a/misc/cronjobs/automatic_renewals.pl
+++ b/misc/cronjobs/automatic_renewals.pl
@@ -76,7 +76,7 @@ my %report;
 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' ) {
         my $date_due = AddRenewal( $auto_renew->borrowernumber, $auto_renew->itemnumber, $auto_renew->branchcode );
         $auto_renew->auto_renew_error(undef)->store;
diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index 9b53952774..6d18724b34 100755
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -279,7 +279,7 @@ $dbh->do(
 
 my ( $reused_itemnumber_1, $reused_itemnumber_2 );
 subtest "CanBookBeRenewed tests" => sub {
-    plan tests => 77;
+    plan tests => 81;
 
     C4::Context->set_preference('ItemsDeniedRenewal','');
     # Generate test biblio
@@ -615,10 +615,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