From 9a772052ca579257de02d4af95bf7f48bc176313 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 | 22 +- .../prog/en/modules/reserve/request.tt | 12 +- reserve/request.pl | 9 +- t/db_dependent/Holds.t | 239 ++++++++++++++++++--- 4 files changed, 244 insertions(+), 38 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 00d921d77e..099bc6d180 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -414,9 +414,13 @@ sub CanItemBeReserved { biblionumber => $item->biblionumber, } ); - if ( defined $holds_per_record && $holds_per_record ne '' - && $holds->count() >= $holds_per_record ) { - return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; + if ( defined $holds_per_record && $holds_per_record ne '' ){ + if ( $holds_per_record == 0 ) { + return { status => "noReservesAllowed" }; + } + if ( $holds->count() >= $holds_per_record ) { + return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; + } } my $today_holds = Koha::Holds->search({ @@ -457,9 +461,13 @@ sub CanItemBeReserved { } # we check if it's ok or not - if ( defined $allowedreserves && $allowedreserves ne '' - && $reservecount >= $allowedreserves ) { - return { status => 'tooManyReserves', limit => $allowedreserves }; + if ( defined $allowedreserves && $allowedreserves ne '' ){ + if( $allowedreserves == 0 ){ + return { status => 'noReservesAllowed' }; + } + if ( $reservecount >= $allowedreserves ) { + return { status => 'tooManyReserves', limit => $allowedreserves }; + } } # Now we need to check hold limits by patron category @@ -483,7 +491,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 00b7b66243..c7a92ac865 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -310,13 +310,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 %] @@ -588,6 +592,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 5864b3d747..de9b7584d9 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} ); } @@ -736,6 +742,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 b648db8f4a..3ca2bcef04 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; @@ -346,33 +346,218 @@ 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( - { - branchcode => $branch_1, - borrowernumber => $borrowernumbers[0], - biblionumber => $biblio->biblionumber, - priority => 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)" -); + 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}; + + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $itemtype_cant, + rules => { + reservesallowed => 0, + holds_per_record => 99, + } + } + ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $itemtype_can, + rules => { + reservesallowed => 2, + holds_per_record => 2, + } + } + ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $itemtype_cant_record, + rules => { + reservesallowed => 0, + holds_per_record => 0, + } + } + ); + + Koha::CirculationRules->set_rules( + { + branchcode => $branch_1, + itemtype => $itemtype_cant, + rules => { + holdallowed => 0, + returnbranch => 'homebranch', + } + } + ); + Koha::CirculationRules->set_rules( + { + branchcode => $branch_1, + itemtype => $itemtype_can, + 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 => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $biblionumber_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 => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $biblionumber_1, + itemnumber => $itemnumber_11, + }); + + $dbh->do('DELETE FROM circulation_rules'); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + 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" + ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + 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" + ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + 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 => $branch_1, + borrowernumber => $borrowernumbers[0], + biblionumber => $biblionumber_2, + itemnumber => $itemnumber_21 + }); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + 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 -- 2.11.0