Attachment 153880 Details for Bug 28966 – Bug 28966: Prefetch patron data for holds queue viewer

[patch] Bug 28966: Prefetch patron data for holds queue viewer

Bug-28966-Prefetch-patron-data-for-holds-queue-vie.patch (text/plain), 17.14 KB, created by Marcel de Rooy on 2023-07-25 14:06:45 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2023-07-25 14:06:45 UTC

Size: 17.14 KB

patch

obsolete

>From 28478bdacc964775e5db26f8e67bde99bea97e11 Mon Sep 17 00:00:00 2001
>From: Kyle Hall <kyle@bywatersolutions.com>
>Date: Wed, 9 Nov 2022 13:11:32 -0500
>Subject: [PATCH] Bug 28966: Prefetch patron data for holds queue viewer
>Content-Type: text/plain; charset=utf-8
>
>Test Plan:
>1) Generate the holds queue
>2) Load the holds queue viewer page
>3) Apply this patch
>4) Restart all the things!
>5) Reload the page
>6) Note nothing has changed
>
>Signed-off-by: Owen Leonard <oleonard@myacpl.org>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> C4/HoldsQueue.pm                              | 80 ++++++++-----------
> Koha/Hold/HoldsQueueItem.pm                   | 39 +++++++++
> Koha/Schema/Result/TmpHoldsqueue.pm           | 48 +++++++++++
> circ/view_holdsqueue.pl                       | 34 ++++----
> .../prog/en/modules/circ/view_holdsqueue.tt   | 36 ++++-----
> t/db_dependent/HoldsQueue.t                   | 22 ++---
> 6 files changed, 168 insertions(+), 91 deletions(-)
>
>diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm
>index 6f0c755360..38f8e3e5ae 100644
>--- a/C4/HoldsQueue.pm
>+++ b/C4/HoldsQueue.pm
>@@ -25,7 +25,7 @@ use warnings;
> use C4::Context;
> use C4::Circulation qw( GetBranchItemRule );
> use Koha::DateUtils qw( dt_from_string );
>-use Koha::HoldsQueueItems;
>+use Koha::Hold::HoldsQueueItems;
> use Koha::Items;
> use Koha::Libraries;
> use Koha::Patrons;
>@@ -126,52 +126,42 @@ sub GetHoldsQueueItems {
>     my $params = shift;
>     my $dbh   = C4::Context->dbh;
> 
>-    my @bind_params = ();
>-    my $query = q/SELECT tmp_holdsqueue.*, biblio.author, items.ccode, items.itype, biblioitems.itemtype, items.location,
>-                         items.enumchron, items.cn_sort, biblioitems.publishercode,
>-                         biblio.copyrightdate, biblio.subtitle, biblio.medium,
>-                         biblio.part_number, biblio.part_name,
>-                         biblioitems.publicationyear, biblioitems.pages, biblioitems.size,
>-                         biblioitems.isbn, biblioitems.editionstatement, items.copynumber,
>-                         item_groups.item_group_id, item_groups.description AS item_group_description
>-                  FROM tmp_holdsqueue
>-                       JOIN biblio      USING (biblionumber)
>-                  LEFT JOIN biblioitems USING (biblionumber)
>-                  LEFT JOIN items       USING (  itemnumber)
>-                  LEFT JOIN item_group_items ON ( items.itemnumber =  item_group_items.item_id )
>-                  LEFT JOIN item_groups ON ( item_group_items.item_group_id = item_groups.item_group_id )
>-                  WHERE 1=1
>-                /;
>-    if ($params->{branchlimit}) {
>-        $query .="AND tmp_holdsqueue.holdingbranch = ? ";
>-        push @bind_params, $params->{branchlimit};
>-    }
>-    if( $params->{itemtypeslimit} ) {
>-        $query .=" AND items.itype = ? ";
>-        push @bind_params, $params->{itemtypeslimit};
>-    }
>-    if( $params->{ccodeslimit} ) {
>-        $query .=" AND items.ccode = ? ";
>-        push @bind_params, $params->{ccodeslimit};
>-    }
>-    if( $params->{locationslimit} ) {
>-        $query .=" AND items.location = ? ";
>-        push @bind_params, $params->{locationslimit};
>-    }
>-    $query .= " ORDER BY ccode, location, cn_sort, author, title, pickbranch, reservedate";
>-    my $sth = $dbh->prepare($query);
>-    $sth->execute(@bind_params);
>-    my $items = [];
>-    while ( my $row = $sth->fetchrow_hashref ){
>-        # return the bib-level or item-level itype per syspref
>-        if (!C4::Context->preference('item-level_itypes')) {
>-            $row->{itype} = $row->{itemtype};
>+    my $search_params;
>+    $search_params->{'me.holdingbranch'} = $params->{branchlimit} if $params->{branchlimit};
>+    $search_params->{'itype'} = $params->{itemtypeslimit} if $params->{itemtypeslimit};
>+    $search_params->{'ccode'} = $params->{ccodeslimit} if $params->{ccodeslimit};
>+    $search_params->{'location'} = $params->{locationslimit} if $params->{locationslimit};
>+
>+    my $results = Koha::Hold::HoldsQueueItems->search(
>+        $search_params,
>+        {
>+            join => [
>+                'borrower',
>+                'biblio',
>+                'biblioitem',
>+                {
>+                    'item' => {
>+                        'item_group_item' => 'item_group'
>+                    }
>+                },
>+            ],
>+            prefetch => [
>+                'biblio',
>+                'biblioitem',
>+                {
>+                    'item' => {
>+                        'item_group_item' => 'item_group'
>+                    }
>+                }
>+            ],
>+            order_by => [
>+                'ccode',        'location',   'item.cn_sort', 'author',
>+                'biblio.title', 'pickbranch', 'reservedate'
>+            ],
>         }
>-        delete $row->{itemtype};
>+    );
> 
>-        push @$items, $row;
>-    }
>-    return $items;
>+    return $results;
> }
> 
> =head2 CreateQueue
>diff --git a/Koha/Hold/HoldsQueueItem.pm b/Koha/Hold/HoldsQueueItem.pm
>index 986ec969e6..9493990cb3 100644
>--- a/Koha/Hold/HoldsQueueItem.pm
>+++ b/Koha/Hold/HoldsQueueItem.pm
>@@ -19,6 +19,10 @@ use Modern::Perl;
> 
> use Koha::Database;
> 
>+use Koha::Items;
>+use Koha::Biblios;
>+use Koha::Patrons;
>+
> use base qw(Koha::Object);
> 
> =head1 NAME
>@@ -27,6 +31,41 @@ Koha::Hold::HoldsQueueItem - Koha hold cancellation request Object class
> 
> =head1 API
> 
>+=head2 Class methods
>+
>+=head3 patron
>+
>+=cut
>+
>+sub patron {
>+    my ( $self ) = @_;
>+    my $rs = $self->_result->borrowernumber;
>+    return unless $rs;
>+    return Koha::Patron->_new_from_dbic( $rs );
>+}
>+
>+=head3 biblio
>+
>+=cut
>+
>+sub biblio {
>+    my ( $self ) = @_;
>+    my $rs = $self->_result->biblionumber;
>+    return unless $rs;
>+    return Koha::Biblio->_new_from_dbic( $rs );
>+}
>+
>+=head3 item
>+
>+=cut
>+
>+sub item {
>+    my ( $self ) = @_;
>+    my $rs = $self->_result->itemnumber;
>+    return unless $rs;
>+    return Koha::Item->_new_from_dbic( $rs );
>+}
>+
> =head2 Internal methods
> 
> =head3 _type
>diff --git a/Koha/Schema/Result/TmpHoldsqueue.pm b/Koha/Schema/Result/TmpHoldsqueue.pm
>index 882a91ad0c..2bf9ec46be 100644
>--- a/Koha/Schema/Result/TmpHoldsqueue.pm
>+++ b/Koha/Schema/Result/TmpHoldsqueue.pm
>@@ -224,6 +224,54 @@ __PACKAGE__->add_columns(
>     '+item_level_request' => { is_boolean => 1 }
> );
> 
>+__PACKAGE__->belongs_to(
>+  "borrower",
>+  "Koha::Schema::Result::Borrower",
>+  { borrowernumber => "borrowernumber" },
>+  {
>+    is_deferrable => 1,
>+    join_type     => "LEFT",
>+    on_delete     => "CASCADE",
>+    on_update     => "CASCADE",
>+  },
>+);
>+
>+__PACKAGE__->belongs_to(
>+  "item",
>+  "Koha::Schema::Result::Item",
>+  { itemnumber => "itemnumber" },
>+  {
>+    is_deferrable => 1,
>+    join_type     => "LEFT",
>+    on_delete     => "CASCADE",
>+    on_update     => "CASCADE",
>+  },
>+);
>+
>+__PACKAGE__->belongs_to(
>+  "biblio",
>+  "Koha::Schema::Result::Biblio",
>+  { biblionumber => "biblionumber" },
>+  {
>+    is_deferrable => 1,
>+    join_type     => "LEFT",
>+    on_delete     => "CASCADE",
>+    on_update     => "CASCADE",
>+  },
>+);
>+
>+__PACKAGE__->belongs_to(
>+  "biblioitem",
>+  "Koha::Schema::Result::Biblioitem",
>+  { biblionumber => "biblionumber" },
>+  {
>+    is_deferrable => 1,
>+    join_type     => "LEFT",
>+    on_delete     => "CASCADE",
>+    on_update     => "CASCADE",
>+  },
>+);
>+
> sub koha_object_class {
>     'Koha::HoldsQueueItem';
> }
>diff --git a/circ/view_holdsqueue.pl b/circ/view_holdsqueue.pl
>index b7070fd7a1..241697ab03 100755
>--- a/circ/view_holdsqueue.pl
>+++ b/circ/view_holdsqueue.pl
>@@ -47,24 +47,24 @@ my $itemtypeslimit = $params->{'itemtypeslimit'};
> my $ccodeslimit = $params->{'ccodeslimit'};
> my $locationslimit = $params->{'locationslimit'};
> 
>-if ( $run_report ) {
>-    my $items = GetHoldsQueueItems({
>-        branchlimit => $branchlimit,
>-        itemtypeslimit => $itemtypeslimit,
>-        ccodeslimit => $ccodeslimit,
>-        locationslimit => $locationslimit
>-    });
>-    for my $item ( @$items ) {
>-        $item->{patron} = Koha::Patrons->find( $item->{borrowernumber} );
>-    }
>+if ($run_report) {
>+    my $items = GetHoldsQueueItems(
>+        {
>+            branchlimit    => $branchlimit,
>+            itemtypeslimit => $itemtypeslimit,
>+            ccodeslimit    => $ccodeslimit,
>+            locationslimit => $locationslimit
>+        }
>+    );
>+
>     $template->param(
>-        branchlimit     => $branchlimit,
>-        itemtypeslimit     => $itemtypeslimit,
>-        ccodeslimit     => $ccodeslimit,
>-        locationslimit     => $locationslimit,
>-        total      => scalar @$items,
>-        itemsloop  => $items,
>-        run_report => $run_report,
>+        branchlimit    => $branchlimit,
>+        itemtypeslimit => $itemtypeslimit,
>+        ccodeslimit    => $ccodeslimit,
>+        locationslimit => $locationslimit,
>+        total          => $items->count,
>+        itemsloop      => $items,
>+        run_report     => $run_report,
>     );
> }
> 
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/view_holdsqueue.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/view_holdsqueue.tt
>index fb87fa514b..fec64fb0ff 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/view_holdsqueue.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/view_holdsqueue.tt
>@@ -156,40 +156,40 @@
>                     <tr>
>                         <td class="hq-title">
>                             <p>
>-                                [% INCLUDE 'biblio-title.inc' biblio=itemsloo link = 1 %]
>+                                [% INCLUDE 'biblio-title.inc' biblio=itemsloo.biblio link = 1 %]
>                             </p>
>                             <p>
>                                 <div class="hq-biblionumber content_hidden">[% itemsloo.biblionumber | html %]</div>
>-                                <div class="hq-author">[% itemsloo.author | html %]</div>
>-                                [% IF ( itemsloo.editionstatement ) %]<div class="hq-editionstatement">[% itemsloo.editionstatement | html %]</div>[% END %]
>+                                <div class="hq-author">[% itemsloo.biblio.author | html %]</div>
>+                                [% IF ( itemsloo.biblio.biblioitem.editionstatement ) %]<div class="hq-editionstatement">[% itemsloo.biblio.biblioitem.editionstatement | html %]</div>[% END %]
>                                 <div class="hq-pubdata">
>-                                    [% IF ( itemsloo.publishercode ) %][% itemsloo.publishercode | html %][% END %]
>+                                    [% IF ( itemsloo.biblio.biblioitem.publishercode ) %][% itemsloo.biblio.biblioitem.publishercode | html %][% END %]
> 
>-                                    [% IF ( itemsloo.publicationyear ) %]
>-                                        , [% itemsloo.publicationyear | html %]
>-                                    [% ELSIF ( itemsloo.copyrightdate ) %]
>-                                        , [% itemsloo.copyrightdate | html %]
>+                                    [% IF ( itemsloo.biblio.biblioitem.publicationyear ) %]
>+                                        , [% itemsloo.biblio.biblioitem.publicationyear | html %]
>+                                    [% ELSIF ( itemsloo.biblio.copyrightdate ) %]
>+                                        , [% itemsloo.biblio.copyrightdate | html %]
>                                     [% END %]
> 
>-                                    [% IF ( itemsloo.pages ) %]: [% itemsloo.pages | html %] [% END %]
>+                                    [% IF ( itemsloo.biblio.biblioitem.pages ) %]: [% itemsloo.biblio.biblioitem.pages | html %] [% END %]
> 
>-                                    [% IF ( itemsloo.item('size') ) %][% itemsloo.item('size') | html %][% END %]
>+                                    [% IF ( itemsloo.biblio.biblioitem.size ) %][% itemsloo.biblio.biblioitem.size | html %][% END %]
> 
>-                                    [% IF ( itemsloo.isbn ) %]<span>ISBN: [% itemsloo.isbn | html %]</span>[% END %]
>+                                    [% IF ( itemsloo.biblio.biblioitem.isbn ) %]<span>ISBN: [% itemsloo.biblio.biblioitem.isbn | html %]</span>[% END %]
>                                 </div>
>                             </p>
>                         </td>
>                         <td class="hq-holdingbranch">[% Branches.GetName( itemsloo.holdingbranch ) | html %]</td>
>-                        <td class="hq-collection">[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => itemsloo.ccode ) | html %]</td>
>-                        <td class="hq-itemtype">[% ItemTypes.GetDescription( itemsloo.itype ) | html %]</td>
>-                        <td class="hq-callnumber">[% IF ( itemsloo.location ) %]<em>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => itemsloo.location ) | html %]</em> [% END %][% itemsloo.itemcallnumber | html %]</td>
>-                        <td class="hq-copynumber">[% itemsloo.copynumber | html %]</td>
>-                        <td class="hq-enumchron">[% itemsloo.enumchron | html %]</td>
>+                        <td class="hq-collection">[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => itemsloo.item.ccode ) | html %]</td>
>+                        <td class="hq-itemtype">[% ItemTypes.GetDescription( itemsloo.item.effective_itemtype ) | html %]</td>
>+                        <td class="hq-callnumber">[% IF ( itemsloo.item.location ) %]<em>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => itemsloo.item.location ) | html %]</em> [% END %][% itemsloo.item.itemcallnumber | html %]</td>
>+                        <td class="hq-copynumber">[% itemsloo.item.copynumber | html %]</td>
>+                        <td class="hq-enumchron">[% itemsloo.item.enumchron | html %]</td>
>                         <td class="hq-barcode">
>                             [% IF ( itemsloo.item_level_request ) %]
>                                 <em>Only item:</em> <strong>[% itemsloo.barcode | html %]</strong>
>-                            [% ELSIF itemsloo.item_group_id %]
>-                                <strong>[% itemsloo.barcode | html %]</strong> <em>or any item from item group <strong>[% itemsloo.item_group_description | html %]</strong></em>
>+                            [% ELSIF itemsloo.item.item_group %]
>+                                <strong>[% itemsloo.barcode | html %]</strong> <em>or any item from item group <strong>[% itemsloo.item.item_group.description | html %]</strong></em>
>                             [% ELSE %]
>                                 <strong>[% itemsloo.barcode | html %]</strong> <em>or any available</em>
>                             [% END %]
>diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t
>index 7eb78ce2bd..0286a33d8d 100755
>--- a/t/db_dependent/HoldsQueue.t
>+++ b/t/db_dependent/HoldsQueue.t
>@@ -163,13 +163,13 @@ $dbh->do("DELETE FROM items WHERE holdingbranch = '$borrower_branchcode'");
> # Frst branch from StaticHoldsQueueWeight
> test_queue ('take from lowest cost branch', 0, $borrower_branchcode, $other_branches[0]);
> test_queue ('take from lowest cost branch', 1, $borrower_branchcode, $least_cost_branch_code);
>-my $queue = C4::HoldsQueue::GetHoldsQueueItems({ branchlmit => $least_cost_branch_code}) || [];
>-my $queue_item = $queue->[0];
>+my $queue = C4::HoldsQueue::GetHoldsQueueItems({ branchlimit => $least_cost_branch_code}) || [];
>+my $queue_item = $queue->next;
> ok( $queue_item
>- && $queue_item->{pickbranch} eq $borrower_branchcode
>- && $queue_item->{holdingbranch} eq $least_cost_branch_code, "GetHoldsQueueItems" )
>-  or diag( "Expected item for pick $borrower_branchcode, hold $least_cost_branch_code, got ".Dumper($queue_item) );
>-ok( exists($queue_item->{itype}), 'item type included in queued items list (bug 5825)' );
>+ && $queue_item->pickbranch eq $borrower_branchcode
>+ && $queue_item->item->holdingbranch eq $least_cost_branch_code, "GetHoldsQueueItems" )
>+  or diag( "Expected item for pick $borrower_branchcode, hold $least_cost_branch_code, got ".Dumper($queue_item->unblessed) );
>+ok( $queue_item->item->effective_itemtype, 'item type included in queued items list (bug 5825)' );
> 
> ok(
>     C4::HoldsQueue::least_cost_branch( 'B', [ 'A', 'B', 'C' ] ) eq 'B',
>@@ -1944,25 +1944,25 @@ subtest "GetHoldsQueueItems" => sub {
>      " );
> 
>     my $queue_items = GetHoldsQueueItems();
>-    is( scalar @$queue_items, $count + 3, 'Three items added to queue' );
>+    is( $queue_items->count, $count + 3, 'Three items added to queue' );
> 
>     $queue_items = GetHoldsQueueItems( { itemtypeslimit => $item_1->itype } );
>-    is( scalar @$queue_items,
>+    is( $queue_items->count,
>         3, 'Three items of same itemtype found when itemtypeslimit passed' );
> 
>     $queue_items = GetHoldsQueueItems(
>         { itemtypeslimit => $item_1->itype, ccodeslimit => $item_2->ccode } );
>-    is( scalar @$queue_items,
>+    is( $queue_items->count,
>         2, 'Two items of same collection found when ccodeslimit passed' );
> 
>-    @$queue_items = GetHoldsQueueItems(
>+    $queue_items = GetHoldsQueueItems(
>         {
>             itemtypeslimit => $item_1->itype,
>             ccodeslimit    => $item_2->ccode,
>             locationslimit => $item_3->location
>         }
>     );
>-    is( scalar @$queue_items,
>+    is( scalar $queue_items->count,
>         1, 'One item of shleving location found when locationslimit passed' );
> 
>     $schema->storage->txn_rollback;
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 28966: 124626 | 124627 | 124628 | 124629 | 124639 | 124652 | 124854 | 125377 | 143583 | 143584 | 143857 | 143859 | 143860 | 145395 | 151733 | 151734 | 153120 | 153875 | 153876 | 153877 | 153878 | 153879 | 153880 | 153881 | 153882 | 153928 | 153929 | 153939