From a322f0d0b3163192b372a1b407468f12c9474334 Mon Sep 17 00:00:00 2001 From: Kevin Chan Date: Mon, 12 Oct 2020 11:38:55 +0800 Subject: [PATCH 1/5] chore: Fix error message grammar --- erpnext/stock/doctype/item_price/item_price.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/erpnext/stock/doctype/item_price/item_price.py b/erpnext/stock/doctype/item_price/item_price.py index 8e39eb5037d..56efa09486b 100644 --- a/erpnext/stock/doctype/item_price/item_price.py +++ b/erpnext/stock/doctype/item_price/item_price.py @@ -23,7 +23,7 @@ class ItemPrice(Document): def validate_item(self): if not frappe.db.exists("Item", self.item_code): - frappe.throw(_("Item {0} not found").format(self.item_code)) + frappe.throw(_("Item {0} not found.").format(self.item_code)) def validate_dates(self): if self.valid_from and self.valid_upto: @@ -38,8 +38,7 @@ class ItemPrice(Document): if not price_list_details: link = frappe.utils.get_link_to_form('Price List', self.price_list) - frappe.throw("The price list {0} does not exists or disabled". - format(link)) + frappe.throw("The price list {0} does not exist or is disabled".format(link)) self.buying, self.selling, self.currency = price_list_details @@ -69,7 +68,7 @@ class ItemPrice(Document): self.reference = self.customer if self.buying: self.reference = self.supplier - + if self.selling and not self.buying: # if only selling then remove supplier self.supplier = None From 39bed3a65e2557d15d2275178120de941e66c536 Mon Sep 17 00:00:00 2001 From: Kevin Chan Date: Mon, 12 Oct 2020 11:42:05 +0800 Subject: [PATCH 2/5] fix: Add null or empty checking to validation This commit adds null or empty checking in the **Item Price** DocType's `check_duplicates()`. This was done to fix a bug where some prices are shown to be duplciates even though they aren't because they don't have values in some fields. --- .../stock/doctype/item_price/item_price.py | 52 +++++++++++++++---- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/erpnext/stock/doctype/item_price/item_price.py b/erpnext/stock/doctype/item_price/item_price.py index 56efa09486b..f88b05cd20a 100644 --- a/erpnext/stock/doctype/item_price/item_price.py +++ b/erpnext/stock/doctype/item_price/item_price.py @@ -48,20 +48,52 @@ class ItemPrice(Document): self.item_code,["item_name", "description"]) def check_duplicates(self): - conditions = "where item_code=%(item_code)s and price_list=%(price_list)s and name != %(name)s" + conditions = """ + where + item_code = %(item_code)s + and price_list = %(price_list)s + and name != %(name)s + """ - for field in ['uom', 'valid_from', - 'valid_upto', 'packing_unit', 'customer', 'supplier']: + for field in [ + "uom", + "valid_from", + "valid_upto", + "packing_unit", + "customer", + "supplier", + ]: if self.get(field): - conditions += " and {0} = %({1})s".format(field, field) + conditions += " and {0} = %({0})s ".format(field) + else: + conditions += """ + and ( + isnull({0}) + or {0} = '' + ) + """.format( + field + ) - price_list_rate = frappe.db.sql(""" - SELECT price_list_rate - FROM `tabItem Price` - {conditions} """.format(conditions=conditions), self.as_dict()) + price_list_rate = frappe.db.sql( + """ + select + price_list_rate + from + `tabItem Price` + {conditions} + """.format( + conditions=conditions + ), + self.as_dict(), + ) - if price_list_rate : - frappe.throw(_("Item Price appears multiple times based on Price List, Supplier/Customer, Currency, Item, UOM, Qty and Dates."), ItemPriceDuplicateItem) + if price_list_rate: + frappe.throw(_(""" + Item Price appears multiple times based on + Price List, Supplier/Customer, Currency, Item, UOM, Qty, + and Dates. + """), ItemPriceDuplicateItem,) def before_save(self): if self.selling: From 58389ebd402d461903fccb522127bdc1adbbb40e Mon Sep 17 00:00:00 2001 From: Kevin Chan Date: Mon, 12 Oct 2020 15:20:11 +0800 Subject: [PATCH 3/5] style: Collapse whitespace --- .../stock/doctype/item_price/item_price.py | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/erpnext/stock/doctype/item_price/item_price.py b/erpnext/stock/doctype/item_price/item_price.py index f88b05cd20a..f05de22d01f 100644 --- a/erpnext/stock/doctype/item_price/item_price.py +++ b/erpnext/stock/doctype/item_price/item_price.py @@ -66,21 +66,12 @@ class ItemPrice(Document): if self.get(field): conditions += " and {0} = %({0})s ".format(field) else: - conditions += """ - and ( - isnull({0}) - or {0} = '' - ) - """.format( - field - ) + conditions += "and (isnull({0}) or {0} = '')".format(field) price_list_rate = frappe.db.sql( """ - select - price_list_rate - from - `tabItem Price` + select price_list_rate + from `tabItem Price` {conditions} """.format( conditions=conditions @@ -89,11 +80,7 @@ class ItemPrice(Document): ) if price_list_rate: - frappe.throw(_(""" - Item Price appears multiple times based on - Price List, Supplier/Customer, Currency, Item, UOM, Qty, - and Dates. - """), ItemPriceDuplicateItem,) + frappe.throw(_("Item Price appears multiple times based on Price List, Supplier/Customer, Currency, Item, UOM, Qty, and Dates."), ItemPriceDuplicateItem,) def before_save(self): if self.selling: From f6bcd5c2b230375f4014e8270a41027da9b9c6b7 Mon Sep 17 00:00:00 2001 From: Kevin Chan Date: Mon, 12 Oct 2020 16:57:33 +0800 Subject: [PATCH 4/5] test: Add test case for duplicate checking --- .../doctype/item_price/test_item_price.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/erpnext/stock/doctype/item_price/test_item_price.py b/erpnext/stock/doctype/item_price/test_item_price.py index 702acc38fe4..f3d406eeca6 100644 --- a/erpnext/stock/doctype/item_price/test_item_price.py +++ b/erpnext/stock/doctype/item_price/test_item_price.py @@ -138,4 +138,23 @@ class TestItemPrice(unittest.TestCase): # Valid price list must already exist self.assertRaises(frappe.ValidationError, doc.save) + def test_empty_duplicate_validation(self): + # Check if none/empty values are not compared during insert validation + doc = frappe.copy_doc(test_records[2]) + doc.customer = None + doc.price_list_rate = 21 + doc.insert() + + args = { + "price_list": doc.price_list, + "uom": "_Test UOM", + "transaction_date": '2017-04-18', + "qty": 7 + } + + price = get_price_list_rate_for(args, doc.item_code) + frappe.db.rollback() + + self.assertEqual(price, 21) + test_records = frappe.get_test_records('Item Price') From a2ba8ea74a39403d9cd2fd2cc2af895059d9cdf8 Mon Sep 17 00:00:00 2001 From: Kevin Chan Date: Tue, 3 Nov 2020 10:57:52 +0800 Subject: [PATCH 5/5] style: Clean up code formatting --- .../stock/doctype/item_price/item_price.py | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/erpnext/stock/doctype/item_price/item_price.py b/erpnext/stock/doctype/item_price/item_price.py index 54f534f3d1c..bed5ea9ab66 100644 --- a/erpnext/stock/doctype/item_price/item_price.py +++ b/erpnext/stock/doctype/item_price/item_price.py @@ -46,12 +46,7 @@ class ItemPrice(Document): self.item_name, self.item_description = frappe.db.get_value("Item", self.item_code,["item_name", "description"]) def check_duplicates(self): - conditions = """ - where - item_code = %(item_code)s - and price_list = %(price_list)s - and name != %(name)s - """ + conditions = """where item_code = %(item_code)s and price_list = %(price_list)s and name != %(name)s""" for field in [ "uom", @@ -59,23 +54,18 @@ class ItemPrice(Document): "valid_upto", "packing_unit", "customer", - "supplier", - ]: + "supplier",]: if self.get(field): conditions += " and {0} = %({0})s ".format(field) else: conditions += "and (isnull({0}) or {0} = '')".format(field) - price_list_rate = frappe.db.sql( - """ + price_list_rate = frappe.db.sql(""" select price_list_rate from `tabItem Price` {conditions} - """.format( - conditions=conditions - ), - self.as_dict(), - ) + """.format(conditions=conditions), + self.as_dict(),) if price_list_rate: frappe.throw(_("Item Price appears multiple times based on Price List, Supplier/Customer, Currency, Item, UOM, Qty, and Dates."), ItemPriceDuplicateItem,)