gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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 <hayleymapley@catalyst.net.nz>

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This commit is contained in:
Jonathan Druart 2017-09-12 15:26:27 -03:00 committed by Nick Clemens
parent f550977f73
commit 917a506ffc
7 changed files with 67 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
C4/Circulation.pm
View File

@ -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);
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, $i;
push @reservable, $itemnumber;
if (@reservable >= @borrowernumbers) {
$resfound = 0;
last ITEM;

2
C4/ILSDI/Services.pm
View File

@ -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';
}

28
C4/Reserves.pm
View File

@ -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 );
}
}

2
opac/opac-reserve.pl
View File

@ -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) {

2
reserve/request.pl
View File

@ -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'
)
{

74
t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
View File

@ -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 $patron2 = $builder->build_object({
class => 'Koha::Patrons',
value => {
branchcode => $library1->{branchcode},
dateexpiry => '3000-01-01',
}
});
my $borrower2 = $builder->build({
source => 'Borrower',
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

20
t/db_dependent/Reserves.t
View File

@ -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',