@@ -, +, @@
and is missing data
message should end "...cannot place holds on some of these titles'
items"
---
C4/Reserves.pm | 9 +-
.../prog/en/modules/reserve/request.tt | 12 +-
reserve/request.pl | 9 +-
t/db_dependent/Holds.t | 210 +++++++++++++++++----
4 files changed, 195 insertions(+), 45 deletions(-)
--- a/C4/Reserves.pm
+++ a/C4/Reserves.pm
@@ -395,6 +395,10 @@ sub CanItemBeReserved {
found => undef, # Found holds don't count against a patron's holds limit
}
);
+
+ if ( $holds_per_record == 0 ) {
+ return { status => "noReservesAllowed" };
+ }
if ( $holds->count() >= $holds_per_record ) {
return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record };
}
@@ -438,6 +442,9 @@ sub CanItemBeReserved {
}
# we check if it's ok or not
+ if( $allowedreserves == 0 ){
+ return { status => 'noReservesAllowed' };
+ }
if ( $reservecount >= $allowedreserves ) {
return { status => 'tooManyReserves', limit => $allowedreserves };
}
@@ -463,7 +470,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} == 0 ) {
return { status => 'notReservable' };
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -309,13 +309,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 %]
@@ -587,6 +591,8 @@
Patron already has hold for this item
[% ELSIF itemloo.not_holdable == 'cannotBeTransferred' %]
Cannot be transferred to pickup library
+ [% ELSIF itemloo.not_holdable == 'noReservesAllowed' %]
+ No reserves are allowed on this item
[% ELSE %]
[% itemloo.not_holdable | html %]
[% END %]
--- a/reserve/request.pl
+++ a/reserve/request.pl
@@ -186,6 +186,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 ) )
{
@@ -288,6 +290,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+$|;
@@ -303,7 +306,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} );
}
@@ -732,6 +738,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));
--- a/t/db_dependent/Holds.t
+++ a/t/db_dependent/Holds.t
@@ -7,7 +7,7 @@ use t::lib::TestBuilder;
use C4::Context;
-use Test::More tests => 61;
+use Test::More tests => 59;
use MARC::Record;
use C4::Biblio;
@@ -244,19 +244,12 @@ is( $hold->priority, '6', "Test AlterPriority(), move to bottom" );
my $foreign_biblio = $builder->build_sample_biblio({ itemtype => 'DUMMY' });
my ($foreign_item_bibnum, $foreign_item_bibitemnum, $foreign_itemnumber)
= AddItem({ homebranch => $branch_2, holdingbranch => $branch_2 } , $foreign_biblio->biblionumber);
-# Cleanup circulation rules
-$dbh->do('DELETE FROM circulation_rules');
-# $dbh->do(
-# q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record)
-# VALUES (?, ?, ?, ?, ?)},
-# {},
-# '*', '*', '*', 25, 99
-# );
+delete_all_rules();
$dbh->do(
q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record)
VALUES (?, ?, ?, ?, ?)},
{},
- '*', '*', 'CANNOT', 0, 99
+ '*', '*', '*', 25, 99
);
# make sure some basic sysprefs are set
@@ -324,38 +317,169 @@ t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 0 );
ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'damaged', "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" );
ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled" );
-# Regression test for bug 9532
-$biblio = $builder->build_sample_biblio({ itemtype => 'CANNOT' });
-($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CANNOT' } , $biblio->biblionumber);
-AddReserve(
- $branch_1,
- $borrowernumbers[0],
- $biblio->biblionumber,
- '',
- 1,
-);
-is(
- CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves',
- "cannot request item if policy that matches on item-level item type forbids it"
-);
-ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber);
-ok(
- CanItemBeReserved( $borrowernumbers[0], $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);
-ModItem({ itype => undef }, $item_bibnum, $itemnumber);
-ok(
- CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'tooManyReserves',
- "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)"
-);
+ delete_all_rules();
+
+ 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};
+
+ $dbh->do(
+ q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {},
+ '*', '*', $itemtype_cant, 0, 99
+ );
+ $dbh->do(
+ q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {},
+ '*', '*', $itemtype_can, 2, 2
+ );
+ $dbh->do(
+ q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) VALUES (?, ?, ?, ?, ?)}, {},
+ '*', '*', $itemtype_cant_record, 0, 0
+ );
+ Koha::CirculationRules->set_rules(
+ {
+ branchcode => $branch_1,
+ itemtype => $itemtype_cant,
+ categorycode => undef,
+ rules => {
+ holdallowed => 0,
+ returnbranch => 'homebranch',
+ }
+ }
+ );
+ Koha::CirculationRules->set_rules(
+ {
+ branchcode => $branch_1,
+ itemtype => $itemtype_can,
+ categorycode => undef,
+ 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 ( undef, undef, $itemnumber_1_can ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_cannot);
+ my ( undef, undef, $itemnumber_1_cannot ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant } , $biblionumber_cannot);
+
+ my ( undef, undef, $itemnumber_2_can ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_can);
+ my ( undef, undef, $itemnumber_2_cannot ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant } , $biblionumber_can);
+
+ my ( undef, undef, $itemnumber_3_cannot ) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_cant_record } , $biblionumber_record_cannot);
+
+ 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 ( undef, undef, $itemnumber_11) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_1);
+ my ( undef, undef, $itemnumber_12) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_1);
+ my $biblionumber_2 = $builder->build_sample_biblio({ itemtype => $itemtype_can })->biblionumber;
+ my ( undef, undef, $itemnumber_21) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_2);
+ my ( undef, undef, $itemnumber_22) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, itype => $itemtype_can } , $biblionumber_2);
+
+ Koha::Holds->search({borrowernumber => $borrowernumbers[0]})->delete;
+
+ # Biblio-level hold
+ AddReserve(
+ $branch_1,
+ $borrowernumbers[0],
+ $biblionumber_1,
+ '',
+ 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_1,
+ $borrowernumbers[0],
+ $biblionumber_1,
+ '',
+ 1,
+ undef, undef, undef, undef, $itemnumber_11
+ );
+ $dbh->do(q{UPDATE issuingrules set 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"
+ );
+ $dbh->do(q{UPDATE issuingrules set 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"
+ );
+ $dbh->do(q{UPDATE issuingrules set 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_1,
+ $borrowernumbers[0],
+ $biblionumber_2,
+ '',
+ 1,
+ undef, undef, undef, undef, $itemnumber_21
+ );
+ $dbh->do(q{UPDATE issuingrules set 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
-$dbh->do('DELETE FROM issuingrules');
-$dbh->do('DELETE FROM circulation_rules');
+delete_all_rules();
$dbh->do(
q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed)
VALUES (?, ?, ?, ?)},
@@ -701,7 +825,7 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub {
# Cleanup database
Koha::Holds->search->delete;
$dbh->do('DELETE FROM issues');
- $dbh->do('DELETE FROM issuingrules');
+ delete_all_rules();
$dbh->do(
q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed)
VALUES (?, ?, ?, ?)},
@@ -789,7 +913,7 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub {
'Patron cannot place hold because patron\'s home library is not part of hold group'
);
- # Cleanup default_cirt_rules
+ # Cleanup default_circ_rules
$dbh->do('DELETE FROM circulation_rules');
# Insert default circ rule to "any" for library 2
@@ -845,6 +969,7 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub {
categorycode => undef,
rules => {
holdallowed => 2,
+ holdsperrecord => 1,
hold_fulfillment_policy => 'any',
returnbranch => 'any'
}
@@ -1114,7 +1239,6 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub {
'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2'
);
- # Cleanup default_branch_item_rules
$dbh->do('DELETE FROM circulation_rules');
# Insert branch item rule to "any" for itemtype 2 and library 2
@@ -1161,3 +1285,9 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub {
$schema->storage->txn_rollback;
};
+
+sub delete_all_rules {
+ my $dbh = C4::Context->dbh;
+ $dbh->do('DELETE FROM issuingrules');
+ $dbh->do('DELETE FROM circulation_rules');
+}
--