Apply discount to min price items

[MostafaKadry]

No description provided.

[engahmed1190]

Detailed Code Review: PR #95 - Apply Discount to Min/Max Price Items

Thank you for this feature! The concept of applying discounts to cheapest/most expensive items is valuable for promotions like "Buy 3, get the cheapest one 50% off". However, I've identified several issues that need to be addressed before merging.

---

🔴 Critical Issues

1. Missing Translation Import (Will Crash)

File: pos_next/overrides/pricing_rule.py, Line 126

frappe.throw(
    _("Failed to apply pricing rule discounts..."),  # _() is not imported!
    title="Min/Max Pricing Rule Failed"
)

Fix: Add from frappe import _ at the top of the file.

---

2. Monkey Patching at Module Load Time (Risky)

File: pos_next/hooks.py, Lines 255-260

try:
    from erpnext.accounts.doctype.pricing_rule import pricing_rule as pricing_rule_module
    from pos_next.overrides.pricing_rule import apply_price_discount_rule
    pricing_rule_module.apply_price_discount_rule = apply_price_discount_rule
except Exception as e:
    frappe.log_error(frappe.get_traceback(), "Error in pricing rule module")

Problems:

• Runs at import time, before Frappe is fully initialized
• frappe.log_error() may fail if called before DB connection is established
• This affects ALL transactions globally, not just POS

Recommended Fix: Use after_migrate hook to apply the patch safely:

# In hooks.py
after_migrate = ["pos_next.install.after_migrate", "pos_next.overrides.pricing_rule.patch_pricing_rule"]

# In pricing_rule.py, add:
def patch_pricing_rule():
    """Safely patch the pricing rule module after Frappe is initialized."""
    try:
        from erpnext.accounts.doctype.pricing_rule import pricing_rule as pricing_rule_module
        pricing_rule_module.apply_price_discount_rule = apply_price_discount_rule
    except Exception:
        frappe.log_error("Failed to patch pricing rule module for Min/Max discounts")

---

3. qty_limit = 0 Bug (Critical Logic Error)

File: pos_next/overrides/pricing_rule.py, Lines 91, 100

remaining_qty = flt(pr.min_or_max_discount_qty_limit or 0)  # Line 91

# ...

if remaining_qty <= 0:  # Line 100 - Always true when limit is 0!
    item.discount_percentage = 0.0
    item.discount_amount = 0.0
    continue

Problem: The custom field description says "Leave 0 for no limit", but the code treats 0 as "no items get discount". This means if a user leaves the field empty or sets it to 0, NO DISCOUNT IS APPLIED AT ALL.

Fix:

# Use infinity for "no limit" when value is 0 or not set
limit = flt(pr.min_or_max_discount_qty_limit)
remaining_qty = limit if limit > 0 else float('inf')

---

🟡 Medium Issues

4. Dead Code - Unreachable Branch

File: pos_next/overrides/pricing_rule.py, Lines 109-112

discount_qty = (
    item.qty if remaining_qty <= 0    # ← This branch is NEVER reached!
    else min(item.qty, remaining_qty)
)

Why unreachable? Line 100 already handles remaining_qty <= 0 with continue, so this ternary's first condition can never be true.

Fix: Simplify to:

discount_qty = min(flt(item.qty), remaining_qty)

---

5. Price List Check Uses Wrong Attribute

File: pos_next/overrides/pricing_rule.py, Line 74

if pr.for_price_list and pr.for_price_list != doc.selling_price_list:
    continue

Problem: Not all documents have selling_price_list. Delivery Notes and other documents may have different field names. This will fail silently.

Fix:

doc_price_list = (
    doc.get("selling_price_list") 
    or doc.get("buying_price_list") 
    or doc.get("price_list")
)
if pr.for_price_list and pr.for_price_list != doc_price_list:
    continue

---

6. Missing pricing_rule_for Field

When apply_price_discount_rule() returns early for Min/Max rules, it doesn't set item_details.pricing_rule_for, which the original function sets. This could cause issues in downstream code.

Fix: Set it before returning:

if apply_discount_on_price in ["Min", "Max"]:
    item_details.pricing_rule_for = pricing_rule.rate_or_discount
    return

---

7. Catch-Then-Throw Anti-Pattern

File: pos_next/overrides/pricing_rule.py, Lines 122-128

except Exception:
    frappe.log_error(frappe.get_traceback(), "Min/Max Pricing Rule Failed")
    frappe.throw(...)  # Always throws, so why catch?

Better approach: Let the exception propagate naturally, or only catch specific exceptions:

except frappe.ValidationError:
    raise  # Re-raise validation errors as-is
except Exception as e:
    frappe.log_error(frappe.get_traceback(), "Min/Max Pricing Rule Failed")
    frappe.throw(
        _("Failed to apply pricing rule discounts: {0}").format(str(e)),
        title=_("Pricing Rule Error")
    )

---

🟢 Minor Issues

8. Inconsistent Indentation

The file mixes tabs and 4-space indentation. Lines 19-45 use tabs, while lines 48-233 use 4 spaces. Please use consistent indentation (tabs preferred for this project).

9. Unnecessary frappe Import in hooks.py

File: pos_next/hooks.py, Line 2

import frappe  # Only used in try/except block

The frappe.log_error call happens at module load time when frappe may not be ready. This import should be removed if we fix the monkey-patching approach.

---

📊 Execution Flow Diagram

PHASE 1: Item Addition (get_item_details)
─────────────────────────────────────────
User adds item → get_pricing_rule_for_item() → apply_price_discount_rule()
                                                        │
                                    ┌───────────────────┴───────────────────┐
                                    │                                       │
                              Standard Rule                           Min/Max Rule
                              (apply discount now)                    (SKIP - return early)
                                    │                                       │
                                    ▼                                       ▼
                              item.discount_% = X                   item.pricing_rules = ["PRULE-001"]
                                                                    (discount NOT applied yet)

PHASE 2: Document Validation (doc.validate)
───────────────────────────────────────────
User saves → doc_events["validate"] hook → apply_min_max_price_discounts()
                                                        │
                                                        ▼
                                          _collect_min_max_rule_items()
                                          (find items with Min/Max rules)
                                                        │
                                                        ▼
                                          Sort items by price (asc for Min, desc for Max)
                                                        │
                                                        ▼
                                          Apply discount to first N items (by qty_limit)
                                                        │
                                                        ▼
                                          calculate_taxes_and_totals()

---

🧪 Test Cases Needed

1. Basic Min discount: 3 items, discount on cheapest
2. Basic Max discount: 3 items, discount on most expensive
3. qty_limit = 0: Should apply to ALL items (currently broken)
4. qty_limit = 2, item qty = 5: Partial quantity discount
5. Multiple pricing rules: Different rules on same document
6. Re-save document: Ensure discount isn't applied twice incorrectly
7. Different document types: Sales Invoice, Sales Order, Quotation, POS Invoice

---

Summary

Issue	Severity	Status
Missing _() import	🔴 Critical	Must fix
Monkey patch timing	🔴 Critical	Must fix
qty_limit=0 bug	🔴 Critical	Must fix
Dead code	🟡 Medium	Should fix
Price list check	🟡 Medium	Should fix
Missing pricing_rule_for	🟡 Medium	Should fix
Catch-then-throw	🟡 Medium	Should fix
Indentation	🟢 Minor	Nice to fix

Please address the critical issues before this PR can be merged. Happy to help with the fixes if needed!

[MostafaKadry]

@engahmed1190 @goondeal
please Review new changes which have handled all issues, and also Implemented client-side (POS Next Screen) checks for invalid offers and new eligible offers before server communication.