Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today

On the same way of Koha::Biblio->get_holds,
Koha::Biblio->get_holds_placed_before_today and Koha::Patron->get_holds,
this new subroutin will permit to retrieve the holds placed on a
specific item.
Note that at the moment we do not need a Koha::Item->get_holds method:
we do not want to display future holds placed in the future.

Test plan:
I would suggest to test this patch with patches from bug 17736 and bug 17738,
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.

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

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

Koha/Item.pm

@@ -22,6 +22,7 @@ use Modern::Perl;
 use Carp;
 
 use Koha::Database;
+use Koha::DateUtils qw( dt_from_string );
 
 use C4::Context;
 use Koha::IssuingRules;
@@ -184,6 +185,26 @@ sub article_request_type {
     return $issuing_rule->article_requests || q{}
 }
 
+=head3 holds_placed_before_today
+
+=cut
+
+sub holds_placed_before_today {
+    my ( $self ) = @_;
+    my $attributes = { order_by => 'priority' };
+    my $dtf = Koha::Database->new->schema->storage->datetime_parser;
+    my $params = {
+        itemnumber => $self->itemnumber,
+        suspend => 0,
+        -or => [
+            reservedate => { '<=' => $dtf->format_date(dt_from_string) },
+            waitingdate => { '!=' => undef },
+        ],
+    };
+    my $hold_rs = $self->_result->reserves->search( $params, $attributes );
+    return Koha::Holds->_new_from_dbic($hold_rs);
+}
+
 =head3 type
 
 =cut

catalogue/detail.pl

@@ -43,6 +43,7 @@ use C4::CourseReserves qw(GetItemCourseReservesInfo);
 use C4::Acquisition qw(GetOrdersByBiblionumber);
 use Koha::AuthorisedValues;
 use Koha::Biblios;
+use Koha::Items;
 use Koha::Patrons;
 use Koha::Virtualshelves;
 
@@ -251,24 +252,25 @@ foreach my $item (@items) {
         $itemfields{$_} = 1 if ( $item->{$_} );
     }
 
-    # checking for holds
-    my ($reservedate,$reservedfor,$expectedAt,undef,$wait) = GetReservesFromItemnumber($item->{itemnumber});
-    my $ItemBorrowerReserveInfo = C4::Members::GetMember( borrowernumber => $reservedfor);
-    
     if (C4::Context->preference('HidePatronName')){
-	$item->{'hidepatronname'} = 1;
+        $item->{'hidepatronname'} = 1;
     }
 
-    if ( defined $reservedate ) {
+
+    # checking for holds
+    my $item_object = Koha::Items->find( $item->{itemnumber} );
+    my $holds = $item_object->holds_placed_before_today;
+    if ( my $first_hold = $holds->next ) {
+        my $ItemBorrowerReserveInfo = C4::Members::GetMember( borrowernumber => $first_hold->borrowernumber); # FIXME could be improved
         $item->{backgroundcolor} = 'reserved';
-        $item->{reservedate}     = $reservedate;
+        $item->{reservedate}     = $first_hold->reservedate;
-        $item->{ReservedForBorrowernumber}     = $reservedfor;
+        $item->{ReservedForBorrowernumber}     = $first_hold->borrowernumber;
         $item->{ReservedForSurname}     = $ItemBorrowerReserveInfo->{'surname'};
         $item->{ReservedForFirstname}   = $ItemBorrowerReserveInfo->{'firstname'};
-        $item->{ExpectedAtLibrary}      = $expectedAt;
+        $item->{ExpectedAtLibrary}      = $first_hold->branchcode;
         $item->{Reservedcardnumber}             = $ItemBorrowerReserveInfo->{'cardnumber'};
         # Check waiting status
-        $item->{waitingdate} = $wait;
+        $item->{waitingdate} = $first_hold->waitingdate;
     }
 
 

circ/transferstoreceive.pl

@@ -35,6 +35,7 @@ use Date::Calc qw(
 
 use C4::Koha;
 use C4::Reserves;
+use Koha::Items;
 use Koha::Libraries;
 use Koha::DateUtils;
 use Koha::BiblioFrameworks;
@@ -100,9 +101,10 @@ while ( my $library = $libraries->next ) {
             $getransf{'subtitle'} = GetRecordValue('subtitle', $record, GetFrameworkCode($gettitle->{'biblionumber'}));
 
             # we check if we have a reserv for this transfer
-            my @checkreserv = GetReservesFromItemnumber($num->{'itemnumber'});
+            my $item = Koha::Items->find( $num->{itemnumber} );
-            if ( $checkreserv[0] ) {
+            my $holds = $item->holds_placed_before_today;
-                my $getborrower = C4::Members::GetMember( borrowernumber => $checkreserv[1] );
+            if ( my $first_hold = $holds->next ) {
+                my $getborrower = C4::Members::GetMember( borrowernumber => $first_hold->borrowernumber );
                 $getransf{'borrowernum'}       = $getborrower->{'borrowernumber'};
                 $getransf{'borrowername'}      = $getborrower->{'surname'};
                 $getransf{'borrowerfirstname'} = $getborrower->{'firstname'};

opac/opac-reserve.pl

@@ -34,6 +34,7 @@ use C4::Overdues;
 use C4::Debug;
 use Koha::AuthorisedValues;
 use Koha::DateUtils;
+use Koha::Items;
 use Koha::Libraries;
 use Koha::Patrons;
 use Date::Calc qw/Today Date_to_Days/;
@@ -470,21 +471,21 @@ foreach my $biblioNum (@biblionumbers) {
         }
 
         # checking reserve
-        my ($reservedate,$reservedfor,$expectedAt,undef,$wait) = GetReservesFromItemnumber($itemNum);
+        my $item = Koha::Items->find( $itemNum );
-        my $ItemBorrowerReserveInfo = GetMember( borrowernumber => $reservedfor );
+        my $holds = $item->holds_placed_before_today;
 
         # the item could be reserved for this borrower vi a host record, flag this
-        $reservedfor //= '';
+        my $reservedfor = q||;
 
-        if ( defined $reservedate ) {
+        if ( my $first_hold = $holds->next ) {
+            my $ItemBorrowerReserveInfo = GetMember( borrowernumber => $first_hold->borrowernumber );
             $itemLoopIter->{backgroundcolor} = 'reserved';
-            $itemLoopIter->{reservedate}     = output_pref({ dt => dt_from_string($reservedate), dateonly => 1 });
+            $itemLoopIter->{reservedate}     = output_pref({ dt => dt_from_string($first_hold->reservedate), dateonly => 1 }); # FIXME Should be formatted in the template
-            $itemLoopIter->{ReservedForBorrowernumber} = $reservedfor;
+            $itemLoopIter->{ReservedForBorrowernumber} = $first_hold->borrowernumber;
             $itemLoopIter->{ReservedForSurname}        = $ItemBorrowerReserveInfo->{'surname'};
             $itemLoopIter->{ReservedForFirstname}      = $ItemBorrowerReserveInfo->{'firstname'};
-            $itemLoopIter->{ExpectedAtLibrary}         = $expectedAt;
+            $itemLoopIter->{ExpectedAtLibrary}         = $first_hold->branchcode;
-            #waiting status
+            $itemLoopIter->{waitingdate} = $first_hold->waitingdate;
-            $itemLoopIter->{waitingdate} = $wait;
         }
 
         $itemLoopIter->{notforloan} = $itemInfo->{notforloan};

reserve/request.pl

@@ -44,6 +44,7 @@ use C4::Members;
 use C4::Search;		# enabled_staff_search_views
 use Koha::DateUtils;
 use Koha::Holds;
+use Koha::Items;
 use Koha::Libraries;
 use Koha::Patrons;
 
@@ -388,17 +389,17 @@ foreach my $biblionumber (@biblionumbers) {
             }
 
             # checking reserve
-            my ($reservedate,$reservedfor,$expectedAt,$reserve_id,$wait) = GetReservesFromItemnumber($itemnumber);
+            my $holds = Koha::Items->find( $itemnumber )->holds_placed_before_today;
-            if ( defined $reservedate ) {
+            if ( my $first_hold = $holds->next ) {
-                my $ItemBorrowerReserveInfo = GetMember( borrowernumber => $reservedfor );
+                my $ItemBorrowerReserveInfo = GetMember( borrowernumber => $first_hold->borrowernumber );
 
                 $item->{backgroundcolor} = 'reserved';
-                $item->{reservedate}     = output_pref({ dt => dt_from_string( $reservedate ), dateonly => 1 });
+                $item->{reservedate}     = output_pref({ dt => dt_from_string( $first_hold->reservedate ), dateonly => 1 }); # FIXME Should be formatted in the template
-                $item->{ReservedForBorrowernumber}     = $reservedfor;
+                $item->{ReservedForBorrowernumber}     = $first_hold->borrowernumber;
                 $item->{ReservedForSurname}     = $ItemBorrowerReserveInfo->{'surname'};
                 $item->{ReservedForFirstname}     = $ItemBorrowerReserveInfo->{'firstname'};
-                $item->{ExpectedAtLibrary}     = $expectedAt;
+                $item->{ExpectedAtLibrary}     = $first_hold->branchcode;
-                $item->{waitingdate} = $wait;
+                $item->{waitingdate} = $first_hold->waitingdate;
             }
 
             # Management of the notforloan document

t/db_dependent/Holds.t

@@ -94,11 +94,17 @@ is( $holds->next->priority, 3, "Reserve 3 has a priority of 3" );
 is( $holds->next->priority, 4, "Reserve 4 has a priority of 4" );
 is( $holds->next->priority, 5, "Reserve 5 has a priority of 5" );
 
-my ( $reservedate, $borrowernumber, $branch_1code, $reserve_id ) = GetReservesFromItemnumber($itemnumber);
+my $item = Koha::Items->find( $itemnumber );
-is( $reservedate, output_pref({ dt => dt_from_string, dateformat => 'iso', dateonly => 1 }), "GetReservesFromItemnumber should return a valid reserve date");
+$holds = $item->holds_placed_before_today;
-is( $borrowernumber, $borrowernumbers[0], "GetReservesFromItemnumber should return a valid borrowernumber");
+my $first_hold = $holds->next;
-is( $branch_1code, $branch_1, "GetReservesFromItemnumber should return a valid branchcode");
+my $reservedate = $first_hold->reservedate;
-ok($reserve_id, "Test GetReservesFromItemnumber()");
+my $borrowernumber = $first_hold->borrowernumber;
+my $branch_1code = $first_hold->branchcode;
+my $reserve_id = $first_hold->reserve_id;
+is( $reservedate, output_pref({ dt => dt_from_string, dateformat => 'iso', dateonly => 1 }), "holds_placed_today should return a valid reserve date");
+is( $borrowernumber, $borrowernumbers[0], "holds_placed_today should return a valid borrowernumber");
+is( $branch_1code, $branch_1, "holds_placed_today should return a valid branchcode");
+ok($reserve_id, "Test holds_placed_today()");
 
 my $hold = Koha::Holds->find( $reserve_id );
 ok( $hold, "Koha::Holds found the hold" );
@@ -126,8 +132,12 @@ CancelReserve({ 'reserve_id' => $reserve_id });
 $holds = $biblio->holds;
 is( $holds->count, $borrowers_count - 1, "Test CancelReserve()" );
 
+$holds = $item->holds_placed_before_today;
+$first_hold = $holds->next;
+$borrowernumber = $first_hold->borrowernumber;
+$branch_1code = $first_hold->branchcode;
+$reserve_id = $first_hold->reserve_id;
 
-( $reservedate, $borrowernumber, $branch_1code, $reserve_id ) = GetReservesFromItemnumber($itemnumber);
 ModReserve({
     reserve_id    => $reserve_id,
     rank          => '4',

t/db_dependent/Reserves.t

@@ -404,8 +404,8 @@ is(
 my $letter = ReserveSlip($branch_1, $requesters{$branch_1}, $bibnum);
 ok(defined($letter), 'can successfully generate hold slip (bug 10949)');
 
-# Tests for bug 9788: Does GetReservesFromItemnumber return a future wait?
+# Tests for bug 9788: Does Koha::Item->holds_placed_before_today return a future wait?
-# 9788a: GetReservesFromItemnumber does not return future next available hold
+# 9788a: holds_placed_before_today does not return future next available hold
 $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum));
 t::lib::Mocks::mock_preference('ConfirmFutureHolds', 2);
 t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1);
@@ -415,19 +415,22 @@ $resdate=output_pref($resdate);
 AddReserve($branch_1,  $requesters{$branch_1}, $bibnum,
            $bibitems,  1, $resdate, $expdate, $notes,
            $title,      $checkitem, $found);
-my @results= GetReservesFromItemnumber($itemnumber);
+my $item = Koha::Items->find( $itemnumber );
-is(defined $results[3]?1:0, 0, 'GetReservesFromItemnumber does not return a future next available hold');
+$holds = $item->holds_placed_before_today;
-# 9788b: GetReservesFromItemnumber does not return future item level hold
+my $dtf = Koha::Database->new->schema->storage->datetime_parser;
+my $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } );
+is( $future_holds->count, 0, 'holds_placed_before_today does not return a future next available hold');
+# 9788b: holds_placed_before_today does not return future item level hold
 $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum));
 AddReserve($branch_1,  $requesters{$branch_1}, $bibnum,
            $bibitems,  1, $resdate, $expdate, $notes,
            $title,      $itemnumber, $found); #item level hold
-@results= GetReservesFromItemnumber($itemnumber);
+$future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } );
-is(defined $results[3]?1:0, 0, 'GetReservesFromItemnumber does not return a future item level hold');
+is( $future_holds->count, 0, 'holds_placed_before_today does not return a future item level hold' );
-# 9788c: GetReservesFromItemnumber returns future wait (confirmed future hold)
+# 9788c: holds_placed_before_today returns future wait (confirmed future hold)
 ModReserveAffect( $itemnumber,  $requesters{$branch_1} , 0); #confirm hold
-@results= GetReservesFromItemnumber($itemnumber);
+$future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } );
-is(defined $results[3]?1:0, 1, 'GetReservesFromItemnumber returns a future wait (confirmed future hold)');
+is( $future_holds->count, 1, 'holds_placed_before_today returns a future wait (confirmed future hold)' );
 # End of tests for bug 9788
 
 $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum));
@@ -547,7 +550,7 @@ is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Res
 ####### EO Bug 13113 <<<
        ####
 
-my $item = GetItem($itemnumber);
+$item = GetItem($itemnumber);
 
 ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $borrower), "Reserving a book on item level" );