Bugzilla – Attachment 151734 Details for
Bug 28966
Holds queue viewer too slow to load for large numbers of holds
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28966: Prefetch patron data for holds queue viewer
Bug-28966-Prefetch-patron-data-for-holds-queue-vie.patch (text/plain), 17.04 KB, created by
Owen Leonard
on 2023-05-26 12:13:24 UTC
(
hide
)
Description:
Bug 28966: Prefetch patron data for holds queue viewer
Filename:
MIME Type:
Creator:
Owen Leonard
Created:
2023-05-26 12:13:24 UTC
Size:
17.04 KB
patch
obsolete
>From 831fe7b6b8b13debe1671a48a4e4dbfd693aba0f 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 > >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> >--- > 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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=151734">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
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