[Koha-patches] [PATCH] Bug 2900: fix GetPendingIssues.
Joe Atzberger
joe.atzberger at liblime.com
Thu Jan 8 04:47:36 CET 2009
GetPendingIssues did several bad things:
~ select * on a 4 table join,
~ including multiple namespace collisions,
~ including large fields marc and marcxml from biblioitems,
~ return ($count, \@array_being_counted).
Not everything is fixed here (see FIXMEs), but the situation is
improved considerably, with bug 2900 resolved. The "timestamp"
namespace collision in query should be resolved by separate patch.
---
C4/Members.pm | 62 ++++++++++++++++++++++++++++++------------------
C4/Print.pm | 6 ++--
C4/SIP/ILS/Patron.pm | 3 +-
circ/circulation.pl | 4 +-
members/deletemem.pl | 13 ++--------
members/moremember.pl | 3 +-
opac/opac-ics.pl | 2 +-
opac/opac-user.pl | 2 +-
opac/sco/sco-main.pl | 2 +-
9 files changed, 53 insertions(+), 44 deletions(-)
diff --git a/C4/Members.pm b/C4/Members.pm
index 3227a40..9a86e71 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -994,44 +994,60 @@ sub UpdateGuarantees {
}
=head2 GetPendingIssues
- ($count, $issues) = &GetPendingIssues($borrowernumber);
+ my $issues = &GetPendingIssues($borrowernumber);
Looks up what the patron with the given borrowernumber has borrowed.
-C<&GetPendingIssues> returns a two-element array. C<$issues> is a
-reference-to-array, where each element is a reference-to-hash; the
-keys are the fields from the C<issues>, C<biblio>, and C<items> tables
-in the Koha database. C<$count> is the number of elements in
-C<$issues>.
+C<&GetPendingIssues> returns a
+reference-to-array where each element is a reference-to-hash; the
+keys are the fields from the C<issues>, C<biblio>, and C<items> tables.
+The keys include C<biblioitems> fields except marc and marcxml.
=cut
#'
sub GetPendingIssues {
my ($borrowernumber) = @_;
- my $dbh = C4::Context->dbh;
-
- my $sth = $dbh->prepare(
- "SELECT *,issues.timestamp as timestamp FROM issues
- LEFT JOIN items ON issues.itemnumber=items.itemnumber
- LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber
- LEFT JOIN biblioitems ON items.biblioitemnumber=biblioitems.biblioitemnumber
+ # must avoid biblioitems.* to prevent large marc and marcxml fields from killing performance
+ # FIXME: namespace collision: each table has "timestamp" fields. Which one is "timestamp" ?
+ # FIXME: circ/ciculation.pl tries to sort by timestamp!
+ # FIXME: C4::Print::printslip tries to sort by timestamp!
+ # FIXME: namespace collision: other collisions possible.
+ # FIXME: most of this data isn't really being used by callers.
+ my $sth = C4::Context->dbh->prepare(
+ "SELECT issues.*,
+ items.*,
+ biblio.*,
+ biblioitems.volume,
+ biblioitems.number,
+ biblioitems.itemtype,
+ biblioitems.isbn,
+ biblioitems.issn,
+ biblioitems.publicationyear,
+ biblioitems.publishercode,
+ biblioitems.volumedate,
+ biblioitems.volumedesc,
+ biblioitems.lccn,
+ biblioitems.url,
+ issues.timestamp AS timestamp,
+ issues.renewals AS renewals,
+ items.renewals AS totalrenewals
+ FROM issues
+ LEFT JOIN items ON items.itemnumber = issues.itemnumber
+ LEFT JOIN biblio ON items.biblionumber = biblio.biblionumber
+ LEFT JOIN biblioitems ON items.biblioitemnumber = biblioitems.biblioitemnumber
WHERE
- borrowernumber=?
+ borrowernumber=?
ORDER BY issues.issuedate"
);
$sth->execute($borrowernumber);
my $data = $sth->fetchall_arrayref({});
- my $today = POSIX::strftime("%Y%m%d", localtime);
- foreach( @$data ) {
- my $datedue = $_->{'date_due'};
- $datedue =~ s/-//g;
- if ( $datedue < $today ) {
- $_->{'overdue'} = 1;
- }
+ my $today = C4::Dates->new->output('iso');
+ foreach (@$data) {
+ $_->{date_due} or next;
+ ($_->{date_due} < $today) and $_->{overdue} = 1;
}
- $sth->finish;
- return ( scalar(@$data), $data );
+ return $data;
}
=head2 GetAllIssues
diff --git a/C4/Print.pm b/C4/Print.pm
index 582ecde..69986dc 100644
--- a/C4/Print.pm
+++ b/C4/Print.pm
@@ -177,9 +177,9 @@ EOF
#'
sub printslip ($) {
- my ( $borrowernumber ) = shift;
- my ( $borrower ) = GetMemberDetails( $borrowernumber);
- my ($countissues,$issueslist) = GetPendingIssues($borrowernumber);
+ my $borrowernumber = shift;
+ my $borrower = GetMemberDetails($borrowernumber);
+ my $issueslist = GetPendingIssues($borrowernumber);
foreach my $it (@$issueslist){
$it->{'date_due'}=format_date($it->{'date_due'});
}
diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm
index 2252be5..682248e 100644
--- a/C4/SIP/ILS/Patron.pm
+++ b/C4/SIP/ILS/Patron.pm
@@ -103,8 +103,7 @@ sub new {
# FIXME: populate fine_items recall_items
# $ilspatron{hold_items} = (GetReservesFromBorrowernumber($kp->{borrowernumber},'F'));
$ilspatron{unavail_holds} = [(GetReservesFromBorrowernumber($kp->{borrowernumber}))];
- my ($count,$issues) = GetPendingIssues($kp->{borrowernumber});
- $ilspatron{items} = $issues;
+ $ilspatron{items} = GetPendingIssues($kp->{borrowernumber});
$self = \%ilspatron;
$debug and warn Dumper($self);
syslog("LOG_DEBUG", "new ILS::Patron(%s): found patron '%s'", $patron_id,$self->{id});
diff --git a/circ/circulation.pl b/circ/circulation.pl
index bf2dca3..ed42e63 100755
--- a/circ/circulation.pl
+++ b/circ/circulation.pl
@@ -422,7 +422,7 @@ my @issued_itemtypes_count_loop;
if ($borrower) {
# get each issue of the borrower & separate them in todayissues & previous issues
- my ($countissues,$issueslist) = GetPendingIssues($borrower->{'borrowernumber'});
+ my ($issueslist) = GetPendingIssues($borrower->{'borrowernumber'});
# split in 2 arrays for today & previous
foreach my $it ( @$issueslist ) {
@@ -482,7 +482,7 @@ FROM issuingrules
WHERE categorycode=?
" );
#my @issued_itemtypes_count; # huh?
-$issueqty_sth->execute("*"); # FIXME: Why have a WHERE clause at all with a hardcoded "*"?
+$issueqty_sth->execute("*"); # This is a literal asterisk, not a wildcard.
while ( my $data = $issueqty_sth->fetchrow_hashref() ) {
diff --git a/members/deletemem.pl b/members/deletemem.pl
index 33f35ae..fe0cbee 100755
--- a/members/deletemem.pl
+++ b/members/deletemem.pl
@@ -1,11 +1,9 @@
#!/usr/bin/perl
-
#script to delete items
#written 2/5/00
#by chris at katipo.co.nz
-
# Copyright 2000-2002 Katipo Communications
#
# This file is part of Koha.
@@ -31,11 +29,8 @@ use C4::Output;
use C4::Auth;
use C4::Members;
-
my $input = new CGI;
-my $flagsrequired;
-$flagsrequired->{borrowers}=1;
my ($template, $borrowernumber, $cookie)
= get_template_and_user({template_name => "members/deletemem.tmpl",
query => $input,
@@ -47,9 +42,8 @@ my ($template, $borrowernumber, $cookie)
#print $input->header;
my $member=$input->param('member');
-my %member2;
-$member2{'borrowernumber'}=$member;
-my ($countissues,$issues)=GetPendingIssues($member);
+my $issues = GetPendingIssues($member); # FIXME: wasteful call when really, we only want the count
+my $countissues = scalar(@$issues);
my ($bor)=GetMemberDetails($member,'');
my $flags=$bor->{flags};
@@ -74,7 +68,6 @@ my $dbh = C4::Context->dbh;
my $sth=$dbh->prepare("Select * from borrowers where guarantorid=?");
$sth->execute($member);
my $data=$sth->fetchrow_hashref;
-$sth->finish;
if ($countissues > 0 or $flags->{'CHARGES'} or $data->{'borrowernumber'}){
# print $input->header;
$template->param(borrowernumber => $member);
@@ -84,7 +77,7 @@ if ($countissues > 0 or $flags->{'CHARGES'} or $data->{'borrowernumber'}){
if ($flags->{'CHARGES'} ne '') {
$template->param(charges => $flags->{'CHARGES'}->{'amount'});
}
- if ($data ne '') {
+ if ($data) {
$template->param(guarantees => 1);
}
output_html_with_http_headers $input, $cookie, $template->output;
diff --git a/members/moremember.pl b/members/moremember.pl
index 5688ae9..941272d 100755
--- a/members/moremember.pl
+++ b/members/moremember.pl
@@ -216,7 +216,8 @@ my $lib2 = &GetSortDetails( "Bsort2", $data->{'sort2'} );
# current issues
#
-my ( $count, $issue ) = GetPendingIssues($borrowernumber);
+my $issue = GetPendingIssues($borrowernumber);
+my $count = scalar(@$issue);
my $roaddetails = &GetRoadTypeDetails( $data->{'streettype'} );
my $today = POSIX::strftime("%Y-%m-%d", localtime); # iso format
my @issuedata;
diff --git a/opac/opac-ics.pl b/opac/opac-ics.pl
index ce35edf..9206e8b 100755
--- a/opac/opac-ics.pl
+++ b/opac/opac-ics.pl
@@ -51,7 +51,7 @@ my ( $borr ) = GetMemberDetails( $borrowernumber );
my $calendar = Data::ICal->new();
# get issued items ....
-my ($countissues,$issues) = GetPendingIssues($borrowernumber);
+my ($issues) = GetPendingIssues($borrowernumber);
foreach my $issue ( @$issues ) {
my $vevent = Data::ICal::Entry::Event->new();
diff --git a/opac/opac-user.pl b/opac/opac-user.pl
index dd69072..807fdbf 100755
--- a/opac/opac-user.pl
+++ b/opac/opac-user.pl
@@ -90,7 +90,7 @@ $template->param( BORROWER_INFO => \@bordat,
);
#get issued items ....
-my ($countissues,$issues) = GetPendingIssues($borrowernumber);
+my ($issues) = GetPendingIssues($borrowernumber);
my @issue_list = sort { $b->{'date_due'} cmp $a->{'date_due'} } @$issues;
my $count = 0;
diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl
index 6ff565d..e7ff367 100755
--- a/opac/sco/sco-main.pl
+++ b/opac/sco/sco-main.pl
@@ -136,7 +136,7 @@ if ($borrower->{cardnumber}) {
my $bornum = $borrower->{borrowernumber};
my $borrowername = $borrower->{firstname} . " " . $borrower->{surname};
my @issues;
- my ($countissues,$issueslist) = GetPendingIssues($borrower->{'borrowernumber'});
+ my ($issueslist) = GetPendingIssues($borrower->{'borrowernumber'});
foreach my $it ( @$issueslist ) {
push @issues, $it;
$cnt++;
--
1.5.5.GIT
More information about the Koha-patches
mailing list