Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds

This patch replace the different calls to GetReservesFromBorrowernumber
with a calls to Koha::Patron->get_holds.
In some places we need to get a restricted set of holds, that's why we
process a search on this holds returned by ->get_holds (on the found
status for instance).

The changes are quite trivial and reading the diff should be enough to
catch bugs.

Test plan:
I would suggest to test this patch with patches from bug 17736 and bug 17737,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.

Tested both patches together, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

C4/ILSDI/Services.pm

@@ -25,7 +25,7 @@ use C4::Items;
 use C4::Circulation;
 use C4::Accounts;
 use C4::Biblio;
-use C4::Reserves qw(AddReserve GetReservesFromBorrowernumber CanBookBeReserved CanItemBeReserved IsAvailableForItemLevelRequest);
+use C4::Reserves qw(AddReserve CanBookBeReserved CanItemBeReserved IsAvailableForItemLevelRequest);
 use C4::Context;
 use C4::AuthoritiesMarc;
 use XML::Simple;
@@ -37,6 +37,7 @@ use C4::Members::Attributes qw(GetBorrowerAttributes);
 
 use Koha::Biblios;
 use Koha::Libraries;
+use Koha::Patrons;
 
 =head1 NAME
 
@@ -375,6 +376,7 @@ sub GetPatronInfo {
     my $borrowernumber = $cgi->param('patron_id');
     my $borrower = GetMember( borrowernumber => $borrowernumber );
     return { code => 'PatronNotFound' } unless $$borrower{borrowernumber};
+    my $patron = Koha::Patrons->find( $borrowernumber );
 
     # Cleaning the borrower hashref
     my $flags = C4::Members::patronflags( $borrower );
@@ -414,23 +416,25 @@ sub GetPatronInfo {
     if ( $cgi->param('show_holds') && $cgi->param('show_holds') eq "1" ) {
 
         # Get borrower's reserves
-        my @reserves = GetReservesFromBorrowernumber( $borrowernumber, undef );
-        foreach my $reserve (@reserves) {
+        my $holds = $patron->holds;
+        while ( my $hold = $holds->next ) {
 
+            my $unblessed_hold = $hold->unblessed;
             # Get additional informations
-            my $item = GetBiblioFromItemNumber( $reserve->{'itemnumber'}, undef );
-            my $library = Koha::Libraries->find( $reserve->{branchcode} );
+            my $item = GetBiblioFromItemNumber( $hold->itemnumber, undef );
+            my $library = Koha::Libraries->find( $hold->branchcode ); # Should $hold->get_library
             my $branchname = $library ? $library->branchname : '';
 
             # Remove unwanted fields
             delete $item->{'more_subfields_xml'};
 
             # Add additional fields
-            $reserve->{'item'}       = $item;
-            $reserve->{'branchname'} = $branchname;
-            $reserve->{'title'}      = GetBiblio( $reserve->{'biblionumber'} )->{'title'};
+            $unblessed_hold->{item}       = $item;
+            $unblessed_hold->{branchname} = $branchname;
+            $unblessed_hold->{title}      = GetBiblio( $hold->biblionumber )->{'title'}; # Should be $hold->get_biblio
+
+            push @{ $borrower->{holds}{hold} }, $unblessed_hold;
         }
-        $borrower->{'holds'}->{'hold'} = \@reserves;
     }
 
     # Issues management
@@ -500,6 +504,8 @@ sub GetServices {
     my $borrower = GetMember( borrowernumber => $borrowernumber );
     return { code => 'PatronNotFound' } unless $$borrower{borrowernumber};
 
+    my $patron = Koha::Patrons->find( $borrowernumber );
+
     # Get the item, or return an error code if not found
     my $itemnumber = $cgi->param('item_id');
     my $item = GetItem( $itemnumber );
@@ -519,10 +525,10 @@ sub GetServices {
     }
 
     # Reserve cancellation management
-    my @reserves = GetReservesFromBorrowernumber( $borrowernumber, undef );
+    my $holds = $patron->holds;
     my @reserveditems;
-    foreach my $reserve (@reserves) {
-        push @reserveditems, $reserve->{'itemnumber'};
+    while ( my $hold = $holds->next ) { # FIXME This could be improved
+        push @reserveditems, $hold->itemnumber;
     }
     if ( grep { $itemnumber eq $_ } @reserveditems ) {
         push @availablefor, 'hold cancellation';

C4/Members.pm

@@ -267,12 +267,14 @@ sub patronflags {
         }
         $flags{'ODUES'} = \%flaginfo;
     }
-    my @itemswaiting = C4::Reserves::GetReservesFromBorrowernumber( $patroninformation->{'borrowernumber'},'W' );
-    my $nowaiting = scalar @itemswaiting;
+
+    my $patron = Koha::Patrons->find( $patroninformation->{borrowernumber} );
+    my $waiting_holds = $patron->holds->search({ found => 'W' });
+    my $nowaiting = $waiting_holds->count;
     if ( $nowaiting > 0 ) {
         my %flaginfo;
         $flaginfo{'message'}  = "Reserved items available";
-        $flaginfo{'itemlist'} = \@itemswaiting;
+        $flaginfo{'itemlist'} = $waiting_holds->unblessed;
         $flags{'WAITING'}     = \%flaginfo;
     }
     return ( \%flags );

C4/SIP/ILS/Patron.pm

@@ -24,6 +24,7 @@ use C4::Items qw( GetBarcodeFromItemnumber GetItemnumbersForBiblio);
 use C4::Auth qw(checkpw);
 
 use Koha::Libraries;
+use Koha::Patrons;
 
 our $kp;    # koha patron
 
@@ -431,20 +432,25 @@ sub _get_address {
 
 sub _get_outstanding_holds {
     my $borrowernumber = shift;
-    my @hold_array = grep { !defined $_->{found} || $_->{found} ne 'W'} GetReservesFromBorrowernumber($borrowernumber);
-    foreach my $h (@hold_array) {
+
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    my $holds = $patron->holds->search( { -or => [ { found => undef }, { found => { '!=' => 'W' } } ] } );
+    my @holds;
+    while ( my $hold = $holds->next ) {
         my $item;
-        if ($h->{itemnumber}) {
-            $item = $h->{itemnumber};
+        if ($hold->itemnumber) {
+            $item = $hold->itemnumber;
         }
         else {
             # We need to return a barcode for the biblio so the client
             # can request the biblio info
-            $item = ( GetItemnumbersForBiblio($h->{biblionumber}) )->[0];
+            $item = ( GetItemnumbersForBiblio($hold->biblionumber) )->[0];
         }
-        $h->{barcode} = GetBarcodeFromItemnumber($item);
+        my $unblessed_hold = $hold->unblessed;
+        $unblessed_hold->{barcode} = GetBarcodeFromItemnumber($item);
+        push @holds, $unblessed_hold;
     }
-    return \@hold_array;
+    return \@holds;
 }
 
 1;

Koha/Patron/Discharge.pm

@@ -7,7 +7,7 @@ use Carp;
 
 use C4::Templates qw ( gettemplate );
 use C4::Members qw( GetPendingIssues );
-use C4::Reserves qw( GetReservesFromBorrowernumber CancelReserve );
+use C4::Reserves qw( CancelReserve );
 
 use Koha::Database;
 use Koha::DateUtils qw( dt_from_string output_pref );
@@ -79,9 +79,10 @@ sub discharge {
     return unless $borrowernumber and can_be_discharged( { borrowernumber => $borrowernumber } );
 
     # Cancel reserves
-    my @reserves = GetReservesFromBorrowernumber($borrowernumber);
-    for my $reserve (@reserves) {
-        CancelReserve( { reserve_id => $reserve->{reserve_id} } );
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    my $holds = $patron->holds;
+    while ( my $hold = $holds->next ) {
+        CancelReserve( { reserve_id => $hold->reserve_id } );
     }
 
     # Debar the member

circ/returns.pl

@@ -50,8 +50,9 @@ use C4::RotatingCollections;
 use Koha::AuthorisedValues;
 use Koha::DateUtils;
 use Koha::Calendar;
-use Koha::Checkouts;
 use Koha::BiblioFrameworks;
+use Koha::Checkouts;
+use Koha::Patrons;
 
 my $query = new CGI;
 
@@ -339,13 +340,8 @@ if ($barcode) {
 
         if (C4::Context->preference("WaitingNotifyAtCheckin") ) {
             #Check for waiting holds
-            my @reserves = GetReservesFromBorrowernumber($borrower->{'borrowernumber'});
-            my $waiting_holds = 0;
-            foreach my $num_res (@reserves) {
-                if ( $num_res->{'found'} eq 'W' && $num_res->{'branchcode'} eq $userenv_branch) {
-                    $waiting_holds++;
-                }
-            }
+            my $patron = Koha::Patrons->find( $borrower->{borrowernumber} );
+            my $waiting_holds = $patron->holds->search({ found => 'W', branchcode => $userenv_branch })->count;
             if ($waiting_holds > 0) {
                 $template->param(
                     waiting_holds       => $waiting_holds,

members/discharge.pl

@@ -38,6 +38,7 @@ use C4::Reserves;
 use C4::Letters;
 use Koha::Patron::Discharge;
 use Koha::Patron::Images;
+use Koha::Patrons;
 
 use Koha::DateUtils;
 
@@ -70,8 +71,9 @@ if ( $input->param('borrowernumber') ) {
     });
 
     # Getting reserves
-    my @reserves    = GetReservesFromBorrowernumber($borrowernumber);
-    my $has_reserves = scalar(@reserves);
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    my $holds = $patron->holds;
+    my $has_reserves = $holds->count;
 
     # Generating discharge if needed
     if ( $input->param('discharge') and $can_be_discharged ) {

opac/opac-reserve.pl

@@ -199,7 +199,8 @@ foreach my $biblioNumber (@biblionumbers) {
 if ( $query->param('place_reserve') ) {
     my $reserve_cnt = 0;
     if ($maxreserves) {
-        $reserve_cnt = GetReservesFromBorrowernumber( $borrowernumber );
+        my $patron = Koha::Patrons->find( $borrowernumber );
+        $reserve_cnt = $patron->holds->count;
     }
 
     # List is composed of alternating biblio/item/branch
@@ -350,7 +351,8 @@ if ( $borr->{lost} && ($borr->{lost} == 1) ) {
     );
 }
 
-if ( Koha::Patrons->find( $borrowernumber )->is_debarred ) {
+my $patron = Koha::Patrons->find( $borrowernumber );
+if ( $patron->is_debarred ) {
     $noreserves = 1;
     $template->param(
         message          => 1,
@@ -360,13 +362,13 @@ if ( Koha::Patrons->find( $borrowernumber )->is_debarred ) {
     );
 }
 
-my @reserves = GetReservesFromBorrowernumber( $borrowernumber );
-my $reserves_count = scalar(@reserves);
-$template->param( RESERVES => \@reserves );
+my $holds = $patron->holds;
+my $reserves_count = $holds->count;
+$template->param( RESERVES => $holds->unblessed );
 if ( $maxreserves && ( $reserves_count >= $maxreserves ) ) {
     $template->param( message => 1 );
     $noreserves = 1;
-    $template->param( too_many_reserves => scalar(@reserves));
+    $template->param( too_many_reserves => $holds->count );
 }
 
 unless ( $noreserves ) {

t/db_dependent/Holds.t

@@ -17,6 +17,7 @@ use Koha::Database;
 use Koha::DateUtils qw( dt_from_string output_pref );
 use Koha::Biblios;
 use Koha::Holds;
+use Koha::Patrons;
 
 BEGIN {
     use FindBin;
@@ -121,8 +122,9 @@ my $hold_found = $hold->found();
 $hold->set({ found => 'W'})->store();
 is( Koha::Holds->waiting()->count(), 1, "Koha::Holds->waiting returns waiting holds" );
 
-my ( $reserve ) = GetReservesFromBorrowernumber($borrowernumbers[0]);
-ok( $reserve->{'borrowernumber'} eq $borrowernumbers[0], "Test GetReservesFromBorrowernumber()");
+my $patron = Koha::Patrons->find( $borrowernumbers[0] );
+$holds = $patron->holds;
+is( $holds->next->borrowernumber, $borrowernumbers[0], "Test Koha::Patron->holds");
 
 
 ok( GetReserveCount( $borrowernumbers[0] ), "Test GetReserveCount()" );
@@ -146,7 +148,7 @@ ModReserve({
     suspend_until => output_pref( { dt => dt_from_string( "2013-01-01", "iso" ), dateonly => 1 } ),
 });
 
-$reserve = GetReserve( $reserve_id );
+my $reserve = GetReserve( $reserve_id );
 ok( $reserve->{'priority'} eq '4', "Test GetReserve(), priority changed correctly" );
 ok( $reserve->{'suspend'}, "Test GetReserve(), suspend hold" );
 is( $reserve->{'suspend_until'}, '2013-01-01 00:00:00', "Test GetReserve(), suspend until date" );
@@ -196,17 +198,18 @@ AddReserve(
     my $checkitem,
     my $found,
 );
-( $reserve ) = GetReservesFromBorrowernumber($borrowernumber);
+$patron = Koha::Patrons->find( $borrowernumber );
+$holds = $patron->holds;
 my $reserveid = C4::Reserves::GetReserveId(
     {
         biblionumber => $biblionumber,
         borrowernumber => $borrowernumber
     }
 );
-is( $reserveid, $reserve->{reserve_id}, "Test GetReserveId" );
+is( $reserveid, $holds->next->reserve_id, "Test GetReserveId" );
 ModReserveMinusPriority( $itemnumber, $reserve->{'reserve_id'} );
-( $reserve ) = GetReservesFromBorrowernumber($borrowernumber);
-ok( $reserve->{'itemnumber'} eq $itemnumber, "Test ModReserveMinusPriority()" );
+$holds = $patron->holds;
+is( $holds->next->itemnumber, $itemnumber, "Test ModReserveMinusPriority()" );
 
 
 my $reserve2 = GetReserveInfo( $reserve->{'reserve_id'} );