feat(Bank Transaction): auto-book included bank fees (backport #346)#368
Draft
mergify[bot] wants to merge 5 commits into
Draft
feat(Bank Transaction): auto-book included bank fees (backport #346)#368mergify[bot] wants to merge 5 commits into
mergify[bot] wants to merge 5 commits into
Conversation
* feat(Bank Transaction): auto-book included bank fees 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> * fix(Bank Transaction): make automatic fee booking optional 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> * chore(Banking Settings): bump modified timestamp for model sync Update the DocType JSON modified timestamp so the controller change is picked up reliably during model sync. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(Bank Transaction): validate withdrawal against included fee 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> * chore: bump min required erp version we depend on frappe/erpnext#52760 * fix: Cancel only fee JEs created by this feature * fix: use bank date as clearance_date Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * test: cover disabled automatic bank fee entries Ensure a Bank Transaction can be submitted without a bank fee account when automatic bank fee journal entries are disabled, and assert that no linked fee Journal Entry is created. * fix(banking): enforce fee account setup for automatic bank fee entries Prevent automatic bank fee journal entries from being enabled until every company Bank Account has a bank fee account, and reject company bank accounts without one while the feature is enabled. * test: submitting a withdrawal with an included fee must create and reconcile the fee JE * fix: keep deposit fee journal entries out of bank reconciliation Create fee Journal Entries for deposit Bank Transactions with included fees, but do not add them to payment entries because the fee is already deducted before the bank credits the deposit. Semantically, a row in Bank Transaction would say "this JE is allocatable against the bank amount later", which is exactly what we don’t want for a deposit fee. Add regression coverage for included-fee deposit and withdrawal flows. * fix: distinguish fee and custom journal entries in bank reconciliation Keep cheque_no-linked custom journal entries hidden from matching while excluding standard fee journal entries only for deposit transactions. This lets unreconciled withdrawal fee journal entries be offered again. * fix: preserve existing allocations for included bank fees Recompute `allocated_amount` and `unallocated_amount` from `payment_entries` after appending the fee journal entry, so prior allocations are not lost. Add a regression test covering fee entry creation when an allocation already exists. * refactor: centralize common testing utils * fix: book fee on deposits during reconciliation * fix: remove obsolete code and test Since we no longer create fee JEs for deposits, there's nothing to exclude from matching. The test was validating behavior that no longer exists. * test: update fee account names This test now creates its own GL account (_Test Bank Fee Reco GL) instead of reusing self.gl_account, avoiding the "account already used by another bank account" error. * fix: backfill party and reference on included-fee reconciliation JEs * fix: respect feature-flag * fix: require full reconciliation for included deposit fees Prevent deposit-side included bank fees from being applied more than once by rejecting partial or follow-up voucher reconciliation for fee-bearing Bank Transactions. * fix: round values before comparison * fix(Bank Transaction): flag zero-amount included fees Log unsupported zero-amount Bank Transactions with included fees and surface the issue in the form so users can review them manually. * test: cancel Bank Transaction cancels linked fee Journal Entry * fix: foreign currency-fee * test: simplify test names * refactor: test error types, not messages --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> (cherry picked from commit 3b3cf37) # Conflicts: # banking/klarna_kosma_integration/doctype/bank_reconciliation_tool_beta/unpaid_vouchers.py # banking/overrides/bank_transaction.py # banking/testing_utils.py # pyproject.toml
This comment was marked as resolved.
This comment was marked as resolved.
9 tasks
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Auto-book included bank fees on Bank Transaction submit (withdrawals) and at reconciliation time (deposits).
Withdrawals: On submit, a Journal Entry is created (
Dr Bank Fees / Cr Bank) and allocated against the Bank Transaction, partially reconciling the fee portion.Deposits: Fee booking is deferred to reconciliation, where the correct counter-account (e.g. receivable) is known. The reconciliation tool matches deposits using
deposit + included_feeas the target amount, so the right vouchers are found. At reconciliation:Dr Bank, Dr Bank Fees / Cr Receivable)Configuration
Bank Fee Accountfield on Bank Account (Link to Expense account, same currency)Enable Automatic Journal Entries for Bank Feescheckbox in Banking SettingsDepends on
fix(accounts): keep zero-net bank transactions with included fees unreconciled frappe/erpnext#54269 (case excluded via e03d141unallocated_amountfordeposit=0 + included_feetransactions)Test plan
This is an automatic backport of pull request feat(Bank Transaction): auto-book included bank fees #346 done by Mergify.
Greptile Summary
This PR implements automatic bank-fee booking on Bank Transaction submit (withdrawals) and reconciliation (deposits), adding a
bank_fee_accountfield to Bank Account and a global feature toggle in Banking Settings.Dr Bank Fees / Cr Bank) is created on submit and allocated against the BT, withcreate_je_automatic_rulescorrectly recalculatingremaining_amountto account for the pre-allocated fee.included_feeso full-invoice vouchers are found; single-currency paths use a JE, multi-currency (company-currency bank) paths use a PE deduction row, and foreign-currency bank accounts raiseCurrencyMismatchError.validateblocks currency mismatch and missing fee accounts when the feature is enabled; Banking Settingsvalidateblocks enabling the feature while any company bank account lacks a fee account.Confidence Score: 4/5
The core withdrawal and deposit fee-booking paths are well-tested and logically sound; a previously-reported unresolved issue means the fee is silently dropped for manual-rate reconciliation of deposits.
The new accounting logic is well-covered by tests and the guard rails (currency checks, full-reconciliation enforcement, feature-flag gating) are solid. However, an open finding from the prior review — that
make_pe_against_invoicesis called withoutincluded_feewhenmanual_reconcile_amountsis provided — remains unaddressed. A deposit with an included fee reconciled via the manual-FX dialog will post the PE without the fee deduction, silently under-expensing to the fee account and leaving the ledger incorrect for any treasury team using statement-rate reconciliation.banking/klarna_kosma_integration/doctype/bank_reconciliation_tool_beta/unpaid_vouchers.py — the manual-reconcile branch in get_payment_entries and the multi-party + foreign-currency-invoice edge case in make_jv_against_invoices.
Important Files Changed
Sequence Diagram
sequenceDiagram participant BT as Bank Transaction participant BS as Banking Settings participant BA as Bank Account participant JE as Journal Entry participant PE as Payment Entry participant UV as unpaid_vouchers Note over BT: WITHDRAWAL FLOW BT->>BS: before_submit: check enable_automatic_journal_entries? BS-->>BT: "enabled=True" BT->>BA: get bank_fee_account BA-->>BT: fee_account BT->>JE: create_je_bank_fees (Dr BankFees / Cr Bank) JE-->>BT: fee_je_name + clearance_date set BT->>BT: append fee_je to payment_entries BT->>BT: recompute unallocated_amount Note over BT: DEPOSIT FLOW (deferred to reconciliation) BT->>UV: bulk_reconcile_vouchers → get_payment_entries UV->>UV: get_deposit_included_fee(bt) UV->>UV: validate_included_fee_reconciliation alt single-currency invoice UV->>JE: "make_jv_against_invoices(included_fee=fee)" Note right of JE: Dr Bank (deposit) + Dr BankFees (fee) / Cr Receivable (full) else multi-currency invoice (company-currency bank) UV->>PE: "make_pe_against_invoices(included_fee=fee)" Note right of PE: PE with fee deduction row else foreign-currency bank account UV-->>BT: CurrencyMismatchError end Note over BT: MATCHING QUERY BT->>UV: check_matching UV->>UV: "matching_amount = unallocated + get_deposit_included_fee" Note right of UV: Searches vouchers for deposit+fee amount Note over BT: CANCEL FLOW BT->>JE: on_cancel: cancel system-generated JEs with BT referenceComments Outside Diff (1)
banking/klarna_kosma_integration/doctype/bank_reconciliation_tool_beta/unpaid_vouchers.py, line 74-86 (link)When
manual_reconcile_amountsis non-empty andincluded_fee > 0, the code falls into theelsebranch wheremake_pe_against_invoicesis called without theincluded_feeargument (defaults to0.0).validate_included_fee_reconciliationruns and passes without blocking this path, so the user receives no warning. The resulting Payment Entry settles the invoice but never records the bank-fee expense to the configured fee account, leaving the ledger incorrect for any manual-rate reconciliation on a deposit BT that carries a fee.Reviews (2): Last reviewed commit: "style: format js file" | Re-trigger Greptile