From 6f266bd2f22b2829e31e9607c20b24d6cbf44c79 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 - yarn build - generate translations - try to place a hold, on the Hold details page you should see a "Holding rules" blue button - click it - check that the info if 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 (it the only thing supporting highlighting) - restore maxreserves syspref - test that sabotaging the matched circulation rule or hold policy is reflected (the limits) in the hold rules checker --- C4/Reserves.pm | 115 +++++++++------- .../intranet-tmpl/prog/css/src/staff-global.scss | 4 + .../prog/en/modules/reserve/request.tt | 148 ++++++++++++++++++++- reserve/request.pl | 15 ++- 4 files changed, 228 insertions(+), 54 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 2a77c3dc5b..81b5748e82 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -285,7 +285,7 @@ sub CanBookBeReserved{ my $canReserve = { status => '' }; foreach my $itemnumber (@itemnumbers) { $canReserve = CanItemBeReserved( $borrowernumber, $itemnumber, $pickup_branchcode ); - return { status => 'OK' } if $canReserve->{status} eq 'OK'; + return $canReserve if $canReserve->{status} eq 'OK'; } return $canReserve; } @@ -323,31 +323,8 @@ sub CanItemBeReserved { my $patron = Koha::Patrons->find( $borrowernumber ); my $borrower = $patron->unblessed; - # If an item is damaged and we don't allow holds on damaged items, we can stop right here - return { status =>'damaged' } - if ( $item->damaged - && !C4::Context->preference('AllowHoldsOnDamagedItems') ); - - # Check for the age restriction - my ( $ageRestriction, $daysToAgeRestriction ) = - C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower ); - return { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0; - - # Check that the patron doesn't have an item level hold on this item already - return { status =>'itemAlreadyOnHold' } - if Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->count(); - my $controlbranch = C4::Context->preference('ReservesControlBranch'); - my $querycount = q{ - SELECT count(*) AS count - FROM reserves - LEFT JOIN items USING (itemnumber) - LEFT JOIN biblioitems ON (reserves.biblionumber=biblioitems.biblionumber) - LEFT JOIN borrowers USING (borrowernumber) - WHERE borrowernumber = ? - }; - my $branchcode = ""; my $branchfield = "reserves.branchcode"; @@ -361,16 +338,49 @@ sub CanItemBeReserved { } # we retrieve rights - if ( my $rights = GetHoldRule( $borrower->{'categorycode'}, $item->effective_itemtype, $branchcode ) ) { - $ruleitemtype = $rights->{itemtype}; - $allowedreserves = $rights->{reservesallowed}; - $holds_per_record = $rights->{holds_per_record}; - $holds_per_day = $rights->{holds_per_day}; + my $issuingrule; + if ( $issuingrule = GetHoldRule( $borrower->{'categorycode'}, $item->effective_itemtype, $branchcode ) ) { + $ruleitemtype = $issuingrule->{itemtype}; + $allowedreserves = $issuingrule->{reservesallowed}; + $holds_per_record = $issuingrule->{holds_per_record}; + $holds_per_day = $issuingrule->{holds_per_day}; } else { $ruleitemtype = '*'; } + my $circulation_rule = Koha::CirculationRules->get_effective_rule( + { + categorycode => $borrower->{categorycode}, + branchcode => $branchcode, + rule_name => 'max_holds', + } + ); + + # If an item is damaged and we don't allow holds on damaged items, we can stop right here + return { status =>'damaged', issuingrule => $issuingrule, circulation_rule => $circulation_rule } + if ( $item->damaged + && !C4::Context->preference('AllowHoldsOnDamagedItems') ); + + # Check for the age restriction + my ( $ageRestriction, $daysToAgeRestriction ) = + C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower ); + return { status => 'ageRestricted', issuingrule => $issuingrule, circulation_rule => $circulation_rule } if $daysToAgeRestriction && $daysToAgeRestriction > 0; + + # Check that the patron doesn't have an item level hold on this item already + return { status =>'itemAlreadyOnHold', issuingrule => $issuingrule, circulation_rule => $circulation_rule } + if Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->count(); + + my $querycount = q{ + SELECT count(*) AS count + FROM reserves + LEFT JOIN items USING (itemnumber) + LEFT JOIN biblioitems ON (reserves.biblionumber=biblioitems.biblionumber) + LEFT JOIN borrowers USING (borrowernumber) + WHERE borrowernumber = ? + }; + + $item = Koha::Items->find( $itemnumber ); my $holds = Koha::Holds->search( { borrowernumber => $borrowernumber, @@ -379,7 +389,11 @@ sub CanItemBeReserved { } ); if ( $holds->count() >= $holds_per_record ) { - return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; + $issuingrule->{exceeded} = 'holds_per_record'; + return { status => "tooManyHoldsForThisRecord", + limit => $holds_per_record, + issuingrule => $issuingrule, + circulation_rule => $circulation_rule }; } my $today_holds = Koha::Holds->search({ @@ -391,7 +405,11 @@ sub CanItemBeReserved { ( ( $holds_per_day > 0 && $today_holds->count() >= $holds_per_day ) or ( $holds_per_day == 0 ) ) ) { - return { status => 'tooManyReservesToday', limit => $holds_per_day }; + $issuingrule->{exceeded} = 'holds_per_day'; + return { status => 'tooManyReservesToday', + limit => $holds_per_day, + issuingrule => $issuingrule, + circulation_rule => $circulation_rule }; } # we retrieve count @@ -422,25 +440,28 @@ sub CanItemBeReserved { # we check if it's ok or not if ( $reservecount >= $allowedreserves ) { - return { status => 'tooManyReserves', limit => $allowedreserves }; + $issuingrule->{exceeded} = 'allowedreserves'; + return { status => 'tooManyReserves', + limit => $allowedreserves, + issuingrule => $issuingrule, + circulation_rule => $circulation_rule }; } # Now we need to check hold limits by patron category - my $rule = Koha::CirculationRules->get_effective_rule( - { - categorycode => $borrower->{categorycode}, - branchcode => $branchcode, - rule_name => 'max_holds', - } - ); - if ( $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) { + if ( $circulation_rule && defined( $circulation_rule->rule_value ) && $circulation_rule->rule_value ne '' ) { my $total_holds_count = Koha::Holds->search( { borrowernumber => $borrower->{borrowernumber} } )->count(); - return { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value; + return { + status => 'tooManyReserves', + limit => $circulation_rule->rule_value, + issuingrule => $issuingrule, + circulation_rule => $circulation_rule, + circulation_rule_exceeded => 1 + } if $total_holds_count >= $circulation_rule->rule_value; } my $reserves_control_branch = @@ -449,13 +470,13 @@ sub CanItemBeReserved { C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype ); # FIXME Should not be item->effective_itemtype? if ( $branchitemrule->{holdallowed} == 0 ) { - return { status => 'notReservable' }; + return { status => 'notReservable', issuingrule => $issuingrule, circulation_rule => $circulation_rule }; } if ( $branchitemrule->{holdallowed} == 1 && $borrower->{branchcode} ne $item->homebranch ) { - return { status => 'cannotReserveFromOtherBranches' }; + return { status => 'cannotReserveFromOtherBranches', issuingrule => $issuingrule, circulation_rule => $circulation_rule }; } # If reservecount is ok, we check item branch if IndependentBranches is ON @@ -464,7 +485,7 @@ sub CanItemBeReserved { and !C4::Context->preference('canreservefromotherbranches') ) { if ( $item->homebranch ne $borrower->{branchcode} ) { - return { status => 'cannotReserveFromOtherBranches' }; + return { status => 'cannotReserveFromOtherBranches', issuingrule => $issuingrule, circulation_rule => $circulation_rule }; } } @@ -474,17 +495,17 @@ sub CanItemBeReserved { }); unless ($destination) { - return { status => 'libraryNotFound' }; + return { status => 'libraryNotFound', issuingrule => $issuingrule, circulation_rule => $circulation_rule }; } unless ($destination->pickup_location) { - return { status => 'libraryNotPickupLocation' }; + return { status => 'libraryNotPickupLocation', issuingrule => $issuingrule, circulation_rule => $circulation_rule }; } unless ($item->can_be_transferred({ to => $destination })) { return { status => 'cannotBeTransferred' }; } } - return { status => 'OK' }; + return { status => 'OK', issuingrule => $issuingrule, circulation_rule => $circulation_rule }; } =head2 CanReserveBeCanceledFromOpac diff --git a/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss b/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss index 1f0c3274d2..745786db39 100644 --- a/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss +++ b/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss @@ -4334,3 +4334,7 @@ div#makechart ol li { border-bottom-right-radius: 5px; } } + +span.exceeded { + color: red; +} 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 43c0f2aa84..8eed08842f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -38,6 +38,136 @@
+ + + [% IF ( multi_hold ) # No sidebar menu when placing multiple holds %]
[% ELSE %] @@ -67,11 +197,19 @@
[% END %] - [% UNLESS ( multi_hold ) %] -

Place a hold on [% INCLUDE 'biblio-default-view.inc' %][% INCLUDE 'biblio-title.inc' %]

- [% ELSE %] -

Confirm holds

- [% END %] +

+ [% UNLESS ( multi_hold ) %] + Place a hold on [% INCLUDE 'biblio-default-view.inc' %][% INCLUDE 'biblio-title.inc' %] + [% ELSE %] + Confirm holds + [% END %] + + [% IF whyicantreserve %] + + [% END %] +

[% UNLESS club OR patron OR patron.borrowernumber OR noitems %] [% IF ( messageborrower ) %] diff --git a/reserve/request.pl b/reserve/request.pl index c7832434a3..8483ecdd57 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -182,8 +182,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'); if ( $maxreserves @@ -197,8 +199,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, ); } @@ -288,6 +288,7 @@ if ($patron) { my $itemdata_enumchron = 0; my $itemdata_ccode = 0; my @biblioloop = (); +my @whyicantreserve; foreach my $biblionumber (@biblionumbers) { next unless $biblionumber =~ m|^\d+$|; @@ -299,6 +300,15 @@ foreach my $biblionumber (@biblionumbers) { if ( $patron ) { { # CanBookBeReserved my $canReserve = CanBookBeReserved( $patron->borrowernumber, $biblionumber, $pickup ); + + push @whyicantreserve, { + biblionumber => $biblionumber, + title => $biblio->title, + issuingrule => $canReserve->{issuingrule}, + circulation_rule => $canReserve->{circulation_rule}, + circulation_rule_exceeded => $canReserve->{circulation_rule_exceeded}, + }; + if ( $canReserve->{status} eq 'OK' ) { #All is OK and we can continue @@ -718,6 +728,7 @@ foreach my $biblionumber (@biblionumbers) { push @biblioloop, \%biblioloopiter; } +$template->param( whyicantreserve => \@whyicantreserve ); $template->param( biblioloop => \@biblioloop ); $template->param( biblionumbers => $biblionumbers ); $template->param( exceeded_maxreserves => $exceeded_maxreserves ); -- 2.11.0