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.
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.
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>
Created attachment 146740 [details] [review] Bug 32878: (QA follow-up) Unit test
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>
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>
Created attachment 146830 [details] [review] Bug 32878: (QA follow-up) Unit test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 146833 [details] [review] Bug 32878: (QA follow-up) Unit test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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>
Pushed to master for 23.05. Nice work everyone, thanks!
We have a failing test in master that appears to stem from here..
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
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.
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>
(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
Follow-up pushed to master. Thanks, you rock!
Can this be backported to 22.11.x?
Just following up to see if this can be backported to 22.11.x?
(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.
Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release