From 49fb6bec6a5ff5a3cda68fd20856e85ece9543e0 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Tue, 2 Jul 2024 15:44:54 +0530 Subject: [PATCH 1/5] refactor: validation to prevent recursion with mixed conditions (cherry picked from commit 406dfd528f2cabc60ece0c917fb02092abed50cc) --- erpnext/accounts/doctype/pricing_rule/pricing_rule.py | 5 +++++ .../doctype/promotional_scheme/promotional_scheme.py | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py index 30dbb14f84c..6a2f3a2a045 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py @@ -31,6 +31,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 @@ -201,6 +202,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 e3278098f1a..f5bc450f6dc 100644 --- a/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py @@ -77,6 +77,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: @@ -108,6 +109,7 @@ class PromotionalScheme(Document): frappe.delete_doc("Pricing Rule", docname.name) def on_update(self): + self.validate() pricing_rules = ( frappe.get_all( "Pricing Rule", @@ -119,6 +121,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 From 9fde7330e0d5a4e2a24bc7ae5a2277f913a9b972 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 17:05:13 +0530 Subject: [PATCH 2/5] fix: use standard method to get `_doc_before_save` (cherry picked from commit 9d7be293ae9d7d2a7cf9f81fa1aadcc071bc29e6) --- .../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 f5bc450f6dc..857ee0e7187 100644 --- a/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/promotional_scheme.py @@ -95,7 +95,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}) From 99317768f68cf44263b71ab2079b237923d43cb5 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 17:11:32 +0530 Subject: [PATCH 3/5] test: validation on mixed condition with recursion (cherry picked from commit 9bd4e7b7094830f56fdb6fd492d1a0873bf5b977) # Conflicts: # erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py --- .../test_promotional_scheme.py | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py index 9e576fb8775..e5b82304d11 100644 --- a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py @@ -107,6 +107,50 @@ class TestPromotionalScheme(unittest.TestCase): price_rules = frappe.get_all("Pricing Rule", filters={"promotional_scheme": ps.name}) self.assertEqual(price_rules, []) +<<<<<<< HEAD +======= + def test_pricing_rule_for_product_discount_slabs(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.save() + pr = frappe.get_doc("Pricing Rule", {"promotional_scheme_id": ps.product_discount_slabs[0].name}) + self.assertSequenceEqual( + [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) + +>>>>>>> 9bd4e7b709 (test: validation on mixed condition with recursion) def make_promotional_scheme(**args): args = frappe._dict(args) From 71cbebd31b367a9085dc5e57c7a171dfe92f1c7c Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 17:54:41 +0530 Subject: [PATCH 4/5] test: validation on mixed condition and recursion on pricing rule (cherry picked from commit eb4af58bf0371437ba7bca36f345488c6c03331b) # Conflicts: # erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py --- .../doctype/pricing_rule/test_pricing_rule.py | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index 6f1cee61637..48fae36aaf8 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -1087,6 +1087,83 @@ class TestPricingRule(unittest.TestCase): frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 1") frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 2") +<<<<<<< HEAD +======= + def test_pricing_rules_with_and_without_apply_multiple(self): + item = make_item("PR Item 99") + + test_records = [ + { + "doctype": "Pricing Rule", + "title": "_Test discount on item group", + "name": "_Test discount on item group", + "apply_on": "Item Group", + "item_groups": [ + { + "item_group": "Products", + } + ], + "selling": 1, + "price_or_product_discount": "Price", + "rate_or_discount": "Discount Percentage", + "discount_percentage": 60, + "has_priority": 1, + "company": "_Test Company", + "apply_multiple_pricing_rules": True, + }, + { + "doctype": "Pricing Rule", + "title": "_Test fixed rate on item code", + "name": "_Test fixed rate on item code", + "apply_on": "Item Code", + "items": [ + { + "item_code": item.name, + } + ], + "selling": 1, + "price_or_product_discount": "Price", + "rate_or_discount": "Rate", + "rate": 25, + "has_priority": 1, + "company": "_Test Company", + "apply_multiple_pricing_rules": False, + }, + ] + + for item_group_priority, item_code_priority in [(2, 4), (4, 2)]: + item_group_rule = frappe.get_doc(test_records[0].copy()) + item_group_rule.priority = item_group_priority + item_group_rule.insert() + + item_code_rule = frappe.get_doc(test_records[1].copy()) + item_code_rule.priority = item_code_priority + item_code_rule.insert() + + si = create_sales_invoice(qty=5, customer="_Test Customer 1", item=item.name, do_not_submit=True) + si.save() + self.assertEqual(len(si.pricing_rules), 1) + # Item Code rule should've applied as it has higher priority + expected_rule = item_group_rule if item_group_priority > item_code_priority else item_code_rule + self.assertEqual(si.pricing_rules[0].pricing_rule, expected_rule.name) + + si.delete() + item_group_rule.delete() + item_code_rule.delete() + + 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", + ) + pricing_rule.mixed_conditions = True + pricing_rule.is_recursive = True + self.assertRaises(frappe.ValidationError, pricing_rule.save) + +>>>>>>> eb4af58bf0 (test: validation on mixed condition and recursion on pricing rule) test_dependencies = ["Campaign"] From d5fa968078a901758c5ca9ef8f64333dcd64bc03 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Wed, 3 Jul 2024 20:58:50 +0530 Subject: [PATCH 5/5] chore: resolve conflicts --- .../doctype/pricing_rule/test_pricing_rule.py | 65 ------------------- .../test_promotional_scheme.py | 25 ------- 2 files changed, 90 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index 48fae36aaf8..46c874b533c 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -1087,70 +1087,6 @@ class TestPricingRule(unittest.TestCase): frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 1") frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 2") -<<<<<<< HEAD -======= - def test_pricing_rules_with_and_without_apply_multiple(self): - item = make_item("PR Item 99") - - test_records = [ - { - "doctype": "Pricing Rule", - "title": "_Test discount on item group", - "name": "_Test discount on item group", - "apply_on": "Item Group", - "item_groups": [ - { - "item_group": "Products", - } - ], - "selling": 1, - "price_or_product_discount": "Price", - "rate_or_discount": "Discount Percentage", - "discount_percentage": 60, - "has_priority": 1, - "company": "_Test Company", - "apply_multiple_pricing_rules": True, - }, - { - "doctype": "Pricing Rule", - "title": "_Test fixed rate on item code", - "name": "_Test fixed rate on item code", - "apply_on": "Item Code", - "items": [ - { - "item_code": item.name, - } - ], - "selling": 1, - "price_or_product_discount": "Price", - "rate_or_discount": "Rate", - "rate": 25, - "has_priority": 1, - "company": "_Test Company", - "apply_multiple_pricing_rules": False, - }, - ] - - for item_group_priority, item_code_priority in [(2, 4), (4, 2)]: - item_group_rule = frappe.get_doc(test_records[0].copy()) - item_group_rule.priority = item_group_priority - item_group_rule.insert() - - item_code_rule = frappe.get_doc(test_records[1].copy()) - item_code_rule.priority = item_code_priority - item_code_rule.insert() - - si = create_sales_invoice(qty=5, customer="_Test Customer 1", item=item.name, do_not_submit=True) - si.save() - self.assertEqual(len(si.pricing_rules), 1) - # Item Code rule should've applied as it has higher priority - expected_rule = item_group_rule if item_group_priority > item_code_priority else item_code_rule - self.assertEqual(si.pricing_rules[0].pricing_rule, expected_rule.name) - - si.delete() - item_group_rule.delete() - item_code_rule.delete() - def test_validation_on_mixed_condition_with_recursion(self): pricing_rule = make_pricing_rule( discount_percentage=10, @@ -1163,7 +1099,6 @@ class TestPricingRule(unittest.TestCase): pricing_rule.is_recursive = True self.assertRaises(frappe.ValidationError, pricing_rule.save) ->>>>>>> eb4af58bf0 (test: validation on mixed condition and recursion on pricing rule) test_dependencies = ["Campaign"] diff --git a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py index e5b82304d11..0d08fd98139 100644 --- a/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py +++ b/erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py @@ -107,30 +107,6 @@ class TestPromotionalScheme(unittest.TestCase): price_rules = frappe.get_all("Pricing Rule", filters={"promotional_scheme": ps.name}) self.assertEqual(price_rules, []) -<<<<<<< HEAD -======= - def test_pricing_rule_for_product_discount_slabs(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.save() - pr = frappe.get_doc("Pricing Rule", {"promotional_scheme_id": ps.product_discount_slabs[0].name}) - self.assertSequenceEqual( - [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", []) @@ -150,7 +126,6 @@ class TestPromotionalScheme(unittest.TestCase): ps.mixed_conditions = True self.assertRaises(frappe.ValidationError, ps.save) ->>>>>>> 9bd4e7b709 (test: validation on mixed condition with recursion) def make_promotional_scheme(**args): args = frappe._dict(args)