Pn 31 apply discount in pos next invoices only

[MostafaKadry]

No description provided.

[engahmed1190]

Deep Code Review — PR #176

Files changed: 8 | +188 / -15 | 6 commits

---

Overall Assessment

This PR adds POS Profile-specific filtering for Promotional Schemes — letting admins restrict which promotions apply to which POS Profile. It uses a child table on the Promotional Scheme doctype and filters at both the API level (get_promotions) and the SQL level (monkey-patched get_other_conditions). The approach is architecturally sound and well-scoped.

---

Issues

1. Correlated subqueries in _add_pos_profile_condition — performance concern

AND (
    `tabPricing Rule`.promotional_scheme IS NULL
    OR `tabPricing Rule`.promotional_scheme = ''
    OR NOT EXISTS (
        SELECT 1 FROM `tabPromotion Scheme POS Profile`
        WHERE parent = `tabPricing Rule`.promotional_scheme
    )
    OR EXISTS (
        SELECT 1 FROM `tabPromotion Scheme POS Profile`
        WHERE parent = `tabPricing Rule`.promotional_scheme
        AND pos_profile = %(pos_profile)s
    )
)

This adds two correlated subqueries per pricing rule evaluation. ERPNext calls get_other_conditions once per item in the cart (inside get_pricing_rule_for_item), and each call hits the database with these subqueries. For a cart with 20 items and 50 pricing rules, that's 20 queries each with 2 subqueries scanning tabPromotion Scheme POS Profile.

The tabPromotion Scheme POS Profile table won't have an index on (parent, pos_profile) by default — child tables in Frappe only get an index on parent. The pos_profile column in the EXISTS subquery will require a full scan of matching parent rows. For small datasets this is fine, but worth noting.

Suggestion: Consider adding a composite index, or pre-filtering promotion scheme names and injecting an IN (...) clause instead of correlated subqueries.

2. get_promotions filter doesn't apply to standalone Pricing Rules

The new POS Profile filtering in get_promotions (lines 63-82) only filters schemes (Promotional Schemes). The standalone pricing_rules fetched below (line 106) are returned unfiltered. If a standalone pricing rule should also be restricted to specific POS Profiles, there's no mechanism for that. This may be intentional (standalone rules apply everywhere) but should be documented.

3. Custom field uses pos_profiles fieldname but fixture name is custom_pos_profiles

The custom field JSON defines:

"fieldname": "pos_profiles",
"name": "Promotional Scheme-custom_pos_profiles"

In Frappe, custom fields are accessed with the custom_ prefix (e.g., doc.custom_pos_profiles). But the child table Promotion Scheme POS Profile uses parent to reference the Promotional Scheme — so the query frappe.get_all("Promotion Scheme POS Profile", filters={"parent": scheme_name}) works regardless of the fieldname. However, if anyone tries to access scheme_doc.pos_profiles directly, it won't work — it needs to be scheme_doc.custom_pos_profiles. No code in this PR does that, so it's fine, but it's a trap for future developers.

4. apply_pricing_rule_on_transaction also gets filtered — unintended?

The monkey-patch applies to ALL calls to get_other_conditions, which includes both:

• Per-item pricing rules (line 135 in utils.py) — args is the item dict merged with doc-level fields
• Transaction-level pricing rules (line 559 in utils.py) — doc is the full document

For the per-item path, pos_profile propagates from pricing_args → args_copy correctly. For the transaction-level path, doc is the same pricing_args dict, so it also has pos_profile. This works correctly.

However, if ERPNext internally calls apply_pricing_rule_on_transaction from a Sales Invoice that doesn't set pos_profile in its args (e.g., standard ERPNext flow, not POS Next), pos_profile will be absent, and _add_pos_profile_condition returns early without adding the filter — which is correct. No issue here, but the two-path behavior should be documented in the function docstring.

5. get_promotions batch fetch doesn't handle empty scheme_names

scheme_names = [s["name"] for s in schemes]
rows = frappe.get_all(
    "Promotion Scheme POS Profile",
    filters={"parent": ["in", scheme_names]},
    ...
)

If schemes is empty, scheme_names is [], and frappe.get_all with ["in", []] will generate invalid SQL (parent IN ()) on some database backends. MariaDB handles it gracefully (returns empty), but it's a code smell. Add an early return.

6. Removed comment in __init__.py was useful

The PR removes this comment:

# No Frappe hook exists for non-whitelisted module-level functions (override_whitelisted_methods
# only works for @frappe.whitelist() HTTP endpoints, override_doctype_class only for DocType
# classes). This is the standard Python module init approach — runs once at import.

This comment explained why monkey-patching is used instead of a Frappe hook — which is valuable context for anyone reading the code. The same explanation exists in pricing_rule.py's docstring, so it's not lost, but removing it from the call site (where the "why" matters most) is a minor loss.

---

Code Quality

7. fieldname in custom field JSON is pos_profiles (no custom_ prefix)

Frappe auto-generates the actual fieldname as custom_pos_profiles based on the name field (Promotional Scheme-custom_pos_profiles). The fieldname property in the JSON says pos_profiles, but Frappe overrides this with the name-based convention. This is confusing — the JSON says one thing but the actual field is different. Should set "fieldname": "custom_pos_profiles" explicitly for clarity.

8. The custom/promotional_scheme.json approach vs fixtures

This PR uses Frappe's {module}/custom/{doctype}.json convention (with sync_on_migrate: 1) instead of the fixtures array in hooks.py. The rest of the codebase uses fixtures for custom fields (e.g., POS Profile-posa_cash_mode_of_payment, Promotional Scheme-pos_only). Mixed approaches make it harder to track where custom fields are defined. Not a blocker, but worth standardizing.

9. Missing newline at end of JSON files

Both promotion_scheme_pos_profile.json and promotional_scheme.json are missing a trailing newline. Minor but will show up in diffs and linters.

---

Risk Assessment

Risk	Level
Correlated subqueries on every item in cart	Medium — acceptable for small catalogs, may slow large ones
Empty scheme_names → invalid SQL	Low — MariaDB handles gracefully
Mixed fixture approaches	Low — maintenance concern only
Standalone pricing rules unfiltered	Low — likely intentional

Verdict

Looks good overall — minor issues only. The core mechanism (monkey-patched SQL condition + API-level filtering) is correct and the pos_profile propagation through ERPNext's pricing rule engine works for both per-item and transaction-level rules. The main concern is the performance of correlated subqueries at scale — consider a pre-filter approach if POS installations have large pricing rule sets. The other issues are code quality / consistency items. Safe to merge after addressing #5 (empty list guard) and optionally #1 (performance).