Bug 32878 - Make it impossible to renew the item if it has active item level hold
Summary: Make it impossible to renew the item if it has active item level hold
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Peter Vashchuk
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 31112
Blocks:
  Show dependency treegraph
 
Reported: 2023-02-03 14:22 UTC by Peter Vashchuk
Modified: 2023-12-28 20:44 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:
23.05.00,22.11.07


Attachments
Bug 32878: Make it impossible to renew the item if it has active item level hold (1.49 KB, patch)
2023-02-03 14:25 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 32878: Make it impossible to renew the item if it has active item level hold (1.55 KB, patch)
2023-02-03 17:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 32878: (QA follow-up) Unit test (1.80 KB, patch)
2023-02-16 16:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32878: Make it impossible to renew the item if it has active item level hold (1.61 KB, patch)
2023-02-16 16:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32878: (QA follow-up) Correct unit test (1.29 KB, patch)
2023-02-16 16:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32878: (QA follow-up) Unit test (1.86 KB, patch)
2023-02-17 11:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32878: (QA follow-up) Unit test (1.86 KB, patch)
2023-02-17 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32878: Make it impossible to renew the item if it has active item level hold (1.66 KB, patch)
2023-02-17 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32878: (QA follow-up) Correct unit test (1.34 KB, patch)
2023-02-17 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32878: (follow-up) Exclude non_priority holds (1004 bytes, patch)
2023-03-15 15:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32878: (follow-up) Exclude non_priority holds (1.04 KB, patch)
2023-03-15 15:56 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2023-02-03 14:22:15 UTC

    
Comment 1 Peter Vashchuk 2023-02-03 14:25:26 UTC
Created attachment 146145 [details] [review]
Bug 32878: Make it impossible to renew the item if it has active item level hold

introduced in:
73c3c5d2f10751c23156372300239d42e5957c66
Bug 31112: (QA follow-up) Reduce database queries

started from:
8ba1a9a5345310c54d9225049d470544b56eeb11
Bug 31112: Remove unnecessary if-clause

Currently, you can renew the item even if someone already made an item level
hold on that item. This patch changes that, making it not possible to do so.

To reproduce:
1. Checkout an item, and make another item level hold on that specific item.
2. Renew it using the "Renew" checkbox, it should get renewed without any problems.
3. Apply the patch.
4. Checkbox should be gone and replaced with "On Hold" link that leads to the hold that doesn't allow you to renew the item again.
5. "Renew all" button should not work either.
Comment 2 Andrew Fuerste-Henry 2023-02-03 17:47:42 UTC
Fleshing this out a little:

To reproduce:
1: have AllowRenewalIfOtherItemsAvailable set to Allow
2: have a bib with multiple items
3: 


3: Checkout an item, and make another item level hold on that specific item.
4: Renew it using the "Renew" checkbox, it should get renewed without any problems.
5: Apply the patch.
6: Checkbox should be gone and replaced with "On Hold" link that leads to the hold that doesn't allow you to renew the item again.
7: "Renew all" button should not work either.
Comment 3 Andrew Fuerste-Henry 2023-02-03 17:51:23 UTC
Created attachment 146166 [details] [review]
Bug 32878: Make it impossible to renew the item if it has active item level hold

introduced in:
73c3c5d2f10751c23156372300239d42e5957c66
Bug 31112: (QA follow-up) Reduce database queries

started from:
8ba1a9a5345310c54d9225049d470544b56eeb11
Bug 31112: Remove unnecessary if-clause

Currently, you can renew the item even if someone already made an item level
hold on that item. This patch changes that, making it not possible to do so.

To reproduce:
1. Checkout an item, and make another item level hold on that specific item.
2. Renew it using the "Renew" checkbox, it should get renewed without any problems.
3. Apply the patch.
4. Checkbox should be gone and replaced with "On Hold" link that leads to the hold that doesn't allow you to renew the item again.
5. "Renew all" button should not work either.

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 4 Nick Clemens (kidclamp) 2023-02-16 16:13:33 UTC
Created attachment 146740 [details] [review]
Bug 32878: (QA follow-up) Unit test
Comment 5 Nick Clemens (kidclamp) 2023-02-16 16:13:35 UTC
Created attachment 146741 [details] [review]
Bug 32878: Make it impossible to renew the item if it has active item level hold

introduced in:
73c3c5d2f10751c23156372300239d42e5957c66
Bug 31112: (QA follow-up) Reduce database queries

started from:
8ba1a9a5345310c54d9225049d470544b56eeb11
Bug 31112: Remove unnecessary if-clause

Currently, you can renew the item even if someone already made an item level
hold on that item. This patch changes that, making it not possible to do so.

To reproduce:
1. Checkout an item, and make another item level hold on that specific item.
2. Renew it using the "Renew" checkbox, it should get renewed without any problems.
3. Apply the patch.
4. Checkbox should be gone and replaced with "On Hold" link that leads to the hold that doesn't allow you to renew the item again.
5. "Renew all" button should not work either.

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2023-02-16 16:13:38 UTC
Created attachment 146742 [details] [review]
Bug 32878: (QA follow-up) Correct unit test

After the patch we fail on renewing a not for loan item with an item
level hold.

I don't find justification for why this should be renewable, so I just
change the test expectation

This needs another QA eye

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Kyle M Hall 2023-02-17 11:47:08 UTC
Created attachment 146830 [details] [review]
Bug 32878: (QA follow-up) Unit test

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2023-02-17 12:22:01 UTC
Created attachment 146833 [details] [review]
Bug 32878: (QA follow-up) Unit test

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2023-02-17 12:22:08 UTC
Created attachment 146834 [details] [review]
Bug 32878: Make it impossible to renew the item if it has active item level hold

introduced in:
73c3c5d2f10751c23156372300239d42e5957c66
Bug 31112: (QA follow-up) Reduce database queries

started from:
8ba1a9a5345310c54d9225049d470544b56eeb11
Bug 31112: Remove unnecessary if-clause

Currently, you can renew the item even if someone already made an item level
hold on that item. This patch changes that, making it not possible to do so.

To reproduce:
1. Checkout an item, and make another item level hold on that specific item.
2. Renew it using the "Renew" checkbox, it should get renewed without any problems.
3. Apply the patch.
4. Checkbox should be gone and replaced with "On Hold" link that leads to the hold that doesn't allow you to renew the item again.
5. "Renew all" button should not work either.

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2023-02-17 12:22:10 UTC
Created attachment 146835 [details] [review]
Bug 32878: (QA follow-up) Correct unit test

After the patch we fail on renewing a not for loan item with an item
level hold.

I don't find justification for why this should be renewable, so I just
change the test expectation

This needs another QA eye

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Tomás Cohen Arazi 2023-03-14 13:56:12 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Martin Renvoize 2023-03-15 15:02:52 UTC
We have a failing test in master that appears to stem from here..
Comment 13 Martin Renvoize 2023-03-15 15:26:53 UTC
Created attachment 148220 [details] [review]
Bug 32878: (follow-up) Exclude non_priority holds

This patch filters out non_priorty holds in the on_reserve condition.

Test plan
1) Run t/db_dependant/Holds.t
2) Note it fails without this patch
3) Apply patch
4) Re-run the above test, note it now passes
Comment 14 Martin Renvoize 2023-03-15 15:27:40 UTC
Must admit.. I always get a little confused around the priority fields of holds.. so a double check that this makes sense would be appreciated.
Comment 15 Nick Clemens (kidclamp) 2023-03-15 15:56:38 UTC
Created attachment 148223 [details] [review]
Bug 32878: (follow-up) Exclude non_priority holds

This patch filters out non_priorty holds in the on_reserve condition.

Test plan
1) Run t/db_dependant/Holds.t
2) Note it fails without this patch
3) Apply patch
4) Re-run the above test, note it now passes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens (kidclamp) 2023-03-15 15:57:16 UTC
(In reply to Martin Renvoize from comment #14)
> Must admit.. I always get a little confused around the priority fields of
> holds.. so a double check that this makes sense would be appreciated.

Non-priority holds specify when placing that they should not block renewals, so this is correct
Comment 17 Tomás Cohen Arazi 2023-03-16 19:11:35 UTC
Follow-up pushed to master. Thanks, you rock!
Comment 18 Lucas Gass 2023-04-04 16:09:34 UTC
Can this be backported to 22.11.x?
Comment 19 Laura Escamilla 2023-04-10 11:31:37 UTC
Just following up to see if this can be backported to 22.11.x?
Comment 20 Laura Escamilla 2023-05-25 17:05:33 UTC
(In reply to Laura Escamilla from comment #19)
> Just following up to see if this can be backported to 22.11.x?

Just wanted to check on this one again to see if it could be backported.
Comment 21 Martin Renvoize 2023-05-26 06:30:40 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release