Bug 29102 - DecreaseLoanHighHolds will decrease loan period if patron has an 'unfound' hold
Summary: DecreaseLoanHighHolds will decrease loan period if patron has an 'unfound' hold
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-23 20:39 UTC by Nick Clemens
Modified: 2023-06-08 22:29 UTC (History)
6 users (show)

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


Attachments
Bug 29102: Unit tests (3.12 KB, patch)
2021-12-23 17:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Do not count patron's own hold against limits (5.37 KB, patch)
2021-12-23 17:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Remove ignore_found_holds (2.55 KB, patch)
2021-12-23 17:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Do not count patron's own hold against limits (7.05 KB, patch)
2022-07-14 14:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Remove ignore_found_holds (2.67 KB, patch)
2022-07-14 14:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Do not count patron's own hold against limits (8.45 KB, patch)
2022-07-15 10:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Remove ignore_found_holds (2.67 KB, patch)
2022-07-15 10:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29102: Do not count patron's own hold against limits (8.50 KB, patch)
2022-09-25 05:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 29102: Remove ignore_found_holds (2.72 KB, patch)
2022-09-25 05:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 29102: Do not count patron's own hold against limits (8.56 KB, patch)
2022-11-02 16:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29102: Remove ignore_found_holds (2.78 KB, patch)
2022-11-02 16:30 UTC, Martin Renvoize
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-09-23 20:39:12 UTC
Similar to bug 25566

If a patron has a hold on a record and there are other holds
 -the hold is not found
 -the rules limit to 1 hold per record
 -no items will be holdable (already have the hone hold)
 -Loan will be decreased if using decreaseLoanHighHoldsControl  - 'over the number of holdable items'/dynamic
Comment 1 Nick Clemens 2021-12-23 17:18:33 UTC
Created attachment 128901 [details] [review]
Bug 29102: Unit tests
Comment 2 Nick Clemens 2021-12-23 17:18:36 UTC
Created attachment 128902 [details] [review]
Bug 29102: Do not count patron's own hold against limits

This patch makes two changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved

Previously a patron's hold could put the count over the threshhold, and
if the patron is only allowed 1 hold per record, and the hold wasn't found before
the checkout, it would make all items unholdable, thus lowering the theshhold for
dynamic HighHolds

To test:
1 - Set sysaprefs:
     decreaseLoanHighHolds  - enable
     decreaseLoanHighHoldsDuration - 1
     decreaseLoanHighHoldsValue - 1
     decreaseLoanHighHoldsControl - dynamic
     decreaseLoanHighHoldsIgnoreStatuses - blank
2 - Set circ rules to allow 1 hold per record and loan period of 5
3 - Find/create a record with 3 items
4 - Place a title level hold for a patron
5 - Attempt to checkout item - note warning about high holds
6 - Cancel checkout
7 - Set decreaseLoanHighHoldsControl - static
8 - Attempt checkout - note warning about high holds
9 - Apply patrch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - dynamic
13 - Checkout item - no warning
Comment 3 Nick Clemens 2021-12-23 17:18:40 UTC
Created attachment 128903 [details] [review]
Bug 29102: Remove ignore_found_holds

If not counting patrons holds, found or unfound, we no longer need this option
introduced by bug 28078
Comment 4 Christopher Brannon 2021-12-28 23:12:22 UTC
I'm using a sandbox, and I haven't applied the patch yet and I am not running into the problem in step 5.  It just checks out the item and removes the hold.
Comment 5 Nick Clemens 2022-01-03 14:18:51 UTC
(In reply to Christopher Brannon from comment #4)
> I'm using a sandbox, and I haven't applied the patch yet and I am not
> running into the problem in step 5.  It just checks out the item and removes
> the hold.

Ah, I described it in initial comment "and there are other holds"

When set to dynamic we check:
$threshold = $items_count + $decreaseLoanHighHoldsValue;
$holds->count() <= $threshold

items_count will be 0 (none holdable) but holds count is still 1, so not over the threshold

Amended steps below:
step 4 - place title level holds for 2 patrons
step 5 - attempt to checkout to one of the patrons who have a hold
Comment 6 Emmanuel Bétemps 2022-02-16 09:25:30 UTC
For the syspref 'decreaseLoanHighHoldsControl', I do not see nor 'static' nor 'dynamic'.

I can choose between 'on the record' and 'over the number of holdable items on the record'.

-> Which of these two is dynamic ?
I assumed it was 'over the number...' but I'd like to be sure.
Comment 7 Christopher Brannon 2022-02-16 15:51:56 UTC
(In reply to Emmanuel Bétemps from comment #6)
> For the syspref 'decreaseLoanHighHoldsControl', I do not see nor 'static'
> nor 'dynamic'.
> 
> I can choose between 'on the record' and 'over the number of holdable items
> on the record'.
> 
> -> Which of these two is dynamic ?
> I assumed it was 'over the number...' but I'd like to be sure.

I believe 'on the record' is static, and 'over the number of holdable items on the record' is dynamic.
Comment 8 David Nind 2022-07-09 22:05:00 UTC
Patch no longer applies 8-(..

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 29102: Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/DecreaseLoanHighHolds.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/DecreaseLoanHighHolds.t
Applying: Bug 29102: Do not count patron's own hold against limits
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	t/db_dependent/DecreaseLoanHighHolds.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/DecreaseLoanHighHolds.t
CONFLICT (content): Merge conflict in t/db_dependent/DecreaseLoanHighHolds.t
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29102: Do not count patron's own hold against limits
Comment 9 Nick Clemens 2022-07-14 14:26:44 UTC
Created attachment 137722 [details] [review]
Bug 29102: Do not count patron's own hold against limits

This patch makes two changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved

Previously a patron's hold could put the count over the threshhold, and
if the patron is only allowed 1 hold per record, and the hold wasn't found before
the checkout, it would make all items unholdable, thus lowering the theshhold for
dynamic HighHolds

To test:
1 - Set sysaprefs:
     decreaseLoanHighHolds  - enable
     decreaseLoanHighHoldsDuration - 1
     decreaseLoanHighHoldsValue - 1
     decreaseLoanHighHoldsControl - dynamic
     decreaseLoanHighHoldsIgnoreStatuses - blank
2 - Set circ rules to allow 1 hold per record and loan period of 5
3 - Find/create a record with 3 items
4 - Place a title level hold for a patron
5 - Attempt to checkout item - note warning about high holds
6 - Cancel checkout
7 - Set decreaseLoanHighHoldsControl - static
8 - Attempt checkout - note warning about high holds
9 - Apply patrch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - dynamic
13 - Checkout item - no warning
14 - prove t/db_dependent/DecreaseLoanHighHolds.t
Comment 10 Nick Clemens 2022-07-14 14:26:47 UTC
Created attachment 137723 [details] [review]
Bug 29102: Remove ignore_found_holds

If not counting patrons holds, found or unfound, we no longer need this option
introduced by bug 28078
Comment 11 Nick Clemens 2022-07-15 10:58:38 UTC
Created attachment 137745 [details] [review]
Bug 29102: Do not count patron's own hold against limits

This patch makes three changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved
3 - Static mode should only decrease hold when over the decreaseLoanHighHoldsValue, not when equal

Previously a patron's hold could put the count over the threshhold, and
if the patron is only allowed 1 hold per record, and the hold wasn't found before
the checkout, it would make all items unholdable, thus lowering the theshhold for
dynamic HighHolds

To test:
 1 - Set sysaprefs:
     decreaseLoanHighHolds  - enable
     decreaseLoanHighHoldsDuration - 1
     decreaseLoanHighHoldsValue - 1
     decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
     decreaseLoanHighHoldsIgnoreStatuses - blank
 2 - Set circ rules to allow 1 hold per record and loan period of 5
 3 - Find/create a record with 3 items
 4 - Place a title level hold for two different patrons
 5 - Attempt to checkout item - note warning about high holds
 6 - Cancel checkout
 7 - Set decreaseLoanHighHoldsControl - "on the record" / static
 8 - Attempt checkout - note warning about high holds
 9 - Apply patch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
13 - Checkout item - no warning
14 - prove t/db_dependent/DecreaseLoanHighHolds.t
Comment 12 Nick Clemens 2022-07-15 10:58:42 UTC
Created attachment 137746 [details] [review]
Bug 29102: Remove ignore_found_holds

If not counting patrons holds, found or unfound, we no longer need this option
introduced by bug 28078
Comment 13 David Nind 2022-09-25 05:40:17 UTC
Created attachment 140970 [details] [review]
Bug 29102: Do not count patron's own hold against limits

This patch makes three changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved
3 - Static mode should only decrease hold when over the decreaseLoanHighHoldsValue, not when equal

Previously a patron's hold could put the count over the threshhold, and
if the patron is only allowed 1 hold per record, and the hold wasn't found before
the checkout, it would make all items unholdable, thus lowering the theshhold for
dynamic HighHolds

To test:
 1 - Set sysaprefs:
     decreaseLoanHighHolds  - enable
     decreaseLoanHighHoldsDuration - 1
     decreaseLoanHighHoldsValue - 1
     decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
     decreaseLoanHighHoldsIgnoreStatuses - blank
 2 - Set circ rules to allow 1 hold per record and loan period of 5
 3 - Find/create a record with 3 items
 4 - Place a title level hold for two different patrons
 5 - Attempt to checkout item - note warning about high holds
 6 - Cancel checkout
 7 - Set decreaseLoanHighHoldsControl - "on the record" / static
 8 - Attempt checkout - note warning about high holds
 9 - Apply patch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
13 - Checkout item - no warning
14 - prove t/db_dependent/DecreaseLoanHighHolds.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2022-09-25 05:40:23 UTC
Created attachment 140971 [details] [review]
Bug 29102: Remove ignore_found_holds

If not counting patrons holds, found or unfound, we no longer need this option
introduced by bug 28078

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Martin Renvoize 2022-11-02 16:30:37 UTC
Created attachment 142990 [details] [review]
Bug 29102: Do not count patron's own hold against limits

This patch makes three changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved
3 - Static mode should only decrease hold when over the decreaseLoanHighHoldsValue, not when equal

Previously a patron's hold could put the count over the threshhold, and
if the patron is only allowed 1 hold per record, and the hold wasn't found before
the checkout, it would make all items unholdable, thus lowering the theshhold for
dynamic HighHolds

To test:
 1 - Set sysaprefs:
     decreaseLoanHighHolds  - enable
     decreaseLoanHighHoldsDuration - 1
     decreaseLoanHighHoldsValue - 1
     decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
     decreaseLoanHighHoldsIgnoreStatuses - blank
 2 - Set circ rules to allow 1 hold per record and loan period of 5
 3 - Find/create a record with 3 items
 4 - Place a title level hold for two different patrons
 5 - Attempt to checkout item - note warning about high holds
 6 - Cancel checkout
 7 - Set decreaseLoanHighHoldsControl - "on the record" / static
 8 - Attempt checkout - note warning about high holds
 9 - Apply patch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
13 - Checkout item - no warning
14 - prove t/db_dependent/DecreaseLoanHighHolds.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-11-02 16:30:44 UTC
Created attachment 142991 [details] [review]
Bug 29102: Remove ignore_found_holds

If not counting patrons holds, found or unfound, we no longer need this option
introduced by bug 28078

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-11-02 16:31:53 UTC
Works as described, nice clean code., unit tests and QA script is happy.

Passing QA
Comment 18 Tomás Cohen Arazi 2022-11-04 22:25:28 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!