https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127610&action=edit Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t -- 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=29474 --- Comment #3 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- (In reply to Joonas Kylmälä from comment #2)

I think adding a comment like "cron wants 'too_soon' over 'on_reserve'"

This sentence was unfinished.. I meant adding this comment next to the code when returning the 'too_soon' reason near the _CanBookBeAutoRenewed function call. -- 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=29474 --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Joonas Kylmälä from comment #2)

If you remove the unless ... makes the code much simpler.

Done

Please also move the cron return right after the call to

...

$auto_renew = _CanBookBeAutoRenewed({

I don't believe I can. That is inside "unless ($override_limit)" so it would make 'too_soon' subject to override

Please include stats too, but maybe you are at it, I noticed that this bug is not even yet for sign-off and I'm here reviewing ;D

Yes, seeing what else I can improve first, will add when ready. Feel free to add to/alter my patches - I appreciate the help! -- 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=29474 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Nick Clemens from comment #5)

...

Please also move the cron return right after the call to

...

$auto_renew = _CanBookBeAutoRenewed({

I don't believe I can. That is inside "unless ($override_limit)" so it would make 'too_soon' subject to override

I was wrong, done -- 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=29474 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- 5 renewals allowed No renewal before unset (i.e. not before due) For a single biblio 100 items on record 10 items checked out and marked auto_renew 10 record level holds Without these patches: ~3.7 second With these patches ~1.5 second Then I set 'On shelf holds allowed' to 'If all unavailable' Without these patches: ~3 MINUTES! With these patches ~1.5 seconds To further test 'ItemsAnyAvailable' I set all the items as overdue, changed nothing else Without these patches: ~3 minutes With these patches ~1.25 minutes -- 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=29474 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #10 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- The last patch is wrong, it checks the availability for the patron who is wanting to do the renewal and not the other patrons as it should (notice that the $patron variable is redefined here). You could however achieve the performance boost by adding the $items_any_available value to a hash table where the keys are borrowernumbers. However, I think we might be able to avoid that (it makes the code pretty ugly imho if we use the hash table) by splitting somehow the while loops and maybe ordering it so that we iterate patrons in the outer loop. Like I think "next if IsItemOnHoldAndFound( $item->itemnumber );" could be moved to its own loop. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #12 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- In the first patch you added the comment: # override_limit will override anything else except on_reserve or too_soon This is actually not what the code does, but it should do this I think, it used to do this but after the patch "Bug 19014: Return auto_too_soon before on_reserve" (51f5f6) the behaviour was changed. It was unadvertised change so it seems like a regression introduced by bug 19014. So actually the too_soon return should be outside the $override_limit if block as you had it the first time but before that we need to fix the regression from bug 19014. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127612|0 |1 is obsolete| | Attachment #127613|0 |1 is obsolete| | --- Comment #14 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127614&action=edit Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - 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=29474 --- Comment #16 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127616 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127616&action=edit Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove 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=29474 --- Comment #18 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- I fixed the incorrect comment about too_soon not being overriden and added couple improvements to the code as well. I obsoleted also the patch from comment #8 now as it was buggy (see comment #10). I could try to work on those perf improvements as what was tried in the patch attached in comment #8 but I'm not sure I will manage to do that in the nearby days so please go ahead and have a new attempt at it as if I will it might take time. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127614|0 |1 is obsolete| | Attachment #127615|0 |1 is obsolete| | Attachment #127616|0 |1 is obsolete| | Attachment #127617|0 |1 is obsolete| | --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127632&action=edit Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - 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=29474 --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127634 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127634&action=edit Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove 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=29474 --- Comment #23 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Joonas Kylmälä from comment #18)

I fixed the incorrect comment about too_soon not being overriden and added couple improvements to the code as well.

Excellent! Thanks! I squashed the comment removal, I thought I had pulled that, I was confused when I added it :-)

I obsoleted also the patch from comment #8 now as it was buggy (see comment #10). I could try to work on those perf improvements as what was tried in the patch attached in comment #8 but I'm not sure I will manage to do that in the nearby days so please go ahead and have a new attempt at it as if I will it might take time.

I am going to move that to it's own bug since it will take a little more reworking. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 127658 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127658&action=edit Bug 29474: Small efficiency improvement -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29537 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29537 [Bug 29537] Simplify auto-renewal code in CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127632|0 |1 is obsolete| | Attachment #127633|0 |1 is obsolete| | Attachment #127634|0 |1 is obsolete| | Attachment #127635|0 |1 is obsolete| | Attachment #127658|0 |1 is obsolete| | --- Comment #27 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127903 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127903&action=edit Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #29 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127905 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127905&action=edit Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove 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=29474 --- Comment #31 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127907 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127907&action=edit Bug 29474: Small efficiency improvement Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Nick Clemens <nick@bywatersolutions.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=29474 --- Comment #34 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127950&action=edit Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #36 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127952 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127952&action=edit Bug 29474: Pass rules/branchcode to subroutines, further simplify logic If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once Additionally, we can fetch all needed rules at once and pass to subroutines as needed Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #38 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Nick, I was wondering this morning (bug 29474 comment 9) if _CanBookBeAutoRenewed shouldn't actually call GetSoonestRenewDate. There is also bug 29537 that will conflict with this. Could we have a clean tree we could test and QA together? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #40 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #38)

Nick, I was wondering this morning (bug 29474 comment 9) if _CanBookBeAutoRenewed shouldn't actually call GetSoonestRenewDate.

Can you elaborate? We pass the soonest renew date now, rather than calling twice, why do we need to call directly? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #42 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I agree with most of the changes, but I don't think it's a good idea to pass the issuing rules and the "soonest date". It's just hiding problem with the code, which shouldn't be resolved that way. For instance if we are facing perf issues with the fetch of the circ rules, we should then cache them. Other areas would benefit from that kind of change. Code will stay clean and we wouldn't need write acrobatic code in this C4::Circ module, it does not need that :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #44 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #42)

I agree with most of the changes, but I don't think it's a good idea to pass the issuing rules and the "soonest date". It's just hiding problem with the code, which shouldn't be resolved that way.

For instance if we are facing perf issues with the fetch of the circ rules, we should then cache them. Other areas would benefit from that kind of change. Code will stay clean and we wouldn't need write acrobatic code in this C4::Circ module, it does not need that :)

I see your point, however, in this instance _CanBookBeAutoRenewed is just a helper function here, moved out to make the logic easier to read. Passing variables we are fetching that are common to both routines seems logical to me. It's not so much that there is a perf problem fetching them, but that we are fetching the same data here multiple times (In reply to Jonathan Druart from comment #43)

And... thinking loud, sorry for the noise if it does not make sense.

There is a _CanBookBeAutoRenewed sub but we are still doing "auto renewal" related checks in CanBookBeRenewed. It should be simpler than that actually, we should call _CanBookBeAutoRenewed that will call CanBookBeRenewed, not the reverse.

Ideally Koha::AutoRenewalHold inherit from Koha::Hold Koha::AutoRenewalHold->can_be_renewed will do its specific check for auto renew then can SUPER::can_be_renewed

I assume you mean 'Issue' not 'Hold'

Aren't we actually doing the reverse here?

Automatic renewals and renewals are not discrete - a regular renewal block will prevent auto_renewal and is cheaper to check the simple bits first - it makes sense to go through the simple renewal checks that will block, before checking the auto_renewal blocks. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29623 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127952|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #47 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #45)

(In reply to Jonathan Druart from comment #42)

...

For instance if we are facing perf issues with the fetch of the circ rules, we should then cache them. Other areas would benefit from that kind of change. Code will stay clean and we wouldn't need write acrobatic code in this C4::Circ module, it does not need that :)

I've attached a patch on bug 29623, for discussion.

I tested that similar to here, I did not se any significant performance change. I then tested here without the last two patches to see how much passing rules and dates save us. It turns out it make the routine take longer. I am not sure how/why, but it seems fetching the values was faster than passing them. I obsoleted the last two patches, as those are the ones you didn't like hopefully we can move this forward ;-) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127953|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127953|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127949|0 |1 is obsolete| | Attachment #127950|0 |1 is obsolete| | Attachment #127951|0 |1 is obsolete| | --- Comment #48 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128554 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128554&action=edit Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #50 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128556 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128556&action=edit Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 --- Comment #51 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.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=29474 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED CC| |andrew@bywatersolutions.com --- Comment #53 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Missing dependency (bug 27032), not backported to 21.05. Please request if needed. -- You are receiving this mail because: You are watching all bug changes.