Bug 25184: Add syspref

It's entirely possible that some libraries are relying on the current
before for part of their workflow. Do to this possibility, it seems like
a good idea to control this behavior via a system preference.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Set TrapHoldsOnOrder to "don't trap"
4) Set an item's notforloan value to -1
5) Place a hold on that item
6) Check in the item
7) Note the item is not trapped for hold
9) Set TrapHoldsOnOrder to "trap"
10) Check in the item
11) Koha should now ask if you'd like to trap the item for the hold!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

C4/Reserves.pm

@@ -793,7 +793,9 @@ sub CheckReserves {
     return unless $itemnumber; # bail if we got nothing.
     # if item is not for loan it cannot be reserved either.....
     # except where items.notforloan < 0 :  This indicates the item is holdable.
-    return if $notforloan_per_item or $notforloan_per_itemtype;
+
+    my $dont_trap = C4::Context->preference('TrapHoldsOnOrder') ? ($notforloan_per_item > 0) : ($notforloan_per_item && 1 );
+    return if $dont_trap or $notforloan_per_itemtype;
 
     # Find this item in the reserves
     my @reserves = _Findgroupreserve( $bibitem, $biblio, $itemnumber, $lookahead_days, $ignore_borrowers);
@@ -803,6 +805,7 @@ sub CheckReserves {
     # the more important the item.)
     # $highest is the most important item we've seen so far.
     my $highest;
+
     if (scalar @reserves) {
         my $LocalHoldsPriority = C4::Context->preference('LocalHoldsPriority');
         my $LocalHoldsPriorityPatronControl = C4::Context->preference('LocalHoldsPriorityPatronControl');

installer/data/mysql/atomicupdate/bug_25184.perl

@@ -0,0 +1,11 @@
+$DBversion = 'XXX'; # will be replaced by the RM
+if( CheckVersion( $DBversion ) ) {
+    # $dbh->do( "ALTER TABLE biblio ADD COLUMN badtaste int" );
+    $dbh->do(q{
+        INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES
+        ('TrapHoldsOnOrder','1',NULL,'If enabled, Koha will trap holds for on order items ( notforloan < 0 )','YesNo't c)
+    });
+
+    # Always end with this (adjust the bug info)
+    NewVersion( $DBversion, 25184, "Items with a negative notforloan status should not be captured for holds");
+}

installer/data/mysql/sysprefs.sql

@@ -652,6 +652,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
 ('TransfersMaxDaysWarning','3',NULL,'Define the days before a transfer is suspected of having a problem','Integer'),
 ('TransferWhenCancelAllWaitingHolds','0',NULL,'Transfer items when cancelling all waiting holds','YesNo'),
 ('TranslateNotices','0',NULL, 'Allow notices to be translated','YesNo'),
+('TrapHoldsOnOrder','1',NULL,'If enabled, Koha will trap holds for on order items ( notforloan < 0 )','YesNo'),
 ('UNIMARCAuthorityField100','afrey50      ba0',NULL,'Define the contents of UNIMARC authority control field 100 position 08-35','Textarea'),
 ('UNIMARCAuthorsFacetsSeparator',', ',NULL,'UNIMARC authors facets separator','short'),
 ('UNIMARCField100Language','fre',NULL,'UNIMARC field 100 default language','short'),

koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref

@@ -514,6 +514,12 @@ Circulation:
             - "<br />itype: [NEWBK,\"\"]"
             - "<br /> Note: the word 'NULL' can be used to block renewal on undefined fields, while an empty string \"\" will block on an empty (but defined) field."
     Checkin policy:
+        -
+            - pref: TrapHoldsOnOrder
+              choices:
+                  yes: Trap
+                  no: "Don't trap"
+            - items that are not for loan but holdable ( notforloan < 0 ) to fill holds.
         -
             - pref: HoldsAutoFill
               choices:

t/db_dependent/Holds.t

@@ -7,7 +7,7 @@ use t::lib::TestBuilder;
 
 use C4::Context;
 
-use Test::More tests => 63;
+use Test::More tests => 64;
 use MARC::Record;
 
 use C4::Biblio;
@@ -345,6 +345,7 @@ ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'damaged',
 ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled" );
 
 # Items that are not for loan, but holdable should not be trapped until they are available for loan
+t::lib::Mocks::mock_preference( 'TrapHoldsOnOrder', 0 );
 Koha::Items->find($itemnumber)->damaged(0)->notforloan(-1)->store;
 Koha::Holds->search({ biblionumber => $biblio->id })->delete();
 is( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'OK', "Patron can place hold on item that is not for loan but holdable ( notforloan < 0 )" );
@@ -353,9 +354,15 @@ $hold = Koha::Hold->new(
         borrowernumber => $borrowernumbers[0],
         itemnumber     => $itemnumber,
         biblionumber   => $biblio->biblionumber,
+        found          => undef,
+        priority       => 1,
+        reservedate    => dt_from_string,
+        branchcode     => $branch_1,
     }
 )->store();
 ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for item that is not for loan but holdable ( notforloan < 0 )" );
+t::lib::Mocks::mock_preference( 'TrapHoldsOnOrder', 1 );
+ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold is trapped for item that is not for loan but holdable ( notforloan < 0 )" );
 $hold->delete();
 
 # Regression test for bug 9532