Bugzilla – Attachment 192132 Details for
Bug 41728
Add `Koha::Item::Checkin::Availability` to centralize logic
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability
8215d69.patch (text/plain), 7.42 KB, created by
Tomás Cohen Arazi (tcohen)
on 2026-01-28 19:12:32 UTC
(
hide
)
Description:
Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2026-01-28 19:12:32 UTC
Size:
7.42 KB
patch
obsolete
>From 8215d69d657def8275cddaabb644ec0cfbc374ab Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= <tomascohen@theke.io> >Date: Wed, 28 Jan 2026 15:34:27 -0300 >Subject: [PATCH] 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 >--- > C4/Circulation.pm | 85 ++++++++++++++++++++++++++++------------------- > 1 file changed, 51 insertions(+), 34 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 1e025cc56a..71bf15cde4 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -2265,31 +2265,70 @@ sub AddReturn { > my $validTransfer = 1; > my $stat_type = 'return'; > >- # get information on item >+ # Get item > my $item = Koha::Items->find( { barcode => $barcode } ); > unless ($item) { > return ( 0, { BadBarcode => $barcode } ); # no barcode means no item or borrower. bail out. > } > >+ # Check availability >+ my $availability = $item->checkin_availability( { branch => $branch } ); >+ >+ # Extract context objects >+ my $issue = $availability->context->{checkout}; >+ $patron = $availability->context->{patron}; >+ >+ # Set NotIssued message if item not checked out >+ if ( !$issue ) { >+ $messages->{'NotIssued'} = $barcode; >+ $item->onloan(undef)->store( { skip_record_index => 1, skip_holds_queue => 1 } ) if defined $item->onloan; >+ $doreturn = 0; >+ } >+ >+ # Handle blockers >+ if ( !$availability->available ) { >+ my $blockers = $availability->blockers; >+ >+ if ( $blockers->{BlockedWithdrawn} ) { >+ $messages->{'withdrawn'} = 1; >+ >+ # Record local use even when blocked, if preference is on >+ if ( !$issue && C4::Context->preference("RecordLocalUseOnReturn") ) { >+ my $localuse_count = $item->localuse || 0; >+ $localuse_count++; >+ $item->localuse($localuse_count)->store; >+ $messages->{'LocalUse'} = 1; >+ } >+ >+ return ( 0, $messages, $issue, ( $patron ? $patron->unblessed : {} ) ); >+ } >+ if ( $blockers->{BlockedLost} ) { >+ $doreturn = 0; >+ } >+ if ( $blockers->{Wrongbranch} ) { >+ $messages->{'Wrongbranch'} = $blockers->{Wrongbranch}; >+ $doreturn = 0; >+ my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); >+ $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" ); >+ return ( $doreturn, $messages, $issue, ( $patron ? $patron->unblessed : {} ) ); >+ } >+ } >+ > my $itemnumber = $item->itemnumber; > my $itemtype = $item->effective_itemtype; > my $localuse_count = $item->localuse || 0; > >- my $issue = $item->checkout; >- if ($issue) { >- $patron = $issue->patron >- or die >+ # Data consistency check for issued items >+ if ( $issue && !$patron ) { >+ die > "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existent borrowernumber '" > . $issue->borrowernumber . "'\n" > . Dumper( $issue->unblessed ) . "\n"; >- } else { >- $messages->{'NotIssued'} = $barcode; >- $item->onloan(undef)->store( { skip_record_index => 1, skip_holds_queue => 1 } ) if defined $item->onloan; >+ } > >- # even though item is not on loan, it may still be transferred; therefore, get current branch info >- $doreturn = 0; >+ # Handle not issued items (continued) >+ if ( !$issue ) { > >- # No issue, no borrowernumber. ONLY if $doreturn, *might* you have a $borrower later. > # Record this as a local use, instead of a return, if the RecordLocalUseOnReturn is on > if ( C4::Context->preference("RecordLocalUseOnReturn") ) { > $localuse_count++; >@@ -2301,11 +2340,6 @@ sub AddReturn { > > if ( $item->withdrawn ) { # book has been cancelled > $messages->{'withdrawn'} = 1; >- >- # In the case where we block return of withdrawn, we should completely block the return >- # without updating item statuses, so we exit early >- return ( 0, $messages, $issue, ( $patron ? $patron->unblessed : {} ) ) >- if C4::Context->preference("BlockReturnOfWithdrawnItems"); > } > > # full item data, but no borrowernumber or checkout info (no issue) >@@ -2367,23 +2401,6 @@ sub AddReturn { > } > } > >- # check if the return is allowed at this branch >- my ( $returnallowed, $message ) = CanBookBeReturned( $item->unblessed, $branch ); >- unless ($returnallowed) { >- $messages->{'Wrongbranch'} = { >- Wrongbranch => $branch, >- Rightbranch => $message >- }; >- $doreturn = 0; >- my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); >- $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" ); >- return ( $doreturn, $messages, $issue, $patron_unblessed ); >- } >- >- if ( $item->itemlost and C4::Context->preference("BlockReturnOfLostItems") ) { >- $doreturn = 0; >- } >- > # case of a return of document (deal with issues and holdingbranch) > if ($doreturn) { > die "The item is not issed and cannot be returned" unless $issue; # Just in case... >@@ -3307,7 +3324,7 @@ sub CanBookBeRenewed { > unless CanItemBeReserved( > $patron_with_reserve, $other_item, undef, > { ignore_hold_counts => 1 } >- )->{status} eq 'OK'; >+ )->{status} eq 'OK'; > > # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' > # CanItemBeReserved checks 'rules' and 'policies' which means >-- >2.50.1 (Apple Git-155) >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 41728
:
192130
|
192131
| 192132 |
192133