Bug 9367: Code optimization: CheckReserves is too often called

This patch rewrites the GetReserveStatus routine in order to take in
parameter the itemnumber and/or the biblionumber.

In some places, the C4::Reserves::CheckReserves routine is called when
we just want to get the status of the reserve. In these cases, the
C4::Reserves::GetReserveStatus is now called.
This routine executes 1 sql query (or 2 max).

Test plan:
Check that there is no regression on the different pages where reserves
are used. The different status will be the same than before applying
this patch.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

diff --git a/C4/Biblio.pm b/C4/Biblio.pm
index 2c7170c..d232fa6 100644 (file)
--- a/C4/Biblio.pm
+++ b/C4/Biblio.pm
@@ -785,20 +785,11 @@ sub GetBiblioData {
     my ($bibnum) = @_;
     my $dbh = C4::Context->dbh;
 
-    #  my $query =  C4::Context->preference('item-level_itypes') ?
-    #   " SELECT * , biblioitems.notes AS bnotes, biblio.notes
-    #       FROM biblio
-    #        LEFT JOIN biblioitems ON biblio.biblionumber = biblioitems.biblionumber
-    #       WHERE biblio.biblionumber = ?
-    #        AND biblioitems.biblionumber = biblio.biblionumber
-    #";
-
     my $query = " SELECT * , biblioitems.notes AS bnotes, itemtypes.notforloan as bi_notforloan, biblio.notes
             FROM biblio
             LEFT JOIN biblioitems ON biblio.biblionumber = biblioitems.biblionumber
             LEFT JOIN itemtypes ON biblioitems.itemtype = itemtypes.itemtype
-            WHERE biblio.biblionumber = ?
-            AND biblioitems.biblionumber = biblio.biblionumber ";
+            WHERE biblio.biblionumber = ?";
 
     my $sth = $dbh->prepare($query);
     $sth->execute($bibnum);

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index f1c8662..0999acf 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -2481,12 +2481,11 @@ sub CanBookBeRenewed {
                        $error="too_many";
                }
                
-        my ( $resfound, $resrec, undef ) = C4::Reserves::CheckReserves($itemnumber);
-        if ($resfound) {
+        my $resstatus = C4::Reserves::GetReserveStatus($itemnumber);
+        if ( $resstatus eq "Waiting" or $resstatus eq "Reserved" ) {
             $renewokay = 0;
-                       $error="on_reserve"
+            $error->{message} = "on_reserve";
         }
-
     }
     return ($renewokay,$error);
 }

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index a380bf0..ee31ea9 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -736,15 +736,29 @@ sub GetReservesForBranch {
 }
 
 sub GetReserveStatus {
-    my ($itemnumber) = @_;
-    
+    my ($itemnumber, $biblionumber) = @_;
+
     my $dbh = C4::Context->dbh;
-    
-    my $itemstatus = $dbh->prepare("SELECT found FROM reserves WHERE itemnumber = ?");
-    
-    $itemstatus->execute($itemnumber);
-    my ($found) = $itemstatus->fetchrow_array;
-    return $found;
+
+    my ($sth, $found, $priority);
+    if ( $itemnumber ) {
+        $sth = $dbh->prepare("SELECT found, priority FROM reserves WHERE itemnumber = ? order by priority LIMIT 1");
+        $sth->execute($itemnumber);
+        ($found, $priority) = $sth->fetchrow_array;
+    }
+
+    if ( $biblionumber and not defined $found and not defined $priority ) {
+        $sth = $dbh->prepare("SELECT found, priority FROM reserves WHERE biblionumber = ? order by priority LIMIT 1");
+        $sth->execute($biblionumber);
+    } else {
+        return;
+    }
+    ($found, $priority) = $sth->fetchrow_array;
+
+    return 'Waiting'  if $found eq 'W' and $priority == 0;
+    return 'Finished' if $found eq 'F';
+    return 'Reserved' if $priority > 0;
+    return;
 }
 
 =head2 CheckReserves
@@ -1441,7 +1455,8 @@ sub IsAvailableForItemLevelRequest {
     if (C4::Context->preference('AllowOnShelfHolds')) {
         return $available_per_item;
     } else {
-        return ($available_per_item and ($item->{onloan} or GetReserveStatus($itemnumber) eq "W")); 
+        my $status = GetReserveStatus($itemnumber);
+        return ($available_per_item and ($item->{onloan} or $status eq "Waiting" or $status = "Reserved"));
     }
 }
 

diff --git a/C4/Search.pm b/C4/Search.pm
index ce6129d..bde9cc8 100644 (file)
--- a/C4/Search.pm
+++ b/C4/Search.pm
@@ -28,7 +28,7 @@ use C4::Dates qw(format_date);
 use C4::Members qw(GetHideLostItemsPreference);
 use C4::XSLT;
 use C4::Branch;
-use C4::Reserves;    # CheckReserves
+use C4::Reserves;    # GetReserveStatus
 use C4::Debug;
 use C4::Charset;
 use YAML;
@@ -1852,8 +1852,7 @@ sub searchResults {
                 my ($transfertfrom, $transfertto);
 
                 # is item on the reserve shelf?
-               my $reservestatus = '';
-               my $reserveitem;
+                my $reservestatus = '';
 
                 unless ($item->{wthdrawn}
                         || $item->{itemlost}
@@ -1874,7 +1873,7 @@ sub searchResults {
                     #        should map transit status to record indexed in Zebra.
                     #
                     ($transfertwhen, $transfertfrom, $transfertto) = C4::Circulation::GetTransfers($item->{itemnumber});
-                   ($reservestatus, $reserveitem, undef) = C4::Reserves::CheckReserves($item->{itemnumber});
+                    $reservestatus = C4::Reserves::GetReserveStatus( $item->{itemnumber}, $oldbiblio->{biblionumber} );
                 }
 
                 # item is withdrawn, lost, damaged, not for loan, reserved or in transit
@@ -1882,14 +1881,14 @@ sub searchResults {
                     || $item->{itemlost}
                     || $item->{damaged}
                     || $item->{notforloan}
-                   || $reservestatus eq 'Waiting'
+                    || $reservestatus eq 'Waiting'
                     || ($transfertwhen ne ''))
                 {
                     $wthdrawn_count++        if $item->{wthdrawn};
                     $itemlost_count++        if $item->{itemlost};
                     $itemdamaged_count++     if $item->{damaged};
                     $item_in_transit_count++ if $transfertwhen ne '';
-                   $item_onhold_count++     if $reservestatus eq 'Waiting';
+                    $item_onhold_count++     if $reservestatus eq 'Waiting';
                     $item->{status} = $item->{wthdrawn} . "-" . $item->{itemlost} . "-" . $item->{damaged} . "-" . $item->{notforloan};
 
                     # can place hold on item ?

diff --git a/C4/XSLT.pm b/C4/XSLT.pm
index ffdc3ab..76a424b 100644 (file)
--- a/C4/XSLT.pm
+++ b/C4/XSLT.pm
@@ -247,7 +247,7 @@ sub buildKohaItemsNamespace {
 
         my ( $transfertwhen, $transfertfrom, $transfertto ) = C4::Circulation::GetTransfers($item->{itemnumber});
 
-       my ( $reservestatus, $reserveitem, undef ) = C4::Reserves::CheckReserves($item->{itemnumber});
+        my $reservestatus = C4::Reserves::GetReserveStatus( $item->{itemnumber} );
 
         if ( $itemtypes->{ $item->{itype} }->{notforloan} || $item->{notforloan} || $item->{onloan} || $item->{wthdrawn} || $item->{itemlost} || $item->{damaged} || 
              (defined $transfertwhen && $transfertwhen ne '') || $item->{itemnotforloan} || (defined $reservestatus && $reservestatus eq "Waiting") ){ 

diff --git a/circ/circulation.pl b/circ/circulation.pl
index bdf0362..3382066 100755 (executable)
--- a/circ/circulation.pl
+++ b/circ/circulation.pl
@@ -448,10 +448,10 @@ sub build_issue_data {
             $it->{'borrowernumber'},$it->{'itemnumber'}
         );
         $it->{"renew_error_${can_renew_error}"} = 1 if defined $can_renew_error;
-        my ( $restype, $reserves, undef ) = CheckReserves( $it->{'itemnumber'} );
+        my $restype = C4::Reserves::GetReserveStatus( $it->{'itemnumber'} );
         $it->{'can_renew'} = $can_renew;
         $it->{'can_confirm'} = !$can_renew && !$restype;
-        $it->{'renew_error'} = $restype;
+        $it->{'renew_error'} = ( $restype eq "Waiting" or $restype eq "Reserved" ) ? 1 : 0;
         $it->{'checkoutdate'} = C4::Dates->new($it->{'issuedate'},'iso')->output('syspref');
         $it->{'issuingbranchname'} = GetBranchName($it->{'branchcode'});
 

diff --git a/members/moremember.pl b/members/moremember.pl
index 4ccba00..fe3b2eb 100755 (executable)
--- a/members/moremember.pl
+++ b/members/moremember.pl
@@ -47,7 +47,6 @@ use C4::Circulation;
 use C4::Koha;
 use C4::Letters;
 use C4::Biblio;
-use C4::Reserves;
 use C4::Branch; # GetBranchName
 use C4::Overdues qw/CheckBorrowerDebarred/;
 use C4::Form::MessagingPreferences;

diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl
index 5b2da84..068122e 100755 (executable)
--- a/opac/opac-detail.pl
+++ b/opac/opac-detail.pl
@@ -554,7 +554,7 @@ for my $itm (@items) {
          $itm->{'lostimageurl'}   = $lostimageinfo->{ 'imageurl' };
          $itm->{'lostimagelabel'} = $lostimageinfo->{ 'label' };
      }
-     my ($reserve_status) = C4::Reserves::CheckReserves($itm->{itemnumber});
+     my $reserve_status = C4::Reserves::GetReserveStatus($itm->{itemnumber});
       if( $reserve_status eq "Waiting"){ $itm->{'waiting'} = 1; }
       if( $reserve_status eq "Reserved"){ $itm->{'onhold'} = 1; }
     

diff --git a/opac/opac-user.pl b/opac/opac-user.pl
index f4273cb..750a9c3 100755 (executable)
--- a/opac/opac-user.pl
+++ b/opac/opac-user.pl
@@ -158,7 +158,7 @@ my $issues = GetPendingIssues($borrowernumber);
 if ($issues){
     foreach my $issue ( sort { $b->{date_due}->datetime() cmp $a->{date_due}->datetime() } @{$issues} ) {
         # check for reserves
-        my ( $restype, $res, undef ) = CheckReserves( $issue->{'itemnumber'} );
+        my $restype = GetReserveStatus( $issue->{'itemnumber'} );
         if ( $restype ) {
             $issue->{'reserved'} = 1;
         }