Skip to content

feat(Bank Transaction): auto-book included bank fees#346

Open
barredterra wants to merge 5 commits intoversion-15-hotfixfrom
split/included-bank-fees
Open

feat(Bank Transaction): auto-book included bank fees#346
barredterra wants to merge 5 commits intoversion-15-hotfixfrom
split/included-bank-fees

Conversation

@barredterra
Copy link
Member

@barredterra barredterra commented Feb 17, 2026

Summary

  • Add Bank Account.bank_fee_account configuration, including UI filtering and server-side currency validation.
  • Create Journal Entries for included fees during Bank Transaction submission and update allocation/status values accordingly.
  • Gate automatic fee Journal Entries behind a new Banking Settings flag (enable_automatic_journal_entries_for_bank_fees) so existing sites can opt in safely.
  • Auto-cancel fee Journal Entries when the originating Bank Transaction is canceled.
  • Add focused tests for bank-account validation and included-fee booking behavior.

Co-author

Test plan

  • bench --site test_site run-tests --app banking --module banking.overrides.test_bank_account
  • bench --site test_site run-tests --app banking --module banking.overrides.test_bank_transaction_included_fees

barredterra and others added 5 commits February 17, 2026 22:29
Add bank fee account configuration and create fee Journal Entries on bank transaction submission so included fees are recognized immediately and reconciled correctly.

Co-authored-by: Cursor <cursoragent@cursor.com>
Guard included-fee Journal Entry creation behind a Banking Settings flag so existing sites without bank fee account setup continue to work until the feature is explicitly enabled.

Co-authored-by: Cursor <cursoragent@cursor.com>
Update the DocType JSON modified timestamp so the controller change is picked up reliably during model sync.

Co-authored-by: Cursor <cursoragent@cursor.com>
Reject withdrawal transactions where included fee exceeds withdrawal amount, while still allowing fee handling when deposit is not set.

Co-authored-by: Cursor <cursoragent@cursor.com>
@barredterra
Copy link
Member Author

barredterra commented Feb 18, 2026

Proposed fixes:

1) Cancel only fee JEs created by this feature

Depends on frappe/erpnext#52760

Use first-class JE row references to mark ownership, not cheque_no heuristics.

  • Ownership model

    • Treat fee JEs as linked vouchers by setting Journal Entry Account.reference_type = "Bank Transaction" and reference_name = <bt.name>.
    • Also set Journal Entry.is_system_generated = 1 when creating these fee JEs.
    • Keep cheque_no if useful for reporting/search, but do not use it as the primary cancel selector.
    • Cancellation of linked JE is handled by framework
  • Tests to add

    • create_automatic_journal_entry writes JE row reference + is_system_generated.
    • Cancel BT cancels only linked system-generated fee JEs.
    • Unrelated JE with same cheque_no is untouched.

2) Make guard clauses feature-aware and migration-friendly

Problem
before_submit raises on missing bank_account even when fee automation is disabled. That can break legacy flows unnecessarily.

Fix
Reorder guards in before_submit():

  1. read flag enable_automatic_journal_entries_for_bank_fees
  2. if flag is off -> return immediately
  3. if included_fee <= 0 -> return immediately
  4. only then validate prerequisites (bank_account, fee constraints, etc.) and try creating fee JE

Compatibility choice

  • For missing setup (bank_account / bank_fee_account):
    • strict mode: frappe.throw (safer accounting)
    • compat mode: frappe.log_warning + return (safer migrations)
  • Given your migration concern, compat mode is reasonable initially.

Test to add

  • With feature flag OFF and missing bank_account, submission path does not fail due to this hook.
  • With feature flag ON and valid fee case, hook runs as expected.

3) Stop overwriting allocation totals manually

Problem
create_je_bank_fees manually sets allocated_amount/unallocated_amount, which can conflict with existing rows or previous allocations.

Fix
Let core calculation do the math.

  • Keep appending the JE payment row (allocated_amount = included_fee for withdrawal, 0 for deposit)
  • Then call:
    • doc.update_allocated_amount()
  • Then set status based on resulting unallocated value:
    • Reconciled if <= 0
    • else Unreconciled

Why this is best
Avoids brittle arithmetic and stays aligned with ERPNext’s own allocation behavior.

Test to add

  • Withdrawal with pre-existing payment entries: total allocation remains correct after fee JE append.
  • Edge case: withdrawal == included_fee ends as fully reconciled.

4) Avoid partial cancel by failing if fee JE cancellation fails

Problem
on_cancel currently swallows JE cancel exceptions via msgprint, which can leave inconsistent state.

Fix

  • Collect cancellation failures
  • If any fail, throw a single ValidationError with JE names/links
  • This aborts BT cancel and keeps consistency

Why this is best
Prevents “BT canceled but linked fee JE still submitted” inconsistencies.

Test to add

  • Mock JE .cancel() to raise
  • Assert BT cancellation is blocked and error message includes the failed JE.

@barredterra barredterra mentioned this pull request Feb 18, 2026
3 tasks
@0xD0M1M0
Copy link
Contributor

"Unreconcile Transaction" in the Bank Transaction should not unlink the fee entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants