Bug 36331 - Items that cannot be held are prevented renewal when there are holds on the record
Summary: Items that cannot be held are prevented renewal when there are holds on the r...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-15 13:45 UTC by Nick Clemens (kidclamp)
Modified: 2024-04-17 11:58 UTC (History)
7 users (show)

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


Attachments
Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed (4.73 KB, patch)
2024-03-15 13:53 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed (4.79 KB, patch)
2024-03-15 15:42 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed (4.88 KB, patch)
2024-03-22 10:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36331: (follow-up) Ignore non_priority holds when checking renewability (1.94 KB, patch)
2024-03-22 13:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36331: (follow-up) Ignore non_priority holds when checking renewability (2.01 KB, patch)
2024-03-22 14:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-03-15 13:45:08 UTC
In the case where an item cannot be held 'AllowRenewalIfOtherItemsAvailable' will deny renewal if no other items are available. Since the holds don't affect this item, it is invalid.

There is a FIXME in the code about this:
 # FIXME: We are not checking whether the item we are renewing can fill the hold
Comment 1 Nick Clemens (kidclamp) 2024-03-15 13:53:27 UTC
Created attachment 163236 [details] [review]
Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed

Before this patch we get all holds on a record and see if we can fill them with available items.
This means we check to fill holds that the item in questoion may not be able to fill, especially
in the case where no holds are allowed on the item type, this is wrong

To test:
1 - Find or create a biblio with two items of different item types
2 - Make sure one item type allows holds, and the other has:
    "Default holds policy by item type"
    Set to "No holds allowed"
3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
4 - Check out the unholdable item to a patron
5 - Set a hold for a different patron on the next available item
6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page)
7 - Checkout the other item to a third patron
8 - Confirm the first item can still be renewed
9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow"
10 - Confirm the item cannot be renewed now
11 - Apply patch, restart all
12 - Confirm the item can be renewed
13 - Set the item type to a type that allows holds
14 - Confirm the item can no longer be renewed
15 - Restore the item type
16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
17 - Confirm the item can be renewed
18 - Check in the item from the third patron
19 - Confirm the item can still be renewed
20 - prove -v t/db_dependent/Circulation.t - test still pass
Comment 2 Andrew Fuerste-Henry 2024-03-15 15:42:52 UTC
Created attachment 163254 [details] [review]
Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed

Before this patch we get all holds on a record and see if we can fill them with available items.
This means we check to fill holds that the item in questoion may not be able to fill, especially
in the case where no holds are allowed on the item type, this is wrong

To test:
1 - Find or create a biblio with two items of different item types
2 - Make sure one item type allows holds, and the other has:
    "Default holds policy by item type"
    Set to "No holds allowed"
3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
4 - Check out the unholdable item to a patron
5 - Set a hold for a different patron on the next available item
6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page)
7 - Checkout the other item to a third patron
8 - Confirm the first item can still be renewed
9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow"
10 - Confirm the item cannot be renewed now
11 - Apply patch, restart all
12 - Confirm the item can be renewed
13 - Set the item type to a type that allows holds
14 - Confirm the item can no longer be renewed
15 - Restore the item type
16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
17 - Confirm the item can be renewed
18 - Check in the item from the third patron
19 - Confirm the item can still be renewed
20 - prove -v t/db_dependent/Circulation.t - test still pass

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 3 Marcel de Rooy 2024-03-22 08:41:57 UTC
Looking here
Comment 4 Marcel de Rooy 2024-03-22 09:03:12 UTC
                  next unless CanItemBeReserved($patron_with_reserve,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK';
                  # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy'
                  # CanItemBeReserved checks 'rules' and 'policies' which means
                  # items will fill holds at checkin that are rejected here

Stumbling over that note while looking here.. Why do we actually need that check? Trying to understand altough we could say that it is outside scope ? Perhaps.
Comment 5 Marcel de Rooy 2024-03-22 10:15:26 UTC
Also wondering (but very theoretical here) if walking thru items and holds in another order could possibly produce other results.. But not for now :)
Comment 6 Marcel de Rooy 2024-03-22 10:18:23 UTC
Created attachment 163673 [details] [review]
Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed

Before this patch we get all holds on a record and see if we can fill them with available items.
This means we check to fill holds that the item in questoion may not be able to fill, especially
in the case where no holds are allowed on the item type, this is wrong

To test:
1 - Find or create a biblio with two items of different item types
2 - Make sure one item type allows holds, and the other has:
    "Default holds policy by item type"
    Set to "No holds allowed"
3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
4 - Check out the unholdable item to a patron
5 - Set a hold for a different patron on the next available item
6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page)
7 - Checkout the other item to a third patron
8 - Confirm the first item can still be renewed
9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow"
10 - Confirm the item cannot be renewed now
11 - Apply patch, restart all
12 - Confirm the item can be renewed
13 - Set the item type to a type that allows holds
14 - Confirm the item can no longer be renewed
15 - Restore the item type
16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
17 - Confirm the item can be renewed
18 - Check in the item from the third patron
19 - Confirm the item can still be renewed
20 - prove -v t/db_dependent/Circulation.t - test still pass

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Nick Clemens (kidclamp) 2024-03-22 10:32:30 UTC
(In reply to Marcel de Rooy from comment #4)
>                   next unless
> CanItemBeReserved($patron_with_reserve,$other_item,undef,
> {ignore_hold_counts=>1})->{status} eq 'OK';
>                   # NOTE: At checkin we call 'CheckReserves' which checks
> hold 'policy'
>                   # CanItemBeReserved checks 'rules' and 'policies' which
> means
>                   # items will fill holds at checkin that are rejected here
> 
> Stumbling over that note while looking here.. Why do we actually need that
> check? Trying to understand altough we could say that it is outside scope ?
> Perhaps.

(In reply to Marcel de Rooy from comment #4)
>                   next unless
> CanItemBeReserved($patron_with_reserve,$other_item,undef,
> {ignore_hold_counts=>1})->{status} eq 'OK';
>                   # NOTE: At checkin we call 'CheckReserves' which checks
> hold 'policy'
>                   # CanItemBeReserved checks 'rules' and 'policies' which
> means
>                   # items will fill holds at checkin that are rejected here
> 
> Stumbling over that note while looking here.. Why do we actually need that
> check? Trying to understand altough we could say that it is outside scope ?
> Perhaps.

I do think it is outside of the scope, it is really meant to be a for the coders - essentially this routine is more strict, but I think that makes sense.

This has come up on other reports, we have two concepts:
Can a hold be placed on this item
Can this item fill a hold

My patch here simply limits the holds checked against renewal to the ones that the item being renewed would fill.

This is pre-existing logic that won't assume another item on the record can fill a hold that can't be placed (by a patron, staff can override)

Librarians want this for the situation I describe in the test plan - a record with a mix of items that can and cannot be held. Generally these are new books, or 'lucky day' books as some libraries call them - books that won't be on hold so you get 'lucky' and find them in the library. These items cannot have holds, but currently holds can prevent their renewal - even with AllowRenewalIfOtherItemsAvailable 


(In reply to Marcel de Rooy from comment #5)
> Also wondering (but very theoretical here) if walking thru items and holds
> in another order could possibly produce other results.. But not for now :)

Very true, perhaps once we get 35826 we could use that logic too:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35826
Comment 8 Katrin Fischer 2024-03-22 10:52:39 UTC
Maybe a unit test testing this specific case would be good?
Comment 9 Katrin Fischer 2024-03-22 11:28:42 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 10 Katrin Fischer 2024-03-22 12:55:55 UTC
It looks like this actually breaks unit tests:

prove t/db_dependent/Holds.t 
t/db_dependent/Holds.t .. 68/74 Use of uninitialized value in numeric le (<=) at /kohadevbox/koha/C4/Circulation.pm line 3014.
Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Circulation.pm line 3021.
Use of uninitialized value in numeric le (<=) at /kohadevbox/koha/C4/Circulation.pm line 3014.
Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Circulation.pm line 3021.

    #   Failed test 'Can renew'
    #   at t/db_dependent/Holds.t line 1531.

    #   Failed test 'Item is on non priority hold'
    #   at t/db_dependent/Holds.t line 1532.
    #          got: 'on_reserve'
    #     expected: undef
Use of uninitialized value in numeric le (<=) at /kohadevbox/koha/C4/Circulation.pm line 3014.
Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Circulation.pm line 3021.
    # Looks like you failed 2 tests of 6.

#   Failed test 'non priority holds'
#   at t/db_dependent/Holds.t line 1559.
t/db_dependent/Holds.t .. 73/74 # Looks like you failed 1 test of 74.
t/db_dependent/Holds.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/74 subtests 

Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 256 Tests: 74 Failed: 1)
  Failed test:  69
  Non-zero exit status: 1
Files=1, Tests=74,  7 wallclock secs ( 0.02 usr  0.01 sys +  4.67 cusr  0.98 csys =  5.68 CPU)
Result: FAIL


Please follow-up!
Comment 11 Nick Clemens (kidclamp) 2024-03-22 13:55:41 UTC
Created attachment 163706 [details] [review]
Bug 36331: (follow-up) Ignore non_priority holds when checking renewability

When changing the fetch of holds, the check for non-priority was lost - added a loop to pull those out
so the totals and checks are correct
Comment 12 Tomás Cohen Arazi 2024-03-22 14:03:09 UTC
Created attachment 163707 [details] [review]
Bug 36331: (follow-up) Ignore non_priority holds when checking renewability

When changing the fetch of holds, the check for non-priority was lost - added a loop to pull those out
so the totals and checks are correct

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tidied (tcohen)
Comment 13 Katrin Fischer 2024-03-22 14:08:06 UTC
Thanks for the fast reaction! Follow-up pushed to master.
Comment 14 Fridolin Somers 2024-04-17 11:58:37 UTC
Pushed to 23.11.x for 23.11.05