Bug 28013

Summary: Improvements to CanBookBeRenewed
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: CirculationAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: normal    
Priority: P5 - low CC: amitddng135, andrew, fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, kyle.m.hall, nugged, stalkernoid, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12
Circulation function:
Bug Depends on: 28078    
Bug Blocks:    
Attachments: Bug 28013: Performance improvements to CanBookBeRenewed
Bug 28013: Performance improvements to CanBookBeRenewed
Bug 28013: Performance improvements to CanBookBeRenewed
Bug 28013: (QA follow-up) Remove unused variable
Bug 28013: (follow-up) Correct and update call to CanItemBeReserved
Bug 28013: Unit tests
Bug 28013: (follow-up) Reset the patron loop
Bug 28013: Performance improvements to CanBookBeRenewed
Bug 28013: Performance improvements to CanBookBeRenewed
Bug 28013: (QA follow-up) Remove unused variable
Bug 28013: (follow-up) Correct and update call to CanItemBeReserved
Bug 28013: Unit tests
Bug 28013: (follow-up) Reset the patron loop

Description Nick Clemens (kidclamp) 2021-03-22 13:05:54 UTC
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
Comment 1 Nick Clemens (kidclamp) 2021-03-22 13:08:13 UTC
Created attachment 118592 [details] [review]
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
Comment 2 Amit Gupta 2021-03-29 03:40:14 UTC
Created attachment 118928 [details] [review]
Bug 28013: Performance improvements to CanBookBeRenewed

To test:
1 - Apply patch
2 - prove -v t/db_dependent/Circulation.t
Comment 3 Joonas Kylmälä 2021-03-29 11:49:42 UTC
Created attachment 118950 [details] [review]
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 4 Joonas Kylmälä 2021-03-29 11:49:45 UTC
Created attachment 118951 [details] [review]
Bug 28013: (QA follow-up) Remove unused variable

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 5 Joonas Kylmälä 2021-03-29 11:55:26 UTC
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.
Comment 6 Joonas Kylmälä 2021-03-29 12:18:54 UTC
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.
Comment 7 Jonathan Druart 2021-04-01 16:21:05 UTC
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?
Comment 8 Nick Clemens (kidclamp) 2021-04-02 14:36:37 UTC
Created attachment 119125 [details] [review]
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
Comment 9 Nick Clemens (kidclamp) 2021-04-02 14:36:41 UTC
Created attachment 119126 [details] [review]
Bug 28013: Unit tests

Cover the case of multiple patrons and multiple items that can fill them
Comment 10 Nick Clemens (kidclamp) 2021-04-02 14:36:44 UTC
Created attachment 119127 [details] [review]
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
Comment 11 Nick Clemens (kidclamp) 2021-04-02 15:14:42 UTC
(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.
Comment 12 Joonas Kylmälä 2021-04-06 07:18:32 UTC
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.
Comment 13 Nick Clemens (kidclamp) 2021-04-06 11:38:17 UTC
(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
Comment 14 Joonas Kylmälä 2021-04-06 11:39:57 UTC
(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
Comment 15 Joonas Kylmälä 2021-04-06 12:45:46 UTC
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?
Comment 16 Joonas Kylmälä 2021-04-07 08:07:54 UTC Comment hidden (obsolete)
Comment 17 Joonas Kylmälä 2021-04-07 08:30:24 UTC Comment hidden (obsolete)
Comment 18 Joonas Kylmälä 2021-04-07 08:47:39 UTC
Created attachment 119257 [details] [review]
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 19 Joonas Kylmälä 2021-04-07 08:47:44 UTC
Created attachment 119258 [details] [review]
Bug 28013: (QA follow-up) Remove unused variable

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 20 Joonas Kylmälä 2021-04-07 08:47:48 UTC
Created attachment 119259 [details] [review]
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>
Comment 21 Joonas Kylmälä 2021-04-07 08:47:54 UTC
Created attachment 119260 [details] [review]
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>
Comment 22 Joonas Kylmälä 2021-04-07 08:47:59 UTC
Created attachment 119261 [details] [review]
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>
Comment 23 Joonas Kylmälä 2021-04-07 08:50:38 UTC
Passing QA.
Comment 24 Jonathan Druart 2021-04-07 14:38:36 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 25 Joonas Kylmälä 2021-04-08 06:53:18 UTC
*** Bug 26659 has been marked as a duplicate of this bug. ***
Comment 26 Fridolin Somers 2021-04-29 09:02:51 UTC
Pushed to 20.11.x for 20.11.06
Comment 27 Andrew Fuerste-Henry 2021-05-24 14:36:21 UTC
Pushed to 20.05.x for 20.05.12
Comment 28 Victor Grousset/tuxayo 2021-05-24 15:01:37 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.