Bug 35612

Summary: Record branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees
Product: Koha Reporter: Pirkko-Liisa Lauhikari <pirkko-liisa.lauhikari>
Component: Fines and feesAssignee: Laura Escamilla <Laura.escamilla>
Status: Passed QA --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: andrew, Laura.escamilla, lisette, lucas, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39802
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement allows the optional recording of the branch used when generating an accountline. For OVERDUE fees, the branch chosen by CircControl will be reflected here. For LOST and PROCESSING fees, the branch chosen by LostChargesControl will be reflected here. This development will allow librarians to reference charges against the circ rules to determine which branches rule was applied when creating a fine.
Version(s) released in:
Circulation function:
Bug Depends on: 36506, 39802    
Bug Blocks:    
Attachments: Bug 35612: Added new AccountLinesBranchSource system preference
Bug 35612: Added branch code logic
Bug 35612: sysprefs.sql updates
Bug 35612: Added the system preference within the Circulation and Fines policy in Administration
Bug 35612: Tests
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees
Bug 35612: Unit test Circulation.t
Bug 35612: Unit tests for Overdues.t
Bug 35612: Updates to Circulation.pm
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees.
Bug 35612: Unit test - Checkout.t
Bug 35612: Unit test - Overdues.t
Bug 35612: Unit test - Circulation.t
Bug 35612: Update to Accounts.pm
Bug 35612: Unit tests - Accounts.t
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees.
Bug 35612: Unit test - Checkout.t
Bug 35612: Unit test - Overdues.t
Bug 35612: Unit test - Circulation.t
Bug 35612: Update to Accounts.pm
Bug 35612: Unit tests - Accounts.t
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees.
Bug 35612: Unit test - Checkout.t
Bug 35612: Unit test - Overdues.t
Bug 35612: Unit test - Circulation.t
Bug 35612: Update to Accounts.pm
Bug 35612: Unit tests - Accounts.t
Bug 35612: Recorded branch context for OVERDUE, LOST, and PROCESSING fees
Bug 35612: Add unit test for Koha::Checkout->branch_for_fee_context
Bug 35612: Add tests for branch context recording in OVERDUE fees
Bug 35612: Add tests for branch context recording in LOST fees via LostItem
Bug 35612: Fix incorrect primary key reference
Bug 35612: Add tests for chargelostitem branch context
Bug 35612: (QA follow-up) Fix uninitialized value warnings
Bug 35612: (QA follow-up) Use interface parameter and remove self-referential use
Bug 35612: (QA follow-up) Document undef handling in branch_for_fee_context
Bug 35612: (QA follow-up) Add test for undefined LostChargesControl preference

Description Pirkko-Liisa Lauhikari 2023-12-20 11:18:34 UTC
We would like to have branchcode recorded to the OVERDUE fees in accountlines table. Configurability is desired, so that it is possible to choose which library is recorded. Options could be customer's home library, issuing library and item's home library.
Comment 1 Pirkko-Liisa Lauhikari 2024-09-27 08:17:23 UTC
We hope also, that branchcode information for all customer fees lines.
Comment 2 Andrew Fuerste-Henry 2025-05-01 13:58:21 UTC
Agreed it would be helpful to consistently populate the branchcode for accountlines.

For overdues fines, I believe the branch used to calculate the fine is determined by the CircControl and HomeOrHoldingBranch system preferences. Accountlines should record the branch used to calculate the fine.

The longoverdue cron also follows CircControl but fails to record that branch in accountlines. It should do so.
Comment 3 Laura Escamilla 2025-09-18 17:20:06 UTC
Created attachment 186567 [details] [review]
Bug 35612: Added new AccountLinesBranchSource system preference
Comment 4 Laura Escamilla 2025-09-18 17:20:09 UTC
Created attachment 186568 [details] [review]
Bug 35612: Added branch code logic
Comment 5 Laura Escamilla 2025-09-18 17:20:12 UTC
Created attachment 186569 [details] [review]
Bug 35612: sysprefs.sql updates
Comment 6 Laura Escamilla 2025-09-18 17:20:14 UTC
Created attachment 186570 [details] [review]
Bug 35612: Added the system preference within the Circulation and Fines policy in Administration

To test:

1. Set system preferences:
    * FinesMode: Calculate and Charge
    * AccountLinesBranchSource: (this will be tested with different values later)
2. Set circulation rules:
    * Ensure a circulation rule exists that charges overdue fines for the item type and branch you're testing with.
3. Check out an item to a test patron with a due date in the past (to trigger overdue).
4.  Run the fines cronjob: perl /usr/share/koha/bin/cronjobs/fines.pl
5. Verify fines appear on the patron’s Accounting tab in the staff interface.
6. Confirm branchcode is NULL in the database:
    * SELECT accountlines_id, branchcode, debit_type_code FROM accountlines ORDER BY accountlines_id DESC LIMIT 10;
    * Expect to see branchcode = NULL for the OVERDUE debit.
7.  Apply the patch to your Koha instance.
8.  Run updatedatabase
9. Run restart_all
10. In the staff interface, go to Administration → System Preferences, search for AccountLinesBranchSource, and set it to: "the library the item was checked out from" (checkoutbranch)
11.  Repeat steps 3–6.
12.  Confirm that in the SQL output, the branchcode now matches the checkout branch of the item.
13. Repeat steps 10–12 with each of the following AccountLinesBranchSource values:
    * itemhomebranch: The item’s home library
    * patronhomebranch: The patron’s home library
    * checkoutbranch: The checkout library (already tested)
    * none: Should result in NULL branchcode
    * Confirm that: The correct branch is recorded in the branchcode field of accountlines. When set to none, branchcode is again NULL.
Comment 7 Laura Escamilla 2025-09-18 17:20:17 UTC
Created attachment 186571 [details] [review]
Bug 35612: Tests

run perl t/db_dependent/Koha/Account.t
Comment 8 Andrew Fuerste-Henry 2025-09-18 18:56:17 UTC
Rather than adding a system preference to determine which branch is used or recorded for a debit, the goal here is to allow the existing system preferences  -- CircControl, LostChargesControl (see bug 39802) -- to determine which branch's rules should be used to calculate the fee and then record that context in accountlines.
Comment 10 Andrew Fuerste-Henry 2025-10-02 20:17:00 UTC
I got so busy thinking about lost items I forgot that Pirkko-Liisa started this bug with OVERDUEs. OVERDUEs also fail to record the branch used to determine the fee and should also be covered in this work.
Comment 11 Laura Escamilla 2025-10-15 16:10:51 UTC
Created attachment 187919 [details] [review]
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees
Comment 12 Laura Escamilla 2025-10-15 16:10:54 UTC
Created attachment 187920 [details] [review]
Bug 35612: Unit test Circulation.t
Comment 13 Laura Escamilla 2025-10-15 16:10:57 UTC
Created attachment 187921 [details] [review]
Bug 35612: Unit tests for Overdues.t
Comment 14 Laura Escamilla 2025-10-15 16:10:59 UTC
Created attachment 187922 [details] [review]
Bug 35612: Updates to Circulation.pm

To test:

There are dependencies for this patch so you’ll need to apply bugs 36506 and 39802

1. Create three test libraries — I used the following existing branches in K-T-D:
    1. FPL (use as item’s home library)
    2. CPL (use as checkout/holding library)
    3. PVL (use as patron home library)
2. Create a patron with home library PVL — Olga Rivera is an example patron.
3. Create an item with:
    1. homebranch = FPL
    2. holdingbranch = CPL
4. System preferences:
    1. FinesMode: set to Calculate and charge.
    2. Within Circulation rules ensure a rule exists that charges overdue fines for the item type/branch you’re using.
5. Check out the item you created or selected to your patron and set the due date to a past date so that it is overdue. 
6. Run fines: perl /usr/share/koha/bin/cronjobs/fines.pl
7. Confirm an OVERDUE fine appears on the patron’s account. 
8. SQL check (expect branchcode is NULL for the OVERDUE line):
SELECT accountlines_id, branchcode, debit_type_code
FROM accountlines
ORDER BY accountlines_id DESC
LIMIT 10; 
9. Apply the patch, run updatedatabase, then restart_all.
10. Enable the system preference CircControl: select an option of PatronLibrary, PickupLibrary, or ItemHomeLibrary
11. Enable the HomeOrHoldingBranch system preference: homebranch or holdingbranch (only matters when CircControl = ItemHomeLibrary)
12. Repeat steps 3-6 for the following scenarios. 
    1. CircControl = PatronLibrary → expect PVL (patron’s home).
    2. CircControl = PickupLibrary → expect CPL (checkout/issuing branch).
    3. CircControl = ItemHomeLibrary + HomeOrHoldingBranch = homebranch → expect FPL (item’s home).
    4. CircControl = ItemHomeLibrary + HomeOrHoldingBranch = holdingbranch → expect CPL (item’s holding).
13. Next we’re going to test this with ‘Lost Items’
14. Make sure WhenLostChargeReplacementFee syspref is set to Charge
15. Set the LostChargesControl syspref to one of the following PatronLibrary, PickupLibrary, or ItemHomeLibrary
16. Replacement price on the item must be > 0, otherwise no LOST debit is created.
17. First test this manually by setting the item to lost. 
18. Start by confirming that an item is currently checked out to the test patron (checked out at CPL).
19. Set preferences for the scenario.
20. Mark the checked out item as lost. 
21. SQL check (find the most recent LOST row):
SELECT accountlines_id, branchcode, debit_type_code
FROM accountlines
ORDER BY accountlines_id DESC
LIMIT 10;
22. Next test this by using the longoverdue cron
23. Re-checkout the item (as above), adjust longoverdue rules so cron will mark it lost.
24. Run perl /usr/share/koha/bin/cronjobs/longoverdue.pl --lost 30=1 --charge 1 --confirm --verbose
25. SQL check again for the new LOST row.
26. Test for the following scenarios:
    1. LostChargesControl = PatronLibrary → expect PVL (patron’s home).
    2. LostChargesControl = PickupLibrary → expect CPL (checkout/issuing branch).
    3. LostChargesControl = ItemHomeLibrary + HomeOrHoldingBranch = homebranch → expect FPL (item’s home).
    4. LostChargesControl = ItemHomeLibrary + HomeOrHoldingBranch = holdingbranch → expect CPL (item’s holding).
27. Make sure that if the replacement price is 0 → no LOST debit should be created.
28. And ensure that if WhenLostChargeReplacementFee = Don’t charge → no LOST debit should be created.
29. Turn FinesMode OFF (i.e., not “Calculate and charge”), run fines again → no new OVERDUE debits should be added.
Comment 15 Laura Escamilla 2025-10-15 16:13:33 UTC
I forgot to add in my test plan to run 

prove t/db_dependent/Overdues.t 

prove t/db_dependent/Circulation.t
Comment 16 Andrew Fuerste-Henry 2025-10-15 18:34:45 UTC
This all worked great for OVERDUE and LOST fees. But with the patches from this bug applied I'm not getting PROCESSING fees generated at all when items are lost. I do get processing fees with just BZ 36506 and BZ 39802 applied.
Comment 17 Laura Escamilla 2025-11-05 19:09:13 UTC
Created attachment 189137 [details] [review]
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees.
Comment 18 Laura Escamilla 2026-02-02 22:53:55 UTC
Created attachment 192345 [details] [review]
Bug 35612: Unit test - Checkout.t
Comment 19 Laura Escamilla 2026-02-02 22:53:58 UTC
Created attachment 192346 [details] [review]
Bug 35612: Unit test - Overdues.t
Comment 20 Laura Escamilla 2026-02-02 22:54:00 UTC
Created attachment 192347 [details] [review]
Bug 35612: Unit test - Circulation.t
Comment 21 Laura Escamilla 2026-02-02 22:54:03 UTC
Created attachment 192348 [details] [review]
Bug 35612: Update to Accounts.pm
Comment 22 Laura Escamilla 2026-02-02 22:54:05 UTC
Created attachment 192349 [details] [review]
Bug 35612: Unit tests - Accounts.t
Comment 23 Andrew Fuerste-Henry 2026-02-03 13:26:04 UTC
This has merge errors due to changes in Bug 40680:
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189876&action=diff
Comment 24 Laura Escamilla 2026-02-03 15:52:54 UTC
Created attachment 192385 [details] [review]
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees.
Comment 25 Laura Escamilla 2026-02-03 15:52:57 UTC
Created attachment 192386 [details] [review]
Bug 35612: Unit test - Checkout.t
Comment 26 Laura Escamilla 2026-02-03 15:52:59 UTC
Created attachment 192387 [details] [review]
Bug 35612: Unit test - Overdues.t
Comment 27 Laura Escamilla 2026-02-03 15:53:02 UTC
Created attachment 192388 [details] [review]
Bug 35612: Unit test - Circulation.t
Comment 28 Laura Escamilla 2026-02-03 15:53:05 UTC
Created attachment 192389 [details] [review]
Bug 35612: Update to Accounts.pm
Comment 29 Laura Escamilla 2026-02-03 15:53:07 UTC
Created attachment 192390 [details] [review]
Bug 35612: Unit tests - Accounts.t
Comment 30 Andrew Fuerste-Henry 2026-02-04 20:51:50 UTC
Created attachment 192497 [details] [review]
Bug 35612: Recorded branch context in accountlines.branchcode for OVERDUE, LOST, and PROCESSING fees.

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 31 Andrew Fuerste-Henry 2026-02-04 20:51:52 UTC
Created attachment 192498 [details] [review]
Bug 35612: Unit test - Checkout.t

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 32 Andrew Fuerste-Henry 2026-02-04 20:51:55 UTC
Created attachment 192499 [details] [review]
Bug 35612: Unit test - Overdues.t

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 33 Andrew Fuerste-Henry 2026-02-04 20:51:58 UTC
Created attachment 192500 [details] [review]
Bug 35612: Unit test - Circulation.t

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 34 Andrew Fuerste-Henry 2026-02-04 20:52:00 UTC
Created attachment 192501 [details] [review]
Bug 35612: Update to Accounts.pm

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 35 Andrew Fuerste-Henry 2026-02-04 20:52:03 UTC
Created attachment 192502 [details] [review]
Bug 35612: Unit tests - Accounts.t

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 36 Martin Renvoize (ashimema) 2026-02-05 08:53:56 UTC
Created attachment 192513 [details] [review]
Bug 35612: Recorded branch context for OVERDUE, LOST, and PROCESSING fees

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>
Comment 37 Martin Renvoize (ashimema) 2026-02-05 08:53:58 UTC
Created attachment 192514 [details] [review]
Bug 35612: Add unit test for Koha::Checkout->branch_for_fee_context

Adds test coverage for the new branch_for_fee_context class method.
Tests are minimal at this stage but verify the method exists and
can be called. More comprehensive testing is covered in the
C4::Accounts, C4::Circulation, and C4::Overdues test additions.

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 38 Martin Renvoize (ashimema) 2026-02-05 08:53:59 UTC
Created attachment 192515 [details] [review]
Bug 35612: Add tests for branch context recording in OVERDUE fees

Adds comprehensive test coverage for C4::Overdues::UpdateFine to verify
that the branchcode field is correctly populated based on CircControl
and HomeOrHoldingBranch system preferences.

Tests verify:
- CircControl = PatronLibrary uses patron's home branch
- CircControl = PickupLibrary uses checkout/issue branch
- CircControl = ItemHomeLibrary respects HomeOrHoldingBranch:
  * homebranch: uses item's home library
  * holdingbranch: uses item's holding library

The tests create distinct libraries for each context (patron home,
item home, item holding, checkout branch) and verify the correct
branch is recorded in accountlines.branchcode for OVERDUE debits.

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 39 Martin Renvoize (ashimema) 2026-02-05 08:54:00 UTC
Created attachment 192516 [details] [review]
Bug 35612: Add tests for branch context recording in LOST fees via LostItem

Adds comprehensive test coverage for C4::Circulation::LostItem to verify
that LOST and PROCESSING fee debits correctly record branchcode based on
LostChargesControl and HomeOrHoldingBranch system preferences.

Tests verify:
- LostChargesControl = PatronLibrary uses patron's home branch
- LostChargesControl = PickupLibrary uses checkout/issue branch
- LostChargesControl = ItemHomeLibrary respects HomeOrHoldingBranch:
  * homebranch: uses item's home library
  * holdingbranch: uses item's holding library

Tests cover both LOST and PROCESSING fee types and verify the branch
context is preserved when fees are charged via the LostItem function,
which is called both from the staff interface when manually marking
items lost and from the longoverdue cronjob.

The tests also verify correct behavior when mark_lost_from = 'cronjob'
to ensure the interface context is properly set for batch operations.

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 40 Martin Renvoize (ashimema) 2026-02-05 08:54:01 UTC
Created attachment 192517 [details] [review]
Bug 35612: Fix incorrect primary key reference

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 41 Martin Renvoize (ashimema) 2026-02-05 08:54:03 UTC
Created attachment 192518 [details] [review]
Bug 35612: Add tests for chargelostitem branch context

Adds test coverage for C4::Accounts::chargelostitem to verify that
LOST and PROCESSING fee debits correctly record branchcode based on
LostChargesControl and HomeOrHoldingBranch system preferences.

Tests verify all scenarios:

1. LostChargesControl behavior:
   - PatronLibrary: Uses patron's home branch
   - ItemHomeLibrary + homebranch: Uses item's home library
   - ItemHomeLibrary + holdingbranch: Uses item's holding library
2. Explicit library_id override:
   - When library_id is passed in options, it takes precedence
3. Deduplication logic:
   - Prevents duplicate LOST charges for same itemnumber+issue_id
   - Regression test for item_id column name bug
4. Issue_id handling:
   - Correctly looks up issue_id when not provided
   - Handles bundled items (looks up host item's checkout)
5. Processing fees:
   - Honors the same branch logic as LOST fees
   - Correctly applies lost_item_processing_fee circulation rule
     using the appropriate branch context

This test subtest was added to the existing "C4::Accounts::chargelostitem tests"
section and increases the test plan from 4 to 5 subtests.

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 42 Martin Renvoize (ashimema) 2026-02-05 08:54:04 UTC
Created attachment 192519 [details] [review]
Bug 35612: (QA follow-up) Fix uninitialized value warnings

When LostChargesControl system preference is not set (undefined),
the code was generating 'Use of uninitialized value' warnings
when comparing $lost_control_pref in string equality checks.

This adds defined checks before the comparisons to handle the
case where the preference is undefined, treating it the same as
'ItemHomeLibrary' (the default).

Test plan:
1. Run prove t/db_dependent/Circulation.t
2. Verify all tests pass without warnings
Comment 43 Martin Renvoize (ashimema) 2026-02-05 08:54:05 UTC
Created attachment 192520 [details] [review]
Bug 35612: (QA follow-up) Use interface parameter and remove self-referential use

This patch fixes two issues:

1. C4::Accounts::chargelostitem now properly uses the $interface
   variable that was set from $opts->{interface} instead of calling
   C4::Context->interface directly. This ensures the interface
   parameter is honored, particularly important for cronjob context.

2. Removes the self-referential 'use Koha::Checkout;' statement in
   Koha/Checkout.pm which is logically incorrect (a module should
   not use itself).

Test plan:
1. Run prove t/db_dependent/Accounts.t
2. Run prove t/db_dependent/Koha/Checkout.t
3. Verify all tests pass
Comment 44 Martin Renvoize (ashimema) 2026-02-05 08:54:06 UTC
Created attachment 192521 [details] [review]
Bug 35612: (QA follow-up) Document undef handling in branch_for_fee_context

Improves POD documentation for branch_for_fee_context to:
- Document all parameters (patron, item, issue objects)
- Explain the resolution logic for each control preference
- Clarify that returning undef is acceptable behavior
- Note how undef is handled by calling code

Adds inline comment explaining that undef return value is
gracefully handled by Koha::Account->add_debit.

This addresses concerns about unclear undef handling and
improves maintainability.
Comment 45 Martin Renvoize (ashimema) 2026-02-05 08:54:08 UTC
Created attachment 192522 [details] [review]
Bug 35612: (QA follow-up) Add test for undefined LostChargesControl preference

Adds test coverage for the case where LostChargesControl system
preference is not set (returns undef). This ensures the code
handles this gracefully and defaults to ItemHomeLibrary behavior.

The test verifies that:
- No warnings are generated when the preference is undefined
- The default behavior (ItemHomeLibrary) is applied correctly
- The correct branch (item homebranch) is recorded in accountlines

This improves test coverage and prevents regression of the fix
for uninitialized value warnings.
Comment 46 Martin Renvoize (ashimema) 2026-02-05 08:55:52 UTC
I restored the test plans in the commits and enhanced the commit messages throughout.

QA was fun here, there's quite a lot of refactoring going on for what on the face of it sounds like a trivial patch.

Tests are passing, QA scripts are happy and I added a few minor QA follow-ups.