From 5e4f5f10298ad4a937b39cbbce0a2c6520a35c0d 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 --- 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 | 73 ++++++++-------------- t/db_dependent/Reserves.t | 20 +++--- 7 files changed, 60 insertions(+), 87 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 53b74bb7cc..810f92d700 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2662,16 +2662,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 56a2f18114..a39f8065ad 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 e2633753c8..cb15fd95ae 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1162,49 +1162,45 @@ 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 = $dbh->selectrow_array("SELECT notforloan FROM itemtypes WHERE itemtype = ?", undef, $itemtype); 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 $circ_control_branch = C4::Circulation::_GetCircControlBranch( $i->unblessed(), $borrower ); + my $circ_control_branch = C4::Circulation::_GetCircControlBranch( $i->unblessed(), $patron->unblessed ); my $branchitemrule = C4::Circulation::GetBranchItemRule( $circ_control_branch, $i->itype ); $any_available = 1 @@ -1216,12 +1212,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 725b31d07b..7291adb4d4 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -542,7 +542,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 486c770c75..312c8e612a 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -481,7 +481,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 255c757bf6..347506ae99 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -35,16 +35,17 @@ t::lib::Mocks::mock_userenv({ branchcode => $library1->{branchcode} }); my $bib_title = "Test Title"; -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', @@ -53,8 +54,6 @@ my $borrower2 = $builder->build({ # Test hold_fulfillment_policy my ( $itemtype ) = @{ $dbh->selectrow_arrayref("SELECT itemtype FROM itemtypes LIMIT 1") }; -my $borrowernumber1 = $borrower1->{borrowernumber}; -my $borrowernumber2 = $borrower2->{borrowernumber}; my $library_A = $library1->{branchcode}; my $library_B = $library2->{branchcode}; @@ -69,27 +68,9 @@ my $biblioitemnumber = $dbh->selectrow_array("SELECT biblioitemnumber FROM biblioitems WHERE biblionumber = $biblionumber") or BAIL_OUT("Cannot find newly created biblioitems record"); -$dbh->do(" - INSERT INTO items (barcode, biblionumber, biblioitemnumber, homebranch, holdingbranch, notforloan, damaged, itemlost, withdrawn, onloan, itype) - VALUES ('AllowHoldIf1', $biblionumber, $biblioitemnumber, '$library_A', '$library_A', 0, 0, 0, 0, NULL, '$itemtype') -"); +my $item1 = $builder->build_object({ class => 'Koha::Items', value => { barcode => 'AllowHoldIf1', biblionumber => $biblionumber, biblioitemnumber => $biblioitemnumber, homebranch => $library_A, holdingbranch => $library_A, notforloan => 0, damaged => 0, itemlost => 0, withdrawn => 0, onloan => undef, itype => $itemtype } }); -my $itemnumber1 = - $dbh->selectrow_array("SELECT itemnumber FROM items WHERE biblionumber = $biblionumber") - or BAIL_OUT("Cannot find newly created item"); - -my $item1 = Koha::Items->find( $itemnumber1 )->unblessed; - -$dbh->do(" - INSERT INTO items (barcode, biblionumber, biblioitemnumber, homebranch, holdingbranch, notforloan, damaged, itemlost, withdrawn, onloan, itype) - VALUES ('AllowHoldIf2', $biblionumber, $biblioitemnumber, '$library_A', '$library_A', 0, 0, 0, 0, NULL, '$itemtype') -"); - -my $itemnumber2 = - $dbh->selectrow_array("SELECT itemnumber FROM items WHERE biblionumber = $biblionumber ORDER BY itemnumber DESC") - or BAIL_OUT("Cannot find newly created item"); - -my $item2 = Koha::Items->find( $itemnumber2 )->unblessed; +my $item2 = $builder->build_object({ class => 'Koha::Items', value => { barcode => 'AllowHoldIf2', biblionumber => $biblionumber, biblioitemnumber => $biblioitemnumber, homebranch => $library_A, holdingbranch => $library_A, notforloan => 0, damaged => 0, itemlost => 0, withdrawn => 0, onloan => undef, itype => $itemtype } }); $dbh->do("DELETE FROM issuingrules"); my $rule = Koha::IssuingRule->new( @@ -105,20 +86,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 { @@ -132,9 +113,7 @@ AddReturn( $item1->{barcode} ); subtest 'Item is available at a different library' => sub { plan tests => 4; - $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 @@ -148,11 +127,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, 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" ); 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" ); } @@ -161,11 +140,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" ); } }; @@ -173,9 +152,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 @@ -189,11 +166,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" ); } @@ -202,11 +179,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" ); } }; @@ -217,8 +194,8 @@ my $biblio = $builder->build({ source => 'Biblio', }); -my $item3 = $builder->build({ - source => 'Item', +my $item3 = $builder->build_object({ + class => 'Koha::Items', value => { biblionumber => $biblio->{biblionumber}, itemlost => 0, @@ -232,7 +209,7 @@ my $item3 = $builder->build({ my $hold = $builder->build({ source => 'Reserve', value =>{ - itemnumber => $item3->{itemnumber}, + itemnumber => $item3->itemnumber, found => 'T' } }); @@ -251,7 +228,7 @@ $rule = Koha::IssuingRule->new( ); $rule->store(); -$is = IsAvailableForItemLevelRequest( $item3, $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 214f42250d..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; 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.11.0