From 0f23622041dba9d882ff27b0a879aae7f642a6c8 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 "Cannot place hold: no item are available to be placed on hold" 7 - Try placing a multihold with either record above and a holdable record, message should end "Cannot place hold on some items' 8 - prove -v t/db_dependent/Holds.t Rebase - Fix test expectation Signed-off-by: Owen Leonard Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- C4/Reserves.pm | 22 +- .../prog/en/modules/reserve/request.tt | 12 +- reserve/request.pl | 9 +- t/db_dependent/Holds.t | 227 ++++++++++++++++-- 4 files changed, 245 insertions(+), 25 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index dc99bc1cbb..8941d4f72c 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -457,9 +457,13 @@ sub CanItemBeReserved { $search_params->{found} = undef if $params->{ignore_found_holds}; my $holds = Koha::Holds->search($search_params); - if (!$params->{ignore_hold_counts} && 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 ( !$params->{ignore_hold_counts} && $holds->count() >= $holds_per_record ) { + return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; + } } my $today_holds = Koha::Holds->search({ @@ -500,9 +504,13 @@ sub CanItemBeReserved { } # we check if it's ok or not - if ( defined $allowedreserves && $allowedreserves ne '' - && $reservecount >= $allowedreserves && (!$params->{ignore_hold_counts} || $allowedreserves == 0 ) ) { - return { status => 'tooManyReserves', limit => $allowedreserves }; + if ( defined $allowedreserves && $allowedreserves ne '' ){ + if( $allowedreserves == 0 ){ + return { status => 'noReservesAllowed' }; + } + if ( !$params->{ignore_hold_counts} && $reservecount >= $allowedreserves ) { + return { status => 'tooManyReserves', limit => $allowedreserves }; + } } # Now we need to check hold limits by patron category @@ -526,7 +534,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} eq 'not_allowed' ) { 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 e7f2479b47..821a99863a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -324,13 +324,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 %] @@ -609,6 +613,8 @@ Cannot be transferred to pickup library [% ELSIF itemloo.not_holdable == 'pickupNotInHoldGroup' %] Only pickup locations within the same hold group are allowed + [% 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 31990f1988..c293ea1011 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -185,6 +185,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 ) ) { @@ -287,6 +289,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+$|; @@ -302,7 +305,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} ); } @@ -729,6 +735,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 285d4d3663..64419b5fee 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 => 72; +use Test::More tests => 71; use MARC::Record; use C4::Biblio; @@ -391,26 +391,225 @@ AddReserve( } ); is( - CanItemBeReserved( $borrowernumbers[0], $item->itemnumber)->{status}, 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $item->itemnumber)->{status}, 'noReservesAllowed', "cannot request item if policy that matches on item-level item type forbids it" ); is( - CanItemBeReserved( $borrowernumbers[0], $item->itemnumber, undef, { ignore_hold_counts => 1 })->{status}, 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $item->itemnumber, undef, { ignore_hold_counts => 1 })->{status}, 'noReservesAllowed', "cannot request item if policy that matches on item-level item type forbids it even if ignoring counts" ); -$item->itype('CAN')->store; -ok( - CanItemBeReserved( $borrowernumbers[0], $item->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); -$item->itype(undef)->store; -ok( - CanItemBeReserved( $borrowernumbers[0], $item->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 $itemnumber_1_can = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can, biblionumber => $biblionumber_cannot })->itemnumber; + my $itemnumber_1_cannot = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant, biblionumber => $biblionumber_cannot })->itemnumber; + my $itemnumber_2_can = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can, biblionumber => $biblionumber_can })->itemnumber; + my $itemnumber_2_cannot = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant, biblionumber => $biblionumber_can })->itemnumber; + my $itemnumber_3_cannot = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant_record, biblionumber => $biblionumber_record_cannot })->itemnumber; + + 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 $itemnumber_11 = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can, biblionumber => $biblionumber_1 })->itemnumber; + my $itemnumber_12 = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can, biblionumber => $biblionumber_1 })->itemnumber; + my $biblionumber_2 = $builder->build_sample_biblio({ itemtype => $itemtype_can })->biblionumber; + my $itemnumber_21 = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can, biblionumber => $biblionumber_2 })->itemnumber; + my $itemnumber_22 = $builder->build_sample_item({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can, biblionumber => $biblionumber_2 })->itemnumber; + + 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.39.5