From 97bb461cfcc30d95339eb7f7aca44ba6b08ef3f6 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 8 Jan 2013 15:33:58 +0100 Subject: [PATCH] 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. --- C4/Biblio.pm | 11 +---------- C4/Circulation.pm | 7 +++---- C4/Reserves.pm | 33 ++++++++++++++++++++++++--------- C4/Search.pm | 11 +++++------ C4/XSLT.pm | 2 +- circ/circulation.pl | 4 ++-- members/moremember.pl | 1 - opac/opac-detail.pl | 2 +- opac/opac-user.pl | 2 +- 9 files changed, 38 insertions(+), 35 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 7d28ccd..a63d7a3 100644 --- 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 71fe20f..53f24f4 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2479,12 +2479,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 --- 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 50f7d2b..76cb615 100644 --- 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; @@ -1854,8 +1854,7 @@ sub searchResults { my ($transfertfrom, $transfertto); # is item on the reserve shelf? - my $reservestatus = ''; - my $reserveitem; + my $reservestatus = ''; unless ($item->{wthdrawn} || $item->{itemlost} @@ -1876,7 +1875,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 @@ -1884,14 +1883,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 19ec162..0770bda 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -240,7 +240,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 91fd1a2..8e96c4f 100755 --- 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 3928286..d740566 100755 --- 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 22c922b..3ec165e 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -559,7 +559,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 1c942d5..bd70ccb 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -156,7 +156,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; } -- 1.7.10.4