From 267fe06f0a48f8ddb0bd2cae48cec8d831df84d2 Mon Sep 17 00:00:00 2001 From: Joe Atzberger Date: Wed, 7 Jan 2009 21:47:36 -0600 Subject: [PATCH] Bug 2900: fix GetPendingIssues. 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. Signed-off-by: Galen Charlton --- 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 09f9a0803e..42b9d2a81d 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, C, and C 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, C, and C tables. +The keys include C 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 582ecdef96..69986dc740 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 257f9e23ab..74b7936ce4 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -102,8 +102,7 @@ sub new { # FIXME: populate items 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 f165eaefe4..5991e086ee 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -420,7 +420,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 ) { @@ -480,7 +480,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 33f35ae9a9..fe0cbeebf4 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@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 03d332365e..151f9c2a30 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 ce35edf4e6..9206e8bfd9 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 71b19b9383..e38ebc0403 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -89,7 +89,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 197975816c..3701f4d77b 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -132,7 +132,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++; -- 2.39.5