Bugzilla – Attachment 192513 Details for
Bug 35612
Record branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 35612: Recorded branch context for OVERDUE, LOST, and PROCESSING fees
b68c02f.patch (text/plain), 18.25 KB, created by
Martin Renvoize (ashimema)
on 2026-02-05 08:53:56 UTC
(
hide
)
Description:
Bug 35612: Recorded branch context for OVERDUE, LOST, and PROCESSING fees
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-02-05 08:53:56 UTC
Size:
18.25 KB
patch
obsolete
>From b68c02facf6026348cedee34227bb44f9e77c5fc Mon Sep 17 00:00:00 2001 >From: Laura_Escamilla <laura.escamilla@bywatersolutions.com> >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 <trevor.diamond@mainlib.org> >Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> >--- > 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 >
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 35612
:
186567
|
186568
|
186569
|
186570
|
186571
|
187919
|
187920
|
187921
|
187922
|
189137
|
192345
|
192346
|
192347
|
192348
|
192349
|
192385
|
192386
|
192387
|
192388
|
192389
|
192390
|
192497
|
192498
|
192499
|
192500
|
192501
|
192502
| 192513 |
192514
|
192515
|
192516
|
192517
|
192518
|
192519
|
192520
|
192521
|
192522