Attachment 138561 Details for Bug 27272 – Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve

[patch] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve

Bug-27272-Remove-GetHostItemsInfo-and-GetItemsInfo.patch (text/plain), 21.95 KB, created by Victor Grousset/tuxayo on 2022-08-03 23:41:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Victor Grousset/tuxayo

Created: 2022-08-03 23:41:05 UTC

Size: 21.95 KB

patch

obsolete

>From 02a45f0ce27c02e1c57a0ffd9607adc0594df98a 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 27272: Remove GetHostItemsInfo and GetItemsInfo from
> opac-reserve
>
>---
> .../bootstrap/en/modules/opac-reserve.tt | 34 ++--
> opac/opac-reserve.pl | 158 ++++++------------
> 2 files changed, 63 insertions(+), 129 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 e16095740e..48cee6cd17 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 ) %]
>- Due [% itemLoo.dateDue | html %]
>+ [% IF ( itemLoo.checkout.date_due) %]
>+ Due [% itemLoo.checkout.date_due| $KohaDates as_due_date => 1 %]
> [% ELSIF ( itemLoo.transfertwhen ) %]
>- In transit from [% Branches.GetName( itemLoo.transfertfrom ) | html %] to [% Branches.GetName( itemLoo.transfertto ) | html %] since [% itemLoo.transfertwhen | html %]
>+ In transit from [% Branches.GetName( itemLoo.transfertfrom ) | html %] to [% Branches.GetName( itemLoo.transfertto ) | html %] since [% itemLoo.transfertwhen | $KohaDates %]
> [% 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? %]
> Unavailable (lost or missing)
> [% END %]
> 
> [% IF ( itemLoo.notforloan ) %]
>- Not for loan ([% itemLoo.notforloanvalue | html %])
>+ Not for loan ([% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.notforloan', authorised_value => itemLoo.notforloan ) %] %])
> [% END %]
> 
>- [% IF ( itemLoo.reservedate ) %]
>+ [% IF ( itemLoo.first_hold ) %]
> 
> [% IF ( itemLoo.waitingdate ) %]
> Waiting
>@@ -448,18 +448,16 @@
> [% ELSE %]
> expected at
> [% END %]
>- [% itemLoo.ExpectedAtLibrary | html %] since
>+ [% Branches.GetName( itemLoo.branchcode ) | html %] since
> [% IF ( itemLoo.waitingdate ) %]
> [% itemLoo.waitingdate | $KohaDates %]
>- [% ELSE %]
>- [% IF ( itemLoo.reservedate ) %]
>- [% itemLoo.reservedate | html %]
>- [% END %]
>+ [% ELSIF ( itemLoo.reservedate ) %]
>+ [% itemLoo.reservedate | $KohaDates %]
> [% END %].
> 
> [% ELSE %]
> Not on hold
>- [% 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 2dfbed123a..9e907d1f5b 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(
>+ {
>+ 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} = output_pref({ dt => dt_from_string($issue->date_due, 'sql'), as_due_date => 1 });
>- $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} = output_pref({ dt => dt_from_string($first_hold->reservedate), dateonly => 1 }); # FIXME Should be formatted in the template
>- $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,
>-- 
>2.37.1

Actions: View | Diff | Splinter Review

Attachments on bug 27272: 128230 | 128231 | 135640 | 135641 | 135642 | 135643 | 135644 | 135645 | 135646 | 135647 | 135648 | 135649 | 135650 | 135651 | 135652 | 135653 | 135654 | 135655 | 135656 | 135657 | 135658 | 135659 | 137966 | 137967 | 137968 | 137969 | 137970 | 137971 | 137972 | 137973 | 137974 | 137975 | 137976 | 137977 | 137978 | 137979 | 137980 | 137981 | 137982 | 138109 | 138296 | 138297 | 138298 | 138303 | 138304 | 138305 | 138306 | 138307 | 138308 | 138437 | 138438 | 138439 | 138440 | 138441 | 138442 | 138553 | 138554 | 138555 | 138556 | 138557 | 138558 | 138559 | 138560 | 138561 | 138562 | 138563 | 138564 | 138565 | 138566 | 138567 | 138568 | 138569 | 138570 | 138571 | 138572 | 138573 | 138574 | 138575 | 138614 | 138615 | 138616 | 138617 | 138618 | 138619 | 138620 | 138621 | 138622 | 138623 | 138624 | 138625 | 138626 | 138627 | 138628 | 138629 | 138630 | 138631 | 138632 | 138633 | 138634 | 138635 | 138636 | 138714 | 138715 | 138716 | 138717 | 138718 | 138719 | 138720 | 138721 | 138722 | 138723 | 138724 | 138725 | 138726 | 138727 | 138728 | 138729 | 138730 | 138731 | 138732 | 138733 | 138734 | 138735 | 138736 | 138773 | 138774 | 138775 | 138776 | 138777 | 138778 | 138779 | 138780 | 138781 | 138782 | 138783 | 138784 | 138785 | 138786 | 138787 | 138788 | 138789 | 138790 | 138791 | 138792 | 138793 | 138794 | 138832 | 139928