From d67bcdc4c4eee746522302af526b0012be79bf3c Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 21 Jun 2016 09:52:47 -0400 Subject: [PATCH] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 7 - Try placing a multihold with either record above and a holdable record, message should end "...cannot place holds on some of these titles' items" 8 - prove -v t/db_dependent/Holds.t --- C4/Reserves.pm | 9 +- .../prog/en/modules/reserve/request.tt | 12 +- reserve/request.pl | 9 +- t/db_dependent/Holds.t | 210 +++++++++++++++++---- 4 files changed, 195 insertions(+), 45 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 503771e914..6517ccae9e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -395,6 +395,10 @@ sub CanItemBeReserved { found => undef, # Found holds don't count against a patron's holds limit } ); + + if ( $holds_per_record == 0 ) { + return { status => "noReservesAllowed" }; + } if ( $holds->count() >= $holds_per_record ) { return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; } @@ -438,6 +442,9 @@ sub CanItemBeReserved { } # we check if it's ok or not + if( $allowedreserves == 0 ){ + return { status => 'noReservesAllowed' }; + } if ( $reservecount >= $allowedreserves ) { return { status => 'tooManyReserves', limit => $allowedreserves }; } @@ -463,7 +470,7 @@ sub CanItemBeReserved { my $reserves_control_branch = GetReservesControlBranch( $item->unblessed(), $borrower ); my $branchitemrule = - C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype ); # FIXME Should not be item->effective_itemtype? + C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); if ( $branchitemrule->{holdallowed} == 0 ) { return { status => 'notReservable' }; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt index 8c34184557..2a76bb9ce0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -309,13 +309,15 @@ [% END %] - [% IF ( exceeded_maxreserves || exceeded_holds_per_record || alreadyreserved || none_available || alreadypossession || ageRestricted ) %] + [% IF ( no_reserves_allowed || exceeded_maxreserves || exceeded_holds_per_record || alreadyreserved || none_available || alreadypossession || ageRestricted ) %]
[% UNLESS ( multi_hold ) %]

Cannot place hold

[% ELSE # UNLESS multi_hold %]

Cannot place hold on some items

- [% IF ( exceeded_maxreserves ) %] + [% IF (no_reserves_allowed ) %] +
  • No holds allowed: [% patron.firstname | html %] [% patron.surname | html %] cannot place holds on some of these title's items.
  • + [% ELSIF ( exceeded_maxreserves ) %]
  • Too many holds: [% patron.firstname | html %] [% patron.surname | html %] can place [% new_reserves_allowed | html %] of the requested [% new_reserves_count | html %] holds for a maximum of [% maxreserves | html %] total holds.
  • [% ELSIF ( exceeded_holds_per_record ) %] [% FOREACH biblioloo IN biblioloop %] @@ -587,6 +591,8 @@ Patron already has hold for this item [% ELSIF itemloo.not_holdable == 'cannotBeTransferred' %] Cannot be transferred to pickup library + [% ELSIF itemloo.not_holdable == 'noReservesAllowed' %] + No reserves are allowed on this item [% ELSE %] [% itemloo.not_holdable | html %] [% END %] diff --git a/reserve/request.pl b/reserve/request.pl index c7376c2857..c88602bcae 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -186,6 +186,8 @@ if ($borrowernumber_hold && !$action) { my $new_reserves_count = scalar( @biblionumbers ); my $maxreserves = C4::Context->preference('maxreserves'); + $template->param( maxreserves => $maxreserves ); + if ( $maxreserves && ( $reserves_count + $new_reserves_count > $maxreserves ) ) { @@ -288,6 +290,7 @@ if ($patron) { my $itemdata_enumchron = 0; my $itemdata_ccode = 0; my @biblioloop = (); +my $no_reserves_allowed = 0; foreach my $biblionumber (@biblionumbers) { next unless $biblionumber =~ m|^\d+$|; @@ -303,7 +306,10 @@ foreach my $biblionumber (@biblionumbers) { #All is OK and we can continue } - elsif ( $canReserve->{status} eq 'tooManyReserves' ) { + elsif ( $canReserve eq 'noReservesAllowed') { + $no_reserves_allowed = 1; + } + elsif ( $canReserve eq 'tooManyReserves' ) { $exceeded_maxreserves = 1; $template->param( maxreserves => $canReserve->{limit} ); } @@ -732,6 +738,7 @@ foreach my $biblionumber (@biblionumbers) { $template->param( biblioloop => \@biblioloop ); $template->param( biblionumbers => $biblionumbers ); +$template->param( no_reserves_allowed => $no_reserves_allowed ); $template->param( exceeded_maxreserves => $exceeded_maxreserves ); $template->param( exceeded_holds_per_record => $exceeded_holds_per_record ); $template->param( subscriptionsnumber => CountSubscriptionFromBiblionumber($biblionumber)); diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 2ecbdb4d9a..bfaa3795d1 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 => 61; +use Test::More tests => 59; use MARC::Record; use C4::Biblio; @@ -244,19 +244,12 @@ is( $hold->priority, '6', "Test AlterPriority(), move to bottom" ); my $foreign_biblio = $builder->build_sample_biblio({ itemtype => 'DUMMY' }); my ($foreign_item_bibnum, $foreign_item_bibitemnum, $foreign_itemnumber) = AddItem({ homebranch => $branch_2, holdingbranch => $branch_2 } , $foreign_biblio->biblionumber); -# Cleanup circulation rules -$dbh->do('DELETE FROM circulation_rules'); -# $dbh->do( -# q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) -# VALUES (?, ?, ?, ?, ?)}, -# {}, -# '*', '*', '*', 25, 99 -# ); +delete_all_rules(); $dbh->do( q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {}, - '*', '*', 'CANNOT', 0, 99 + '*', '*', '*', 25, 99 ); # make sure some basic sysprefs are set @@ -324,38 +317,169 @@ t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 0 ); ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'damaged', "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled" ); -# Regression test for bug 9532 -$biblio = $builder->build_sample_biblio({ itemtype => 'CANNOT' }); -($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CANNOT' } , $biblio->biblionumber); -AddReserve( - $branch_1, - $borrowernumbers[0], - $biblio->biblionumber, - '', - 1, -); -is( - CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves', - "cannot request item if policy that matches on item-level item type forbids it" -); -ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber); -ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'OK', - "can request item if policy that matches on item type allows it" -); +subtest 'CanItemBeReserved' => sub { + plan tests => 2; -t::lib::Mocks::mock_preference('item-level_itypes', 0); -ModItem({ itype => undef }, $item_bibnum, $itemnumber); -ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'tooManyReserves', - "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)" -); + delete_all_rules(); + + my $itemtype_can = $builder->build({source => "Itemtype"})->{itemtype}; + my $itemtype_cant = $builder->build({source => "Itemtype"})->{itemtype}; + my $itemtype_cant_record = $builder->build({source => "Itemtype"})->{itemtype}; + + $dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {}, + '*', '*', $itemtype_cant, 0, 99 + ); + $dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {}, + '*', '*', $itemtype_can, 2, 2 + ); + $dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {}, + '*', '*', $itemtype_cant_record, 0, 0 + ); + Koha::CirculationRules->set_rules( + { + branchcode => $branch_1, + itemtype => $itemtype_cant, + categorycode => undef, + rules => { + holdallowed => 0, + returnbranch => 'homebranch', + } + } + ); + Koha::CirculationRules->set_rules( + { + branchcode => $branch_1, + itemtype => $itemtype_can, + categorycode => undef, + rules => { + holdallowed => 1, + returnbranch => 'homebranch', + } + } + ); + + subtest 'noReservesAllowed' => sub { + plan tests => 5; + + my $biblionumber_cannot = $builder->build_sample_biblio({ itemtype => $itemtype_cant })->biblionumber; + my $biblionumber_can = $builder->build_sample_biblio({ itemtype => $itemtype_can })->biblionumber; + my $biblionumber_record_cannot = $builder->build_sample_biblio({ itemtype => $itemtype_cant_record })->biblionumber; + my ( undef, undef, $itemnumber_1_can ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_cannot); + my ( undef, undef, $itemnumber_1_cannot ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant } , $biblionumber_cannot); + + my ( undef, undef, $itemnumber_2_can ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_can); + my ( undef, undef, $itemnumber_2_cannot ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant } , $biblionumber_can); + + my ( undef, undef, $itemnumber_3_cannot ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant_record } , $biblionumber_record_cannot); + + Koha::Holds->search({borrowernumber => $borrowernumbers[0]})->delete; + + t::lib::Mocks::mock_preference('item-level_itypes', 1); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_2_cannot)->{status}, 'noReservesAllowed', + "With item level set, rule from item must be picked (CANNOT)" + ); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_1_can)->{status}, 'OK', + "With item level set, rule from item must be picked (CAN)" + ); + t::lib::Mocks::mock_preference('item-level_itypes', 0); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_1_can)->{status}, 'noReservesAllowed', + "With biblio level set, rule from biblio must be picked (CANNOT)" + ); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_2_cannot)->{status}, 'OK', + "With biblio level set, rule from biblio must be picked (CAN)" + ); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_3_cannot)->{status}, 'noReservesAllowed', + "When no holds allowed and no holds per record allowed should return noReservesAllowed" + ); + }; + + subtest 'tooManyHoldsForThisRecord + tooManyReserves + itemAlreadyOnHold' => sub { + plan tests => 7; + + my $biblionumber_1 = $builder->build_sample_biblio({ itemtype => $itemtype_can })->biblionumber; + my ( undef, undef, $itemnumber_11) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_1); + my ( undef, undef, $itemnumber_12) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_1); + my $biblionumber_2 = $builder->build_sample_biblio({ itemtype => $itemtype_can })->biblionumber; + my ( undef, undef, $itemnumber_21) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_2); + my ( undef, undef, $itemnumber_22) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_2); + + Koha::Holds->search({borrowernumber => $borrowernumbers[0]})->delete; + + # Biblio-level hold + AddReserve( + $branch_1, + $borrowernumbers[0], + $biblionumber_1, + '', + 1, + ); + for my $item_level ( 0..1 ) { + t::lib::Mocks::mock_preference('item-level_itypes', $item_level); + is( + # FIXME This is not really correct, but CanItemBeReserved does not check if biblio-level holds already exist + CanItemBeReserved( $borrowernumbers[0], $itemnumber_11)->{status}, 'OK', + "A biblio-level hold already exists - another hold can be placed on a specific item item" + ); + } + + Koha::Holds->search({borrowernumber => $borrowernumbers[0]})->delete; + # Item-level hold + AddReserve( + $branch_1, + $borrowernumbers[0], + $biblionumber_1, + '', + 1, + undef, undef, undef, undef, $itemnumber_11 + ); + $dbh->do(q{UPDATE issuingrules set reservesallowed=5, holds_per_record=1}); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_12)->{status}, 'tooManyHoldsForThisRecord', + "A item-level hold already exists and holds_per_record=1, another hold cannot be placed on this record" + ); + $dbh->do(q{UPDATE issuingrules set reservesallowed=1, holds_per_record=1}); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_12)->{status}, 'tooManyHoldsForThisRecord', + "A item-level hold already exists and holds_per_record=1 - tooManyHoldsForThisRecord has priority over tooManyReserves" + ); + $dbh->do(q{UPDATE issuingrules set reservesallowed=5, holds_per_record=2}); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_12)->{status}, 'OK', + "A item-level hold already exists but holds_per_record=2- another item-level hold can be placed on this record" + ); + + AddReserve( + $branch_1, + $borrowernumbers[0], + $biblionumber_2, + '', + 1, + undef, undef, undef, undef, $itemnumber_21 + ); + $dbh->do(q{UPDATE issuingrules set reservesallowed=2, holds_per_record=2}); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_21)->{status}, 'itemAlreadyOnHold', + "A item-level holds already exists on this item, itemAlreadyOnHold should be raised" + ); + is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber_22)->{status}, 'tooManyReserves', + "This patron has already placed reservesallowed holds, tooManyReserves should be raised" + ); + }; +}; # Test branch item rules -$dbh->do('DELETE FROM issuingrules'); -$dbh->do('DELETE FROM circulation_rules'); +delete_all_rules(); $dbh->do( q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) VALUES (?, ?, ?, ?)}, @@ -701,7 +825,7 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { # Cleanup database Koha::Holds->search->delete; $dbh->do('DELETE FROM issues'); - $dbh->do('DELETE FROM issuingrules'); + delete_all_rules(); $dbh->do( q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) VALUES (?, ?, ?, ?)}, @@ -789,7 +913,7 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { 'Patron cannot place hold because patron\'s home library is not part of hold group' ); - # Cleanup default_cirt_rules + # Cleanup default_circ_rules $dbh->do('DELETE FROM circulation_rules'); # Insert default circ rule to "any" for library 2 @@ -845,6 +969,7 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { categorycode => undef, rules => { holdallowed => 2, + holdsperrecord => 1, hold_fulfillment_policy => 'any', returnbranch => 'any' } @@ -1114,7 +1239,6 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { '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 circulation_rules'); # Insert branch item rule to "any" for itemtype 2 and library 2 @@ -1161,3 +1285,9 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { $schema->storage->txn_rollback; }; + +sub delete_all_rules { + my $dbh = C4::Context->dbh; + $dbh->do('DELETE FROM issuingrules'); + $dbh->do('DELETE FROM circulation_rules'); +} -- 2.11.0