fix(pricing rule): cumulative for product slab #49358
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
erpnext/accounts/doctype/pricing_rule/utils.py (3)
532-538: Push aggregation into SQL and handle empty results; current row-by-row sum is avoidable.Using SQL aggregates cuts payload and is faster on large histories.
Apply:
-data_set = query.run(as_dict=True) - -for data in data_set: - sum_qty += data.get("stock_qty") - sum_amt += data.get("amount") - -return [sum_qty, sum_amt] +from frappe.query_builder import functions as fn +agg = query.select( + fn.Sum(child.stock_qty).as_("sum_qty"), + fn.Sum(child.amount).as_("sum_amt"), +).run(as_dict=True) +row = agg[0] if agg else {} +return [flt(row.get("sum_qty")) if row.get("sum_qty") is not None else 0, flt(row.get("sum_amt")) if row.get("sum_amt") is not None else 0]
288-295: Consider summing cumulative quantities across the entire rule scope (all qualifying items), not just the current row’s item value.Passing only
[args.get(scrub(pr_doc.apply_on))]can undercount when the rule applies to a parent Item Group/Brand with multiple items. Usingget_pricing_rule_items(pr_doc)here would align with mixed-conditions handling.If acceptable, I can draft a targeted patch for this call site.
501-539: Add tests (label: needs-tests).Please cover:
- Cumulative product free-item rule across multiple invoices of the same customer and company; verify correct deduction and non-negative clamp.
- Free item different from purchased item; ensure past free items are counted using
pricing_rulesanditem_code.- Open-ended validity (no
valid_upto); ensure results included via default bounds.- Warehouse scoping with a parent warehouse (child expansion).
- Backward compatibility: caller passes
argsdict vsdocDocument.I can provide a minimal test matrix for Sales Order and Sales Invoice. Do you want me to draft it?
Also applies to: 647-652
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
erpnext/accounts/doctype/pricing_rule/utils.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/accounts/doctype/pricing_rule/utils.py (1)
501-506: Retain dict guard or migrate caller at utils.py:290Call site at line 290 still passes the raw args dict (the others at 466/649 pass a Document). The proposed
isinstance(doc, dict)branch prevents an AttributeError for that case; decide whether to update the L290 caller to pass the actual Document instead ofargs, or keep the dict branch in place.
Comment on lines
+514
to
+524
| parent = frappe.qb.DocType(doctype) | ||
| child = frappe.qb.DocType(child_doctype) | ||
| query = ( | ||
| frappe.qb.from_(child) | ||
| .join(parent) | ||
| .on(child.parent == parent.name) | ||
| .select(child.stock_qty, child.amount) | ||
| .where(parent[date_field].between(pr_doc.valid_from, pr_doc.valid_upto)) | ||
| .where(parent.docstatus == 1) | ||
| .where(child.is_free_item == free_item) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scope cumulative query by validity dates (with open-ended bounds), company, and party; otherwise results leak across all customers/suppliers.
- If
valid_from/valid_uptois null,.between()with None will miss records; use safe bounds. - Restrict by
companyand the relevant party (customerfor selling,supplierfor buying); otherwise cumulative tallies include other parties’ documents.
Apply:
- parent = frappe.qb.DocType(doctype)
- child = frappe.qb.DocType(child_doctype)
- query = (
- frappe.qb.from_(child)
- .join(parent)
- .on(child.parent == parent.name)
- .select(child.stock_qty, child.amount)
- .where(parent[date_field].between(pr_doc.valid_from, pr_doc.valid_upto))
- .where(parent.docstatus == 1)
- .where(child.is_free_item == free_item)
- )
+ parent = frappe.qb.DocType(doctype)
+ child = frappe.qb.DocType(child_doctype)
+ from_date = pr_doc.valid_from or getdate("2000-01-01")
+ to_date = pr_doc.valid_upto or getdate("2500-12-31")
+ query = (
+ frappe.qb.from_(child)
+ .join(parent)
+ .on(child.parent == parent.name)
+ .where(parent[date_field].between(from_date, to_date))
+ .where(parent.docstatus == 1)
+ .where(child.is_free_item == free_item)
+ )
+ # company / party scoping
+ company = (doc.get("company") if isinstance(doc, dict) else getattr(doc, "company", None))
+ if company:
+ query = query.where(parent.company == company)
+ party_field = "customer" if getattr(pr_doc, "selling", 0) else ("supplier" if getattr(pr_doc, "buying", 0) else None)
+ if party_field:
+ party_val = (doc.get(party_field) if isinstance(doc, dict) else getattr(doc, party_field, None))
+ if party_val:
+ query = query.where(parent[party_field] == party_val)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
erpnext/accounts/doctype/pricing_rule/utils.py around lines 514-524: the date
range and party/company scoping for the cumulative query are too loose and miss
records when valid_from/valid_upto are null; change the .between() to explicit
>=/<= checks using safe bounds (use a very early min date when valid_from is
None and a very late max date when valid_upto is None) and add WHERE filters for
parent.company and for the relevant party field (parent.customer when
pr_doc.party_type == "Customer", parent.supplier when pr_doc.party_type ==
"Supplier") so results are limited to the same company and party; keep the
existing docstatus and is_free_item filters.
Comment on lines
529
to
533
| if items: | ||
| condition += " and `tab{child_doc}`.{apply_on} in ({items})".format( | ||
| child_doc=child_doctype, apply_on=apply_on, items=",".join(["%s"] * len(items)) | ||
| ) | ||
| query = query.where(child[apply_on].isin(items)) | ||
|
|
||
| values.extend(items) | ||
|
|
||
| data_set = frappe.db.sql( | ||
| f""" SELECT `tab{child_doctype}`.stock_qty, | ||
| `tab{child_doctype}`.amount | ||
| FROM `tab{child_doctype}`, `tab{doctype}` | ||
| WHERE | ||
| `tab{child_doctype}`.parent = `tab{doctype}`.name and `tab{doctype}`.{date_field} | ||
| between %s and %s and `tab{doctype}`.docstatus = 1 | ||
| {condition} group by `tab{child_doctype}`.name | ||
| """, | ||
| tuple(values), | ||
| as_dict=1, | ||
| ) | ||
| data_set = query.run(as_dict=True) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the filter dimension when counting free items; also bind to this Pricing Rule to avoid cross-rule bleed.
- When
free_item=True, the lookup must filter bychild.item_code(free item code), not the purchased-apply field (apply_on). Otherwise it will miss free items if the free product differs from the purchased item, and it may count unrelated rows. - Additionally, restrict to
child.pricing_rules == pr_doc.nameso past free items from other rules aren’t deducted.
Apply:
-if items:
- query = query.where(child[apply_on].isin(items))
+if items:
+ filter_field = "item_code" if free_item else apply_on
+ query = query.where(child[filter_field].isin(items))
+if free_item:
+ # ensure we only deduct free items given by this rule
+ query = query.where(child.pricing_rules == pr_doc.name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if items: | |
| condition += " and `tab{child_doc}`.{apply_on} in ({items})".format( | |
| child_doc=child_doctype, apply_on=apply_on, items=",".join(["%s"] * len(items)) | |
| ) | |
| query = query.where(child[apply_on].isin(items)) | |
| values.extend(items) | |
| data_set = frappe.db.sql( | |
| f""" SELECT `tab{child_doctype}`.stock_qty, | |
| `tab{child_doctype}`.amount | |
| FROM `tab{child_doctype}`, `tab{doctype}` | |
| WHERE | |
| `tab{child_doctype}`.parent = `tab{doctype}`.name and `tab{doctype}`.{date_field} | |
| between %s and %s and `tab{doctype}`.docstatus = 1 | |
| {condition} group by `tab{child_doctype}`.name | |
| """, | |
| tuple(values), | |
| as_dict=1, | |
| ) | |
| data_set = query.run(as_dict=True) | |
| if items: | |
| # choose the right field: item_code for free items, otherwise the original apply_on field | |
| filter_field = "item_code" if free_item else apply_on | |
| query = query.where(child[filter_field].isin(items)) | |
| if free_item: | |
| # ensure we only deduct free items given by this rule | |
| query = query.where(child.pricing_rules == pr_doc.name) | |
| data_set = query.run(as_dict=True) |
🤖 Prompt for AI Agents
In erpnext/accounts/doctype/pricing_rule/utils.py around lines 529 to 533, the
query filter currently uses child[apply_on] when counting free items; change
this to filter by child.item_code when free_item is True and also add a filter
to bind the lookup to this pricing rule (child.pricing_rules == pr_doc.name) so
results only count free items issued by the same rule—apply both conditions to
the query.where chain before running query.run(as_dict=True).
Comment on lines
+647
to
+652
| if pricing_rule.is_cumulative: | ||
| items = [args.get(frappe.scrub(pricing_rule.get("apply_on")))] | ||
| data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True) | ||
| if data and data[0]: | ||
| qty -= data[0] | ||
|
|
There was a problem hiding this comment.
Deduct previously issued free items correctly and avoid negative quantities.
- For cumulative free products, subtract past free items for this rule and free item code; pass the free item code to the lookup and clamp to ≥ 0. Current code filters by the purchased apply-on field, which is wrong if the free product differs.
Apply:
-if pricing_rule.is_cumulative:
- items = [args.get(frappe.scrub(pricing_rule.get("apply_on")))]
- data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True)
- if data and data[0]:
- qty -= data[0]
+if pricing_rule.is_cumulative:
+ # look back on the actual free item (and same rule), not the purchased apply-on
+ items = [free_item]
+ data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True)
+ issued = flt(data[0]) if data else 0
+ qty = max(0, flt(qty) - issued)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if pricing_rule.is_cumulative: | |
| items = [args.get(frappe.scrub(pricing_rule.get("apply_on")))] | |
| data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True) | |
| if data and data[0]: | |
| qty -= data[0] | |
| if pricing_rule.is_cumulative: | |
| # look back on the actual free item (and same rule), not the purchased apply-on | |
| items = [free_item] | |
| data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True) | |
| issued = flt(data[0]) if data else 0 | |
| qty = max(0, flt(qty) - issued) |
🤖 Prompt for AI Agents
In erpnext/accounts/doctype/pricing_rule/utils.py around lines 647 to 652, the
code subtracts previously issued free items using the purchased "apply_on" item
instead of the actual free item code and doesn't prevent negative qty; change
the items list to use the pricing rule's free item (e.g.
frappe.scrub(pricing_rule.get("free_item"))) when calling
get_qty_amount_data_for_cumulative, pass free_item=True as before, and clamp the
resulting qty with qty = max(0, qty - data[0]) so qty cannot go below zero.
|
@ravibharathi656 can you add test case? |
ravibharathi656
force-pushed
the
pricing-rule-cumulative-discount
branch
from
24fbaa3 to
dd800a9
Compare
|
@rohitwaghchaure added test case |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
erpnext/accounts/doctype/pricing_rule/utils.py (3)
514-524: Fix date bounds and scope by company/party; current query can miss open-ended ranges and leak across customers.
- between(None, None) drops rows when valid_from/upto are empty.
- Missing company/customer/supplier filters can over-count cumulative data.
Apply:
- parent = frappe.qb.DocType(doctype) - child = frappe.qb.DocType(child_doctype) - query = ( - frappe.qb.from_(child) - .join(parent) - .on(child.parent == parent.name) - .select(child.stock_qty, child.amount) - .where(parent[date_field].between(pr_doc.valid_from, pr_doc.valid_upto)) - .where(parent.docstatus == 1) - .where(child.is_free_item == free_item) - ) + parent = frappe.qb.DocType(doctype) + child = frappe.qb.DocType(child_doctype) + from_date = pr_doc.valid_from or getdate("2000-01-01") + to_date = pr_doc.valid_upto or getdate("2500-12-31") + query = ( + frappe.qb.from_(child) + .join(parent) + .on(child.parent == parent.name) + .select(child.stock_qty, child.amount) + .where(parent[date_field] >= from_date) + .where(parent[date_field] <= to_date) + .where(parent.docstatus == 1) + .where(child.is_free_item == free_item) + ) + # scope to same company and party + company = (doc.get("company") if isinstance(doc, dict) else getattr(doc, "company", None)) + if company: + query = query.where(parent.company == company) + party_field = "customer" if getattr(pr_doc, "selling", 0) else ("supplier" if getattr(pr_doc, "buying", 0) else None) + if party_field: + party_val = (doc.get(party_field) if isinstance(doc, dict) else getattr(doc, party_field, None)) + if party_val: + query = query.where(parent[party_field] == party_val)
529-533: Filter dimension is wrong for free items; also bind to this rule to avoid cross-rule deductions.When free_item=True, filter by child.item_code, not the purchased apply_on, and add child.pricing_rules == pr_doc.name.
Apply:
- if items: - query = query.where(child[apply_on].isin(items)) + if items: + filter_field = "item_code" if free_item else apply_on + query = query.where(child[filter_field].isin(items)) + if free_item: + query = query.where(child.pricing_rules == pr_doc.name)
647-652: Deduct previously issued free items by the actual free item code; clamp to ≥ 0.Current code subtracts using the purchased apply_on, which breaks when free_item != purchased item and can go negative.
Apply:
- if pricing_rule.is_cumulative: - items = [args.get(frappe.scrub(pricing_rule.get("apply_on")))] - data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True) - if data and data[0]: - qty -= data[0] + if pricing_rule.is_cumulative: + items = [free_item] + data = get_qty_amount_data_for_cumulative(pricing_rule, doc, items, free_item=True) + issued = flt(data[0]) if data else 0 + qty = max(0, flt(qty) - issued)
🧹 Nitpick comments (1)
erpnext/accounts/doctype/pricing_rule/utils.py (1)
532-538: Micro-opt: aggregate in SQL to cut memory and Python loops.Use SUM() in the select and fetch a single row.
Apply:
- query = ( + from pypika.functions import Sum # top of file or inline import + query = ( ... - .select(child.stock_qty, child.amount) + .select(Sum(child.stock_qty).as_("stock_qty"), Sum(child.amount).as_("amount")) ... ) - data_set = query.run(as_dict=True) - - for data in data_set: - sum_qty += data.get("stock_qty") - sum_amt += data.get("amount") + data = (query.run(as_dict=True) or [{}])[0] + sum_qty += flt(data.get("stock_qty")) + sum_amt += flt(data.get("amount"))
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
erpnext/accounts/doctype/pricing_rule/utils.py(3 hunks)erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py (1)
7-7: LGTM on deterministic validity window.Importing nowdate() keeps the test stable across days.
erpnext/accounts/doctype/pricing_rule/utils.py (1)
501-501: Signature change looks good.Backward compatible thanks to default free_item=False.
Comment on lines
+177
to
+220
| def test_cumulative_product_discount_slabs(self): | ||
| ps = make_promotional_scheme(applicable_for="Customer", customer="_Test Customer 1") | ||
| ps.is_cumulative = True | ||
| ps.valid_from = ps.valid_upto = nowdate() | ||
| ps.set("price_discount_slabs", []) | ||
| ps.set("items", []) | ||
| ps.set( | ||
| "items", | ||
| [ | ||
| { | ||
| "item_code": "_Test Item 2", | ||
| } | ||
| ], | ||
| ) | ||
| ps.set( | ||
| "product_discount_slabs", | ||
| [ | ||
| { | ||
| "rule_description": "Test1", | ||
| "min_qty": 500, | ||
| "max_qty": 500, | ||
| "same_item": True, | ||
| "free_qty": 50, | ||
| }, | ||
| { | ||
| "rule_description": "Test2", | ||
| "min_qty": 1000, | ||
| "max_qty": 1000, | ||
| "same_item": True, | ||
| "free_qty": 120, | ||
| }, | ||
| ], | ||
| ) | ||
| ps.save() | ||
|
|
||
| so1 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500) | ||
| self.assertEqual(so1.items[1].qty, 50) | ||
|
|
||
| so2 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500) | ||
| self.assertEqual(so2.items[1].qty, 70) | ||
|
|
||
| so1.reload().cancel().delete() | ||
| so2.reload().cancel().delete() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make assertions robust and clean up created scheme.
Indexing items[1] is brittle. Assert on is_free_item and ensure the scheme is deleted to avoid test leakage.
Apply:
- so1 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500)
- self.assertEqual(so1.items[1].qty, 50)
+ so1 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500)
+ free1 = [d for d in so1.items if d.is_free_item]
+ self.assertEqual(len(free1), 1)
+ self.assertEqual(free1[0].qty, 50)
- so2 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500)
- self.assertEqual(so2.items[1].qty, 70)
+ so2 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500)
+ free2 = [d for d in so2.items if d.is_free_item]
+ self.assertEqual(len(free2), 1)
+ self.assertEqual(free2[0].qty, 70)
so1.reload().cancel().delete()
so2.reload().cancel().delete()
+ frappe.delete_doc("Promotional Scheme", ps.name)Want me to add a companion test where free_item != purchased item to guard the general case?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_cumulative_product_discount_slabs(self): | |
| ps = make_promotional_scheme(applicable_for="Customer", customer="_Test Customer 1") | |
| ps.is_cumulative = True | |
| ps.valid_from = ps.valid_upto = nowdate() | |
| ps.set("price_discount_slabs", []) | |
| ps.set("items", []) | |
| ps.set( | |
| "items", | |
| [ | |
| { | |
| "item_code": "_Test Item 2", | |
| } | |
| ], | |
| ) | |
| ps.set( | |
| "product_discount_slabs", | |
| [ | |
| { | |
| "rule_description": "Test1", | |
| "min_qty": 500, | |
| "max_qty": 500, | |
| "same_item": True, | |
| "free_qty": 50, | |
| }, | |
| { | |
| "rule_description": "Test2", | |
| "min_qty": 1000, | |
| "max_qty": 1000, | |
| "same_item": True, | |
| "free_qty": 120, | |
| }, | |
| ], | |
| ) | |
| ps.save() | |
| so1 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500) | |
| self.assertEqual(so1.items[1].qty, 50) | |
| so2 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500) | |
| self.assertEqual(so2.items[1].qty, 70) | |
| so1.reload().cancel().delete() | |
| so2.reload().cancel().delete() | |
| def test_cumulative_product_discount_slabs(self): | |
| ps = make_promotional_scheme(applicable_for="Customer", customer="_Test Customer 1") | |
| ps.is_cumulative = True | |
| ps.valid_from = ps.valid_upto = nowdate() | |
| ps.set("price_discount_slabs", []) | |
| ps.set("items", []) | |
| ps.set( | |
| "items", | |
| [ | |
| { | |
| "item_code": "_Test Item 2", | |
| } | |
| ], | |
| ) | |
| ps.set( | |
| "product_discount_slabs", | |
| [ | |
| { | |
| "rule_description": "Test1", | |
| "min_qty": 500, | |
| "max_qty": 500, | |
| "same_item": True, | |
| "free_qty": 50, | |
| }, | |
| { | |
| "rule_description": "Test2", | |
| "min_qty": 1000, | |
| "max_qty": 1000, | |
| "same_item": True, | |
| "free_qty": 120, | |
| }, | |
| ], | |
| ) | |
| ps.save() | |
| so1 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500) | |
| free1 = [d for d in so1.items if d.is_free_item] | |
| self.assertEqual(len(free1), 1) | |
| self.assertEqual(free1[0].qty, 50) | |
| so2 = make_sales_order(customer="_Test Customer 1", item_code="_Test Item 2", qty=500) | |
| free2 = [d for d in so2.items if d.is_free_item] | |
| self.assertEqual(len(free2), 1) | |
| self.assertEqual(free2[0].qty, 70) | |
| so1.reload().cancel().delete() | |
| so2.reload().cancel().delete() | |
| frappe.delete_doc("Promotional Scheme", ps.name) |
🤖 Prompt for AI Agents
In erpnext/accounts/doctype/promotional_scheme/test_promotional_scheme.py around
lines 177 to 220, the test uses brittle indexing (items[1]) when asserting
free-item quantities and never deletes the created promotional scheme; update
the assertions to locate the free item via its is_free_item flag (e.g., find the
item where is_free_item is True and assert its qty equals the expected free_qty
for each order), and after cancelling/deleting the sales orders, delete the
promotional scheme (ps.reload().delete() or ps.delete()) to prevent test
leakage; keep existing creation/cancel/delete of sales orders intact.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: When a promotion scheme is cumulative and has multiple product discount slabs, the product discount is not calculated correctly.
Ref: 46833
Before :
Screen.Recording.2025-08-28.at.1.44.15.PM.mov
After:
Screen.Recording.2025-08-28.at.1.32.36.PM.mov
Backport needed: v15