From 8730d9832e27ac8cde87949e4e23c78546f789db Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 16 Aug 2018 14:41:37 -0300 Subject: [PATCH] Bug 14711: Change prototype for AddReserve - pass a hashref The number of parameters of AddReserve makes it hard to read and maintain. This patch replace it with a hashref, which will make the calls more readable. Moreover the bibitems has been removed as it was not used by the subroutine. Test plan: - Make sure the tests pass - Read the diff and search for typos - Place a hold on few items Note for QA: reservation_date and expiration_date do not match the DB column's names, should we? --- C4/ILSDI/Services.pm | 26 ++- C4/Reserves.pm | 33 ++- C4/SIP/ILS/Transaction/Hold.pm | 8 +- Koha/REST/V1/Hold.pm | 15 +- opac/opac-reserve.pl | 17 +- reserve/placerequest.pl | 34 ++- serials/routing-preview.pl | 11 +- t/db_dependent/Circulation.t | 78 +++++-- t/db_dependent/Circulation/issue.t | 12 +- t/db_dependent/Holds.t | 90 +++++--- t/db_dependent/Holds/HoldFulfillmentPolicy.t | 81 ++++++- t/db_dependent/Holds/HoldItemtypeLimit.t | 30 ++- t/db_dependent/Holds/LocalHoldsPriority.t | 18 +- t/db_dependent/Holds/RevertWaitingStatus.t | 17 +- t/db_dependent/HoldsQueue.t | 161 ++++++++++++-- t/db_dependent/Koha/Biblios.t | 17 +- t/db_dependent/Koha/Holds.t | 85 ++++---- t/db_dependent/Koha/Patrons.t | 19 +- t/db_dependent/Letters/TemplateToolkit.t | 21 +- t/db_dependent/Reserves.t | 308 +++++++++++++++++++-------- t/db_dependent/Reserves/GetReserveFee.t | 18 +- t/db_dependent/Reserves/MultiplePerRecord.t | 19 +- t/db_dependent/UsageStats.t | 10 +- t/db_dependent/api/v1/holds.t | 22 +- 24 files changed, 870 insertions(+), 280 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 5534d3ccbc..fb8edfd5bb 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -680,7 +680,15 @@ sub HoldTitle { # $constraint, $bibitems, $priority, $resdate, $expdate, $notes, # $title, $checkitem, $found my $priority= C4::Reserves::CalculatePriority( $biblionumber ); - AddReserve( $branch, $borrowernumber, $biblionumber, undef, $priority, undef, undef, undef, $title, undef, undef ); + AddReserve( + { + branch => $branch, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + title => $title, + } + ); # Hashref building my $out; @@ -753,11 +761,17 @@ sub HoldItem { } # Add the reserve - # $branch, $borrowernumber, $biblionumber, - # $constraint, $bibitems, $priority, $resdate, $expdate, $notes, - # $title, $checkitem, $found - my $priority= C4::Reserves::CalculatePriority( $biblionumber ); - AddReserve( $branch, $borrowernumber, $biblionumber, undef, $priority, undef, undef, undef, $title, $itemnumber, undef ); + my $priority = C4::Reserves::CalculatePriority($biblionumber); + AddReserve( + { + branch => $branch, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + title => $title, + checkitem => $itemnumber, + } + ); # Hashref building my $out; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index ab901c8953..35fcf2180d 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -144,7 +144,21 @@ BEGIN { =head2 AddReserve - AddReserve($branch,$borrowernumber,$biblionumber,$bibitems,$priority,$resdate,$expdate,$notes,$title,$checkitem,$found) + AddReserve( + { + branch => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + resdate => $reservation_date, + expdate => $expiration_date, + notes => $notes, + title => $title, + checkitem => $itemnumber, + found => $found, + itemtype => $itemtype, + } + ); Adds reserve and generates HOLDPLACED message. @@ -160,11 +174,18 @@ The following tables are available witin the HOLDPLACED message: =cut sub AddReserve { - my ( - $branch, $borrowernumber, $biblionumber, $bibitems, - $priority, $resdate, $expdate, $notes, - $title, $checkitem, $found, $itemtype - ) = @_; + my ($params) = @_; + my $branch = $params->{branchcode}; + my $borrowernumber = $params->{borrowernumber}; + my $biblionumber = $params->{biblionumber}; + my $priority = $params->{priority}; + my $resdate = $params->{reservation_date}; + my $expdate = $params->{expiration_date}; + my $notes = $params->{notes}; + my $title = $params->{title}; + my $checkitem = $params->{itemnumber}; + my $found = $params->{found}; + my $itemtype = $params->{itemtype}; $resdate = output_pref( { str => dt_from_string( $resdate ), dateonly => 1, dateformat => 'iso' }) or output_pref({ dt => dt_from_string, dateonly => 1, dateformat => 'iso' }); diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm index d834516d10..0bd77ad3ea 100644 --- a/C4/SIP/ILS/Transaction/Hold.pm +++ b/C4/SIP/ILS/Transaction/Hold.pm @@ -60,7 +60,13 @@ sub do_hold { $self->ok(0); return $self; } - AddReserve( $branch, $patron->borrowernumber, $item->biblionumber ); + AddReserve( + { + branch => $branch, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber + } + ); # unfortunately no meaningful return value $self->ok(1); diff --git a/Koha/REST/V1/Hold.pm b/Koha/REST/V1/Hold.pm index 2833f39f19..ad9ecaa54c 100644 --- a/Koha/REST/V1/Hold.pm +++ b/Koha/REST/V1/Hold.pm @@ -103,9 +103,18 @@ sub add { $expirationdate = output_pref(dt_from_string($expirationdate, 'iso')); } - my $reserve_id = C4::Reserves::AddReserve($branchcode, $borrowernumber, - $biblionumber, undef, $priority, undef, $expirationdate, undef, - $biblio->title, $itemnumber, undef, $itemtype); + my $reserve_id = C4::Reserves::AddReserve( + { + branchcode => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + expiration_date => $expirationdate, + title => $biblio->title, + itemnumber => $itemnumber, + itemtype => $itemtype + } + ); unless ($reserve_id) { return $c->render( status => 500, openapi => { diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index a9af8d168a..a13da5ba82 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -300,10 +300,19 @@ if ( $query->param('place_reserve') ) { # Here we actually do the reserveration. Stage 3. if ($canreserve) { my $reserve_id = AddReserve( - $branch, $borrowernumber, $biblioNum, - [$biblioNum], $rank, $startdate, - $expiration_date, $notes, $biblioData->{title}, - $itemNum, $found, $itemtype, + { + branchcode => $branch, + borrowernumber => $borrowernumber, + biblionumber => $biblioNum, + priority => $rank, + reservation_date => $startdate, + expiration_date => $expiration_date, + notes => $notes, + title => $biblioData->{title}, + itemnumber => $itemNum, + found => $found, + itemtype => $itemtype, + } ); $failed_holds++ unless $reserve_id; ++$reserve_cnt; diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl index 351041b5df..0cceeac978 100755 --- a/reserve/placerequest.pl +++ b/reserve/placerequest.pl @@ -95,14 +95,38 @@ if ( $type eq 'str8' && $borrower ) { if ($multi_hold) { my $bibinfo = $bibinfos{$biblionumber}; - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,[$biblionumber], - $bibinfo->{rank},$startdate,$expirationdate,$notes,$bibinfo->{title},$checkitem,$found); + AddReserve( + { + branchcode => $branch, + borrowernumber => $borrower->{'borrowernumber'}, + biblionumber => $biblionumber, + priority => $bibinfo->{rank}, + reservation_date => $startdate, + expiration_date => $expirationdate, + notes => $notes, + title => $bibinfo->{title}, + itemnumber => $checkitem, + found => $found, + } + ); } else { # place a request on 1st available for ( my $i = 0 ; $i < $holds_to_place_count ; $i++ ) { - AddReserve( $branch, $borrower->{'borrowernumber'}, - $biblionumber, \@realbi, $rank[0], $startdate, $expirationdate, $notes, $title, - $checkitem, $found, $itemtype ); + AddReserve( + { + branchcode => $branch, + borrowernumber => $borrower->{'borrowernumber'}, + biblionumber => $biblionumber, + priority => $rank[0], + reservation_date => $startdate, + expiration_date => $expirationdate, + notes => $notes, + title => $title, + itemnumber => $checkitem, + found => $found, + itemtype => $itemtype, + } + ); } } } diff --git a/serials/routing-preview.pl b/serials/routing-preview.pl index 015005c80e..db42b209dd 100755 --- a/serials/routing-preview.pl +++ b/serials/routing-preview.pl @@ -94,7 +94,16 @@ if($ok){ branchcode => $branch }); } else { - AddReserve($branch,$routing->{borrowernumber},$biblionumber,undef,$routing->{ranking}, undef, undef, $notes,$title); + AddReserve( + { + branchcode => $branch, + borrowernumber => $routing->{borrowernumber}, + biblionumber => $biblionumber, + priority => $routing->{ranking}, + notes => $notes, + title => $title, + } + ); } } } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index d383d3f9c8..a59b9ebe35 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -343,9 +343,18 @@ C4::Context->dbh->do("DELETE FROM accountlines"); # Biblio-level hold, renewal test AddReserve( - $branch, $reserving_borrowernumber, $biblionumber, - $bibitems, $priority, $resdate, $expdate, $notes, - $title, $checkitem, $found + { + branchcode => $branch, + borrowernumber => $reserving_borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + reservation_date => $resdate, + expiration_date => $expdate, + notes => $notes, + title => $title, + itemnumber => $checkitem, + found => $found, + } ); # Testing of feature to allow the renewal of reserved items if other items on the record can fill all needed holds @@ -408,9 +417,18 @@ C4::Context->dbh->do("DELETE FROM accountlines"); # Item-level hold, renewal test AddReserve( - $branch, $reserving_borrowernumber, $biblionumber, - $bibitems, $priority, $resdate, $expdate, $notes, - $title, $itemnumber, $found + { + branchcode => $branch, + borrowernumber => $reserving_borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + reservation_date => $resdate, + expiration_date => $expdate, + notes => $notes, + title => $title, + itemnumber => $itemnumber, + found => $found, + } ); ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $itemnumber, 1); @@ -1096,9 +1114,12 @@ C4::Context->dbh->do("DELETE FROM accountlines"); is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with no hold on the record' ); AddReserve( - $library2->{branchcode}, $borrowernumber2, $biblionumber, - '', 1, undef, undef, '', - undef, undef, undef + { + branchcode => $library2->{branchcode}, + borrowernumber => $borrowernumber2, + biblionumber => $biblionumber, + priority => 1, + } ); C4::Context->dbh->do("UPDATE issuingrules SET onshelfholds = 0"); @@ -1522,9 +1543,18 @@ subtest 'MultipleReserves' => sub { ); my $reserving_borrowernumber1 = Koha::Patron->new(\%reserving_borrower_data1)->store->borrowernumber; AddReserve( - $branch, $reserving_borrowernumber1, $biblionumber, - $bibitems, $priority, $resdate, $expdate, $notes, - $title, $checkitem, $found + { + branchcode => $branch, + borrowernumber => $reserving_borrowernumber1, + biblionumber => $biblionumber, + priority => $priority, + reservation_date => $resdate, + expiration_date => $expdate, + notes => $notes, + title => $title, + itemnumber => $checkitem, + found => $found, + } ); my %reserving_borrower_data2 = ( @@ -1535,9 +1565,18 @@ subtest 'MultipleReserves' => sub { ); my $reserving_borrowernumber2 = Koha::Patron->new(\%reserving_borrower_data2)->store->borrowernumber; AddReserve( - $branch, $reserving_borrowernumber2, $biblionumber, - $bibitems, $priority, $resdate, $expdate, $notes, - $title, $checkitem, $found + { + branchcode => $branch, + borrowernumber => $reserving_borrowernumber2, + biblionumber => $biblionumber, + priority => $priority, + reservation_date => $resdate, + expiration_date => $expdate, + notes => $notes, + title => $title, + itemnumber => $checkitem, + found => $found, + } ); { @@ -2133,8 +2172,13 @@ subtest 'Set waiting flag' => sub { set_userenv( $library_2 ); my $reserve_id = AddReserve( - $library_2->{branchcode}, $patron_2->{borrowernumber}, $biblioitem->{biblionumber}, - '', 1, undef, undef, '', undef, $item->{itemnumber}, + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->{borrowernumber}, + biblionumber => $biblioitem->{biblionumber}, + priority => 1, + itemnumber => $item->{itemnumber}, + } ); set_userenv( $library_1 ); diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t index f613f081f1..06e7cbf840 100644 --- a/t/db_dependent/Circulation/issue.t +++ b/t/db_dependent/Circulation/issue.t @@ -371,8 +371,16 @@ $item = GetItem( $itemnumber ); ok( $item->{notforloan} eq 9, q{UpdateNotForLoanStatusOnCheckin does not update notforloan value from 9 with setting "1: 9"} ); # Bug 14640 - Cancel the hold on checking out if asked -my $reserve_id = AddReserve($branchcode_1, $borrower_id1, $biblionumber, - undef, 1, undef, undef, "a note", "a title", undef, ''); +my $reserve_id = AddReserve( + { + branchcode => $branchcode_1, + borrowernumber => $borrower_id1, + biblionumber => $biblionumber, + priority => 1, + notes => "a note", + title => "a title", + } +); ok( $reserve_id, 'The reserve should have been inserted' ); AddIssue( $borrower_2, $barcode_1, dt_from_string, 'cancel' ); my $hold = Koha::Holds->find( $reserve_id ); diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 3c5fd1e90a..5e24c88cee 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -72,17 +72,14 @@ my $biblionumber = $bibnum; # Create five item level holds foreach my $borrowernumber ( @borrowernumbers ) { AddReserve( - $branch_1, - $borrowernumber, - $biblionumber, - my $bibitems = q{}, - my $priority = C4::Reserves::CalculatePriority( $biblionumber ), - my $resdate, - my $expdate, - my $notes = q{}, - $title, - my $checkitem = $itemnumber, - my $found, + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => C4::Reserves::CalculatePriority( $biblionumber ), + title => $title, + itemnumber => $itemnumber, + } ); } @@ -188,19 +185,15 @@ is( $hold->suspend, 0, "Test resuming with SuspendAll()" ); is( $hold->suspend_until, undef, "Test resuming with SuspendAll(), should have no suspend until date" ); # Add a new hold for the borrower whose hold we canceled earlier, this time at the bib level -AddReserve( - $branch_1, - $borrowernumbers[0], - $biblionumber, - my $bibitems = q{}, - my $priority, - my $resdate, - my $expdate, - my $notes = q{}, - $title, - my $checkitem, - my $found, -); + AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $biblionumber, + title => $title, + } + ); + $patron = Koha::Patrons->find( $borrowernumber ); $holds = $patron->holds; my $reserveid = Koha::Holds->search({ biblionumber => $bibnum, borrowernumber => $borrowernumbers[0] })->next->reserve_id; @@ -284,11 +277,34 @@ ok( # Regression test for bug 11336 # Test if ModReserve correctly recalculate the priorities ($bibnum, $title, $bibitemnum) = create_helper_biblio('DUMMY'); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1 } , $bibnum); - my $reserveid1 = AddReserve($branch_1, $borrowernumbers[0], $bibnum, '', 1); + my $reserveid1 = AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $bibnum, + priority => 1 + } + ); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1 } , $bibnum); - my $reserveid2 = AddReserve($branch_1, $borrowernumbers[1], $bibnum, '', 2); + my $reserveid2 = AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumbers[1], + biblionumber => $bibnum, + priority => 2 + } + ); + ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1 } , $bibnum); - my $reserveid3 = AddReserve($branch_1, $borrowernumbers[2], $bibnum, '', 3); + my $reserveid3 = AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumbers[2], + biblionumber => $bibnum, + priority => 3 + } + ); + my $hhh = Koha::Holds->search({ biblionumber => $bibnum }); my $hold3 = Koha::Holds->find( $reserveid3 ); is( $hold3->priority, 3, "The 3rd hold should have a priority set to 3" ); @@ -322,11 +338,12 @@ ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for d ($bibnum, $title, $bibitemnum) = create_helper_biblio('CANNOT'); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CANNOT' } , $bibnum); AddReserve( - $branch_1, - $borrowernumbers[0], - $bibnum, - '', - 1, + { + branchcode => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $bibnum, + priority => 1, + } ); is( CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'tooManyReserves', @@ -406,7 +423,14 @@ $dbh->do( is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), 'OK', 'Patron can reserve item with hold limit of 1, no holds placed' ); -my $res_id = AddReserve( $branch_1, $borrowernumbers[0], $bibnum, '', 1, ); +my $res_id = AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $bibnum, + priority => 1, + } +); is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), 'tooManyReserves', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); diff --git a/t/db_dependent/Holds/HoldFulfillmentPolicy.t b/t/db_dependent/Holds/HoldFulfillmentPolicy.t index 34bb3eddfa..b6d6e370e3 100755 --- a/t/db_dependent/Holds/HoldFulfillmentPolicy.t +++ b/t/db_dependent/Holds/HoldFulfillmentPolicy.t @@ -77,19 +77,40 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'homebranch' )"); # Home branch matches pickup branch -my $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); +my $reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); my ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup branch matches home branch targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is($status, q{}, "Hold where pickup ne home, pickup eq home not targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, q{}, "Hold where pickup ne home, pickup ne holding not targeted" ); Koha::Holds->find( $reserve_id )->cancel; @@ -99,19 +120,40 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'holdingbranch' )"); # Home branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, q{}, "Hold where pickup eq home, pickup ne holding not targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup ne home, pickup eq holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, q{}, "Hold where pickup ne home, pickup ne holding not targeted" ); Koha::Holds->find( $reserve_id )->cancel; @@ -121,19 +163,40 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'any' )"); # Home branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup eq home, pickup ne holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup ne home, pickup eq holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup ne home, pickup ne holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; diff --git a/t/db_dependent/Holds/HoldItemtypeLimit.t b/t/db_dependent/Holds/HoldItemtypeLimit.t index ba3e9c71f1..b5e47e569b 100644 --- a/t/db_dependent/Holds/HoldItemtypeLimit.t +++ b/t/db_dependent/Holds/HoldItemtypeLimit.t @@ -93,19 +93,43 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'any' )"); # Itemtypes match -my $reserve_id = AddReserve( $branchcode, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $right_itemtype ); +my $reserve_id = AddReserve( + { + branchcode => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + itemtype => $right_itemtype, + } +); my ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where itemtype matches item's itemtype targed" ); Koha::Holds->find( $reserve_id )->cancel; # Itemtypes don't match -$reserve_id = AddReserve( $branchcode, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $wrong_itemtype ); +$reserve_id = AddReserve( + { + branchcode => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + itemtype => $wrong_itemtype, + } +); + ( $status ) = CheckReserves($itemnumber); is($status, q{}, "Hold where itemtype does not match item's itemtype not targeted" ); Koha::Holds->find( $reserve_id )->cancel; # No itemtype set -$reserve_id = AddReserve( $branchcode, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, undef ); +$reserve_id = AddReserve( + { + branchcode => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Item targeted with no hold itemtype set" ); Koha::Holds->find( $reserve_id )->cancel; diff --git a/t/db_dependent/Holds/LocalHoldsPriority.t b/t/db_dependent/Holds/LocalHoldsPriority.t index af914d6895..680da29060 100755 --- a/t/db_dependent/Holds/LocalHoldsPriority.t +++ b/t/db_dependent/Holds/LocalHoldsPriority.t @@ -73,17 +73,13 @@ my $biblionumber = $bibnum; my $i = 1; foreach my $borrowernumber (@borrowernumbers) { AddReserve( - $branchcodes[$i], - $borrowernumber, - $biblionumber, - my $bibitems = q{}, - my $priority = $i, - my $resdate, - my $expdate, - my $notes = q{}, - $title, - my $checkitem, - my $found, + { + branchcode => $branchcodes[$i], + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => $i, + title => $title, + } ); $i++; diff --git a/t/db_dependent/Holds/RevertWaitingStatus.t b/t/db_dependent/Holds/RevertWaitingStatus.t index 9877e68b2c..0efac88405 100755 --- a/t/db_dependent/Holds/RevertWaitingStatus.t +++ b/t/db_dependent/Holds/RevertWaitingStatus.t @@ -84,17 +84,12 @@ my $biblionumber = $bibnum; # Create five item level holds foreach my $borrowernumber (@borrowernumbers) { AddReserve( - $branchcode, - $borrowernumber, - $biblionumber, - my $bibitems = q{}, - my $priority, - my $resdate, - my $expdate, - my $notes = q{}, - $title, - my $checkitem, - my $found, + { + branchcode => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + title => $title, + } ); } diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t index e2f749a623..c346ee689d 100755 --- a/t/db_dependent/HoldsQueue.t +++ b/t/db_dependent/HoldsQueue.t @@ -109,7 +109,14 @@ $dbh->do("DELETE FROM reserves"); my $bibitems = undef; my $priority = 1; # Make a reserve -AddReserve ( $borrower_branchcode, $borrowernumber, $biblionumber, $bibitems, $priority ); +AddReserve( + { + branchcode => $borrower_branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => $priority, + } +); # $resdate, $expdate, $notes, $title, $checkitem, $found $dbh->do("UPDATE reserves SET reservedate = DATE_SUB( reservedate, INTERVAL 1 DAY )"); @@ -464,7 +471,14 @@ $dbh->do( "UPDATE systempreferences SET value = ? WHERE variable = 'StaticHoldsQ undef, join( ',', $library_B, $library_A, $library_C ) ); $dbh->do( "UPDATE systempreferences SET value = 0 WHERE variable = 'RandomizeHoldsQueueWeight'" ); -my $reserve_id = AddReserve ( $library_C, $borrowernumber, $biblionumber, '', 1 ); +my $reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} }); is( @$holds_queue, 1, "Bug 14297 - Holds Queue building ignoring holds where pickup & home branch don't match and item is not from le"); @@ -512,7 +526,15 @@ $dbh->do(" VALUES ($biblionumber, $biblioitemnumber, '$library_A', '$library_A', 0, 0, 0, 0, NULL, '$itemtype') "); -$reserve_id = AddReserve ( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} }); is( @$holds_queue, 0, "Bug 15062 - Holds queue with Transport Cost Matrix will transfer item even if transfers disabled"); @@ -558,21 +580,46 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'homebranch' )"); # Home branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup branch matches home branch targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup ne home, pickup eq home not targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup ne home, pickup ne holding not targeted" ); @@ -583,21 +630,45 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'holdingbranch' )"); # Home branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup eq home, pickup ne holding not targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup ne home, pickup eq holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup ne home, pickup ne holding not targeted" ); @@ -608,21 +679,45 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'any' )"); # Home branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup eq home, pickup ne holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_B, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup ne home, pickup eq holding targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); +$reserve_id = AddReserve( + { + branchcode => $library_C, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup ne home, pickup ne holding targeted" ); @@ -670,21 +765,47 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy ) VALUES ( 2, 'any' )"); # Home branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $wrong_itemtype ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + itemtype => $wrong_itemtype, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Item with incorrect itemtype not targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $right_itemtype ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + itemtype => $right_itemtype, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Item with matching itemtype is targeted" ); Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch -$reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, undef ); +$reserve_id = AddReserve( + { + branchcode => $library_A, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Item targeted when hold itemtype is not set" ); @@ -718,7 +839,15 @@ my $item3 = Koha::Item->new( $item->unblessed ); $item3->itemnumber( undef ); $item3->store(); -$reserve_id = AddReserve( $item->homebranch, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, undef ); +$reserve_id = AddReserve( + { + branchcode => $item->homebranch, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + C4::HoldsQueue::CreateQueue(); diff --git a/t/db_dependent/Koha/Biblios.t b/t/db_dependent/Koha/Biblios.t index e92ae415b1..8099e1c033 100644 --- a/t/db_dependent/Koha/Biblios.t +++ b/t/db_dependent/Koha/Biblios.t @@ -61,7 +61,13 @@ subtest 'store' => sub { subtest 'holds + current_holds' => sub { plan tests => 5; - C4::Reserves::AddReserve( $patron->branchcode, $patron->borrowernumber, $biblio->biblionumber ); + C4::Reserves::AddReserve( + { + branchcode => $patron->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblio->biblionumber, + } + ); my $holds = $biblio->holds; is( ref($holds), 'Koha::Holds', '->holds should return a Koha::Holds object' ); is( $holds->count, 1, '->holds should only return 1 hold' ); @@ -69,7 +75,14 @@ subtest 'holds + current_holds' => sub { $holds->delete; # Add a hold in the future - C4::Reserves::AddReserve( $patron->branchcode, $patron->borrowernumber, $biblio->biblionumber, undef, undef, dt_from_string->add( days => 2 ) ); + C4::Reserves::AddReserve( + { + branchcode => $patron->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblio->biblionumber, + reservation_date => dt_from_string->add( days => 2 ), + } + ); $holds = $biblio->holds; is( $holds->count, 1, '->holds should return future holds' ); $holds = $biblio->current_holds; diff --git a/t/db_dependent/Koha/Holds.t b/t/db_dependent/Koha/Holds.t index c2399d66b5..2cfe9afbd5 100644 --- a/t/db_dependent/Koha/Holds.t +++ b/t/db_dependent/Koha/Holds.t @@ -58,11 +58,14 @@ subtest 'cancel' => sub { } ); my $reserve_id = C4::Reserves::AddReserve( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - $priority, undef, - undef, '', - "title for fee", $item->itemnumber, + { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => $priority, + title => "title for fee", + itemnumber => $item->itemnumber, + } ); my $hold = Koha::Holds->find($reserve_id); push @patrons, $patron; @@ -105,30 +108,31 @@ subtest 'cancel' => sub { my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { categorycode => $patron_category->categorycode } }); is( $patron->account->balance, 0, 'A new patron does not have any charges' ); - my @hold_info = ( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - 1, undef, - undef, '', - "title for fee", $item->itemnumber, - ); + my $hold_info = { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + title => "title for fee", + itemnumber => $item->itemnumber, + }; # First, test cancelling a reserve when there's no charge configured. t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 0); - my $reserve_id = C4::Reserves::AddReserve( @hold_info ); + my $reserve_id = C4::Reserves::AddReserve( $hold_info ); Koha::Holds->find( $reserve_id )->cancel( { charge_cancel_fee => 1 } ); is( $patron->account->balance, 0, 'ExpireReservesMaxPickUpDelayCharge=0 - The patron should not have been charged' ); # Then, test cancelling a reserve when there's no charge desired. t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 42); - $reserve_id = C4::Reserves::AddReserve( @hold_info ); + $reserve_id = C4::Reserves::AddReserve( $hold_info ); Koha::Holds->find( $reserve_id )->cancel(); # charge_cancel_fee => 0 is( $patron->account->balance, 0, 'ExpireReservesMaxPickUpDelayCharge=42, but charge_cancel_fee => 0, The patron should not have been charged' ); # Finally, test cancelling a reserve when there's a charge desired and configured. t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 42); - $reserve_id = C4::Reserves::AddReserve( @hold_info ); + $reserve_id = C4::Reserves::AddReserve( $hold_info ); Koha::Holds->find( $reserve_id )->cancel( { charge_cancel_fee => 1 } ); is( int($patron->account->balance), 42, 'ExpireReservesMaxPickUpDelayCharge=42 and charge_cancel_fee => 1, The patron should have been charged!' ); }; @@ -137,12 +141,15 @@ subtest 'cancel' => sub { plan tests => 1; my $patron = $builder->build_object({ class => 'Koha::Patrons' }); my $reserve_id = C4::Reserves::AddReserve( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - 1, undef, - undef, '', - "title for fee", $item->itemnumber, - 'W', + { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + title => "title for fee", + itemnumber => $item->itemnumber, + found => 'W', + } ); Koha::Holds->find( $reserve_id )->cancel; my $hold_old = Koha::Old::Holds->find( $reserve_id ); @@ -152,22 +159,23 @@ subtest 'cancel' => sub { subtest 'HoldsLog' => sub { plan tests => 2; my $patron = $builder->build_object({ class => 'Koha::Patrons' }); - my @hold_info = ( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - 1, undef, - undef, '', - "title for fee", $item->itemnumber, - ); + my $hold_info = { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + title => "title for fee", + itemnumber => $item->itemnumber, + }; t::lib::Mocks::mock_preference('HoldsLog', 0); - my $reserve_id = C4::Reserves::AddReserve(@hold_info); + my $reserve_id = C4::Reserves::AddReserve($hold_info); Koha::Holds->find( $reserve_id )->cancel; my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'HOLDS', action => 'CANCEL', object => $reserve_id } )->count; is( $number_of_logs, 0, 'Without HoldsLog, Koha::Hold->cancel should not have logged' ); t::lib::Mocks::mock_preference('HoldsLog', 1); - $reserve_id = C4::Reserves::AddReserve(@hold_info); + $reserve_id = C4::Reserves::AddReserve($hold_info); Koha::Holds->find( $reserve_id )->cancel; $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'HOLDS', action => 'CANCEL', object => $reserve_id } )->count; is( $number_of_logs, 1, 'With HoldsLog, Koha::Hold->cancel should have logged' ); @@ -187,16 +195,17 @@ subtest 'cancel' => sub { value => { categorycode => $patron_category->categorycode } } ); - my @hold_info = ( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - 1, undef, - undef, '', - "title for fee", $item->itemnumber, - ); + my $hold_info = { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + title => "title for fee", + itemnumber => $item->itemnumber, + }; t::lib::Mocks::mock_preference( 'ExpireReservesMaxPickUpDelayCharge',42 ); - my $reserve_id = C4::Reserves::AddReserve(@hold_info); + my $reserve_id = C4::Reserves::AddReserve($hold_info); my $hold = Koha::Holds->find($reserve_id); # Add a row with the same id to make the cancel fails diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 6521db1a51..07c95bf263 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -774,11 +774,22 @@ subtest 'holds and old_holds' => sub { 'Koha::Patron->holds should return a Koha::Holds objects' ); is( $holds->count, 0, 'There should not be holds placed by this patron yet' ); - C4::Reserves::AddReserve( $library->{branchcode}, - $patron->borrowernumber, $biblionumber_1 ); + C4::Reserves::AddReserve( + { + branchcode => $library->{branchcode}, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblionumber_1 + } + ); # In the future - C4::Reserves::AddReserve( $library->{branchcode}, - $patron->borrowernumber, $biblionumber_2, undef, undef, dt_from_string->add( days => 2 ) ); + C4::Reserves::AddReserve( + { + branchcode => $library->{branchcode}, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblionumber_2, + expiration_date => dt_from_string->add( days => 2 ) + } + ); $holds = $patron->holds; is( $holds->count, 2, 'There should be 2 holds placed by this patron' ); diff --git a/t/db_dependent/Letters/TemplateToolkit.t b/t/db_dependent/Letters/TemplateToolkit.t index c4f4e82866..dcdfabaa72 100644 --- a/t/db_dependent/Letters/TemplateToolkit.t +++ b/t/db_dependent/Letters/TemplateToolkit.t @@ -521,8 +521,25 @@ You have [% count %] items due my $code = 'HOLD_SLIP'; - C4::Reserves::AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio1->{biblionumber}, undef, undef, undef, undef, "a note", undef, $item1->{itemnumber}, 'W' ); - C4::Reserves::AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio2->{biblionumber}, undef, undef, undef, undef, "another note", undef, $item2->{itemnumber} ); + C4::Reserves::AddReserve( + { + branchcode => $library->{branchcode}, + borrowernumber => $patron->{borrowernumber}, + biblionumber => $biblio1->{biblionumber}, + notes => "a note", + itemnumber => $item1->{itemnumber}, + found => 'W' + } + ); + C4::Reserves::AddReserve( + { + branchcode => $library->{branchcode}, + borrowernumber => $patron->{borrowernumber}, + biblionumber => $biblio2->{biblionumber}, + notes => "another note", + itemnumber => $item2->{itemnumber}, + } + ); my $template = <Date: <> diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 406e45036e..4f8beffb92 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -125,19 +125,16 @@ my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; my $biblionumber = $bibnum; my $barcode = $testbarcode; -my $bibitems = ''; -my $priority = '1'; -my $resdate = undef; -my $expdate = undef; -my $notes = ''; -my $checkitem = undef; -my $found = undef; - my $branchcode = Koha::Libraries->search->next->branchcode; -AddReserve($branchcode, $borrowernumber, $biblionumber, - $bibitems, $priority, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branchcode, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); my ($status, $reserve, $all_reserves) = CheckReserves($itemnumber, $barcode); @@ -260,15 +257,30 @@ my ($itemnum_cpl, $itemnum_fpl); # Ensure that priorities are numbered correcly when a hold is moved to waiting # (bug 11947) $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum2)); -AddReserve($branch_3, $requesters{$branch_3}, $bibnum2, - $bibitems, 1, $resdate, $expdate, $notes, - $title, $checkitem, $found); -AddReserve($branch_2, $requesters{$branch_2}, $bibnum2, - $bibitems, 2, $resdate, $expdate, $notes, - $title, $checkitem, $found); -AddReserve($branch_1, $requesters{$branch_1}, $bibnum2, - $bibitems, 3, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_3, + borrowernumber => $requesters{$branch_3}, + biblionumber => $bibnum2, + priority => 1, + } +); +AddReserve( + { + branchcode => $branch_2, + borrowernumber => $requesters{$branch_2}, + biblionumber => $bibnum2, + priority => 2, + } +); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum2, + priority => 3, + } +); ModReserveAffect($itemnum_cpl, $requesters{$branch_3}, 0); # Now it should have different priorities. @@ -284,15 +296,31 @@ is( $reserves[0]->borrowernumber(), $requesters{$branch_3}, 'GetWaiting got the $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum2)); -AddReserve($branch_3, $requesters{$branch_3}, $bibnum2, - $bibitems, 1, $resdate, $expdate, $notes, - $title, $checkitem, $found); -AddReserve($branch_2, $requesters{$branch_2}, $bibnum2, - $bibitems, 2, $resdate, $expdate, $notes, - $title, $checkitem, $found); -AddReserve($branch_1, $requesters{$branch_1}, $bibnum2, - $bibitems, 3, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_3, + borrowernumber => $requesters{$branch_3}, + biblionumber => $bibnum2, + priority => 1, + } +); +AddReserve( + { + branchcode => $branch_2, + borrowernumber => $requesters{$branch_2}, + biblionumber => $bibnum2, + priority => 2, + } +); + +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum2, + priority => 3, + } +); # Ensure that the item's home library controls hold policy lookup t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'ItemHomeLibrary' ); @@ -326,12 +354,15 @@ my $reserve_id = $holds->next->reserve_id; # Tests for bug 9761 (ConfirmFutureHolds): new CheckReserves lookahead parameter, and corresponding change in AddReturn # Note that CheckReserve uses its lookahead parameter and does not check ConfirmFutureHolds pref (it should be passed if needed like AddReturn does) # Test 9761a: Add a reserve without date, CheckReserve should return it -$resdate= undef; #defaults to today in AddReserve -$expdate= undef; #no expdate $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); -AddReserve($branch_1, $requesters{$branch_1}, $bibnum, - $bibitems, 1, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum, + priority => 1, + } +); ($status)=CheckReserves($itemnumber,undef,undef); is( $status, 'Reserved', 'CheckReserves returns reserve without lookahead'); ($status)=CheckReserves($itemnumber,undef,7); @@ -340,13 +371,18 @@ is( $status, 'Reserved', 'CheckReserves also returns reserve with lookahead'); # Test 9761b: Add a reserve with future date, CheckReserve should not return it $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); -$resdate= dt_from_string(); +my $resdate= dt_from_string(); $resdate->add_duration(DateTime::Duration->new(days => 4)); $resdate=output_pref($resdate); -$expdate= undef; #no expdate -AddReserve($branch_1, $requesters{$branch_1}, $bibnum, - $bibitems, 1, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum, + priority => 1, + reservation_date => $resdate, + } +); ($status)=CheckReserves($itemnumber,undef,undef); is( $status, '', 'CheckReserves returns no future reserve without lookahead'); @@ -401,9 +437,16 @@ t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); $resdate= dt_from_string(); $resdate->add_duration(DateTime::Duration->new(days => 2)); $resdate=output_pref($resdate); -AddReserve($branch_1, $requesters{$branch_1}, $bibnum, - $bibitems, 1, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum, + priority => 1, + reservation_date => $resdate, + } +); + my $item = Koha::Items->find( $itemnumber ); $holds = $item->current_holds; my $dtf = Koha::Database->new->schema->storage->datetime_parser; @@ -411,9 +454,16 @@ my $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( d is( $future_holds->count, 0, 'current_holds does not return a future next available hold'); # 9788b: current_holds 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 +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum, + priority => 1, + reservation_date => $resdate, + itemnumber => $itemnumber, + } +); #item level hold $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } ); is( $future_holds->count, 0, 'current_holds does not return a future item level hold' ); # 9788c: current_holds returns future wait (confirmed future hold) @@ -426,10 +476,14 @@ $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); # Tests for CalculatePriority (bug 8918) my $p = C4::Reserves::CalculatePriority($bibnum2); is($p, 4, 'CalculatePriority should now return priority 4'); -$resdate=undef; -AddReserve($branch_1, $requesters{'CPL2'}, $bibnum2, - $bibitems, $p, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{'CPL2'}, + biblionumber => $bibnum2, + priority => $p, + } +); $p = C4::Reserves::CalculatePriority($bibnum2); is($p, 5, 'CalculatePriority should now return priority 5'); #some tests on bibnum @@ -437,27 +491,44 @@ $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 1, 'CalculatePriority should now return priority 1'); #add a new reserve and confirm it to waiting -AddReserve($branch_1, $requesters{$branch_1}, $bibnum, - $bibitems, $p, $resdate, $expdate, $notes, - $title, $itemnumber, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $bibnum, + priority => $p, + itemnumber => $itemnumber, + } +); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 2, 'CalculatePriority should now return priority 2'); ModReserveAffect( $itemnumber, $requesters{$branch_1} , 0); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 1, 'CalculatePriority should now return priority 1'); #add another biblio hold, no resdate -AddReserve($branch_1, $requesters{'CPL2'}, $bibnum, - $bibitems, $p, $resdate, $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{'CPL2'}, + biblionumber => $bibnum, + priority => $p, + } +); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 2, 'CalculatePriority should now return priority 2'); #add another future hold t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); $resdate= dt_from_string(); $resdate->add_duration(DateTime::Duration->new(days => 1)); -AddReserve($branch_1, $requesters{'CPL3'}, $bibnum, - $bibitems, $p, output_pref($resdate), $expdate, $notes, - $title, $checkitem, $found); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{'CPL2'}, + biblionumber => $bibnum, + priority => $p, + reservation_date => output_pref($resdate), + } +); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 2, 'CalculatePriority should now still return priority 2'); #calc priority with future resdate @@ -467,9 +538,14 @@ is($p, 3, 'CalculatePriority should now return priority 3'); # Tests for cancel reserves by users from OPAC. $dbh->do('DELETE FROM reserves', undef, ($bibnum)); -AddReserve($branch_1, $requesters{$branch_1}, $item_bibnum, - $bibitems, 1, undef, $expdate, $notes, - $title, $checkitem, ''); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $item_bibnum, + priority => 1, + } +); my (undef, $canres, undef) = CheckReserves($itemnumber); is( CanReserveBeCanceledFromOpac(), undef, @@ -497,9 +573,14 @@ $cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$br is($cancancel, 0, 'Reserve in transfer status cant be canceled'); $dbh->do('DELETE FROM reserves', undef, ($bibnum)); -AddReserve($branch_1, $requesters{$branch_1}, $item_bibnum, - $bibitems, 1, undef, $expdate, $notes, - $title, $checkitem, ''); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $requesters{$branch_1}, + biblionumber => $item_bibnum, + priority => 1, + } +); (undef, $canres, undef) = CheckReserves($itemnumber); ModReserveAffect($itemnumber, $requesters{$branch_1}, 0); @@ -548,14 +629,27 @@ ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $borrower), "Reserving a $dbh->do('DELETE FROM reserves', undef, ($bibnum)); t::lib::Mocks::mock_preference('ConfirmFutureHolds', 0); t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); -AddReserve($branch_1, $borrowernumber, $item_bibnum, - $bibitems, 1, undef, $expdate, $notes, $title, $checkitem, ''); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $item_bibnum, + priority => 1, + } +); MoveReserve( $itemnumber, $borrowernumber ); ($status)=CheckReserves( $itemnumber ); is( $status, '', 'MoveReserve filled hold'); # hold from A waiting, today, no fut holds: MoveReserve should fill it -AddReserve($branch_1, $borrowernumber, $item_bibnum, - $bibitems, 1, undef, $expdate, $notes, $title, $checkitem, 'W'); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $item_bibnum, + priority => 1, + found => 'W', + } +); MoveReserve( $itemnumber, $borrowernumber ); ($status)=CheckReserves( $itemnumber ); is( $status, '', 'MoveReserve filled waiting hold'); @@ -563,22 +657,43 @@ is( $status, '', 'MoveReserve filled waiting hold'); $resdate= dt_from_string(); $resdate->add_duration(DateTime::Duration->new(days => 1)); $resdate=output_pref($resdate); -AddReserve($branch_1, $borrowernumber, $item_bibnum, - $bibitems, 1, $resdate, $expdate, $notes, $title, $checkitem, ''); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $item_bibnum, + priority => 1, + reservation_date => $resdate, + } +); MoveReserve( $itemnumber, $borrowernumber ); ($status)=CheckReserves( $itemnumber, undef, 1 ); is( $status, 'Reserved', 'MoveReserve did not fill future hold'); $dbh->do('DELETE FROM reserves', undef, ($bibnum)); # hold from A pos 1, tomorrow, fut holds=2: MoveReserve should fill it t::lib::Mocks::mock_preference('ConfirmFutureHolds', 2); -AddReserve($branch_1, $borrowernumber, $item_bibnum, - $bibitems, 1, $resdate, $expdate, $notes, $title, $checkitem, ''); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $item_bibnum, + priority => 1, + reservation_date => $resdate, + } +); MoveReserve( $itemnumber, $borrowernumber ); ($status)=CheckReserves( $itemnumber, undef, 2 ); is( $status, '', 'MoveReserve filled future hold now'); # hold from A waiting, tomorrow, fut holds=2: MoveReserve should fill it -AddReserve($branch_1, $borrowernumber, $item_bibnum, - $bibitems, 1, $resdate, $expdate, $notes, $title, $checkitem, 'W'); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $item_bibnum, + priority => 1, + reservation_date => $resdate, + } +); MoveReserve( $itemnumber, $borrowernumber ); ($status)=CheckReserves( $itemnumber, undef, 2 ); is( $status, '', 'MoveReserve filled future waiting hold now'); @@ -586,8 +701,15 @@ is( $status, '', 'MoveReserve filled future waiting hold now'); $resdate= dt_from_string(); $resdate->add_duration(DateTime::Duration->new(days => 3)); $resdate=output_pref($resdate); -AddReserve($branch_1, $borrowernumber, $item_bibnum, - $bibitems, 1, $resdate, $expdate, $notes, $title, $checkitem, ''); +AddReserve( + { + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $item_bibnum, + priority => 1, + reservation_date => $resdate, + } +); MoveReserve( $itemnumber, $borrowernumber ); ($status)=CheckReserves( $itemnumber, undef, 3 ); is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days'); @@ -648,8 +770,12 @@ subtest '_koha_notify_reserve() tests' => sub { })->{borrowernumber}; C4::Reserves::AddReserve( - $item->{homebranch}, $hold_borrower, - $item->{biblionumber} ); + { + branchcode => $item->{homebranch}, + borrowernumber => $hold_borrower, + biblionumber => $item->{biblionumber}, + } + ); ModReserveAffect($item->{itemnumber}, $hold_borrower, 0); my $sms_message_address = $schema->resultset('MessageQueue')->search({ @@ -694,11 +820,14 @@ subtest 'ReservesNeedReturns' => sub { t::lib::Mocks::mock_preference('ReservesNeedReturns', 0); # '0' means 'Automatically mark a hold as found and waiting' $hold_id = C4::Reserves::AddReserve( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - $priority, undef, - undef, '', - "title for fee", $item->itemnumber, + { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => $priority, + title => "title for fee", + itemnumber => $item->itemnumber, + } ); $hold = Koha::Holds->find($hold_id); is( $hold->priority, 0, 'If ReservesNeedReturns is 0, priority must have been set to 0' ); @@ -708,11 +837,14 @@ subtest 'ReservesNeedReturns' => sub { t::lib::Mocks::mock_preference('ReservesNeedReturns', 1); # '0' means "Don't automatically mark a hold as found and waiting" $hold_id = C4::Reserves::AddReserve( - $library->branchcode, $patron->borrowernumber, - $item->biblionumber, '', - $priority, undef, - undef, '', - "title for fee", $item->itemnumber, + { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => $priority, + title => "title for fee", + itemnumber => $item->itemnumber, + } ); $hold = Koha::Holds->find($hold_id); is( $hold->priority, $priority, 'If ReservesNeedReturns is 1, priority must not have been set to changed' ); diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t index 80d428b0b5..4645c49901 100755 --- a/t/db_dependent/Reserves/GetReserveFee.t +++ b/t/db_dependent/Reserves/GetReserveFee.t @@ -205,17 +205,13 @@ sub acctlines { #calculate number of accountlines for a patron sub addreserve { return AddReserve( - $library->{branchcode}, - $_[0], - $biblio->{biblionumber}, - undef, - '1', - undef, - undef, - '', - $biblio->{title}, - undef, - '' + { + branchcode => $library->{branchcode}, + borrowernumber => $_[0], + biblionumber => $biblio->{biblionumber}, + priority => '1', + title => $biblio->{title}, + } ); } diff --git a/t/db_dependent/Reserves/MultiplePerRecord.t b/t/db_dependent/Reserves/MultiplePerRecord.t index 70f26001dc..ae5853c861 100755 --- a/t/db_dependent/Reserves/MultiplePerRecord.t +++ b/t/db_dependent/Reserves/MultiplePerRecord.t @@ -278,12 +278,27 @@ $rule = $rules_rs->new( my $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}); is( $can, 'OK', 'Hold can be placed with 0 holds' ); -my $hold_id = AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio->{biblionumber}, '', 1 ); +my $hold_id = AddReserve( + { + branchcode => $library->{branchcode}, + borrowernumber => $patron->{borrowernumber}, + biblionumber => $biblio->{biblionumber}, + priority => 1 + } +); ok( $hold_id, 'First hold was placed' ); $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}); is( $can, 'OK', 'Hold can be placed with 1 hold' ); -$hold_id = AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio->{biblionumber}, '', 1 ); +$hold_id = AddReserve( + { + branchcode => $library->{branchcode}, + borrowernumber => $patron->{borrowernumber}, + biblionumber => $biblio->{biblionumber}, + priority => 1 + } +); + ok( $hold_id, 'Second hold was placed' ); $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}); diff --git a/t/db_dependent/UsageStats.t b/t/db_dependent/UsageStats.t index 9d11af1674..8bd6e5bb9c 100644 --- a/t/db_dependent/UsageStats.t +++ b/t/db_dependent/UsageStats.t @@ -301,7 +301,15 @@ sub construct_objects_needed { my $issue_id1 = $dbh->last_insert_id( undef, undef, 'old_issues', undef ); # ---------- Add 1 old_reserves - AddReserve( $branchcode, $borrowernumber1, $biblionumber1, '', 1, undef, undef, '', 'Title', undef, undef ); + AddReserve( + { + branchcode => $branchcode, + borrowernumber => $borrowernumber1, + biblionumber => $biblionumber1, + priority => 1, + title => 'Title', + } + ); my $biblio = Koha::Biblios->find( $biblionumber1 ); my $holds = $biblio->holds; $holds->next->cancel if $holds->count; diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index 1e00be1a3b..45220c460f 100644 --- a/t/db_dependent/api/v1/holds.t +++ b/t/db_dependent/api/v1/holds.t @@ -142,12 +142,26 @@ $dbh->do('DELETE FROM issuingrules'); VALUES (?, ?, ?, ?) }, {}, '*', '*', '*', 1); -my $reserve_id = C4::Reserves::AddReserve($branchcode, $patron_1->borrowernumber, - $biblionumber, undef, 1, undef, undef, undef, '', $itemnumber); +my $reserve_id = C4::Reserves::AddReserve( + { + branchcode => $branchcode, + borrowernumber => $patron_1->borrowernumber, + biblionumber => $biblionumber, + priority => 1, + itemnumber => $itemnumber, + } +); # Add another reserve to be able to change first reserve's rank -my $reserve_id2 = C4::Reserves::AddReserve($branchcode, $patron_2->borrowernumber, - $biblionumber, undef, 2, undef, undef, undef, '', $itemnumber); +my $reserve_id2 = C4::Reserves::AddReserve( + { + branchcode => $branchcode, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $biblionumber, + priority => 2, + itemnumber => $itemnumber, + } +); my $suspend_until = DateTime->now->add(days => 10)->ymd; my $expirationdate = DateTime->now->add(days => 10)->ymd; -- 2.11.0