Bug 14702: Refactor GetReserveFee

The code of GetReserveFee was not very clear.
What it did was: check if there are some items not issued. If so and there
are no holds, calculate no fee.

While doing so, I moved the code to charge the fee (in AddReserve) to a small
new sub ChargeReserveFee.

There is no change in behavior.
The follow-up patch adds unit tests.

Test plan:
[1] Make sure that a patron category (X) includes a hold fee.
[2] Select a biblio with 2 items.
[3] Issue one item to another patron.
[4] Place a hold on this biblio by patron with category X. No charge?
[5] Cancel the hold from the previous step.
[6] Use another patron to place another hold on this biblio.
[7] Place hold again by patron with category X. Is it charged?
[8] Cancel that hold again. Issue the second item to another patron.
[9] Place hold again by patron with category X. Is it charged again?

Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 801adc7..639692b 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -103,7 +103,6 @@ BEGIN {
         &GetReservesForBranch
         &GetReservesToBranch
         &GetReserveCount
-        &GetReserveFee
         &GetReserveInfo
         &GetReserveStatus
 
@@ -156,9 +155,6 @@ sub AddReserve {
         $bibitems,  $priority, $resdate, $expdate, $notes,
         $title,      $checkitem, $found
     ) = @_;
-    my $fee =
-          GetReserveFee($borrowernumber, $biblionumber,
-            $bibitems );
     my $dbh     = C4::Context->dbh;
     $resdate = format_date_in_iso( $resdate ) if ( $resdate );
     $resdate = C4::Dates->today( 'iso' ) unless ( $resdate );
@@ -178,21 +174,7 @@ sub AddReserve {
         $waitingdate = $resdate;
     }
 
-    #eval {
     # updates take place here
-    if ( $fee > 0 ) {
-        my $nextacctno = &getnextacctno( $borrowernumber );
-        my $query      = qq{
-        INSERT INTO accountlines
-            (borrowernumber,accountno,date,amount,description,accounttype,amountoutstanding)
-        VALUES
-            (?,?,now(),?,?,'Res',?)
-    };
-        my $usth = $dbh->prepare($query);
-        $usth->execute( $borrowernumber, $nextacctno, $fee,
-            "Reserve Charge - $title", $fee );
-    }
-
     my $query = qq{
         INSERT INTO reserves
             (borrowernumber,biblionumber,reservedate,branchcode,
@@ -207,6 +189,9 @@ sub AddReserve {
         $notes,          $checkitem,    $found,   $waitingdate, $expdate
     );
     my $reserve_id = $sth->{mysql_insertid};
+    # add a reserve fee if needed
+    my $fee = GetReserveFee( $borrowernumber, $biblionumber );
+    ChargeReserveFee( $borrowernumber, $fee, $title );
 
     # Send e-mail to librarian if syspref is active
     if(C4::Context->preference("emailLibrarianWhenHoldIsPlaced")){
@@ -655,91 +640,50 @@ sub GetOtherReserves {
     return ( $messages, $nextreservinfo );
 }
 
-=head2 GetReserveFee
+=head2 ChargeReserveFee
 
-  $fee = GetReserveFee($borrowernumber,$biblionumber,$biblionumber);
+    $fee = ChargeReserveFee( $borrowernumber, $fee, $title );
 
-Calculate the fee for a reserve
+    Charge the fee for a reserve (if $fee > 0)
 
 =cut
 
-sub GetReserveFee {
-    my ($borrowernumber, $biblionumber, $bibitems ) = @_;
-
-    #check for issues;
-    my $dbh   = C4::Context->dbh;
-    my $query = qq{
-      SELECT * FROM borrowers
-    LEFT JOIN categories ON borrowers.categorycode = categories.categorycode
-    WHERE borrowernumber = ?
+sub ChargeReserveFee {
+    my ( $borrowernumber, $fee, $title ) = @_;
+    return if !$fee;
+    my $accquery = qq{
+INSERT INTO accountlines ( borrowernumber, accountno, date, amount, description, accounttype, amountoutstanding ) VALUES (?, ?, NOW(), ?, ?, 'Res', ?)
     };
-    my $sth = $dbh->prepare($query);
-    $sth->execute($borrowernumber);
-    my $data = $sth->fetchrow_hashref;
-    my $fee      = $data->{'reservefee'};
-    my $cntitems = @- > $bibitems;
+    my $dbh = C4::Context->dbh;
+    my $nextacctno = &getnextacctno( $borrowernumber );
+    $dbh->do( $accquery, undef, ( $borrowernumber, $nextacctno, $fee, "Reserve Charge - $title", $fee ) );
+}
 
-    if ( $fee > 0 ) {
+=head2 GetReserveFee
 
-        # check for items on issue
-        # first find biblioitem records
-        my @biblioitems;
-        my $sth1 = $dbh->prepare(
-            "SELECT * FROM biblio LEFT JOIN biblioitems on biblio.biblionumber = biblioitems.biblionumber
-                   WHERE (biblio.biblionumber = ?)"
-        );
-        $sth1->execute($biblionumber);
-        while ( my $data1 = $sth1->fetchrow_hashref ) {
-            my $found = 0;
-            my $x     = 0;
-            while ( $x < $cntitems ) {
-                if ( @$bibitems->{'biblioitemnumber'} ==
-                    $data->{'biblioitemnumber'} )
-                {
-                    $found = 1;
-                }
-                $x++;
-            }
+    $fee = GetReserveFee( $borrowernumber, $biblionumber );
 
-            if ( $found == 0 ) {
-                push @biblioitems, $data1;
-            }
-        }
-        my $cntitemsfound = @biblioitems;
-        my $issues        = 0;
-        my $x             = 0;
-        my $allissued     = 1;
-        while ( $x < $cntitemsfound ) {
-            my $bitdata = $biblioitems[$x];
-            my $sth2    = $dbh->prepare(
-                "SELECT * FROM items
-                     WHERE biblioitemnumber = ?"
-            );
-            $sth2->execute( $bitdata->{'biblioitemnumber'} );
-            while ( my $itdata = $sth2->fetchrow_hashref ) {
-                my $sth3 = $dbh->prepare(
-                    "SELECT * FROM issues
-                       WHERE itemnumber = ?"
-                );
-                $sth3->execute( $itdata->{'itemnumber'} );
-                if ( my $isdata = $sth3->fetchrow_hashref ) {
-                }
-                else {
-                    $allissued = 0;
-                }
-            }
-            $x++;
-        }
-        if ( $allissued == 0 ) {
-            my $rsth =
-              $dbh->prepare("SELECT * FROM reserves WHERE biblionumber = ?");
-            $rsth->execute($biblionumber);
-            if ( my $rdata = $rsth->fetchrow_hashref ) {
-            }
-            else {
-                $fee = 0;
-            }
-        }
+    Calculate the fee for a reserve (if applicable).
+
+=cut
+
+sub GetReserveFee {
+    my ( $borrowernumber, $biblionumber ) = @_;
+    my $borquery = qq{
+SELECT reservefee FROM borrowers LEFT JOIN categories ON borrowers.categorycode = categories.categorycode WHERE borrowernumber = ?
+    };
+
+    my $dbh = C4::Context->dbh;
+    my ( $fee ) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) );
+    if( $fee && $fee > 0 ) {
+        # This is a reconstruction of the old code:
+        # Calculate number of items, items issued and holds
+        my ( $cnt1 ) = $dbh->selectrow_array( "SELECT COUNT(*) FROM items WHERE biblionumber=?", undef, ( $biblionumber ) );
+        my ( $cnt2 ) = $dbh->selectrow_array( "SELECT COUNT(*) FROM issues LEFT JOIN items USING (itemnumber) WHERE biblionumber=?", undef, ( $biblionumber ));
+        my ( $cnt3 ) = $dbh->selectrow_array( "SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>?", undef, ( $biblionumber, $borrowernumber ) );
+        # If not all items are issued and there are no holds: charge no fee
+        # NOTE: Lost, damaged, not-for-loan, etc. are just ignored here
+        $fee = 0 if $cnt1 - $cnt2 > 0 && $cnt3 == 0;
     }
     return $fee;
 }