From 0355c440c56f649c7e425314267d2cff4c583e28 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Tue, 26 Sep 2023 13:53:44 +0000
Subject: [PATCH] Bug 34924: Add Koha::Checkout->attempt_auto_renew

This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script.

To test:
1 - prove -v t/db_dependent/Koha/Checkouts.t
2 - Add a circ rule with auto_renew checked
3 - Checkout an item to a patron and set due date in the past
4 - Checkout an item to a patron and set due date in the future
5 - perl misc/cronjobs/automatic_renewals.pl -v
6 - Confirm one would be renewed and the other is too_soon
7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
8 - Confirm the expected issue is successfully renewed
---
 Koha/Checkout.pm                    | 44 +++++++++++++++++-
 misc/cronjobs/automatic_renewals.pl | 20 ++-------
 t/db_dependent/Koha/Checkouts.t     | 70 ++++++++++++++++++++++++++++-
 3 files changed, 114 insertions(+), 20 deletions(-)

diff --git a/Koha/Checkout.pm b/Koha/Checkout.pm
index 178a8c0da4..dab86db7b2 100644
--- a/Koha/Checkout.pm
+++ b/Koha/Checkout.pm
@@ -23,7 +23,7 @@ use Modern::Perl;
 use DateTime;
 use Try::Tiny qw( catch try );
 
-use C4::Circulation qw( LostItem MarkIssueReturned );
+use C4::Circulation qw( AddRenewal CanBookBeRenewed LostItem MarkIssueReturned );
 use Koha::Checkouts::Renewals;
 use Koha::Checkouts::ReturnClaims;
 use Koha::Database;
@@ -151,6 +151,48 @@ sub renewals {
     return Koha::Checkouts::Renewals->_new_from_dbic( $renewals_rs );
 }
 
+=head3 attempt_auto_renew
+
+  my ($success, $error, $updated) = $checkout->auto_renew({ confirm => 1 });
+
+Attempt to automatically renew a book. Return error reason if it cannot be renewed.
+Also return whether a change has been made to avoid notifying on more than one attempt.
+
+If not passed confirm, we will only report and no changes will be made.
+
+=cut
+
+sub attempt_auto_renew {
+    my ( $self, $params ) = @_;
+    my $confirm = $params->{confirm} // 0;
+
+    # CanBookBeRenewed returns 'auto_renew' when the renewal should be done by this script
+    my ( $ok, $error ) = C4::Circulation::CanBookBeRenewed( $self->patron, $self, undef, 1 );
+    if ( $error eq 'auto_renew' ) {
+        if ($confirm) {
+            my $date_due = C4::Circulation::AddRenewal(
+                {
+                    borrowernumber => $self->borrowernumber,
+                    itemnumber     => $self->itemnumber,
+                    branch         => $self->branchcode,
+                    seen           => 0,
+                    automatic      => 1,
+                }
+            );
+            $self->auto_renew_error(undef)->store;
+        }
+        return ( 1, undef, 1 );
+    } else {
+        my $updated = 0;
+        if ( !$self->auto_renew_error || $error ne $self->auto_renew_error ) {
+            $self->auto_renew_error($error)->store if $confirm;
+            $updated = 1;
+        }
+        return ( 0, $error, $updated );
+    }
+
+}
+
 =head3 to_api_mapping
 
 This method returns the mapping for representing a Koha::Checkout object
diff --git a/misc/cronjobs/automatic_renewals.pl b/misc/cronjobs/automatic_renewals.pl
index 4fa83566a8..7ad80a2fb6 100755
--- a/misc/cronjobs/automatic_renewals.pl
+++ b/misc/cronjobs/automatic_renewals.pl
@@ -171,31 +171,19 @@ while ( my $auto_renew = $auto_renews->next ) {
         $wants_digest = 0;
     }
 
-    # CanBookBeRenewed returns 'auto_renew' when the renewal should be done by this script
-    my ( $ok, $error ) = CanBookBeRenewed( $auto_renew->patron, $auto_renew, undef, 1 );
-    my $updated;
-    if ( $error eq 'auto_renew' ) {
-        $updated = 1;
+    my ( $success, $error, $updated ) = $auto_renew->attempt_auto_renew();
+    if ( $success ) {
         if ($verbose) {
             say sprintf "Issue id: %s for borrower: %s and item: %s %s be renewed.",
               $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, $confirm ? 'will' : 'would';
         }
         if ($confirm){
-            my $date_due = AddRenewal(
-                {
-                    borrowernumber    => $auto_renew->borrowernumber,
-                    itemnumber        => $auto_renew->itemnumber,
-                    branch            => $auto_renew->branchcode,
-                    seen              => 0,
-                    automatic         => 1,
-                }
-            );
             push @item_renewal_ids, $auto_renew->itemnumber;
-            $auto_renew->auto_renew_error(undef)->store;
         }
         push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew
             if ( $wants_messages ) && !$wants_digest;
     } elsif (
+        # FIXME Do we need to list every status? Why not simply else?
         $error eq 'too_many' ||
         $error eq 'on_reserve' ||
         $error eq 'restriction' ||
@@ -212,9 +200,7 @@ while ( my $auto_renew = $auto_renews->next ) {
             say sprintf "Issue id: %s for borrower: %s and item: %s %s not be renewed. (%s)",
               $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, $confirm ? 'will' : 'would', $error;
         }
-        $updated = 1 if (!$auto_renew->auto_renew_error || $error ne $auto_renew->auto_renew_error);
         if ( $updated ) {
-            $auto_renew->auto_renew_error($error)->store if $confirm;
             push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew
               if $error ne 'auto_too_soon' && ( $wants_messages  && !$wants_digest );    # Do not notify if it's too soon
         }
diff --git a/t/db_dependent/Koha/Checkouts.t b/t/db_dependent/Koha/Checkouts.t
index 4373904142..c776985fb1 100755
--- a/t/db_dependent/Koha/Checkouts.t
+++ b/t/db_dependent/Koha/Checkouts.t
@@ -19,7 +19,9 @@
 
 use Modern::Perl;
 
-use Test::More tests => 11;
+use Test::More tests => 12;
+use Test::MockModule;
+use Test::Warn;
 
 use C4::Circulation qw( MarkIssueReturned AddReturn );
 use Koha::Checkouts;
@@ -406,4 +408,68 @@ subtest 'automatic_checkin' => sub {
     is( dt_from_string($searched->returndate), $yesterday, 'old checkout for odue_ac_item has the right return date' );
 
     $schema->storage->txn_rollback;
-}
+};
+
+subtest 'attempt_auto_renew' => sub {
+
+    plan tests => 17;
+
+    $schema->storage->txn_begin;
+
+    my $renew_error = 'auto_renew';
+    my $module      = Test::MockModule->new('C4::Circulation');
+    $module->mock( 'CanBookBeRenewed', sub { return ( 1, $renew_error ) } );
+    my $around_now = dt_from_string();
+    $module->mock( 'AddRenewal', sub { warn "AddRenewal called" } );
+    my $checkout = $builder->build_object(
+        {
+            class => 'Koha::Checkouts',
+            value => {
+                date_due         => '2023-01-01 23:59:59',
+                returndate       => undef,
+                auto_renew       => 1,
+                auto_renew_error => undef,
+                onsite_checkout  => 0,
+                renewals_count   => 0,
+            }
+        }
+    );
+
+    my ( $success, $error, $updated );
+    warning_is {
+        ( $success, $error, $updated ) = $checkout->attempt_auto_renew();
+    }
+    undef, "AddRenewal not called without confirm";
+    ok( $success, "Issue is renewed when error is 'auto_renew'" );
+    is( $error, undef, "No error when renewed" );
+    ok( $updated, "Issue reported as updated when renewed" );
+
+    warning_is {
+        ( $success, $error, $updated ) = $checkout->attempt_auto_renew( { confirm => 1 } );
+    }
+    "AddRenewal called", "AddRenewal called when confirm is passed";
+    ok( $success, "Issue is renewed when error is 'auto_renew'" );
+    is( $error, undef, "No error when renewed" );
+    ok( $updated, "Issue reported as updated when renewed" );
+
+    $renew_error = 'anything_else';
+    ( $success, $error, $updated ) = $checkout->attempt_auto_renew();
+    ok( !$success, "Success is untrue for any other status" );
+    is( $error, 'anything_else', "The error is passed through" );
+    ok( $updated, "Issue reported as updated when status changes" );
+    $checkout->discard_changes();
+    is( $checkout->auto_renew_error, undef, "Error not updated if confirm not passed" );
+
+    ( $success, $error, $updated ) = $checkout->attempt_auto_renew( { confirm => 1 } );
+    ok( !$success, "Success is untrue for any other status" );
+    is( $error, 'anything_else', "The error is passed through" );
+    ok( $updated, "Issue updated when confirm passed" );
+    $checkout->discard_changes();
+    is( $checkout->auto_renew_error, 'anything_else', "Error updated if confirm passed" );
+
+    # Error now equals 'anything_else'
+    ( $success, $error, $updated ) = $checkout->attempt_auto_renew();
+    ok( !$updated, "Issue not reported as updated when status has not changed" );
+
+    $schema->storage->txn_rollback;
+};
-- 
2.30.2