From 19c01b145765fbbd2be95e914c818c44697b7d76 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Wed, 5 Feb 2025 18:48:52 +0530 Subject: [PATCH 1/6] feat: added option to enforce free item qty in pricing rule --- .../accounts/doctype/pricing_rule/pricing_rule.json | 10 +++++++++- erpnext/accounts/doctype/pricing_rule/pricing_rule.py | 1 + erpnext/accounts/doctype/pricing_rule/utils.py | 3 ++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json index a16f618d0a8..9106abd6ded 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json @@ -53,6 +53,7 @@ "column_break_42", "free_item_uom", "round_free_qty", + "enforce_free_item_qty", "is_recursive", "recurse_for", "apply_recursion_over", @@ -643,12 +644,19 @@ "fieldname": "has_priority", "fieldtype": "Check", "label": "Has Priority" + }, + { + "default": "0", + "depends_on": "eval:doc.price_or_product_discount == 'Product'", + "fieldname": "enforce_free_item_qty", + "fieldtype": "Check", + "label": "Enforce Free Item Qty" } ], "icon": "fa fa-gift", "idx": 1, "links": [], - "modified": "2024-09-16 18:14:51.314765", + "modified": "2025-02-05 18:05:03.886828", "modified_by": "Administrator", "module": "Accounts", "name": "Pricing Rule", diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py index 9a7f6896fb8..53d44effcff 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py @@ -60,6 +60,7 @@ class PricingRule(Document): disable: DF.Check discount_amount: DF.Currency discount_percentage: DF.Float + enforce_free_item_qty: DF.Check for_price_list: DF.Link | None free_item: DF.Link | None free_item_rate: DF.Currency diff --git a/erpnext/accounts/doctype/pricing_rule/utils.py b/erpnext/accounts/doctype/pricing_rule/utils.py index d8eea29bfc0..991426c73b7 100644 --- a/erpnext/accounts/doctype/pricing_rule/utils.py +++ b/erpnext/accounts/doctype/pricing_rule/utils.py @@ -713,7 +713,8 @@ def apply_pricing_rule_for_free_items(doc, pricing_rule_args): args.pop((item.item_code, item.pricing_rules)) for free_item in args.values(): - doc.append("items", free_item) + if frappe.get_value("Pricing Rule", free_item["pricing_rules"], "enforce_free_item_qty"): + doc.append("items", free_item) def get_pricing_rule_items(pr_doc, other_items=False) -> list: From 366ae85d855c130c89fa1f151a2cca5b1e7dbdd8 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Wed, 5 Feb 2025 19:18:01 +0530 Subject: [PATCH 2/6] fix: tests --- erpnext/accounts/doctype/pricing_rule/pricing_rule.py | 2 +- erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py | 5 +++++ erpnext/stock/doctype/pick_list/test_pick_list.py | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py index 53d44effcff..f5d15029f09 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py @@ -646,7 +646,7 @@ def remove_pricing_rule_for_item(pricing_rules, item_details, item_code=None, ra if pricing_rule.margin_type in ["Percentage", "Amount"]: item_details.margin_rate_or_amount = 0.0 item_details.margin_type = None - elif pricing_rule.get("free_item"): + elif pricing_rule.get("free_item") and pricing_rule.get("enforce_free_item_qty"): item_details.remove_free_item = ( item_code if pricing_rule.get("same_item") else pricing_rule.get("free_item") ) diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index 9787fda86d3..dac04a39d99 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -396,6 +396,7 @@ class TestPricingRule(IntegrationTestCase): "price_or_product_discount": "Product", "same_item": 1, "free_qty": 1, + "enforce_free_item_qty": 1, "company": "_Test Company", } frappe.get_doc(test_record.copy()).insert() @@ -428,6 +429,7 @@ class TestPricingRule(IntegrationTestCase): "same_item": 0, "free_item": "_Test Item 2", "free_qty": 1, + "enforce_free_item_qty": 1, "company": "_Test Company", } frappe.get_doc(test_record.copy()).insert() @@ -1121,6 +1123,7 @@ class TestPricingRule(IntegrationTestCase): "price_or_product_discount": "Product", "same_item": 1, "free_qty": 1, + "enforce_free_item_qty": 1, "round_free_qty": 1, "is_recursive": 1, "recurse_for": 2, @@ -1166,6 +1169,7 @@ class TestPricingRule(IntegrationTestCase): "price_or_product_discount": "Product", "same_item": 1, "free_qty": 10, + "enforce_free_item_qty": 1, "round_free_qty": 1, "is_recursive": 1, "recurse_for": 100, @@ -1461,6 +1465,7 @@ def make_pricing_rule(**args): "discount_amount": args.discount_amount or 0.0, "apply_multiple_pricing_rules": args.apply_multiple_pricing_rules or 0, "has_priority": args.has_priority or 0, + "enforce_free_item_qty": args.enforce_free_item_qty or 1, } ) diff --git a/erpnext/stock/doctype/pick_list/test_pick_list.py b/erpnext/stock/doctype/pick_list/test_pick_list.py index 2b7bcc76f62..7783c5ba1ca 100644 --- a/erpnext/stock/doctype/pick_list/test_pick_list.py +++ b/erpnext/stock/doctype/pick_list/test_pick_list.py @@ -1251,6 +1251,7 @@ class TestPickList(IntegrationTestCase): "is_recursive": 1, "recurse_for": 2, "free_qty": 1, + "enforce_free_item_qty": 1, "company": "_Test Company", "customer": "_Test Customer", } From ac3259b8f156131e2d5ab27e411f9d735f835626 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Wed, 5 Feb 2025 21:34:19 +0530 Subject: [PATCH 3/6] test: added test --- .../doctype/pricing_rule/pricing_rule.json | 4 +- .../doctype/pricing_rule/test_pricing_rule.py | 52 +++++++++++++++++-- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json index 9106abd6ded..28fc040c518 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json @@ -646,7 +646,7 @@ "label": "Has Priority" }, { - "default": "0", + "default": "1", "depends_on": "eval:doc.price_or_product_discount == 'Product'", "fieldname": "enforce_free_item_qty", "fieldtype": "Check", @@ -656,7 +656,7 @@ "icon": "fa fa-gift", "idx": 1, "links": [], - "modified": "2025-02-05 18:05:03.886828", + "modified": "2025-02-05 21:03:22.103044", "modified_by": "Administrator", "module": "Accounts", "name": "Pricing Rule", diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index dac04a39d99..3e0a0cac71b 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -396,7 +396,6 @@ class TestPricingRule(IntegrationTestCase): "price_or_product_discount": "Product", "same_item": 1, "free_qty": 1, - "enforce_free_item_qty": 1, "company": "_Test Company", } frappe.get_doc(test_record.copy()).insert() @@ -429,7 +428,6 @@ class TestPricingRule(IntegrationTestCase): "same_item": 0, "free_item": "_Test Item 2", "free_qty": 1, - "enforce_free_item_qty": 1, "company": "_Test Company", } frappe.get_doc(test_record.copy()).insert() @@ -440,6 +438,54 @@ class TestPricingRule(IntegrationTestCase): self.assertEqual(so.items[1].is_free_item, 1) self.assertEqual(so.items[1].item_code, "_Test Item 2") + def test_enforce_free_item_qty(self): + # this test is only for testing non-enforcement as all other tests in this file already test with enforcement + frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule") + test_record = { + "doctype": "Pricing Rule", + "title": "_Test Pricing Rule", + "apply_on": "Item Code", + "currency": "USD", + "items": [ + { + "item_code": "_Test Item", + } + ], + "selling": 1, + "rate_or_discount": "Discount Percentage", + "rate": 0, + "min_qty": 0, + "max_qty": 7, + "discount_percentage": 17.5, + "price_or_product_discount": "Product", + "same_item": 0, + "free_item": "_Test Item 2", + "free_qty": 1, + "company": "_Test Company", + } + pricing_rule = frappe.get_doc(test_record.copy()).insert() + + # With enforcement + so = make_sales_order(item_code="_Test Item", qty=1, do_not_submit=True) + self.assertEqual(so.items[1].is_free_item, 1) + self.assertEqual(so.items[1].item_code, "_Test Item 2") + + # Test 1 : Saving a document with an item with pricing list without it's corresponding free item will cause it the free item to be refetched on save + so.items.pop(1) + so.save() + so.reload() + self.assertEqual(len(so.items), 2) + + # Without enforcement + pricing_rule.enforce_free_item_qty = 0 + pricing_rule.save() + + # Test 2 : Deleted free item will not be fetched again on save without enfrocement + so.items.pop(1) + so.save() + so.reload() + self.assertEqual(len(so.items), 1) + def test_cumulative_pricing_rule(self): frappe.delete_doc_if_exists("Pricing Rule", "_Test Cumulative Pricing Rule") test_record = { @@ -1123,7 +1169,6 @@ class TestPricingRule(IntegrationTestCase): "price_or_product_discount": "Product", "same_item": 1, "free_qty": 1, - "enforce_free_item_qty": 1, "round_free_qty": 1, "is_recursive": 1, "recurse_for": 2, @@ -1169,7 +1214,6 @@ class TestPricingRule(IntegrationTestCase): "price_or_product_discount": "Product", "same_item": 1, "free_qty": 10, - "enforce_free_item_qty": 1, "round_free_qty": 1, "is_recursive": 1, "recurse_for": 100, From 4dcac564863ee3fed391adccc5898f3cd07f4ccf Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 11 Feb 2025 16:29:01 +0530 Subject: [PATCH 4/6] fix: add is_new in if condition --- erpnext/accounts/doctype/pricing_rule/utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/erpnext/accounts/doctype/pricing_rule/utils.py b/erpnext/accounts/doctype/pricing_rule/utils.py index 991426c73b7..e0b53aa580c 100644 --- a/erpnext/accounts/doctype/pricing_rule/utils.py +++ b/erpnext/accounts/doctype/pricing_rule/utils.py @@ -713,7 +713,9 @@ def apply_pricing_rule_for_free_items(doc, pricing_rule_args): args.pop((item.item_code, item.pricing_rules)) for free_item in args.values(): - if frappe.get_value("Pricing Rule", free_item["pricing_rules"], "enforce_free_item_qty"): + if doc.is_new() or frappe.get_value( + "Pricing Rule", free_item["pricing_rules"], "enforce_free_item_qty" + ): doc.append("items", free_item) From f3d598881c9b1135d5afca01059e27056a62b305 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 17 Feb 2025 18:21:22 +0530 Subject: [PATCH 5/6] refactor: rename field --- erpnext/accounts/doctype/pricing_rule/pricing_rule.json | 8 ++++---- erpnext/accounts/doctype/pricing_rule/pricing_rule.py | 4 ++-- .../accounts/doctype/pricing_rule/test_pricing_rule.py | 8 ++++---- erpnext/accounts/doctype/pricing_rule/utils.py | 4 ++-- erpnext/stock/doctype/pick_list/test_pick_list.py | 2 +- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json index 28fc040c518..8a968e52bbc 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json @@ -53,7 +53,7 @@ "column_break_42", "free_item_uom", "round_free_qty", - "enforce_free_item_qty", + "dont_enforce_free_item_qty", "is_recursive", "recurse_for", "apply_recursion_over", @@ -648,15 +648,15 @@ { "default": "1", "depends_on": "eval:doc.price_or_product_discount == 'Product'", - "fieldname": "enforce_free_item_qty", + "fieldname": "dont_enforce_free_item_qty", "fieldtype": "Check", - "label": "Enforce Free Item Qty" + "label": "Don't Enforce Free Item Qty" } ], "icon": "fa fa-gift", "idx": 1, "links": [], - "modified": "2025-02-05 21:03:22.103044", + "modified": "2025-02-17 18:15:39.824639", "modified_by": "Administrator", "module": "Accounts", "name": "Pricing Rule", diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py index f5d15029f09..9f1e16661d5 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py @@ -60,7 +60,7 @@ class PricingRule(Document): disable: DF.Check discount_amount: DF.Currency discount_percentage: DF.Float - enforce_free_item_qty: DF.Check + dont_enforce_free_item_qty: DF.Check for_price_list: DF.Link | None free_item: DF.Link | None free_item_rate: DF.Currency @@ -646,7 +646,7 @@ def remove_pricing_rule_for_item(pricing_rules, item_details, item_code=None, ra if pricing_rule.margin_type in ["Percentage", "Amount"]: item_details.margin_rate_or_amount = 0.0 item_details.margin_type = None - elif pricing_rule.get("free_item") and pricing_rule.get("enforce_free_item_qty"): + elif pricing_rule.get("free_item") and not pricing_rule.get("dont_enforce_free_item_qty"): item_details.remove_free_item = ( item_code if pricing_rule.get("same_item") else pricing_rule.get("free_item") ) diff --git a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py index 3e0a0cac71b..d61b123c67b 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -438,7 +438,7 @@ class TestPricingRule(IntegrationTestCase): self.assertEqual(so.items[1].is_free_item, 1) self.assertEqual(so.items[1].item_code, "_Test Item 2") - def test_enforce_free_item_qty(self): + def test_dont_enforce_free_item_qty(self): # this test is only for testing non-enforcement as all other tests in this file already test with enforcement frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule") test_record = { @@ -477,10 +477,10 @@ class TestPricingRule(IntegrationTestCase): self.assertEqual(len(so.items), 2) # Without enforcement - pricing_rule.enforce_free_item_qty = 0 + pricing_rule.dont_enforce_free_item_qty = 1 pricing_rule.save() - # Test 2 : Deleted free item will not be fetched again on save without enfrocement + # Test 2 : Deleted free item will not be fetched again on save without enforcement so.items.pop(1) so.save() so.reload() @@ -1509,7 +1509,7 @@ def make_pricing_rule(**args): "discount_amount": args.discount_amount or 0.0, "apply_multiple_pricing_rules": args.apply_multiple_pricing_rules or 0, "has_priority": args.has_priority or 0, - "enforce_free_item_qty": args.enforce_free_item_qty or 1, + "enforce_free_item_qty": args.dont_enforce_free_item_qty or 0, } ) diff --git a/erpnext/accounts/doctype/pricing_rule/utils.py b/erpnext/accounts/doctype/pricing_rule/utils.py index e0b53aa580c..da75c0dc7fe 100644 --- a/erpnext/accounts/doctype/pricing_rule/utils.py +++ b/erpnext/accounts/doctype/pricing_rule/utils.py @@ -713,8 +713,8 @@ def apply_pricing_rule_for_free_items(doc, pricing_rule_args): args.pop((item.item_code, item.pricing_rules)) for free_item in args.values(): - if doc.is_new() or frappe.get_value( - "Pricing Rule", free_item["pricing_rules"], "enforce_free_item_qty" + if doc.is_new() or not frappe.get_value( + "Pricing Rule", free_item["pricing_rules"], "dont_enforce_free_item_qty" ): doc.append("items", free_item) diff --git a/erpnext/stock/doctype/pick_list/test_pick_list.py b/erpnext/stock/doctype/pick_list/test_pick_list.py index 7783c5ba1ca..fad31e5f91a 100644 --- a/erpnext/stock/doctype/pick_list/test_pick_list.py +++ b/erpnext/stock/doctype/pick_list/test_pick_list.py @@ -1251,7 +1251,7 @@ class TestPickList(IntegrationTestCase): "is_recursive": 1, "recurse_for": 2, "free_qty": 1, - "enforce_free_item_qty": 1, + "dont_enforce_free_item_qty": 0, "company": "_Test Company", "customer": "_Test Customer", } From 844f1636c0a82dd6e0d0e6e6e3a86bc6652e3ad9 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 17 Feb 2025 18:47:35 +0530 Subject: [PATCH 6/6] fix: set default value to 0 as per new logic --- erpnext/accounts/doctype/pricing_rule/pricing_rule.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json index 8a968e52bbc..31b0f411681 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json @@ -646,7 +646,7 @@ "label": "Has Priority" }, { - "default": "1", + "default": "0", "depends_on": "eval:doc.price_or_product_discount == 'Product'", "fieldname": "dont_enforce_free_item_qty", "fieldtype": "Check",