From ef7ebc4ed4004dd3287e87500a6cce266c59cba6 Mon Sep 17 00:00:00 2001 From: Colin Campbell Date: Fri, 27 Apr 2012 18:20:08 +0100 Subject: [PATCH] Bug 8017 reduce manipulation of GetAllIssues return GetAllIssues can produce large lists For performance purposes: Dont loop over the list without cause Dont do expensive processing in the loop Dont needlessly copy the array Do display formatting in the template Dont extract the barcode list unless we are producing it Reduce db calls by using the data to hand Make the table in the template a bit more readable where everything was stuffed into one line Signed-off-by: Paul Poulain --- C4/Members.pm | 19 +--- .../prog/en/modules/members/readingrec.tt | 37 +++--- .../prog/en/modules/opac-readingrecord.tt | 73 ++++++++---- members/readingrec.pl | 103 +++++++---------- opac/opac-readingrecord.pl | 105 +++++++++--------- 5 files changed, 178 insertions(+), 159 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 2e315aadc1..b2f45b955b 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1072,10 +1072,9 @@ C tables of the Koha database. sub GetAllIssues { my ( $borrowernumber, $order, $limit ) = @_; - #FIXME: sanity-check order and limit - my $dbh = C4::Context->dbh; + my $dbh = C4::Context->dbh; my $query = - "SELECT *, issues.timestamp as issuestimestamp, issues.renewals AS renewals,items.renewals AS totalrenewals,items.timestamp AS itemstimestamp +'SELECT *, issues.timestamp as issuestimestamp, issues.renewals AS renewals,items.renewals AS totalrenewals,items.timestamp AS itemstimestamp FROM issues LEFT JOIN items on items.itemnumber=issues.itemnumber LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber @@ -1088,20 +1087,14 @@ sub GetAllIssues { LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber LEFT JOIN biblioitems ON items.biblioitemnumber=biblioitems.biblioitemnumber WHERE borrowernumber=? AND old_issues.itemnumber IS NOT NULL - order by $order"; - if ( $limit != 0 ) { + order by ' . $order; + if ($limit) { $query .= " limit $limit"; } my $sth = $dbh->prepare($query); - $sth->execute($borrowernumber, $borrowernumber); - my @result; - my $i = 0; - while ( my $data = $sth->fetchrow_hashref ) { - push @result, $data; - } - - return \@result; + $sth->execute( $borrowernumber, $borrowernumber ); + return $sth->fetchall_arrayref( {} ); } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt index 3b1f55a3e8..b7c5f53ac2 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt @@ -1,3 +1,4 @@ +[% USE KohaDates %] [% INCLUDE 'doc-head-open.inc' %] Circulation History for [% INCLUDE 'patron-title.inc' %] [% INCLUDE 'doc-head-close.inc' %] @@ -26,7 +27,7 @@
[% INCLUDE 'circ-toolbar.inc' %]

Circulation history

-[% IF ( loop_reading ) %] +[% IF loop_reading %]
@@ -43,29 +44,37 @@ Date due Return date -[% FOREACH loop_readin IN loop_reading %] - [% IF ( loop_readin.returndate ) %][% ELSE %][% END %] +[% FOREACH issue IN loop_reading %] + [% IF issue.returndate %][% ELSE %][% END %] - [% loop_readin.issuestimestamp %] + [% issue.issuestimestamp | $KohaDates %] - [% loop_readin.title |html %] + [% issue.title |html %] - [% loop_readin.author %] + [% issue.author %] - [% loop_readin.classification %] + + [% IF issue.classification %] + [% issue.classification %] + [% ELSE %] + [% issue.itemcallnumber %] + [% END %] + - [% loop_readin.barcode %] + [% issue.barcode %] - [% loop_readin.renewals %] + [% issue.renewals %] - [% loop_readin.issuedate %] + [% issue.issuedate | $KohaDates %] - [% loop_readin.issuingbranch %] - [% IF ( loop_readin.date_due ) %][% loop_readin.date_due %][% ELSE %] [% END %] + [% issue.issuingbranch %] + [% IF issue.date_due %] + [% issue.date_due | $KohaDates %] + [% ELSE %] [% END %] - [% IF ( loop_readin.returndate ) %] - [% loop_readin.returndate %] + [% IF issue.returndate %] + [% issue.returndate | $KohaDates %] [% ELSE %] Checked Out [% END %] diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt index c1047aed15..05f4c45238 100644 --- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt +++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt @@ -26,7 +26,7 @@ $(document).ready(function(){

[% firstname %] [% surname %]'s account ⇢ Checkout history

-[% UNLESS ( count ) %] +[% IF READING_RECORD.size() == 0 %] You have never borrowed anything from this library. [% ELSE %]
@@ -64,32 +64,67 @@ You have never borrowed anything from this library. [% END %] -[% FOREACH READING_RECOR IN READING_RECORD %] +[% FOREACH issue IN READING_RECORD %] -[% UNLESS ( loop.odd ) %][% ELSE %][% END %] +[% IF loop.even %][% ELSE %][% END %] -[% IF ( OPACAmazonCoverImages ) %][% IF ( READING_RECOR.normalized_isbn ) %]Cover Image[% ELSE %]No cover image available[% END %][% END %] +[% IF OPACAmazonCoverImages %] + [% IF issue.normalized_isbn %] + Cover Image + [% ELSE %] + No cover image available + [% END %] +[% END %] - [% IF ( GoogleJackets ) %][% IF ( READING_RECOR.normalized_isbn ) %]
[% ELSE %]No cover image available[% END %][% END %] +[% IF GoogleJackets %] + [% IF issue.normalized_isbn %] +
+ [% ELSE %] + No cover image available + [% END %] +[% END %] - [% IF ( BakerTaylorEnabled ) %][% IF ( READING_RECOR.normalized_isbn ) %]See Baker & Taylor[% ELSE %]No cover image available[% END %][% END %] +[% IF BakerTaylorEnabled %] + [% IF issue.normalized_isbn %] + See Baker & Taylor + [% ELSE %] + No cover image available + [% END %] +[% END %] - [% IF ( SyndeticsEnabled ) %][% IF ( SyndeticsCoverImages ) %][% IF ( using_https ) %] - - [% ELSE %] - [% END %][% END %][% END %] +[% IF SyndeticsEnabled %] + [% IF SyndeticsCoverImages %] + [% IF using_https %] + + [% ELSE %] + + [% END %] + [% END %] +[% END %] -[% IF ( READING_RECOR.BiblioDefaultViewmarc ) %][% READING_RECOR.title |html %] [% IF ( READING_RECOR.subtitle ) %][% FOREACH subtitl IN READING_RECOR.subtitle %][% subtitl.subfield %][% END %][% END %][% ELSE %] -[% IF ( READING_RECOR.BiblioDefaultViewisbd ) %][% READING_RECOR.title |html %] [% IF ( READING_RECOR.subtitle ) %][% FOREACH subtitl IN READING_RECOR.subtitle %][% subtitl.subfield %][% END %][% END %][% ELSE %] -[% READING_RECOR.title |html %] [% IF ( READING_RECOR.subtitle ) %][% FOREACH subtitl IN READING_RECOR.subtitle %][% subtitl.subfield %][% END %][% END %][% END %][% END %] + +[% IF issue.BiblioDefaultViewmarc %] + [% issue.title |html %] [% IF issue.subtitle %][% FOREACH subtitl IN issue.subtitle %][% subtitl.subfield %][% END %][% END %] +[% ELSIF issue.BiblioDefaultViewisbd %] + [% issue.title |html %] [% IF issue.subtitle %][% FOREACH subtitl IN issue.subtitle %][% subtitl.subfield %][% END %][% END %] +[% ELSE %] + [% issue.title |html %] [% IF issue.subtitle %][% FOREACH subtitl IN issue.subtitle %][% subtitl.subfield %][% END %][% END %] +[% END %] - [% READING_RECOR.author %] + [% issue.author %] -[% UNLESS ( noItemTypeImages ) %][% IF ( READING_RECOR.imageurl ) %][% END %][% END %] [% READING_RECOR.description %] -[% READING_RECOR.itemcallnumber %] -[% IF ( READING_RECOR.returndate ) %][% READING_RECOR.returndate | $KohaDates %][% ELSE %](Checked out)[% END %] -[% IF ( OPACMySummaryHTML ) %] -[% READING_RECOR.MySummaryHTML %] + +[% UNLESS ( noItemTypeImages ) %][% IF ( issue.imageurl ) %][% END %][% END %] [% issue.description %] +[% issue.itemcallnumber %] + +[% IF issue.returndate %] + [% issue.returndate | $KohaDates %] +[% ELSE %] + (Checked out) +[% END %] + +[% IF OPACMySummaryHTML %] + [% issue.MySummaryHTML %] [% END %] diff --git a/members/readingrec.pl b/members/readingrec.pl index d95cc8f558..d1cf5cfb45 100755 --- a/members/readingrec.pl +++ b/members/readingrec.pl @@ -28,7 +28,7 @@ use CGI; use C4::Auth; use C4::Output; use C4::Members; -use C4::Branch; +use C4::Branch qw(GetBranches); use List::MoreUtils qw/any uniq/; use Koha::DateUtils; @@ -62,44 +62,29 @@ if ($input->param('borrowernumber')) { my $order = 'date_due desc'; my $limit = 0; -my ( $issues ) = GetAllIssues($borrowernumber,$order,$limit); - -my @loop_reading; -my @barcodes; -my $today = C4::Dates->new(); -$today = $today->output("iso"); - -foreach my $issue (@{$issues}){ - my %line; - $line{issuestimestamp} = format_date($issue->{'issuestimestamp'}); - $line{biblionumber} = $issue->{'biblionumber'}; - $line{title} = $issue->{'title'}; - $line{author} = $issue->{'author'}; - $line{classification} = $issue->{'classification'} || $issue->{'itemcallnumber'}; - $line{date_due} = format_sqldatetime($issue->{date_due}); - $line{returndate} = format_sqldatetime($issue->{returndate}); - $line{issuedate} = format_sqldatetime($issue->{issuedate}); - $line{issuingbranch} = GetBranchName($issue->{'branchcode'}); - $line{renewals} = $issue->{'renewals'}; - $line{barcode} = $issue->{'barcode'}; - $line{volumeddesc} = $issue->{'volumeddesc'}; - push(@loop_reading,\%line); - my $return_dt = Koha::DateUtils::dt_from_string($issue->{'returndate'}, 'iso'); - if ( ( $input->param('op') eq 'export_barcodes' ) and ( $today eq $return_dt->ymd() ) ) { - push( @barcodes, $issue->{'barcode'} ); - } +my $issues = GetAllIssues($borrowernumber,$order,$limit); + +my $branches = GetBranches(); +foreach my $issue ( @{$issues} ) { + $issue->{issuingbranch} = $branches->{ $issue->{branchcode} }->{branchname}; } -if ($input->param('op') eq 'export_barcodes') { - my $borrowercardnumber = GetMember( borrowernumber => $borrowernumber )->{'cardnumber'} ; +# barcode export +if ( $input->param('op') eq 'export_barcodes' ) { + my $today = C4::Dates->new(); + $today = $today->output('iso'); + my @barcodes = + map { $_->{barcode} } grep { $_->{returndate} =~ m/^$today/o } @{$issues}; + my $borrowercardnumber = + GetMember( borrowernumber => $borrowernumber )->{'cardnumber'}; my $delimiter = "\n"; - binmode( STDOUT, ":encoding(UTF-8)"); + binmode( STDOUT, ":encoding(UTF-8)" ); print $input->header( -type => 'application/octet-stream', -charset => 'utf-8', -attachment => "$today-$borrowercardnumber-checkinexport.txt" ); - my $content = join($delimiter, uniq(@barcodes)); + my $content = join $delimiter, uniq(@barcodes); print $content; exit; } @@ -116,6 +101,7 @@ if (! $limit){ $limit = 'full'; } + my ($picture, $dberror) = GetPatronImage($data->{'cardnumber'}); $template->param( picture => 1 ) if $picture; @@ -128,36 +114,31 @@ if (C4::Context->preference('ExtendedPatronAttributes')) { } $template->param( - readingrecordview => 1, - biblionumber => $data->{'biblionumber'}, - title => $data->{'title'}, - initials => $data->{'initials'}, - surname => $data->{'surname'}, - othernames => $data->{'othernames'}, - borrowernumber => $borrowernumber, - limit => $limit, - firstname => $data->{'firstname'}, - cardnumber => $data->{'cardnumber'}, - categorycode => $data->{'categorycode'}, - category_type => $data->{'category_type'}, - # category_description => $data->{'description'}, - categoryname => $data->{'description'}, - address => $data->{'address'}, - address2 => $data->{'address2'}, - city => $data->{'city'}, - state => $data->{'state'}, - zipcode => $data->{'zipcode'}, - country => $data->{'country'}, - phone => $data->{'phone'}, - email => $data->{'email'}, - branchcode => $data->{'branchcode'}, - is_child => ($data->{'category_type'} eq 'C'), - branchname => GetBranchName($data->{'branchcode'}), - showfulllink => (scalar @loop_reading > 50), - loop_reading => \@loop_reading, - activeBorrowerRelationship => (C4::Context->preference('borrowerRelationship') ne ''), + readingrecordview => 1, + title => $data->{title}, + initials => $data->{initials}, + surname => $data->{surname}, + othernames => $data->{othernames}, + borrowernumber => $borrowernumber, + firstname => $data->{firstname}, + cardnumber => $data->{cardnumber}, + categorycode => $data->{categorycode}, + category_type => $data->{category_type}, + categoryname => $data->{description}, + address => $data->{address}, + address2 => $data->{address2}, + city => $data->{city}, + state => $data->{state}, + zipcode => $data->{zipcode}, + country => $data->{country}, + phone => $data->{phone}, + email => $data->{email}, + branchcode => $data->{branchcode}, + is_child => ( $data->{category_type} eq 'C' ), + branchname => $branches->{ $data->{branchcode} }->{branchname}, + loop_reading => $issues, + activeBorrowerRelationship => + ( C4::Context->preference('borrowerRelationship') ne '' ), ); output_html_with_http_headers $input, $cookie, $template->output; - - diff --git a/opac/opac-readingrecord.pl b/opac/opac-readingrecord.pl index 0c92de633b..4f16c66398 100755 --- a/opac/opac-readingrecord.pl +++ b/opac/opac-readingrecord.pl @@ -27,8 +27,10 @@ use C4::Biblio; use C4::Circulation; use C4::Members; use Koha::DateUtils; +use MARC::Record; use C4::Output; +use C4::Charset qw(StripNonXmlChars); my $query = new CGI; my ( $template, $borrowernumber, $cookie ) = get_template_and_user( @@ -50,65 +52,65 @@ $template->param(%{$borr}); my $itemtypes = GetItemTypes(); # get the record -my $order = $query->param('order') || ''; -if ( $order eq '' ) { - $order = "date_due desc"; - $template->param( orderbydate => 1 ); -} - +my $order = $query->param('order') || ''; if ( $order eq 'title' ) { $template->param( orderbytitle => 1 ); } - -if ( $order eq 'author' ) { +elsif ( $order eq 'author' ) { $template->param( orderbyauthor => 1 ); } - -my $limit = $query->param('limit') || 50; -if ( $limit eq 'full' ) { - $limit = 0; -} else { - $limit = 50; + $order = "date_due desc"; + $template->param( orderbydate => 1 ); } -my ( $issues ) = GetAllIssues( $borrowernumber, $order, $limit ); - - -my @loop_reading; - -foreach my $issue (@{$issues} ) { - my %line; - - my $record = GetMarcBiblio($issue->{'biblionumber'}); - - # XISBN Stuff - my $isbn = GetNormalizedISBN($issue->{'isbn'}); - $line{normalized_isbn} = $isbn; - $line{biblionumber} = $issue->{'biblionumber'}; - $line{title} = $issue->{'title'}; - $line{author} = $issue->{'author'}; - $line{itemcallnumber} = $issue->{'itemcallnumber'}; - $line{date_due} = $issue->{'date_due'}; - $line{returndate} = $issue->{'returndate'}; - $line{volumeddesc} = $issue->{'volumeddesc'}; - $issue->{'itemtype'} = C4::Context->preference('item-level_itypes') ? $issue->{'itype'} : $issue->{'itemtype'}; - if($issue->{'itemtype'}) { - $line{'description'} = $itemtypes->{ $issue->{'itemtype'} }->{'description'}; - $line{imageurl} = getitemtypeimagelocation( 'opac', $itemtypes->{ $issue->{'itemtype'} }->{'imageurl'} ); + +my $limit = $query->param('limit'); +$limit = ( $limit eq 'full' ) ? 0 : 50; + +my $issues = GetAllIssues( $borrowernumber, $order, $limit ); + +my $itype_attribute = + ( C4::Context->preference('item-level_itypes') ) ? 'itype' : 'itemtype'; + +my $opac_summary_html = C4::Context->preference('OPACMySummaryHTML'); +foreach my $issue ( @{$issues} ) { + $issue->{normalized_isbn} = GetNormalizedISBN( $issue->{isbn} ); + if ( $issue->{$itype_attribute} ) { + $issue->{description} = + $itemtypes->{ $issue->{$itype_attribute} }->{description}; + $issue->{imageurl} = + getitemtypeimagelocation( 'opac', + $itemtypes->{ $issue->{$itype_attribute} }->{imageurl} ); + } + if ( $issue->{marcxml} ) { + my $marcxml = StripNonXmlChars( $issue->{marcxml} ); + my $marc_rec = + MARC::Record::new_from_xml( $marcxml, 'utf8', + C4::Context->preference('marcflavour') ); + $issue->{subtitle} = + GetRecordValue( 'subtitle', $marc_rec, $issue->{frameworkcode} ); } # My Summary HTML - if (my $my_summary_html = C4::Context->preference('OPACMySummaryHTML')){ - $line{author} ? $my_summary_html =~ s/{AUTHOR}/$line{author}/g : $my_summary_html =~ s/{AUTHOR}//g; - $line{title} =~ s/\/+$//; # remove trailing slash - $line{title} =~ s/\s+$//; # remove trailing space - $line{title} ? $my_summary_html =~ s/{TITLE}/$line{title}/g : $my_summary_html =~ s/{TITLE}//g; - $line{normalized_isbn} ? $my_summary_html =~ s/{ISBN}/$line{normalized_isbn}/g : $my_summary_html =~ s/{ISBN}//g; - $line{biblionumber} ? $my_summary_html =~ s/{BIBLIONUMBER}/$line{biblionumber}/g : $my_summary_html =~ s/{BIBLIONUMBER}//g; - $line{MySummaryHTML} = $my_summary_html; + if ($opac_summary_html) { + my $my_summary_html = $opac_summary_html; + $issue->{author} + ? $my_summary_html =~ s/{AUTHOR}/$issue->{author}/g + : $my_summary_html =~ s/{AUTHOR}//g; + my $title = $issue->{title}; + $title =~ s/\/+$//; # remove trailing slash + $title =~ s/\s+$//; # remove trailing space + $title + ? $my_summary_html =~ s/{TITLE}/$title/g + : $my_summary_html =~ s/{TITLE}//g; + $issue->{normalized_isbn} + ? $my_summary_html =~ s/{ISBN}/$issue->{normalized_isbn}/g + : $my_summary_html =~ s/{ISBN}//g; + $issue->{biblionumber} + ? $my_summary_html =~ s/{BIBLIONUMBER}/$issue->{biblionumber}/g + : $my_summary_html =~ s/{BIBLIONUMBER}//g; + $issue->{MySummaryHTML} = $my_summary_html; } - push( @loop_reading, \%line ); - $line{subtitle} = GetRecordValue('subtitle', $record, GetFrameworkCode($issue->{'biblionumber'})); } if (C4::Context->preference('BakerTaylorEnabled')) { @@ -135,12 +137,11 @@ for(qw(AmazonCoverImages GoogleJackets)) { # BakerTaylorEnabled handled above } $template->param( - READING_RECORD => \@loop_reading, + READING_RECORD => $issues, limit => $limit, showfulllink => 1, - readingrecview => 1, - count => scalar @loop_reading, - OPACMySummaryHTML => (C4::Context->preference("OPACMySummaryHTML")) ? 1 : 0, + readingrecview => 1, + OPACMySummaryHTML => $opac_summary_html ? 1 : 0, ); output_html_with_http_headers $query, $cookie, $template->output; -- 2.39.5