[Bug 42999] New: $valid_items flag contamination in reserve/request.pl causes non-holdable items to appear as available after the first holdable item in the loop
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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 --- Comment #1 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 201539 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201539&action=edit Patch The fix is minimal: merge the two consecutive if blocks (lines 570–583 in the original) back into one, so the pickup-location logic is gated by the per-item condition check rather than the loop-persistent $valid_items flag. $valid_items retains its correct post-loop role at line 661 (none_available_override). When handling flags there should be caution with what they are used for and the way they change values. In the About page there was a bug because the flag of the background color was never reset. Once a perl library was missing, all the following inherited that class. This bug is similar. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 201683 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201683&action=edit Bug 42999: Fix $valid_items flag contamination in reserve/request.pl $valid_items was set inside the per-item availability check but used as the gate of a separate if block, causing it to carry over across loop iterations. Once any item set it to 1, all subsequent items entered the if ($valid_items) branch and received $item->{available} = 1 regardless of their individual availability. Fix: merge the two consecutive if blocks into one so the pickup-location logic only executes when the current item individually passes all availability conditions. $valid_items continues to serve its correct post-loop role (line 661: none_available_override). Regression introduced by Bug 38412 (commit 1900effc9b). Steps to follow: 1. Create a biblio with at least two items: one with a holdable item type and one with a not-for-loan item type (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). 2. Open reserve/request.pl?biblionumber=X&borrowernumber=Y 3. Observe that the not-for-loan item shows a radio button and a pickup location dropdown despite the Information column showing "Not for loan (Itemtype not for loan)" — bug confirmed. 4. Apply this patch and restart services. If running under Plack, restarting Plack is required for the change to take effect. 5. Reload the place-hold page. 6. Verify the not-for-loan item now shows "X Not for loan" without a radio button or pickup location dropdown. 7. Verify the holdable items still show radio buttons and pickup location dropdowns correctly. Signed-off-by: Adolfo Rodríguez <adolfo.rodriguez@xercode.es> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Attachment #201539|0 |1 is obsolete| | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This fixes placing holds release notes| |using the staff interface | |for a specific item, where | |there is an item that has a | |"Not for loan" status. | | | |Previously, you could | |select the item that is not | |for loan. | | | |Now the item that is not | |for loan has an "X Not for | |loan" in red in the hold | |column, and you can't | |select it. --- Comment #3 from David Nind <david@davidnind.com> --- Testing notes (using KTD): 1. I used Effective Perl programming (139), as it has two items already. 2. I changed the second item (for Fairview) so that the "7 - Not for loan" was set to "Not for loan". 3. Before the patch, when placing a hold using the staff interface: - For the "Hold a specific item" section, you can still select the item for the item that has "Not on hold Not for loan (Not for loan)" in the information column 4. After the patch, when placing a hold using the staff interface: - For the "Hold a specific item" section, the hold column has "X Not for loan" in red for the item that has "Not on hold Not for loan (Not for loan)" in the information column -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |adolfo.rodriguez@xercode.es |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201683|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202000 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202000&action=edit Bug 42999: Fix $valid_items flag contamination in reserve/request.pl $valid_items was set inside the per-item availability check but used as the gate of a separate if block, causing it to carry over across loop iterations. Once any item set it to 1, all subsequent items entered the if ($valid_items) branch and received $item->{available} = 1 regardless of their individual availability. Fix: merge the two consecutive if blocks into one so the pickup-location logic only executes when the current item individually passes all availability conditions. $valid_items continues to serve its correct post-loop role (line 661: none_available_override). Regression introduced by Bug 38412 (commit 1900effc9b). Steps to follow: 1. Create a biblio with at least two items: one with a holdable item type and one with a not-for-loan item type (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). 2. Open reserve/request.pl?biblionumber=X&borrowernumber=Y 3. Observe that the not-for-loan item shows a radio button and a pickup location dropdown despite the Information column showing "Not for loan (Itemtype not for loan)" — bug confirmed. 4. Apply this patch and restart services. If running under Plack, restarting Plack is required for the change to take effect. 5. Reload the place-hold page. 6. Verify the not-for-loan item now shows "X Not for loan" without a radio button or pickup location dropdown. 7. Verify the holdable items still show radio buttons and pickup location dropdowns correctly. Signed-off-by: Adolfo Rodríguez <adolfo.rodriguez@xercode.es> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks Adolfo, great work. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 --- Comment #6 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.11.00 |26.11.00,26.05.03 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 --- Comment #7 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to 26.05.x for the upcoming 26.05.03 release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42999 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=43448 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org