From 4b277a6e4a2c3b44a30217ca262d9a8de7f13433 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Wed, 28 Jan 2026 15:34:27 -0300 Subject: [PATCH] Bug 41728: Make AddReturn use Koha::Item::Checkin::Availability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Martin Renvoize Signed-off-by: Tomás Cohen Arazi Signed-off-by: Martin Renvoize Signed-off-by: Pedro Amorim --- C4/Circulation.pm | 97 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 20359da039..811228d948 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2395,20 +2395,62 @@ 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"; @@ -2416,11 +2458,11 @@ sub AddReturn { } 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++; @@ -2432,11 +2474,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) @@ -2479,6 +2516,22 @@ sub AddReturn { my $borrowernumber = $patron ? $patron->borrowernumber : undef; # we don't know if we had a borrower or not my $patron_unblessed = $patron ? $patron->unblessed : {}; + # Check branch transfer limits (bug 7376) + if ( $doreturn && defined($returnbranch) ) { + my $from_library = Koha::Libraries->find($branch); + my $to_library = Koha::Libraries->find($returnbranch); + if ( !$item->can_be_transferred( { from => $from_library, to => $to_library } ) ) { + $messages->{'Wrongbranch'} = { + Wrongbranch => $branch, + Rightbranch => $returnbranch + }; + $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 ); + } + } + # Update item location my $loc_messages = $item->location_update_trigger('checkin'); foreach my $loc_msg_key ( keys %$loc_messages ) { @@ -2512,24 +2565,6 @@ sub AddReturn { } } - # check if the return is allowed at this branch - my ( $returnallowed, $message ) = CanBookBeReturned( $item, $branch, $returnbranch ); - - unless ($returnallowed) { - $messages->{'Wrongbranch'} = { - Wrongbranch => $branch, - Rightbranch => $message - } if $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...