Bug 26139: Centralize code for "Place hold" button (detail)

There is a "norequest" boolean passed to the include cat-toolbar.inc, to
display or not the "Pace hold" button.
This flag was not calculated in some place (ISBDdetail and moredetail
for instance)

Here we centralize the code to make it more robust and less regression
prone.

Note that the same problem appears at the OPAC (opac-MARCdetail is
always display the button). That will have to be fixed separately

Test plan:
Create biblio A with 0 item, B and C with 1 item and D with 2 items
item for B is not for loan, C is for loan and D has 1 of each
Go to the bibliographic detail page of each record and confirm that the
"Place hold" button appears appropriately.
Test the different tabs of the catalogue module

QA note: This patch is only centralizing the existing code, but it is
still too naive. To manage the visibility of this button we certainly
want to copy what is done in C4::Search::searchResults:
  # can place a hold on a item if
  # not lost nor withdrawn
  # not damaged unless AllowHoldsOnDamagedItems is true
  # item is either for loan or on order (notforloan < 0)
  $can_place_holds = 1
    if (
         !$item->{itemlost}
      && !$item->{withdrawn}
      && ( !$item->{damaged} || C4::Context->preference('AllowHoldsOnDamagedItems') )
      && ( !$item->{notforloan} || $item->{notforloan} < 0 )
    );

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Koha/Items.pm

@@ -37,6 +37,19 @@ Koha::Items - Koha Item object set class
 
 =cut
 
+=head3 filter_by_for_loan
+
+$items->filter_by_not_for_loan;
+
+Return the items of the set that are not for loan
+
+=cut
+
+sub filter_by_for_loan {
+    my ($self) = @_;
+    return $self->search( { notforloan => [ 0, undef ] } );
+}
+
 =head3 type
 
 =cut

catalogue/MARCdetail.pl

@@ -267,7 +267,6 @@ my %witness
   ; #---- stores the list of subfields used at least once, with the "meaning" of the code
 my @item_subfield_codes;
 my @item_loop;
-my $norequests = 1;
 
 foreach my $field (@fields) {
     next if ( $field->tag() < 10 );
@@ -295,8 +294,6 @@ foreach my $field (@fields) {
             $item->{ $subf[$i][0] } .= GetAuthorisedValueDesc( $field->tag(), $subf[$i][0], $subf[$i][1], '', $tagslib) || $subf[$i][1];
         }
 
-        $norequests = 0 if  $tagslib->{ $field->tag() }->{ $subf[$i][0] }->{kohafield} eq 'items.notforloan' and $subf[$i][1] == 0;
-
         my $kohafield = $tagslib->{ $field->tag() }->{ $subf[$i][0] }->{kohafield};
         $item->{ $subf[$i][0] } = output_pref( { str => $item->{ $subf[$i][0] }, dateonly => 1 } )
           if grep { $kohafield eq $_ }
@@ -330,7 +327,6 @@ if ($subscriptionscount) {
 }
 
 $template->param (
-    norequests              => $norequests,
     item_loop               => \@item_loop,
     item_header_loop        => \@item_header_loop,
     item_subfield_codes     => \@item_subfield_codes,

catalogue/detail.pl

@@ -270,7 +270,6 @@ my $collections =
 my $copynumbers =
   { map { $_->{authorised_value} => $_->{lib} } Koha::AuthorisedValues->get_descriptions_by_koha_field( { frameworkcode => $fw, kohafield => 'items.copynumber' } ) };
 my (@itemloop, @otheritemloop, %itemfields);
-my $norequests = 1;
 
 my $mss = Koha::MarcSubfieldStructures->search({ frameworkcode => $fw, kohafield => 'items.itemlost', authorised_value => [ -and => {'!=' => undef }, {'!=' => ''}] });
 if ( $mss->count ) {
@@ -306,9 +305,6 @@ my $separatebranch = C4::Context->preference('SeparateHoldingsBranch') || 'homeb
 foreach my $item (@items) {
     my $itembranchcode = $item->{$separatebranch};
 
-    # can place holds defaults to yes
-    $norequests = 0 unless ( ( $item->{'notforloan'} > 0 ) || ( $item->{'itemnotforloan'} > 0 ) );
-
     $item->{imageurl} = defined $item->{itype} ? getitemtypeimagelocation('intranet', $itemtypes->{ $item->{itype} }{imageurl})
                                                : '';
 
@@ -409,7 +405,6 @@ if (scalar(@itemloop) == 0 || scalar(@otheritemloop) == 0) {
     }
 }
 
-$template->param( norequests => $norequests );
 $template->param(
     MARCNOTES   => $marcnotesarray,
     itemdata_ccode      => $itemfields{ccode},

catalogue/imageviewer.pl

@@ -47,15 +47,6 @@ my $biblio = Koha::Biblios->find( $biblionumber );
 my $itemcount = $biblio ? $biblio->items->count : 0;
 my @items = GetItemsInfo($biblionumber);
 
-my $norequests = 1;
-foreach my $item (@items) {
-
-    # can place holds defaults to yes
-    $norequests = 0
-      unless ( ( $item->{'notforloan_per_itemtype'} > 0 )
-        || ( $item->{'itemnotforloan'} > 0 ) );
-}
-
 if ( $query->cookie("holdfor") ) {
     my $holdfor_patron = Koha::Patrons->find( $query->cookie("holdfor") );
     $template->param(
@@ -82,7 +73,6 @@ if ( C4::Context->preference("LocalCoverImages") ) {
 }
 $template->{VARS}->{'count'}        = $itemcount;
 $template->{VARS}->{'biblionumber'} = $biblionumber;
-$template->{VARS}->{'norequests'}   = $norequests;
 $template->param(C4::Search::enabled_staff_search_views);
 $template->{VARS}->{'biblio'} = $biblio;
 

koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc

@@ -1,3 +1,4 @@
+[% USE Context %]
 [% INCLUDE 'blocking_errors.inc' %]
 <div id="toolbar" class="btn-toolbar">
 
@@ -116,7 +117,9 @@ CAN_user_serials_create_subscription ) %]
     <div class="btn-group"><a id="printbiblio" class="btn btn-default"><i class="fa fa-print"></i> Print</a></div>
 
 [% IF ( CAN_user_reserveforothers ) %]
-    [% UNLESS ( norequests ) %]
+    [%# biblio.items.filter_by_for_loan.count %]
+    [% SET items = biblio.items %]
+    [% IF Context.Scalar(Context.Scalar(items, "filter_by_for_loan"), "count") %]
         [% IF ( holdfor ) %]
             <div class="btn-group">
                 <button class="btn btn-default dropdown-toggle" data-toggle="dropdown">

t/db_dependent/Koha/Items.t

@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 11;
+use Test::More tests => 12;
 use Test::Exception;
 use Time::Fake;
 
@@ -926,6 +926,19 @@ subtest 'can_be_transferred' => sub {
        'We get the same result also if we pass the from-library parameter.');
 };
 
+subtest 'filter_by_for_loan' => sub {
+    plan tests => 3;
+
+    my $biblio = $builder->build_sample_biblio;
+    is( $biblio->items->filter_by_for_loan->count, 0, 'no item yet' );
+    $builder->build_sample_item( { biblionumber => $biblio->biblionumber, notforloan => 1 } );
+    is( $biblio->items->filter_by_for_loan->count, 0, 'no item for loan' );
+    $builder->build_sample_item( { biblionumber => $biblio->biblionumber, notforloan => 0 } );
+    is( $biblio->items->filter_by_for_loan->count, 1, '1 item for loan' );
+
+    $biblio->delete;
+};
+
 # Reset nb_of_items prior to testing delete
 $nb_of_items = Koha::Items->search->count;