[Koha-patches] [PATCH] CanBookBeRenewed Overhaul

[Joe Atzberger]

Note: the logic of the function is chanaged.  Previously, if no such issue was found
for the borrower/patron, you would get return of (0, undef).  Also, if we encounter
a renewal error, we now return (0, $error_string) immediately, since there is no
point checking for other errors, when we can only return one anyway.  That makes it
different than the $messages hashref used elsewhere in Circ, the latter being
preferable.  However, that also affects the precedence, where the last error
encountered used to be the one returned.

POD updated.
---
 C4/Circulation.pm |   77 ++++++++++++++++++++---------------------------------
 1 files changed, 29 insertions(+), 48 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 9199cd9..e8fa45b 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1996,8 +1996,6 @@ END_SQL
 
 Find out whether a borrowed item may be renewed.
 
-C<$dbh> is a DBI handle to the Koha database.
-
 C<$borrowernumber> is the borrower number of the patron who currently
 has the item on loan.
 
@@ -2007,67 +2005,50 @@ C<$override_limit>, if supplied with a true value, causes
 the limit on the number of times that the loan can be renewed
 (as controlled by the item type) to be ignored.
 
-C<$CanBookBeRenewed> returns a true value iff the item may be renewed. The
+C<$CanBookBeRenewed> returns a true C<$ok> value iff the item may be renewed. The
 item must currently be on loan to the specified borrower; renewals
 must be allowed for the item's type; and the borrower must not have
-already renewed the loan. $error will contain the reason the renewal can not proceed
+already renewed the loan more than allowed. C<$error> will contain the reason the renewal cannot proceed.
 
 =cut
 
 sub CanBookBeRenewed {
-
-    # check renewal status
     my ( $borrowernumber, $itemnumber, $override_limit ) = @_;
-    my $dbh       = C4::Context->dbh;
-    my $renews    = 1;
-    my $renewokay = 0;
-	my $error;
+    my $dbh = C4::Context->dbh;
 
-    # Look in the issues table for this item, lent to this borrower,
-    # and not yet returned.
+    # Look in the issues table for this item, lent to this borrower, and not yet returned.
 
     # FIXME - I think this function could be redone to use only one SQL call.
     my $sth1 = $dbh->prepare(
         "SELECT * FROM issues
-            WHERE borrowernumber = ?
-            AND itemnumber = ?"
+         WHERE borrowernumber = ?
+          AND  itemnumber     = ?"
     );
     $sth1->execute( $borrowernumber, $itemnumber );
-    if ( my $data1 = $sth1->fetchrow_hashref ) {
-
-        # Found a matching item
-
-        # See if this item may be renewed. This query is convoluted
-        # because it's a bit messy: given the item number, we need to find
-        # the biblioitem, which gives us the itemtype, which tells us
-        # whether it may be renewed.
-        my $query = "SELECT renewalsallowed FROM items ";
-        $query .= (C4::Context->preference('item-level_itypes'))
-                    ? "LEFT JOIN itemtypes ON items.itype = itemtypes.itemtype "
-                    : "LEFT JOIN biblioitems on items.biblioitemnumber = biblioitems.biblioitemnumber
-                       LEFT JOIN itemtypes ON biblioitems.itemtype = itemtypes.itemtype ";
-        $query .= "WHERE items.itemnumber = ?";
-        my $sth2 = $dbh->prepare($query);
-        $sth2->execute($itemnumber);
-        if ( my $data2 = $sth2->fetchrow_hashref ) {
-            $renews = $data2->{'renewalsallowed'};
-        }
-        if ( ( $renews && $renews > $data1->{'renewals'} ) || $override_limit ) {
-            $renewokay = 1;
-        }
-        else {
-			$error="too_many";
-		}
-        $sth2->finish;
-        my ( $resfound, $resrec ) = C4::Reserves::CheckReserves($itemnumber);
-        if ($resfound) {
-            $renewokay = 0;
-			$error="on_reserve"
-        }
+    my $data1 = $sth1->fetchrow_hashref;
+    $data1 or return (0, "not_issued");     # bail if it wasn't checked out (to that patron)
 
+    # See if this item may be renewed. This query is convoluted
+    # because it's a bit messy: given the item number, we need to find
+    # the biblioitem, which gives us the itemtype, which tells us
+    # whether it may be renewed.
+    my $query = "SELECT renewalsallowed FROM items ";
+    $query .= (C4::Context->preference('item-level_itypes'))
+                ? "LEFT JOIN itemtypes   ON            items.itype = itemtypes.itemtype "
+                : "LEFT JOIN biblioitems ON items.biblioitemnumber = biblioitems.biblioitemnumber
+                   LEFT JOIN itemtypes   ON   biblioitems.itemtype = itemtypes.itemtype ";
+    $query .= "WHERE items.itemnumber = ?";
+    my $sth2 = $dbh->prepare($query);
+    $sth2->execute($itemnumber);
+    my $data2 = $sth2->fetchrow_hashref;
+    my $renews = $data2->{'renewalsallowed'} || 1;  # DEFAULT 1 renewal available, if query hit nothing or NULL
+                                                    # FIXME: default probably should be overridable via syspref or circ rule
+    unless ($renews > $data1->{'renewals'} or $override_limit) {
+        return (0, "too_many");
     }
-    $sth1->finish;
-    return ($renewokay,$error);
+    my ($resfound, $resrec) = C4::Reserves::CheckReserves($itemnumber);
+    return (0, "on_reserve") if ($resfound);
+    return (1);
 }
 
 =head2 AddRenewal
-- 
1.5.6.5