From 96187695d7782778a30af90fd47fca25107dbfc4 Mon Sep 17 00:00:00 2001 From: Kyle Hall 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 Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- 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 @@

- [% INCLUDE 'biblio-title.inc' biblio=itemsloo link = 1 %] + [% INCLUDE 'biblio-title.inc' biblio=itemsloo.biblio link = 1 %]

[% itemsloo.biblionumber | html %]
-
[% itemsloo.author | html %]
- [% IF ( itemsloo.editionstatement ) %]
[% itemsloo.editionstatement | html %]
[% END %] +
[% itemsloo.biblio.author | html %]
+ [% IF ( itemsloo.biblio.biblioitem.editionstatement ) %]
[% itemsloo.biblio.biblioitem.editionstatement | html %]
[% END %]
- [% 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 ) %]ISBN: [% itemsloo.isbn | html %][% END %] + [% IF ( itemsloo.biblio.biblioitem.isbn ) %]ISBN: [% itemsloo.biblio.biblioitem.isbn | html %][% END %]

[% Branches.GetName( itemsloo.holdingbranch ) | html %] - [% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => itemsloo.ccode ) | html %] - [% ItemTypes.GetDescription( itemsloo.itype ) | html %] - [% IF ( itemsloo.location ) %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => itemsloo.location ) | html %] [% END %][% itemsloo.itemcallnumber | html %] - [% itemsloo.copynumber | html %] - [% itemsloo.enumchron | html %] + [% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => itemsloo.item.ccode ) | html %] + [% ItemTypes.GetDescription( itemsloo.item.effective_itemtype ) | html %] + [% IF ( itemsloo.item.location ) %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => itemsloo.item.location ) | html %] [% END %][% itemsloo.item.itemcallnumber | html %] + [% itemsloo.item.copynumber | html %] + [% itemsloo.item.enumchron | html %] [% IF ( itemsloo.item_level_request ) %] Only item: [% itemsloo.barcode | html %] - [% ELSIF itemsloo.item_group_id %] - [% itemsloo.barcode | html %] or any item from item group [% itemsloo.item_group_description | html %] + [% ELSIF itemsloo.item.item_group %] + [% itemsloo.barcode | html %] or any item from item group [% itemsloo.item.item_group.description | html %] [% ELSE %] [% itemsloo.barcode | html %] or any available [% 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.39.5