Bug 34902

Summary: decreaseLoanHighHolds does not reduce loan period
Product: Koha Reporter: Eric Phetteplace <ephetteplace>
Component: CirculationAssignee: Magnus Enger <magnus>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: gmcharlt, kyle.m.hall, lisette, m.de.rooy, magnus, pierre.genty, pnavarrete
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 34902: decreaseLoanHighHolds does not reduce loan period
Bug 34902: decreaseLoanHighHolds does not reduce loan period

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.
Comment 5 Magnus Enger 2024-05-24 07:34:13 UTC
In the static branch, the comment and the code agrees: 

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

It says "If the number of holds is not above the threshold" and we are testing that "$holds->count() is less than or equal to $decreaseLoanHighHoldsValue", which is the same as "$holds->count() is not above $decreaseLoanHighHoldsValue". 

But in the "dynamic" branch, the comment and the code does not agree: 

            # 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;
            }

The comment says "If the number of holds is less than the count of items", but we are testing "$holds->count() <= $threshold", which is "the number of holds is less than *or equal to* the count of items". There is also this comment: 

            # dynamic means X more than the number of holdable items on the record

So again, it says "more than", not "more than or equal to". 

I also tested this again. I set these sysprefs:

decreaseLoanHighHolds
decreaseLoanHighHoldsDuration
decreaseLoanHighHoldsValue
decreaseLoanHighHoldsControl

so I had: "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]."

Then I placed four holds on a record with one item. 

- Check out to the first patron in the list of four holds => loan time decreased 
- Check the item in, ignore the next hold
- Check out to the patron that is first in the list of three holds now => loan time NOT decreased
- Check the item in, ignore the next hold

So, when checking out to a patron when there are three holds, the loan time is not decreased. As far as I can tell, this is wrong. 

Then I applied the patch and made two more holds on the record, for a total of four. 

- Check out to the first patron in the list of four holds => loan time decreased 
- Check the item in, ignore the next hold
- Check out to the patron that is first in the list of three holds now => loan time decreased
- Check the item in, ignore the next hold
- Check out to the patron that is first in the list of two holds => loan time NOT decreased

As far as I can tell, this is correct. The sysprefs read "items with *more than* [1] holds [over the number of holdable items on the record]". I have 1 holdable item and decreaseLoanHighHoldsValue adds 1 to that, for a threshold of 2. So when the number of holds on the record (2) equals that threshold, the loan time is not decreased. But when the number of holds (3 or 4) exceeds the threshold, the loan time is decreased.