Bug 28013 - Improvements to CanBookBeRenewed
Summary: Improvements to CanBookBeRenewed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
: 26659 (view as bug list)
Depends on: 28078
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-22 13:05 UTC by Nick Clemens
Modified: 2021-12-13 21:10 UTC (History)
10 users (show)

See Also:
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


Attachments
Bug 28013: Performance improvements to CanBookBeRenewed (4.57 KB, patch)
2021-03-22 13:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28013: Performance improvements to CanBookBeRenewed (4.62 KB, patch)
2021-03-29 03:40 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 28013: Performance improvements to CanBookBeRenewed (4.77 KB, patch)
2021-03-29 11:49 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: (QA follow-up) Remove unused variable (1.04 KB, patch)
2021-03-29 11:49 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: (follow-up) Correct and update call to CanItemBeReserved (1.18 KB, patch)
2021-04-02 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28013: Unit tests (4.18 KB, patch)
2021-04-02 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28013: (follow-up) Reset the patron loop (750 bytes, patch)
2021-04-02 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28013: Performance improvements to CanBookBeRenewed (4.77 KB, patch)
2021-04-07 08:30 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: Performance improvements to CanBookBeRenewed (4.77 KB, patch)
2021-04-07 08:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: (QA follow-up) Remove unused variable (1.04 KB, patch)
2021-04-07 08:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: (follow-up) Correct and update call to CanItemBeReserved (1.33 KB, patch)
2021-04-07 08:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: Unit tests (4.32 KB, patch)
2021-04-07 08:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28013: (follow-up) Reset the patron loop (902 bytes, patch)
2021-04-07 08:47 UTC, Joonas Kylmälä
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-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 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 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 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 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 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 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.