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.

[engahmed1190]

PR #95 Deep Review — All Issues with Fixes

I've done a thorough analysis of this PR against ERPNext's Pricing Rule / Promotional Scheme internals. The core architectural idea (deferring Min/Max evaluation to a post-processing step) is sound, but there are several issues that need addressing before merge.

---

Issue 1 (Critical): Bare except: in Monkey Patch — Silent Failure Causes Double Discounts

File: pos_next/hooks.py lines 275-280

Problem:

try:
    erpnext_pricing_rule.apply_price_discount_rule = pos_next_apply_price_discount_rule
except:
    import frappe
    frappe.log_error(frappe.get_traceback(), "Pricing Rule Override Error")

If this fails silently, the monkey patch is never applied. ERPNext's original apply_price_discount_rule runs normally and applies discounts to ALL matching items. Then the doc_events validate hook still fires apply_min_max_price_discounts, which applies discounts AGAIN — double discounting.

Also, bare except: catches SystemExit and KeyboardInterrupt.

Fix:

try:
    from erpnext.accounts.doctype.pricing_rule import pricing_rule as erpnext_pricing_rule
    from pos_next.overrides.pricing_rule import apply_price_discount_rule as pos_next_apply_price_discount_rule
    erpnext_pricing_rule.apply_price_discount_rule = pos_next_apply_price_discount_rule
except Exception:
    import frappe
    frappe.log_error(frappe.get_traceback(), "Pricing Rule Override Error")
    # Raise to prevent silent double-discount application.
    # If the patch fails, the validate hook will still fire and apply
    # Min/Max discounts ON TOP of the standard ones.
    raise

---

Issue 2 (Critical): calculate_taxes_and_totals() Overwrites Min/Max Discounts

File: pos_next/overrides/pricing_rule.py line 131

Problem:
After _apply_discount sets blended rate, discount_percentage, and discount_amount, calling calculate_taxes_and_totals() triggers ERPNext's calculate_item_values() (taxes_and_totals.py line 192):

if not item.rate or (item.pricing_rules and item.discount_percentage > 0):
    item.rate = price_list_rate * (1.0 - (discount_percentage / 100.0))
    item.discount_amount = price_list_rate * (discount_percentage / 100.0)

This recalculates rate from discount_percentage uniformly across all qty. For partial-qty discounts (e.g., 4 units, only 2 discounted), the blended percentage gets reapplied to the full price_list_rate, and discount_amount gets overwritten from total to per-unit (see Issue 3).

Fix:
Remove the calculate_taxes_and_totals() call. For doc_events (validate hook), Frappe calls it after all validate hooks automatically. For POS apply_offers (mock_doc), the mock has no such method anyway:

        # Do NOT call calculate_taxes_and_totals() here.
        # For doc_events (validate hook), Frappe calls it after all validate hooks.
        # For POS apply_offers (mock_doc), the mock has no such method.

---

Issue 3 (Critical): discount_amount Semantic Mismatch — Total vs Per-Unit

File: pos_next/overrides/pricing_rule.py, _apply_discount() lines 156-164

Problem:
The PR sets:

item.update({
    "discount_amount": total_line_value - net_amount,  # TOTAL across all qty
})

But ERPNext treats discount_amount as per-unit:

• taxes_and_totals.py line 198: item.discount_amount = price_list_rate * (discount_pct / 100) — per-unit
• taxes_and_totals.py line 201: item.rate = item.price_list_rate - item.discount_amount — subtracts per-unit from per-unit rate

If calculate_taxes_and_totals() runs and enters the elif item.discount_amount and item.pricing_rules branch (line 200):

item.rate = item.price_list_rate - item.discount_amount  # subtracts TOTAL from per-unit rate!

Example: 4 items at 100 each, total discount = 200:

• PR sets discount_amount = 200
• ERPNext calculates: rate = 100 - 200 = -100

Fix:
Store discount_amount as per-unit:

    per_unit_discount = (total_line_value - net_amount) / qty if qty else 0

    item.update({
        "amount": net_amount,
        "rate": net_amount / qty if qty else 0,
        "discount_amount": per_unit_discount,
        "discount_percentage": (per_unit_discount / base_rate * 100) if base_rate else 0,
    })

---

Issue 4 (Critical): _apply_discount Sets pricing_rule (Singular) — Field Doesn't Exist

File: pos_next/overrides/pricing_rule.py, _apply_discount() line 163

Problem:

item.update({
    "pricing_rule": pr.name,        # Field doesn't exist on Sales Invoice Item
    "pricing_rule_for": pr.rate_or_discount
})

The Sales Invoice Item doctype only has pricing_rules (plural, Small Text — stores JSON array). There is no pricing_rule (singular) field. This sets an orphan attribute that is not saved to DB and is not recognized by calculate_item_values() (which checks item.pricing_rules plural at line 192).

Fix:
Remove these lines — the item already has pricing_rules set from the standard engine (that's how _collect_min_max_rule_items found the item in the first place):

    item.update({
        "amount": net_amount,
        "rate": net_amount / qty if qty else 0,
        "discount_amount": per_unit_discount,
        "discount_percentage": (per_unit_discount / base_rate * 100) if base_rate else 0,
        # REMOVED: "pricing_rule" and "pricing_rule_for"
    })

---

Issue 5 (High): Resetting Non-Qualifying Items Wipes Other Pricing Rule Discounts

File: pos_next/overrides/pricing_rule.py, lines 116-120 and 125-129

Problem:

# Reset other items that might have had discounts
item.discount_percentage = 0.0
item.discount_amount = 0.0
item.rate = base_rate
item.amount = base_rate * qty

An item can have MULTIPLE pricing rules applied (e.g., a standard 10% discount rule + a Min/Max rule). If the item doesn't qualify for the Min/Max discount (it's not the cheapest), this code resets ALL discounts to zero — destroying the standard 10% discount that was validly applied by the regular pricing engine.

Fix:
Remove the reset blocks entirely. Since the monkey patch already prevents the standard engine from applying any discount for Min/Max rules (returns None), items that don't qualify simply keep whatever discount the standard engine applied for their other rules:

            for item in items:
                base_rate = flt(item.price_list_rate) or flt(item.rate)
                qty = flt(item.qty)
                if not base_rate or qty <= 0:
                    continue

                if not has_qty_limit:
                    if item == items[0]:
                        _apply_discount(pr, item, qty)
                    # else: do nothing — leave item's existing discounts intact
                    continue

                if remaining_qty <= 0:
                    # Do nothing — leave item's existing discounts from other rules
                    continue

                discount_qty = min(qty, remaining_qty)
                _apply_discount(pr, item, discount_qty)
                remaining_qty -= discount_qty

---

Issue 6 (High): qty_limit == 0 Means "Only 1 Item" — Contradicts "No Limit" Label

File: pos_next/overrides/pricing_rule.py lines 105-108, pos_next/fixtures/custom_field.json

Problem:
The custom field description says: "Quantity limit for applying discount on Min/Max priced item. Leave 0 for no limit."

But the code does:

has_qty_limit = qty_limit > 0
# ...
if not has_qty_limit:
    if item == items[0]:  # Only first item gets discount

So qty_limit = 0 ("no limit") actually means "discount only the single cheapest/most expensive item." This is the opposite of "no limit."

Fix (option A — make code match label):

            if not has_qty_limit:
                # No limit: apply discount to ALL items matching this rule
                for item in items:
                    base_rate = flt(item.price_list_rate) or flt(item.rate)
                    qty = flt(item.qty)
                    if base_rate and qty > 0:
                        _apply_discount(pr, item, qty)
                continue

Fix (option B — make label match code):
Change the custom field description to:
"Number of units to apply discount to. 0 = discount only the single cheapest/most expensive item."

---

Issue 7 (High): Purchase Document Hooks Are Unnecessary Scope Expansion

File: pos_next/hooks.py lines 233-252

Problem:
POS Next is a sales POS app. Adding validate hooks to Purchase Order, Supplier Quotation, Purchase Receipt, Purchase Invoice, and Opportunity:

• Increases blast radius (bugs in pricing_rule.py affect procurement workflows)
• Runs unnecessary code on every purchase document save
• Opportunity doctype doesn't have selling_price_list or buying_price_list — the for_price_list check will always evaluate to None

Fix:
Remove purchase document hooks. Keep only sales-related doctypes:

    "Sales Order": {
        "validate": "pos_next.overrides.pricing_rule.apply_min_max_price_discounts"
    },
    "Quotation": {
        "validate": "pos_next.overrides.pricing_rule.apply_min_max_price_discounts"
    },
    "Delivery Note": {
        "validate": "pos_next.overrides.pricing_rule.apply_min_max_price_discounts"
    },
    "POS Invoice": {
        "validate": "pos_next.overrides.pricing_rule.apply_min_max_price_discounts"
    },
    # REMOVED: Purchase Order, Supplier Quotation, Purchase Receipt,
    #          Purchase Invoice, Opportunity

---

Issue 8 (Medium): Override Skips Margin Handling from Original Function

File: pos_next/overrides/pricing_rule.py lines 47-50

Problem:
The original apply_price_discount_rule handles margin logic (lines 560-569 in ERPNext). The override skips ALL of it for Min/Max rules. If a Min/Max rule also has a margin configured, the margin is silently ignored.

Also, pricing_rule_for is set conditionally via hasattr instead of unconditionally like the original.

Fix:

if apply_discount_on_price in ["Min", "Max"]:
    # Still set pricing_rule_for (the original always does this)
    item_details.pricing_rule_for = pricing_rule.get("rate_or_discount")

    # Still handle margin if configured (same as original function)
    if (pricing_rule.get("margin_type") in ["Amount", "Percentage"]
        and pricing_rule.get("currency") == args.get("currency")):
        item_details.margin_type = pricing_rule.get("margin_type")
        item_details.has_margin = True
        if pricing_rule.get("apply_multiple_pricing_rules"):
            item_details.margin_rate_or_amount = flt(
                item_details.get("margin_rate_or_amount") or 0
            ) + flt(pricing_rule.get("margin_rate_or_amount") or 0)
        else:
            item_details.margin_rate_or_amount = pricing_rule.get("margin_rate_or_amount")

    return None

---

Issue 9 (Medium): selected_offers is not None vs selected_offer_names Variable Mismatch

File: pos_next/api/invoices.py line 2275

Problem:
The condition uses selected_offers (raw parameter) but the filter uses selected_offer_names (parsed set):

if selected_offers is not None:    # ← raw parameter
    rule_map = {
        name: details
        for name, details in rule_map.items()
        if name in selected_offer_names  # ← parsed set
    }

This works currently but the variable name mismatch is fragile.

Fix:
Use consistent variable:

selected_offer_names = None
if selected_offers is not None:
    if isinstance(selected_offers, str):
        selected_offers = json.loads(selected_offers) if selected_offers else []
    selected_offer_names = set(selected_offers)

# ... later:
if selected_offer_names is not None:
    rule_map = {
        name: details
        for name, details in rule_map.items()
        if name in selected_offer_names
    }

---

Issue 10 (Medium): Min/Max Applied Rules Not Added to applied_rules Response

File: pos_next/api/invoices.py lines 2405-2430

Problem:
The standard pricing rule loop adds rule names to applied_rules. But apply_min_max_price_discounts(mock_doc) is called as a black box — rules it applies are never added to applied_rules. The response's applied_pricing_rules will be missing Min/Max rules, so the frontend won't track them as applied.

Fix:

        apply_min_max_price_discounts(mock_doc, allowed_rules=allowed_rules)

        # Add Min/Max rules to applied_rules so they appear in the response
        for item in prepared_items:
            if item.get("pricing_rules"):
                for pr_name in erpnext_get_applied_pricing_rules(item.get("pricing_rules")):
                    if pr_name in rule_map:
                        applied_rules.add(pr_name)

---

Issue 11 (Medium): Rate Discount Type Can Produce Negative total_discount

File: pos_next/overrides/pricing_rule.py, _apply_discount() line 148

Problem:

if pr.rate_or_discount == "Rate":
    total_discount = (base_rate - flt(pr.rate)) * eligible_qty

If pr.rate > base_rate (pricing rule sets a HIGHER rate), total_discount is negative, resulting in negative discount_amount and discount_percentage.

Fix:

    total_discount = 0.0
    if pr.rate_or_discount == "Rate":
        total_discount = max((base_rate - flt(pr.rate)) * eligible_qty, 0.0)
    elif pr.rate_or_discount == "Discount Percentage":
        total_discount = max((base_rate * flt(pr.discount_percentage) / 100) * eligible_qty, 0.0)
    elif pr.rate_or_discount == "Discount Amount":
        total_discount = max(flt(pr.discount_amount) * eligible_qty, 0.0)

    # Cap discount to not exceed line value
    total_discount = min(total_discount, total_line_value)

---

Issue 12 (Low): Frontend Consolidated Flow Loses Error Isolation

File: POS/src/stores/posCart.js lines 1496-1597

Problem:
The old flow had separate reapplyOffer() then autoApplyEligibleOffers(). The new flow wraps everything in one try/catch — any error in eligibility checking stops all processing.

Fix:
Wrap client-side eligibility checks individually:

const invalidOffers = []
for (const entry of appliedOffers.value) {
    if (entry.offer) {
        try {
            const { eligible } = offersStore.checkOfferEligibility(entry.offer)
            if (!eligible) invalidOffers.push(entry)
        } catch (e) {
            console.warn("Error checking offer eligibility:", entry.code, e)
            invalidOffers.push(entry) // Treat errored offers as invalid
        }
    }
}

---

Issue 13 (Low): Whitespace Inconsistency — Mixed Tabs and Spaces

File: pos_next/overrides/pricing_rule.py

Problem:
apply_price_discount_rule uses tabs, but all other functions use 4-space indentation. The project convention is tabs for Python files.

Fix:
Standardize on tabs throughout the file.

---

Issue 14 (Low): Error Handling Swallows Exceptions — Invoice Saves with Wrong Prices

File: pos_next/overrides/pricing_rule.py lines 133-136

Problem:

except Exception as e:
    frappe.log_error(...)
    frappe.msgprint(_("Warning: ..."), indicator="orange")

The user sees a toast but the invoice saves with wrong prices. For a pricing function, silent failure = revenue loss.

Fix:

    except Exception as e:
        frappe.log_error(frappe.get_traceback(), "Min/Max Pricing Rule Failed")
        frappe.throw(
            _("Failed to apply Min/Max pricing rule discounts. Please check your pricing rules."),
            title=_("Pricing Rule Error")
        )

---

Summary

#	Severity	File	Issue
1	Critical	hooks.py	Bare except: — silent failure = double discounts
2	Critical	pricing_rule.py	calculate_taxes_and_totals() overwrites blended rates
3	Critical	pricing_rule.py	discount_amount stored as total, ERPNext expects per-unit
4	Critical	pricing_rule.py	Sets pricing_rule (singular) — field doesn't exist
5	High	pricing_rule.py	Reset wipes discounts from other pricing rules
6	High	pricing_rule.py + fixture	qty_limit=0 = "only 1 item" but label says "no limit"
7	High	hooks.py	Purchase doc hooks unnecessary for POS app
8	Medium	pricing_rule.py	Override skips margin handling from original function
9	Medium	invoices.py	Variable name mismatch in selected_offers condition
10	Medium	invoices.py	Min/Max rules missing from applied_rules response
11	Medium	pricing_rule.py	Rate type can produce negative discount
12	Low	posCart.js	Consolidated flow loses per-step error isolation
13	Low	pricing_rule.py	Mixed tabs and spaces
14	Low	pricing_rule.py	Exception swallowed — invoice saves with wrong prices

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity for 21 days.

To keep this PR open, please:

• Rebase on the latest develop branch
• Address any pending review comments
• Reply with an update on the PR status

If no further activity occurs within the next 14 days, this PR will be automatically closed.