Bugzilla – Attachment 95435 Details for
Bug 23732
Hold rules checker: show matched rules and syspref values to help understand why a hold is possible or not
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23732: Hold rules checker
Bug-23732-Hold-rules-checker.patch (text/plain), 23.14 KB, created by
Arthur Suzuki
on 2019-11-14 16:44:08 UTC
(
hide
)
Description:
Bug 23732: Hold rules checker
Filename:
MIME Type:
Creator:
Arthur Suzuki
Created:
2019-11-14 16:44:08 UTC
Size:
23.14 KB
patch
obsolete
>From 6f266bd2f22b2829e31e9607c20b24d6cbf44c79 Mon Sep 17 00:00:00 2001 >From: Alex Arnaud <alex.arnaud@biblibre.com> >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 @@ > > <div class="main container-fluid"> > <div class="row"> >+ >+ <div class="modal fade" id="TODOmyModal" tabindex="-1" role="dialog" aria-labelledby="TODOmyModalLabel"> >+ <div class="modal-dialog" role="document"> >+ <div class="modal-content"> >+ <div class="modal-header"> >+ <h4 class="modal-title" id="myModalLabel">Holding rules</h4> >+ </div> >+ <div class="modal-body"> >+ <div> >+ <h4>Overall info</h4> >+ <ul> >+ <li> >+ Patron has <strong>[% reserves_count | html %]</strong> hold(s) and this request would raise the number to <strong>[% new_reserves_count + reserves_count | html %]</strong>. >+ </li> >+ <li> >+ <span [% IF reserves_count >= Koha.Preference('maxreserves') %] class="text-danger" [% END %]> >+ <i>maxreserves</i> preference: <strong>[% Koha.Preference('maxreserves') | html %]</strong> >+ </span> >+ </li> >+ <li> >+ <i>ReservesControlBranch</i> preference: <strong>[% Koha.Preference('ReservesControlBranch') | html %]</strong> <!-- TODO make translatable --> >+ </li >+ </ul> >+ </div> >+ >+ <h4>Per record info</h4> >+ <div class="panel-group" id="accordion" role="tablist" aria-multiselectable="true"> >+ >+ [% FOREACH element IN whyicantreserve %] >+ <div class="panel panel-default"> >+ <div class="panel-heading" role="tab" id="heading-[% element.biblionumber | html %]"> >+ <h4 class="panel-title"> >+ <a role="button" data-toggle="collapse" data-parent="#accordion" href="#collapse-[% element.biblionumber | html %]" aria-expanded="true" aria-controls="collapse-[% element.biblionumber | html %]"> >+ [% element.title | html %] >+ </a> >+ </h4> >+ </div> >+ <div id="collapse-[% element.biblionumber | html %]" class="panel-collapse collapse in" role="tabpanel" aria-labelledby="heading-[% element.biblionumber | html %]"> >+ <div class="panel-body"> >+ >+ <div id="elements-to-reserve-details"> >+ <h5>Matched circulation rule:</h5> >+ <table> >+ <tr> >+ <th>Library</th> >+ <th>Category</th> >+ <th>Item type</th> >+ <th>Holds</th> >+ <th>Holds per record</th> >+ <th>Holds per day</th> >+ </tr> >+ <tr> >+ <td> >+ [% IF element.issuingrule.branchcode == '*' %] >+ <i>All</i> >+ [% ELSE %] >+ [% Branches.GetName(element.issuingrule.branchcode) | html %] >+ ([% element.issuingrule.branchcode | html %]) >+ [% END %] >+ </td> >+ <td> >+ [% IF element.issuingrule.categorycode == '*' %] >+ <i>All</i> >+ [% ELSE %] >+ [% Categories.GetName(element.issuingrule.categorycode) | html %] >+ [% END %] >+ </td> >+ <td> >+ [% IF element.issuingrule.itemtype == '*' %] >+ <i>All</i> >+ [% ELSE %] >+ [% ItemTypes.GetDescription(element.issuingrule.itemtype) | html %] >+ [% END %] >+ </td> >+ <td> >+ [% element.issuingrule.reservesallowed | html %] >+ </td> >+ <td> >+ [% element.issuingrule.holds_per_record | html %] >+ </td> >+ <td> >+ [% element.issuingrule.holds_per_day || 'unlimited' | html %] >+ </td> >+ </tr> >+ </table> >+ >+ <h5>Matched hold policy:</h5> >+ >+ <table> >+ <tr> >+ <th>Library</th> >+ <th>Category</th> >+ <th>Holds</th> >+ </tr> >+ <tr> >+ [% IF element.circulation_rule %] >+ <td> >+ [% IF element.circulation_rule.branchcode %] >+ [% Branches.GetName(element.circulation_rule.branchcode) | html %] >+ ([% element.circulation_rule.branchcode | html %]) >+ [% ELSE %] >+ <i>All</i> >+ [% END %] >+ </td> >+ <td> >+ [% Categories.GetName(element.circulation_rule.categorycode) | html %] >+ </td> >+ <td> >+ [% element.circulation_rule.rule_value || 'unlimited' | html %] >+ </td> >+ [% ELSE %] >+ <td colspan="3">No rule matched</td> >+ [% END %] >+ </tr> >+ </table> >+ >+ </div><!-- END elements-to-reserve-details --> >+ </div> >+ </div> >+ </div> >+ [% END %] >+ </div> >+ </div> >+ <div class="modal-footer"> >+ <button type="button" class="btn btn-default" data-dismiss="modal">Close</button> >+ </div> >+ </div> >+ </div> >+ </div> <!-- end of Modal --> >+ > [% IF ( multi_hold ) # No sidebar menu when placing multiple holds %] > <div class="col-md-10 col-md-offset-1"> > [% ELSE %] >@@ -67,11 +197,19 @@ > </div> > [% END %] > >- [% UNLESS ( multi_hold ) %] >- <h1>Place a hold on [% INCLUDE 'biblio-default-view.inc' %][% INCLUDE 'biblio-title.inc' %]</a></h1> >- [% ELSE %] >- <h1>Confirm holds</h1> >- [% END %] >+ <h1> >+ [% UNLESS ( multi_hold ) %] >+ Place a hold on [% INCLUDE 'biblio-default-view.inc' %][% INCLUDE 'biblio-title.inc' %]</a> >+ [% ELSE %] >+ Confirm holds >+ [% END %] >+ >+ [% IF whyicantreserve %] >+ <button type="button" class="btn btn-info btn-sm" data-toggle="modal" data-target="#TODOmyModal"> >+ Show holding rules >+ </button> >+ [% END %] >+ </h1> > > [% 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23732
:
95435
|
95436
|
95443
|
95444
|
95445
|
95934
|
147501
|
147504
|
173427
|
173428
|
178886
|
178887
|
178888
|
179026
|
179027