[Bug 7016] New: CanBookBeReserved uses GetItemsInfo unnecessarily
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Bug #: 7016 Summary: CanBookBeReserved uses GetItemsInfo unnecessarily Classification: Unclassified Change sponsored?: --- Product: Koha Version: rel_3_6 Platform: All OS/Version: All Status: NEW Severity: normal Priority: P5 Component: Architecture, internals, and plumbing AssignedTo: ian.walls@bywatersolutions.com ReportedBy: ian.walls@bywatersolutions.com QAContact: koha-bugs@lists.koha-community.org C4::Reserves::CanBookBeReserved uses GetItemsInfo($biblionumber) to get the itemnumbers for all the items attached to the biblio in question. However, GetItemsInfo is a very heavy-weight subroutine, pulling in data from all over the database. This data includes a run of C4::Reserves::CheckReserves(), as well as pulling in the last three borrowers from old_issues. Using get_itemnumbers_of(($biblionumber)) would greatly reduce the cost of CanBookBeReserved. -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 --- Comment #1 from Ian Walls <ian.walls@bywatersolutions.com> 2011-10-12 19:38:54 UTC --- Created attachment 5861 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=5861 Proposed Patch Replaces CanBookBeReserved()'s call to GetItemsInfo with a call to get_itemnumbers_of, to save on system resources. -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <ian.walls@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 |PATCH-Sent Patch Status|--- |Needs Signoff -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nengard@gmail.com Patch Status|Needs Signoff |Does not apply --- Comment #2 from Nicole C. Engard <nengard@gmail.com> 2011-10-19 19:50:08 UTC --- nengard@debian:~/kohaclone$ git bz apply 7016 Bug 7016 - CanBookBeReserved uses GetItemsInfo unnecessarily Proposed Patch Apply? [yn] y Applying: Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily error: patch failed: C4/Reserves.pm:382 error: C4/Reserves.pm: patch does not apply Patch failed at 0001 Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Proposed-Patch-Jv873P.patch nengard@debian:~/kohaclone$ -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <ian.walls@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #5861|0 |1 is obsolete| | --- Comment #3 from Ian Walls <ian.walls@bywatersolutions.com> 2011-10-19 20:01:44 UTC --- Created attachment 5998 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=5998 Proposed patch (rebased for analytics code) With the addition of Host Items, having a more efficient subroutine here is even more important. Thankfully, a get_hostitemnumbers_of subroutine was provided in parallel to the get_itemnumbers_of, so no compromises needed to be made. -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <ian.walls@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch Status|Does not apply |Needs Signoff -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #5998|0 |1 is obsolete| | --- Comment #4 from Nicole C. Engard <nengard@gmail.com> 2011-10-19 20:05:43 UTC --- Created attachment 5999 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=5999 [SIGNED-OFF] Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily CanBookBeReserved uses the very heavy-weight function GetItemsInfo to simply retrieve the itemnumbers attached to a given biblio. The function get_itemnumbers_of() is much lighter-weight and returns the required information; switching to it will reduce the overall system resource cost of placing a hold. Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch Status|Needs Signoff |Signed Off -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <ian.walls@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch Status|Signed Off |Passed QA --- Comment #5 from Ian Walls <ian.walls@bywatersolutions.com> 2011-10-19 22:15:24 UTC --- Marking Passed QA to catch RM's attention ;). Chris, if you also think this is a sound change, please push, otherwise it can wait. -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz Patch Status|Passed QA |Patch Pushed --- Comment #6 from Chris Cormack <chris@bigballofwax.co.nz> 2011-10-19 22:56:13 UTC --- I have no issues with this, works and is a lot lighter, pushing -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Maxime Pelletier <pelletiermaxime@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pelletiermaxime@gmail.com Severity|normal |major --- Comment #7 from Maxime Pelletier <pelletiermaxime@gmail.com> 2012-01-06 14:32:45 UTC --- This patch broke the function. CanItemBeReserved needs an itemnumber and it seems that it now gets an array of hash (for exemple you would get itemnumber with $item->{$biblionumber}->[0]). I would suggest either reverting this patch or using/adding subs that just return an array of itemnumber correctly. -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Maxime Pelletier <pelletiermaxime@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|PATCH-Sent |P1 - high -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 --- Comment #8 from Ian Walls <ian.walls@bywatersolutions.com> 2012-01-06 15:10:20 UTC --- Created attachment 7071 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=7071 Proposed Followup Adds a new subroutine to just get an array of itemnumbers for any given biblionumber, and replaces the use of get_itemnumbers_of in CanBookBeReserved. -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |ASSIGNED -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #9 from Ian Walls <koha.sekjal@gmail.com> --- Followup patch needs signoff still, I believe. -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 --- Comment #10 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 9127 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9127&action=edit Bug 7016 Followup: Add new GetItemnumberForBiblio subroutine Adds a new subroutine in C4::Items, GetItemnumbersForBiblio, which takes a single biblionumber, and returns an array of all the corresponding itemnumbers. This patch also replaces the usage of get_itemnumbers_of in C4::Reserves::CanBookBeReserved with this new subroutine, as the output is more consistent with what we were lookng for (this is what fixes the bug issue). Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 9128 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9128&action=edit bug 7016 further followup: clarify return of GetItemnumbersForBiblio New function was actually returning an arrayref, so made perldoc and function usage consistent. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #7071|0 |1 is obsolete| | --- Comment #12 from Galen Charlton <gmcharlt@gmail.com> --- Comment on attachment 7071 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=7071 Proposed Followup obsoleted by signed-off attachments -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |gmcharlt@gmail.com --- Comment #13 from Galen Charlton <gmcharlt@gmail.com> --- Now signed off - note that it's a two patch series: Bug 7016 Followup: Add new GetItemnumberForBiblio subroutine bug 7016 further followup: clarify return of GetItemnumbersForBiblio -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|rel_3_6 |rel_3_8 Severity|major |blocker --- Comment #14 from Galen Charlton <gmcharlt@gmail.com> --- Marking as blocker for 3.8; without the two follow-up patches, holdability checking is effectively broken in 3.6 and 3.8. -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #5999|0 |1 is obsolete| | -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |m.de.rooy@rijksmuseum.nl QA Contact|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | --- Comment #15 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: Two followups look good to me. Marking as Passed QA. No translation issues involved. -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |paul.poulain@biblibre.com Version|rel_3_8 |rel_3_6 --- Comment #16 from Paul Poulain <paul.poulain@biblibre.com> --- comment 7 describe the problem, however i'm surprised that noone has spotted it before. I've pushed the patch, but could not see the bugguy behaviour and the fixed behaviour. Maybe that's because of my setup, or a missing test plan. Anyway, patch pushed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 --- Comment #17 from Chris Cormack <chris@bigballofwax.co.nz> --- Hmm have you not merged yet? Maybe could you not set pushed to Master until its in master ... it confuses me everytime, maybe just set that when you merge and push to master? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7016 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org