From 8215d69d657def8275cddaabb644ec0cfbc374ab 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 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)