http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 --- Comment #1 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 9317 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9317&action=edit Proposed patch -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9317|0 |1 is obsolete| | --- Comment #2 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 9536 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9536&action=edit 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: Jared Camins-Esakov <jcamins@cpbibliography.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@biblibre.co | |m --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- QA Comments: 1/ You remove the FIXME in getAllIssues (sanity-check order and limit) but $order is not sanitized. Please call $dbh->quote with the $order variable. 2/ Why do you remove the parenthesis for the IF statement (template) ? It seems mandatory by the koha guidelines. But I agree with you, it is useless :) Failed QA for 1. Replacement of fetchrow_hashref with a more adapted fetchall_arrayref is good. I think there is others occurrences of this into the koha code. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9536|0 |1 is obsolete| | Status|Failed QA |Needs Signoff Assignee|gmcharlt@gmail.com |colin.campbell@ptfs-europe. | |com --- Comment #4 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 10192 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10192&action=edit Proposed Amended Patch Patch rebased against master. Have added sanitizing of order and limit parameters (although order is currently always passed a constant string from the caller) Note on issue 2 adding parens around the variable forces the tt parser to evaluate as an expression. i.e. potentially adding processing where a simple variable is used. While it was used defensively in converting templates I'd suggest following normal tt style in new code -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10192|0 |1 is obsolete| | --- Comment #6 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 11330 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11330&action=edit Patch rebased against current head Rebase of patch against HEAD -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.poulain@biblibre.com --- Comment #8 from Paul Poulain <paul.poulain@biblibre.com> --- I realize that this patch modify a SELECT * that is LARGE as it's a JOIN on issues / biblio / items / biblioitems I agree it was like this before the patch. But Colin, could you improve it to return only needed columns, because that's probably also a source of the perfs problem. Side note : there is a call to +my $branches = GetBranches(); +foreach my $issue ( @{$issues} ) { + $issue->{issuingbranch} = $branches->{ $issue->{branchcode} }->{branchname}; } That could be replaced by another JOIN (not sure it's a good idea, just sharing) (not failing QA, the mistake was here before your patch. If you can't provide a follow-up, i'll push like this. But if you agree for a follow-up, it will be cool ;-) ) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 --- Comment #10 from Paul Poulain <paul.poulain@biblibre.com> --- Colin = pinging my comment 8 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Patch doesn't apply --- Comment #12 from Paul Poulain <paul.poulain@biblibre.com> --- Sorry, but the patch does not apply anymore, because of bug 6494. Could you rebase & resubmit (conflict not trivial to solve: I feel most of it is tidying code, but to avoid any mistake, I let you do) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA --- Comment #14 from Jonathan Druart <jonathan.druart@biblibre.com> --- switching back to passed QA -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz --- Comment #16 from Chris Cormack <chris@bigballofwax.co.nz> --- I would like this in 3.8.x but unfortunately does not apply cleanly # Changes to be committed: # # modified: C4/Members.pm # modified: koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt # modified: members/readingrec.pl # # Unmerged paths: # (use "git add/rm <file>..." as appropriate to mark resolution) # # both modified: koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt # both modified: opac/opac-readingrecord.pl -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8017 --- Comment #18 from Chris Cormack <chris@bigballofwax.co.nz> --- Patch contains no string changes, makes some template changes which I think are safe, pushed to 3.8.x, will be in 3.8.5 -- You are receiving this mail because: You are watching all bug changes.