From 4146ce863b851ab2cdc5b477ce4e81c2b627f050 Mon Sep 17 00:00:00 2001 From: Agustin Moyano Date: Mon, 25 Mar 2019 00:45:58 -0300 Subject: [PATCH] Bug 22284: Control hold groups in C4::Reserves 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 --- C4/Reserves.pm | 33 +++- t/db_dependent/Holds.t | 397 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 427 insertions(+), 3 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 5fc68f1359..a4f18758bb 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -298,11 +298,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 @@ -457,6 +459,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') @@ -481,6 +492,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' }; @@ -797,8 +811,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 ( ($branchitemrule->{hold_fulfillment_policy} ne 'any') && ($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; @@ -1191,6 +1209,12 @@ sub IsAvailableForItemLevelRequest { return 0 unless $destination; return 0 unless $destination->pickup_location; return 0 unless $item_object->can_be_transferred( { to => $destination } ); + my $reserves_control_branch = + GetReservesControlBranch( $item_object->unblessed(), $borrower ); + my $branchitemrule = + C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item_object->itype ); + my $home_library = Koka::Libraries->find( {branchcode => $item_object->homebranch} ); + return 0 unless $branchitemrule->{hold_fulfillment_policy} ne 'holdgroup' || $home_library->validate_hold_sibling( {branchcode => $pickup_branchcode} ); } if ( $on_shelf_holds == 1 ) { @@ -1204,6 +1228,10 @@ sub IsAvailableForItemLevelRequest { foreach my $i (@items) { my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $borrower ); 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 @@ -1214,7 +1242,8 @@ 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 && $borrower->{branchcode} ne $i->homebranch + || $branchitemrule->{holdallowed} == 3 && !$item_library->validate_hold_sibling( {branchcode => $pickup_branchcode} ); } return $any_available ? 0 : 1; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 1515b4bef4..e0ff970216 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -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,8 +24,11 @@ use Koha::IssuingRules; use Koha::Item::Transfer::Limits; use Koha::Items; use Koha::Libraries; +use Koha::Library::Groups; use Koha::Patrons; +use Data::Dumper; + BEGIN { use FindBin; use lib $FindBin::Bin; @@ -262,6 +265,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)' @@ -675,3 +679,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; +}; -- 2.11.0