From 8018f9c5a535d8800fb5577fb8dc100ecefca6f1 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Tue, 2 Jul 2024 15:44:54 +0530 Subject: [PATCH 1/6] refactor: validation to prevent recursion with mixed conditions --- .../doctype/pricing_rule/pricing_rule.py | 5 ++++ .../promotional_scheme/promotional_scheme.py | 28 ++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py index c002c322d1..4df452fbdc 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py @@ -138,6 +138,7 @@ class PricingRule(Document): self.validate_price_list_with_currency() self.validate_dates() self.validate_condition() + self.validate_mixed_with_recursion() if not self.margin_type: self.margin_rate_or_amount = 0.0 @@ -307,6 +308,10 @@ class PricingRule(Document): ): frappe.throw(_("Invalid condition expression")) + def validate_mixed_with_recursion(self): + if self.mixed_conditions and self.is_recursive: + frappe.throw(_("Recursive Discounts with Mixed condition is not supported by the system")) + # -------------------------------------------------------------------------------- diff --git a/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py b/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py index af6d096b05..76ebf46317 100644 --- a/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py @@ -79,6 +79,7 @@ class PromotionalScheme(Document): self.validate_applicable_for() self.validate_pricing_rules() + self.validate_mixed_with_recursion() def validate_applicable_for(self): if self.applicable_for: @@ -102,9 +103,7 @@ class PromotionalScheme(Document): docnames = frappe.get_all("Pricing Rule", filters={"promotional_scheme": self.name}) for docname in docnames: - if frappe.db.exists( - "Pricing Rule Detail", {"pricing_rule": docname.name, "docstatus": ("<", 2)} - ): + if frappe.db.exists("Pricing Rule Detail", {"pricing_rule": docname.name, "docstatus": ("<", 2)}): raise_for_transaction_exists(self.name) if docnames and not transaction_exists: @@ -112,6 +111,7 @@ class PromotionalScheme(Document): frappe.delete_doc("Pricing Rule", docname.name) def on_update(self): + self.validate() pricing_rules = ( frappe.get_all( "Pricing Rule", @@ -123,6 +123,15 @@ class PromotionalScheme(Document): ) self.update_pricing_rules(pricing_rules) + def validate_mixed_with_recursion(self): + if self.mixed_conditions: + if self.product_discount_slabs: + for slab in self.product_discount_slabs: + if slab.is_recursive: + frappe.throw( + _("Recursive Discounts with Mixed condition is not supported by the system") + ) + def update_pricing_rules(self, pricing_rules): rules = {} count = 0 @@ -189,7 +198,14 @@ def _get_pricing_rules(doc, child_doc, discount_fields, rules=None): for applicable_for_value in args.get(applicable_for): docname = get_pricing_rule_docname(d, applicable_for, applicable_for_value) pr = prepare_pricing_rule( - args, doc, child_doc, discount_fields, d, docname, applicable_for, applicable_for_value + args, + doc, + child_doc, + discount_fields, + d, + docname, + applicable_for, + applicable_for_value, ) new_doc.append(pr) @@ -214,9 +230,7 @@ def _get_pricing_rules(doc, child_doc, discount_fields, rules=None): return new_doc -def get_pricing_rule_docname( - row: dict, applicable_for: str = None, applicable_for_value: str = None -) -> str: +def get_pricing_rule_docname(row: dict, applicable_for: str = None, applicable_for_value: str = None) -> str: fields = ["promotional_scheme_id", "name"] filters = {"promotional_scheme_id": row.name} -- GitLab From 9eb6cb89d78c77c17f4c3bffd4322dfc1facff27 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 17:05:13 +0530 Subject: [PATCH 2/6] fix: use standard method to get `_doc_before_save` --- .../accounts/doctype/promotional_scheme/promotional_scheme.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py b/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py index 76ebf46317..6cd3c137dd 100644 --- a/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py @@ -97,7 +97,7 @@ class PromotionalScheme(Document): docnames = [] # If user has changed applicable for - if self._doc_before_save.applicable_for == self.applicable_for: + if self.get_doc_before_save() and self.get_doc_before_save().applicable_for == self.applicable_for: return docnames = frappe.get_all("Pricing Rule", filters={"promotional_scheme": self.name}) -- GitLab From 6d049866ef0c2969fd6a210ef27b9c86b54193a6 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 17:11:32 +0530 Subject: [PATCH 3/6] test: validation on mixed condition with recursion --- .../test_promotional_scheme.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py index 9b40c98829..74ba6cf923 100644 --- a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py @@ -129,6 +129,25 @@ class TestPromotionalScheme(unittest.TestCase): [pr.min_qty, pr.free_item, pr.free_qty, pr.recurse_for], [12, "_Test Item 2", 1, 12] ) + def test_validation_on_recurse_with_mixed_condition(self): + ps = make_promotional_scheme() + ps.set("price_discount_slabs", []) + ps.set( + "product_discount_slabs", + [ + { + "rule_description": "12+1", + "min_qty": 12, + "free_item": "_Test Item 2", + "free_qty": 1, + "is_recursive": 1, + "recurse_for": 12, + } + ], + ) + ps.mixed_conditions = True + self.assertRaises(frappe.ValidationError, ps.save) + def make_promotional_scheme(**args): args = frappe._dict(args) -- GitLab From e05092da10dc46a8df70dc723f034e3589c12401 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 17:54:41 +0530 Subject: [PATCH 4/6] test: validation on mixed condition and recursion on pricing rule --- .../doctype/pricing_rule/test_pricing_rule.py | 74 +++---------------- 1 file changed, 10 insertions(+), 64 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index 21605bd1a0..404c19dbff 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -1300,71 +1300,17 @@ class TestPricingRule(unittest.TestCase): item_group_rule.delete() item_code_rule.delete() - def test_pricing_rule_override(self): - """ - Pricing rule validation on save can be overriden by adding a server script triggered `Before Validate` - with the following code: `doc.flags.ignore_pricing_rule = True` - """ - - from erpnext.selling.doctype.quotation.test_quotation import make_quotation - - test_record = { - "doctype": "Pricing Rule", - "title": "_Test Pricing Rule", - "apply_on": "Item Code", - "items": [{"item_code": "_Test Item"}], - "currency": "USD", - "selling": 1, - "rate_or_discount": "Discount Percentage", - "rate": 0, - "discount_percentage": 10, - "company": "_Test Company", - } - frappe.get_doc(test_record.copy()).insert() - - item_price = frappe.new_doc("Item Price") - item_price.item_code = "_Test Item" - item_price.uom = "_Test UOM" - item_price.price_list = "Standard Selling" - item_price.price_list_rate = 100.0 - item_price.insert() - - quotation = make_quotation( - company="_Test Company", currency="USD", item="_Test Item", uom="_Test UOM", do_not_submit=True - ) - - self.assertEqual(quotation.items[0].rate, 90.0) - - quotation.items[0].rate = 100 - quotation.items[0].discount_percentage = 0.0 - quotation.items[0].discount_amount = 0.0 - quotation.flags.ignore_pricing_rule = True - quotation.save() - - quotation.reload() - self.assertEqual(quotation.items[0].rate, 100.0) - - quotation.items[0].rate = 100 - quotation.items[0].discount_percentage = 0.0 - quotation.items[0].discount_amount = 0.0 - quotation.flags.ignore_pricing_rule = False - quotation.save() - - quotation.reload() - self.assertEqual(quotation.items[0].rate, 90.0) - - quotation.append( - "items", - { - "item_code": "_Test Item", - "qty": 10, - "uom": "_Test UOM", - }, + def test_validation_on_mixed_condition_with_recursion(self): + pricing_rule = make_pricing_rule( + discount_percentage=10, + selling=1, + priority=2, + min_qty=4, + title="_Test Pricing Rule with Min Qty - 2", ) - quotation.save() - self.assertEqual(quotation.items[1].rate, 90.0) - - frappe.delete_doc("Item Price", item_price.name, force=True) + pricing_rule.mixed_conditions = True + pricing_rule.is_recursive = True + self.assertRaises(frappe.ValidationError, pricing_rule.save) test_dependencies = ["Campaign"] -- GitLab From 681b2ea9f4824f537eae9679c2d368fe200a211f Mon Sep 17 00:00:00 2001 From: Charles-Henri Decultot Date: Thu, 11 Jul 2024 10:17:01 +0200 Subject: [PATCH 5/6] fix: deleted test --- .../doctype/pricing_rule/test_pricing_rule.py | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index 404c19dbff..bcc55c82a5 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -1300,6 +1300,72 @@ class TestPricingRule(unittest.TestCase): item_group_rule.delete() item_code_rule.delete() + def test_pricing_rule_override(self): + """ + Pricing rule validation on save can be overriden by adding a server script triggered `Before Validate` + with the following code: `doc.flags.ignore_pricing_rule = True` + """ + + from erpnext.selling.doctype.quotation.test_quotation import make_quotation + + test_record = { + "doctype": "Pricing Rule", + "title": "_Test Pricing Rule", + "apply_on": "Item Code", + "items": [{"item_code": "_Test Item"}], + "currency": "USD", + "selling": 1, + "rate_or_discount": "Discount Percentage", + "rate": 0, + "discount_percentage": 10, + "company": "_Test Company", + } + frappe.get_doc(test_record.copy()).insert() + + item_price = frappe.new_doc("Item Price") + item_price.item_code = "_Test Item" + item_price.uom = "_Test UOM" + item_price.price_list = "Standard Selling" + item_price.price_list_rate = 100.0 + item_price.insert() + + quotation = make_quotation( + company="_Test Company", currency="USD", item="_Test Item", uom="_Test UOM", do_not_submit=True + ) + + self.assertEqual(quotation.items[0].rate, 90.0) + + quotation.items[0].rate = 100 + quotation.items[0].discount_percentage = 0.0 + quotation.items[0].discount_amount = 0.0 + quotation.flags.ignore_pricing_rule = True + quotation.save() + + quotation.reload() + self.assertEqual(quotation.items[0].rate, 100.0) + + quotation.items[0].rate = 100 + quotation.items[0].discount_percentage = 0.0 + quotation.items[0].discount_amount = 0.0 + quotation.flags.ignore_pricing_rule = False + quotation.save() + + quotation.reload() + self.assertEqual(quotation.items[0].rate, 90.0) + + quotation.append( + "items", + { + "item_code": "_Test Item", + "qty": 10, + "uom": "_Test UOM", + }, + ) + quotation.save() + self.assertEqual(quotation.items[1].rate, 90.0) + + frappe.delete_doc("Item Price", item_price.name, force=True) + def test_validation_on_mixed_condition_with_recursion(self): pricing_rule = make_pricing_rule( discount_percentage=10, -- GitLab From 1c4571f213c7898034f3d80c76660745697e99d1 Mon Sep 17 00:00:00 2001 From: Charles-Henri Decultot Date: Fri, 12 Jul 2024 15:50:27 +0200 Subject: [PATCH 6/6] fix: delete scheme after test --- .../doctype/promotional_scheme/test_promotional_scheme.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py index 74ba6cf923..c7338c5f41 100644 --- a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py @@ -14,6 +14,10 @@ class TestPromotionalScheme(unittest.TestCase): if frappe.db.exists("Promotional Scheme", "_Test Scheme"): frappe.delete_doc("Promotional Scheme", "_Test Scheme") + def tearDown(self): + if frappe.db.exists("Promotional Scheme", "_Test Scheme"): + frappe.delete_doc("Promotional Scheme", "_Test Scheme") + def test_promotional_scheme(self): ps = make_promotional_scheme(applicable_for="Customer", customer="_Test Customer") price_rules = frappe.get_all( -- GitLab