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

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This commit is contained in:
Aleisha Amohia 2020-01-23 03:59:37 +00:00 committed by Jonathan Druart
parent 18368d6a0c
commit 2285c2d657
3 changed files with 258 additions and 165 deletions

View File

@ -469,4 +469,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;

View File

@ -152,143 +152,218 @@ 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 ( $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 };
}
if ($enddate_iso) {
$sqldatewhere .= " AND reservedate <= ?";
push @query_params, $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,
GROUP_CONCAT(DISTINCT items.barcode
ORDER BY items.itemnumber SEPARATOR '|') l_barcode,
biblio.title,
biblio.copyrightdate,
biblioitems.publicationyear,
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 itemnumber 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
#FIXME "found IS NOT NULL AND itemnumber IS NOT NULL" is just a workaround: see BZ 25726
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);
# get all distinct unfulfilled reserves
my @distinct_holds = Koha::Holds->search(
{ %where },
{ join => 'itembib', group_by => 'reserve.biblionumber', alias => 'reserve' }
);
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})],
barcode => [split('\|', $data->{l_barcode})],
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},
publicationyear => C4::Context->preference('marcflavour') eq "MARC21" ? $data->{copyrightdate} : $data->{publicationyear},
# make final reserves hash and fill with info
my $reserves;
foreach my $dh ( @distinct_holds ){
my $bibnum = $dh->biblionumber;
my @items = Koha::Items->search({ biblionumber => $bibnum });
foreach my $i ( @items ){
if ( $i->checkout ){
next;
}
}
my @branchtransfers = map { $_->itemnumber } Koha::Item::Transfers->search({ datearrived => undef }, { columns => [ 'itemnumber' ], collapse => 1 });
my @issues = map { $_->itemnumber } Koha::Checkouts->search({}, { columns => [ 'itemnumber' ], collapse => 1 });
# 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 => [ @branchtransfers, @issues ] },
},
{ columns => 'itype',
distinct => 1,
}
);
} else {
my $res_itemtypes = Koha::Biblioitems->search(
{ biblionumber => $bibnum, itemtype => { '!=', undef } },
{ columns => 'itemtype',
distinct => 1,
}
);
}
$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 => [ @branchtransfers, @issues ] },
},
{ columns => 'location',
distinct => 1,
}
);
$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 => [ @branchtransfers, @issues ] },
},
{ columns => 'itemcallnumber',
distinct => 1,
}
);
$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 => [ @branchtransfers, @issues ] },
},
{ columns => 'enumchron',
distinct => 1,
}
);
$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 => [ @branchtransfers, @issues ] },
},
{ columns => 'copynumber',
distinct => 1,
}
);
$reserves->{$bibnum}->{copynumbers} = $res_copynumbers;
# get available barcodes for each biblio
my $res_barcodes = Koha::Items->search(
{ biblionumber => $bibnum,
barcode => { '!=', undef },
itemlost => 0,
withdrawn => 0,
notforloan => 0,
itemnumber => { -not_in => [ @branchtransfers, @issues ] },
},
{ columns => 'barcode',
distinct => 1,
}
);
$reserves->{$bibnum}->{barcodes} = $res_barcodes;
# 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 => [ @branchtransfers, @issues ] },
},
{ columns => 'holdingbranch',
distinct => 1,
}
);
$reserves->{$bibnum}->{holdingbranches} = $res_holdingbranches;
# items available
my $items_count = Koha::Items->search(
{ biblionumber => $bibnum,
itemlost => 0,
withdrawn => 0,
notforloan => 0,
itemnumber => { -not_in => [ @branchtransfers, @issues ] },
},
{ 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;
if ( $pull_count == 0 ) {
delete($reserves->{$bibnum});
next;
}
$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,

View File

@ -47,7 +47,7 @@
<h3>Reported on [% todaysdate | $KohaDates %]</h3>
<p>The following holds have not been filled. Please retrieve them and check them in.</p>
<div id="searchresults">
[% IF ( reserveloop ) %]
[% IF ( reserves ) %]
<table id="holdst">
<thead>
<tr>
@ -68,20 +68,20 @@
</tr>
</thead>
<tbody>
[% FOREACH reserveloo IN reserveloop %]
[% FOREACH reserve IN reserves %]
<tr>
[% IF ( reserveloo.borrowernumber ) %]
<td><p><strong>[% reserveloo.pullcount | html %]</strong></p></td>
<td>[% reserveloo.count | html %]</td>
<td>[% reserveloo.rcount | html %]</td>
<td><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% reserveloo.borrowernumber | uri %]">[% reserveloo.firstname | html %] [% reserveloo.surname | html %]</a></td>
[% IF ( reserve.value.borrower ) %]
<td><p><strong>[% reserve.value.pull_count | html %]</strong></p></td>
<td>[% reserve.value.items_count | html %]</td>
<td>[% reserve.value.patrons_count | html %]</td>
<td><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% reserve.value.borrower.borrowernumber | uri %]">[% reserve.value.borrower.firstname | html %] [% reserve.value.borrower.surname | html %]</a></td>
<td>
<p>
[% INCLUDE 'biblio-title.inc' biblio=reserveloo link = 1 %]
</p>
[% IF ( reserveloo.author ) %]<p> by [% reserveloo.author | html %]</p>[% END %]
[% IF ( reserveloo.editionstatement ) %]<p>[% reserveloo.editionstatement | html %]</p>[% END %]
[% IF ( reserveloo.publicationyear ) %]<p>[% reserveloo.publicationyear | html %]</p>[% END %]
<p>
[% INCLUDE 'biblio-title.inc' biblio=reserve.value.biblio link = 1 %]
</p>
[% IF ( reserve.value.biblio.author ) %]<p> by [% reserve.value.biblio.author | html %]</p>[% END %]
[% IF ( reserve.value.biblio.biblioitem.editionstatement ) %]<p>[% reserve.value.biblio.biblioitem.editionstatement | html %]</p>[% END %]
[% IF ( reserve.value.biblio.biblioitem.publicationyear ) %]<p>[% reserve.value.biblio.biblioitem.publicationyear | html %]</p>[% END %]
</td>
[% ELSE %]
<td>"</td>
@ -91,71 +91,78 @@
<td>"</td>
[% END %]
<td>
[% IF ( reserveloo.holdingbranches ) %]
<ul>
[% FOREACH holdingbranch IN reserveloo.holdingbranches %]
<li>
[% Branches.GetName ( holdingbranch ) | html %]
</li>
[% END %]
</ul>
[% IF ( reserve.value.holdingbranches ) %]
<ul>
[% FOREACH holdingbranch IN reserve.value.holdingbranches %]
<li>[% Branches.GetName ( holdingbranch.holdingbranch ) | html %]</li>
[% END %]
</ul>
[% END %]
</td>
<td>
[% IF ( reserveloo.barcode ) %]
[% IF ( reserveloo.itemnumber ) %]Only [% reserveloo.barcode.first | html %][% ELSE %][% reserveloo.barcode.first | html %] or any available.[% END %]
[% IF ( reserve.value.barcodes.count ) %]
[% SET barcode = reserve.value.barcodes.next.barcode %]
[% IF ( reserve.value.itemnumber ) %]Only [% barcode | html %][% ELSE %][% barcode | html %] or any available.[% END %]
[% END %]
</td>
<td>
[% IF ( reserveloo.itemcallnumber ) %]
[% IF ( reserve.value.callnumbers ) %]
<ul>
[% FOREACH itemcallnumber IN reserveloo.itemcallnumber %]
[% FOREACH callnumber IN reserve.value.callnumbers %]
<li>
[% itemcallnumber | html %]
[% callnumber.itemcallnumber | html %]
</li>
[% END %]
</ul>
[% END %]
</td>
<td>
[% IF ( reserveloo.copyno ) %]
[% IF ( reserve.value.copynumbers ) %]
<ul>
[% FOREACH copyno IN reserveloo.copyno %]
[% FOREACH copyno IN reserve.value.copynumbers %]
<li>
[% copyno | html %]
[% copyno.copynumber | html %]
</li>
[% END %]
</ul>
[% END %]
</td>
<td>
[% IF ( reserveloo.enumchron ) %]
[% IF ( reserve.value.enumchrons ) %]
<ul>
[% FOREACH enumchron IN reserveloo.enumchron %]
[% FOREACH enumchron IN reserve.value.enumchrons %]
<li>
[% enumchron | html %]
[% enumchron.enumchron | html %]
</li>
[% END %]
</ul>
[% END %]
</td>
<td>
[% FOREACH itemType IN reserveloo.itemTypes %]
[% ItemTypes.GetDescription( itemType ) | html %]
[% END %]
<ul>
[% FOREACH type IN reserve.value.itemtypes %]
[% IF Koha.Preference('item-level_itypes') %]
<li>[% ItemTypes.GetDescription( type.itype ) | html %]</li>
[% ELSE %]
<li>[% ItemTypes.GetDescription( type.itemtype ) | html %]</li>
[% END %]
[% END %]
</ul>
</td>
<td>
[% FOREACH loc IN reserveloo.locations %]
[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => loc) | html %]
[% END %]
<ul>
[% FOREACH loc IN reserve.value.locations %]
<li>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => loc.location) | html %]</li>
[% END %]
</ul>
</td>
<td>
<span title="[% reserveloo.reservedate | html %]">[% reserveloo.reservedate | $KohaDates %] in [% Branches.GetName ( reserveloo.branch ) | html %]</span>
<span title="[% reserve.value.reserve.reservedate | html %]">[% reserve.value.reserve.reservedate | $KohaDates %] in [% Branches.GetName ( reserve.value.reserve.branchcode ) | html %]</span>
</td>
<td>
<form name="cancelReserve" action="/cgi-bin/koha/circ/pendingreserves.pl" method="post">
<input type="hidden" name="op" value="cancel_reserve" />
<input type="hidden" name="reserve_id" value="[% reserveloo.reserve_id | html %]" />
<input type="hidden" name="reserve_id" value="[% reserve.value.reserve.reserve_id | html %]" />
[% SET hold_cancellation = AuthorisedValues.GetAuthValueDropbox('HOLD_CANCELLATION') %]
[% IF hold_cancellation %]
@ -170,17 +177,17 @@
</div>
[% END %]
[% IF reserveloo.holdingbranch != reserveloo.homebranch %]
<input class="btn btn-default" type="submit" value="Cancel hold and return to : [% Branches.GetName( reserveloo.homebranch ) | html %]" />
[% IF reserve.value.item.holdingbranch != reserve.value.item.homebranch %]
<input class="btn btn-default" type="submit" value="Cancel hold and return to : [% Branches.GetName( reserve.value.item.homebranch ) | html %]" />
[% ELSE %]
<input class="btn btn-default" type="submit" value="Cancel hold" />
[% END %]
</form>
[% IF Koha.Preference('CanMarkHoldsToPullAsLost') != 'do_not_allow' %]
[% IF reserveloo.itemnumber %]
[% IF reserve.value.reserve.itemnumber %]
<form name="cancelReserve" action="/cgi-bin/koha/circ/pendingreserves.pl" method="post">
<input type="hidden" name="reserve_id" value="[% reserveloo.reserve_id | html %]" />
<input type="hidden" name="reserve_id" value="[% reserve.value.reserve.reserve_id | html %]" />
[% IF Koha.Preference('CanMarkHoldsToPullAsLost') == 'allow' %]
<input type="hidden" name="op" value="mark_as_lost" />
<input type="submit" value="Mark item as lost" />
@ -304,8 +311,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 () {