Bug: Bank Transactions get reversed withdrawals/deposits on v13 upgrade

casesolved-co-uk · April 30, 2022, 12:47am

I’ll let the evidence speak for itself:

As part of this PR in v13 from the start, 15 months ago.

	rename_field("Bank Transaction", "debit", "deposit")
	rename_field("Bank Transaction", "credit", "withdrawal")

Here for an explanation.

Meaning Bank Transactions are now stored with the opposite sign. I dread to think how the bank reconciliation corrupts the General Ledger.

And just to be clear, we’re talking about Bank Account Statement Transactions, not Chart of Account Asset Bank Accounts explanation which are correctly the opposite sign.

I noticed because old transactions before this PR are now completely the opposite sign to transactions after the change. An old sale is now a ‘withdrawal’ and an old purchase is a ‘deposit’.

Patch file making the sign change

And no this hasn’t been fixed: bank_transaction.py (develop branch on date of posting).

Before:
self.unallocated_amount = abs(flt(self.credit) - flt(self.debit))

Now:
self.unallocated_amount = abs(flt(self.withdrawal) - flt(self.deposit))

dj12djdjs · April 30, 2022, 1:30am

I’ve never used this tool in my implementation, so I’m not all that familiar with it. However, I would think this kind of bug would result in your actual bank account balance and ERP bank account balance not matching wouldn’t it? If this is the case I’m shocked the PR is 1 year ago and only now the bug is noticed.

peterg · April 30, 2022, 1:33am

A deposit into your bank account should list as a positive number in the debit column, matched to a credit (usually in either a receivables account or an income account). Is that not what’s happening here? I have never used this tool either, so I may be completely misunderstanding.

casesolved-co-uk · April 30, 2022, 1:56am

Read it again. I said Bank Transaction, not chart of accounts.

peterg · April 30, 2022, 8:40am

There’s no need to be rude, Richard. We’re all just volunteers here, yeah?

I understood that you were making a distinction between Bank Transactions and the Chart of Accounts when I responded to you. My question was about what’s actually getting written to the register in your system, as that’s not immediately clear. The terminology is inherently a bit slippery, as banks often issue statements from their perspective: what counts as a debit to them is a credit to us, and vice versa. I would guess that frappe changed the field names to avoid exactly that ambiguity.

I don’t have pre-existing data to look at, but the Bank Transactions doctype seems to be working correctly for me now with the Bank Reconciliation Tool.

Starting Spreadsheet:

Bank Reconciliation Tool → Bank Transactions:

Bank Reconciliation Tool → Vouchers:

General Ledger:

It all looks correct to me.

From the patch you’ve linked, the only thing I can see changed were the field/label names. Unless I’m just missing it, I don’t see anything there or in the PR reversing the sign of actual accounting entries. It is very understandable why someone might be misled by the previous use of “debit”/“credit” here, but like @dj12djdjs I would have thought the confusion would be quickly noticed via incorrect bank balances.

Are you saying that you previously had correct accounting entries but now they’ve been changed to be incorrect? That’s the part I’m not understanding from your post.

-Peter

casesolved-co-uk · April 30, 2022, 11:28am

A bank statement is always from the customer perspective, credit increases the balance, debit decreases the balance, as the external links state.

Plaid import has been the wrong sign since this PR.

peterg · April 30, 2022, 1:09pm

That’s not how it’s been explained to me, but in the end the difference between “from the bank’s perspective” and “from the customer’s perspective but reversed from chart of accounts” seems purely academic. Both are describing the same credit/debit structure.

In either case, before the fields were renamed, it certainly looks like ERPNext using standard double-entry language, with “Debit” describing money going into an asset account and “Credit” describing money coming out. If you were doing the opposite, I’m confused about how you were getting correct bank balances.

I’m not doubting you, but I just created a plaid sandbox account out of curiosity and it seems to be working fine for me (at least on this sandbox data). If you’re getting the reverse of that, something more complicated seems to be going on.

casesolved-co-uk · April 30, 2022, 1:41pm

I’m not sure which part of definition you’re trying to argue with.
A bank transaction credit is a deposit not a withdrawal.
Since this PR, bank transactions have been reversed, i.e existing transactions are now the opposite sign to new ones. Seems pretty simple to me.

peterg · April 30, 2022, 1:58pm

It would be really helpful, I think, if you posted some screenshots of the erroneous behavior you’re seeing. At the moment, we’re not really getting anywhere.

casesolved-co-uk · April 30, 2022, 3:12pm

I’m not sure where you want to get?
I’ve reported a serious issue, you don’t understand it.

I’ll figure out the ramifications for people who have upgraded from v12 and report those too.

The two that I can think of are:

if there are unreconciled bank transactions from v12, the v13 tool will at best not reconcile against existing transactions, at worst will corrupt your general ledger. Best approach would be to delete them and reimport
if you want your bank transaction history to make any sense, you’ll have to swap the old amounts from one column to the other (and maybe other things)

peterg · April 30, 2022, 3:13pm

As said, a simple screenshot of this happening would be very helpful.

dj12djdjs · April 30, 2022, 3:20pm

I think I know what @casesolved-co-uk is trying to point out. The calculation remains the same, just the labels are incorrect.

If I understand correctly

rename_field("Bank Transaction", "debit", "deposit")
rename_field("Bank Transaction", "credit", "withdrawal")

Should be set as

rename_field("Bank Transaction", "debit", "withdrawal")
rename_field("Bank Transaction", "credit", "deposit")

Then
self.unallocated_amount = abs(flt(self.deposit) - flt(self.withdrawal))

peterg · April 30, 2022, 4:14pm

It’s definitely possible that the patch was applied wrong, but it’s tricky to test since I don’t have any v12 documents to test against. In any case, I’m more curious about why OP is seeing erroneous behavior on new transactions. Whatever the bug is, I haven’t been able to reproduce it. New transactions are created correctly on my system.

casesolved-co-uk · May 5, 2022, 3:06am

So here’s a fix for pre-v13 Bank Transactions:

Paste this as a new Server Script:
Name = Bank Transaction Polarity Swap
Type = API
Method = bank_transaction_polarity_swap

bts = frappe.db.get_all('Bank Transaction',
    filters={'modified': ('<', frappe.form_dict.modified_before)},
    fields=('name', 'withdrawal', 'deposit'))

for bt in bts:
    frappe.db.set_value('Bank Transaction', bt['name'], 'withdrawal', bt['deposit'])
    frappe.db.set_value('Bank Transaction', bt['name'], 'deposit', bt['withdrawal'])

Paste this as a new client script:
DocType = Bank Transaction
Apply To = List

frappe.listview_settings['Bank Transaction'] = {
    onload(listview) {
        listview.page.add_menu_item('Swap Polarity', () => swap_polarity());
    }
};

function swap_polarity() {
    frappe.prompt([{'fieldname':'modified_before','fieldtype':'Datetime','label':'Modified Before','reqd':1}],
        (values) => {
            frappe.call('bank_transaction_polarity_swap', values);
        }, 'Swap polarity on transactions:', 'Swap Now!'
    );
}

Find the modified date of the latest bad pre-v13 Bank Transaction by hovering your mouse over the modified column in the Bank Transaction list view (you may need to change the sort options at the top of the list to find it easily):
Refresh the list view and run the new menu action Swap Polarity
Enter a Modified Before Datetime that is just after the Datetime found in 3, and run
Disable the client and server scripts and refresh the list view to restore the usual indicators

Unfortunately this changes the modified dates of the Transactions. I would have preferred to use a frappe.db.sql("UPDATE"), but those aren’t allowed in a Server Script.

peterg · May 5, 2022, 3:17am

Glad you found a solution!

To prevent the modified timestamp from being updated, one could add the parameter update_modified=False to the frappe.db.set_value call.