Bug 9823: Refactor return from GetReservesFromBiblionumber

The return from GetReservesFromBiblionumber contains an unnecessary
extra variable. In scalar context an array returns its element count.
Maintaining 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 <m.de.rooy@rijksmuseum.nl>
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 <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>

diff --git a/C4/Biblio.pm b/C4/Biblio.pm
index dc40d68..330951d 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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'} );