https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |tomascohen@gmail.com -- 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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192131&action=edit Bug 41728: Add Koha::Item::Checkin::Availability class This patch introduces a new class to centralize and extract check-in validation logic from C4::Circulation::AddReturn. The class provides a clean interface for checking whether an item can be checked in, returning a Koha::Availability::Result object. The class is designed with the item as the subject of the check-in operation, following the pattern: "Can THIS ITEM be checked in?" The class extracts the following validation logic from AddReturn: - Withdrawn item handling (BlockedWithdrawn blocker) - Lost item handling (BlockedLost blocker) - Branch check-in policy validation (Wrongbranch blocker) - Not issued detection (NotIssued confirmation) This separation enables: - Reusable validation logic for REST API and legacy code - Pre-flight availability checks without side effects - Structured responses for confirmation flow implementation - Better testability and maintainability The check() method accepts an item object and returns a Koha::Availability::Result object with blockers, confirmations, warnings, and context (checkout, patron). A convenience instance method is added to Koha::Item: $item->checkin_availability({ branch => $branch }) Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! 3. Verify all validation scenarios work correctly: - Withdrawn items blocked when syspref enabled - Lost items blocked when syspref enabled - Wrong branch returns blocked per AllowReturnToBranch - Not issued items handled correctly - Checked out items return checkout and patron objects 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com, | |Laura.escamilla@bywatersolu | |tions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |24401 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24401 [Bug 24401] REST API: Check-in -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #5 from Jonathan Druart <jonathan.druart@gmail.com> --- Wondering if this is not a good candidate for the Koha::Policy namespace. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #6)

(In reply to Jonathan Druart from comment #5)

...

Wondering if this is not a good candidate for the Koha::Policy namespace.

The main reason I didn't use the Policy namespace is the return value i.e. boolean (policy current) vs the aggregate class with many return values.

This was just to illustrate, it also returns: * cardnumber min/max length * a branchcode

We could move into something like:

```perl Koha::Availability::Result # Base result class Koha::Policy::Checkin::Availability # Policy namespace, availability focus Koha::Policy::Checkout::Availability

```

^^^ that's my proposal if people wants to move it to the `Koha::Policy` namespace, making the 'Availability' classes consistently return a `Koha::Availability::Result` object. I even envision the object could also carry the JWT for the 'Confirmation Flow' and maybe ways to validate the JWT (e.g. Koha::Availability::Result->new_from_jwt that would explode if JTW validation fails, etc) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |7376 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- New dependency (bug 7376) requires reworking this. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7376 [Bug 7376] Transfer limits should be checked at check-in -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192130|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192132|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=41728 --- Comment #10 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 196134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196134&action=edit Bug 41728: Add Koha::Availability::Result base class This patch introduces a standardized result class for availability checks, providing a clean API with proper methods instead of raw hashrefs. The Koha::Availability::Result class provides: - add_blocker() - Add conditions that prevent the action - add_confirmation() - Add conditions requiring user confirmation - add_warning() - Add informational messages - set_context() - Store related objects (item, checkout, patron, etc.) - available() - Check if action can proceed (no blockers) - needs_confirmation() - Check if confirmations are required - Accessor methods for all categories - to_hashref() for backward compatibility This design: - Provides a consistent interface across all availability checks - Makes the API more discoverable and self-documenting - Enables future enhancements without breaking changes - Follows OO principles with proper encapsulation - Can be used as base for checkout, hold, renewal availability Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/Availability/Result.t => SUCCESS: Tests pass! 3. Verify all methods work correctly 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #12 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 196136 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196136&action=edit Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability This patch refactors AddReturn to use the new Koha::Item::Checkin::Availability class for validation logic, eliminating code duplication and centralizing check-in validation. Changes: - Add use statement for Koha::Item::Checkin::Availability - Look up item first, return BadBarcode if not found - Call $item->checkin_availability() for validation - Use Result object methods (available(), blockers, context) - Replace inline withdrawn/lost/branch validation with blocker handling - Remove duplicate CanBookBeReturned() call (now in Availability class) - Preserve all existing behavior and error messages - Maintain backward compatibility The refactoring extracts ~40 lines of validation logic while maintaining identical functionality. All validation results are obtained from a single availability check that returns a Result object. Note: The withdrawn message is set in two places depending on context: - When BlockReturnOfWithdrawnItems is ON: set in blocker handler before early return - When BlockReturnOfWithdrawnItems is OFF: set by existing check later in the flow This preserves the original behavior where withdrawn items always get the message, but are only blocked when the syspref is enabled. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! k$ prove t/db_dependent/Circul* => SUCCESS: Tests pass! 3. Verify check-in operations work correctly: - Items not found return BadBarcode - Withdrawn items blocked when syspref enabled - Lost items blocked when syspref enabled - Wrong branch returns blocked per AllowReturnToBranch - Not issued items handled correctly - Normal check-ins complete successfully 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #14 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 196138 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196138&action=edit Bug 41728: (follow-up) Add POD for _add_statistic Patch from commit 63c26a6 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196134|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=41728 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196136|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=41728 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196138|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=41728 --- Comment #16 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196889 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196889&action=edit Bug 41728: Add Koha::Item::Checkin::Availability class Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #18 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196891 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196891&action=edit Bug 41728: (follow-up) Add transfer limit check and clean param names - Rename params: branch => library, add to_library for destination - Move $returnbranch computation before the availability check in AddReturn so it can be passed as to_library - Add transfer limit check (bug 7376) to the Availability class - Rename internal _can_book_be_returned to _check_return_policy - Remove duplicate $returnbranch block and standalone transfer limit check from AddReturn - Update tests for new param names Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #20 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196893 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196893&action=edit Bug 41728: (QA follow-up) Populate checkout context before blocker checks The availability check is returning early on blockers (BlockedWithdrawn, BlockedLost, Wrongbranch) before checking if the item had an active checkout. This causes AddReturn to lose checkout/patron context for checked-out items that hit a blocker, resulting in incorrect NotIssued messages, unintended onloan clearing, and incorrect local use recording. Move the checkout check to run first so context is always populated regardless of blockers. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #22 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196895 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196895&action=edit Bug 41728: (QA follow-up) Fix test accessing wrong key for checkout The test was checking $result->{issue} which doesn't exist on the Result object. The correct accessor is $result->context->{checkout}, so the assertion was always trivially true. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196888|0 |1 is obsolete| | Attachment #196889|0 |1 is obsolete| | Attachment #196890|0 |1 is obsolete| | Attachment #196891|0 |1 is obsolete| | Attachment #196892|0 |1 is obsolete| | Attachment #196893|0 |1 is obsolete| | Attachment #196894|0 |1 is obsolete| | Attachment #196895|0 |1 is obsolete| | Attachment #196896|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=41728 --- Comment #25 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197347 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197347&action=edit Bug 41728: Add Koha::Item::Checkin::Availability class Signed-off-by: Kyle M Hall <kyle@bywatersolutions.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=41728 --- Comment #27 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197349 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197349&action=edit Bug 41728: (follow-up) Add transfer limit check and clean param names - Rename params: branch => library, add to_library for destination - Move $returnbranch computation before the availability check in AddReturn so it can be passed as to_library - Add transfer limit check (bug 7376) to the Availability class - Rename internal _can_book_be_returned to _check_return_policy - Remove duplicate $returnbranch block and standalone transfer limit check from AddReturn - Update tests for new param names Signed-off-by: Kyle M Hall <kyle@bywatersolutions.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=41728 --- Comment #29 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197351 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197351&action=edit Bug 41728: (QA follow-up) Populate checkout context before blocker checks The availability check is returning early on blockers (BlockedWithdrawn, BlockedLost, Wrongbranch) before checking if the item had an active checkout. This causes AddReturn to lose checkout/patron context for checked-out items that hit a blocker, resulting in incorrect NotIssued messages, unintended onloan clearing, and incorrect local use recording. Move the checkout check to run first so context is always populated regardless of blockers. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.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=41728 --- Comment #31 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197353&action=edit Bug 41728: (QA follow-up) Fix test accessing wrong key for checkout The test was checking $result->{issue} which doesn't exist on the Result object. The correct accessor is $result->context->{checkout}, so the assertion was always trivially true. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.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=41728 --- Comment #33 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197355 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197355&action=edit Bug 41728: (QA follow-up) Move data consistency check before blocker handling The data inconsistency die (checkout exists but patron does not) was placed after the blocker handling block. This meant that if an item had corrupt data AND triggered a blocker (e.g. withdrawn+blocked), the blocker handler would return early with patron as undef instead of dying on the inconsistency, silently masking database corruption. Move the check to immediately after extracting context from the availability result, before any blocker handling, matching the original AddReturn behavior where the die fired during checkout lookup. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #35 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197357 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197357&action=edit Bug 41728: (QA follow-up) Move return policy logic to Koha::Item Move the AllowReturnToBranch and branch transfer limit checks from both C4::Circulation::CanBookBeReturned and Koha::Item::Checkin::Availability::_check_return_policy into a single authoritative method on Koha::Item: can_be_returned_at(). This eliminates the duplicate implementations and follows the Koha convention of placing item-related logic on the Koha::Item object. Changes: - Add Koha::Item->can_be_returned_at() with AllowReturnToBranch policy and transfer limit checks - Make CanBookBeReturned a thin wrapper delegating to the new method - Update Checkin::Availability to call $item->can_be_returned_at() - Remove _check_return_policy and unused withdrawn warning from the Availability class -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #37 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197359 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197359&action=edit Bug 41728: (QA follow-up) Collect all blockers instead of short-circuiting Remove early returns from Koha::Item::Checkin::Availability::check() so that all blockers, confirmations, and warnings are collected in a single pass. Previously, the first blocker triggered an early return, meaning callers could only discover one problem at a time. This is particularly useful for API consumers who want to present the full set of issues to the user in a single response rather than requiring fix-one-recheck cycles. The change is safe for AddReturn because it already handles blocker priority through its sequential if/return structure: 1. BlockedWithdrawn → early return before status updates 2. Wrongbranch → early return after status updates 3. BlockedLost → sets doreturn=0 Even when multiple blockers are present, AddReturn processes them in the correct priority order. Also makes existing tests deterministic by explicitly setting withdrawn and itemlost values on test items, preventing random TestBuilder values from triggering unrelated blockers now that all checks always run. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #38 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I'm happy with this and have added my stamp to the commits.. happy for this to go PQA if you give my, and Kyles, follow-ups a quick review Tomas? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #40 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Cool.. we could probably just drop that final follow-up then.. or move it to the next bug? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197346|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197348|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197350|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197352|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197354|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197356|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197358|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=41728 --- Comment #42 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197399 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197399&action=edit Bug 41728: Add Koha::Availability::Result base class This patch introduces a standardized result class for availability checks, providing a clean API with proper methods instead of raw hashrefs. The Koha::Availability::Result class provides: - add_blocker() - Add conditions that prevent the action - add_confirmation() - Add conditions requiring user confirmation - add_warning() - Add informational messages - set_context() - Store related objects (item, checkout, patron, etc.) - available() - Check if action can proceed (no blockers) - needs_confirmation() - Check if confirmations are required - Accessor methods for all categories - to_hashref() for backward compatibility This design: - Provides a consistent interface across all availability checks - Makes the API more discoverable and self-documenting - Enables future enhancements without breaking changes - Follows OO principles with proper encapsulation - Can be used as base for checkout, hold, renewal availability Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/Availability/Result.t => SUCCESS: Tests pass! 3. Verify all methods work correctly 4. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #43 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197400 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197400&action=edit Bug 41728: Add Koha::Availability::Result base class This patch introduces a standardized result class for availability checks, providing a clean API with proper methods instead of raw hashrefs. The Koha::Availability::Result class provides: - add_blocker() - Add conditions that prevent the action - add_confirmation() - Add conditions requiring user confirmation - add_warning() - Add informational messages - set_context() - Store related objects (item, checkout, patron, etc.) - available() - Check if action can proceed (no blockers) - needs_confirmation() - Check if confirmations are required - Accessor methods for all categories - to_hashref() for backward compatibility This design: - Provides a consistent interface across all availability checks - Makes the API more discoverable and self-documenting - Enables future enhancements without breaking changes - Follows OO principles with proper encapsulation - Can be used as base for checkout, hold, renewal availability Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/Availability/Result.t => SUCCESS: Tests pass! 3. Verify all methods work correctly 4. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #45 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197402 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197402&action=edit Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability This patch refactors AddReturn to use the new Koha::Item::Checkin::Availability class for validation logic, eliminating code duplication and centralizing check-in validation. Changes: - Add use statement for Koha::Item::Checkin::Availability - Look up item first, return BadBarcode if not found - Call $item->checkin_availability() for validation - Use Result object methods (available(), blockers, context) - Replace inline withdrawn/lost/branch validation with blocker handling - Remove duplicate CanBookBeReturned() call (now in Availability class) - Preserve all existing behavior and error messages - Maintain backward compatibility The refactoring extracts ~40 lines of validation logic while maintaining identical functionality. All validation results are obtained from a single availability check that returns a Result object. Note: The withdrawn message is set in two places depending on context: - When BlockReturnOfWithdrawnItems is ON: set in blocker handler before early return - When BlockReturnOfWithdrawnItems is OFF: set by existing check later in the flow This preserves the original behavior where withdrawn items always get the message, but are only blocked when the syspref is enabled. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! k$ prove t/db_dependent/Circul* => SUCCESS: Tests pass! 3. Verify check-in operations work correctly: - Items not found return BadBarcode - Withdrawn items blocked when syspref enabled - Lost items blocked when syspref enabled - Wrong branch returns blocked per AllowReturnToBranch - Not issued items handled correctly - Normal check-ins complete successfully 4. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #47 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197404 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197404&action=edit Bug 41728: (follow-up) Add POD for _add_statistic Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #49 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197406 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197406&action=edit Bug 41728: (QA follow-up) Match original blocker priority order Reorder blocker checks to match the original AddReturn priority: Withdrawn > Wrongbranch > Lost. Previously Lost was checked before Wrongbranch, which changed behavior for items that were both lost and at the wrong branch. QA note: There are other checks in-between these, but I don't think their order betwixt these blockers matters for retaining existing behavior. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #51 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197408 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197408&action=edit Bug 41728: (QA follow-up) Declare issue and patron variables together There seems to be no reason to declare $patron near the start of the sub while $issue is declared where they are both populated. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #53 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197410 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197410&action=edit Bug 41728: (QA follow-up) Restore original execution order for Wrongbranch The refactoring changed the relative order of the Wrongbranch early return and several status-update operations. In the original AddReturn, the execution order was: 1. checkout/patron lookup 2. not-issued handling + RecordLocalUseOnReturn 3. withdrawn check (early return) 4. location_update_trigger 5. UpdateNotForLoanStatusOnCheckin 6. CanBookBeReturned / Wrongbranch check (early return) 7. BlockReturnOfLostItems After the refactoring, the Wrongbranch blocker returned early before steps 2, 4 and 5, causing three regressions: - RecordLocalUseOnReturn was not recorded for non-issued items scanned at the wrong branch - location_update_trigger was skipped for Wrongbranch returns - UpdateNotForLoanStatusOnCheckin was skipped for Wrongbranch returns Fix by splitting blocker handling into two phases: 1. BlockedWithdrawn: earliest early return, before status updates (matches original withdrawn check position) 2. Wrongbranch and BlockedLost: after RecordLocalUseOnReturn, location_update_trigger, and UpdateNotForLoanStatusOnCheckin (matches original CanBookBeReturned position) Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #55 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197412&action=edit Bug 41728: (QA follow-up) Restore withdrawn warning and use from availability Restore the withdrawn warning in Koha::Item::Checkin::Availability that was incorrectly removed. This warning is set when an item is withdrawn but BlockReturnOfWithdrawnItems is off, making it useful for API consumers in subsequent work. Update AddReturn to read the withdrawn state from the availability result's warnings rather than re-checking $item->withdrawn directly, avoiding duplication of logic. Add test for the withdrawn warning (non-blocked) path. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #57 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197414&action=edit Bug 41728: (follow-up) Add no_short_circuit option to checkin availability This follow-up replaces the unconditional removal of short-circuit logic with a caller-controlled no_short_circuit parameter, following the same pattern established by Koha::Patron->can_place_holds. By default, check() short-circuits on the first blocker. This is the correct behavior for AddReturn, which processes blockers sequentially and only acts on the first one it finds (BlockedWithdrawn => early return, Wrongbranch => early return, BlockedLost => sets doreturn=0). Running all policy checks only to discard the results is wasteful, specially when some of them (e.g. can_be_returned_at) hit the database. When no_short_circuit => 1 is passed, all checks run and all blockers are collected. This is the behavior API consumers will need: present the full set of issues in a single response instead of requiring fix-one-recheck cycles. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! k$ prove t/db_dependent/Circul* => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- 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=41728 --- Comment #59 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197415 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197415&action=edit Bug 41728: (follow-up) Add no_short_circuit option to checkin availability This follow-up replaces the unconditional removal of short-circuit logic with a caller-controlled no_short_circuit parameter, following the same pattern established by Koha::Patron->can_place_holds. By default, check() short-circuits on the first blocker. This is the correct behavior for AddReturn, which processes blockers sequentially and only acts on the first one it finds (BlockedWithdrawn => early return, Wrongbranch => early return, BlockedLost => sets doreturn=0). Running all policy checks only to discard the results is wasteful, specially when some of them (e.g. can_be_returned_at) hit the database. When no_short_circuit => 1 is passed, all checks run and all blockers are collected. This is the behavior API consumers will need: present the full set of issues in a single response instead of requiring fix-one-recheck cycles. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! k$ prove t/db_dependent/Circul* => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> 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=41728 --- Comment #61 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197685 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197685&action=edit Bug 41728: (QA follow-up) Move Item::Checkin::Availability to Availability::Checkin Co-locate the check-in availability checker with Koha::Result::Availability under a unified Koha::Availability::* namespace. This sets up a consistent home for future parallel checkers (Checkout, Hold, Renewal) rather than scattering them as single-file directories under Koha::Item::*. The $item->checkin_availability convenience method on Koha::Item is preserved; only its internal delegation target changes. Renames: Koha::Item::Checkin::Availability -> Koha::Availability::Checkin Koha/Item/Checkin/Availability.pm -> Koha/Availability/Checkin.pm t/db_dependent/Koha/Item/Checkin/Availability.t -> t/db_dependent/Koha/Availability/Checkin.t Test plan: prove t/db_dependent/Koha/Availability/Checkin.t prove t/db_dependent/Circulation/Returns.t 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=41728 --- Comment #62 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197690 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197690&action=edit Bug 41728: (QA follow-up) Move Item::Checkin::Availability to Item::Availability::Checkin Introduce a dedicated Koha::Item::Availability::* namespace for checkers that return a Koha::Result::Availability. Keeping these under the Item subject prefix keeps them co-located with the Item they describe, while the Availability sub-namespace gives future parallel checkers (Checkout, Hold, Book, ...) an obvious home rather than scattering them as single-file directories alongside Koha/Item/Transfer/ etc. The $item->checkin_availability convenience method on Koha::Item is preserved; only its internal delegation target changes. Renames: Koha::Item::Checkin::Availability -> Koha::Item::Availability::Checkin Koha/Item/Checkin/Availability.pm -> Koha/Item/Availability/Checkin.pm t/db_dependent/Koha/Item/Checkin/Availability.t -> t/db_dependent/Koha/Item/Availability/Checkin.t Test plan: prove t/db_dependent/Koha/Item/Availability/Checkin.t prove t/db_dependent/Circulation/Returns.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42418 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add |Add |`Koha::Item::Checkin::Avail |`Koha::Item::Availability:: |ability` to centralize |Checkin` to centralize |logic |logic Keywords| |release-notes-needed CC| |lisette@bywatersolutions.co | |m Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 [Bug 42418] Add Koha::Checkin(s) - checkin as a first-class resource -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197690|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=41728 --- Comment #66 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197821 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197821&action=edit Bug 41728: (QA follow-up) Move Item::Checkin::Availability to Item::Availability::Checkin Introduce a dedicated Koha::Item::Availability::* namespace for checkers that return a Koha::Result::Availability. Keeping these under the Item subject prefix keeps them co-located with the Item they describe, while the Availability sub-namespace gives future parallel checkers (Checkout, Hold, Book, ...) an obvious home rather than scattering them as single-file directories alongside Koha/Item/Transfer/ etc. The $item->checkin_availability convenience method on Koha::Item is preserved; only its internal delegation target changes. Renames: Koha::Item::Checkin::Availability -> Koha::Item::Availability::Checkin Koha/Item/Checkin/Availability.pm -> Koha/Item/Availability/Checkin.pm t/db_dependent/Koha/Item/Checkin/Availability.t -> t/db_dependent/Koha/Item/Availability/Checkin.t Test plan: prove t/db_dependent/Koha/Item/Availability/Checkin.t prove t/db_dependent/Circulation/Returns.t Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #68 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi, Jonathan! (In reply to Jonathan Druart from comment #64)

+sub checkin_availability { + my ( $self, $params ) = @_; + + require Koha::Item::Availability::Checkin; + return Koha::Item::Availability::Checkin->check( $self, $params ); +}

We should not require here.

Good catch. Solved! Thanks! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197400|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197402|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197404|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197406|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197408|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197410|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197412|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197415|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=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197821|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=41728 --- Comment #69 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198176 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198176&action=edit Bug 41728: Add Koha::Availability::Result base class This patch introduces a standardized result class for availability checks, providing a clean API with proper methods instead of raw hashrefs. The Koha::Availability::Result class provides: - add_blocker() - Add conditions that prevent the action - add_confirmation() - Add conditions requiring user confirmation - add_warning() - Add informational messages - set_context() - Store related objects (item, checkout, patron, etc.) - available() - Check if action can proceed (no blockers) - needs_confirmation() - Check if confirmations are required - Accessor methods for all categories - to_hashref() for backward compatibility This design: - Provides a consistent interface across all availability checks - Makes the API more discoverable and self-documenting - Enables future enhancements without breaking changes - Follows OO principles with proper encapsulation - Can be used as base for checkout, hold, renewal availability Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/Availability/Result.t => SUCCESS: Tests pass! 3. Verify all methods work correctly 4. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #71 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198178 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198178&action=edit Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability This patch refactors AddReturn to use the new Koha::Item::Checkin::Availability class for validation logic, eliminating code duplication and centralizing check-in validation. Changes: - Add use statement for Koha::Item::Checkin::Availability - Look up item first, return BadBarcode if not found - Call $item->checkin_availability() for validation - Use Result object methods (available(), blockers, context) - Replace inline withdrawn/lost/branch validation with blocker handling - Remove duplicate CanBookBeReturned() call (now in Availability class) - Preserve all existing behavior and error messages - Maintain backward compatibility The refactoring extracts ~40 lines of validation logic while maintaining identical functionality. All validation results are obtained from a single availability check that returns a Result object. Note: The withdrawn message is set in two places depending on context: - When BlockReturnOfWithdrawnItems is ON: set in blocker handler before early return - When BlockReturnOfWithdrawnItems is OFF: set by existing check later in the flow This preserves the original behavior where withdrawn items always get the message, but are only blocked when the syspref is enabled. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! k$ prove t/db_dependent/Circul* => SUCCESS: Tests pass! 3. Verify check-in operations work correctly: - Items not found return BadBarcode - Withdrawn items blocked when syspref enabled - Lost items blocked when syspref enabled - Wrong branch returns blocked per AllowReturnToBranch - Not issued items handled correctly - Normal check-ins complete successfully 4. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #73 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198180 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198180&action=edit Bug 41728: (QA follow-up) Populate checkout context before blocker checks The availability check is returning early on blockers (BlockedWithdrawn, BlockedLost, Wrongbranch) before checking if the item had an active checkout. This causes AddReturn to lose checkout/patron context for checked-out items that hit a blocker, resulting in incorrect NotIssued messages, unintended onloan clearing, and incorrect local use recording. Move the checkout check to run first so context is always populated regardless of blockers. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #75 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198182 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198182&action=edit Bug 41728: (QA follow-up) Fix test accessing wrong key for checkout The test was checking $result->{issue} which doesn't exist on the Result object. The correct accessor is $result->context->{checkout}, so the assertion was always trivially true. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #77 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198184 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198184&action=edit Bug 41728: (QA follow-up) Move data consistency check before blocker handling The data inconsistency die (checkout exists but patron does not) was placed after the blocker handling block. This meant that if an item had corrupt data AND triggered a blocker (e.g. withdrawn+blocked), the blocker handler would return early with patron as undef instead of dying on the inconsistency, silently masking database corruption. Move the check to immediately after extracting context from the availability result, before any blocker handling, matching the original AddReturn behavior where the die fired during checkout lookup. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #79 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198186 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198186&action=edit Bug 41728: (QA follow-up) Move return policy logic to Koha::Item Move the AllowReturnToBranch and branch transfer limit checks from both C4::Circulation::CanBookBeReturned and Koha::Item::Checkin::Availability::_check_return_policy into a single authoritative method on Koha::Item: can_be_returned_at(). This eliminates the duplicate implementations and follows the Koha convention of placing item-related logic on the Koha::Item object. Changes: - Add Koha::Item->can_be_returned_at() with AllowReturnToBranch policy and transfer limit checks - Make CanBookBeReturned a thin wrapper delegating to the new method - Update Checkin::Availability to call $item->can_be_returned_at() - Remove _check_return_policy and unused withdrawn warning from the Availability class Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #81 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198188 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198188&action=edit Bug 41728: (QA follow-up) Collect all blockers instead of short-circuiting Remove early returns from Koha::Item::Checkin::Availability::check() so that all blockers, confirmations, and warnings are collected in a single pass. Previously, the first blocker triggered an early return, meaning callers could only discover one problem at a time. This is particularly useful for API consumers who want to present the full set of issues to the user in a single response rather than requiring fix-one-recheck cycles. The change is safe for AddReturn because it already handles blocker priority through its sequential if/return structure: 1. BlockedWithdrawn → early return before status updates 2. Wrongbranch → early return after status updates 3. BlockedLost → sets doreturn=0 Even when multiple blockers are present, AddReturn processes them in the correct priority order. Also makes existing tests deterministic by explicitly setting withdrawn and itemlost values on test items, preventing random TestBuilder values from triggering unrelated blockers now that all checks always run. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #83 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198190 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198190&action=edit Bug 41728: (QA follow-up) Move Availability::Result to Result::Availability Consolidate result base classes under the Koha::Result::* namespace alongside the existing Koha::Result::Boolean. This keeps return-value wrappers discoverable in one place and avoids a lone Koha::Availability:: namespace that would otherwise host a single base class. Renames: Koha::Availability::Result -> Koha::Result::Availability Koha/Availability/Result.pm -> Koha/Result/Availability.pm t/Koha/Availability/Result.t -> t/Koha/Result/Availability.t Call sites in Koha::Item, Koha::Item::Checkin::Availability, and the test files are updated accordingly. Test plan: prove t/Koha/Result/Availability.t prove t/db_dependent/Koha/Item/Checkin/Availability.t Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #85 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198192 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198192&action=edit Bug 41728: (follow-up) Replace require with use for Availability::Checkin There is no circular dependency, so the runtime require can be replaced with a compile-time use statement. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Patch doesn't apply CC| |david@davidnind.com --- Comment #86 from David Nind <david@davidnind.com> --- Testing another bug where this one is a dependency, this bug no longer applies 8-(.. Apply? [(y)es, (n)o, (i)nteractive] y Preparing 17 patch(es): ✓ Prepared 17 patch(es) Applying 17 patch(es): ✓ [1/17] Applied Bug 41728: Add Koha::Availability::Result base class ✓ [2/17] Applied Bug 41728: Add Koha::Item::Checkin::Availability class Patch application failed for attachment 198178 - Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42386 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42386 [Bug 42386] Unify hold availability checks into Koha::Availability pattern -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198176|0 |1 is obsolete| | Attachment #198177|0 |1 is obsolete| | Attachment #198178|0 |1 is obsolete| | Attachment #198179|0 |1 is obsolete| | Attachment #198180|0 |1 is obsolete| | Attachment #198181|0 |1 is obsolete| | Attachment #198182|0 |1 is obsolete| | Attachment #198183|0 |1 is obsolete| | Attachment #198184|0 |1 is obsolete| | Attachment #198185|0 |1 is obsolete| | Attachment #198186|0 |1 is obsolete| | Attachment #198187|0 |1 is obsolete| | Attachment #198188|0 |1 is obsolete| | Attachment #198189|0 |1 is obsolete| | Attachment #198190|0 |1 is obsolete| | Attachment #198191|0 |1 is obsolete| | Attachment #198192|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=41728 --- Comment #88 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200322&action=edit Bug 41728: Add Koha::Item::Checkin::Availability class Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #90 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200324 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200324&action=edit Bug 41728: (follow-up) Add transfer limit check and clean param names - Rename params: branch => library, add to_library for destination - Move $returnbranch computation before the availability check in AddReturn so it can be passed as to_library - Add transfer limit check (bug 7376) to the Availability class - Rename internal _can_book_be_returned to _check_return_policy - Remove duplicate $returnbranch block and standalone transfer limit check from AddReturn - Update tests for new param names Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #92 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200326&action=edit Bug 41728: (QA follow-up) Match original blocker priority order Reorder blocker checks to match the original AddReturn priority: Withdrawn > Wrongbranch > Lost. Previously Lost was checked before Wrongbranch, which changed behavior for items that were both lost and at the wrong branch. QA note: There are other checks in-between these, but I don't think their order betwixt these blockers matters for retaining existing behavior. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #94 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200328 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200328&action=edit Bug 41728: (QA follow-up) Declare issue and patron variables together There seems to be no reason to declare $patron near the start of the sub while $issue is declared where they are both populated. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #96 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200330 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200330&action=edit Bug 41728: (QA follow-up) Restore original execution order for Wrongbranch The refactoring changed the relative order of the Wrongbranch early return and several status-update operations. In the original AddReturn, the execution order was: 1. checkout/patron lookup 2. not-issued handling + RecordLocalUseOnReturn 3. withdrawn check (early return) 4. location_update_trigger 5. UpdateNotForLoanStatusOnCheckin 6. CanBookBeReturned / Wrongbranch check (early return) 7. BlockReturnOfLostItems After the refactoring, the Wrongbranch blocker returned early before steps 2, 4 and 5, causing three regressions: - RecordLocalUseOnReturn was not recorded for non-issued items scanned at the wrong branch - location_update_trigger was skipped for Wrongbranch returns - UpdateNotForLoanStatusOnCheckin was skipped for Wrongbranch returns Fix by splitting blocker handling into two phases: 1. BlockedWithdrawn: earliest early return, before status updates (matches original withdrawn check position) 2. Wrongbranch and BlockedLost: after RecordLocalUseOnReturn, location_update_trigger, and UpdateNotForLoanStatusOnCheckin (matches original CanBookBeReturned position) Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #98 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200332 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200332&action=edit Bug 41728: (QA follow-up) Restore withdrawn warning and use from availability Restore the withdrawn warning in Koha::Item::Checkin::Availability that was incorrectly removed. This warning is set when an item is withdrawn but BlockReturnOfWithdrawnItems is off, making it useful for API consumers in subsequent work. Update AddReturn to read the withdrawn state from the availability result's warnings rather than re-checking $item->withdrawn directly, avoiding duplication of logic. Add test for the withdrawn warning (non-blocked) path. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #100 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200334 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200334&action=edit Bug 41728: (follow-up) Add no_short_circuit option to checkin availability This follow-up replaces the unconditional removal of short-circuit logic with a caller-controlled no_short_circuit parameter, following the same pattern established by Koha::Patron->can_place_holds. By default, check() short-circuits on the first blocker. This is the correct behavior for AddReturn, which processes blockers sequentially and only acts on the first one it finds (BlockedWithdrawn => early return, Wrongbranch => early return, BlockedLost => sets doreturn=0). Running all policy checks only to discard the results is wasteful, specially when some of them (e.g. can_be_returned_at) hit the database. When no_short_circuit => 1 is passed, all checks run and all blockers are collected. This is the behavior API consumers will need: present the full set of issues in a single response instead of requiring fix-one-recheck cycles. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t => SUCCESS: Tests pass! k$ prove t/db_dependent/Circul* => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> 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=41728 --- Comment #102 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200336 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200336&action=edit Bug 41728: (QA follow-up) Move Item::Checkin::Availability to Item::Availability::Checkin Introduce a dedicated Koha::Item::Availability::* namespace for checkers that return a Koha::Result::Availability. Keeping these under the Item subject prefix keeps them co-located with the Item they describe, while the Availability sub-namespace gives future parallel checkers (Checkout, Hold, Book, ...) an obvious home rather than scattering them as single-file directories alongside Koha/Item/Transfer/ etc. The $item->checkin_availability convenience method on Koha::Item is preserved; only its internal delegation target changes. Renames: Koha::Item::Checkin::Availability -> Koha::Item::Availability::Checkin Koha/Item/Checkin/Availability.pm -> Koha/Item/Availability/Checkin.pm t/db_dependent/Koha/Item/Checkin/Availability.t -> t/db_dependent/Koha/Item/Availability/Checkin.t Test plan: prove t/db_dependent/Koha/Item/Availability/Checkin.t prove t/db_dependent/Circulation/Returns.t Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.