From be7e5c7fcec33e99eb6174fb8be9f36418283b5e 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 | 11 +++--- C4/Reserves.pm | 21 +++++------ opac/opac-reserve.pl | 2 +- reserve/request.pl | 2 +- .../Holds/DisallowHoldIfItemsAvailable.t | 43 ++++++---------------- t/db_dependent/Reserves.t | 7 ++-- 7 files changed, 43 insertions(+), 63 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 8bd8d85946..4339a9fb69 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2647,16 +2647,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 = GetItem($i); - 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 a0aec9987d..f341a593af 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -38,6 +38,7 @@ use Koha::DateUtils; use Koha::Biblios; use Koha::Checkouts; +use Koha::Items; use Koha::Libraries; use Koha::Patrons; @@ -514,17 +515,17 @@ sub GetServices { my $borrower = $patron->unblessed; # Get the item, or return an error code if not found my $itemnumber = $cgi->param('item_id'); - my $item = GetItem( $itemnumber ); - return { code => 'RecordNotFound' } unless $$item{itemnumber}; + my $item = Koha::Items->find( $itemnumber ); + return { code => 'RecordNotFound' } unless $item; my @availablefor; # Reserve level management - my $biblionumber = $item->{'biblionumber'}; + my $biblionumber = $item->biblionumber; my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber ); if ($canbookbereserved eq 'OK') { push @availablefor, 'title level hold'; - my $canitembereserved = IsAvailableForItemLevelRequest($item, $borrower); + my $canitembereserved = IsAvailableForItemLevelRequest($item, $patron); if ($canitembereserved) { push @availablefor, 'item level hold'; } @@ -547,7 +548,7 @@ sub GetServices { } # Issuing management - my $barcode = $item->{'barcode'} || ''; + my $barcode = $item->barcode || ''; $barcode = barcodedecode($barcode) if ( $barcode && C4::Context->preference('itemBarcodeInputFilter') ); if ($barcode) { my ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $borrower, $barcode ); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 51be4ef8db..74e5125721 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1127,35 +1127,32 @@ and canreservefromotherbranches. =cut sub IsAvailableForItemLevelRequest { - my $item = shift; - my $borrower = shift; + my ( $item, $patron ) = @_; 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 ( $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; @@ -1174,7 +1171,7 @@ sub IsAvailableForItemLevelRequest { return $any_available ? 0 : 1; } - return $item->{onloan} || GetReserveStatus($item->{itemnumber}) eq "Waiting"; + return $item->onloan || GetReserveStatus( $item->itemnumber ) eq "Waiting"; } sub _get_itype { diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index dcaec358af..bfd1dc655d 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -536,7 +536,7 @@ foreach my $biblioNum (@biblionumbers) { my $policy_holdallowed = !$itemLoopIter->{already_reserved}; $policy_holdallowed &&= - IsAvailableForItemLevelRequest($itemInfo,$patron_unblessed) && + IsAvailableForItemLevelRequest($item, $patron) && CanItemBeReserved($borrowernumber,$itemNum) eq 'OK'; if ($policy_holdallowed) { diff --git a/reserve/request.pl b/reserve/request.pl index 9cb9c06b6f..ddd66e85ba 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -474,7 +474,7 @@ foreach my $biblionumber (@biblionumbers) { if ( !$item->{cantreserve} && !$exceeded_maxreserves - && IsAvailableForItemLevelRequest($item, $patron_unblessed) + && IsAvailableForItemLevelRequest($item_object, $patron) && $can_item_be_reserved eq 'OK' ) { diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index ec76403286..edace55114 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -6,6 +6,7 @@ use C4::Context; use C4::Items; use C4::Circulation; use Koha::IssuingRule; +use Koha::Items; use Test::More tests => 4; @@ -31,16 +32,16 @@ C4::Context->set_userenv(0,0,0,'firstname','surname', $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 $borrower2 = $builder->build({ - source => 'Borrower', +my $patron2 = $builder->build_object({ + class => 'Koha::Patrons', value => { branchcode => $library1->{branchcode}, dateexpiry => '3000-01-01', @@ -49,8 +50,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}; $dbh->do("INSERT INTO biblio (frameworkcode, author, title, datecreated) VALUES ('', 'Koha test', '$bib_title', '2011-02-01')"); @@ -64,27 +63,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 = GetItem( $itemnumber1 ); - -$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 = GetItem( $itemnumber2 ); +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( @@ -101,17 +82,17 @@ 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" ); -AddIssue( $borrower2, $item1->{barcode} ); +AddIssue( $patron2, $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, $item2->barcode ); -$is = IsAvailableForItemLevelRequest( $item1, $borrower1); +$is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 1, "Item can be held, no items available" ); # Cleanup diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 4db6b8a63b..cabc5a0065 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -118,7 +118,8 @@ my %data = ( ); Koha::Patron::Categories->find($category_1)->set({ enrolmentfee => 0})->store; my $borrowernumber = AddMember(%data); -my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; +my $patron = Koha::Patrons->find( $borrowernumber ); +my $borrower = $patron->unblessed; my $biblionumber = $bibnum; my $barcode = $testbarcode; @@ -536,9 +537,9 @@ is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Res ####### EO Bug 13113 <<< #### -$item = GetItem($itemnumber); +$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" ); # tests for MoveReserve in relation to ConfirmFutureHolds (BZ 14526) # hold from A pos 1, today, no fut holds: MoveReserve should fill it -- 2.11.0