From 3a818c63ed5f663c354b4c45547c01bd5a86b0ed Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 18 Mar 2013 14:37:18 +0000 Subject: [PATCH] Bug 9823: Refactor return from GetReservesFromBiblionumber The return from GetReservesFromBiblionumber contains an unnecessary extra variable. In scalar context an array returns its element count. Maintaing a separate count can lead to unforeseen bugs and imposes ugly constructions on the subroutine's users. Remove the useless count variable from the return This patch also changes the parameters: now the routine takes a hashref. Signed-off-by: Marcel de Rooy Placed biblio holds, future holds and item holds. Works as expected. Tested Holds.t and Reserves.t. Pass. Tested /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=999 with two holds on one item. Fine. C4/SIP/ILS/Item.pm: Looked for "whatever" and "arrayref" and could not find them anymore. Looks good. Handled a few unneeded calls in QA follow-up. Left only one point to-do for serials/routing-preview.pl. See Bugzilla. Signed-off-by: Kyle M Hall --- C4/Biblio.pm | 2 +- C4/ILSDI/Services.pm | 4 ++-- C4/Reserves.pm | 19 ++++++++++++------- C4/SIP/ILS/Item.pm | 4 ++-- acqui/parcel.pl | 4 ++-- catalogue/ISBDdetail.pl | 3 ++- catalogue/MARCdetail.pl | 3 ++- catalogue/detail.pl | 3 ++- catalogue/imageviewer.pl | 3 ++- catalogue/labeledMARCdetail.pl | 3 ++- catalogue/moredetail.pl | 3 ++- opac/opac-detail.pl | 4 ++-- opac/opac-reserve.pl | 4 ++-- reserve/request.pl | 5 +++-- serials/routing-preview.pl | 5 +++-- t/db_dependent/Holds.t | 10 +++++----- 16 files changed, 46 insertions(+), 33 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index dc40d68..330951d 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -443,7 +443,7 @@ sub DelBiblio { # We delete any existing holds require C4::Reserves; - my ($count, $reserves) = C4::Reserves::GetReservesFromBiblionumber($biblionumber); + my $reserves = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber }); foreach my $res ( @$reserves ) { C4::Reserves::CancelReserve({ reserve_id => $res->{'reserve_id'} }); } diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 9a86d2e..f5e6747 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -208,7 +208,7 @@ sub GetRecords { # Get most of the needed data my $biblioitemnumber = $biblioitem->{'biblioitemnumber'}; - my @reserves = GetReservesFromBiblionumber( $biblionumber, undef, undef ); + my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber }); my $issues = GetBiblioIssues($biblionumber); my $items = GetItemsByBiblioitemnumber($biblioitemnumber); @@ -225,7 +225,7 @@ sub GetRecords { # Hashref building... $biblioitem->{'items'}->{'item'} = $items; - $biblioitem->{'reserves'}->{'reserve'} = $reserves[1]; + $biblioitem->{'reserves'}->{'reserve'} = $reserves; $biblioitem->{'issues'}->{'issue'} = $issues; push @records, $biblioitem; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index f066404..7f3c5fb 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -268,17 +268,22 @@ sub GetReserve { =head2 GetReservesFromBiblionumber - ($count, $title_reserves) = GetReservesFromBiblionumber($biblionumber); + my $reserves = GetReservesFromBiblionumber({ + biblionumber => $biblionumber, + itemnumber => $itemnumber, + all_dates => 1|0 + }); -This function gets the list of reservations for one C<$biblionumber>, returning a count -of the reserves and an arrayref pointing to the reserves for C<$biblionumber>. +This function gets the list of reservations for one C<$biblionumber>, +returning an arrayref pointing to the reserves for C<$biblionumber>. =cut sub GetReservesFromBiblionumber { - my ($biblionumber) = shift or return (0, []); - my ($all_dates) = shift; - my ($itemnumber) = shift; + my ( $params ) = @_; + my $biblionumber = $params->{biblionumber} or return []; + my $itemnumber = $params->{itemnumber}; + my $all_dates = $params->{all_dates} // 0; my $dbh = C4::Context->dbh; # Find the desired items in the reserves @@ -353,7 +358,7 @@ sub GetReservesFromBiblionumber { } push @results, $data; } - return ( $#results + 1, \@results ); + return \@results; } =head2 GetReservesFromItemnumber diff --git a/C4/SIP/ILS/Item.pm b/C4/SIP/ILS/Item.pm index ba85cd7..049dd17 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -95,8 +95,8 @@ sub new { } my $borrower = GetMember(borrowernumber=>$issue->{'borrowernumber'}); $item->{patron} = $borrower->{'cardnumber'}; - my ($whatever, $arrayref) = GetReservesFromBiblionumber($item->{biblionumber}); - $item->{hold_queue} = [ sort priority_sort @$arrayref ]; + my $reserves = GetReservesFromBiblionumber({ biblionumber => $item->{biblionumber} }); + $item->{hold_queue} = [ sort priority_sort @$reserves ]; $item->{hold_shelf} = [( grep { defined $_->{found} and $_->{found} eq 'W' } @{$item->{hold_queue}} )]; $item->{pending_queue} = [( grep {(! defined $_->{found}) or $_->{found} ne 'W' } @{$item->{hold_queue}} )]; $self = $item; diff --git a/acqui/parcel.pl b/acqui/parcel.pl index 94cf987..b45155c 100755 --- a/acqui/parcel.pl +++ b/acqui/parcel.pl @@ -175,8 +175,8 @@ for my $order ( @orders ) { $line{holds} = 0; my @itemnumbers = GetItemnumbersFromOrder( $order->{ordernumber} ); for my $itemnumber ( @itemnumbers ) { - my ( $count ) = &GetReservesFromBiblionumber($line{biblionumber}, undef, $itemnumber); - $line{holds} += $count; + my $holds = GetReservesFromBiblionumber({ biblionumber => $line{biblionumber}, itemnumber => $itemnumber }); + $line{holds} += scalar( @$holds ); } $line{budget} = GetBudgetByOrderNumber( $line{ordernumber} ); $totalprice += $order->{unitprice}; diff --git a/catalogue/ISBDdetail.pl b/catalogue/ISBDdetail.pl index 28b5c2d..dd1c5c8 100755 --- a/catalogue/ISBDdetail.pl +++ b/catalogue/ISBDdetail.pl @@ -113,7 +113,8 @@ $template->param ( ); -my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1); +my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); +my $holdcount = scalar( @$holds ); $template->param( holdcount => $holdcount, holds => $holds ); output_html_with_http_headers $query, $cookie, $template->output; diff --git a/catalogue/MARCdetail.pl b/catalogue/MARCdetail.pl index 835568a..73e44be 100755 --- a/catalogue/MARCdetail.pl +++ b/catalogue/MARCdetail.pl @@ -336,7 +336,8 @@ $template->param ( searchid => $query->param('searchid'), ); -my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1); +my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); +my $holdcount = scalar( @$holds ); $template->param( holdcount => $holdcount, holds => $holds ); output_html_with_http_headers $query, $cookie, $template->output; diff --git a/catalogue/detail.pl b/catalogue/detail.pl index 9cbf7f2..b5135a9 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -414,7 +414,8 @@ if (C4::Context->preference('TagsEnabled') and $tag_quantity = C4::Context->pref 'sort'=>'-weight', limit=>$tag_quantity})); } -my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1); +my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); +my $holdcount = scalar ( @$holds ); $template->param( holdcount => $holdcount, holds => $holds ); my $StaffDetailItemSelection = C4::Context->preference('StaffDetailItemSelection'); if ($StaffDetailItemSelection) { diff --git a/catalogue/imageviewer.pl b/catalogue/imageviewer.pl index c96e5af..7f994f6 100755 --- a/catalogue/imageviewer.pl +++ b/catalogue/imageviewer.pl @@ -78,7 +78,8 @@ $template->{VARS}->{'norequests'} = $norequests; $template->param(C4::Search::enabled_staff_search_views); $template->{VARS}->{'biblio'} = $biblio; -my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1); +my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); +my $holdcount = scalar( @$holds ); $template->param( holdcount => $holdcount, holds => $holds ); output_html_with_http_headers $query, $cookie, $template->output; diff --git a/catalogue/labeledMARCdetail.pl b/catalogue/labeledMARCdetail.pl index e82e3b9..20a73d0 100755 --- a/catalogue/labeledMARCdetail.pl +++ b/catalogue/labeledMARCdetail.pl @@ -137,7 +137,8 @@ $template->param ( searchid => $query->param('searchid'), ); -my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1); +my $holds= C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); +my $holdcount = scalar( @$holds ); $template->param( holdcount => $holdcount, holds => $holds ); output_html_with_http_headers $query, $cookie, $template->output; diff --git a/catalogue/moredetail.pl b/catalogue/moredetail.pl index 91877cc..8b07f15 100755 --- a/catalogue/moredetail.pl +++ b/catalogue/moredetail.pl @@ -217,7 +217,8 @@ $template->param(ONLY_ONE => 1) if ( $itemnumber && $showncount != @items ); $template->{'VARS'}->{'searchid'} = $query->param('searchid'); -my ( $holdcount, $holds ) = GetReservesFromBiblionumber($biblionumber,1); +my $holds = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); +my $holdcount = scalar( @$holds ); $template->param( holdcount => $holdcount, holds => $holds ); output_html_with_http_headers $query, $cookie, $template->output; diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index 77de12f..e083075 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -539,8 +539,8 @@ for ( C4::Context->preference("OPACShowHoldQueueDetails") ) { } my $has_hold; if ( $show_holds_count || $show_priority) { - my ($reserve_count,$reserves) = GetReservesFromBiblionumber($biblionumber); - $template->param( holds_count => $reserve_count ) if $show_holds_count; + my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber }); + $template->param( holds_count => scalar( @$reserves ) ) if $show_holds_count; foreach (@$reserves) { $item_reserves{ $_->{itemnumber} }++ if $_->{itemnumber}; if ($show_priority && $_->{borrowernumber} == $borrowernumber) { diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index df55c19..6d5d46f 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -147,8 +147,8 @@ foreach my $biblioNumber (@biblionumbers) { } # Compute the priority rank. - my ( $rank, $reserves ) = - GetReservesFromBiblionumber( $biblioNumber, 1 ); + my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblioNumber, all_dates => 1 }); + my $rank = scalar( @$reserves ); $biblioData->{reservecount} = 1; # new reserve foreach my $res (@{$reserves}) { my $found = $res->{found}; diff --git a/reserve/request.pl b/reserve/request.pl index e915dbf..1b303c7 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -196,7 +196,8 @@ foreach my $biblionumber (@biblionumbers) { } # get existing reserves ..... - my ( $count, $reserves ) = GetReservesFromBiblionumber($biblionumber,1); + my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); + my $count = scalar( @$reserves ); my $totalcount = $count; my $holds_count = 0; my $alreadyreserved = 0; @@ -446,7 +447,7 @@ foreach my $biblionumber (@biblionumbers) { # existingreserves building my @reserveloop; - ( $count, $reserves ) = GetReservesFromBiblionumber($biblionumber,1); + $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); foreach my $res ( sort { my $a_found = $a->{found} || ''; my $b_found = $a->{found} || ''; diff --git a/serials/routing-preview.pl b/serials/routing-preview.pl index 154d477..1e0bd09 100755 --- a/serials/routing-preview.pl +++ b/serials/routing-preview.pl @@ -71,8 +71,9 @@ if($ok){ if (C4::Context->preference('RoutingListAddReserves')){ # get existing reserves ..... - my ($count,$reserves) = GetReservesFromBiblionumber($biblio); - my $totalcount = $count; + my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblio }); + my $count = scalar( @$reserves ); + my $totalcount = $count; foreach my $res (@$reserves) { if ($res->{'found'} eq 'W') { $count--; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 2c28c07..76373e4 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -71,8 +71,8 @@ foreach my $borrowernumber ( @borrowernumbers ) { } -my ($count, $reserves) = GetReservesFromBiblionumber($biblionumber); -is( $count, $borrowers_count, "Test GetReserves()" ); +my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber }); +is( scalar(@$reserves), $borrowers_count, "Test GetReserves()" ); my ( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); @@ -87,8 +87,8 @@ ok( GetReserveCount( $borrowernumbers[0] ), "Test GetReserveCount()" ); CancelReserve({ 'reserve_id' => $reserve_id }); -($count, $reserves) = GetReservesFromBiblionumber($biblionumber); -ok( $count == $borrowers_count - 1, "Test CancelReserve()" ); +$reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber }); +is( scalar(@$reserves), $borrowers_count - 1, "Test CancelReserve()" ); ( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); @@ -148,7 +148,7 @@ my $reserve2 = GetReserveInfo( $reserve->{'reserve_id'} ); ok( $reserve->{'reserve_id'} eq $reserve2->{'reserve_id'}, "Test GetReserveInfo()" ); -($count, $reserves) = GetReservesFromBiblionumber($biblionumber,1); +$reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); $reserve = $reserves->[1]; AlterPriority( 'top', $reserve->{'reserve_id'} ); $reserve = GetReserve( $reserve->{'reserve_id'} ); -- 1.7.2.5