From 917a506ffc983f63d9541c862180916bb2adc0a0 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 12 Sep 2017 15:26:27 -0300 Subject: [PATCH] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected Signed-off-by: Hayley Mapley Signed-off-by: Marcel de Rooy Signed-off-by: Nick Clemens --- C4/Circulation.pm | 20 +++--- C4/ILSDI/Services.pm | 2 +- C4/Reserves.pm | 28 ++++----- opac/opac-reserve.pl | 2 +- reserve/request.pl | 2 +- .../Holds/DisallowHoldIfItemsAvailable.t | 62 ++++++++----------- t/db_dependent/Reserves.t | 20 +++--- 7 files changed, 62 insertions(+), 74 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b9ff0f2824..e1b0ed6348 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2686,16 +2686,16 @@ sub CanBookBeRenewed { # can be filled with available items. We can get the union of the sets simply # by pushing all the elements onto an array and removing the duplicates. my @reservable; - my %borrowers; - ITEM: foreach my $i (@itemnumbers) { - my $item = Koha::Items->find($i)->unblessed; - next if IsItemOnHoldAndFound($i); - for my $b (@borrowernumbers) { - my $borr = $borrowers{$b} //= Koha::Patrons->find( $b )->unblessed; - next unless IsAvailableForItemLevelRequest($item, $borr); - next unless CanItemBeReserved($b,$i); - - push @reservable, $i; + my %patrons; + ITEM: foreach my $itemnumber (@itemnumbers) { + my $item = Koha::Items->find( $itemnumber ); + next if IsItemOnHoldAndFound( $itemnumber ); + for my $borrowernumber (@borrowernumbers) { + my $patron = $patrons{$borrowernumber} //= Koha::Patrons->find( $borrowernumber ); + next unless IsAvailableForItemLevelRequest($item, $patron); + next unless CanItemBeReserved($borrowernumber,$itemnumber); + + push @reservable, $itemnumber; if (@reservable >= @borrowernumbers) { $resfound = 0; last ITEM; diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 43935bbf8f..8833094109 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -562,7 +562,7 @@ sub GetServices { my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber ); if ($canbookbereserved->{status} eq 'OK') { push @availablefor, 'title level hold'; - my $canitembereserved = IsAvailableForItemLevelRequest($item->unblessed, $borrower); + my $canitembereserved = IsAvailableForItemLevelRequest($item, $patron); if ($canitembereserved) { push @availablefor, 'item level hold'; } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 5fc68f1359..205ed7873a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1163,46 +1163,42 @@ and canreservefromotherbranches. =cut sub IsAvailableForItemLevelRequest { - my $item = shift; - my $borrower = shift; - my $pickup_branchcode = shift; + my ( $item, $patron, $pickup_branchcode ) = @_; my $dbh = C4::Context->dbh; # must check the notforloan setting of the itemtype # FIXME - a lot of places in the code do this # or something similar - need to be # consolidated - my $patron = Koha::Patrons->find( $borrower->{borrowernumber} ); - my $item_object = Koha::Items->find( $item->{itemnumber } ); - my $itemtype = $item_object->effective_itemtype; + my $itemtype = $item->effective_itemtype; my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan; return 0 if $notforloan_per_itemtype || - $item->{itemlost} || - $item->{notforloan} > 0 || - $item->{withdrawn} || - ($item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems')); + $item->itemlost || + $item->notforloan > 0 || + $item->withdrawn || + ($item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems')); - my $on_shelf_holds = Koha::IssuingRules->get_onshelfholds_policy( { item => $item_object, patron => $patron } ); + my $on_shelf_holds = Koha::IssuingRules->get_onshelfholds_policy( { item => $item, patron => $patron } ); if ($pickup_branchcode) { my $destination = Koha::Libraries->find($pickup_branchcode); return 0 unless $destination; return 0 unless $destination->pickup_location; - return 0 unless $item_object->can_be_transferred( { to => $destination } ); + return 0 unless $item->can_be_transferred( { to => $destination } ); } if ( $on_shelf_holds == 1 ) { return 1; } elsif ( $on_shelf_holds == 2 ) { my @items = - Koha::Items->search( { biblionumber => $item->{biblionumber} } ); + Koha::Items->search( { biblionumber => $item->biblionumber } ); my $any_available = 0; foreach my $i (@items) { - my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $borrower ); + my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $patron->unblessed ); my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); $any_available = 1 @@ -1214,12 +1210,12 @@ sub IsAvailableForItemLevelRequest { || ( $i->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ) || Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan - || $branchitemrule->{holdallowed} == 1 && $borrower->{branchcode} ne $i->homebranch; + || $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch; } return $any_available ? 0 : 1; } else { # on_shelf_holds == 0 "If any unavailable" (the description is rather cryptic and could still be improved) - return $item->{onloan} || IsItemOnHoldAndFound( $item->{itemnumber} ); + return $item->onloan || IsItemOnHoldAndFound( $item->itemnumber ); } } diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index eb017b6485..472d0f5519 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -537,7 +537,7 @@ foreach my $biblioNum (@biblionumbers) { my $policy_holdallowed = !$itemLoopIter->{already_reserved}; $policy_holdallowed &&= - IsAvailableForItemLevelRequest($itemInfo,$patron_unblessed) && + IsAvailableForItemLevelRequest($item, $patron) && CanItemBeReserved( $borrowernumber, $itemNum )->{status} eq 'OK'; if ($policy_holdallowed) { diff --git a/reserve/request.pl b/reserve/request.pl index df9cae861d..f559ed0929 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -497,7 +497,7 @@ foreach my $biblionumber (@biblionumbers) { if ( !$item->{cantreserve} && !$exceeded_maxreserves - && IsAvailableForItemLevelRequest($item, $patron_unblessed) + && IsAvailableForItemLevelRequest($item_object, $patron) && $can_item_be_reserved->{status} eq 'OK' ) { diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index 4302170d02..c5a3b8fbb1 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -36,32 +36,31 @@ my $itemtype = $builder->build({ t::lib::Mocks::mock_userenv({ branchcode => $library1->{branchcode} }); -my $borrower1 = $builder->build({ - source => 'Borrower', +my $patron1 = $builder->build_object({ + class => 'Koha::Patrons', value => { branchcode => $library1->{branchcode}, dateexpiry => '3000-01-01', } }); +my $borrower1 = $patron1->unblessed; -my $borrower2 = $builder->build({ - source => 'Borrower', +my $patron2 = $builder->build_object({ + class => 'Koha::Patrons', value => { branchcode => $library1->{branchcode}, dateexpiry => '3000-01-01', } }); -my $borrower3 = $builder->build({ - source => 'Borrower', +my $patron3 = $builder->build_object({ + class => 'Koha::Patrons', value => { branchcode => $library2->{branchcode}, dateexpiry => '3000-01-01', } }); -my $borrowernumber1 = $borrower1->{borrowernumber}; -my $borrowernumber2 = $borrower2->{borrowernumber}; my $library_A = $library1->{branchcode}; my $library_B = $library2->{branchcode}; @@ -72,18 +71,15 @@ my $item1 = $builder->build_sample_item({ itype=>$itemtype, homebranch => $library_A, holdingbranch => $library_A -})->unblessed; +}); my $item2 = $builder->build_sample_item({ biblionumber=>$biblionumber, itype=>$itemtype, homebranch => $library_A, holdingbranch => $library_A -})->unblessed; +}); # Test hold_fulfillment_policy - - - my $rule = Koha::IssuingRule->new( { categorycode => '*', @@ -97,20 +93,20 @@ my $rule = Koha::IssuingRule->new( ); $rule->store(); -my $is = IsAvailableForItemLevelRequest( $item1, $borrower1); +my $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Item cannot be held, 2 items available" ); -my $issue1 = AddIssue( $borrower2, $item1->{barcode} ); +my $issue1 = AddIssue( $patron2->unblessed, $item1->barcode ); -$is = IsAvailableForItemLevelRequest( $item1, $borrower1); +$is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Item cannot be held, 1 item available" ); -AddIssue( $borrower2, $item2->{barcode} ); +AddIssue( $patron2->unblessed, $item2->barcode ); -$is = IsAvailableForItemLevelRequest( $item1, $borrower1); +$is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 1, "Item can be held, no items available" ); -AddReturn( $item1->{barcode} ); +AddReturn( $item1->barcode ); { # Remove the issue for the first patron, and modify the branch for item1 subtest 'IsAvailableForItemLevelRequest behaviours depending on ReservesControlBranch + holdallowed' => sub { @@ -125,9 +121,7 @@ AddReturn( $item1->{barcode} ); subtest 'Item is available at a different library' => sub { plan tests => 7; - $item1 = Koha::Items->find( $item1->{itemnumber} ); $item1->set({homebranch => $library_B, holdingbranch => $library_B })->store; - $item1 = $item1->unblessed; #Scenario is: #One shelf holds is 'If all unavailable'/2 #Item 1 homebranch library B is available @@ -139,21 +133,21 @@ AddReturn( $item1->{barcode} ); $sth_insert_rule->execute( $hold_allowed_from_home_library ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 1, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at different library, not holdable = none available => the hold is allowed at item level" ); - $is = IsAvailableForItemLevelRequest( $item1, $borrower2); + $is = IsAvailableForItemLevelRequest( $item1, $patron2); is( $is, 1, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at home library, holdable = one available => the hold is not allowed at item level" ); $sth_insert_branch_rule->execute( $library_B, $hold_allowed_from_any_libraries ); #Adding a rule for the item's home library affects the availability for a borrower from another library because ReservesControlBranch is set to ItemHomeLibrary - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at different library, holdable = one available => the hold is not allowed at item level" ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 1, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at different library, not holdable = none available => the hold is allowed at item level" ); #Adding a rule for the patron's home library affects the availability for an item from another library because ReservesControlBranch is set to PatronLibrary $sth_insert_branch_rule->execute( $library_A, $hold_allowed_from_any_libraries ); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at different library, holdable = one available => the hold is not allowed at item level" ); } @@ -162,11 +156,11 @@ AddReturn( $item1->{barcode} ); $sth_insert_rule->execute( $hold_allowed_from_any_libraries ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=PatronLibrary, One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" ); } }; @@ -174,9 +168,7 @@ AddReturn( $item1->{barcode} ); subtest 'Item is available at the same library' => sub { plan tests => 4; - $item1 = Koha::Items->find( $item1->{itemnumber} ); $item1->set({homebranch => $library_A, holdingbranch => $library_A })->store; - $item1 = $item1->unblessed; #Scenario is: #One shelf holds is 'If all unavailable'/2 #Item 1 homebranch library A is available @@ -190,11 +182,11 @@ AddReturn( $item1->{barcode} ); $sth_insert_rule->execute( $hold_allowed_from_home_library ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); } @@ -203,11 +195,11 @@ AddReturn( $item1->{barcode} ); $sth_insert_rule->execute( $hold_allowed_from_any_libraries ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = IsAvailableForItemLevelRequest( $item1, $borrower1); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=PatronLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); } }; @@ -241,7 +233,7 @@ $rule = Koha::IssuingRule->new( ); $rule->store(); -$is = IsAvailableForItemLevelRequest( $item3->unblessed, $borrower1); +$is = IsAvailableForItemLevelRequest( $item3, $patron1); is( $is, 1, "Item can be held, items in transit are not available" ); # Cleanup diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 27b2701ab6..e78f74014e 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -109,7 +109,8 @@ my %data = ( ); Koha::Patron::Categories->find($category_1)->set({ enrolmentfee => 0})->store; my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber; -my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; +my $patron = Koha::Patrons->find( $borrowernumber ); +my $borrower = $patron->unblessed; my $biblionumber = $bibnum; my $barcode = $testbarcode; @@ -520,22 +521,21 @@ is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblio_with_no_item->{bibl ####### EO Bug 13113 <<< #### -$item = Koha::Items->find($itemnumber)->unblessed; +$item = Koha::Items->find($itemnumber); -ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $borrower), "Reserving a book on item level" ); +ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $patron), "Reserving a book on item level" ); my $pickup_branch = $builder->build({ source => 'Branch' })->{ branchcode }; t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '1' ); t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' ); -my ($item_object) = Koha::Biblios->find($biblionumber)->items->as_list; my $limit = Koha::Item::Transfer::Limit->new( { toBranch => $pickup_branch, - fromBranch => $item_object->holdingbranch, - itemtype => $item_object->effective_itemtype, + fromBranch => $item->holdingbranch, + itemtype => $item->effective_itemtype, } )->store(); -is( C4::Reserves::IsAvailableForItemLevelRequest($item, $borrower, $pickup_branch), 0, "Item level request not available due to transfer limit" ); +is( C4::Reserves::IsAvailableForItemLevelRequest($item, $patron, $pickup_branch), 0, "Item level request not available due to transfer limit" ); t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '0' ); # tests for MoveReserve in relation to ConfirmFutureHolds (BZ 14526) @@ -643,10 +643,10 @@ subtest '_koha_notify_reserve() tests' => sub { })->{borrowernumber}; C4::Reserves::AddReserve( - $item->{homebranch}, $hold_borrower, - $item->{biblionumber} ); + $item->homebranch, $hold_borrower, + $item->biblionumber ); - ModReserveAffect($item->{itemnumber}, $hold_borrower, 0); + ModReserveAffect($item->itemnumber, $hold_borrower, 0); my $sms_message_address = $schema->resultset('MessageQueue')->search({ letter_code => 'HOLD', message_transport_type => 'sms', -- 2.39.5