Bug 29474 - Automatic renewals cronjob is slow on systems with large numbers of reserves
Summary: Automatic renewals cronjob is slow on systems with large numbers of reserves
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
: 22333 (view as bug list)
Depends on:
Blocks: 29483 29537
  Show dependency treegraph
 
Reported: 2021-11-13 14:42 UTC by Nick Clemens
Modified: 2022-12-12 21:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03


Attachments
Bug 29474: Return too_soon before checking renewals (1.87 KB, patch)
2021-11-13 15:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.49 KB, patch)
2021-11-13 18:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.53 KB, patch)
2021-11-13 19:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Check ItemsAnyAvailableAndNOtRestricted outside of loop (2.07 KB, patch)
2021-11-13 20:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.53 KB, patch)
2021-11-14 12:48 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Fix incorrect comment (957 bytes, patch)
2021-11-14 12:48 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.45 KB, patch)
2021-11-14 12:49 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.72 KB, patch)
2021-11-14 12:49 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.10 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.45 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.72 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic (5.65 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Small efficiency improvement (1.06 KB, patch)
2021-11-15 15:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.25 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.45 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.72 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic (5.79 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Small efficiency improvement (1.19 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.25 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.50 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.78 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic (5.79 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Small efficiency improvement (1.19 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.32 KB, patch)
2021-12-15 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.58 KB, patch)
2021-12-15 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.85 KB, patch)
2021-12-15 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-11-13 14:42:47 UTC
When a site has many holds the automatic renewals cron can take a long time to run

This is increased if the system preference 'AllowRenewalIfOtherItemsAvailable' is set to allow
Comment 1 Nick Clemens 2021-11-13 15:29:31 UTC
Created attachment 127610 [details] [review]
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
Comment 2 Joonas Kylmälä 2021-11-13 16:06:37 UTC
If you remove the unless then you can delete the line

> return ( 0, "on_reserve" ) if $resfound && $cron;    # '' when no hold was found

Makes the code much simpler.

Please also move the cron return right after the call to 

> $auto_renew = _CanBookBeAutoRenewed({

so that now that the return value decision is not in an if-else condition at the end the requirement for returning the values in a different order is clear and the order won't be changed by some other developer accidentally. I think adding a comment like "cron wants 'too_soon' over 'on_reserve'"

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
Comment 3 Joonas Kylmälä 2021-11-13 16:12:04 UTC
(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.
Comment 4 Nick Clemens 2021-11-13 18:50:50 UTC
Created attachment 127611 [details] [review]
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
Comment 5 Nick Clemens 2021-11-13 18:53:23 UTC
(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!
Comment 6 Nick Clemens 2021-11-13 19:23:23 UTC
Created attachment 127612 [details] [review]
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
Comment 7 Nick Clemens 2021-11-13 19:23:59 UTC
(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
Comment 8 Nick Clemens 2021-11-13 20:01:38 UTC
Created attachment 127613 [details] [review]
Bug 29474: Check ItemsAnyAvailableAndNOtRestricted outside of loop

IsAvailableForItemLevelRequest takes $items_any_available to avoid
a double loop - this was done in request.pl, but it makes sense
to do the same here
Comment 9 Nick Clemens 2021-11-13 20:51:45 UTC
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
Comment 10 Joonas Kylmälä 2021-11-14 07:55:17 UTC
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.
Comment 11 Joonas Kylmälä 2021-11-14 08:26:38 UTC
oh, I think I discovered a bug in the CanBookBeRenewed function. We are calling

> next unless IsAvailableForItemLevelRequest($item, $patron); 

and we are not passing the ignore_hold_counts=>1 here even though we should be. The patch "Bug 28013: (follow-up) Correct and update call to CanItemBeReserved" which is pushed already fixed it for the CanItemBeReserved() function call just the line below but this has the same bug.

If you follow this code path you will notice the bug:

IsAvailableForItemLevelRequest -> ItemsAnyAvailableAndNotRestricted -> CanItemBeReserved

So this needs to kept also in mind while doing the perf work here.
Comment 12 Joonas Kylmälä 2021-11-14 10:45:49 UTC
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.
Comment 13 Joonas Kylmälä 2021-11-14 10:53:29 UTC
(In reply to Joonas Kylmälä from comment #12)
> 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.

Hmm, actually there is no regression, there was the $override_limit check also before bug 19014. So the comment you added should be removed. Sorry for the noise, the code is just so hard to understand...
Comment 14 Joonas Kylmälä 2021-11-14 12:48:54 UTC
Created attachment 127614 [details] [review]
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
Comment 15 Joonas Kylmälä 2021-11-14 12:48:57 UTC
Created attachment 127615 [details] [review]
Bug 29474: Fix incorrect comment

The too_soon is actually being overriden if $override_limit is true.
Comment 16 Joonas Kylmälä 2021-11-14 12:49:01 UTC
Created attachment 127616 [details] [review]
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
Comment 17 Joonas Kylmälä 2021-11-14 12:49:04 UTC
Created attachment 127617 [details] [review]
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
Comment 18 Joonas Kylmälä 2021-11-14 13:01:17 UTC
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.
Comment 19 Nick Clemens 2021-11-15 12:14:27 UTC
Created attachment 127632 [details] [review]
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
Comment 20 Nick Clemens 2021-11-15 12:14:30 UTC
Created attachment 127633 [details] [review]
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
Comment 21 Nick Clemens 2021-11-15 12:14:33 UTC
Created attachment 127634 [details] [review]
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
Comment 22 Nick Clemens 2021-11-15 12:14:37 UTC
Created attachment 127635 [details] [review]
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
Comment 23 Nick Clemens 2021-11-15 12:35:30 UTC
(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.
Comment 24 Kyle M Hall 2021-11-15 15:53:09 UTC
Created attachment 127658 [details] [review]
Bug 29474: Small efficiency improvement
Comment 25 Joonas Kylmälä 2021-11-20 11:40:26 UTC
The renewal date calculation is buggy for auto-renewals, I made a fix for that a few days ago in bug 29476. I will mark this bug to now depend on that bug as "Bug 29474: Pass rules/branchcode to subroutines, further simplify logic" uses now GetSoonestRenewDate also for auto-renewals as well, thus requires the bug fix. Also, now that we depend on bug 29476 I can make at least one further simplification in the code, will attach a patch shortly.
Comment 26 Joonas Kylmälä 2021-11-20 12:07:50 UTC
(In reply to Joonas Kylmälä from comment #25)
> The renewal date calculation is buggy for auto-renewals, I made a fix for
> that a few days ago in bug 29476. I will mark this bug to now depend on that
> bug as "Bug 29474: Pass rules/branchcode to subroutines, further simplify
> logic" uses now GetSoonestRenewDate also for auto-renewals as well, thus
> requires the bug fix. Also, now that we depend on bug 29476 I can make at
> least one further simplification in the code, will attach a patch shortly.

After further verification, there is no dependency so I created a completely new bug report for that: bug 29537. It should help *tiny* bit with performance as well so you might consider signing-off that.
Comment 27 Joonas Kylmälä 2021-11-20 12:14:08 UTC
Created attachment 127903 [details] [review]
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>
Comment 28 Joonas Kylmälä 2021-11-20 12:14:12 UTC
Created attachment 127904 [details] [review]
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
Comment 29 Joonas Kylmälä 2021-11-20 12:14:16 UTC
Created attachment 127905 [details] [review]
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
Comment 30 Joonas Kylmälä 2021-11-20 12:14:19 UTC
Created attachment 127906 [details] [review]
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>
Comment 31 Joonas Kylmälä 2021-11-20 12:14:23 UTC
Created attachment 127907 [details] [review]
Bug 29474: Small efficiency improvement

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 32 Joonas Kylmälä 2021-11-20 12:15:31 UTC
Added sign-off to all the patches that were not from me, Nick or Kyle, you could sign-off the patches from me to get this to signed-off state :)
Comment 33 Nick Clemens 2021-11-23 12:00:08 UTC
Created attachment 127949 [details] [review]
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>
Comment 34 Nick Clemens 2021-11-23 12:00:12 UTC
Created attachment 127950 [details] [review]
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>
Comment 35 Nick Clemens 2021-11-23 12:00:15 UTC
Created attachment 127951 [details] [review]
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>
Comment 36 Nick Clemens 2021-11-23 12:00:18 UTC
Created attachment 127952 [details] [review]
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>
Comment 37 Nick Clemens 2021-11-23 12:00:22 UTC
Created attachment 127953 [details] [review]
Bug 29474: Small efficiency improvement

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 38 Jonathan Druart 2021-12-02 13:57:02 UTC
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?
Comment 39 Jonathan Druart 2021-12-02 13:57:46 UTC
Forget the second part, it's in the dependency tree already!
Comment 40 Nick Clemens 2021-12-02 14:57:46 UTC
(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?
Comment 41 Jonathan Druart 2021-12-02 15:18:34 UTC
(In reply to Nick Clemens from comment #40)
> (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?

Sorry, I meant bug 29476 comment 9.
And it's actually what Joonas did here in the second patch.
Comment 42 Jonathan Druart 2021-12-02 15:26:08 UTC
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 :)
Comment 43 Jonathan Druart 2021-12-02 15:33:56 UTC
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

Aren't we actually doing the reverse here?
Comment 44 Nick Clemens 2021-12-02 15:52:03 UTC
(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.
Comment 45 Jonathan Druart 2021-12-02 16:32:28 UTC
(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.
Comment 46 Jonathan Druart 2021-12-02 16:43:04 UTC
(In reply to Nick Clemens from comment #44)
> (In reply to Jonathan Druart from comment #42)
> > 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'

Of course! I actually replaced "Issue" with "Hold" instead of "Checkout", erk!
Comment 47 Nick Clemens 2021-12-13 18:25:40 UTC
(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 ;-)
Comment 48 Jonathan Druart 2021-12-15 10:36:34 UTC
Created attachment 128554 [details] [review]
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>
Comment 49 Jonathan Druart 2021-12-15 10:36:38 UTC
Created attachment 128555 [details] [review]
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>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 50 Jonathan Druart 2021-12-15 10:36:43 UTC
Created attachment 128556 [details] [review]
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>
Comment 51 Fridolin Somers 2022-01-19 07:20:51 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 52 Kyle M Hall 2022-02-04 19:17:53 UTC
Pushed to 21.11.x for 21.11.03
Comment 53 Andrew Fuerste-Henry 2022-02-11 17:15:04 UTC
Missing dependency (bug 27032), not backported to 21.05. Please request if needed.
Comment 54 Nick Clemens 2022-04-20 12:29:47 UTC
*** Bug 22333 has been marked as a duplicate of this bug. ***