From a53ca7848b783f2d66fcb11bba75b660d0a3e874 Mon Sep 17 00:00:00 2001
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Date: Wed, 24 Mar 2021 16:38:32 +0000
Subject: [PATCH] Bug 25508: Only return renewal outcomes to the controller

There are a few cases where the `renew_item` method in
Koha::Account::Line will return `undef`. For these cases, we should not
pass the error up the chain to the controllers as it leads to malformed
error messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(cherry picked from commit d4dc15ee36dc108e074e4d1b78edc055b1a22597)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
(cherry picked from commit b51efada26130d16264486514c59e8dca12639a9)

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
---
 Koha/Account.pm      |  4 ++--
 Koha/Account/Line.pm | 13 +++++--------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/Koha/Account.pm b/Koha/Account.pm
index 86ab6ee391..f2681665de 100644
--- a/Koha/Account.pm
+++ b/Koha/Account.pm
@@ -203,9 +203,9 @@ sub pay {
         # If we need to make a note of the item associated with this line,
         # in order that we can potentially renew it, do so.
         my $amt = $old_amountoutstanding - $amount_to_pay;
-        if ($fine->renewable) {
+        if ( $fine->renewable ) {
             my $outcome = $fine->renew_item;
-            push @{$renew_outcomes}, $outcome;
+            push @{$renew_outcomes}, $outcome if $outcome;
         }
 
         if ( C4::Context->preference('MarkLostItemsAsReturned') =~ m|onpayment|
diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm
index 7ed59a9ed0..012c37242a 100644
--- a/Koha/Account/Line.pm
+++ b/Koha/Account/Line.pm
@@ -806,7 +806,9 @@ sub renewable {
         $self->debit_type_code &&
         $self->debit_type_code eq 'OVERDUE' &&
         $self->status &&
-        $self->status eq 'UNRETURNED'
+        $self->status eq 'UNRETURNED' &&
+        $self->item &&
+        $self->patron
     ) ? 1 : 0;
 }
 
@@ -824,19 +826,14 @@ sub renew_item {
 
     my $outcome = {};
 
-    # We want to reject the call to renew if any of these apply:
+    # We want to reject the call to renew if:
     # - The RenewAccruingItemWhenPaid syspref is off
-    # - The line item doesn't have an item attached to it
-    # - The line item doesn't have a patron attached to it
-    #
+    # OR
     # - The RenewAccruingItemInOpac syspref is off
-    # AND
     # - There is an interface param passed and it's value is 'opac'
 
     if (
         !C4::Context->preference('RenewAccruingItemWhenPaid') ||
-        !$self->item ||
-        !$self->patron ||
         (
             !C4::Context->preference('RenewAccruingItemInOpac') &&
             $params->{interface} &&
-- 
2.39.5