From b68c02facf6026348cedee34227bb44f9e77c5fc Mon Sep 17 00:00:00 2001 From: Laura_Escamilla Date: Wed, 5 Nov 2025 19:08:04 +0000 Subject: [PATCH] Bug 35612: Recorded branch context for OVERDUE, LOST, and PROCESSING fees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch ensures that the accountlines.branchcode field correctly records which branch's circulation rules were applied when creating OVERDUE, LOST, and PROCESSING fee debits. Previously, this field was often NULL or set to the logged-in user's branch, making it difficult to track which branch's policies governed the fee calculation. Changes made: 1. New helper method: Koha::Checkout->branch_for_fee_context() - Centralizes branch resolution logic based on system preferences - For LOST/PROCESSING: honors LostChargesControl preference - For OVERDUE: honors CircControl preference - Respects HomeOrHoldingBranch when choosing item's branch - Supports both Koha objects and plain hashrefs - Returns branchcode or undef if context unavailable 2. C4::Accounts::chargelostitem() - Now accepts optional $opts hashref parameter - Accepts interface, library_id, and issue_id in options - Determines appropriate branch using branch_for_fee_context - Passes resolved branch to add_debit as library_id - Issue_id lookup handles bundled items correctly 3. C4::Circulation::LostItem() - Calls branch_for_fee_context before chargelostitem - Passes interface context ('cron' for cronjob, otherwise current) - Passes resolved branch and issue_id to chargelostitem 4. C4::Overdues::UpdateFine() - Calls branch_for_fee_context to determine OVERDUE branch - Passes result as library_id to add_debit - Resolves long-standing FIXME about branch selection 5. misc/cronjobs/longoverdue.pl - Determines branch context before marking items lost - Passes library_id to LostItem when charging fees The branch resolution logic respects these system preferences: - CircControl: PatronLibrary | PickupLibrary | ItemHomeLibrary - LostChargesControl: PatronLibrary | PickupLibrary | ItemHomeLibrary - HomeOrHoldingBranch: homebranch | holdingbranch Test plan: 1. Create three test libraries (e.g., FPL, CPL, PVL) 2. Create a test patron with home library PVL 3. Create an item with: - homebranch = FPL - holdingbranch = CPL 4. Set system preferences: - FinesMode: Calculate and charge - Create circulation rule that charges overdue fines 5. Test OVERDUE fees with CircControl: a. Set CircControl = PatronLibrary, HomeOrHoldingBranch = homebranch b. Check out item to patron at CPL, backdate to be overdue c. Run: perl /usr/share/koha/bin/cronjobs/fines.pl d. Check accountlines: SELECT accountlines_id, branchcode, debit_type_code FROM accountlines ORDER BY accountlines_id DESC LIMIT 5; e. Verify branchcode = PVL (patron's home library) f. Delete the fine, repeat with CircControl = PickupLibrary g. Verify branchcode = CPL (checkout library) h. Delete the fine, repeat with CircControl = ItemHomeLibrary i. Verify branchcode = FPL (item's home library) j. Change HomeOrHoldingBranch = holdingbranch, repeat k. Verify branchcode = CPL (item's holding library) 6. Test LOST fees with LostChargesControl: a. Set WhenLostChargeReplacementFee = Charge b. Set item replacement price > 0 c. Ensure item is checked out to patron d. Set LostChargesControl = PatronLibrary e. Mark item as lost in staff interface f. Check accountlines for LOST debit g. Verify branchcode = PVL (patron's home library) h. Return item, re-checkout, repeat with LostChargesControl = PickupLibrary i. Verify branchcode = CPL (checkout library) j. Repeat with LostChargesControl = ItemHomeLibrary + homebranch k. Verify branchcode = FPL (item's home) l. Repeat with LostChargesControl = ItemHomeLibrary + holdingbranch m. Verify branchcode = CPL (item's holding) 7. Test longoverdue cronjob: a. Re-checkout item with due date triggering longoverdue b. Run: perl /usr/share/koha/bin/cronjobs/longoverdue.pl --lost 30=1 --charge 1 --confirm --verbose c. Verify LOST debit created with correct branchcode per LostChargesControl 8. Test edge cases: - WhenLostChargeReplacementFee = Don't charge → no LOST debit - Replacement price = 0 → no LOST debit - FinesMode = Don't calculate → no OVERDUE debits Signed-off-by: Trevor Diamond Signed-off-by: Martin Renvoize --- C4/Accounts.pm | 60 +++++++++++++++++++++--------------- C4/Circulation.pm | 47 ++++++++++++++++++++++------ C4/Overdues.pm | 14 +++++++-- Koha/Checkout.pm | 47 ++++++++++++++++++++++++++++ misc/cronjobs/longoverdue.pl | 21 ++++++++++++- 5 files changed, 152 insertions(+), 37 deletions(-) diff --git a/C4/Accounts.pm b/C4/Accounts.pm index a1613882ad3..5f3f06ee0f4 100644 --- a/C4/Accounts.pm +++ b/C4/Accounts.pm @@ -33,7 +33,10 @@ use C4::Members; use Koha::Account; use Koha::Account::Lines; use Koha::Account::Offsets; +use Koha::CirculationRules; +use Koha::Checkouts; use Koha::Items; +use Koha::Patrons; =head1 NAME @@ -65,12 +68,17 @@ FIXME : if no replacement price, borrower just doesn't get charged? sub chargelostitem { my $dbh = C4::Context->dbh(); - my ( $borrowernumber, $itemnumber, $replacementprice, $description ) = @_; + my ( $borrowernumber, $itemnumber, $replacementprice, $description, $opts ) = @_; + $opts ||= {}; + my $patron = Koha::Patrons->find($borrowernumber); my $item = Koha::Items->find($itemnumber); my $itype = $item->itemtype; $replacementprice //= 0; - my $defaultreplacecost = $itype->defaultreplacecost; + + my $defaultreplacecost = $itype->defaultreplacecost; + my $usedefaultreplacementcost = C4::Context->preference("useDefaultReplacementCost"); + my $processingfeenote = C4::Context->preference("ProcessingFeeNote"); my $lost_control_pref = C4::Context->preference('LostChargesControl'); my $lost_control_branch; @@ -85,41 +93,45 @@ sub chargelostitem { : $item->holdingbranch; } - my $processfee = Koha::CirculationRules->get_effective_rule_value( - { - rule_name => "lost_item_processing_fee", - categorycode => undef, - itemtype => $itype->itemtype, - branchcode => $lost_control_branch + my $interface = $opts->{interface} // C4::Context->interface; + my $library_id = defined $opts->{library_id} ? $opts->{library_id} : $lost_control_branch; + + my $issue_id = $opts->{issue_id}; + if ( !defined $issue_id ) { + my $checkout = Koha::Checkouts->find( { itemnumber => $itemnumber } ); + if ( !$checkout && $item->in_bundle ) { + my $host = $item->bundle_host; + $checkout = $host->checkout; } - ) // 0; - my $usedefaultreplacementcost = C4::Context->preference("useDefaultReplacementCost"); - my $processingfeenote = C4::Context->preference("ProcessingFeeNote"); + $issue_id = $checkout ? $checkout->issue_id : undef; + } if ( $usedefaultreplacementcost && $replacementprice == 0 && $defaultreplacecost ) { $replacementprice = $defaultreplacecost; } - my $checkout = Koha::Checkouts->find( { itemnumber => $itemnumber } ); - if ( !$checkout && $item->in_bundle ) { - my $host = $item->bundle_host; - $checkout = $host->checkout; - } - my $issue_id = $checkout ? $checkout->issue_id : undef; my $account = Koha::Account->new( { patron_id => $borrowernumber } ); - # first make sure the borrower hasn't already been charged for this item (for this issuance) my $existing_charges = $account->lines->search( { - itemnumber => $itemnumber, + item_id => $itemnumber, debit_type_code => 'LOST', - issue_id => $issue_id + issue_id => $issue_id, } )->count(); # OK, they haven't unless ($existing_charges) { + my $processfee = Koha::CirculationRules->get_effective_rule_value( + { + rule_name => "lost_item_processing_fee", + categorycode => undef, + itemtype => $itype->itemtype, + branchcode => $library_id, + } + ) // 0; + #add processing fee if ( $processfee && $processfee > 0 ) { my $accountline = $account->add_debit( @@ -129,10 +141,10 @@ sub chargelostitem { note => $processingfeenote, user_id => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef, interface => C4::Context->interface, - library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef, + library_id => $library_id, type => 'PROCESSING', item_id => $itemnumber, - issue_id => $issue_id, + ( defined $issue_id ? ( issue_id => $issue_id ) : () ), } ); } @@ -146,10 +158,10 @@ sub chargelostitem { note => undef, user_id => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef, interface => C4::Context->interface, - library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef, + library_id => $library_id, type => 'LOST', item_id => $itemnumber, - issue_id => $issue_id, + ( defined $issue_id ? ( issue_id => $issue_id ) : () ), } ); } diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 52b04abdd4c..962b22844ec 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -4447,22 +4447,49 @@ sub LostItem { or warn "_FixOverduesOnReturn($borrowernumber, $itemnumber...) failed!"; # zero is OK, check defined if ( C4::Context->preference('WhenLostChargeReplacementFee') ) { + my $desc = sprintf( + "%s %s %s", + $issues->{'title'} || q{}, + $issues->{'barcode'} || q{}, + $issues->{'itemcallnumber'} || q{}, + ); + + my $interface = + ( $mark_lost_from && $mark_lost_from eq 'cronjob' ) + ? 'cron' + : C4::Context->interface; + + my $item_obj = Koha::Items->find($itemnumber); + my $issue_obj = Koha::Checkouts->search( + { + itemnumber => $itemnumber, + borrowernumber => $borrowernumber, + } + )->next; + my $rule_branch = Koha::Checkout->branch_for_fee_context( + fee_type => 'LOST', + patron => $patron, + item => $item_obj, + issue => $issue_obj, + ); + my $issue_id = $issues->{issue_id} // ( $issue_obj ? $issue_obj->issue_id : undef ); + C4::Accounts::chargelostitem( $borrowernumber, $itemnumber, - $issues->{'replacementprice'}, - sprintf( - "%s %s %s", - $issues->{'title'} || q{}, - $issues->{'barcode'} || q{}, - $issues->{'itemcallnumber'} || q{}, - ), + ( ( $issues->{'replacementprice'} // 0 ) + 0 ), + $desc, + { + interface => $interface, + library_id => $rule_branch, + issue_id => $issue_id, + } ); - - #FIXME : Should probably have a way to distinguish this from an item that really was returned. - #warn " $issues->{'borrowernumber'} / $itemnumber "; } + #FIXME : Should probably have a way to distinguish this from an item that really was returned. + #warn " $issues->{'borrowernumber'} / $itemnumber "; + MarkIssueReturned( $borrowernumber, $itemnumber, undef, $patron->privacy, $params ) if $mark_returned; } diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 33776e4a146..491fd135e12 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -647,7 +647,17 @@ sub UpdateFine { }; my $desc = $letter ? $letter->{content} : sprintf( "Item %s - due %s", $itemnum, output_pref($due) ); - my $account = Koha::Account->new( { patron_id => $borrowernumber } ); + my $account = Koha::Account->new( { patron_id => $borrowernumber } ); + my $item_obj = Koha::Items->find($itemnum); + my $issue_obj = Koha::Checkouts->find($issue_id); + + my $rule_branch = Koha::Checkout->branch_for_fee_context( + fee_type => 'OVERDUE', + patron => $patron, + item => $item_obj, + issue => $issue_obj, + ); + $accountline = $account->add_debit( { amount => $amount, @@ -655,7 +665,7 @@ sub UpdateFine { note => undef, user_id => undef, interface => C4::Context->interface, - library_id => undef, #FIXME: Should we grab the checkout or circ-control branch here perhaps? + library_id => $rule_branch, type => 'OVERDUE', item_id => $itemnum, issue_id => $issue_id, diff --git a/Koha/Checkout.pm b/Koha/Checkout.pm index 242a71b9e18..967b199a55d 100644 --- a/Koha/Checkout.pm +++ b/Koha/Checkout.pm @@ -24,6 +24,7 @@ use DateTime; use Try::Tiny qw( catch try ); use C4::Circulation qw( AddRenewal CanBookBeRenewed LostItem MarkIssueReturned ); +use Koha::Checkout; use Koha::Booking; use Koha::Checkouts::Renewals; use Koha::Checkouts::ReturnClaims; @@ -341,6 +342,52 @@ sub claim_returned { }; } +=head2 branch_for_fee_context + +my $branchcode = Koha::Checkout->branch_for_fee_context( + fee_type => 'OVERDUE' | 'LOST' | 'PROCESSING', + ); + +Determines which branch’s rules should apply to a fee based on system preferences and available context. For LOST/PROCESSING it honors LostChargesControl; for OVERDUE it honors CircControl. It also respects HomeOrHoldingBranch when choosing the item’s branch. Returns a branchcode (string) or undef if it cannot be determined. + +=cut + +sub branch_for_fee_context { + my ( $class, %args ) = @_; + + my $fee_type = $args{fee_type} // 'OVERDUE'; + + # Choose the controlling preference depending on fee type + my $control_pref = + ( $fee_type eq 'LOST' || $fee_type eq 'PROCESSING' ) + ? C4::Context->preference('LostChargesControl') + : C4::Context->preference('CircControl'); + + # Item branch selection (home vs holding) + my $hoh_pref = C4::Context->preference('HomeOrHoldingBranch') // 'home'; + my $use_holding = ( $hoh_pref =~ /holding/i ) ? 1 : 0; + + # Support Koha objects OR plain hashes + my $patron_branch = eval { $args{patron}->branchcode } // ( $args{patron} && $args{patron}->{branchcode} ); + my $issuing_branch = eval { $args{issue}->branchcode } // ( $args{issue} && $args{issue}->{branchcode} ); + my $item_home = eval { $args{item}->homebranch } // ( $args{item} && $args{item}->{homebranch} ); + my $item_holding = eval { $args{item}->holdingbranch } // ( $args{item} && $args{item}->{holdingbranch} ); + my $item_branch = $use_holding ? ( $item_holding // $item_home ) : $item_home; + + # Normalize empty strings to undef so // works as intended + for my $ref ( \$patron_branch, \$issuing_branch, \$item_home, \$item_holding, \$item_branch ) { + $$ref = undef if defined $$ref && $$ref eq ''; + } + + # Resolution order depends on controlling pref + my $resolved = + ( defined $control_pref && $control_pref eq 'PatronLibrary' ) ? $patron_branch + : ( defined $control_pref && $control_pref eq 'PickupLibrary' ) + ? ( $issuing_branch // $patron_branch // $item_branch ) + : $item_branch; # ItemLibrary (default) + return $resolved; +} + =head2 Internal methods =head3 _type diff --git a/misc/cronjobs/longoverdue.pl b/misc/cronjobs/longoverdue.pl index c1b3460aa3a..31d2c7cd32c 100755 --- a/misc/cronjobs/longoverdue.pl +++ b/misc/cronjobs/longoverdue.pl @@ -34,6 +34,9 @@ use Pod::Usage qw( pod2usage ); use C4::Circulation qw( LostItem MarkIssueReturned ); use C4::Context; use C4::Log qw( cronlogaction ); +use Koha::Checkout; +use Koha::Checkouts; +use Koha::Items; use Koha::ItemTypes; use Koha::Patron::Categories; use Koha::Patrons; @@ -522,7 +525,23 @@ foreach my $startrange ( sort keys %$lost ) { if ($confirm) { Koha::Items->find( $row->{itemnumber} )->itemlost($lostvalue)->store; if ( $charge && $charge eq $lostvalue ) { - LostItem( $row->{'itemnumber'}, 'cronjob', $mark_returned ); + my $patron = Koha::Patrons->find( $row->{borrowernumber} ); + my $item = Koha::Items->find( $row->{itemnumber} ); + my $issue = Koha::Checkouts->search( + { + itemnumber => $row->{itemnumber}, + borrowernumber => $row->{borrowernumber}, + } + )->next; + + my $rule_branch = Koha::Checkout->branch_for_fee_context( + fee_type => 'LOST', + patron => $patron, + item => $item, + issue => $issue, + ); + + LostItem( $row->{itemnumber}, 'cronjob', $mark_returned, { library_id => $rule_branch } ); } elsif ($mark_returned) { $patron ||= Koha::Patrons->find( $row->{borrowernumber} ); MarkIssueReturned( $row->{borrowernumber}, $row->{itemnumber}, undef, $patron->privacy ); -- 2.52.0