[Bug 41728] New: Add `Koha::Item::Checkin::Availability` to centralize logic
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Bug ID: 41728 Summary: Add `Koha::Item::Checkin::Availability` to centralize logic Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org I propose we move the checks from `C4::Circulation::AddReturn` into a proper class for reusing it in other places, specially on the API controllers. -- 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=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 ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, | |martin.renvoize@openfifth.c | |o.uk, | |nick@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 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 #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192130 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192130&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 #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 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192132 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192132&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 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 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192133&action=edit [POC]Â Bug 41728: Add Koha::Item::Checkout::Availability class This is for illustration purposes only! Not intended to be tested or pushed. This patch introduces a checkout availability class that wraps C4::Circulation::CanBookBeIssued in a Result object interface, providing a consistent API for availability checks. The class: - Wraps CanBookBeIssued and maps its return values to Result object - Maps 'impossible' to blockers - Maps 'confirmation' to confirmations - Maps 'alerts' and 'messages' to warnings - Provides $item->checkout_availability() convenience method This enables the REST API and other code to use a consistent availability checking interface across different operations (checkout, check-in, renewal, etc.). Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item/Checkout/Availability.t => SUCCESS: Tests pass! 3. Verify checkout availability checks work: - Available items return no blockers - Debarred patrons get DEBARRED blocker - Context includes patron and item 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 ---------------------------------------------------------------------------- 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 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- 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 #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. It felt odd to mix things. The return value is a hint: the policy checks validate a rule, while the availability ones orchestrate many rules and return a complex result. 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 ``` but I still feel having this clearly be item-centric is a plus. If we wanted to replace (say) `CanBookBeReserved` we could do something like: ```perl Koha::Biblio::Hold::Availability ``` which would internally instantiate the relevant `Koha::Policy::*` classes to check things. I'm open to ideas about this OFC. -- 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 Jan Kissig <bibliothek@th-wildau.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bibliothek@th-wildau.de -- 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 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I really like the direction this is going.. as soon as I can get it to apply I will do a more formal review.. but honestly looking at the patches here it's going in exactly the direction I was envisaging :) -- 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 #192131|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 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192133|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 #11 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 196135 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196135&action=edit Bug 41728: Add Koha::Item::Checkin::Availability class Patch from commit 629d5fd -- 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 #13 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 196137 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196137&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 -- 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 ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- 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 #196135|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 #196137|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 #15 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196888 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196888&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> -- 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 #17 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196890 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196890&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> -- 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 #19 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196892 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196892&action=edit Bug 41728: (follow-up) Add POD for _add_statistic 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 #21 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196894 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196894&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> -- 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 --- Comment #23 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 196896 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196896&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> -- 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 #24 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197346 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197346&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> -- 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 #26 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197348 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197348&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> -- 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 #28 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197350 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197350&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> -- 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 #30 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197352&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> -- 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 #32 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197354 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197354&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> -- 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 #34 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197356 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197356&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) -- 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 #36 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197358 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197358&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. -- 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 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.
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 #39 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Martin Renvoize (ashimema) from comment #37)
Created attachment 197359 [details] [review] 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.
I like this but... As part of the endpoint work my plan was to introduce a 'short_circuit' boolean flag to this method. The idea is that the current implementation in C4::Circulation really short-circuits, and we would like to keep the current behavior until the UI is changed. -- 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 --- Comment #41 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Martin Renvoize (ashimema) from comment #40)
Cool.. we could probably just drop that final follow-up then.. or move it to the next bug?
Let's keep it. I'll redo my work with this. It will be easier :-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 ---------------------------------------------------------------------------- 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 #197347|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 #197349|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 #197351|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 #197353|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 #197355|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 #197357|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 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197359|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 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197399|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 #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 #44 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197401 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197401&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 #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 #46 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197403 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197403&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 #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 #48 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197405 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197405&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 #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 #50 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197407 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197407&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 #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 #52 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197409&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 #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 #54 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197411 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197411&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 #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 #56 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197413&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 #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 --- Comment #58 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi, I really value the feedback and follow-ups from two knowledgeable folks in the area. I like that my misses got corrected (changing validation order is not always safe). I prefer to follow the short-circuit-by-default pattern to avoid unneeded checks even if it is safe to do it. We are already facing problems at scale so worth keeping the behavior for now. Hence my follow-up. I'll move forward with the API now this is stabilized. Thanks! -- 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 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197414|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 #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 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42385 Blocks| |42386 Blocks| |42388 Blocks| |42389 --- Comment #60 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197684 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197684&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> Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42385 [Bug 42385] Extract _GetCircControlBranch into Koha::Policy::Circulation https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42386 [Bug 42386] Unify hold availability checks into Koha::Availability pattern https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42388 [Bug 42388] Extract patron charge limit resolution into Koha::Policy::Patrons::ChargeLimits https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42389 [Bug 42389] Refactor Koha::Patron->can_checkout 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 --- 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 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197685|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 #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 --- Comment #63 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Tomas and I discussed this again.. we agreed on a clearer naming scheme which we think is clearer and more future proof. -- 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 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #64 from Jonathan Druart <jonathan.druart@gmail.com> --- +sub checkin_availability { + my ( $self, $params ) = @_; + + require Koha::Item::Availability::Checkin; + return Koha::Item::Availability::Checkin->check( $self, $params ); +} We should not require here. -- 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 #197684|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 #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 #65 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197820 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197820&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 #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 #67 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197822 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197822&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 --- 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 ---------------------------------------------------------------------------- Keywords|release-notes-needed | Text to go in the| |This development refactors release notes| |and modernizes the code | |responsible for checking if | |an item can be checked in. | |All the changes are covered | |by tests. | | | |The ultimate goal is to | |implement an API endpoint | |for a confirmation flow for | |checking items in. | | | |It proposes a pattern to | |follow on the needed | |rewrite of the circulation | |core functionality that | |will provide a coherent | |return value and behavior. | |Areas like checking out or | |the ability to place holds | |are the next targets for | |implementing this 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 #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 #197401|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 #197403|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 #197405|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 #197407|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 #197409|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 #197411|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 #197413|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 #197820|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 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197822|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 #70 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198177 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198177&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 #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 #72 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198179 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198179&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 #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 #74 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198181 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198181&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 #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 #76 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198183 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198183&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 #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 #78 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198185 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198185&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 #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 #80 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198187 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198187&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 #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 #82 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198189 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198189&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 #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 #84 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198191 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198191&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 #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 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|24401 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24401 [Bug 24401] Add POST /checkins endpoint with availability check and confirmation flow -- 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|42389 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42389 [Bug 42389] Refactor Koha::Patron->can_checkout 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 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 ---------------------------------------------------------------------------- Status|Patch doesn't apply |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 #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 #87 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200321 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200321&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 #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 #89 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200323 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200323&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 #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 #91 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200325 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200325&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 #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 #93 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200327 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200327&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 #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 #95 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200329 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200329&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 #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 #97 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200331&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 #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 #99 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200333 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200333&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 #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 #101 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200335 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200335&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 #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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #103 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200337 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200337&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 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=43108 -- 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| |43124 Blocks|43124 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43124 [Bug 43124] Add Koha::Item::Availability::Hold and Koha::Biblio::Availability::Hold -- 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| |43108 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43108 [Bug 43108] Add a REST API endpoint for a biblio's booking availability over a date range -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 --- Comment #104 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Created attachment 204865 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204865&action=edit Bug 41728: Add AddReturn marks ISO18626 request as LoanCompleted prove ./t/db_dependent/Circulation.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k Status|Passed QA |Failed QA --- Comment #105 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- The patch 'Move data consistency check before blocker handling' is meant to move the check but duplicated it. This causes a regression, apply the latest test patch, run: prove ./t/db_dependent/Circulation.t -- 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|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 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200321|0 |1 is obsolete| | Attachment #200322|0 |1 is obsolete| | Attachment #200323|0 |1 is obsolete| | Attachment #200324|0 |1 is obsolete| | Attachment #200325|0 |1 is obsolete| | Attachment #200326|0 |1 is obsolete| | Attachment #200327|0 |1 is obsolete| | Attachment #200328|0 |1 is obsolete| | Attachment #200329|0 |1 is obsolete| | Attachment #200330|0 |1 is obsolete| | Attachment #200331|0 |1 is obsolete| | Attachment #200332|0 |1 is obsolete| | Attachment #200333|0 |1 is obsolete| | Attachment #200334|0 |1 is obsolete| | Attachment #200335|0 |1 is obsolete| | Attachment #200336|0 |1 is obsolete| | Attachment #200337|0 |1 is obsolete| | Attachment #204865|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 #106 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204882 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204882&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> 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 #107 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204883&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> 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 #108 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204884 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204884&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> 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 #109 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204885 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204885&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> 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 #110 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204886 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204886&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> 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 #111 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204887&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> 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 #112 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204888 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204888&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> 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 #113 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204889 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204889&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> 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 #114 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204890 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204890&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> 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 #115 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204891 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204891&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> 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 #116 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204892 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204892&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> 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 #117 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204893 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204893&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> 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 #118 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204894 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204894&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> 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 #119 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204895 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204895&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 #120 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204896 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204896&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> 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 #121 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204897 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204897&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> 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 #122 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204898 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204898&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> 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 #123 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204899 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204899&action=edit Bug 41728: (QA follow-up) Remove leftover duplicate data-consistency block d43945e7279 moved the issued/patron data-consistency check earlier in AddReturn (before blocker handling), as its commit message describes, but only copied the block instead of deleting the original. The leftover block ran unconditionally afterwards: since $patron is already guaranteed set whenever $issue is true by that point, its "else" branch fired on every normal checkin of an already-issued item, incorrectly setting $messages->{NotIssued} and clearing item.onloan. This corrupted the not_issued message surfaced to circ/returns.pl, SIP, and the checkin audit record on every ordinary return. The same leftover block also buried $iso18626_request's assignment after a die (making it dead code), so ISO18626 progress-reporting on checkin was never triggered. Restore the assignment at the point $issue/$patron are extracted from the availability context, where it belongs. Test plan: 1. Return an item that is currently checked out. 2. Confirm the returned $messages hashref no longer contains a NotIssued key, and that item.onloan is only cleared for items that were genuinely not checked out. 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 #124 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204900 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204900&action=edit Bug 41728: (QA follow-up) Add coverage for the Wrongbranch blocker No test in the suite asserted on $messages->{Wrongbranch} content despite this being one of the blockers AddReturn's rewired availability check produces (AllowReturnToBranch enforcement, moved into Koha::Item->can_be_returned_at / Koha::Item::Availability::Checkin by this bug). Add coverage for it. Test plan: 1. prove t/db_dependent/Circulation.t => SUCCESS: 88 tests pass. 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 #125 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204901 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204901&action=edit Bug 41728: Add AddReturn marks ISO18626 request as LoanCompleted Regression test from the RM's QA review (attachment 204865, comment 104/105 on the bug): drives a real ISO18626 request through AddReturn and confirms it gets marked LoanCompleted and receives a supplyingAgencyMessage, rather than only checking that the $messages hashref no longer contains a stray NotIssued key. This closes the gap the "Remove leftover duplicate data-consistency block" follow-up left - that fix restored the $iso18626_request assignment, but nothing previously exercised the ISO18626 progress-reporting path end to end. Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk> Test plan: 1. prove t/db_dependent/Circulation.t => SUCCESS: 89 tests pass. 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 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=41728 --- Comment #126 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=41728 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |26.11 Initiative type|--- |Feature -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org