Bug 31112 - Able to renew checkout when the number of holds exceeds available number of items
Summary: Able to renew checkout when the number of holds exceeds available number of i...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Joonas Kylmälä
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 31436
  Show dependency treegraph
 
Reported: 2022-07-06 20:28 UTC by Joonas Kylmälä
Modified: 2022-11-14 10:49 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
When AllowRenewalIfOtherItemsAvailable is set to Allow it now correctly takes into account all the holds instead of just one per patron.
Version(s) released in:
22.11.00, 22.05.07


Attachments
Bug 31112: Remove unnecessary if-clause (7.11 KB, patch)
2022-07-18 16:38 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31112: Remove unit test that forces specific internal implementation (2.27 KB, patch)
2022-07-18 16:38 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio (8.45 KB, patch)
2022-07-18 16:39 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31112: Remove unnecessary if-clause (7.18 KB, patch)
2022-07-19 08:52 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 31112: Remove unit test that forces specific internal implementation (2.34 KB, patch)
2022-07-19 08:52 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio (8.51 KB, patch)
2022-07-19 08:53 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 31112: Remove unnecessary if-clause (7.23 KB, patch)
2022-08-19 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio (8.57 KB, patch)
2022-08-19 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31112: (QA follow-up) Restore check to avoid extra processing (957 bytes, patch)
2022-08-19 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31112: (QA follow-up) Fetch patrons with reserves (1.19 KB, patch)
2022-08-19 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31112: (follow-up) Bring back the check for non-priority holds (1.95 KB, patch)
2022-08-22 19:08 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31112: (follow-up) Don't return "on_reserve" when there are 0 possible holds (1.22 KB, patch)
2022-08-22 19:09 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31112: Remove unnecessary if-clause (7.29 KB, patch)
2022-08-24 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio (8.62 KB, patch)
2022-08-24 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31112: (QA follow-up) Restore check to avoid extra processing (1013 bytes, patch)
2022-08-24 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31112: (QA follow-up) Fetch patrons with reserves (1.24 KB, patch)
2022-08-24 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31112: (follow-up) Bring back the check for non-priority holds (2.00 KB, patch)
2022-08-24 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31112: (follow-up) Don't return "on_reserve" when there are 0 possible holds (1.28 KB, patch)
2022-08-24 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31112: (QA follow-up) Reduce database queries (6.33 KB, patch)
2022-08-24 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2022-07-06 20:28:47 UTC
If AllowRenewalIfOtherItemsAvailable is set to Allow we are incorrectly (against the syspref description) allowing to renew some checkouts when the holds are coming from a single patron. When there is only 1 available item for checkout in the biblio and a single patron has made 2 biblio level holds we allow the patron who has a single item on loan to renew their item and disregard the fact the we are leaving one hold unfilled. According to the AllowRenewalIfOtherItemsAvailable syspref the renewal should only be possible when other items can fill all the unfilled holds.

I have made a patch for this and will submit it shortly.
Comment 1 Joonas Kylmälä 2022-07-18 16:38:54 UTC
Created attachment 137826 [details] [review]
Bug 31112: Remove unnecessary if-clause

To test:
 1) Please check manually that the logic stays the same, use git's -w command line parameter to
    ignore whitespace changes in the diff output.
 2) prove t/db_dependent/Circulation.t
Comment 2 Joonas Kylmälä 2022-07-18 16:38:58 UTC
Created attachment 137827 [details] [review]
Bug 31112: Remove unit test that forces specific internal implementation

To test:
 1) Make sure tests still pass:
    $ prove t/db_dependent/Circulation.t
Comment 3 Joonas Kylmälä 2022-07-18 16:39:02 UTC
Created attachment 137828 [details] [review]
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio

If a single patron had more than 1 hold to a biblio and there was only one
available item we allowed incorrectly renewing the checkout when
AllowRenewalIfOtherItemsAvailable was set to "Allow". This
changes CanBookBeRenewed so that it makes sure all the holds are
filled and not just one per patron.

To test:
 1) prove t/db_dependent/Circulation.t
 2) (Optional, as unit test is provided)
    - Set AllowRenewalIfOtherItemsAvailable = Allow
    - Create biblio with three items
    - Checkout one item to patron A
    - Add two biblio-level holds for patron B
    - Try to renew patron A's checkout with and without this patch.
    - Notice that without this patch the renewal succeeds even though we
      one unfilled hold left. After applying the patch the renewal should
      fail.
Comment 4 Biblibre Sandboxes 2022-07-19 08:52:56 UTC
Created attachment 137856 [details] [review]
Bug 31112: Remove unnecessary if-clause

To test:
 1) Please check manually that the logic stays the same, use git's -w command line parameter to
    ignore whitespace changes in the diff output.
 2) prove t/db_dependent/Circulation.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 5 Biblibre Sandboxes 2022-07-19 08:52:59 UTC
Created attachment 137857 [details] [review]
Bug 31112: Remove unit test that forces specific internal implementation

To test:
 1) Make sure tests still pass:
    $ prove t/db_dependent/Circulation.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 6 Biblibre Sandboxes 2022-07-19 08:53:03 UTC
Created attachment 137858 [details] [review]
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio

If a single patron had more than 1 hold to a biblio and there was only one
available item we allowed incorrectly renewing the checkout when
AllowRenewalIfOtherItemsAvailable was set to "Allow". This
changes CanBookBeRenewed so that it makes sure all the holds are
filled and not just one per patron.

To test:
 1) prove t/db_dependent/Circulation.t
 2) (Optional, as unit test is provided)
    - Set AllowRenewalIfOtherItemsAvailable = Allow
    - Create biblio with three items
    - Checkout one item to patron A
    - Add two biblio-level holds for patron B
    - Try to renew patron A's checkout with and without this patch.
    - Notice that without this patch the renewal succeeds even though we
      one unfilled hold left. After applying the patch the renewal should
      fail.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 7 Sally 2022-07-19 08:55:51 UTC
Works as described, signed off.  Thanks for this, good improvement.

There's a small typo in the test plan - either 2.2. should be:  'Create biblio with two items' or 2.3/2.4 should be:  'Checkout out one item to patron A, one item to patron B, and add two biblio-level holds for patron C'.  

To test:
 1) prove t/db_dependent/Circulation.t
 2) (Optional, as unit test is provided)
    - Set AllowRenewalIfOtherItemsAvailable = Allow
    - Create biblio with three items
    - Checkout one item to patron A
    - Add two biblio-level holds for patron B
    - Try to renew patron A's checkout with and without this patch.
    - Notice that without this patch the renewal succeeds even though we
      one unfilled hold left. After applying the patch the renewal should
      fail.
Comment 8 Nick Clemens 2022-08-19 13:43:06 UTC
Created attachment 139492 [details] [review]
Bug 31112: Remove unnecessary if-clause

To test:
 1) Please check manually that the logic stays the same, use git's -w command line parameter to
    ignore whitespace changes in the diff output.
 2) prove t/db_dependent/Circulation.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2022-08-19 13:43:09 UTC
Created attachment 139493 [details] [review]
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio

If a single patron had more than 1 hold to a biblio and there was only one
available item we allowed incorrectly renewing the checkout when
AllowRenewalIfOtherItemsAvailable was set to "Allow". This
changes CanBookBeRenewed so that it makes sure all the holds are
filled and not just one per patron.

To test:
 1) prove t/db_dependent/Circulation.t
 2) (Optional, as unit test is provided)
    - Set AllowRenewalIfOtherItemsAvailable = Allow
    - Create biblio with three items
    - Checkout one item to patron A
    - Add two biblio-level holds for patron B
    - Try to renew patron A's checkout with and without this patch.
    - Notice that without this patch the renewal succeeds even though we
      one unfilled hold left. After applying the patch the renewal should
      fail.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2022-08-19 13:43:13 UTC
Created attachment 139494 [details] [review]
Bug 31112: (QA follow-up) Restore check to avoid extra processing

We now count all holds for all patrons, we can still eject if we have more
holds than we do items

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2022-08-19 13:43:16 UTC
Created attachment 139495 [details] [review]
Bug 31112: (QA follow-up) Fetch patrons with reserves

Previously we fetched all in a single call using biblionumbers

Fetching each individually could be a performance hit on systems
with large numbers of holds

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2022-08-19 13:45:00 UTC
I reinstated the test for early exit - this is still a valid performance boost - we can't fill all holds if we don't have enough items

Also added a prefetch to avoid single calls for each borrower

Kyle is going to followup after me
Comment 13 Joonas Kylmälä 2022-08-19 20:31:39 UTC
Thanks for the review Nick! :)

(In reply to Nick Clemens from comment #12)
> I reinstated the test for early exit - this is still a valid performance
> boost - we can't fill all holds if we don't have enough items

OK, works for me. I didn't know it was for performance reasons and seems to make the logic not work correctly. I could open another bug report to fix this as I feel like we need some better way to make it more performant while still keeping the check correct.
Comment 14 Kyle M Hall 2022-08-22 13:36:02 UTC
prove t/db_dependent/Holds.t
t/db_dependent/Holds.t .. 69/75
    #   Failed test 'Can renew'
    #   at t/db_dependent/Holds.t line 1528.

    #   Failed test 'Item is on non priority hold'
    #   at t/db_dependent/Holds.t line 1529.
    #          got: 'on_reserve'
    #     expected: undef
    # Looks like you failed 2 tests of 6.
t/db_dependent/Holds.t .. 71/75
#   Failed test 'non priority holds'
#   at t/db_dependent/Holds.t line 1556.
t/db_dependent/Holds.t .. 75/75 # Looks like you failed 1 test of 75.
t/db_dependent/Holds.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/75 subtests

Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 256 Tests: 75 Failed: 1)
  Failed test:  71
  Non-zero exit status: 1
Files=1, Tests=75,  7 wallclock secs ( 0.03 usr  0.01 sys +  5.04 cusr  0.68 csys =  5.76 CPU)
Result: FAIL
Comment 15 Joonas Kylmälä 2022-08-22 19:08:56 UTC
Created attachment 139622 [details] [review]
Bug 31112: (follow-up) Bring back the check for non-priority holds

Before the changes from bug 31112 when CheckReserves returned a
non-priority hold we didn't return "on_reserve" status but checked in
addition to that whether there are any priority holds and if there
were, only then we returned the "on_reserve" error.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 16 Joonas Kylmälä 2022-08-22 19:09:00 UTC
Created attachment 139623 [details] [review]
Bug 31112: (follow-up) Don't return "on_reserve" when there are 0 possible holds

It's possible that there could be 0 possible reserves, for example
when the hold has already been filled, thus the check would fail as
the item count can never be less than 0.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 17 Joonas Kylmälä 2022-08-22 19:12:17 UTC
Thanks Kyle for spotting the failing test. I fixed the issue by restoring the previous check that I had accidentally removed (btw, the check looks somewhat buggy actually but that's for another bug report).

There was also a small issue with the follow-up patch from Nick, added a fix for that too.
Comment 18 Kyle M Hall 2022-08-24 17:13:31 UTC
Created attachment 139729 [details] [review]
Bug 31112: Remove unnecessary if-clause

To test:
 1) Please check manually that the logic stays the same, use git's -w command line parameter to
    ignore whitespace changes in the diff output.
 2) prove t/db_dependent/Circulation.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2022-08-24 17:13:58 UTC
Created attachment 139730 [details] [review]
Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio

If a single patron had more than 1 hold to a biblio and there was only one
available item we allowed incorrectly renewing the checkout when
AllowRenewalIfOtherItemsAvailable was set to "Allow". This
changes CanBookBeRenewed so that it makes sure all the holds are
filled and not just one per patron.

To test:
 1) prove t/db_dependent/Circulation.t
 2) (Optional, as unit test is provided)
    - Set AllowRenewalIfOtherItemsAvailable = Allow
    - Create biblio with three items
    - Checkout one item to patron A
    - Add two biblio-level holds for patron B
    - Try to renew patron A's checkout with and without this patch.
    - Notice that without this patch the renewal succeeds even though we
      one unfilled hold left. After applying the patch the renewal should
      fail.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2022-08-24 17:14:02 UTC
Created attachment 139731 [details] [review]
Bug 31112: (QA follow-up) Restore check to avoid extra processing

We now count all holds for all patrons, we can still eject if we have more
holds than we do items

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2022-08-24 17:14:05 UTC
Created attachment 139732 [details] [review]
Bug 31112: (QA follow-up) Fetch patrons with reserves

Previously we fetched all in a single call using biblionumbers

Fetching each individually could be a performance hit on systems
with large numbers of holds

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2022-08-24 17:14:08 UTC
Created attachment 139733 [details] [review]
Bug 31112: (follow-up) Bring back the check for non-priority holds

Before the changes from bug 31112 when CheckReserves returned a
non-priority hold we didn't return "on_reserve" status but checked in
addition to that whether there are any priority holds and if there
were, only then we returned the "on_reserve" error.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2022-08-24 17:14:12 UTC
Created attachment 139734 [details] [review]
Bug 31112: (follow-up) Don't return "on_reserve" when there are 0 possible holds

It's possible that there could be 0 possible reserves, for example
when the hold has already been filled, thus the check would fail as
the item count can never be less than 0.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2022-08-24 17:14:15 UTC
Created attachment 139735 [details] [review]
Bug 31112: (QA follow-up) Reduce database queries

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Tomás Cohen Arazi 2022-08-26 19:05:25 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 26 Lucas Gass 2022-10-31 17:48:01 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 27 Arthur Suzuki 2022-11-14 10:49:09 UTC
This one brings in conflicts because of "UseRecalls" present in 22.05 (and not 21.11.x).
It is not easy to backport, I'm afraid to miss the logic here.
Can I get some help with a backport patch for 21.11? Wont backport unless provided.
Arthur