|
Description
Nick Clemens (kidclamp)
2021-09-23 20:39:12 UTC
Created attachment 128901 [details] [review] Bug 29102: Unit tests 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 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 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. (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 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. (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. 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 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 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 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 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 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> 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> 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> 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> Works as described, nice clean code., unit tests and QA script is happy. Passing QA Pushed to master for 22.11. Nice work everyone, thanks! |