From 3084a13fc59e7e22e7aef14e3ae8c5c138384dea Mon Sep 17 00:00:00 2001
From: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Date: Wed, 26 Mar 2014 10:51:59 +0100
Subject: [PATCH] [SIGNED-OFF] Bug 11999: Add two checks in CanBookBeReserved
 and CanItemBeReserved

From: Julian Maurice <julian.maurice@biblibre.com>

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
---
 C4/Reserves.pm                                     | 36 +++++++++++++-
 .../prog/en/modules/reserve/request.tt             |  4 +-
 reserve/request.pl                                 | 19 ++------
 t/db_dependent/Holds.t                             | 55 +++++++++++++++-------
 4 files changed, 79 insertions(+), 35 deletions(-)

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index f405262..3682ad8 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -461,6 +461,20 @@ See CanItemBeReserved() for possible return values.
 sub CanBookBeReserved{
     my ($borrowernumber, $biblionumber) = @_;
 
+    # Check if borrower already has reserved the same biblio.
+    my @reserves = GetReservesFromBorrowernumber($borrowernumber);
+    foreach my $reserve (@reserves) {
+        if ($reserve->{biblionumber} == $biblionumber) {
+            return 'alreadyReserved';
+        }
+    }
+
+    # Check if borrower has reached the maximum number of holds allowed
+    my $maxreserves = C4::Context->preference('maxreserves');
+    if ($maxreserves && scalar(@reserves) >= $maxreserves) {
+        return 'tooManyReserves';
+    }
+
     my $items = GetItemnumbersForBiblio($biblionumber);
     #get items linked via host records
     my @hostitems = get_hostitemnumbers_of($biblionumber);
@@ -473,7 +487,8 @@ sub CanBookBeReserved{
         $canReserve = CanItemBeReserved( $borrowernumber, $item );
         return 'OK' if $canReserve eq 'OK';
     }
-    return $canReserve;
+
+    return 'none_available';
 }
 
 =head2 CanItemBeReserved
@@ -486,6 +501,7 @@ sub CanBookBeReserved{
          damaged,         if the Item is damaged.
          cannotReserveFromOtherBranches, if syspref 'canreservefromotherbranches' is OK.
          tooManyReserves, if the borrower has exceeded his maximum reserve amount.
+         alreadyReserved, if the borrower has already reserved this item.
 
 =cut
 
@@ -495,7 +511,7 @@ sub CanItemBeReserved{
     my $dbh             = C4::Context->dbh;
     my $ruleitemtype; # itemtype of the matching issuing rule
     my $allowedreserves = 0;
-            
+
     # we retrieve borrowers and items informations #
     # item->{itype} will come for biblioitems if necessery
     my $item = GetItem($itemnumber);
@@ -509,6 +525,22 @@ sub CanItemBeReserved{
     my ($ageRestriction, $daysToAgeRestriction) = C4::Circulation::GetAgeRestriction( $biblioData->{agerestriction}, $borrower );
     return 'ageRestricted' if $daysToAgeRestriction && $daysToAgeRestriction > 0;
 
+    # Check if borrower already has reserved the same item or biblio.
+    my @reserves = GetReservesFromBorrowernumber($borrowernumber);
+    foreach my $reserve (@reserves) {
+        if (   $reserve->{itemnumber} == $itemnumber
+            or $reserve->{biblionumber} == $item->{biblionumber} )
+        {
+            return 'alreadyReserved';
+        }
+    }
+
+    # Check if borrower has reached the maximum number of holds allowed
+    my $maxreserves = C4::Context->preference('maxreserves');
+    if ($maxreserves && scalar(@reserves) >= $maxreserves) {
+        return 'tooManyReserves';
+    }
+
     my $controlbranch = C4::Context->preference('ReservesControlBranch');
     my $itemtypefield = C4::Context->preference('item-level_itypes') ? "itype" : "itemtype";
 
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 db044b5..009db8d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -247,7 +247,7 @@ function checkMultiHold() {
     </form>
   [% ELSE %]
 
-[% IF ( maxreserves || alreadyreserved || none_available || alreadypossession || ageRestricted ) %]
+[% IF ( maxreserves || alreadyReserved || none_available || alreadypossession || ageRestricted ) %]
     <div class="dialog alert">
 
     [% UNLESS ( multi_hold ) %]
@@ -259,7 +259,7 @@ function checkMultiHold() {
         [% IF ( ageRestricted ) %]
           <li><strong>Age restricted</strong></li>
         [% END %]
-        [% IF ( alreadyreserved ) %]
+        [% IF ( alreadyReserved ) %]
           <li><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">[% borrowerfirstname %] [% borrowersurname %]</a> <strong>already has a hold</strong> on this item </li>
         [% END %]
         [% IF ( none_available ) %]
diff --git a/reserve/request.pl b/reserve/request.pl
index ce0e70f..9f3a189 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -199,7 +199,9 @@ foreach my $biblionumber (@biblionumbers) {
     elsif ( $canReserve eq 'tooManyReserves' ) {
         $maxreserves = 1;
     }
-    elsif ( $canReserve eq 'ageRestricted' ) {
+    elsif ( grep { $canReserve eq $_ }
+        (qw(ageRestricted alreadyReserved none_available)) )
+    {
         $template->param( $canReserve => 1 );
         $biblioloopiter{$canReserve} = 1;
     }
@@ -216,27 +218,14 @@ foreach my $biblionumber (@biblionumbers) {
     my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
     my $count = scalar( @$reserves );
     my $totalcount = $count;
-    my $holds_count = 0;
-    my $alreadyreserved = 0;
 
     foreach my $res (@$reserves) {
         if ( defined $res->{found} ) { # found can be 'W' or 'T'
             $count--;
         }
-
-        if ( defined $borrowerinfo && defined($borrowerinfo->{borrowernumber}) && ($borrowerinfo->{borrowernumber} eq $res->{borrowernumber}) ) {
-            $holds_count++;
-        }
-    }
-
-    if ( $holds_count ) {
-            $alreadyreserved = 1;
-            $biblioloopiter{warn} = 1;
-            $biblioloopiter{alreadyres} = 1;
     }
 
     $template->param(
-        alreadyreserved => $alreadyreserved,
         alreadypossession => $alreadypossession,
     );
 
@@ -420,7 +409,7 @@ foreach my $biblionumber (@biblionumbers) {
                      $borrowerinfo->{'branchcode'} ne $item->{'homebranch'} ) ) {
                 $policy_holdallowed = 0;
             }
-            
+
             if (
                    $policy_holdallowed
                 && !$item->{cantreserve}
diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t
index 4aee01b..5b31aad 100755
--- a/t/db_dependent/Holds.t
+++ b/t/db_dependent/Holds.t
@@ -6,7 +6,7 @@ use t::lib::Mocks;
 use C4::Context;
 use C4::Branch;
 
-use Test::More tests => 38;
+use Test::More tests => 41;
 use MARC::Record;
 use C4::Biblio;
 use C4::Items;
@@ -56,8 +56,8 @@ foreach (1..$borrowers_count) {
 
 my $biblionumber   = $bibnum;
 
-my @branches = GetBranchesLoop();
-my $branch = $branches[0][0]{value};
+my $branches = GetBranchesLoop();
+my $branch = $branches->[0]{value};
 
 # Create five item level holds
 foreach my $borrowernumber ( @borrowernumbers ) {
@@ -205,7 +205,7 @@ $dbh->do(
 );
 
 # make sure some basic sysprefs are set
-t::lib::Mocks::mock_preference('ReservesControlBranch', 'homebranch');
+t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary');
 t::lib::Mocks::mock_preference('item-level_itypes', 1);
 
 # if IndependentBranches is OFF, a CPL patron can reserve an MPL item
@@ -293,10 +293,12 @@ ModReserve({ reserve_id => $reserveid2, rank => 'del' });
 $reserve3 = GetReserve( $reserveid3 );
 is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" );
 
+ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" );
+
 ModItem({ damaged => 1 }, $item_bibnum, $itemnumber);
+CancelReserve({reserve_id => $reserveid3});
 C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 1 );
 ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" );
-ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" );
 C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 0 );
 ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber) 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" );
@@ -312,23 +314,35 @@ AddReserve(
     '',
     1,
 );
+my ($bibnum2, $title2, $bibitemnum2) = create_helper_biblio('CANNOT');
+my ($item_bibnum2, $item_bibitemnum2, $itemnumber2) = AddItem({ homebranch => $branch, holdingbranch => $branch, itype => 'CANNOT' } , $bibnum2);
 ok(
-    CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves',
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'tooManyReserves',
     "cannot request item if policy that matches on item-level item type forbids it"
 );
-ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber);
+ModItem({ itype => 'CAN' }, $item_bibnum2, $itemnumber2);
 ok(
-    CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'OK',
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'OK',
     "can request item if policy that matches on item type allows it"
 );
 
 t::lib::Mocks::mock_preference('item-level_itypes', 0);
-ModItem({ itype => undef }, $item_bibnum, $itemnumber);
+ModItem({ itype => undef }, $item_bibnum2, $itemnumber2);
 ok(
-    CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves',
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'tooManyReserves',
     "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)"
 );
 
+is(CanBookBeReserved($borrowernumbers[0], $bibnum), 'alreadyReserved');
+
+C4::Context->set_preference('maxreserves', 1);
+ok(CanBookBeReserved($borrowernumbers[0], $biblionumber) eq 'tooManyReserves');
+
+C4::Context->set_preference('maxreserves', 0);
+t::lib::Mocks::mock_preference('IndependentBranches', 1);
+t::lib::Mocks::mock_preference('canreservefromotherbranches', 0);
+ok(CanBookBeReserved($borrowernumbers[0], $foreign_bibnum) eq 'none_available');
+
 # Test CancelExpiredReserves
 C4::Context->set_preference('ExpireReservesMaxPickUpDelay', 1);
 C4::Context->set_preference('ReservesMaxPickUpDelay', 1);
@@ -370,10 +384,19 @@ sub create_helper_biblio {
     my $itemtype = shift;
     my $bib = MARC::Record->new();
     my $title = 'Silence in the library';
-    $bib->append_fields(
-        MARC::Field->new('100', ' ', ' ', a => 'Moffat, Steven'),
-        MARC::Field->new('245', ' ', ' ', a => $title),
-        MARC::Field->new('942', ' ', ' ', c => $itemtype),
-    );
-    return ($bibnum, $title, $bibitemnum) = AddBiblio($bib, '');
+    if (C4::Context->preference('marcflavour') eq 'UNIMARC') {
+        $bib->append_fields(
+            MARC::Field->new('700', ' ', '0', a => 'Moffat, Steven'),
+            MARC::Field->new('200', ' ', ' ', a => $title),
+            MARC::Field->new('099', ' ', ' ', t => $itemtype),
+        );
+    } else {
+        $bib->append_fields(
+            MARC::Field->new('100', ' ', ' ', a => 'Moffat, Steven'),
+            MARC::Field->new('245', ' ', ' ', a => $title),
+            MARC::Field->new('942', ' ', ' ', c => $itemtype),
+        );
+    }
+
+    return AddBiblio($bib, '');
 }
-- 
2.1.4