[Bug 28013] New: Improvements to CanBookBeRenewed
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Bug ID: 28013 Summary: Improvements to CanBookBeRenewed Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com CanBookBeRenewed has a few performance issues: 1 - We call CheckReserves multiple times, however, a single call returns all possible reserves 2 - We fetch items and patrons one at a time, however, we can use a search to get all at once -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 118592 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118592&action=edit Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27032 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com, | |stalkernoid@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118592|0 |1 is obsolete| | CC| |amitddng135@gmail.com --- Comment #2 from Amit Gupta <amitddng135@gmail.com> --- Created attachment 118928 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118928&action=edit Bug 28013: Performance improvements to CanBookBeRenewed To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi QA Contact|testopia@bugs.koha-communit |joonas.kylmala@helsinki.fi |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118928|0 |1 is obsolete| | --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 118950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118950&action=edit Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t Signed-off-by: Amit Gupta <amitddng135@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 118951 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118951&action=edit Bug 28013: (QA follow-up) Remove unused variable Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Signed Off --- Comment #5 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- I want to check one more thing.. Moving this back to SO meanwhile. I wonder what happens with when there is two waiting holds and one checked out item. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #6 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Ok, that works as well. Passing QA. The new code is is more simple! :) Hopefully we can get rid of the remaining bugs in this function in other bug reports. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Status|Passed QA |In Discussion --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I don't manage to convince myself it's correct. There is an edge case, looking at the code: The patron iterator is set before the ITEM loop. On the second item, it won't be reset and we won't iterate over all the patrons. Nick and/or Joonas, can you double check it please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |28078 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28078 [Bug 28078] Add option to ignore hold counts when checking CanItemBeReserved -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 119125 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119125&action=edit Bug 28013: (follow-up) Correct and update call to CanItemBeReserved The current call checks for truth against a returned hash, we need to check the 'status' value -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 119126 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119126&action=edit Bug 28013: Unit tests Cover the case of multiple patrons and multiple items that can fill them -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #10 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 119127 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119127&action=edit Bug 28013: (follow-up) Reset the patron loop To test: 1 - Apply all other patches and dependencies 2 - prove -v t/db_dependent/Circulation.t 3 - It fails 4 - Apply this patch 5 - It passes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #7)
I don't manage to convince myself it's correct. There is an edge case, looking at the code:
The patron iterator is set before the ITEM loop. On the second item, it won't be reset and we won't iterate over all the patrons.
Nick and/or Joonas, can you double check it please?
Indeed, simply resetting after the loop should cover that. Adding test to prove that, however, uncovered additional problems. I filed bug 28078 and updated code here. I set to SO for another QA round to check what I added. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #12 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Nick, is it possible to drop the dependency to bug 28078? I mean if we don't take it would the behaviour change with your refactoring here or stay same? Please see my comments in bug 28078, it is probably not the right solution there so if we can drop that as dependency to this it would be great as the discussion over there wouldn't block this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Joonas Kylmälä from comment #12)
Nick, is it possible to drop the dependency to bug 28078? I mean if we don't take it would the behaviour change with your refactoring here or stay same? Please see my comments in bug 28078, it is probably not the right solution there so if we can drop that as dependency to this it would be great as the discussion over there wouldn't block this.
If we only keep the last patch here and drop the correction of the call to CanItemBeReserved and the units tests we can. They can be filed as separate bugs if you think that is best -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #14 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Nick Clemens from comment #13)
(In reply to Joonas Kylmälä from comment #12)
Nick, is it possible to drop the dependency to bug 28078? I mean if we don't take it would the behaviour change with your refactoring here or stay same? Please see my comments in bug 28078, it is probably not the right solution there so if we can drop that as dependency to this it would be great as the discussion over there wouldn't block this.
If we only keep the last patch here and drop the correction of the call to CanItemBeReserved and the units tests we can.
They can be filed as separate bugs if you think that is best
I think it is for the best. Isn't the bug already reported in: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Nick, I reviewed the bug 28078 again and your fix there seems great. I think we can keep the patches here like this, I will QA the whole thing after the minor bug I spotted in bug 28078 is fixed. Sounds good? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Status|Passed QA |Signed Off Status|Signed Off |Passed QA Attachment #118950|0 |1 is obsolete| | Attachment #118951|0 |1 is obsolete| | Attachment #119125|0 |1 is obsolete| | Attachment #119126|0 |1 is obsolete| | Attachment #119127|0 |1 is obsolete| | Attachment #119253|0 |1 is obsolete| | --- Comment #16 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Moving back to signed-off until I am able to upload the patches with my sign-off (currently bugzilla is experiencing some difficulties) --- Comment #17 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 119253 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119253&action=edit Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t Signed-off-by: Amit Gupta <amitddng135@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Comment #18 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 119257 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119257&action=edit Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t Signed-off-by: Amit Gupta <amitddng135@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #19 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 119258 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119258&action=edit Bug 28013: (QA follow-up) Remove unused variable Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #20 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 119259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119259&action=edit Bug 28013: (follow-up) Correct and update call to CanItemBeReserved The current call checks for truth against a returned hash, we need to check the 'status' value Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #21 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 119260 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119260&action=edit Bug 28013: Unit tests Cover the case of multiple patrons and multiple items that can fill them Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #22 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 119261 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119261&action=edit Bug 28013: (follow-up) Reset the patron loop To test: 1 - Apply all other patches and dependencies 2 - prove -v t/db_dependent/Circulation.t 3 - It fails 4 - Apply this patch 5 - It passes Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #23 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Passing QA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 --- Comment #25 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- *** Bug 26659 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.05.00 |21.05.00,20.11.06 released in| | CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable --- Comment #26 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Bug 28013 depends on bug 28078, which changed state. Bug 28078 Summary: Add option to ignore hold counts when checking CanItemBeReserved https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28078 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|21.05.00,20.11.06 |21.05.00,20.11.06,20.05.12 released in| | CC| |andrew@bywatersolutions.com --- Comment #27 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Pushed to 20.05.x for 20.05.12 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |victor@tuxayo.net Status|Pushed to oldstable |RESOLVED --- Comment #28 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org