From a248dfb9a5f58fae3219d9bbb7bcc1fa8e5d4bfe Mon Sep 17 00:00:00 2001 From: Rohit Waghchaure Date: Thu, 16 Jul 2020 17:27:26 +0530 Subject: [PATCH 1/2] fix: multiple pricing rules are not working on selling side --- .../doctype/pricing_rule/pricing_rule.py | 38 ++++++++++--------- .../accounts/doctype/pricing_rule/utils.py | 15 ++++++++ erpnext/controllers/accounts_controller.py | 17 +++++++++ erpnext/controllers/taxes_and_totals.py | 11 ++++-- 4 files changed, 59 insertions(+), 22 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py index 454776e4e7..149c47673c 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.py @@ -60,6 +60,15 @@ class PricingRule(Document): if self.price_or_product_discount == 'Price' and not self.rate_or_discount: throw(_("Rate or Discount is required for the price discount."), frappe.MandatoryError) + if self.apply_discount_on_rate: + if not self.priority: + throw(_("As the field {0} is enabled, the field {1} is mandatory.") + .format(frappe.bold("Apply Discount on Discounted Rate"), frappe.bold("Priority"))) + + if self.priority and cint(self.priority) == 1: + throw(_("As the field {0} is enabled, the value of the field {1} should be more than 1.") + .format(frappe.bold("Apply Discount on Discounted Rate"), frappe.bold("Priority"))) + def validate_applicable_for_selling_or_buying(self): if not self.selling and not self.buying: throw(_("Atleast one of the Selling or Buying must be selected")) @@ -226,12 +235,11 @@ def get_pricing_rule_for_item(args, price_list_rate=0, doc=None, for_validate=Fa item_details = frappe._dict({ "doctype": args.doctype, + "has_margin": False, "name": args.name, "parent": args.parent, "parenttype": args.parenttype, - "child_docname": args.get('child_docname'), - "discount_percentage_on_rate": [], - "discount_amount_on_rate": [] + "child_docname": args.get('child_docname') }) if args.ignore_pricing_rule or not args.item_code: @@ -279,6 +287,10 @@ def get_pricing_rule_for_item(args, price_list_rate=0, doc=None, for_validate=Fa else: get_product_discount_rule(pricing_rule, item_details, args, doc) + if not item_details.get("has_margin"): + item_details.margin_type = None + item_details.margin_rate_or_amount = 0.0 + item_details.has_pricing_rule = 1 item_details.pricing_rules = frappe.as_json([d.pricing_rule for d in rules]) @@ -330,13 +342,11 @@ def get_pricing_rule_details(args, pricing_rule): def apply_price_discount_rule(pricing_rule, item_details, args): item_details.pricing_rule_for = pricing_rule.rate_or_discount - if ((pricing_rule.margin_type == 'Amount' and pricing_rule.currency == args.currency) + if ((pricing_rule.margin_type in ['Amount', 'Percentage'] and pricing_rule.currency == args.currency) or (pricing_rule.margin_type == 'Percentage')): item_details.margin_type = pricing_rule.margin_type item_details.margin_rate_or_amount = pricing_rule.margin_rate_or_amount - else: - item_details.margin_type = None - item_details.margin_rate_or_amount = 0.0 + item_details.has_margin = True if pricing_rule.rate_or_discount == 'Rate': pricing_rule_rate = 0.0 @@ -351,9 +361,9 @@ def apply_price_discount_rule(pricing_rule, item_details, args): if pricing_rule.rate_or_discount != apply_on: continue field = frappe.scrub(apply_on) - if pricing_rule.apply_discount_on_rate: - discount_field = "{0}_on_rate".format(field) - item_details[discount_field].append(pricing_rule.get(field, 0)) + if pricing_rule.apply_discount_on_rate and item_details.get("discount_percentage"): + # Apply discount on discounted rate + item_details[field] += ((100 - item_details[field]) * (pricing_rule.get(field, 0) / 100)) else: if field not in item_details: item_details.setdefault(field, 0) @@ -361,14 +371,6 @@ def apply_price_discount_rule(pricing_rule, item_details, args): item_details[field] += (pricing_rule.get(field, 0) if pricing_rule else args.get(field, 0)) -def set_discount_amount(rate, item_details): - for field in ['discount_percentage_on_rate', 'discount_amount_on_rate']: - for d in item_details.get(field): - dis_amount = (rate * d / 100 - if field == 'discount_percentage_on_rate' else d) - rate -= dis_amount - item_details.rate = rate - def remove_pricing_rule_for_item(pricing_rules, item_details, item_code=None): from erpnext.accounts.doctype.pricing_rule.utils import (get_applied_pricing_rules, get_pricing_rule_items) diff --git a/erpnext/accounts/doctype/pricing_rule/utils.py b/erpnext/accounts/doctype/pricing_rule/utils.py index 25840d4fb7..1a58b1dba1 100644 --- a/erpnext/accounts/doctype/pricing_rule/utils.py +++ b/erpnext/accounts/doctype/pricing_rule/utils.py @@ -42,6 +42,7 @@ def get_pricing_rules(args, doc=None): if not pricing_rules: return [] if apply_multiple_pricing_rules(pricing_rules): + pricing_rules = sorted_by_priority(pricing_rules) for pricing_rule in pricing_rules: pricing_rule = filter_pricing_rules(args, pricing_rule, doc) if pricing_rule: @@ -53,6 +54,20 @@ def get_pricing_rules(args, doc=None): return rules +def sorted_by_priority(pricing_rules): + # If more than one pricing rules, then sort by priority + pricing_rules_list = [] + pricing_rule_dict = {} + for pricing_rule in pricing_rules: + if not pricing_rule.get("priority"): continue + + pricing_rule_dict.setdefault(cint(pricing_rule.get("priority")), []).append(pricing_rule) + + for key in sorted(pricing_rule_dict): + pricing_rules_list.append(pricing_rule_dict.get(key)) + + return pricing_rules_list or pricing_rules + def filter_pricing_rule_based_on_condition(pricing_rules, doc=None): filtered_pricing_rules = [] if doc: diff --git a/erpnext/controllers/accounts_controller.py b/erpnext/controllers/accounts_controller.py index 983cfa8c15..6108a614ca 100644 --- a/erpnext/controllers/accounts_controller.py +++ b/erpnext/controllers/accounts_controller.py @@ -263,6 +263,7 @@ class AccountsController(TransactionBase): if self.doctype == "Quotation" and self.quotation_to == "Customer" and parent_dict.get("party_name"): parent_dict.update({"customer": parent_dict.get("party_name")}) + self.pricing_rules = [] for item in self.get("items"): if item.get("item_code"): args = parent_dict.copy() @@ -301,6 +302,7 @@ class AccountsController(TransactionBase): if ret.get("pricing_rules"): self.apply_pricing_rule_on_items(item, ret) + self.set_pricing_rule_details(item, ret) if self.doctype == "Purchase Invoice": self.set_expense_account(for_validate) @@ -322,6 +324,9 @@ class AccountsController(TransactionBase): if item.get('discount_amount'): item.rate = item.price_list_rate - item.discount_amount + if item.get("apply_discount_on_discounted_rate") and pricing_rule_args.get("rate"): + item.rate = pricing_rule_args.get("rate") + elif pricing_rule_args.get('free_item_data'): apply_pricing_rule_for_free_items(self, pricing_rule_args.get('free_item_data')) @@ -335,6 +340,18 @@ class AccountsController(TransactionBase): frappe.msgprint(_("Row {0}: user has not applied the rule {1} on the item {2}") .format(item.idx, frappe.bold(title), frappe.bold(item.item_code))) + def set_pricing_rule_details(self, item_row, args): + pricing_rules = get_applied_pricing_rules(args.get("pricing_rules")) + if not pricing_rules: return + + for pricing_rule in pricing_rules: + self.append("pricing_rules", { + "pricing_rule": pricing_rule, + "item_code": item_row.item_code, + "child_docname": item_row.name, + "rule_applied": True + }) + def set_taxes(self): if not self.meta.get_field("taxes"): return diff --git a/erpnext/controllers/taxes_and_totals.py b/erpnext/controllers/taxes_and_totals.py index 92cfdb7f1a..81d07c1327 100644 --- a/erpnext/controllers/taxes_and_totals.py +++ b/erpnext/controllers/taxes_and_totals.py @@ -608,16 +608,19 @@ class calculate_taxes_and_totals(object): base_rate_with_margin = 0.0 if item.price_list_rate: if item.pricing_rules and not self.doc.ignore_pricing_rule: + has_margin = False for d in get_applied_pricing_rules(item.pricing_rules): pricing_rule = frappe.get_cached_doc('Pricing Rule', d) - if (pricing_rule.margin_type == 'Amount' and pricing_rule.currency == self.doc.currency)\ + if (pricing_rule.margin_type in ['Amount', 'Percentage'] and pricing_rule.currency == self.doc.currency)\ or (pricing_rule.margin_type == 'Percentage'): item.margin_type = pricing_rule.margin_type item.margin_rate_or_amount = pricing_rule.margin_rate_or_amount - else: - item.margin_type = None - item.margin_rate_or_amount = 0.0 + has_margin = True + + if not has_margin: + item.margin_type = None + item.margin_rate_or_amount = 0.0 if item.margin_type and item.margin_rate_or_amount: margin_value = item.margin_rate_or_amount if item.margin_type == 'Amount' else flt(item.price_list_rate) * flt(item.margin_rate_or_amount) / 100 From 0fa741b267631d7fbc16fae098905eebb6cb5b80 Mon Sep 17 00:00:00 2001 From: Rohit Waghchaure Date: Wed, 28 Oct 2020 16:51:42 +0530 Subject: [PATCH 2/2] added test cases --- .../doctype/pricing_rule/pricing_rule.json | 6 +-- .../doctype/pricing_rule/test_pricing_rule.py | 48 +++++++++++++++---- .../accounts/doctype/pricing_rule/utils.py | 14 +++--- 3 files changed, 50 insertions(+), 18 deletions(-) diff --git a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json index c681c897fc..cc8ed4bc49 100644 --- a/erpnext/accounts/doctype/pricing_rule/pricing_rule.json +++ b/erpnext/accounts/doctype/pricing_rule/pricing_rule.json @@ -504,10 +504,10 @@ }, { "default": "0", - "depends_on": "eval:in_list(['Discount Percentage', 'Discount Amount'], doc.rate_or_discount) && doc.apply_multiple_pricing_rules", + "depends_on": "eval:in_list(['Discount Percentage'], doc.rate_or_discount) && doc.apply_multiple_pricing_rules", "fieldname": "apply_discount_on_rate", "fieldtype": "Check", - "label": "Apply Discount on Rate" + "label": "Apply Discount on Discounted Rate" }, { "default": "0", @@ -563,7 +563,7 @@ "icon": "fa fa-gift", "idx": 1, "links": [], - "modified": "2020-08-26 12:24:44.740734", + "modified": "2020-10-28 16:53:14.416172", "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 3555ca895f..22a031c162 100644 --- a/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py +++ b/erpnext/accounts/doctype/pricing_rule/test_pricing_rule.py @@ -385,7 +385,7 @@ class TestPricingRule(unittest.TestCase): so.load_from_db() self.assertEqual(so.items[1].is_free_item, 1) self.assertEqual(so.items[1].item_code, "_Test Item 2") - + def test_cumulative_pricing_rule(self): frappe.delete_doc_if_exists('Pricing Rule', '_Test Cumulative Pricing Rule') test_record = { @@ -429,34 +429,61 @@ class TestPricingRule(unittest.TestCase): details = get_item_details(args) self.assertTrue(details) - + def test_pricing_rule_for_condition(self): frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule") - + make_pricing_rule(selling=1, margin_type="Percentage", \ condition="customer=='_Test Customer 1' and is_return==0", discount_percentage=10) - + # Incorrect Customer and Correct is_return value si = create_sales_invoice(do_not_submit=True, customer="_Test Customer 2", is_return=0) si.items[0].price_list_rate = 1000 si.submit() item = si.items[0] self.assertEquals(item.rate, 100) - + # Correct Customer and Incorrect is_return value si = create_sales_invoice(do_not_submit=True, customer="_Test Customer 1", is_return=1, qty=-1) si.items[0].price_list_rate = 1000 si.submit() item = si.items[0] self.assertEquals(item.rate, 100) - + # Correct Customer and correct is_return value si = create_sales_invoice(do_not_submit=True, customer="_Test Customer 1", is_return=0) si.items[0].price_list_rate = 1000 si.submit() item = si.items[0] self.assertEquals(item.rate, 900) - + + def test_multiple_pricing_rules(self): + make_pricing_rule(discount_percentage=20, selling=1, priority=1, apply_multiple_pricing_rules=1, + title="_Test Pricing Rule 1") + make_pricing_rule(discount_percentage=10, selling=1, title="_Test Pricing Rule 2", priority=2, + apply_multiple_pricing_rules=1) + si = create_sales_invoice(do_not_submit=True, customer="_Test Customer 1", qty=1) + self.assertEqual(si.items[0].discount_percentage, 30) + si.delete() + + frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 1") + frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 2") + + def test_multiple_pricing_rules_with_apply_discount_on_discounted_rate(self): + frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule") + + make_pricing_rule(discount_percentage=20, selling=1, priority=1, apply_multiple_pricing_rules=1, + title="_Test Pricing Rule 1") + make_pricing_rule(discount_percentage=10, selling=1, priority=2, + apply_discount_on_rate=1, title="_Test Pricing Rule 2", apply_multiple_pricing_rules=1) + + si = create_sales_invoice(do_not_submit=True, customer="_Test Customer 1", qty=1) + self.assertEqual(si.items[0].discount_percentage, 28) + si.delete() + + frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 1") + frappe.delete_doc_if_exists("Pricing Rule", "_Test Pricing Rule 2") + def make_pricing_rule(**args): args = frappe._dict(args) @@ -468,6 +495,7 @@ def make_pricing_rule(**args): "applicable_for": args.applicable_for, "selling": args.selling or 0, "currency": "USD", + "apply_discount_on_rate": args.apply_discount_on_rate or 0, "buying": args.buying or 0, "min_qty": args.min_qty or 0.0, "max_qty": args.max_qty or 0.0, @@ -476,9 +504,13 @@ def make_pricing_rule(**args): "rate": args.rate or 0.0, "margin_type": args.margin_type, "margin_rate_or_amount": args.margin_rate_or_amount or 0.0, - "condition": args.condition or '' + "condition": args.condition or '', + "apply_multiple_pricing_rules": args.apply_multiple_pricing_rules or 0 }) + if args.get("priority"): + doc.priority = args.get("priority") + apply_on = doc.apply_on.replace(' ', '_').lower() child_table = {'Item Code': 'items', 'Item Group': 'item_groups', 'Brand': 'brands'} doc.append(child_table.get(doc.apply_on), { diff --git a/erpnext/accounts/doctype/pricing_rule/utils.py b/erpnext/accounts/doctype/pricing_rule/utils.py index 1a58b1dba1..b003328cc4 100644 --- a/erpnext/accounts/doctype/pricing_rule/utils.py +++ b/erpnext/accounts/doctype/pricing_rule/utils.py @@ -14,9 +14,8 @@ import frappe from erpnext.setup.doctype.item_group.item_group import get_child_item_groups from erpnext.stock.doctype.warehouse.warehouse import get_child_warehouses from erpnext.stock.get_item_details import get_conversion_factor -from frappe import _, throw -from frappe.utils import cint, flt, get_datetime, get_link_to_form, getdate, today - +from frappe import _, bold +from frappe.utils import cint, flt, get_link_to_form, getdate, today, fmt_money class MultiplePricingRuleConflict(frappe.ValidationError): pass @@ -299,12 +298,13 @@ def validate_quantity_and_amount_for_suggestion(args, qty, amount, item_code, tr fieldname = field if fieldname: - msg = _("""If you {0} {1} quantities of the item {2}, the scheme {3} - will be applied on the item.""").format(type_of_transaction, args.get(fieldname), item_code, args.rule_description) + msg = (_("If you {0} {1} quantities of the item {2}, the scheme {3} will be applied on the item.") + .format(type_of_transaction, args.get(fieldname), bold(item_code), bold(args.rule_description))) if fieldname in ['min_amt', 'max_amt']: - msg = _("""If you {0} {1} worth item {2}, the scheme {3} will be applied on the item. - """).format(frappe.fmt_money(type_of_transaction, args.get(fieldname)), item_code, args.rule_description) + msg = (_("If you {0} {1} worth item {2}, the scheme {3} will be applied on the item.") + .format(type_of_transaction, fmt_money(args.get(fieldname), currency=args.get("currency")), + bold(item_code), bold(args.rule_description))) frappe.msgprint(msg)