Bug 34902 - decreaseLoanHighHolds does not reduce loan period
Summary: decreaseLoanHighHolds does not reduce loan period
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Magnus Enger
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-24 21:07 UTC by Eric Phetteplace
Modified: 2024-05-13 13:51 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:


Attachments
Bug 34902: decreaseLoanHighHolds does not reduce loan period (7.05 KB, patch)
2024-03-19 20:45 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 34902: decreaseLoanHighHolds does not reduce loan period (7.11 KB, patch)
2024-04-08 10:49 UTC, Philip Orr
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Phetteplace 2023-09-24 21:07:00 UTC
To recreate:

- create item type with 14 day checkout period that allows holds
- create bib with one of those items
- decreaseLoanHighHolds = enable, decreaseLoanHighHoldsDuration = 7, decreaseLoanHighHoldsValue = 1, decreaseLoanHighHoldsControl = "over the number of holdable items on the records"
- place 2 holds on the item (which would be 1 over the number of holdable items)
- checkout the item to a different patron

Expected: the manual says you'll see a warning because decreaseLoanHighHolds is triggered and the checkout period should be 7 days (the reduced loan duration).

Actual: no warning and the item checks out for 14 days (the item type's default).

The bug is likely specific to the item-level holds configuration for decreaseLoanHighHolds but I'm not sure, all we've done is verify these exact conditions recreate it. We asked ByWater if we were doing something wrong and they also verified the bug.

I see there's a few bugs related to decreaseLoanHighHolds like Bug 25824 (about its tests) and Bug 22005 (about it counting the hold currently being filled towards the holds value, note that is NOT this bug describes because it occurs when a patron _without_ a hold goes to check out the item) but I believe this specific behavior is not covered by those.
Comment 1 Magnus Enger 2024-03-19 20:44:06 UTC
The description of the relevant syspres says: 

[Enable] the reduction of loan period to [7] days for high demand items with more than [1] holds [over the number of holdable items on the record].

So we should reduce the loan period for items with *more than* [1] holds over the number of holdable items. 

The scenario is that a patron is in the library to pick up a book that has been on hold for them, and we need to consider the holds that are left, after this patron checks out their loan. Should the loan period of this loan be made shorter, or not? 

So if there is

- 1 holdable item and 1 remaining hold, we should not reduce the loan period (this is *equal to* the number of holdable items + 1, but not *more than*)

- 1 holdable item and 2 remaining holds, we should reduce the loan period (because this is *more than* the limit we have set)

But the code looks like this now:

            my $threshold = $items_count + $decreaseLoanHighHoldsValue;

            # If the number of holds is less than the count of items we have
            # plus the number of holds allowed above that count, we can stop here
            if ( $holds->count() <= $threshold ) {
                return $return_data;
            }

So with 1 item and decreaseLoanHighHoldsValue = 1 we get a $threshold of 2. But this is then compared with a "more than or equal" to the number of remaining holds, meaning that both with 1 and 2 remaining holds, the loan period is not shortened. But it should be shortened when there are two remaining holds. So the comparison should be "more than", not "more than or equal". Patch coming.
Comment 2 Magnus Enger 2024-03-19 20:45:09 UTC
Created attachment 163481 [details] [review]
Bug 34902: decreaseLoanHighHolds does not reduce loan period

See the bug for a description of the problem.

Settings:
- decreaseLoanHighHolds = Enable
- decreaseLoanHighHoldsDuration = 7
- decreaseLoanHighHoldsValue = 1
- decreaseLoanHighHoldsControl = over the number of holdable
  items on the record
- decreaseLoanHighHoldsIgnoreStatuses = [none selcted]

Set a default loan period of 28 days under Administration >
Circulation and fine rules

To reproduce:
- Find a record with one item
- Add three holds on the record, for three different patrons
- Check out the item to the first patron on the waiting list
- Verify that the item is issued without any shortened loan time

To test:
- Return the loan you made above, and ignore the holds on the record
- Add another hold to the record
- Apply the patch
- Restart all the things
- Check out the item to the first patron on the waiting list
- Verify that there is now a warning about a shortened loan time
Comment 3 Philip Orr 2024-04-08 10:49:32 UTC
Created attachment 164495 [details] [review]
Bug 34902: decreaseLoanHighHolds does not reduce loan period

See the bug for a description of the problem.

Settings:
- decreaseLoanHighHolds = Enable
- decreaseLoanHighHoldsDuration = 7
- decreaseLoanHighHoldsValue = 1
- decreaseLoanHighHoldsControl = over the number of holdable
  items on the record
- decreaseLoanHighHoldsIgnoreStatuses = [none selcted]

Set a default loan period of 28 days under Administration >
Circulation and fine rules

To reproduce:
- Find a record with one item
- Add three holds on the record, for three different patrons
- Check out the item to the first patron on the waiting list
- Verify that the item is issued without any shortened loan time

To test:
- Return the loan you made above, and ignore the holds on the record
- Add another hold to the record
- Apply the patch
- Restart all the things
- Check out the item to the first patron on the waiting list
- Verify that there is now a warning about a shortened loan time

Signed-off-by: Philip Orr <philip.orr@lmscloud.de>
Comment 4 Marcel de Rooy 2024-04-12 09:43:37 UTC
Please note the static branch:

            # If the number of holds is not above the threshold, we can stop here
            if ( $holds->count() <= $decreaseLoanHighHoldsValue ) {
                return $return_data;
            }

And as I read the original code, I think that it does what the pref says.

             # If the number of holds is less than the count of items we have
             # plus the number of holds allowed above that count, we can stop here
-            if ( $holds->count() <= $threshold ) {
+            if ( $holds->count() < $threshold ) {
                 return $return_data;
             }

So if you have 1 holdable item and pref = 1, than we should check if holds->count <= 2. In that case you do not exceed. If it is 3, than the holds count is more than 1 PLUS the number of holdable items (1) on the record.