Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Date filters on bank reconciliation tool #33271

Merged

Conversation

sonali8848
Copy link
Contributor

@sonali8848 sonali8848 commented Dec 8, 2022

Bank.Reconciliation.Tool.mp4

Date filters in Bank Reconciliation Tool
Step 1-To apply date filter as per posting date in Bank Transactions and Vouchers and sorted bank transactions as per ascending order.
Screenshot 2022-12-08 170131
Step 2- Added checkbox of 'filtered as per reference date' and two fields 'from reference date' and 'to reference date' to filter by reference date. If the checkbox is ticked then the vouchers will be filtered and sorted as per the reference date.
Screenshot 2022-12-08 172838
Step 3- Added field of posting date in the dialog.
Screenshot 2022-12-08 173810
Step 4- Journal Entry checkbox is ticked by default.
Screenshot 2022-12-08 173810

Consolidated auto bank reconciliation

All the transactions will get auto-reconciled if the reference number matched with bank uploaded transaction and voucher posted, we can see that that pop message will show the count of transactions reconciled and if no transactions are reconciled then the pop will show “There is no matched reference number for reconciliation.
image
image

Applying date filter on transactions and all the bank entries and also gives the filter the bank entries as per reference date. Sorted all transactions and entries as per date in ascending order.
Also added posting date columns in all bank entries and default checkbox tick of journal entry, hide the sales invoice and purchase invoice checkbox.
Applying date filter on transactions and all the bank entries and also gives the filter the bank entries as per reference date. Sorted all transactions and entries as per date in ascending order. Also added posting date columns in all bank entries and default checkbox tick of journal entry, hide the sales invoice and purchase invoice checkbox.
Adding fields in bank reconciliation tool
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Dec 8, 2022
@sonali8848 sonali8848 changed the title Filters on bank reconciliation Feat:Filters on bank reconciliation Dec 14, 2022
Applying filters on Payement entries and Journal Entries as per reference  date and posting date
Copy link
Member

@deepeshgarg007 deepeshgarg007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Assuming the user will be filtering either by reference or posting date. So when filter by reference date is enabled those fields should hide
  • The commits are non-semantic, please add semantic comments
  • This is the first level review, will add more once this cleanup is done

@@ -51,6 +53,7 @@ def get_bank_transactions(bank_account, from_date=None, to_date=None):
],
filters=filters,
)
transactions = sorted(transactions, key=lambda x: x["date"]) if transactions else []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add a order by clause in query itself, this seems unnecessary

@@ -340,6 +343,9 @@ def get_linked_payments(bank_transaction_name, document_types=None):

def check_matching(bank_account, company, transaction, document_types):
# combine all types of vouchers
filtered_by_reference_date = frappe.db.get_single_value(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass from client side, don't make a DB call

Comment on lines 523 to 531
from_date = frappe.db.get_single_value("Bank Reconciliation Tool", "bank_statement_from_date")
to_date = frappe.db.get_single_value("Bank Reconciliation Tool", "bank_statement_to_date")
from_reference_date = frappe.db.get_single_value(
"Bank Reconciliation Tool", "from_reference_date"
)
to_reference_date = frappe.db.get_single_value("Bank Reconciliation Tool", "to_reference_date")
filtered_by_reference_date = frappe.db.get_single_value(
"Bank Reconciliation Tool", "filtered_by_reference_date"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass from the client side

erpnext/hooks.py Outdated
@@ -469,8 +469,8 @@
bank_reconciliation_doctypes = [
"Payment Entry",
"Journal Entry",
"Purchase Invoice",
"Sales Invoice",
# "Purchase Invoice",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are they commented?

Comment on lines 47 to 620
callback: (response) => {
const alert_string = __("Bank Transaction {0} added as Payment Entry", [this.bank_transaction.name]);
frappe.show_alert(alert_string);
this.update_dt_cards(response.message);
this.dialog.hide();
},
});
}

add_journal_entry(values) {
frappe.call({
method:
"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.create_journal_entry_bts",
args: {
bank_transaction_name: this.bank_transaction.name,
reference_number: values.reference_number,
reference_date: values.reference_date,
party_type: values.party_type,
party: values.party,
posting_date: values.posting_date,
mode_of_payment: values.mode_of_payment,
entry_type: values.journal_entry_type,
second_account: values.second_account,
},
callback: (response) => {
const alert_string = __("Bank Transaction {0} added as Journal Entry", [this.bank_transaction.name]);
frappe.show_alert(alert_string);
this.update_dt_cards(response.message);
this.dialog.hide();
},
});
}

update_transaction(values) {
frappe.call({
method:
"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.update_bank_transaction",
args: {
bank_transaction_name: this.bank_transaction.name,
reference_number: values.reference_number,
party_type: values.party_type,
party: values.party,
},
callback: (response) => {
const alert_string = __("Bank Transaction {0} updated", [this.bank_transaction.name]);
frappe.show_alert(alert_string);
this.update_dt_cards(response.message);
this.dialog.hide();
},
});
}

edit_in_full_page() {
const values = this.dialog.get_values(true);
if (values.document_type == "Payment Entry") {
frappe.call({
method:
"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.create_payment_entry_bts",
args: {
bank_transaction_name: this.bank_transaction.name,
reference_number: values.reference_number,
reference_date: values.reference_date,
party_type: values.party_type,
party: values.party,
posting_date: values.posting_date,
mode_of_payment: values.mode_of_payment,
project: values.project,
cost_center: values.cost_center,
allow_edit: true
},
callback: (r) => {
const doc = frappe.model.sync(r.message);
frappe.set_route("Form", doc[0].doctype, doc[0].name);
},
});
} else {
frappe.call({
method:
"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.create_journal_entry_bts",
args: {
bank_transaction_name: this.bank_transaction.name,
reference_number: values.reference_number,
reference_date: values.reference_date,
party_type: values.party_type,
party: values.party,
posting_date: values.posting_date,
mode_of_payment: values.mode_of_payment,
entry_type: values.journal_entry_type,
second_account: values.second_account,
allow_edit: true
},
callback: (r) => {
var doc = frappe.model.sync(r.message);
frappe.set_route("Form", doc[0].doctype, doc[0].name);
},
});
}
}

};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for, can't see any mention in the description. Please be more descriptive

Added date filters on bank transactions, payment entries and journal entries and sorted list as per date in ascending order.
@sonali8848 sonali8848 changed the title Feat:Filters on bank reconciliation Feat: Date filters on bank reconciliation Dec 21, 2022
@sonali8848 sonali8848 changed the title Feat: Date filters on bank reconciliation Feat: Date filters on bank reconciliation tool Dec 21, 2022
Comment on lines 163 to 165
filtered_by_reference_date:frm.doc.filtered_by_reference_date,
from_reference_date:frm.doc.from_reference_date,
to_reference_date:frm.doc.to_reference_date,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
filtered_by_reference_date:frm.doc.filtered_by_reference_date,
from_reference_date:frm.doc.from_reference_date,
to_reference_date:frm.doc.to_reference_date,
filtered_by_reference_date: frm.doc.filtered_by_reference_date,
from_reference_date: frm.doc.from_reference_date,
to_reference_date: frm.doc.to_reference_date,

# get matching payment entries query
if transaction.deposit > 0:
currency_field = "paid_to_account_currency as currency"
else:
currency_field = "paid_from_account_currency as currency"
filter_by_date = f"AND posting_date between '{from_date}' and '{to_date}'"
order_by = " posting_date"
if filtered_by_reference_date == "1":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be an int, why comparing with a string

# We have mapping at the bank level
# So one bank could have both types of bank accounts like asset and liability
# So cr_or_dr should be judged only on basis of withdrawal and deposit and not account type
cr_or_dr = "credit" if transaction.withdrawal > 0 else "debit"
# filter_by_date = f"AND je.posting_date between '{from_date}' and '{to_date}'"
order_by = " je.posting_date"
if filtered_by_reference_date == "1":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

@sonali8848
Copy link
Contributor Author

sonali8848 commented Dec 29, 2022 via email

@deepeshgarg007
Copy link
Member

@sonali8848 Please also add in the description why these changes were needed, what changes are added is well understood but why they were needed is still a bit confusing to me.

Unit tests also failing after the changes

@sonali8848
Copy link
Contributor Author

sonali8848 commented Dec 29, 2022 via email

On bank reconciliation, transactions will be filtered as per date selected in 'from_date' and 'to_date' fields , In dialog, all the bank entries will  be fetched as per the posting date selected and if filtered by reference date checkbox is tick then then there will be two fields 'from_reference_date' and 'to_reference_date' then all bank entries in dialog box came as per reference date, selected. And by default journal entry checkbox is tick.
Also sorted the bank transactions and bank entries as per ascending order date wise.
@deepeshgarg007
Copy link
Member

Okk, I will add the proper description, but unit testing is failing because we have passed the filters in get_linked_payments function, in test cases the filters are not passed that's why it is failing

So pass the filters from the backend, is there any issue you are facing with this?

@sonali8848
Copy link
Contributor Author

sonali8848 commented Dec 29, 2022 via email

passing from_date and to_date filters in  test_linked_payments and test_debit_credit_output  for unit testing
@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Merging #33271 (2327262) into develop (f598da7) will increase coverage by 0.17%.
The diff coverage is 28.57%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #33271      +/-   ##
===========================================
+ Coverage    63.97%   64.15%   +0.17%     
===========================================
  Files          818      819       +1     
  Lines        58617    59085     +468     
===========================================
+ Hits         37501    37905     +404     
- Misses       21116    21180      +64     
Impacted Files Coverage Δ
...nk_reconciliation_tool/bank_reconciliation_tool.py 44.12% <28.57%> (-6.62%) ⬇️
erpnext/erpnext_integrations/exotel_integration.py 68.04% <0.00%> (-3.73%) ⬇️
erpnext/erpnext_integrations/stripe_integration.py 16.21% <0.00%> (-1.97%) ⬇️
erpnext/accounts/report/cash_flow/cash_flow.py 41.34% <0.00%> (-1.52%) ⬇️
...t/support/doctype/warranty_claim/warranty_claim.py 31.25% <0.00%> (-1.11%) ⬇️
...nce/doctype/maintenance_visit/maintenance_visit.py 59.25% <0.00%> (-0.99%) ⬇️
...rpnext/assets/doctype/asset_repair/asset_repair.py 66.04% <0.00%> (-0.83%) ⬇️
erpnext/assets/doctype/asset/asset.py 81.48% <0.00%> (-0.77%) ⬇️
erpnext/crm/doctype/appointment/appointment.py 76.02% <0.00%> (-0.74%) ⬇️
erpnext/selling/doctype/customer/customer.py 73.61% <0.00%> (-0.66%) ⬇️
... and 64 more

Added a button of Auto Reconcile, to reconcile the bank entries as per the matching reference number with the bank transaction and count of transactions reconciled message will be pop up on clicking the auto reconcile button.
@sonali8848
Copy link
Contributor Author

sonali8848 commented Jan 2, 2023 via email

@deepeshgarg007
Copy link
Member

@sonali8848 Please also add in the description why these changes were needed, what changes are added is well understood but why they were needed is still a bit confusing to me.

@sonali8848 this is still unanswered, can you update this as well

@sonali8848
Copy link
Contributor Author

sonali8848 commented Jan 2, 2023 via email

Copy link
Member

@deepeshgarg007 deepeshgarg007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some code cleanup changes, please add a simple test case for the Auto Reconciliation use case and also a working GIF in the description

@@ -25,6 +25,24 @@ frappe.ui.form.on("Bank Reconciliation Tool", {
frappe.require("bank-reconciliation-tool.bundle.js", () =>
frm.trigger("make_reconciliation_tool")
);
frm.add_custom_button(__('Auto Reconcile'), function(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
frm.add_custom_button(__('Auto Reconcile'), function(){
frm.add_custom_button(__('Auto Reconcile'), function() {

@@ -25,6 +25,24 @@ frappe.ui.form.on("Bank Reconciliation Tool", {
frappe.require("bank-reconciliation-tool.bundle.js", () =>
frm.trigger("make_reconciliation_tool")
);
frm.add_custom_button(__('Auto Reconcile'), function(){
frappe.call({
method:"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.auto_reconcile_vouchers",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
method:"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.auto_reconcile_vouchers",
method: "erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.auto_reconcile_vouchers",

frappe.call({
method:"erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool.auto_reconcile_vouchers",
args: {
bank_account:frm.doc.bank_account,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bank_account:frm.doc.bank_account,
bank_account: frm.doc.bank_account,

Comment on lines 35 to 37
filtered_by_reference_date:frm.doc.filtered_by_reference_date,
from_reference_date:frm.doc.from_reference_date,
to_reference_date:frm.doc.to_reference_date,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
filtered_by_reference_date:frm.doc.filtered_by_reference_date,
from_reference_date:frm.doc.from_reference_date,
to_reference_date:frm.doc.to_reference_date,
filtered_by_reference_date: frm.doc.filtered_by_reference_date,
from_reference_date: frm.doc.from_reference_date,
to_reference_date: frm.doc.to_reference_date,

Comment on lines 301 to 302
# vouchers = frappe.as_json(voucherss)
# vouchers = json.loads(voucherss)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comments

transaction.update_allocations()
matched_transaction_len = len(set(matched_transaction))
if matched_transaction_len == 0:
frappe.msgprint(_("There is no any matched reference number for reconciliation"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
frappe.msgprint(_("There is no any matched reference number for reconciliation"))
frappe.msgprint(_("No matching references found for auto reconciliation"))

Comment on lines 25 to 26
from_date:this.bank_statement_from_date,
to_date:this.bank_statement_to_date
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from_date:this.bank_statement_from_date,
to_date:this.bank_statement_to_date
from_date: this.bank_statement_from_date,
to_date: this.bank_statement_to_date

@deepeshgarg007 deepeshgarg007 changed the title Feat: Date filters on bank reconciliation tool feat: Date filters on bank reconciliation tool Jan 12, 2023
@deepeshgarg007 deepeshgarg007 merged commit afb33f2 into frappe:develop Jan 12, 2023
@deepeshgarg007 deepeshgarg007 added the backport version-14-hotfix backport to version 14 label Jan 13, 2023
deepeshgarg007 added a commit that referenced this pull request Jan 15, 2023
* Update bank_reconciliation_tool.py

Applying date filter on transactions and all the bank entries and also gives the filter the bank entries as per reference date. Sorted all transactions and entries as per date in ascending order.
Also added posting date columns in all bank entries and default checkbox tick of journal entry, hide the sales invoice and purchase invoice checkbox.

(cherry picked from commit e5a1189)

* Filters on Bank Reconciliation

Applying date filter on transactions and all the bank entries and also gives the filter the bank entries as per reference date. Sorted all transactions and entries as per date in ascending order. Also added posting date columns in all bank entries and default checkbox tick of journal entry, hide the sales invoice and purchase invoice checkbox.

(cherry picked from commit 447272a)

* Update bank_reconciliation_tool.json

Adding fields in bank reconciliation tool

(cherry picked from commit 8e7c8a6)

* Feat:Filter on Payment Entries and Journal Entries

Applying filters on Payement entries and Journal Entries as per reference  date and posting date

(cherry picked from commit 408c89d)

* feat:filters on bank reconciliation

Added date filters on bank transactions, payment entries and journal entries and sorted list as per date in ascending order.

(cherry picked from commit 05b6fce)

* feat: added arguments of posting date and reference date

(cherry picked from commit 645869e)

* fix: linters

(cherry picked from commit 6b52763)

* fix: json issue

(cherry picked from commit 81e5f71)

* fix: filtered as per reference date

On bank reconciliation, transactions will be filtered as per date selected in 'from_date' and 'to_date' fields , In dialog, all the bank entries will  be fetched as per the posting date selected and if filtered by reference date checkbox is tick then then there will be two fields 'from_reference_date' and 'to_reference_date' then all bank entries in dialog box came as per reference date, selected. And by default journal entry checkbox is tick.
Also sorted the bank transactions and bank entries as per ascending order date wise.

(cherry picked from commit 3aaa2f5)

* fix: pre-commit

(cherry picked from commit e2614b8)

* fix: passing from_date and to_date filters in test cases

passing from_date and to_date filters in  test_linked_payments and test_debit_credit_output  for unit testing

(cherry picked from commit f181080)

* fix: pre-commit

(cherry picked from commit 35c29e0)

* fix: pre-commit

(cherry picked from commit c764f14)

* feat: consolidated auto bank reconciliation

Added a button of Auto Reconcile, to reconcile the bank entries as per the matching reference number with the bank transaction and count of transactions reconciled message will be pop up on clicking the auto reconcile button.

(cherry picked from commit d65243e)

* fix: data format

(cherry picked from commit 12822f7)

* fix: remove comments

(cherry picked from commit 917b219)

* chore: fix fieldnames and order

(cherry picked from commit 2327262)

Co-authored-by: sonali <sonali@8848digital.com>
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
frappe-pr-bot pushed a commit that referenced this pull request Jan 17, 2023
# [14.13.0](v14.12.1...v14.13.0) (2023-01-17)

### Bug Fixes

* allow to create sales order from expired quotation ([#33582](#33582)) ([fe51343](fe51343))
* asset repair link ([bc55f44](bc55f44))
* asset value in fixed asset register ([#33608](#33608)) ([4d2497f](4d2497f))
* attribute error while submitting Repost PLE ([0431a57](0431a57))
* better comparision of difference value between stock and account ([5869fcb](5869fcb))
* minor filter issue while reconciliation tool from bench console ([bddf330](bddf330))
* Missing constructor args in Bank Reco Tool ([#33705](#33705)) ([f88c8c4](f88c8c4))
* only group similar items in print format if group_same_items is checked in pick list (backport [#33627](#33627)) ([#33630](#33630)) ([28f2d35](28f2d35))
* patch item_reposting_for_incorrect_sl_and_gl ([1928195](1928195))
* Rate from LDC in TDS reports (backport [#33699](#33699)) ([#33700](#33700)) ([9fa4c1a](9fa4c1a))
* Return against internal purchase invoice (backport [#33635](#33635)) ([#33658](#33658)) ([35fbd67](35fbd67))
* Sales ORder Connections on Material Request ([8a04031](8a04031))
* Updating SO throws ordered_qty not allowed to change after submission ([f915c18](f915c18))
* zero rm-cost in SCR ([2dfbc6e](2dfbc6e))

### Features

* Date filters on bank reconciliation tool ([#33271](#33271)) ([91b08f1](91b08f1))
* provision to select date type based on filter ([4d65d6f](4d65d6f))

### Performance Improvements

* improve reconciliation speed on JE's with 1000's of rows ([8a498ed](8a498ed))

### Reverts

* Reverting changes done on 33495 ([#33662](#33662)) ([23b9f66](23b9f66))
@barredterra barredterra mentioned this pull request Mar 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants