From ca362e127416c9b2b7e20d3157163b470b6a1452 Mon Sep 17 00:00:00 2001 From: Aleisha Amohia Date: Thu, 23 Jan 2020 03:59:37 +0000 Subject: [PATCH] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand --- Koha/Schema/Result/Reserve.pm | 12 + circ/pendingreserves.pl | 282 ++++++++++++--------- .../prog/en/modules/circ/pendingreserves.tt | 79 +++--- 3 files changed, 218 insertions(+), 155 deletions(-) diff --git a/Koha/Schema/Result/Reserve.pm b/Koha/Schema/Result/Reserve.pm index c5460e34da..debf6351ce 100644 --- a/Koha/Schema/Result/Reserve.pm +++ b/Koha/Schema/Result/Reserve.pm @@ -377,4 +377,16 @@ sub koha_objects_class { 'Koha::Holds'; } +__PACKAGE__->belongs_to( + "itembib", + "Koha::Schema::Result::Item", + { biblionumber => "biblionumber" }, + { + is_deferrable => 1, + join_type => "LEFT", + on_delete => "CASCADE", + on_update => "CASCADE", + }, +); + 1; diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl index 22c93f7130..f52234dd79 100755 --- a/circ/pendingreserves.pl +++ b/circ/pendingreserves.pl @@ -152,137 +152,189 @@ unless ( $enddate ) { $enddate = $today + DateTime::Duration->new( days => C4::Context->preference('ConfirmFutureHolds') || 0 ); } -my @reservedata; -my $dbh = C4::Context->dbh; -my $sqldatewhere = ""; +# building query parameters +my %where = ( + 'reserve.found' => undef, + 'reserve.suspend' => 0, + 'itembib.itemlost' => 0, + 'itembib.withdrawn' => 0, + 'itembib.notforloan' => 0, + 'itembib.itemnumber' => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' } +); + +# date boundaries my $startdate_iso = output_pref({ dt => $startdate, dateformat => 'iso', dateonly => 1 }); my $enddate_iso = output_pref({ dt => $enddate, dateformat => 'iso', dateonly => 1 }); - -$debug and warn $startdate_iso. "\n" . $enddate_iso; - -my @query_params = (); - -if ($startdate_iso) { - $sqldatewhere .= " AND reservedate >= ?"; - push @query_params, $startdate_iso; -} -if ($enddate_iso) { - $sqldatewhere .= " AND reservedate <= ?"; - push @query_params, $enddate_iso; +if ( $startdate_iso && $enddate_iso ){ + $where{'reserve.reservedate'} = [ -and => { '>=', $startdate_iso }, { '<=', $enddate_iso } ]; +} elsif ( $startdate_iso ){ + $where{'reserve.reservedate'} = { '>=', $startdate_iso }; +} elsif ( $enddate_iso ){ + $where{'reserve.reservedate'} = { '<=', $enddate_iso }; } -my $item_type = C4::Context->preference('item-level_itypes') ? "items.itype" : "biblioitems.itemtype"; - # Bug 21320 -if ( ! C4::Context->preference('AllowHoldsOnDamagedItems') ) { - $sqldatewhere .= " AND damaged = 0"; +if ( !C4::Context->preference('AllowHoldsOnDamagedItems') ){ + $where{'itembib.damaged'} = 0; } -my $strsth = - "SELECT min(reservedate) as l_reservedate, - reserves.reserve_id, - reserves.borrowernumber as borrowernumber, - - GROUP_CONCAT(DISTINCT items.holdingbranch - ORDER BY items.itemnumber SEPARATOR '|') l_holdingbranch, - reserves.biblionumber, - reserves.branchcode as l_branch, - reserves.itemnumber, - items.holdingbranch, - items.homebranch, - GROUP_CONCAT(DISTINCT $item_type - ORDER BY items.itemnumber SEPARATOR '|') l_item_type, - GROUP_CONCAT(DISTINCT items.location - ORDER BY items.itemnumber SEPARATOR '|') l_location, - GROUP_CONCAT(DISTINCT items.itemcallnumber - ORDER BY items.itemnumber SEPARATOR '|') l_itemcallnumber, - GROUP_CONCAT(DISTINCT items.enumchron - ORDER BY items.itemnumber SEPARATOR '|') l_enumchron, - GROUP_CONCAT(DISTINCT items.copynumber - ORDER BY items.itemnumber SEPARATOR '|') l_copynumber, - biblio.title, - biblio.subtitle, - biblio.medium, - biblio.part_number, - biblio.part_name, - biblio.author, - biblioitems.editionstatement, - count(DISTINCT items.itemnumber) as icount, - count(DISTINCT reserves.borrowernumber) as rcount, - borrowers.firstname, - borrowers.surname - FROM reserves - LEFT JOIN items ON items.biblionumber=reserves.biblionumber - LEFT JOIN biblio ON reserves.biblionumber=biblio.biblionumber - LEFT JOIN biblioitems ON biblio.biblionumber=biblioitems.biblionumber - LEFT JOIN branchtransfers ON items.itemnumber=branchtransfers.itemnumber - LEFT JOIN issues ON items.itemnumber=issues.itemnumber - LEFT JOIN borrowers ON reserves.borrowernumber=borrowers.borrowernumber - LEFT JOIN circulation_rules ON ( items.itype=circulation_rules.itemtype AND rule_name = 'holdallowed' AND circulation_rules.branchcode IS NULL AND circulation_rules.categorycode IS NULL ) - WHERE - reserves.found IS NULL - $sqldatewhere - AND (reserves.itemnumber IS NULL OR reserves.itemnumber = items.itemnumber) - AND items.itemnumber NOT IN (SELECT itemnumber FROM branchtransfers where datearrived IS NULL) - AND items.itemnumber NOT IN (select itemnumber FROM reserves where found IS NOT NULL) - AND issues.itemnumber IS NULL - AND reserves.priority <> 0 - AND reserves.suspend = 0 - AND notforloan = 0 AND itemlost = 0 AND withdrawn = 0 - AND ( circulation_rules.rule_value IS NULL OR circulation_rules.rule_value != 0 ) - "; - # GROUP BY reserves.biblionumber allows only items that are not checked out, else multiples occur when - # multiple patrons have a hold on an item - - -if (C4::Context->preference('IndependentBranches')){ - $strsth .= " AND items.holdingbranch=? "; - push @query_params, C4::Context->userenv->{'branch'}; +if ( C4::Context->preference('IndependentBranches') ){ + $where{'itembib.holdingbranch'} = C4::Context->userenv->{'branch'}; } -$strsth .= " GROUP BY reserves.biblionumber ORDER BY biblio.title "; - -my $sth = $dbh->prepare($strsth); -$sth->execute(@query_params); - -while ( my $data = $sth->fetchrow_hashref ) { - push( - @reservedata, { - reservedate => $data->{l_reservedate}, - firstname => $data->{firstname} || '', - surname => $data->{surname}, - title => $data->{title}, - editionstatement => $data->{editionstatement}, - subtitle => $data->{subtitle}, - medium => $data->{medium}, - part_number => $data->{part_number}, - part_name => $data->{part_name}, - author => $data->{author}, - borrowernumber => $data->{borrowernumber}, - biblionumber => $data->{biblionumber}, - holdingbranches => [split('\|', $data->{l_holdingbranch})], - branch => $data->{l_branch}, - itemcallnumber => [split('\|', $data->{l_itemcallnumber})], - enumchron => [split('\|', $data->{l_enumchron})], - copyno => [split('\|', $data->{l_copynumber})], - count => $data->{icount}, - rcount => $data->{rcount}, - pullcount => $data->{icount} <= $data->{rcount} ? $data->{icount} : $data->{rcount}, - itemTypes => [split('\|', $data->{l_item_type})], - locations => [split('\|', $data->{l_location})], - reserve_id => $data->{reserve_id}, - holdingbranch => $data->{holdingbranch}, - homebranch => $data->{homebranch}, - itemnumber => $data->{itemnumber}, + +# get all distinct unfulfilled reserves +my @distinct_holds = Koha::Holds->search( + { %where }, + { join => 'itembib', group_by => 'reserve.biblionumber', alias => 'reserve' } +); + +# make final reserves hash and fill with info +my $reserves; +foreach my $dh ( @distinct_holds ){ + + my $bibnum = $dh->biblionumber; + + # check for checkouts + my $bib_has_checkouts = 0; + + my @items = Koha::Items->search({ biblionumber => $bibnum }); + foreach my $i ( @items ){ + if ( $i->checkout ){ + $bib_has_checkouts++; + last; } + } + if ( $bib_has_checkouts > 0 ){ + next; + } + + # get available item types for each biblio + my $res_itemtypes; + if ( C4::Context->preference('item-level_itypes') ){ + $res_itemtypes = Koha::Items->search( + { biblionumber => $bibnum, + itype => { '!=', undef }, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { group_by => 'itype' } + ); + } else { + $res_itemtypes = Koha::Biblioitems->search( + { biblionumber => $bibnum, itemtype => { '!=', undef } }, + { group_by => 'itemtype' } + ); + } + $reserves->{$bibnum}->{itemtypes} = $res_itemtypes; + + # get available locations for each biblio + my $res_locations = Koha::Items->search( + { biblionumber => $bibnum, + location => { '!=', undef }, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { group_by => 'location' } + ); + $reserves->{$bibnum}->{locations} = $res_locations; + + # get available callnumbers for each biblio + my $res_callnumbers = Koha::Items->search( + { biblionumber => $bibnum, + itemcallnumber => { '!=', undef }, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { group_by => 'itemcallnumber' } + ); + $reserves->{$bibnum}->{callnumbers} = $res_callnumbers; + + # get available enumchrons for each biblio + my $res_enumchrons = Koha::Items->search( + { biblionumber => $bibnum, + enumchron => { '!=', undef }, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { group_by => 'enumchron' } + ); + $reserves->{$bibnum}->{enumchrons} = $res_enumchrons; + + # get available copynumbers for each biblio + my $res_copynumbers = Koha::Items->search( + { biblionumber => $bibnum, + copynumber => { '!=', undef }, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { group_by => 'copynumber' } ); + $reserves->{$bibnum}->{copynumbers} = $res_copynumbers; + + # get available holding branches for each biblio + my $res_holdingbranches = Koha::Items->search( + { biblionumber => $bibnum, + holdingbranch => { '!=', undef }, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { group_by => 'holdingbranch' } + ); + $reserves->{$bibnum}->{holdingbranches} = $res_holdingbranches; + + # items available + my $items_count = Koha::Items->search( + { biblionumber => $bibnum, + itemlost => 0, + withdrawn => 0, + notforloan => 0, + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }, + }, + { select => [ { distinct => 'itemnumber' } ] } + )->count; + $reserves->{$bibnum}->{items_count} = $items_count; + + # patrons with holds + my $patrons_count = Koha::Holds->search( + { biblionumber => $bibnum }, + { select => [ { distinct => 'borrowernumber' } ] } + )->count; + $reserves->{$bibnum}->{patrons_count} = $patrons_count; + + my $pull_count = $items_count <= $patrons_count ? $items_count : $patrons_count; + $reserves->{$bibnum}->{pull_count} = $pull_count; + + # get other relevant information + my $res_info = Koha::Holds->search( + { 'reserve.biblionumber' => $bibnum, %where }, + { prefetch => [ 'borrowernumber', 'itembib', 'biblio' ], + order_by => 'reserve.reservedate', + alias => 'reserve' + } + )->next; # get first item in results + $reserves->{$bibnum}->{borrower} = $res_info->borrower; + $reserves->{$bibnum}->{item} = $res_info->item; + $reserves->{$bibnum}->{biblio} = $res_info->biblio; + $reserves->{$bibnum}->{reserve} = $res_info; } -$sth->finish; $template->param( todaysdate => $today, from => $startdate, to => $enddate, - reserveloop => \@reservedata, + reserves => $reserves, "BiblioDefaultView".C4::Context->preference("BiblioDefaultView") => 1, HoldsToPullStartDate => C4::Context->preference('HoldsToPullStartDate') || PULL_INTERVAL, HoldsToPullEndDate => C4::Context->preference('ConfirmFutureHolds') || 0, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt index 7783acc707..1480d30e07 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt @@ -47,7 +47,7 @@

Reported on [% todaysdate | $KohaDates %]

The following holds have not been filled. Please retrieve them and check them in.

- [% IF ( reserveloop ) %] + [% IF ( reserves ) %] @@ -67,20 +67,20 @@ - [% FOREACH reserveloo IN reserveloop %] + [% FOREACH reserve IN reserves %] - [% IF ( reserveloo.borrowernumber ) %] - - - - + [% IF ( reserve.value.borrower ) %] + + + + [% ELSE %] @@ -90,77 +90,77 @@ [% END %]

[% reserveloo.pullcount | html %]

[% reserveloo.count | html %][% reserveloo.rcount | html %][% reserveloo.firstname | html %] [% reserveloo.surname | html %]

[% reserve.value.pull_count | html %]

[% reserve.value.items_count | html %][% reserve.value.patrons_count | html %][% reserve.value.borrower.firstname | html %] [% reserve.value.borrower.surname | html %]

- [% INCLUDE 'biblio-default-view.inc' biblionumber = reserveloo.biblionumber %] - [% INCLUDE 'biblio-title.inc' biblio=reserveloo %] + [% INCLUDE 'biblio-default-view.inc' biblionumber = reserve.value.biblio.biblionumber %] + [% INCLUDE 'biblio-title.inc' biblio=reserve.value.biblio %]

- [% IF ( reserveloo.author ) %]

by [% reserveloo.author | html %]

[% END %] - [% IF ( reserveloo.editionstatement ) %]

[% reserveloo.editionstatement | html %]

[% END %] + [% IF ( reserve.value.biblio.biblioitem.editionstatement ) %]

[% reserve.value.biblio.biblioitem.editionstatement | html %]

[% END %] + [% IF ( reserve.value.biblio.author ) %]

by [% reserve.value.biblio.author | html %]

[% END %]
"" - [% IF ( reserveloo.holdingbranches ) %] -
    - [% FOREACH holdingbranch IN reserveloo.holdingbranches %] -
  • - [% Branches.GetName ( holdingbranch ) | html %] -
  • - [% END %] -
+ [% IF ( reserve.value.holdingbranches ) %] + [% FOREACH holdingbranch IN reserve.value.holdingbranches %] + [% Branches.GetName ( holdingbranch.holdingbranch ) | html %] + [% END %] [% END %]
- [% IF ( reserveloo.itemcallnumber ) %] + [% IF ( reserve.value.callnumbers ) %]
    - [% FOREACH itemcallnumber IN reserveloo.itemcallnumber %] + [% FOREACH callnumber IN reserve.value.callnumbers %]
  • - [% itemcallnumber | html %] + [% callnumber.itemcallnumber | html %]
  • [% END %]
[% END %]
- [% IF ( reserveloo.copyno ) %] + [% IF ( reserve.value.copynumbers ) %]
    - [% FOREACH copyno IN reserveloo.copyno %] + [% FOREACH copyno IN reserve.value.copynumbers %]
  • - [% copyno | html %] + [% copyno.copynumber | html %]
  • [% END %]
[% END %]
- [% IF ( reserveloo.enumchron ) %] + [% IF ( reserve.value.enumchrons ) %]
    - [% FOREACH enumchron IN reserveloo.enumchron %] + [% FOREACH enumchron IN reserve.value.enumchrons %]
  • - [% enumchron | html %] + [% enumchron.enumchron | html %]
  • [% END %]
[% END %]
- [% FOREACH itemType IN reserveloo.itemTypes %] - [% ItemTypes.GetDescription( itemType ) | html %] + [% FOREACH type IN reserve.value.itemtypes %] + [% IF Koha.Preference('item-level_itypes') %] + [% ItemTypes.GetDescription( type.itype ) | html %] + [% ELSE %] + [% ItemTypes.GetDescription( type.itemtype ) | html %] + [% END %] [% END %] - [% FOREACH loc IN reserveloo.locations %] - [% AuthorisedValues.GetByCode('LOC', loc) | html %] + [% FOREACH loc IN reserve.value.locations %] + [% AuthorisedValues.GetByCode('LOC', loc.location) | html %] [% END %] - [% reserveloo.reservedate | $KohaDates %] in [% Branches.GetName ( reserveloo.branch ) | html %] + [% reserve.value.reserve.reservedate | $KohaDates %] in [% Branches.GetName ( reserve.value.reserve.branchcode ) | html %]
- - [% IF reserveloo.holdingbranch != reserveloo.homebranch %] - + + [% IF reserve.value.item.holdingbranch != reserve.value.item.homebranch %] + [% ELSE %] [% END %]
[% IF Koha.Preference('CanMarkHoldsToPullAsLost') != 'do_not_allow' %] - [% IF reserveloo.itemnumber %] + [% IF reserve.value.reserve.itemnumber %]
- + [% IF Koha.Preference('CanMarkHoldsToPullAsLost') == 'allow' %] @@ -288,8 +288,7 @@ $("#homebranchfilter").each( function () { $(this).html( createSelect( holdst.fnGetColumnData(5) ) ); $('select', this).change( function () { - var filter_value = $(this).val(); - holdst.fnFilter( filter_value, 5, true ); + holdst.fnFilter( $(this).child().val(), 5 ); }); }); $("#itemtype-filter").each( function () { -- 2.11.0