Browse Source

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 <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
21.05.x
Nick Clemens 6 years ago
committed by Jonathan Druart
parent
commit
0f23622041
  1. 22
      C4/Reserves.pm
  2. 12
      koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
  3. 9
      reserve/request.pl
  4. 227
      t/db_dependent/Holds.t

22
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' };

12
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt

@ -324,13 +324,15 @@
</div>
[% 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 ) %]
<div class="dialog alert">
[% UNLESS ( multi_hold ) %]
<h3>Cannot place hold</h3>
<ul>
[% IF ( exceeded_maxreserves ) %]
[% IF ( no_reserves_allowed ) %]
<li><strong>No holds allowed: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% patron.borrowernumber | uri %]">[% patron.firstname | html %] [% patron.surname | html %] </a> cannot place a hold on any of these items.</li>
[% ELSIF ( exceeded_maxreserves ) %]
<li><strong>Too many holds: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% patron.borrowernumber | uri %]">[% patron.firstname | html %] [% patron.surname | html %] </a> can only place a maximum of [% maxreserves | html %] total holds.</li>
[% ELSIF ( exceeded_holds_per_record ) %]
<li><strong>Too many holds for this record: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% patron.borrowernumber | uri %]">[% patron.firstname | html %] [% patron.surname | html %] </a> can only place a maximum of [% max_holds_for_record | html %] hold(s) on this record.</li>
@ -348,7 +350,9 @@
</ul>
[% ELSE # UNLESS multi_hold %]
<h3>Cannot place hold on some items</h3>
[% IF ( exceeded_maxreserves ) %]
[% IF (no_reserves_allowed ) %]
<li><strong>No holds allowed: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% patron.borrowernumber | uri %]">[% patron.firstname | html %] [% patron.surname | html %] </a> cannot place holds on some of these title's items.</li>
[% ELSIF ( exceeded_maxreserves ) %]
<li><strong>Too many holds: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% patron.borrowernumber | uri %]">[% patron.firstname | html %] [% patron.surname | html %] </a> can place [% new_reserves_allowed | html %] of the requested [% new_reserves_count | html %] holds for a maximum of [% maxreserves | html %] total holds.</li>
[% 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 %]

9
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));

227
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

Loading…
Cancel
Save