Bug 7016

Summary: CanBookBeReserved uses GetItemsInfo unnecessarily
Product: Koha Reporter: Ian Walls <koha.sekjal>
Component: Architecture, internals, and plumbingAssignee: Ian Walls <koha.sekjal>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: blocker    
Priority: P1 - high CC: chris, gmcharlt, m.de.rooy, nengard, paul.poulain, pelletiermaxime
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed Patch
Proposed patch (rebased for analytics code)
[SIGNED-OFF] Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily
Proposed Followup
Bug 7016 Followup: Add new GetItemnumberForBiblio subroutine
bug 7016 further followup: clarify return of GetItemnumbersForBiblio

Description Ian Walls 2011-10-12 19:30:19 UTC
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.
Comment 1 Ian Walls 2011-10-12 19:38:54 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 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$
Comment 3 Ian Walls 2011-10-19 20:01:44 UTC Comment hidden (obsolete)
Comment 4 Nicole C. Engard 2011-10-19 20:05:43 UTC Comment hidden (obsolete)
Comment 5 Ian Walls 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.
Comment 6 Chris Cormack 2011-10-19 22:56:13 UTC
I have no issues with this, works and is a lot lighter, pushing
Comment 7 Maxime Pelletier 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.
Comment 8 Ian Walls 2012-01-06 15:10:20 UTC Comment hidden (obsolete)
Comment 9 Ian Walls 2012-04-11 19:59:36 UTC
Followup patch needs signoff still, I believe.
Comment 10 Galen Charlton 2012-04-11 20:16:20 UTC
Created attachment 9127 [details] [review]
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>
Comment 11 Galen Charlton 2012-04-11 20:16:38 UTC
Created attachment 9128 [details] [review]
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>
Comment 12 Galen Charlton 2012-04-11 20:18:16 UTC
Comment on attachment 7071 [details] [review]
Proposed Followup

obsoleted by signed-off attachments
Comment 13 Galen Charlton 2012-04-11 20:19:10 UTC
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
Comment 14 Galen Charlton 2012-04-11 20:27:47 UTC
Marking as blocker for 3.8; without the two follow-up patches, holdability checking is effectively broken in 3.6 and 3.8.
Comment 15 Marcel de Rooy 2012-04-16 08:39:15 UTC
QA Comment: Two followups look good to me. Marking as Passed QA. No translation issues involved.
Comment 16 Paul Poulain 2012-04-18 14:54:06 UTC
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
Comment 17 Chris Cormack 2012-04-18 21:08:42 UTC
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?