From ecd1ab3634cb7b11c716ac1919e57b711fa9eed2 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Tue, 21 Jun 2016 09:52:47 -0400
Subject: [PATCH] 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>
---
 C4/Reserves.pm                                |  22 +-
 .../prog/en/modules/reserve/request.tt        |  12 +-
 reserve/request.pl                            |   9 +-
 t/db_dependent/Holds.t                        | 227 ++++++++++++++++--
 4 files changed, 245 insertions(+), 25 deletions(-)

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index dc99bc1cbb..8941d4f72c 100644
--- a/C4/Reserves.pm
+++ b/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' };
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 963889cc68..c4eb1dc24d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/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 %]
diff --git a/reserve/request.pl b/reserve/request.pl
index dffe1da436..9e53e429e6 100755
--- a/reserve/request.pl
+++ b/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));
diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t
index 285d4d3663..64419b5fee 100755
--- a/t/db_dependent/Holds.t
+++ b/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
-- 
2.20.1