https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Bug ID: 42999 Summary: $valid_items flag contamination in reserve/request.pl causes non-holdable items to appear as available after the first holdable item in the loop Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: adolfo.rodriguez@xercode.es QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, lisette@bywatersolutions.com, tomascohen@gmail.com Target Milestone: --- Bug 38412 (commit 1900effc9b) introduced $valid_items as a flag to track whether any item passes all availability conditions. However, the flag is also used as a per-item gate in the same loop without being reset between iterations. Once any item sets $valid_items = 1, all subsequent items — regardless of their individual availability — enter the if ($valid_items) branch and are assigned $item->{available} = 1. Root cause (reserve/request.pl): The commit split what was a single if block into two consecutive, separate blocks: # Line 455 — initialized once, outside the loop my $valid_items = 0; for my $item_object (@items) { ... # Block 1 (line 570): per-item condition check if ( !$item->{cantreserve} && !$exceeded_maxreserves && $can_item_be_reserved eq 'OK' && IsAvailableForItemLevelRequest( $item_object, $patron, undef ) ) { $valid_items = 1; # set but never reset between iterations } # Block 2 (line 583): uses $valid_items as per-item gate — BUG if ($valid_items) { # true for ALL items after the first valid one $num_items_available++; $item->{available} = 1; ... } elsif ( C4::Context->preference('AllowHoldPolicyOverride') ) { ... } else { $item->{available} = 0; # never reached after first valid item } } # Line 661 — post-loop: this use of $valid_items is correct if ( $valid_items == 0 ) { $template->param( none_available_override => 1 ); } The variable serves two conflicting roles: Global accumulator (correct, line 661): "did any item pass all checks?" Per-item gate (incorrect, line 583): "does this item pass all checks?" — but it carries over from previous iterations. Steps to reproduce: Create a biblio with at least two items: one with a holdable item type (e.g. Books) and one with a not-for-loan item type (e.g. Reference, itemtypes.notforloan = 1). The not-for-loan item must have a higher itemnumber than at least one holdable item (i.e., it is processed later in the loop). Open the place-hold staff form: reserve/request.pl?biblionumber=X&borrowernumber=Y Expected: The not-for-loan item shows no radio button / is not selectable for holds. Actual: The not-for-loan item has a radio button and available = 1, despite the Information column correctly showing "Not for loan (Itemtype not for loan)". This is reproducible with AllowHoldPolicyOverride either on or off. Diagnostic trace on a local instance running main (as of 2026-07-06, biblionumber 47160, patron borrowernumber 2): AllowHoldPolicyOverride: NO itemno barcode itype ifl can_reserve cond_passes valid_items available branch_taken --------------------------------------------------------------------------------------------------- 27924 FFL24040002 BK 0 OK YES 0->1 1 if($valid_items) 27926 CPL24040004 BK 0 OK YES 1->1 1 if($valid_items) 27936 CPL24050001 BK 0 OK YES 1->1 1 if($valid_items) 28043 CPL24110003 REF 1 OK NO 1->1 1 if($valid_items) <-- BUG 28046 CPL25030002 BK 0 OK YES 1->1 1 if($valid_items) ... Legend: ifl = itemtype notforloan flag cond_passes = item individually passes all availability conditions valid_items = flag value before->after this iteration <-- BUG = actual result differs from correct result Note: in this test case CanItemBeReserved returns OK for the REF item (depending on circulation rules, it may also return notforloan); either way, IsAvailableForItemLevelRequest returns false due to the itemtype flag, so cond_passes = NO. The bug manifests regardless. Proposed fix: Merge the two if blocks back into one, so the pickup-location logic and $item->{available} assignment only execute when the current item individually passes all conditions. The global $valid_items accumulator for the post-loop check at line 661 is preserved: if ( !$item->{cantreserve} && !$exceeded_maxreserves && $can_item_be_reserved eq 'OK' && IsAvailableForItemLevelRequest( $item_object, $patron, undef ) ) { $valid_items = 1; my $pickup_locations = $item_object->pickup_locations( { patron => $patron } ); $item->{pickup_locations_count} = $pickup_locations->count; if ( $item->{pickup_locations_count} > 0 || C4::Context->preference('AllowHoldPolicyOverride') ) { $num_items_available++; $item->{available} = 1; ... } else { $item->{available} = 0; $item->{not_holdable} = "no_valid_pickup_location"; } } elsif ( C4::Context->preference('AllowHoldPolicyOverride') ) { ... } else { $item->{available} = 0; } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.