From 752a92bd8be6d25325b01e4780f1b37b2195da87 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 6 Jun 2023 18:59:07 +0530 Subject: [PATCH] chore: Remove Bank Party Mapper implementation - Matching by Acc No/IBAN can easily happen with Bank Accounts. It's not a tedious query - Historical lookups for Party Name/Desc match are very tricky. The user could have manually set a match and we would not know. Also this leaves the Bank Party Mapper only useful for Party Name/Desc lookups, which feels excessive. - We want to reduce the number of places the same data is stored and reduce confusion - The Party Name/Desc will optionally happen fuzzily, or not at all - There will be no Mapper lookups --- .../doctype/bank_party_mapper/__init__.py | 0 .../bank_party_mapper/bank_party_mapper.js | 10 -- .../bank_party_mapper/bank_party_mapper.json | 80 ----------- .../bank_party_mapper/bank_party_mapper.py | 24 ---- .../test_bank_party_mapper.py | 9 -- .../bank_transaction/auto_match_party.py | 135 ++++-------------- .../bank_transaction/bank_transaction.json | 13 +- .../bank_transaction/bank_transaction.py | 34 +---- .../bank_transaction/test_auto_match_party.py | 79 ---------- 9 files changed, 37 insertions(+), 347 deletions(-) delete mode 100644 erpnext/accounts/doctype/bank_party_mapper/__init__.py delete mode 100644 erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.js delete mode 100644 erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.json delete mode 100644 erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.py delete mode 100644 erpnext/accounts/doctype/bank_party_mapper/test_bank_party_mapper.py diff --git a/erpnext/accounts/doctype/bank_party_mapper/__init__.py b/erpnext/accounts/doctype/bank_party_mapper/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.js b/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.js deleted file mode 100644 index b13e46a0e7..0000000000 --- a/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.js +++ /dev/null @@ -1,10 +0,0 @@ -// Copyright (c) 2023, Frappe Technologies Pvt. Ltd. and contributors -// For license information, please see license.txt - -frappe.ui.form.on("Bank Party Mapper", { - refresh(frm) { - if (!frm.is_new()) { - frm.set_intro(__("Please avoid editing unless you are absolutely certain.")); - } - }, -}); diff --git a/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.json b/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.json deleted file mode 100644 index ddc79f24a0..0000000000 --- a/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.json +++ /dev/null @@ -1,80 +0,0 @@ -{ - "actions": [], - "creation": "2023-03-31 10:48:20.249481", - "default_view": "List", - "doctype": "DocType", - "editable_grid": 1, - "engine": "InnoDB", - "field_order": [ - "party_type", - "party", - "column_break_wbna", - "bank_party_name", - "bank_party_account_number", - "bank_party_iban" - ], - "fields": [ - { - "fieldname": "bank_party_account_number", - "fieldtype": "Data", - "in_list_view": 1, - "label": "Party Account No. (Bank Statement)" - }, - { - "fieldname": "bank_party_iban", - "fieldtype": "Data", - "in_list_view": 1, - "label": "Party IBAN (Bank Statement)" - }, - { - "fieldname": "column_break_wbna", - "fieldtype": "Column Break" - }, - { - "fieldname": "party_type", - "fieldtype": "Link", - "in_list_view": 1, - "in_standard_filter": 1, - "label": "Party Type", - "options": "DocType" - }, - { - "fieldname": "party", - "fieldtype": "Dynamic Link", - "in_list_view": 1, - "in_standard_filter": 1, - "label": "Party", - "options": "party_type" - }, - { - "fieldname": "bank_party_name", - "fieldtype": "Data", - "label": "Party Name (Bank Statement)" - } - ], - "index_web_pages_for_search": 1, - "links": [], - "modified": "2023-04-04 14:27:23.450456", - "modified_by": "Administrator", - "module": "Accounts", - "name": "Bank Party Mapper", - "owner": "Administrator", - "permissions": [ - { - "create": 1, - "delete": 1, - "email": 1, - "export": 1, - "print": 1, - "read": 1, - "report": 1, - "role": "System Manager", - "share": 1, - "write": 1 - } - ], - "sort_field": "modified", - "sort_order": "DESC", - "states": [], - "track_changes": 1 -} \ No newline at end of file diff --git a/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.py b/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.py deleted file mode 100644 index 02e36b08fe..0000000000 --- a/erpnext/accounts/doctype/bank_party_mapper/bank_party_mapper.py +++ /dev/null @@ -1,24 +0,0 @@ -# Copyright (c) 2023, Frappe Technologies Pvt. Ltd. and contributors -# For license information, please see license.txt - -import frappe -from frappe.model.document import Document - - -class BankPartyMapper(Document): - def on_update(self): - self.update_party_in_linked_transactions() - - def update_party_in_linked_transactions(self): - if self.is_new(): - return - - # Set updated party values in other linked bank transactions - bank_transaction = frappe.qb.DocType("Bank Transaction") - - frappe.qb.update(bank_transaction).set("party_type", self.party_type).set( - "party", self.party - ).where( - (bank_transaction.bank_party_mapper == self.name) - & ((bank_transaction.party_type != self.party_type) | (bank_transaction.party != self.party)) - ).run() diff --git a/erpnext/accounts/doctype/bank_party_mapper/test_bank_party_mapper.py b/erpnext/accounts/doctype/bank_party_mapper/test_bank_party_mapper.py deleted file mode 100644 index c05b23f1a5..0000000000 --- a/erpnext/accounts/doctype/bank_party_mapper/test_bank_party_mapper.py +++ /dev/null @@ -1,9 +0,0 @@ -# Copyright (c) 2023, Frappe Technologies Pvt. Ltd. and Contributors -# See license.txt - -# import frappe -from frappe.tests.utils import FrappeTestCase - - -class TestBankPartyMapper(FrappeTestCase): - pass diff --git a/erpnext/accounts/doctype/bank_transaction/auto_match_party.py b/erpnext/accounts/doctype/bank_transaction/auto_match_party.py index 753f0c1171..5d94a08f2f 100644 --- a/erpnext/accounts/doctype/bank_transaction/auto_match_party.py +++ b/erpnext/accounts/doctype/bank_transaction/auto_match_party.py @@ -10,22 +10,7 @@ class AutoMatchParty: Matches by Account/IBAN and then by Party Name/Description sequentially. Returns when a result is obtained. - Result (if present) is of the form: (Party Type, Party, Mapper,) - - Mapper(if present) is one of the forms: - 1. {"mapper_name": }: Indicates that an existing Bank Party Mapper matched against - the transaction and the same must be linked in the Bank Transaction. - - 2. {"bank_party_account_number": , "bank_party_iban": } : Indicates that a match was - found in Customer/Supplier/Employee by account details. A Bank Party Mapper is now created - mapping the Party to the Account No./IBAN - - 3. {"bank_party_name": }: Indicates that a match was found in - Customer/Supplier/Employee by party name. A Bank Party Mapper is now created mapping the Party - to the Party Name (Bank Statement). If matched by Description, no mapper is created as - description is not a static key. - - Mapper data is used either to create a new Bank Party Mapper or link an existing mapper to a transaction. + Result (if present) is of the form: (Party Type, Party,) """ def __init__(self, **kwargs) -> None: @@ -44,7 +29,7 @@ class AutoMatchParty: fuzzy_matching_enabled = frappe.db.get_single_value("Accounts Settings", "enable_fuzzy_matching") if not result and fuzzy_matching_enabled: - result = AutoMatchbyPartyDescription( + result = AutoMatchbyPartyNameDescription( bank_party_name=self.bank_party_name, description=self.description, deposit=self.deposit ).match() @@ -62,50 +47,16 @@ class AutoMatchbyAccountIBAN: if not (self.bank_party_account_number or self.bank_party_iban): return None - result = self.match_account_in_bank_party_mapper() - if not result: - result = self.match_account_in_party() - - return result - - def match_account_in_bank_party_mapper(self) -> Union[Tuple, None]: - """Check for a IBAN/Account No. match in Bank Party Mapper""" - result = None - or_filters = {} - if self.bank_party_account_number: - or_filters["bank_party_account_number"] = self.bank_party_account_number - - if self.bank_party_iban: - or_filters["bank_party_iban"] = self.bank_party_iban - - mapper = frappe.db.get_all( - "Bank Party Mapper", - or_filters=or_filters, - fields=["party_type", "party", "name"], - limit_page_length=1, - ) - if mapper: - data = mapper[0] - return (data["party_type"], data["party"], {"mapper_name": data["name"]}) - + result = self.match_account_in_party() return result def match_account_in_party(self) -> Union[Tuple, None]: """Check if there is a IBAN/Account No. match in Customer/Supplier/Employee""" result = None - - parties = ["Supplier", "Employee", "Customer"] # most -> least likely to receive - if flt(self.deposit) > 0: - parties = ["Customer", "Supplier", "Employee"] # most -> least likely to pay + parties = get_parties_in_order(self.deposit) + or_filters = self.get_or_filters() for party in parties: - or_filters = {} - if self.bank_party_account_number: - or_filters["bank_account_no"] = self.bank_party_account_number - - if self.bank_party_iban: - or_filters["iban"] = self.bank_party_iban - party_result = frappe.db.get_all( "Bank Account", or_filters=or_filters, pluck="party", limit_page_length=1 ) @@ -123,17 +74,23 @@ class AutoMatchbyAccountIBAN: result = ( party, party_result[0], - { - "bank_party_account_number": self.get("bank_party_account_number"), - "bank_party_iban": self.get("bank_party_iban"), - }, ) break return result + def get_or_filters(self) -> dict: + or_filters = {} + if self.bank_party_account_number: + or_filters["bank_account_no"] = self.bank_party_account_number -class AutoMatchbyPartyDescription: + if self.bank_party_iban: + or_filters["iban"] = self.bank_party_iban + + return or_filters + + +class AutoMatchbyPartyNameDescription: def __init__(self, **kwargs) -> None: self.__dict__.update(kwargs) @@ -141,52 +98,21 @@ class AutoMatchbyPartyDescription: return self.__dict__.get(key, None) def match(self) -> Union[Tuple, None]: - # Match by Customer, Supplier or Employee Name - # search bank party mapper by party # fuzzy search by customer/supplier & employee if not (self.bank_party_name or self.description): return None - result = self.match_party_name_in_bank_party_mapper() - - if not result: - result = self.match_party_name_desc_in_party() - - return result - - def match_party_name_in_bank_party_mapper(self) -> Union[Tuple, None]: - """Check if match exists for party name in Bank Party Mapper""" - result = None - if not self.bank_party_name: - return - - mapper_res = frappe.get_all( - "Bank Party Mapper", - filters={"bank_party_name": self.bank_party_name}, - fields=["party_type", "party", "name"], - limit_page_length=1, - ) - if mapper_res: - mapper_res = mapper_res[0] - return ( - mapper_res["party_type"], - mapper_res["party"], - {"mapper_name": mapper_res["name"]}, - ) - + result = self.match_party_name_desc_in_party() return result def match_party_name_desc_in_party(self) -> Union[Tuple, None]: """Fuzzy search party name and/or description against parties in the system""" result = None - parties = ["Supplier", "Employee", "Customer"] # most-least likely to receive - if flt(self.deposit) > 0.0: - parties = ["Customer", "Supplier", "Employee"] # most-least likely to pay + parties = get_parties_in_order(self.deposit) for party in parties: - name_field = party.lower() + "_name" filters = {"status": "Active"} if party == "Employee" else {"disabled": 0} - names = frappe.get_all(party, filters=filters, pluck=name_field) + names = frappe.get_all(party, filters=filters, pluck=party.lower() + "_name") for field in ["bank_party_name", "description"]: if not self.get(field): @@ -197,8 +123,7 @@ class AutoMatchbyPartyDescription: break if result or skip: - # We skip if: - # If it was hard to distinguish between close matches and so match is None + # Skip If: It was hard to distinguish between close matches and so match is None # OR if the right match was found break @@ -206,19 +131,15 @@ class AutoMatchbyPartyDescription: def fuzzy_search_and_return_result(self, party, names, field) -> Union[Tuple, None]: skip = False - result = process.extract(query=self.get(field), choices=names, scorer=fuzz.token_set_ratio) party_name, skip = self.process_fuzzy_result(result) if not party_name: return None, skip - # Dont set description as a key in Bank Party Mapper due to its volatility - mapper = {"bank_party_name": self.get(field)} if field == "bank_party_name" else None return ( party, party_name, - mapper, ), skip def process_fuzzy_result(self, result: Union[list, None]): @@ -226,7 +147,7 @@ class AutoMatchbyPartyDescription: If there are multiple valid close matches return None as result may be faulty. Return the result only if one accurate match stands out. - Returns: Result, Skip (whether or not to continue matching) + Returns: Result, Skip (whether or not to discontinue matching) """ PARTY, SCORE, CUTOFF = 0, 1, 80 @@ -234,18 +155,24 @@ class AutoMatchbyPartyDescription: return None, False first_result = result[0] - if len(result) == 1: - return (result[0][PARTY] if first_result[SCORE] > CUTOFF else None), True + return (first_result[PARTY] if first_result[SCORE] > CUTOFF else None), True second_result = result[1] - if first_result[SCORE] > CUTOFF: # If multiple matches with the same score, return None but discontinue matching - # Matches were found but were too closes to distinguish between + # Matches were found but were too close to distinguish between if first_result[SCORE] == second_result[SCORE]: return None, True return first_result[PARTY], True else: return None, False + + +def get_parties_in_order(deposit: float) -> list: + parties = ["Supplier", "Employee", "Customer"] # most -> least likely to receive + if flt(deposit) > 0: + parties = ["Customer", "Supplier", "Employee"] # most -> least likely to pay + + return parties diff --git a/erpnext/accounts/doctype/bank_transaction/bank_transaction.json b/erpnext/accounts/doctype/bank_transaction/bank_transaction.json index e7de71aef4..b32022e6fd 100644 --- a/erpnext/accounts/doctype/bank_transaction/bank_transaction.json +++ b/erpnext/accounts/doctype/bank_transaction/bank_transaction.json @@ -37,8 +37,7 @@ "column_break_3czf", "bank_party_name", "bank_party_account_number", - "bank_party_iban", - "bank_party_mapper" + "bank_party_iban" ], "fields": [ { @@ -226,19 +225,11 @@ "fieldname": "bank_party_account_number", "fieldtype": "Data", "label": "Party Account No. (Bank Statement)" - }, - { - "fieldname": "bank_party_mapper", - "fieldtype": "Link", - "hidden": 1, - "label": "Bank Party Mapper", - "options": "Bank Party Mapper", - "read_only": 1 } ], "is_submittable": 1, "links": [], - "modified": "2023-05-17 14:56:10.547480", + "modified": "2023-06-06 13:58:12.821411", "modified_by": "Administrator", "module": "Accounts", "name": "Bank Transaction", diff --git a/erpnext/accounts/doctype/bank_transaction/bank_transaction.py b/erpnext/accounts/doctype/bank_transaction/bank_transaction.py index 04c3013a00..f82337fbd7 100644 --- a/erpnext/accounts/doctype/bank_transaction/bank_transaction.py +++ b/erpnext/accounts/doctype/bank_transaction/bank_transaction.py @@ -29,9 +29,6 @@ class BankTransaction(StatusUpdater): self.update_allocations() self._saving_flag = False - if frappe.db.get_single_value("Accounts Settings", "enable_party_matching"): - self.update_automatch_bank_party_mapper() - def on_cancel(self): self.clear_linked_payment_entries(for_cancel=True) self.set_status(update=True) @@ -167,33 +164,10 @@ class BankTransaction(StatusUpdater): ).match() if result: - party_type, party, mapper = result - to_update = {"party_type": party_type, "party": party} - - if mapper and mapper.get("mapper_name"): - # Transaction matched with an existing Bank party Mapper record - to_update["bank_party_mapper"] = mapper.get("mapper_name") - elif mapper: - # Make new Mapper record to remember match - mapper_doc = frappe.get_doc( - {"doctype": "Bank Party Mapper", "party_type": party_type, "party": party} - ) - mapper_doc.update(mapper) - mapper_doc.insert() - to_update["bank_party_mapper"] = mapper_doc.name - - frappe.db.set_value("Bank Transaction", self.name, field=to_update) - - def update_automatch_bank_party_mapper(self): - """Update Bank Party Mapper if Party Type & Party are manually changed after submit.""" - doc_before_update = self.get_doc_before_save() - party_type_changed = self.party_type and (doc_before_update.party_type != self.party_type) - party_changed = self.party and (doc_before_update.party != self.party) - - if (party_type_changed or party_changed) and self.bank_party_mapper: - mapper_doc = frappe.get_doc("Bank Party Mapper", self.bank_party_mapper) - mapper_doc.update({"party_type": self.party_type, "party": self.party}) - mapper_doc.save() + party_type, party = result + frappe.db.set_value( + "Bank Transaction", self.name, field={"party_type": party_type, "party": party} + ) @frappe.whitelist() diff --git a/erpnext/accounts/doctype/bank_transaction/test_auto_match_party.py b/erpnext/accounts/doctype/bank_transaction/test_auto_match_party.py index 8c6dc9d766..a498545f2a 100644 --- a/erpnext/accounts/doctype/bank_transaction/test_auto_match_party.py +++ b/erpnext/accounts/doctype/bank_transaction/test_auto_match_party.py @@ -22,7 +22,6 @@ class TestAutoMatchParty(FrappeTestCase): frappe.db.set_single_value("Accounts Settings", "enable_fuzzy_matching", 0) def test_match_by_account_number(self): - """Test if transaction matches with existing (Bank Party Mapper) or new match.""" create_supplier_for_match(account_no="000000003716541159") doc = create_bank_transaction( withdrawal=1200, @@ -33,22 +32,6 @@ class TestAutoMatchParty(FrappeTestCase): self.assertEqual(doc.party_type, "Supplier") self.assertEqual(doc.party, "John Doe & Co.") - self.assertTrue(doc.bank_party_mapper) - - # Check if Bank Party Mapper is created to remember mapping - bank_party_mapper = frappe.get_doc("Bank Party Mapper", doc.bank_party_mapper) - self.assertEqual(bank_party_mapper.party, "John Doe & Co.") - self.assertEqual(bank_party_mapper.bank_party_account_number, "000000003716541159") - self.assertEqual(bank_party_mapper.bank_party_iban, "DE02000000003716541159") - - # Check if created mapping is used for quick match - doc_2 = create_bank_transaction( - withdrawal=500, - transaction_id="602413b8ji8bf838fub8f2c6a39bah7y", - account_no="000000003716541159", - ) - self.assertEqual(doc_2.party, "John Doe & Co.") - self.assertEqual(doc_2.bank_party_mapper, bank_party_mapper.name) def test_match_by_iban(self): create_supplier_for_match(iban="DE02000000003716541159") @@ -61,12 +44,6 @@ class TestAutoMatchParty(FrappeTestCase): self.assertEqual(doc.party_type, "Supplier") self.assertEqual(doc.party, "John Doe & Co.") - self.assertTrue(doc.bank_party_mapper) - - bank_party_mapper = frappe.get_doc("Bank Party Mapper", doc.bank_party_mapper) - self.assertEqual(bank_party_mapper.party, "John Doe & Co.") - self.assertEqual(bank_party_mapper.bank_party_account_number, "000000003716541159") - self.assertEqual(bank_party_mapper.bank_party_iban, "DE02000000003716541159") def test_match_by_party_name(self): create_supplier_for_match(supplier_name="Jackson Ella W.") @@ -78,22 +55,6 @@ class TestAutoMatchParty(FrappeTestCase): ) self.assertEqual(doc.party_type, "Supplier") self.assertEqual(doc.party, "Jackson Ella W.") - self.assertTrue(doc.bank_party_mapper) - - bank_party_mapper = frappe.get_doc("Bank Party Mapper", doc.bank_party_mapper) - self.assertEqual(bank_party_mapper.party, "Jackson Ella W.") - self.assertEqual(bank_party_mapper.bank_party_name, "Ella Jackson") - self.assertEqual(bank_party_mapper.bank_party_iban, None) - - # Check if created mapping is used for quick match - doc_2 = create_bank_transaction( - withdrawal=500, - transaction_id="578313b8ji8bf838fub8f2c6a39bah7y", - party_name="Ella Jackson", - account_no="000000004316531152", - ) - self.assertEqual(doc_2.party, "Jackson Ella W.") - self.assertEqual(doc_2.bank_party_mapper, bank_party_mapper.name) def test_match_by_description(self): create_supplier_for_match(supplier_name="Microsoft") @@ -107,46 +68,6 @@ class TestAutoMatchParty(FrappeTestCase): self.assertEqual(doc.party, "Microsoft") self.assertFalse(doc.bank_party_mapper) - def test_correct_match_after_submit(self): - """Correct wrong mapping after submit. Test impact.""" - # Similar named suppliers - create_supplier_for_match(supplier_name="Amazon") - create_supplier_for_match(supplier_name="Amazing Co.") - - # Bank Transactions actually from "Amazon" that match better with "Amazing Co." - doc = create_bank_transaction( - description="visa06323202 amzn.com/bill 7,88eur1,5324711959 90.22. 1,62 87861003", - withdrawal=24.85, - transaction_id="3a1da4ee2dc5a980138d36ef5297cbd9", - party_name="Amazn Co.", - ) - doc_2 = create_bank_transaction( - description="visa61268005 amzn.com/bill 22,345eur1,7654711959 89.23. 1,64 61268005", - withdrawal=80, - transaction_id="584314e459b00f792bfd569267efba6e", - party_name="Amazn Co.", - ) - - self.assertEqual(doc.party_type, "Supplier") - self.assertEqual(doc.party, "Amazing Co.") - self.assertTrue(doc.bank_party_mapper) - self.assertTrue(doc_2.bank_party_mapper, doc.bank_party_mapper) - - bank_party_mapper = frappe.get_doc("Bank Party Mapper", doc.bank_party_mapper) - self.assertEqual(bank_party_mapper.party, "Amazing Co.") - self.assertEqual(bank_party_mapper.bank_party_name, "Amazn Co.") - - # User corrects the value after submit to "Amazon" - doc.party = "Amazon" - doc.save() - bank_party_mapper.reload() - doc_2.reload() - - # Mapping is edited and all transactions with this mapping are updated - self.assertEqual(bank_party_mapper.party, "Amazon") - self.assertEqual(bank_party_mapper.bank_party_name, "Amazn Co.") - self.assertEqual(doc_2.party, "Amazon") - def test_skip_match_if_multiple_close_results(self): create_supplier_for_match(supplier_name="Adithya Medical & General Stores") create_supplier_for_match(supplier_name="Adithya Medical And General Stores")