From 034ae7b683ced5121c819decc240387628181340 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Fri, 3 Jun 2022 07:44:46 +0200
Subject: [PATCH] Bug 31314: Remove GetHostItemsInfo and GetItemsInfo from
 opac-reserve

Bug 27272 is going to remove C4::Items::GetItemsInfo in favour of Koha::Items->search_ordered.

Here we are going to deal with opac-reserve

Test plan:
List items on the modified view and confirm that all the info is
displayed correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
---
 .../bootstrap/en/modules/opac-reserve.tt      |  26 +--
 opac/opac-reserve.pl                          | 160 ++++++------------
 2 files changed, 61 insertions(+), 125 deletions(-)

diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
index 8bb46e8ff7..1d1d55fe4d 100644
--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
@@ -370,7 +370,7 @@
                                                     [% SET unholdable_items = 0 %]
                                                     [% FOREACH itemLoo IN bibitemloo.itemLoop %]
                                                         [% IF ( itemLoo.available ) %]
-                                                            [% IF ( itemLoo.onloan ) %]
+                                                            [% IF ( itemLoo.checkout ) %]
                                                                 <tr class="holdable onloan">
                                                             [% ELSE %]
                                                                 <tr class="holdable">
@@ -379,7 +379,7 @@
                                                                         <input type="radio" class="checkitem checkitem_[% bibitemloo.biblionumber | html %]" name="checkitem_[% bibitemloo.biblionumber | html %]" value="[% itemLoo.itemnumber | html %]" />
                                                         [% ELSE %]
                                                             [% SET unholdable_items = 1 %]
-                                                            [% IF ( itemLoo.onloan ) %]
+                                                            [% IF ( itemLoo.checkout ) %]
                                                                 <tr class="unholdable onloan" style="display:none;">
                                                             [% ELSE %]
                                                                 <tr class="unholdable" style="display:none;">
@@ -401,19 +401,19 @@
                                                                             <img src="[% itemLoo.imageurl | html %]" alt="" />
                                                                         [% END %]
                                                                     [% END %]
-                                                                    [% itemLoo.translated_description | html %]
+                                                                    [% itemLoo.itemtype.translated_description | html %]
                                                                 </td>
                                                             [% END %]
 
                                                             <td class="barcode">[% itemLoo.barcode | html %]</td>
                                                             [% UNLESS ( singleBranchMode ) %]
-                                                                <td class="homebranch">[% Branches.GetName( itemLoo.homeBranchName ) | html %]</td>
-                                                                <td class="holdingbranch">[% Branches.GetName( itemLoo.holdingBranchName ) | html %]</td>
+                                                                <td class="homebranch">[% Branches.GetName( itemLoo.homebranch) | html %]</td>
+                                                                <td class="holdingbranch">[% Branches.GetName( itemLoo.holdingbranch ) | html %]</td>
                                                             [% END %]
                                                             [% IF ( itemdata_ccode ) %]
                                                                 <td class="ccode"> [% IF ( itemLoo.ccode ) %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => itemLoo.ccode, opac => 1 ) | html %][% END %]</td>
                                                             [% END %]
-                                                            <td class="call_no">[% itemLoo.callNumber | html %]</td>
+                                                            <td class="call_no">[% itemLoo.itemcallnumber | html %]</td>
                                                             [% IF ( itemdata_enumchron ) %]
                                                                 <td class="vol_info">[% itemLoo.enumchron | html %]</td>
                                                             [% END %]
@@ -421,21 +421,21 @@
                                                                 [% itemLoo.itemnotes | html %]
                                                             </td>
                                                             <td class="information">
-                                                                [% IF ( itemLoo.dateDue ) %]
-                                                                    <span class="checkedout">Due [% itemLoo.dateDue | $KohaDates as_due_date => 1 %]</span>
+                                                                [% IF ( itemLoo.checkout.date_due) %]
+                                                                    <span class="checkedout">Due [% itemLoo.checkout.date_due| $KohaDates as_due_date => 1 %]</span>
                                                                 [% ELSIF ( itemLoo.transfertwhen ) %]
-                                                                    <span class="intransit">In transit from [% Branches.GetName( itemLoo.transfertfrom ) | html %] to [% Branches.GetName( itemLoo.transfertto ) | html %] since [% itemLoo.transfertwhen | html %]</span>
+                                                                    <span class="intransit">In transit from [% Branches.GetName( itemLoo.transfertfrom ) | html %] to [% Branches.GetName( itemLoo.transfertto ) | html %] since [% itemLoo.transfertwhen | $KohaDates %]</span>
                                                                 [% END %]
 
-                                                                [% IF ( itemLoo.message ) %]
+                                                                [% IF ( itemLoo.itemlost == 1 || itemLoo.itemlost == 2 ) %] [%# FIXME Why only for 1 or 2? Shouldn't we test for withdrawn as well? %]
                                                                     <span class="lost">Unavailable (lost or missing)</span>
                                                                 [% END %]
 
                                                                 [% IF ( itemLoo.notforloan ) %]
-                                                                    <span class="notforloan">Not for loan ([% itemLoo.notforloanvalue | html %])</span>
+                                                                    <span class="notforloan">Not for loan ([% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.notforloan', authorised_value => itemLoo.notforloan ) %] %])</span>
                                                                 [% END %]
 
-                                                                [% IF ( itemLoo.reservedate ) %]
+                                                                [% IF ( itemLoo.first_hold ) %]
                                                                     <span class="waiting">
                                                                         [% IF ( itemLoo.waitingdate ) %]
                                                                             <span>Waiting for patron at [% Branches.GetName( itemLoo.ExpectedAtLibrary ) | html %] since [% itemLoo.waitingdate | $KohaDates %]</span>
@@ -447,7 +447,7 @@
                                                                     </span>
                                                                 [% ELSE %]
                                                                     <span class="notonhold">Not on hold</span>
-                                                                [% END # / IF ( itemLoo.reservedate )%]
+                                                                [% END # / IF ( itemLoo.first_hold )%]
                                                             </td>
                                                         </tr>
                                                     [% END # / FOREACH itemLoo IN bibitemloo.itemLoop%]
diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index a4cacfe50f..6cdbbbd3df 100755
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -26,7 +26,6 @@ use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc );
 use C4::Circulation qw( GetBranchItemRule GetTransfers );
 use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve GetReservesControlBranch ItemsAnyAvailableAndNotRestricted IsAvailableForItemLevelRequest );
 use C4::Biblio qw( GetBiblioData GetFrameworkCode );
-use C4::Items qw( GetHostItemsInfo GetItemsInfo );
 use C4::Output qw( output_html_with_http_headers );
 use C4::Context;
 use C4::Members;
@@ -142,30 +141,32 @@ $template->param( branch => $branch );
 #
 
 my %biblioDataHash; # Hash of biblionumber to biblio/biblioitems record.
-my %itemInfoHash; # Hash of itemnumber to item info.
 foreach my $biblioNumber (@biblionumbers) {
 
     my $biblioData = GetBiblioData($biblioNumber);
     $biblioDataHash{$biblioNumber} = $biblioData;
 
-    my @itemInfos = GetItemsInfo($biblioNumber);
-
     my $biblio = Koha::Biblios->find( $biblioNumber );
     next unless $biblio;
 
     my $marcrecord = $biblio->metadata->record;
 
-    # flag indicating existence of at least one item linked via a host record
-    # adding items linked via host biblios
-    my @hostitemInfos = GetHostItemsInfo($marcrecord);
-    if (@hostitemInfos){
-        push (@itemInfos,@hostitemInfos);
-    }
+    my $items =
+      $biblio->items->filter_by_visible_in_opac( { patron => $patron } );
+    my $host_items =
+      $biblio->host_items->filter_by_visible_in_opac( { patron => $patron } );
+    $items = $biblio->items->search_ordered(
+        {
+            itemnumber => {
+                -in => [
+                    $items->get_column('itemnumber'),
+                    $host_items->get_column('itemnumber')
+                ]
+            }
+        }
+    );
 
-    $biblioData->{itemInfos} = \@itemInfos;
-    foreach my $itemInfo (@itemInfos) {
-        $itemInfoHash{$itemInfo->{itemnumber}} = $itemInfo;
-    }
+    $biblioData->{items} = [$items->as_list]; # FIXME Potentially a lot in memory here!
 
     # Compute the priority rank.
     $biblioData->{object} = $biblio;
@@ -237,6 +238,7 @@ if ( $query->param('place_reserve') ) {
             $branch = $patron->branchcode;
         }
 
+        # FIXME We shouldn't need to fetch the item here
         my $item = $itemNum ? Koha::Items->find( $itemNum ) : undef;
         # When choosing a specific item, the default pickup library should be dictated by the default hold policy
         if ( ! C4::Context->preference("OPACAllowUserToChooseBranch") && $item ) {
@@ -411,6 +413,7 @@ my $itemLevelTypes = C4::Context->preference('item-level_itypes');
 my $pickup_locations = Koha::Libraries->search({ pickup_location => 1 });
 $template->param('item_level_itypes' => $itemLevelTypes);
 
+my $patron_unblessed = $patron->unblessed;
 foreach my $biblioNum (@biblionumbers) {
 
     # Init the bib item with the choices for branch pickup
@@ -448,25 +451,7 @@ foreach my $biblioNum (@biblionumbers) {
         $biblioLoopIter{imageurl} = getitemtypeimagesrc() . "/". $itemtypes->{$biblioData->{itemtype}}{imageurl};
     }
 
-    foreach my $itemInfo (@{$biblioData->{itemInfos}}) {
-        if ($itemLevelTypes && $itemInfo->{itype}) {
-            $itemInfo->{translated_description} = $itemtypes->{$itemInfo->{itype}}{translated_description};
-            $itemInfo->{imageurl} = getitemtypeimagesrc() . "/". $itemtypes->{$itemInfo->{itype}}{imageurl};
-        }
-
-        if (!$itemInfo->{'notforloan'} && !($itemInfo->{'itemnotforloan'} > 0)) {
-            $biblioLoopIter{forloan} = 1;
-        }
-    }
-
-    my @notforloan_avs = Koha::AuthorisedValues->search_by_koha_field({ kohafield => 'items.notforloan', frameworkcode => $frameworkcode })->as_list;
-    my $notforloan_label_of = { map { $_->authorised_value => $_->opac_description } @notforloan_avs };
-
-    my $visible_items = { map { $_->itemnumber => $_ } $biblio->items->filter_by_visible_in_opac( { patron => $patron } )->as_list };
 
-    # Only keep the items that are visible in the opac (i.e. those in %visible_items)
-    # FIXME: We should get rid of itemInfos altogether and use $visible_items
-    $biblioData->{itemInfos} = [ grep { $visible_items->{ $_->{itemnumber} } } @{ $biblioData->{itemInfos} } ];
 
     $biblioLoopIter{itemLoop} = [];
     my $numCopiesAvailable = 0;
@@ -477,91 +462,44 @@ foreach my $biblioNum (@biblionumbers) {
     # (before this loop was inside that sub loop so it was O(n^2) )
     my $items_any_available;
     $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblioNum, patron => $patron }) if $patron;
-    foreach my $itemInfo (@{$biblioData->{itemInfos}}) {
-        my $itemNum = $itemInfo->{itemnumber};
-        my $item = $visible_items->{$itemNum};
-        my $itemLoopIter = {};
-
-        $itemLoopIter->{itemnumber} = $itemNum;
-        $itemLoopIter->{barcode} = $itemInfo->{barcode};
-        $itemLoopIter->{homeBranchName} = $itemInfo->{homebranch};
-        $itemLoopIter->{callNumber} = $itemInfo->{itemcallnumber};
-        $itemLoopIter->{enumchron} = $itemInfo->{enumchron};
-        $itemLoopIter->{ccode} = $itemInfo->{ccode};
-        $itemLoopIter->{copynumber} = $itemInfo->{copynumber};
-        $itemLoopIter->{itemnotes} = $itemInfo->{itemnotes};
-        if ($itemLevelTypes) {
-            $itemLoopIter->{translated_description} = $itemInfo->{translated_description};
-            $itemLoopIter->{itype} = $itemInfo->{itype};
-            $itemLoopIter->{imageurl} = $itemInfo->{imageurl};
-        }
-
-        # If the holdingbranch is different than the homebranch, we show the
-        # holdingbranch of the document too.
-        if ( $itemInfo->{homebranch} ne $itemInfo->{holdingbranch} ) {
-            $itemLoopIter->{holdingBranchName} = $itemInfo->{holdingbranch};
-        }
+    foreach my $item (@{$biblioData->{items}}) {
 
-        # If the item is currently on loan, we display its return date and
-        # change the background color.
-        my $issue = Koha::Checkouts->find( { itemnumber => $itemNum } );
-        if ( $issue ) {
-            $itemLoopIter->{dateDue} = $issue->date_due;
-            $itemLoopIter->{onloan} = 'onloan';
+        my $item_info = $item->unblessed;
+        $item_info->{holding_branch} = $item->holding_branch;
+        $item_info->{home_branch}    = $item->home_branch;
+        if ($itemLevelTypes) {
+            my $itemtype = $item->itemtype;
+            $item_info->{'imageurl'} = getitemtypeimagelocation( 'opac',
+                $itemtypes->{ $itemtype->itemtype }->{'imageurl'} );
+            $item_info->{'translated_description'} =
+              $itemtypes->{ $itemtype->itemtype }->{translated_description};
         }
 
-        # checking reserve
+        # checking for holds
         my $holds = $item->current_holds;
-
         if ( my $first_hold = $holds->next ) {
-            $itemLoopIter->{backgroundcolor} = 'reserved';
-            $itemLoopIter->{reservedate}     = $first_hold->reservedate;
-            $itemLoopIter->{ExpectedAtLibrary}         = $first_hold->branchcode;
-            $itemLoopIter->{waitingdate} = $first_hold->waitingdate;
+            $item_info->{first_hold} = $first_hold;
         }
 
-        $itemLoopIter->{notforloan} = $itemInfo->{notforloan};
-        $itemLoopIter->{itemnotforloan} = $itemInfo->{itemnotforloan};
-
-        # Management of the notforloan document
-        if ( $itemLoopIter->{notforloan} || $itemLoopIter->{itemnotforloan}) {
-            $itemLoopIter->{backgroundcolor} = 'other';
-            $itemLoopIter->{notforloanvalue} =
-              $notforloan_label_of->{ $itemLoopIter->{notforloan} };
-        }
-
-        # Management of lost or long overdue items
-        if ( $itemInfo->{itemlost} ) {
-
-            # FIXME localized strings should never be in Perl code
-            $itemLoopIter->{message} =
-                $itemInfo->{itemlost} == 1 ? "(lost)"
-              : $itemInfo->{itemlost} == 2 ? "(long overdue)"
-              : "";
-            $itemInfo->{backgroundcolor} = 'other';
-        }
+        $item_info->{checkout} = $item->checkout;
 
         # Check of the transferred documents
         my ( $transfertwhen, $transfertfrom, $transfertto ) =
-          GetTransfers($itemNum);
+          GetTransfers($item->itemnumber);
         if ( $transfertwhen && ($transfertwhen ne '') ) {
-            $itemLoopIter->{transfertwhen} = output_pref({ dt => dt_from_string($transfertwhen), dateonly => 1 });
-            $itemLoopIter->{transfertfrom} = $transfertfrom;
-            $itemLoopIter->{transfertto} = $transfertto;
-            $itemLoopIter->{nocancel} = 1;
+            $item_info->{transfertwhen} = $transfertwhen;
+            $item_info->{transfertfrom} = $transfertfrom;
+            $item_info->{transfertto} = $transfertto;
+            $item_info->{nocancel} = 1;
         }
 
         # if the items belongs to a host record, show link to host record
-        if ( $itemInfo->{biblionumber} ne $biblioNum ) {
-            $itemLoopIter->{hostbiblionumber} = $itemInfo->{biblionumber};
-            $itemLoopIter->{hosttitle}        = Koha::Biblios->find( $itemInfo->{biblionumber} )->title;
+        if ( $item_info->{biblionumber} ne $biblioNum ) {
+            $item_info->{hostbiblionumber} = $item->biblionumber;
+            $item_info->{hosttitle}        = Koha::Biblios->find( $item_info->{biblionumber} )->title;
         }
 
-        # If there is no loan, return and transfer, we show a checkbox.
-        $itemLoopIter->{notforloan} = $itemLoopIter->{notforloan} || 0;
-
-        my $patron_unblessed = $patron->unblessed;
-        my $branch = GetReservesControlBranch( $itemInfo, $patron_unblessed );
+        my $branch = GetReservesControlBranch( $item_info, $patron_unblessed );
 
         # items_any_available defined outside of the current loop,
         # so we avoiding loop inside IsAvailableForItemLevelRequest:
@@ -572,33 +510,31 @@ foreach my $biblioNum (@biblionumbers) {
         if ($policy_holdallowed) {
             my $opac_hold_policy = Koha::CirculationRules->get_opacitemholds_policy( { item => $item, patron => $patron } );
             if ( $opac_hold_policy ne 'N' ) { # If Y or F
-                $itemLoopIter->{available} = 1;
+                $item_info->{available} = 1;
                 $numCopiesOPACAvailable++;
                 $biblioLoopIter{force_hold} = 1 if $opac_hold_policy eq 'F';
             }
             $numCopiesAvailable++;
 
             unless ( $can_place_hold_if_available_at_pickup ) {
-                my $items_in_this_library = Koha::Items->search({ biblionumber => $itemInfo->{biblionumber}, holdingbranch => $itemInfo->{holdingbranch} });
+                my $items_in_this_library = Koha::Items->search({ biblionumber => $item->biblionumber, holdingbranch => $item->holdingbranch });
                 my $nb_of_items_issued = $items_in_this_library->search({ 'issue.itemnumber' => { not => undef }}, { join => 'issue' })->count;
                 if ( $items_in_this_library->count > $nb_of_items_issued ) {
-                    push @not_available_at, $itemInfo->{holdingbranch};
+                    push @not_available_at, $item->holdingbranch;
                 }
             }
         }
 
-        $itemLoopIter->{imageurl} = getitemtypeimagelocation( 'opac', $itemtypes->{ $itemInfo->{itype} }{imageurl} );
-
-    # Show serial enumeration when needed
-        if ($itemLoopIter->{enumchron}) {
+        # Show serial enumeration when needed
+        if ($item_info->{enumchron}) {
             $itemdata_enumchron = 1;
         }
-    # Show collection when needed
-        if ($itemLoopIter->{ccode}) {
+        # Show collection when needed
+        if ($item_info->{ccode}) {
             $itemdata_ccode = 1;
         }
 
-        push @{$biblioLoopIter{itemLoop}}, $itemLoopIter;
+        push @{$biblioLoopIter{itemLoop}}, $item_info;
     }
     $template->param(
         itemdata_enumchron => $itemdata_enumchron,
@@ -635,7 +571,7 @@ foreach my $biblioNum (@biblionumbers) {
 
     if ( $biblioLoopIter{holdable} and C4::Context->preference('AllowHoldItemTypeSelection') ) {
         # build the allowed item types loop
-        my $rs = $biblio->items->search(
+        my $rs = $biblio->items->search_ordered(
             undef,
             {   select => [ { distinct => 'itype' } ],
                 as     => 'item_type'
-- 
2.30.2