Bug 40792 - Do not fill hold requests when issuing non-holdable items
Summary: Do not fill hold requests when issuing non-holdable items
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 24.11
Hardware: All All
: P5 - low minor
Assignee: Andreas Jonsson
QA Contact: Testopia
URL:
Keywords:
Depends on: 38650
Blocks:
  Show dependency treegraph
 
Reported: 2025-09-11 10:55 UTC by Andreas Jonsson
Modified: 2025-10-23 12:36 UTC (History)
14 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 40792: Do not fill holds with non-reservable items. (3.33 KB, patch)
2025-09-11 14:10 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 40792: Do not fill holds with non-reservable items. (2.92 KB, patch)
2025-09-18 14:09 UTC, Andreas Jonsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2025-09-11 10:55:07 UTC
It is a regression from bug 38650 that issuing non-holdable items now fills holds.

The subroutine C4::Reserves::MoveReserve would previously only do anything if there where a hold that had been filled with the specified item, which mean that the item issued was holdable.

But bug 38650 opened up the possibility to fill the borrowers hold even if the issued item is not holdable.

I suggest a check that the item issued is holdable if the biblio is reserved by the borrower.
Comment 1 Andreas Jonsson 2025-09-11 14:10:22 UTC
Created attachment 186376 [details] [review]
Bug 40792: Do not fill holds with non-reservable items.

Verify that the item issued is reservable before filling the patron's
hold.

1. Go to circulation and fine rules and add a rule for an itemtype
   that can be issued, for instance "Maps" in koha-testing-docker,
   with 0 in the column "Holds per record (count)".
2. Go to any biblio where there are items that may be reserved.
   Create an item with the above created item type and give it a barcode,
   e.g., 'tempbarcode'.
3. Place a hold for any patron (for instance 23529000120056 in
   koha-testing-docker).
4. Issue the item 'tempbarcode' to the patron with the hold.
5. Make sure the hold remains.
6. Create a new hold to another borrower (for instance
   23529000197047), and make sure this new hold has priority 2.
7. Return the item 'tempbarcode' and issue it to the second patron.
8. Make sure both holds remain.
Comment 2 Hans Pålsson 2025-09-11 14:27:08 UTC
Tested in production environment, works as expected. Great work!

Note: this is to enable patrons to have a title level reservation on a title that has multiple item types, one type that is allowed to fill a hold (normal loan) and one type that is not allowed to fill a hold (might be short-time loan etc). Patrons must be able to keep their title level reservation on normal loan item type while they can check out the short-time item type.
Comment 3 Jonathan Druart 2025-09-16 13:30:30 UTC
There are several conflicts.

CONFLICT (content): Merge conflict in C4/Reserves.pm

Please rebase.
Comment 4 Andreas Jonsson 2025-09-18 14:09:39 UTC
Created attachment 186565 [details] [review]
Bug 40792: Do not fill holds with non-reservable items.

Verify that the item issued is reservable before filling the patron's
hold.

1. Go to circulation and fine rules and add a rule for an itemtype
   that can be issued, for instance "Maps" in koha-testing-docker,
   with 0 in the column "Holds per record (count)".
2. Go to any biblio where there are items that may be reserved.
   Create an item with the above created item type and give it a barcode,
   e.g., 'tempbarcode'.
3. Place a hold for any patron (for instance 23529000120056 in
   koha-testing-docker).
4. Issue the item 'tempbarcode' to the patron with the hold.
5. Make sure the hold remains.
6. Create a new hold to another borrower (for instance
   23529000197047), and make sure this new hold has priority 2.
7. Return the item 'tempbarcode' and issue it to the second patron.
8. Make sure both holds remain.
Comment 5 Andreas Jonsson 2025-09-18 14:10:23 UTC
Rebased on main.
Comment 6 Marcel de Rooy 2025-09-19 06:46:17 UTC
Hi,
This patch changes a module (Reserves) and does not add tests.
Please add unit tests.
And verify that your change does not break other tests.
Comment 7 Marcel de Rooy 2025-09-19 06:47:05 UTC
And please fill Assignee next time :)
Comment 8 Nick Clemens (kidclamp) 2025-10-16 13:33:53 UTC
I believe this also needs a system preference or a switch in the circulation rules. The intention on 38650 was to fill the hold regardless of the item's holdability.

We have some libraries who maintain a 'lucky day' collection of popular books which cannot have holds placed directly, however, a patron having a 'lucky day' can find the book they want on the shelf and check it out - when doing so, the hold is expected to be filled
Comment 9 Andreas Jonsson 2025-10-16 13:53:03 UTC
(In reply to Nick Clemens (kidclamp) from comment #8)
> I believe this also needs a system preference or a switch in the circulation
> rules. The intention on 38650 was to fill the hold regardless of the item's
> holdability.
> 
> We have some libraries who maintain a 'lucky day' collection of popular
> books which cannot have holds placed directly, however, a patron having a
> 'lucky day' can find the book they want on the shelf and check it out - when
> doing so, the hold is expected to be filled

Maybe a boolean parameter on the itemtype: "may fill reservations"? Or even a circulation rule?
Comment 10 Steve, OSLRI, USA 2025-10-16 14:35:58 UTC
I could see both sides of this... that is wanting to satisfy the hold and not wanted to satisfy the hold.  I think the most elegant way to do this would be at the itemtype level.  Just a true/false that allows an itemtype to fill a hold.  That would allow a library or consortia to take a more nuanced, situational approach.