From 136466d255651ba29be16248e822c2a374114c67 Mon Sep 17 00:00:00 2001 From: Saqib Ansari Date: Tue, 29 Mar 2022 10:47:27 +0530 Subject: [PATCH 1/8] fix(asset): do not validate warehouse on asset purchase --- erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py index 7654aa44dc6..d1e7fb40d12 100644 --- a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py @@ -226,7 +226,7 @@ class PurchaseInvoice(BuyingController): def validate_warehouse(self, for_validate=True): if self.update_stock and for_validate: for d in self.get('items'): - if not d.warehouse: + if not d.warehouse and not d.is_fixed_asset: frappe.throw(_("Row No {0}: Warehouse is required. Please set a Default Warehouse for Item {1} and Company {2}"). format(d.idx, d.item_code, self.company), exc=WarehouseMissingError) From 6528218ac31001e04e6b5ebfa0f3d429e296742f Mon Sep 17 00:00:00 2001 From: Saqib Ansari Date: Tue, 29 Mar 2022 18:43:33 +0530 Subject: [PATCH 2/8] perf: skip warehouse validation for non-stock items --- erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py | 3 ++- erpnext/controllers/accounts_controller.py | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py index 84077f6ac0b..e6a46d0676b 100644 --- a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py @@ -249,8 +249,9 @@ class PurchaseInvoice(BuyingController): def validate_warehouse(self, for_validate=True): if self.update_stock and for_validate: + stock_items = self.get_stock_items() for d in self.get("items"): - if not d.warehouse and not d.is_fixed_asset: + if not d.warehouse and d.item_code in stock_items: frappe.throw( _( "Row No {0}: Warehouse is required. Please set a Default Warehouse for Item {1} and Company {2}" diff --git a/erpnext/controllers/accounts_controller.py b/erpnext/controllers/accounts_controller.py index 72ac1b37ef6..3ad61d4dc24 100644 --- a/erpnext/controllers/accounts_controller.py +++ b/erpnext/controllers/accounts_controller.py @@ -1264,6 +1264,9 @@ class AccountsController(TransactionBase): return get_company_default(self.company, fieldname, ignore_validation=ignore_validation) def get_stock_items(self): + if hasattr(self, "_stock_items") and self._stock_items: + return self._stock_items + stock_items = [] item_codes = list(set(item.item_code for item in self.get("items"))) if item_codes: @@ -1279,6 +1282,7 @@ class AccountsController(TransactionBase): ) ] + self._stock_items = stock_items return stock_items def set_total_advance_paid(self): From d4cc7c553eaec88edb128da3224cb94650cc12ed Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Wed, 30 Mar 2022 12:43:18 +0530 Subject: [PATCH 3/8] fix: Account currency validation --- erpnext/accounts/doctype/account/account.py | 4 +- .../accounts/doctype/account/test_account.py | 76 ++++--------------- 2 files changed, 19 insertions(+), 61 deletions(-) diff --git a/erpnext/accounts/doctype/account/account.py b/erpnext/accounts/doctype/account/account.py index 150f68b7bd3..c71ea3648b9 100644 --- a/erpnext/accounts/doctype/account/account.py +++ b/erpnext/accounts/doctype/account/account.py @@ -204,7 +204,9 @@ class Account(NestedSet): if not self.account_currency: self.account_currency = frappe.get_cached_value("Company", self.company, "default_currency") - elif self.account_currency != frappe.db.get_value("Account", self.name, "account_currency"): + gl_currency = frappe.db.get_value("GL Entry", {"account": self.name}, "account_currency") + + if gl_currency and self.account_currency != gl_currency: if frappe.db.get_value("GL Entry", {"account": self.name}): frappe.throw(_("Currency can not be changed after making entries using some other currency")) diff --git a/erpnext/accounts/doctype/account/test_account.py b/erpnext/accounts/doctype/account/test_account.py index efc063de56a..a6d44882eb6 100644 --- a/erpnext/accounts/doctype/account/test_account.py +++ b/erpnext/accounts/doctype/account/test_account.py @@ -241,71 +241,27 @@ class TestAccount(unittest.TestCase): for doc in to_delete: frappe.delete_doc("Account", doc) + def test_validate_account_currency(self): + from erpnext.accounts.doctype.journal_entry.test_journal_entry import make_journal_entry -def _make_test_records(verbose=None): - from frappe.test_runner import make_test_objects + if not frappe.db.get_value("Account", "Test Currency Account - _TC"): + acc = frappe.new_doc("Account") + acc.account_name = "Test Currency Account" + acc.parent_account = "Tax Assets - _TC" + acc.company = "_Test Company" + acc.insert() + else: + acc = frappe.get_doc("Account", "Test Currency Account - _TC") - accounts = [ - # [account_name, parent_account, is_group] - ["_Test Bank", "Bank Accounts", 0, "Bank", None], - ["_Test Bank USD", "Bank Accounts", 0, "Bank", "USD"], - ["_Test Bank EUR", "Bank Accounts", 0, "Bank", "EUR"], - ["_Test Cash", "Cash In Hand", 0, "Cash", None], - ["_Test Account Stock Expenses", "Direct Expenses", 1, None, None], - ["_Test Account Shipping Charges", "_Test Account Stock Expenses", 0, "Chargeable", None], - ["_Test Account Customs Duty", "_Test Account Stock Expenses", 0, "Tax", None], - ["_Test Account Insurance Charges", "_Test Account Stock Expenses", 0, "Chargeable", None], - ["_Test Account Stock Adjustment", "_Test Account Stock Expenses", 0, "Stock Adjustment", None], - ["_Test Employee Advance", "Current Liabilities", 0, None, None], - ["_Test Account Tax Assets", "Current Assets", 1, None, None], - ["_Test Account VAT", "_Test Account Tax Assets", 0, "Tax", None], - ["_Test Account Service Tax", "_Test Account Tax Assets", 0, "Tax", None], - ["_Test Account Reserves and Surplus", "Current Liabilities", 0, None, None], - ["_Test Account Cost for Goods Sold", "Expenses", 0, None, None], - ["_Test Account Excise Duty", "_Test Account Tax Assets", 0, "Tax", None], - ["_Test Account Education Cess", "_Test Account Tax Assets", 0, "Tax", None], - ["_Test Account S&H Education Cess", "_Test Account Tax Assets", 0, "Tax", None], - ["_Test Account CST", "Direct Expenses", 0, "Tax", None], - ["_Test Account Discount", "Direct Expenses", 0, None, None], - ["_Test Write Off", "Indirect Expenses", 0, None, None], - ["_Test Exchange Gain/Loss", "Indirect Expenses", 0, None, None], - ["_Test Account Sales", "Direct Income", 0, None, None], - # related to Account Inventory Integration - ["_Test Account Stock In Hand", "Current Assets", 0, None, None], - # fixed asset depreciation - ["_Test Fixed Asset", "Current Assets", 0, "Fixed Asset", None], - ["_Test Accumulated Depreciations", "Current Assets", 0, "Accumulated Depreciation", None], - ["_Test Depreciations", "Expenses", 0, None, None], - ["_Test Gain/Loss on Asset Disposal", "Expenses", 0, None, None], - # Receivable / Payable Account - ["_Test Receivable", "Current Assets", 0, "Receivable", None], - ["_Test Payable", "Current Liabilities", 0, "Payable", None], - ["_Test Receivable USD", "Current Assets", 0, "Receivable", "USD"], - ["_Test Payable USD", "Current Liabilities", 0, "Payable", "USD"], - ] + self.assertEqual(acc.account_currency, "INR") - for company, abbr in [ - ["_Test Company", "_TC"], - ["_Test Company 1", "_TC1"], - ["_Test Company with perpetual inventory", "TCP1"], - ]: - test_objects = make_test_objects( - "Account", - [ - { - "doctype": "Account", - "account_name": account_name, - "parent_account": parent_account + " - " + abbr, - "company": company, - "is_group": is_group, - "account_type": account_type, - "account_currency": currency, - } - for account_name, parent_account, is_group, account_type, currency in accounts - ], + # Make a JV against this account + make_journal_entry( + "Test Currency Account - _TC", "Miscellaneous Expenses - _TC", 100, submit=True ) - return test_objects + acc.account_currency = "USD" + self.assertRaises(frappe.ValidationError, acc.save) def get_inventory_account(company, warehouse=None): From d93edbc859268fee20be208f84d66b7542bd52ee Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Wed, 30 Mar 2022 21:07:56 +0530 Subject: [PATCH 4/8] fix: make test record --- .../accounts/doctype/account/test_account.py | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/erpnext/accounts/doctype/account/test_account.py b/erpnext/accounts/doctype/account/test_account.py index a6d44882eb6..f9c9173af08 100644 --- a/erpnext/accounts/doctype/account/test_account.py +++ b/erpnext/accounts/doctype/account/test_account.py @@ -264,6 +264,72 @@ class TestAccount(unittest.TestCase): self.assertRaises(frappe.ValidationError, acc.save) +def _make_test_records(verbose=None): + from frappe.test_runner import make_test_objects + + accounts = [ + # [account_name, parent_account, is_group] + ["_Test Bank", "Bank Accounts", 0, "Bank", None], + ["_Test Bank USD", "Bank Accounts", 0, "Bank", "USD"], + ["_Test Bank EUR", "Bank Accounts", 0, "Bank", "EUR"], + ["_Test Cash", "Cash In Hand", 0, "Cash", None], + ["_Test Account Stock Expenses", "Direct Expenses", 1, None, None], + ["_Test Account Shipping Charges", "_Test Account Stock Expenses", 0, "Chargeable", None], + ["_Test Account Customs Duty", "_Test Account Stock Expenses", 0, "Tax", None], + ["_Test Account Insurance Charges", "_Test Account Stock Expenses", 0, "Chargeable", None], + ["_Test Account Stock Adjustment", "_Test Account Stock Expenses", 0, "Stock Adjustment", None], + ["_Test Employee Advance", "Current Liabilities", 0, None, None], + ["_Test Account Tax Assets", "Current Assets", 1, None, None], + ["_Test Account VAT", "_Test Account Tax Assets", 0, "Tax", None], + ["_Test Account Service Tax", "_Test Account Tax Assets", 0, "Tax", None], + ["_Test Account Reserves and Surplus", "Current Liabilities", 0, None, None], + ["_Test Account Cost for Goods Sold", "Expenses", 0, None, None], + ["_Test Account Excise Duty", "_Test Account Tax Assets", 0, "Tax", None], + ["_Test Account Education Cess", "_Test Account Tax Assets", 0, "Tax", None], + ["_Test Account S&H Education Cess", "_Test Account Tax Assets", 0, "Tax", None], + ["_Test Account CST", "Direct Expenses", 0, "Tax", None], + ["_Test Account Discount", "Direct Expenses", 0, None, None], + ["_Test Write Off", "Indirect Expenses", 0, None, None], + ["_Test Exchange Gain/Loss", "Indirect Expenses", 0, None, None], + ["_Test Account Sales", "Direct Income", 0, None, None], + # related to Account Inventory Integration + ["_Test Account Stock In Hand", "Current Assets", 0, None, None], + # fixed asset depreciation + ["_Test Fixed Asset", "Current Assets", 0, "Fixed Asset", None], + ["_Test Accumulated Depreciations", "Current Assets", 0, "Accumulated Depreciation", None], + ["_Test Depreciations", "Expenses", 0, None, None], + ["_Test Gain/Loss on Asset Disposal", "Expenses", 0, None, None], + # Receivable / Payable Account + ["_Test Receivable", "Current Assets", 0, "Receivable", None], + ["_Test Payable", "Current Liabilities", 0, "Payable", None], + ["_Test Receivable USD", "Current Assets", 0, "Receivable", "USD"], + ["_Test Payable USD", "Current Liabilities", 0, "Payable", "USD"], + ] + + for company, abbr in [ + ["_Test Company", "_TC"], + ["_Test Company 1", "_TC1"], + ["_Test Company with perpetual inventory", "TCP1"], + ]: + test_objects = make_test_objects( + "Account", + [ + { + "doctype": "Account", + "account_name": account_name, + "parent_account": parent_account + " - " + abbr, + "company": company, + "is_group": is_group, + "account_type": account_type, + "account_currency": currency, + } + for account_name, parent_account, is_group, account_type, currency in accounts + ], + ) + + return test_objects + + def get_inventory_account(company, warehouse=None): account = None if warehouse: From 199a6da960c0419a16db59e7c93b2d23405efdc4 Mon Sep 17 00:00:00 2001 From: Saqib Ansari Date: Thu, 31 Mar 2022 11:41:52 +0530 Subject: [PATCH 5/8] perf: skip warehouse validation for non-stock items --- erpnext/controllers/accounts_controller.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/erpnext/controllers/accounts_controller.py b/erpnext/controllers/accounts_controller.py index 3ad61d4dc24..3a20d3f232f 100644 --- a/erpnext/controllers/accounts_controller.py +++ b/erpnext/controllers/accounts_controller.py @@ -1264,25 +1264,13 @@ class AccountsController(TransactionBase): return get_company_default(self.company, fieldname, ignore_validation=ignore_validation) def get_stock_items(self): - if hasattr(self, "_stock_items") and self._stock_items: - return self._stock_items - stock_items = [] item_codes = list(set(item.item_code for item in self.get("items"))) if item_codes: - stock_items = [ - r[0] - for r in frappe.db.sql( - """ - select name from `tabItem` - where name in (%s) and is_stock_item=1 - """ - % (", ".join(["%s"] * len(item_codes)),), - item_codes, - ) - ] + stock_items = frappe.db.get_values( + "Item", {"name": ["in", item_codes], "is_stock_item": 1}, pluck="name", cache=True + ) - self._stock_items = stock_items return stock_items def set_total_advance_paid(self): From 982a246eecfa0bbe9f1d31c06905227b0671267d Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 31 Mar 2022 11:47:20 +0530 Subject: [PATCH 6/8] fix: Add non-existent Item check and cleanup in `validate_for_items` - Added a validation if invalid item code ia passed via data import/API, etc. - Used DB APIs instead of raw sql - Separated checks into separate functions - Added return types to functions - Better variable naming and removed redundant utils import in function --- erpnext/buying/utils.py | 97 +++++++++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 38 deletions(-) diff --git a/erpnext/buying/utils.py b/erpnext/buying/utils.py index f97cd5e9dd0..e904af0dce3 100644 --- a/erpnext/buying/utils.py +++ b/erpnext/buying/utils.py @@ -3,19 +3,19 @@ import json +from typing import Dict import frappe from frappe import _ -from frappe.utils import cint, cstr, flt +from frappe.utils import cint, cstr, flt, getdate from erpnext.stock.doctype.item.item import get_last_purchase_details, validate_end_of_life -def update_last_purchase_rate(doc, is_submit): +def update_last_purchase_rate(doc, is_submit) -> None: """updates last_purchase_rate in item table for each item""" - import frappe.utils - this_purchase_date = frappe.utils.getdate(doc.get("posting_date") or doc.get("transaction_date")) + this_purchase_date = getdate(doc.get("posting_date") or doc.get("transaction_date")) for d in doc.get("items"): # get last purchase details @@ -41,7 +41,7 @@ def update_last_purchase_rate(doc, is_submit): frappe.db.set_value("Item", d.item_code, "last_purchase_rate", flt(last_purchase_rate)) -def validate_for_items(doc): +def validate_for_items(doc) -> None: items = [] for d in doc.get("items"): if not d.qty: @@ -49,40 +49,11 @@ def validate_for_items(doc): continue frappe.throw(_("Please enter quantity for Item {0}").format(d.item_code)) - # update with latest quantities - bin = frappe.db.sql( - """select projected_qty from `tabBin` where - item_code = %s and warehouse = %s""", - (d.item_code, d.warehouse), - as_dict=1, - ) - - f_lst = { - "projected_qty": bin and flt(bin[0]["projected_qty"]) or 0, - "ordered_qty": 0, - "received_qty": 0, - } - if d.doctype in ("Purchase Receipt Item", "Purchase Invoice Item"): - f_lst.pop("received_qty") - for x in f_lst: - if d.meta.get_field(x): - d.set(x, f_lst[x]) - - item = frappe.db.sql( - """select is_stock_item, - is_sub_contracted_item, end_of_life, disabled from `tabItem` where name=%s""", - d.item_code, - as_dict=1, - )[0] - + set_stock_levels(row=d) # update with latest quantities + item = validate_item_and_get_basic_data(row=d) + validate_stock_item_warehouse(row=d, item=item) validate_end_of_life(d.item_code, item.end_of_life, item.disabled) - # validate stock item - if item.is_stock_item == 1 and d.qty and not d.warehouse and not d.get("delivered_by_supplier"): - frappe.throw( - _("Warehouse is mandatory for stock Item {0} in row {1}").format(d.item_code, d.idx) - ) - items.append(cstr(d.item_code)) if ( @@ -93,7 +64,57 @@ def validate_for_items(doc): frappe.throw(_("Same item cannot be entered multiple times.")) -def check_on_hold_or_closed_status(doctype, docname): +def set_stock_levels(row) -> None: + projected_qty = frappe.db.get_value( + "Bin", + { + "item_code": row.item_code, + "warehouse": row.warehouse, + }, + "projected_qty", + ) + + qty_data = { + "projected_qty": flt(projected_qty), + "ordered_qty": 0, + "received_qty": 0, + } + if row.doctype in ("Purchase Receipt Item", "Purchase Invoice Item"): + qty_data.pop("received_qty") + + for field in qty_data: + if row.meta.get_field(field): + row.set(field, qty_data[field]) + + +def validate_item_and_get_basic_data(row) -> Dict: + item = frappe.db.get_values( + "Item", + filters={"name": row.item_code}, + fieldname=["is_stock_item", "is_sub_contracted_item", "end_of_life", "disabled"], + as_dict=1, + ) + if not item: + frappe.throw(_("Row #{0}: Item {1} does not exist").format(row.idx, frappe.bold(row.item_code))) + + return item[0] + + +def validate_stock_item_warehouse(row, item) -> None: + if ( + item.is_stock_item == 1 + and row.qty + and not row.warehouse + and not row.get("delivered_by_supplier") + ): + frappe.throw( + _("Row #{1}: Warehouse is mandatory for stock Item {0}").format( + frappe.bold(row.item_code), row.idx + ) + ) + + +def check_on_hold_or_closed_status(doctype, docname) -> None: status = frappe.db.get_value(doctype, docname, "status") if status in ("Closed", "On Hold"): From 4623a1bc5777f8bb16a147eae52b9f8e695612af Mon Sep 17 00:00:00 2001 From: Saqib Ansari Date: Thu, 31 Mar 2022 12:48:00 +0530 Subject: [PATCH 7/8] fix(test): Item MacBook does not exist --- erpnext/assets/doctype/asset/test_asset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/assets/doctype/asset/test_asset.py b/erpnext/assets/doctype/asset/test_asset.py index f681b3480c2..e759ad0719b 100644 --- a/erpnext/assets/doctype/asset/test_asset.py +++ b/erpnext/assets/doctype/asset/test_asset.py @@ -68,7 +68,7 @@ class TestAsset(AssetSetup): def test_item_exists(self): asset = create_asset(item_code="MacBook", do_not_save=1) - self.assertRaises(frappe.DoesNotExistError, asset.save) + self.assertRaises(frappe.ValidationError, asset.save) def test_validate_item(self): asset = create_asset(item_code="MacBook Pro", do_not_save=1) From 93f6346ceaaba582fc84ca090049bda48997e48b Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 31 Mar 2022 13:07:51 +0530 Subject: [PATCH 8/8] fix: (test) change expected exception due to https://github.com/frappe/frappe/pull/16454 --- erpnext/assets/doctype/asset/test_asset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/assets/doctype/asset/test_asset.py b/erpnext/assets/doctype/asset/test_asset.py index f681b3480c2..e759ad0719b 100644 --- a/erpnext/assets/doctype/asset/test_asset.py +++ b/erpnext/assets/doctype/asset/test_asset.py @@ -68,7 +68,7 @@ class TestAsset(AssetSetup): def test_item_exists(self): asset = create_asset(item_code="MacBook", do_not_save=1) - self.assertRaises(frappe.DoesNotExistError, asset.save) + self.assertRaises(frappe.ValidationError, asset.save) def test_validate_item(self): asset = create_asset(item_code="MacBook Pro", do_not_save=1)