fix(Bank Reconciliation): Clear Bank Transactions against Journal Entry Account #23721
Conversation
gwhitney
requested review from
deepeshgarg007 and
nextchamp-saqib
as code owners
|
The associated PR to Frappe which would enhance this PR (in particular, prevent accumulating fields in a Bank Reconciliation screen and ensure that only unreconciled Bank Transactions of the Bank Account being reconciled are shown) is frappe/frappe#11776. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
104aaa0
to
e211a95
Compare
|
Checked in mostly whitespace changes for the sake of Frappe Linter, Codacy, and Sider checks. |
|
UPDATE: received word from the Frappe framework maintainers that changes are desired with the associated pull request frappe/frappe#11776. Therefore, this PR for ERPNext should be considered Work-in-Progress (and so it does not make sense to merge it into develop) until a mutually agreeable change is settled on for the necessary Frappe changes, and I can update this PR correspondingly. In addition, now with more experience running with this code change locally, I see that there is a need to add back in the ability to reconcile all of the Journal Entry Account lines in a single Journal Entry simultaneously, as sometimes they match the Bank Transaction as a whole, and it is extra work to reconcile them one at a time. (But I need to preserve the new ability to also reconcile them individually, as sometimes it is a single Journal Entry Account line that matches a Bank Transaction, even though there are other lines in the same Journal Entry.) As I don't see a way to mark this as work-in-progress, please let me know if you prefer that I close it for now and file a new PR when the above amendments are ready, or leave this open and simply update it when ready. Thanks! |
gwhitney
force-pushed
the
reconcile_with_jea
branch
7 times, most recently
from
fd3395b
to
795c5ab
Compare
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
795c5ab
to
9406941
Compare
|
UPDATE: I have received feedback on the Frappe framework change frappe/frappe#11776 enhancing the standard fields feature of the frappe.ui.Page, and updated this pull request accordingly. I once again am recommending it for merging into the main development line of ERPNext as an improvement to the bank reconciliation process. In addition, I have taken the time while waiting for the Frappe issue to resolve to further organize the improvements to the bank reconciliation system in this pull request. The core principles are now laid out in a significant comment block at the top of |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
9406941
to
af10e1e
Compare
|
Checked in mostly whitespace changes to satisfy the Sider/Codacy checks. |
|
I see that there is an SQL error in a query generated by the test suite. My apologies -- as noted in a separate issue, I am unable to run the test suite locally. I will track down the error and amend the pull request within a day. Thanks very much for your patience. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
af10e1e
to
1a694ff
Compare
|
OK, I just amended the pull request to (hopefully) fix the SQL syntax errors. Also, I changed the special custom_filter_configs value that will trigger no adding of fields in the (Base) List view on the Bank Reconciliation page to one that is innocuous for existing Frappe code, so that this change can be merged into develop without breaking anything, and then the field non-duplication will take effect when the related Frappe pull request as linked above is merged. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
1a694ff
to
313f67a
Compare
|
Two more small fixes uncovered by the Travis run. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
3 times, most recently
from
92e72fa
to
16dbd0a
Compare
|
[NOTE: This comment appears to be moot, as it appears the Codacy test has been dropped.] I don't understand the Codacy issue: BankTransaction.get_paid_amount is too complex (19) (MC0001) What is the difficulty here and how should I fix it? Other than that, all of the checks pass. I had to slightly update the Bank Transaction test suite to be sure that the Company values of the inserted transactions were correct. All now appears to be well, and I look forward to your feedback. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
2 times, most recently
from
336fe40
to
667dae6
Compare
|
Just corrected two small typos in the SQL query for Purchase Invoices in the Bank Reconciliation Statement. I remain confident in this pull request and look forward to your feedback. Thanks! |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
667dae6
to
5914c58
Compare
|
Sider parameters appear to have changed, in that the latest run produced new failures that were not flagged before. In any case, I have checked in fixes for everything that was alerted. Mostly it was whitespace and punctuation, but I will say that Sider also caught one bona fide bug (that pre-existed this PR, but is now fixed) -- namely, the Account was not being filled in correctly in any new Payment Entry created from the Reconciliation Tool. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
5914c58
to
2f7fb92
Compare
|
The Travis error is in the procurement tracker, so does not seem to have to do with this PR. Let me know if there's anything else you'd like done before this PR is reviewed. |
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
2f7fb92
to
71ce677
Compare
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
71ce677
to
cb8f5a1
Compare
|
Rebased on develop to avoid merge conflicts. |
…ions Prior to this change, the exact criteria for a Bank Transaction to reconcile with a payment record in ERPNext accounting records varied from place to place in the code. This commit lays out the basic technical principles for bank reconciliation in a central place, namely a series of comments in erpnext/accounts/page/bank_reconciliation/bank_reconciliation.py (if the developers would like this information moved to somewhere in the documentation, leaving just a pointer to it from the code, please just let me know where in the documentation it should go). The core idea of these principles, reflecting the way that the Bank Clearance Summary report already worked, is that the Bank Transaction Payment records in the Bank Transaction (that catalogue everything it is reconciled against) should consist only of documents that correspond one-to-one with individual GL Entry lines against the Account of the Bank Account of the Bank Transaction. As far as I can tell, in current ERPNext code there are only four DocTypes with this property: Payment Entry, Journal Entry Account (i.e. a single line in the accounts table of a Journal Entry), Sales Invoice Payment (i.e. a single line in the payments table of a Sales Invoice, used primarily for Point of Sale recordkeeping as far as I know), and Purchase Invoice (but only for those where the cash_bank_account is set for a direct payment; typically Purchase Invoices are resolved with Payment Entry records). So with this commit, those become the only four payment_document values added to the payments table of a Bank Transaction (although these changes do maintain legacy support for existing Bank Transactions with other payment_documents already listed in the database). Other DocTypes that stand for one or more payments, such as Journal Entry, Sales Invoice, other Purchase Invoice documents besides the direct payment sort mentioned above, and Expense Claim, can still be used in the Bank Reconciliation user interface for convenience (although the interface does show the individual component payments as well, allowing the user to reconcile all of them at once or a single one individually if desired). The core block of comments also catalogues all of the places in the code that are affected by the bank reconciliation process, in an effort to help ensure that any future changes to the process are hopefully made consistently throughout. Along the way, this commit provides numerous minor improvements to the reconciliation process: it is now possible to reconcile the statements from two different banks involved in a bank transfer against the same Journal Entry (since in fact they actually reconcile with separate Journal Entry Account lines in that transfer); the "Reconcile" buttons in the user interface are colored differently depending on whether the payment document was posted before or after the date of the Bank Transaction (with a third color for individual subpayments of a composite payment document); with a small parallel change to frappe.ui.Page in a separate PR in the Frappe project, the accumulation of "extra" field boxes on the top bar of a Bank Reconciliation if you click on "Reconcile This Account" multiple times is eliminated; also with that same change to frappe.ui.Page, this commit immediately filters the Bank Transactions in a Bank Reconciliation to just those matching the Bank Account of the Reconciliation; makes the payment amount information in the Reconciliation Tool more compact so that it can be displayed even on narrow screens; properly objects if the amount of the payment is larger than the Bank Transaction being reconciled; when searching for a payment, produces all associated Payment Entries for any specified Purchase Invoice or Expense Claim; correctly unclears payment entries when a payment row of a Bank Transaction is deleted; correctly fills in the Account value when a new Payment is created from a Bank Transaction; and allows for easy traversal from a reconciled Bank Transaction to the Journal Entry documents it is reconciled against, and vice versa.
gwhitney
force-pushed
the
reconcile_with_jea
branch
from
cb8f5a1
to
54aa21f
Compare
This comment was marked as abuse.
This comment was marked as abuse.
|
In re casesolved-co-uk's comment, I must say that I too was not originally trained as an accountant. That said, I have years of experience with not-for-profit accounting in practice, and none of the institutions I've ever worked with record transfers as two journal entries, nor have we ever had an auditor recommend doing it that way. Moreover, here's the top hit I got by websearching for "bookkeeping journal entry for bank transfer": https://support.accountingseed.com/hc/en-us/articles/217569887-Record-Bank-Transfers and it clearly instructs the reader to make one entry, crediting the "From" account and debiting the "To" account. So I stand by my position that there really isn't any technically correct way to reconcile Bank Transactions other than against individual Journal Entry Account rows rather than against entire Journal Entry records. Hence, the need for this PR, which unfortunately appears will have to be re-done from scratch once #24273 lands. |
This comment was marked as abuse.
This comment was marked as abuse.
|
Yes, well it seems pretty clear this will have to be completely redone, since it received no attention for three months and now there are a bunch of incompatible changes coming down the pipeline. I guess my plan is to wait for them to land, and then re-do this. I like your suggestion of trying to do it in clearer steps. I doubt that I can necessarily do it in multiple PRs -- maybe I can split it a bit that way -- but at least I can present it as a series of smaller, clearly-explained commits within the PR. Thanks! |
|
@gwhitney Glen, as the original sponsor of the Plaid integration (and related Bank Reconciliation code) I just want to take a minute to thank you for the incredible effort that you have put in here. You are clearly bringing a bunch of expertise in code/ accounting, and UI that is really valuable. Thanks so much for this contribution. |
This comment was marked as abuse.
This comment was marked as abuse.
That sounds to me like a recipe for disaster; redundant, potentially conflicting information in the database? In the intermediate stage, could a Bank Transaction reconcile against either a Journal Entry or a Journal Entry Account? How would one choose? I am very afraid that I would just make things much worse if I tried to enable reconciliation against Journal Entry Account records first and then afterwards disable reconciliation against Journal Entry records. But I do very much appreciate your efforts to make suggestions that could help this get in -- thank you! I will think about your ideas when things settle with respect to Bank Reconciliation and I make another crack at this. |
|
@MichaelPinkowski thank you for the encouraging words! |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
|
Right, we certainly have to reconcile both sides of a single Journal Entry fairly often. Thanks for that supportive comment, casesolved-co-uk! |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within a week if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
Prior to this change, a Bank Transaction would reconcile against an entire
Journal Entry. But a single Journal Entry (for example a transfer) could
have individual Journal Entry Accounts lines for multiple different bank
accounts. Thus, if the Bank Transactions from the statement of one Bank
Account had already been reconciled, the corresponding Journal Entry would
be marked as entirely cleared. Then when one later attempts to reconcile the
Bank Transactions from the other Bank Account on the other side of the
transfer, there is nothing "left" for the other side of the transfer to
reconcile with.
This commit improves the situation by moving the clearance_date field from
Journal Entry to Journal Entry Account, and reconciling Bank Transactions
against individual Journal Entry Account lines. This is a more realistic
representation of what is going on, allows for different clearance_date
values for the two different sides of non-same-day transfers, and alleviates
the problem of only being able to reconcile one side of a transfer.
Along the way, it also fixes several other issues: with a small parallel
change to frappe.ui.Page in a separate PR in that project, the accumulation
of "extra" field boxes on the top bar of a Bank Reconciliation if you click
on "Reconcile This Account" multiple times is eliminated; also with that
same change to frappe.ui.Page, this commit immediately filters the
Bank Transactions in a Bank Reconciliation to just those matching the
Bank Account of the Reconciliation; makes the payment amount information
in the Reconciliation Tool more compact so that it can be displayed even
on narrow screens; properly objects if the amount of the payment is larger
than the Bank Transaction being reconciled; when searching for a payment,
produces all associated Payment Entries for any specified Purchase Invoice
or Expense Claim; correctly unclears payment entries when a payment row of a
Bank Transaction is deleted; and allows for easy traversal from a reconciled
Bank Transaction to the Journal Entry documents it is reconciled against,
and vice versa.
Resolves #23660
Note that in my analysis of that issue, I inquired whether this (relatively minor) architectural change, of reconciling bank transactions with Journal Entry Accounts rather than with entire Journal Entry documents, would meet with the approval of ERPNext developers. Having no response and needing to proceed with our institution's bookkeeping, I thought I would go ahead and make the changes and offer them as a pull request. I think this PR offers a number of advantages over the previous state of Bank Reconciliation, and I hope that the developers find it to be helpful. Thank you for all of the effort you have put into ERPNext.