From 53da1e6697c7d1809b90ab10f56657a06b7447a1 Mon Sep 17 00:00:00 2001 From: Alex Arnaud Date: Wed, 12 Dec 2018 11:08:43 +0100 Subject: [PATCH] Bug 23732: Hold rules checker Test plan: - apply patch - try to place a hold, on the Hold details page you should see a "Holding rules" button. Click on it - check that the info is consistent with the holding rules, the syspref and the current and future number of holds of the patron - place the hold - set maxreserves syspref to 1 - try to place a hold - see in the holds rules checker that the maxreserves is highlighted in red - restore maxreserves syspref - test that sabotaging the matched circulation rule or hold policy is reflected in the hold rules checker --- C4/Reserves.pm | 246 +++++++++++++----- .../prog/en/modules/reserve/request.tt | 150 +++++++++++ reserve/request.pl | 11 +- t/db_dependent/Reserves/HoldRulesChecker.t | 110 ++++++++ 4 files changed, 454 insertions(+), 63 deletions(-) create mode 100644 t/db_dependent/Reserves/HoldRulesChecker.t diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 987543e1cd..8e4e8da7d5 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -341,19 +341,23 @@ sub CanBookBeReserved{ return { status =>'alreadypossession' }; } + my @circulation_rules; + if ( $params->{itemtype} ) { # biblio-level, item type-contrained my $patron = Koha::Patrons->find($borrowernumber); - my $reservesallowed = Koha::CirculationRules->get_effective_rule( + my $reservesallowed_rule = Koha::CirculationRules->get_effective_rule( { itemtype => $params->{itemtype}, categorycode => $patron->categorycode, branchcode => $pickup_branchcode, rule_name => 'reservesallowed', } - )->rule_value; + ); + push @circulation_rules, $reservesallowed_rule if $reservesallowed_rule; + my $reservesallowed = $reservesallowed_rule ? $reservesallowed_rule->rule_value : undef; $reservesallowed = ( $reservesallowed eq '' ) ? undef : $reservesallowed; my $count = $patron->holds->search( @@ -368,7 +372,7 @@ sub CanBookBeReserved{ } )->count; - return { status => '' } + return { status => '', circulation_rules => \@circulation_rules } if defined $reservesallowed and $reservesallowed < $count + 1; } @@ -386,36 +390,100 @@ sub CanBookBeReserved{ $items = Koha::Items->search({ biblionumber => $biblionumber}); } - my $canReserve = { status => '' }; + my $response = { status => '', item_responses => [] }; my $patron = Koha::Patrons->find( $borrowernumber ); while ( my $item = $items->next ) { - $canReserve = CanItemBeReserved( $patron, $item, $pickup_branchcode, $params ); - return { status => 'OK' } if $canReserve->{status} eq 'OK'; + my $itemResponse = CanItemBeReserved( $patron, $item, $pickup_branchcode, $params ); + push @{ $response->{item_responses} }, { %$itemResponse, item => $item }; + + if ($itemResponse->{status} eq 'OK') { + $response->{status} = 'OK'; + + return $response; + } } - return $canReserve; + + return $response; } =head2 CanItemBeReserved - $canReserve = &CanItemBeReserved($patron, $item, $branchcode, $params) + $canReserve = CanItemBeReserved($patron, $item, $branchcode, $params) if ($canReserve->{status} eq 'OK') { #We can reserve this Item! } - current params are: - 'ignore_hold_counts' - we use this routine to check if an item can fill a hold - on this case we - should not check if there are too many holds as we only care about reservability - -@RETURNS { status => OK }, if the Item can be reserved. - { 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. - { status => recall }, if the borrower has already placed a recall on this item +Current params are: + +=over + +=item * C + +we use this routine to check if an item can +fill a hold - on this case we should not check if there are too many holds as +we only care about reservability + +=back + +Returns a hashref with the following keys: + +=over + +=item * C + +A string that can have the following values: + +=over + +=item * C if the item can be reserved + +=item * C if the item is aged restricted for this borrower + +=item * C if the item is damaged + +=item * C if syspref +'canreservefromotherbranches' is OK + +=item * C if borrower home library is not in hold group, +and holds are only allowed from hold groups. + +=item * C if the borrower has exceeded their maximum reserve +amount. + +=item * C if the borrower has exceeded their maximum +reserve amount for this biblio record + +=item * C if the borrower has exceeded their maximum +reserve amount for today + +=item * C if holds on this item are not allowed + +=item * C if given branchcode is not an existing library + +=item * C if given branchcode is not configured to be +a pickup location + +=item * C if branch transfer limit applies on given item +and branchcode + +=item * C if pickup location is not in hold group, and +pickup locations are only allowed from hold groups. + +=item * C if the borrower has already placed a recall on this item + +=back + +=item * C + +Only if C is equal to C, C, +or C. + +It contains the number of reserves allowed. + +=item * C + +An arrayref containing all circulations rules (L) that +have been checked + +=back =cut @@ -423,8 +491,6 @@ sub CanItemBeReserved { my ( $patron, $item, $pickup_branchcode, $params ) = @_; my $dbh = C4::Context->dbh; - my $ruleitemtype; # itemtype of the matching issuing rule - my $allowedreserves = 0; # Total number of holds allowed across all records, default to none # We check item branch if IndependentBranches is ON # and canreservefromotherbranches is OFF @@ -481,34 +547,21 @@ sub CanItemBeReserved { $reserves_control_branch = $borrower->{branchcode}; } - # we retrieve rights - if ( - my $reservesallowed = Koha::CirculationRules->get_effective_rule({ - itemtype => $item->effective_itemtype, - categorycode => $borrower->{categorycode}, - branchcode => $reserves_control_branch, - rule_name => 'reservesallowed', - }) - ) { - $ruleitemtype = $reservesallowed->itemtype; - $allowedreserves = $reservesallowed->rule_value // 0; #undefined is 0, blank is unlimited - } - else { - $ruleitemtype = undef; - } + my @circulation_rules; - my $rights = Koha::CirculationRules->get_effective_rules({ - categorycode => $borrower->{'categorycode'}, + my $holds_per_record_rule = Koha::CirculationRules->get_effective_rule({ + categorycode => $borrower->{categorycode}, itemtype => $item->effective_itemtype, branchcode => $reserves_control_branch, - rules => ['holds_per_record','holds_per_day'] + rule_name => 'holds_per_record', }); - my $holds_per_record = $rights->{holds_per_record} // 1; - my $holds_per_day = $rights->{holds_per_day}; + push @circulation_rules, $holds_per_record_rule if $holds_per_record_rule; + + my $holds_per_record = $holds_per_record_rule ? $holds_per_record_rule->rule_value : 1; if ( defined $holds_per_record && $holds_per_record ne '' ){ if ( $holds_per_record == 0 ) { - return { status => "noReservesAllowed" }; + return { status => "noReservesAllowed", circulation_rules => \@circulation_rules }; } if ( !$params->{ignore_hold_counts} ) { my $search_params = { @@ -516,24 +569,66 @@ sub CanItemBeReserved { biblionumber => $item->biblionumber, }; my $holds = Koha::Holds->search($search_params); - return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } if $holds->count() >= $holds_per_record; + + if ($holds->count() >= $holds_per_record) { + return { + status => "tooManyHoldsForThisRecord", + limit => $holds_per_record, + circulation_rules => \@circulation_rules, + }; + } } } + my $holds_per_day_rule = Koha::CirculationRules->get_effective_rule({ + categorycode => $borrower->{'categorycode'}, + itemtype => $item->effective_itemtype, + branchcode => $reserves_control_branch, + rule_name => 'holds_per_day', + }); + push @circulation_rules, $holds_per_day_rule if $holds_per_day_rule; + + my $holds_per_day = $holds_per_day_rule ? $holds_per_day_rule->rule_value : undef; if (!$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne '') { my $today_holds = Koha::Holds->search({ borrowernumber => $patron->borrowernumber, reservedate => dt_from_string->date }); - return { status => 'tooManyReservesToday', limit => $holds_per_day } if $today_holds->count() >= $holds_per_day; + + if ($today_holds->count() >= $holds_per_day) { + return { + status => 'tooManyReservesToday', + limit => $holds_per_day, + circulation_rules => \@circulation_rules, + }; + } + } + + my $reservesallowed_rule = Koha::CirculationRules->get_effective_rule({ + itemtype => $item->effective_itemtype, + categorycode => $borrower->{categorycode}, + branchcode => $reserves_control_branch, + rule_name => 'reservesallowed', + }); + push @circulation_rules, $reservesallowed_rule if $reservesallowed_rule; + + my $ruleitemtype; # itemtype of the matching issuing rule + my $allowedreserves = 0; # Total number of holds allowed across all records, default to none + if ($reservesallowed_rule) { + $ruleitemtype = $reservesallowed_rule->itemtype; + $allowedreserves = $reservesallowed_rule->rule_value // 0; #undefined is 0, blank is unlimited } # we check if it's ok or not if ( defined $allowedreserves && $allowedreserves ne '' ){ - if( $allowedreserves == 0 ){ - return { status => 'noReservesAllowed' }; + if ( $allowedreserves == 0 ) { + return { + status => 'noReservesAllowed', + circulation_rules => \@circulation_rules, + }; } + if ( !$params->{ignore_hold_counts} ) { # we retrieve count my $querycount = q{ @@ -577,45 +672,66 @@ sub CanItemBeReserved { $reservecount = $rowcount->{count}; } - return { status => 'tooManyReserves', limit => $allowedreserves } if $reservecount >= $allowedreserves; + if ($reservecount >= $allowedreserves) { + return { + status => 'tooManyReserves', + limit => $allowedreserves, + circulation_rules => \@circulation_rules, + }; + } } } # Now we need to check hold limits by patron category - my $rule = Koha::CirculationRules->get_effective_rule( + my $max_holds_rule = Koha::CirculationRules->get_effective_rule( { categorycode => $patron->categorycode, branchcode => $reserves_control_branch, rule_name => 'max_holds', } ); - if (!$params->{ignore_hold_counts} && $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) { + push @circulation_rules, $max_holds_rule if $max_holds_rule; + + if (!$params->{ignore_hold_counts} && $max_holds_rule && defined( $max_holds_rule->rule_value ) && $max_holds_rule->rule_value ne '' ) { my $total_holds_count = Koha::Holds->search( { borrowernumber => $patron->borrowernumber } )->count(); - return { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value; + if ($total_holds_count >= $max_holds_rule->rule_value) { + return { + status => 'tooManyReserves', + limit => $max_holds_rule->rule_value, + circulation_rules => \@circulation_rules, + }; + } } + my $holdallowed_rule = Koha::CirculationRules->get_effective_rule({ + branchcode => $reserves_control_branch, + itemtype => $item->effective_itemtype, + rule_name => 'holdallowed', + }); + push @circulation_rules, $holdallowed_rule if $holdallowed_rule; + my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); if ( $branchitemrule->{holdallowed} eq 'not_allowed' ) { - return { status => 'notReservable' }; + return { status => 'notReservable', circulation_rules => \@circulation_rules }; } if ( $branchitemrule->{holdallowed} eq 'from_home_library' && $borrower->{branchcode} ne $item->homebranch ) { - return { status => 'cannotReserveFromOtherBranches' }; + return { status => 'cannotReserveFromOtherBranches', circulation_rules => \@circulation_rules }; } my $item_library = Koha::Libraries->find( {branchcode => $item->homebranch} ); if ( $branchitemrule->{holdallowed} eq 'from_local_hold_group') { if($patron->branchcode ne $item->homebranch && !$item_library->validate_hold_sibling( {branchcode => $patron->branchcode} )) { - return { status => 'branchNotInHoldGroup' }; + return { status => 'branchNotInHoldGroup', circulation_rules => \@circulation_rules }; } } @@ -633,15 +749,23 @@ sub CanItemBeReserved { unless ($item->can_be_transferred({ to => $destination })) { return { status => 'cannotBeTransferred' }; } + + my $hold_fulfillment_policy_rule = Koha::CirculationRules->get_effective_rule({ + branchcode => $reserves_control_branch, + itemtype => $item->effective_itemtype, + rule_name => 'hold_fulfillment_policy', + }); + push @circulation_rules, $hold_fulfillment_policy_rule if $hold_fulfillment_policy_rule; + if ($branchitemrule->{hold_fulfillment_policy} eq 'holdgroup' && !$item_library->validate_hold_sibling( {branchcode => $pickup_branchcode} )) { - return { status => 'pickupNotInHoldGroup' }; + return { status => 'pickupNotInHoldGroup', circulation_rules => \@circulation_rules }; } if ($branchitemrule->{hold_fulfillment_policy} eq 'patrongroup' && !Koha::Libraries->find({branchcode => $borrower->{branchcode}})->validate_hold_sibling({branchcode => $pickup_branchcode})) { - return { status => 'pickupNotInHoldGroup' }; + return { status => 'pickupNotInHoldGroup', circulation_rules => \@circulation_rules }; } } - return { status => 'OK' }; + return { status => 'OK', circulation_rules => \@circulation_rules }; } =head2 CanReserveBeCanceledFromOpac 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 e2d33c5415..1531188c19 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -205,6 +205,10 @@ [% END %] + [% IF canbookbereserved_responses %] + + [% END %] + [% UNLESS club OR patron OR patron.borrowernumber OR noitems OR nobiblio %] [% IF ( messageborrower ) %]
@@ -472,6 +476,7 @@
  • No items available: One or more records have no items that can be held
    • [% END # /IF exceeded_maxreserves %] [% END # /UNLESS multi_hold %] +
    [% END # /IF ( exceeded_maxreserves || ... %] @@ -1756,4 +1761,149 @@ [% END %] + + [% INCLUDE 'intranet-bottom.inc' %] diff --git a/reserve/request.pl b/reserve/request.pl index 0c37d6e4c1..71a87b16b6 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -181,8 +181,10 @@ if ($borrowernumber_hold && !$action) { # FIXME At this time we have a simple count of reservs, but, later, we could improve the infos "title" ... my $reserves_count = $patron->holds->count; + $template->param(reserves_count => $reserves_count); my $new_reserves_count = scalar( @biblionumbers ); + $template->param(new_reserves_count => $new_reserves_count); my $maxreserves = C4::Context->preference('maxreserves'); $template->param( maxreserves => $maxreserves ); @@ -198,8 +200,6 @@ if ($borrowernumber_hold && !$action) { $exceeded_maxreserves = 1; $template->param( new_reserves_allowed => $new_reserves_allowed, - new_reserves_count => $new_reserves_count, - reserves_count => $reserves_count, maxreserves => $maxreserves, ); } @@ -293,6 +293,7 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) my @biblioloop = (); my $no_reserves_allowed = 0; my $num_bibs_available = 0; + my @canbookbereserved_responses; foreach my $biblionumber (@biblionumbers) { next unless $biblionumber =~ m|^\d+$|; @@ -311,6 +312,9 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) if ( $patron ) { { # CanBookBeReserved my $canReserve = CanBookBeReserved( $patron->borrowernumber, $biblionumber ); + + push @canbookbereserved_responses, { %$canReserve, biblio => $biblio }; + if ( $canReserve->{status} eq 'OK' ) { #All is OK and we can continue @@ -694,6 +698,9 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) $template->param( no_bibs_available => 1 ) unless $num_bibs_available > 0; + $Data::Dumper::Maxdepth = 3; + use Data::Dumper; warn Dumper \@canbookbereserved_responses; + $template->param( canbookbereserved_responses => \@canbookbereserved_responses ); $template->param( biblioloop => \@biblioloop ); $template->param( no_reserves_allowed => $no_reserves_allowed ); $template->param( exceeded_maxreserves => $exceeded_maxreserves ); diff --git a/t/db_dependent/Reserves/HoldRulesChecker.t b/t/db_dependent/Reserves/HoldRulesChecker.t new file mode 100644 index 0000000000..1bebad86ef --- /dev/null +++ b/t/db_dependent/Reserves/HoldRulesChecker.t @@ -0,0 +1,110 @@ +#!/usr/bin/perl + +# Copyright 2019 Biblibre +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 13; +use t::lib::TestBuilder; + +use C4::Reserves qw( CanBookBeReserved ); + +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; + +my $builder = t::lib::TestBuilder->new(); + +my $library = $builder->build_object({ class => 'Koha::Libraries' }); +my $category = $builder->build_object({ class => 'Koha::Patron::Categories' }); +my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + categorycode => $category->categorycode, + branchcode => $library->branchcode, + }, + } +); + +my $itemtype = $builder->build_object({ class => 'Koha::ItemTypes' }); + +my $biblio = $builder->build_sample_biblio( + { + title => 'Title 1', + } +); +my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library->branchcode, + itype => $itemtype->itemtype, + damaged => 0, + } +); + +my $rules_rs = Koha::Database->new()->schema()->resultset('CirculationRule'); +$rules_rs->delete(); + +my $default_reservesallowed_rule = $rules_rs->create( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rule_name => 'reservesallowed', + rule_value => 10, + } +); + +my $reservesallowed_rule = $rules_rs->create( + { + categorycode => $patron->categorycode, + itemtype => undef, + branchcode => $library->branchcode, + rule_name => 'reservesallowed', + rule_value => 0, + } +); + +my $can = CanBookBeReserved($patron->borrowernumber, $biblio->biblionumber); +is($can->{status}, ''); +is(scalar @{ $can->{circulation_rules} }, 0); +is(scalar @{ $can->{item_responses} }, 1); +is($can->{item_responses}->[0]->{status}, 'noReservesAllowed'); +is(scalar @{ $can->{item_responses}->[0]->{circulation_rules} }, 1); +is($can->{item_responses}->[0]->{circulation_rules}->[0]->id, $reservesallowed_rule->id); + +my $holds_per_record_rule = $rules_rs->create( + { + categorycode => $patron->categorycode, + itemtype => undef, + branchcode => $library->branchcode, + rule_name => 'holds_per_record', + rule_value => 1, + } +); + +$can = CanBookBeReserved($patron->borrowernumber, $biblio->biblionumber); +is($can->{status}, ''); +is(scalar @{ $can->{circulation_rules} }, 0); +is(scalar @{ $can->{item_responses} }, 1); +is($can->{item_responses}->[0]->{status}, 'noReservesAllowed'); +is(scalar @{ $can->{item_responses}->[0]->{circulation_rules} }, 2); +is($can->{item_responses}->[0]->{circulation_rules}->[0]->id, $holds_per_record_rule->id); +is($can->{item_responses}->[0]->{circulation_rules}->[1]->id, $reservesallowed_rule->id); + +$schema->storage->txn_rollback; -- 2.30.2