Koha-community/Koha
13
7
Fork 0
Code Issues Releases Wiki Activity

Bug 22284: Control hold groups in C4::Reserves

Browse source 
This patch modifies C4::Reserves to control when hold group options where selected
in smart rules.
In CanItemBeReserved adds 2 new error status messages
1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group
2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group
Also CheckReserves is modified when item's priority is defined, to control by hold
group when required.
Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when
required.

To test:
1) Apply this patch
2) prove t/db_dependent/Holds.t
SUCCESS => Result: PASS
3) Sign off

Sponsored-by: VOKAL
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This commit is contained in:
Agustin Moyano 2019-03-25 00:45:58 -03:00 committed by Martin Renvoize
parent 8141ebbac6
commit a998ba5714
Signed by: martin.renvoize
GPG key ID: 422B469130441A0F
2 changed files with 425 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
C4/Reserves.pm
View file

@ -314,11 +314,13 @@ sub CanBookBeReserved{
{ status => ageRestricted }, if the Item is age restricted for this borrower.
{ status => damaged }, if the Item is damaged.
{ status => cannotReserveFromOtherBranches }, if syspref 'canreservefromotherbranches' is OK.
{ status => branchNotInHoldGroup }, if borrower home library is not in hold group, and holds are only allowed from hold groups.
{ status => tooManyReserves, limit => $limit }, if the borrower has exceeded their maximum reserve amount.
{ status => notReservable }, if holds on this item are not allowed
{ status => libraryNotFound }, if given branchcode is not an existing library
{ status => libraryNotPickupLocation }, if given branchcode is not configured to be a pickup location
{ status => cannotBeTransferred }, if branch transfer limit applies on given item and branchcode
{ status => pickupNotInHoldGroup }, pickup location is not in hold group, and pickup locations are only allowed from hold groups.
=cut
@ -473,6 +475,15 @@ sub CanItemBeReserved {
return { status => 'cannotReserveFromOtherBranches' };
}
my $branch_control = C4::Context->preference('HomeOrHoldingBranch');
my $itembranchcode = $branch_control eq 'holdingbranch' ? $item->holdingbranch : $item->homebranch;
my $item_library = Koha::Libraries->find( {branchcode => $itembranchcode} );
if ( $branchitemrule->{holdallowed} == 3) {
if($borrower->{branchcode} ne $itembranchcode && !$item_library->validate_hold_sibling( {branchcode => $borrower->{branchcode}} )) {
return { status => 'branchNotInHoldGroup' };
}
}
# If reservecount is ok, we check item branch if IndependentBranches is ON
# and canreservefromotherbranches is OFF
if ( C4::Context->preference('IndependentBranches')
@ -497,6 +508,9 @@ sub CanItemBeReserved {
unless ($item->can_be_transferred({ to => $destination })) {
return { status => 'cannotBeTransferred' };
}
unless ($branchitemrule->{hold_fulfillment_policy} ne 'holdgroup' || $item_library->validate_hold_sibling( {branchcode => $pickup_branchcode} )) {
return { status => 'pickupNotInHoldGroup' };
}
}
return { status => 'OK' };
@ -810,14 +824,12 @@ sub CheckReserves {
my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$item->effective_itemtype);
next if ($branchitemrule->{'holdallowed'} == 0);
next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $patron->branchcode));
my $library = Koha::Libraries->find({branchcode=>$branch});
next if (($branchitemrule->{'holdallowed'} == 3) && (!$library->validate_hold_sibling({branchcode => $patron->branchcode}) ));
my $hold_fulfillment_policy = $branchitemrule->{hold_fulfillment_policy};
next
if $hold_fulfillment_policy ne 'any'
&& (
$hold_fulfillment_policy eq ''
|| ( $res->{branchcode} ne
$item->$hold_fulfillment_policy )
);
next if ( ($hold_fulfillment_policy eq 'holdgroup') && (!$library->validate_hold_sibling({branchcode => $res->{branchcode}})) );
next if ( ($hold_fulfillment_policy eq 'homebranch') && ($res->{branchcode} ne $item->$hold_fulfillment_policy) );
next if ( ($hold_fulfillment_policy eq 'holdingbranch') && ($res->{branchcode} ne $item->$hold_fulfillment_policy) );
next unless $item->can_be_transferred( { to => scalar Koha::Libraries->find( $res->{branchcode} ) } );
$priority = $res->{'priority'};
$highest = $res;
@ -1211,6 +1223,12 @@ sub IsAvailableForItemLevelRequest {
return 0 unless $destination;
return 0 unless $destination->pickup_location;
return 0 unless $item->can_be_transferred( { to => $destination } );
my $reserves_control_branch =
GetReservesControlBranch( $item->unblessed(), $patron->unblessed() );
my $branchitemrule =
C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype );
my $home_library = Koka::Libraries->find( {branchcode => $item->homebranch} );
return 0 unless $branchitemrule->{hold_fulfillment_policy} ne 'holdgroup' || $home_library->validate_hold_sibling( {branchcode => $pickup_branchcode} );
}
if ( $on_shelf_holds == 1 ) {
@ -1224,6 +1242,10 @@ sub IsAvailableForItemLevelRequest {
foreach my $i (@items) {
my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $patron->unblessed );
my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype );
my $branch_control = C4::Context->preference('HomeOrHoldingBranch');
my $itembranchcode = $branch_control eq 'holdingbranch' ? $item->holdingbranch : $item->homebranch;
my $item_library = Koha::Libraries->find( {branchcode => $itembranchcode} );
$any_available = 1
unless $i->itemlost
@ -1234,7 +1256,8 @@ sub IsAvailableForItemLevelRequest {
|| ( $i->damaged
&& !C4::Context->preference('AllowHoldsOnDamagedItems') )
|| Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan
|| $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch;
|| $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch
|| $branchitemrule->{holdallowed} == 3 && !$item_library->validate_hold_sibling( {branchcode => $pickup_branchcode} );
}
return $any_available ? 0 : 1;

395
t/db_dependent/Holds.t
View file

@ -7,7 +7,7 @@ use t::lib::TestBuilder;
use C4::Context;
use Test::More tests => 59;
use Test::More tests => 60;
use MARC::Record;
use C4::Biblio;
@ -24,6 +24,7 @@ use Koha::IssuingRules;
use Koha::Item::Transfer::Limits;
use Koha::Items;
use Koha::Libraries;
use Koha::Library::Groups;
use Koha::Patrons;
BEGIN {
@ -263,6 +264,7 @@ t::lib::Mocks::mock_preference('item-level_itypes', 1);
# if IndependentBranches is OFF, a $branch_1 patron can reserve an $branch_2 item
t::lib::Mocks::mock_preference('IndependentBranches', 0);
ok(
CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'OK',
'$branch_1 patron allowed to reserve $branch_2 item with IndependentBranches OFF (bug 2394)'
@ -687,3 +689,394 @@ subtest 'CanItemBeReserved / holds_per_day tests' => sub {
$schema->storage->txn_rollback;
};
subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub {
plan tests => 9;
$schema->storage->txn_begin;
# Cleanup database
Koha::Holds->search->delete;
$dbh->do('DELETE FROM issues');
$dbh->do('DELETE FROM issuingrules');
$dbh->do(
q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed)
VALUES (?, ?, ?, ?)},
{},
'*', '*', '*', 25
);
$dbh->do('DELETE FROM branch_item_rules');
$dbh->do('DELETE FROM default_branch_circ_rules');
$dbh->do('DELETE FROM default_branch_item_rules');
$dbh->do('DELETE FROM default_circ_rules');
Koha::Items->search->delete;
Koha::Biblios->search->delete;
# Create item types
my $itemtype1 = $builder->build_object( { class => 'Koha::ItemTypes' } );
my $itemtype2 = $builder->build_object( { class => 'Koha::ItemTypes' } );
# Create libraries
my $library1 = $builder->build_object( { class => 'Koha::Libraries' } );
my $library2 = $builder->build_object( { class => 'Koha::Libraries' } );
my $library3 = $builder->build_object( { class => 'Koha::Libraries' } );
# Create library groups hierarchy
my $rootgroup = $builder->build_object( { class => 'Koha::Library::Groups', value => {ft_local_hold_group => 1} } );
my $group1 = $builder->build_object( { class => 'Koha::Library::Groups', value => {parent_id => $rootgroup->id, branchcode => $library1->branchcode}} );
my $group2 = $builder->build_object( { class => 'Koha::Library::Groups', value => {parent_id => $rootgroup->id, branchcode => $library2->branchcode} } );
# Create 2 patrons
my $patron1 = $builder->build_object( { class => 'Koha::Patrons', value => {branchcode => $library1->branchcode} } );
my $patron3 = $builder->build_object( { class => 'Koha::Patrons', value => {branchcode => $library3->branchcode} } );
# Create 3 biblios with items
my $biblio_1 = $builder->build_sample_biblio({ itemtype => $itemtype1->itemtype });
my ( undef, undef, $itemnumber_1 ) = AddItem(
{ homebranch => $library1->branchcode,
holdingbranch => $library1->branchcode
},
$biblio_1->biblionumber
);
my $biblio_2 = $builder->build_sample_biblio({ itemtype => $itemtype2->itemtype });
my ( undef, undef, $itemnumber_2 ) = AddItem(
{ homebranch => $library2->branchcode,
holdingbranch => $library2->branchcode
},
$biblio_2->biblionumber
);
my $biblio_3 = $builder->build_sample_biblio({ itemtype => $itemtype1->itemtype });
my ( undef, undef, $itemnumber_3 ) = AddItem(
{ homebranch => $library1->branchcode,
holdingbranch => $library1->branchcode
},
$biblio_3->biblionumber
);
# Test 1: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'OK' },
'Patron can place hold if no circ_rules where defined'
);
# Insert default circ rule of holds allowed only from local hold group for all libraries
$dbh->do(
q{INSERT INTO default_circ_rules (holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?)},
{},
3, 'any', 'any'
);
# Test 2: Patron 1 can place hold
is_deeply(
CanItemBeReserved( $patron1->borrowernumber, $itemnumber_2 ),
{ status => 'OK' },
'Patron can place hold because patron\'s home library is part of hold group'
);
# Test 3: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'branchNotInHoldGroup' },
'Patron cannot place hold because patron\'s home library is not part of hold group'
);
# Cleanup default_cirt_rules
$dbh->do('DELETE FROM default_circ_rules');
# Insert default circ rule to "any" for library 2
$dbh->do(
q{INSERT INTO default_branch_circ_rules (branchcode, holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?,?)},
{},
$library2->branchcode, 2, 'any', 'any'
);
# Test 4: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'OK' },
'Patron can place hold if holdallowed is set to "any" for library 2'
);
# Update default circ rule to "hold group" for library 2
$dbh->do(
q{UPDATE default_branch_circ_rules set holdallowed = ?
WHERE branchcode = ?},
{},
3, $library2->branchcode
);
# Test 5: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'branchNotInHoldGroup' },
'Patron cannot place hold if holdallowed is set to "hold group" for library 2'
);
# Cleanup default_branch_cirt_rules
$dbh->do('DELETE FROM default_branch_circ_rules');
# Insert default item rule to "any" for itemtype 2
$dbh->do(
q{INSERT INTO default_branch_item_rules (itemtype, holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?,?)},
{},
$itemtype2->itemtype, 2, 'any', 'any'
);
# Test 6: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'OK' },
'Patron can place hold if holdallowed is set to "any" for itemtype 2'
);
# Update default item rule to "hold group" for itemtype 2
$dbh->do(
q{UPDATE default_branch_item_rules set holdallowed = ?
WHERE itemtype = ?},
{},
3, $itemtype2->itemtype
);
# Test 7: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'branchNotInHoldGroup' },
'Patron cannot place hold if holdallowed is set to "hold group" for itemtype 2'
);
# Cleanup default_branch_item_rules
$dbh->do('DELETE FROM default_branch_item_rules');
# Insert branch item rule to "any" for itemtype 2 and library 2
$dbh->do(
q{INSERT INTO branch_item_rules (branchcode, itemtype, holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?,?,?)},
{},
$library2->branchcode, $itemtype2->itemtype, 2, 'any', 'any'
);
# Test 8: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'OK' },
'Patron can place hold if holdallowed is set to "any" for itemtype 2 and library 2'
);
# Update branch item rule to "hold group" for itemtype 2 and library 2
$dbh->do(
q{UPDATE branch_item_rules set holdallowed = ?
WHERE branchcode = ? and itemtype = ?},
{},
3, $library2->branchcode, $itemtype2->itemtype
);
# Test 9: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2 ),
{ status => 'branchNotInHoldGroup' },
'Patron cannot place hold if holdallowed is set to "hold group" for itemtype 2 and library 2'
);
$schema->storage->txn_rollback;
};
subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub {
plan tests => 9;
$schema->storage->txn_begin;
# Cleanup database
Koha::Holds->search->delete;
$dbh->do('DELETE FROM issues');
$dbh->do('DELETE FROM issuingrules');
$dbh->do(
q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed)
VALUES (?, ?, ?, ?)},
{},
'*', '*', '*', 25
);
$dbh->do('DELETE FROM branch_item_rules');
$dbh->do('DELETE FROM default_branch_circ_rules');
$dbh->do('DELETE FROM default_branch_item_rules');
$dbh->do('DELETE FROM default_circ_rules');
Koha::Items->search->delete;
Koha::Biblios->search->delete;
# Create item types
my $itemtype1 = $builder->build_object( { class => 'Koha::ItemTypes' } );
my $itemtype2 = $builder->build_object( { class => 'Koha::ItemTypes' } );
# Create libraries
my $library1 = $builder->build_object( { class => 'Koha::Libraries' } );
my $library2 = $builder->build_object( { class => 'Koha::Libraries' } );
my $library3 = $builder->build_object( { class => 'Koha::Libraries' } );
# Create library groups hierarchy
my $rootgroup = $builder->build_object( { class => 'Koha::Library::Groups', value => {ft_local_hold_group => 1} } );
my $group1 = $builder->build_object( { class => 'Koha::Library::Groups', value => {parent_id => $rootgroup->id, branchcode => $library1->branchcode}} );
my $group2 = $builder->build_object( { class => 'Koha::Library::Groups', value => {parent_id => $rootgroup->id, branchcode => $library2->branchcode} } );
# Create 2 patrons
my $patron1 = $builder->build_object( { class => 'Koha::Patrons', value => {branchcode => $library1->branchcode} } );
my $patron3 = $builder->build_object( { class => 'Koha::Patrons', value => {branchcode => $library3->branchcode} } );
# Create 3 biblios with items
my $biblio_1 = $builder->build_sample_biblio({ itemtype => $itemtype1->itemtype });
my ( undef, undef, $itemnumber_1 ) = AddItem(
{ homebranch => $library1->branchcode,
holdingbranch => $library1->branchcode
},
$biblio_1->biblionumber
);
my $biblio_2 = $builder->build_sample_biblio({ itemtype => $itemtype2->itemtype });
my ( undef, undef, $itemnumber_2 ) = AddItem(
{ homebranch => $library2->branchcode,
holdingbranch => $library2->branchcode
},
$biblio_2->biblionumber
);
my $biblio_3 = $builder->build_sample_biblio({ itemtype => $itemtype1->itemtype });
my ( undef, undef, $itemnumber_3 ) = AddItem(
{ homebranch => $library1->branchcode,
holdingbranch => $library1->branchcode
},
$biblio_3->biblionumber
);
# Test 1: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'OK' },
'Patron can place hold if no circ_rules where defined'
);
# Insert default circ rule of holds allowed only from local hold group for all libraries
$dbh->do(
q{INSERT INTO default_circ_rules (holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?)},
{},
2, 'holdgroup', 'any'
);
# Test 2: Patron 1 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library1->branchcode ),
{ status => 'OK' },
'Patron can place hold because pickup location is part of hold group'
);
# Test 3: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'pickupNotInHoldGroup' },
'Patron cannot place hold because pickup location is not part of hold group'
);
# Cleanup default_cirt_rules
$dbh->do('DELETE FROM default_circ_rules');
# Insert default circ rule to "any" for library 2
$dbh->do(
q{INSERT INTO default_branch_circ_rules (branchcode, holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?,?)},
{},
$library2->branchcode, 2, 'any', 'any'
);
# Test 4: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'OK' },
'Patron can place hold if default_branch_circ_rules is set to "any" for library 2'
);
# Update default circ rule to "hold group" for library 2
$dbh->do(
q{UPDATE default_branch_circ_rules set hold_fulfillment_policy = ?
WHERE branchcode = ?},
{},
'holdgroup', $library2->branchcode
);
# Test 5: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'pickupNotInHoldGroup' },
'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for library 2'
);
# Cleanup default_branch_cirt_rules
$dbh->do('DELETE FROM default_branch_circ_rules');
# Insert default item rule to "any" for itemtype 2
$dbh->do(
q{INSERT INTO default_branch_item_rules (itemtype, holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?,?)},
{},
$itemtype2->itemtype, 2, 'any', 'any'
);
# Test 6: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'OK' },
'Patron can place hold if hold_fulfillment_policy is set to "any" for itemtype 2'
);
# Update default item rule to "hold group" for itemtype 2
$dbh->do(
q{UPDATE default_branch_item_rules set hold_fulfillment_policy = ?
WHERE itemtype = ?},
{},
'holdgroup', $itemtype2->itemtype
);
# Test 7: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'pickupNotInHoldGroup' },
'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2'
);
# Cleanup default_branch_item_rules
$dbh->do('DELETE FROM default_branch_item_rules');
# Insert branch item rule to "any" for itemtype 2 and library 2
$dbh->do(
q{INSERT INTO branch_item_rules (branchcode, itemtype, holdallowed, hold_fulfillment_policy, returnbranch)
VALUES (?,?,?,?,?)},
{},
$library2->branchcode, $itemtype2->itemtype, 2, 'any', 'any'
);
# Test 8: Patron 3 can place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'OK' },
'Patron can place hold if hold_fulfillment_policy is set to "any" for itemtype 2 and library 2'
);
# Update branch item rule to "hold group" for itemtype 2 and library 2
$dbh->do(
q{UPDATE branch_item_rules set hold_fulfillment_policy = ?
WHERE branchcode = ? and itemtype = ?},
{},
'holdgroup', $library2->branchcode, $itemtype2->itemtype
);
# Test 9: Patron 3 cannot place hold
is_deeply(
CanItemBeReserved( $patron3->borrowernumber, $itemnumber_2, $library3->branchcode ),
{ status => 'pickupNotInHoldGroup' },
'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2'
);
$schema->storage->txn_rollback;
};