From 52a0639f8471b6793aafc082da17e94fcf6fea70 Mon Sep 17 00:00:00 2001 From: denisireri Date: Fri, 9 Oct 2026 10:04:50 +0300 Subject: [PATCH] Close gate bypasses found in the pre-implementation review - Work Order: copy the Packaging Specification from the Sales Order Item (walking back through the Production Plan for sub-assembly orders), and re-derive release blockers on submit instead of trusting a stored status. Tooling readiness is now derived and enforced (B6/M1/M2). - Whitelisted operations no longer save with ignore_permissions: callers need the right role and write permission on the document (B6). - Dispatch Gate matches final/outgoing inspections by exact batch and needs a batch on every controlled line; Reserved mode needs stock reservation (M3). - Returns check read a field Delivery Note Item doesn't have. - Drop the credit-note rule that blocked legitimate partial credits; ERPNext already caps cumulative returned qty (m5). - Billing recompute and Supply Close query only this order's documents (m6). Co-Authored-By: Claude Opus 5.5 --- makap_backend/makap_backend/controls.py | 139 +++++++++--------- .../order_closure_and_margin_view.py | 40 +++-- makap_backend/makap_backend/operations.py | 98 ++++++------ .../makap_backend/work_order_readiness.py | 107 ++++++++++++++ 4 files changed, 261 insertions(+), 123 deletions(-) create mode 100644 makap_backend/makap_backend/work_order_readiness.py diff --git a/makap_backend/makap_backend/controls.py b/makap_backend/makap_backend/controls.py index 273959c..f550d23 100644 --- a/makap_backend/makap_backend/controls.py +++ b/makap_backend/makap_backend/controls.py @@ -1,5 +1,7 @@ import frappe +from makap_backend.makap_backend import work_order_readiness + def _is_manufactured_item(item_code): if not item_code or not frappe.db.exists("Item", item_code): @@ -93,24 +95,17 @@ def validate_sales_order(doc, method=None): def validate_work_order(doc, method=None): + work_order_readiness.apply_specification(doc) + blockers = work_order_readiness.release_blockers(doc) + if doc.docstatus != 1: return - if doc.makap_manufacturing_readiness_status not in {"Ready", "Released"}: - frappe.throw("Submitted Work Order requires Manufacturing Readiness Status Ready or Released.") - - if doc.makap_packaging_specification: - spec_status = frappe.db.get_value( - "Packaging Specification", doc.makap_packaging_specification, "specification_status" - ) - if spec_status != "Approved for Manufacture": - frappe.throw("Work Order requires a Packaging Specification approved for manufacture.") - - if doc.makap_bom_compatibility_status not in {"Compatible", "Not Applicable"}: - frappe.throw("Work Order requires a compatible BOM decision before submission.") - - if doc.makap_process_compatibility_status not in {"Compatible", "Not Applicable"}: - frappe.throw("Work Order requires a compatible process decision before submission.") + # Re-derived here rather than trusting the stored readiness status, so a + # stale "Ready" set before the spec or tooling changed can't release a job. + if blockers: + frappe.throw("Work Order cannot be released:\n" + "\n".join(blockers)) + doc.makap_manufacturing_readiness_status = "Released" def validate_quality_inspection(doc, method=None): @@ -134,15 +129,35 @@ def _is_controlled_item(item_code): return control_class in {"Controlled", "High Traceability"} -def _latest_quality_disposition(item_code, batch_no=None): - """Best-effort read of the most recent relevant Quality Inspection for this - item/batch. Real batch-population matching (M8 section 8/9) is intentionally - left as a follow-up once Makap's actual QI-to-batch linkage in this bench is - confirmed — this covers the common case where batch_no is captured on the QI. +DISPATCH_GATE_STAGES = ["Final", "Outgoing"] + + +def _line_batches(line): + """Batches on a stock line, whether held directly or in a Serial and Batch Bundle.""" + if line.get("batch_no"): + return [line.batch_no] + if line.get("serial_and_batch_bundle"): + return frappe.get_all( + "Serial and Batch Entry", + filters={"parent": line.serial_and_batch_bundle, "batch_no": ["is", "set"]}, + pluck="batch_no", + distinct=True, + ) + return [] + + +def _latest_quality_disposition(item_code, batch_no): + """Most recent submitted final/outgoing Quality Inspection for this exact batch. + + Matching on the batch is mandatory: an item-only match would let a pass on + one roll release a different roll that was never inspected. """ - filters = {"item_code": item_code, "docstatus": 1} - if batch_no: - filters["batch_no"] = batch_no + filters = { + "item_code": item_code, + "batch_no": batch_no, + "docstatus": 1, + "makap_quality_gate_stage": ["in", DISPATCH_GATE_STAGES], + } rows = frappe.get_all( "Quality Inspection", filters=filters, @@ -164,6 +179,8 @@ def _compute_dispatch_gate(doc): if not doc.customer: blockers.append("No Customer on Delivery Note.") + reservation_enabled = frappe.db.get_single_value("Stock Settings", "enable_stock_reservation") + for line in doc.get("items") or []: if not _is_controlled_item(line.item_code): continue @@ -173,19 +190,27 @@ def _compute_dispatch_gate(doc): if not line.makap_dispatch_allocation_mode: blockers.append(prefix + "Dispatch Allocation Mode (Reserved/Pooled) not set.") - if line.makap_dispatch_allocation_mode == "Reserved" and not line.makap_stock_reservation_entry: - blockers.append(prefix + "Reserved mode requires a Stock Reservation Entry reference.") + if line.makap_dispatch_allocation_mode == "Reserved": + if not reservation_enabled: + blockers.append(prefix + "Reserved mode needs Stock Reservation enabled in Stock Settings; use Pooled.") + elif not line.makap_stock_reservation_entry: + blockers.append(prefix + "Reserved mode requires a Stock Reservation Entry reference.") if not line.qty or line.qty <= 0: blockers.append(prefix + "Delivery quantity must be greater than zero.") - disposition = _latest_quality_disposition(line.item_code, getattr(line, "batch_no", None)) - if not disposition or disposition.makap_quality_eligibility != "Eligible": - blockers.append(prefix + "No current Quality Eligibility = Eligible found for this item/batch.") - elif disposition.makap_quality_disposition not in {"Release", None, ""}: - blockers.append( - prefix + f"Latest Quality Inspection disposition is '{disposition.makap_quality_disposition}', not Release." - ) + batches = _line_batches(line) + if not batches: + blockers.append(prefix + "Controlled stock must be dispatched by batch/roll; no batch selected.") + for batch in batches: + disposition = _latest_quality_disposition(line.item_code, batch) + if not disposition or disposition.makap_quality_eligibility != "Eligible": + blockers.append(prefix + f"Batch {batch} has no final Quality Inspection marked Eligible.") + elif disposition.makap_quality_disposition not in {"Release", None, ""}: + blockers.append( + prefix + + f"Batch {batch}: latest final inspection is '{disposition.makap_quality_disposition}', not Release." + ) if doc.makap_dispatch_material_accountability_status != "Cleared": blockers.append("Dispatch Material Accountability is not Cleared.") @@ -219,7 +244,8 @@ def validate_delivery_note(doc, method=None): # directly back into a dispatchable FG Released location, and require a # fresh Quality disposition before the stock is reusable (M8-FR-072/073/074). if doc.is_return: - if not _returns_hold_warehouse(line.warehouse) and not _returns_hold_warehouse(line.t_warehouse): + # On a return Delivery Note, line.warehouse is where goods come back in. + if not _returns_hold_warehouse(line.warehouse): frappe.throw( f"Return item {line.item_code} must be received into a warehouse flagged " "as Returns Hold, not directly into a dispatchable location." @@ -291,12 +317,6 @@ def _recompute_billing_and_collection(sales_order_name): delivered_unbilled = bool(so.per_delivered and so.per_delivered > 0 and (so.per_billed or 0) < so.per_delivered) - invoices = frappe.get_all( - "Sales Invoice", - filters={"docstatus": 1}, - fields=["name", "outstanding_amount", "grand_total", "is_return"], - or_filters=None, - ) # Sales Invoice doesn't carry sales_order directly at header level in all cases; # use the child table linkage to scope invoices to this Sales Order. linked_invoice_names = frappe.get_all( @@ -305,7 +325,15 @@ def _recompute_billing_and_collection(sales_order_name): pluck="parent", distinct=True, ) - linked_invoices = [i for i in invoices if i.name in set(linked_invoice_names)] + linked_invoices = ( + frappe.get_all( + "Sales Invoice", + filters={"name": ["in", linked_invoice_names], "docstatus": 1}, + fields=["name", "outstanding_amount", "grand_total", "is_return"], + ) + if linked_invoice_names + else [] + ) if not linked_invoices: collection_status = "Not Yet Due" @@ -335,34 +363,9 @@ def _sales_orders_on_invoice(doc): def validate_sales_invoice(doc, method=None): - if doc.is_return and doc.return_against: - # M8-BR-14: one physical return event = one stock consequence. Block a second - # Credit Note crediting the same original invoice for a controlled item. - for line in doc.get("items") or []: - if not _is_controlled_item(line.item_code): - continue - existing = frappe.get_all( - "Sales Invoice", - filters={ - "is_return": 1, - "return_against": doc.return_against, - "docstatus": 1, - "name": ["!=", doc.name], - }, - pluck="name", - ) - if existing: - credited_items = frappe.get_all( - "Sales Invoice Item", - filters={"parent": ["in", existing], "item_code": line.item_code}, - pluck="name", - ) - if credited_items: - frappe.throw( - f"Item {line.item_code} against {doc.return_against} already has a Credit " - f"Note ({existing[0]}) — this looks like a duplicate return credit." - ) - + # M8-BR-14 (no double credit for one return) is enforced by ERPNext's own + # return validation, which caps cumulative credited qty at the invoiced qty. + # A second, partial Credit Note for the same item is legitimate and allowed. for line in doc.get("items") or []: if not _is_controlled_item(line.item_code): continue diff --git a/makap_backend/makap_backend/doctype/order_closure_and_margin_view/order_closure_and_margin_view.py b/makap_backend/makap_backend/doctype/order_closure_and_margin_view/order_closure_and_margin_view.py index 563e079..23f3476 100644 --- a/makap_backend/makap_backend/doctype/order_closure_and_margin_view/order_closure_and_margin_view.py +++ b/makap_backend/makap_backend/doctype/order_closure_and_margin_view/order_closure_and_margin_view.py @@ -26,6 +26,14 @@ class OrderClosureAndMarginView(Document): @frappe.whitelist() def recompute(self): + self.check_permission("write") + self._recompute() + return { + "operating_close": self.operating_close, + "primary_blocker": self.primary_blocker, + } + + def _recompute(self): """Best-effort recomputation from linked M4-M7 records already in the bench. Cost/Margin fields are intentionally left as manual entry — deriving those correctly needs a real pass over Stock Ledger/GL/Landed Cost Voucher data, @@ -102,24 +110,32 @@ class OrderClosureAndMarginView(Document): ) self.quality_close = "Open" if open_quality else "Closed" - # Supply Close — first-pass only. Real M7 lineage (Material Request -> - # Production Plan -> Sales Order) isn't traversed here yet; this checks - # Purchase Order Items sharing this order's items as a rough proxy and - # should be replaced once the actual M7 linkage is confirmed against data. - item_codes = frappe.get_all( - "Sales Order Item", filters={"parent": so_name}, pluck="item_code", distinct=True + # Supply Close — purchases raised for this order through its Production + # Plan(s): Sales Order -> Production Plan -> Material Request -> PO. + plans = frappe.get_all( + "Production Plan Sales Order", filters={"sales_order": so_name}, pluck="parent", distinct=True + ) + requests = ( + frappe.get_all( + "Material Request Item", + filters={"production_plan": ["in", plans], "docstatus": 1}, + pluck="parent", + distinct=True, + ) + if plans + else [] ) at_risk_po = ( frappe.get_all( "Purchase Order Item", filters={ - "item_code": ["in", item_codes], + "material_request": ["in", requests], "makap_supplier_confirmation_status": ["in", ["At Risk", "Declined"]], "docstatus": 1, }, limit=1, ) - if item_codes + if requests else [] ) self.supply_close = "Open" if at_risk_po else "Closed" @@ -127,11 +143,7 @@ class OrderClosureAndMarginView(Document): self._derive_operating_close() self.last_recomputed_by = frappe.session.user self.last_recomputed_on = frappe.utils.now_datetime() - self.save(ignore_permissions=True) - return { - "operating_close": self.operating_close, - "primary_blocker": self.primary_blocker, - } + self.save() @frappe.whitelist() @@ -139,7 +151,7 @@ def get_or_create(sales_order): if not frappe.db.exists("Order Closure And Margin View", sales_order): doc = frappe.get_doc( {"doctype": "Order Closure And Margin View", "sales_order": sales_order} - ).insert(ignore_permissions=True) + ).insert() else: doc = frappe.get_doc("Order Closure And Margin View", sales_order) doc.recompute() diff --git a/makap_backend/makap_backend/operations.py b/makap_backend/makap_backend/operations.py index 8363011..41f8acf 100644 --- a/makap_backend/makap_backend/operations.py +++ b/makap_backend/makap_backend/operations.py @@ -1,4 +1,12 @@ import frappe +from frappe.utils import cint + +from makap_backend.makap_backend import work_order_readiness + +READINESS_ROLES = ("Production Authority", "Manufacturing Manager") +MATERIAL_CHECK_ROLES = ("Production Authority", "Manufacturing Manager", "Materials Controller", "Stock Manager") +DISPOSITION_ROLES = ("Quality Manager",) +CONCESSION_ROLES = ("Concession Authority",) def _require_doc(doctype, name): @@ -7,53 +15,52 @@ def _require_doc(doctype, name): return frappe.get_doc(doctype, name) +def _record(doc, values): + """Store derived evidence under the caller's own permissions. + + Submitted documents can't be re-saved, so evidence on them is written + directly; the caller has already passed the role and write checks. + """ + doc.check_permission("write") + if doc.docstatus == 1: + doc.db_set(values) + else: + doc.update(values) + doc.save() + + @frappe.whitelist() def evaluate_work_order_readiness(work_order_name, save=True): - """Derive bounded readiness fields from standard Work Order/BOM/specification data.""" + """Derive readiness from the same rules the Work Order submit gate enforces.""" + frappe.only_for(READINESS_ROLES) work_order = _require_doc("Work Order", work_order_name) - blockers = [] + blockers = work_order_readiness.release_blockers(work_order) + status = "Blocked" if blockers else "Ready" - spec_status = None - if work_order.makap_packaging_specification: - spec_status = frappe.db.get_value( - "Packaging Specification", work_order.makap_packaging_specification, "specification_status" + if cint(save): + _record( + work_order, + { + "makap_manufacturing_readiness_status": status, + "makap_manufacturing_readiness_blockers": "\n".join(blockers), + "makap_tooling_readiness_status": work_order.makap_tooling_readiness_status, + "makap_readiness_checked_by": frappe.session.user, + "makap_readiness_checked_on": frappe.utils.now_datetime(), + }, ) - if spec_status != "Approved for Manufacture": - blockers.append("Packaging Specification is not Approved for Manufacture") - elif work_order.production_item: - requires_spec = frappe.db.get_value( - "Item", work_order.production_item, "makap_requires_packaging_specification" - ) - if requires_spec: - blockers.append("Production Item requires a Packaging Specification") - - if not work_order.bom_no: - blockers.append("No BOM is selected") - elif work_order.makap_bom_compatibility_status not in {"Compatible", "Not Applicable"}: - blockers.append("BOM compatibility is not confirmed") - - if work_order.makap_process_compatibility_status not in {"Compatible", "Not Applicable"}: - blockers.append("Process compatibility is not confirmed") - - work_order.makap_manufacturing_readiness_status = "Blocked" if blockers else "Ready" - work_order.makap_manufacturing_readiness_blockers = "\n".join(blockers) - work_order.makap_readiness_checked_by = frappe.session.user - work_order.makap_readiness_checked_on = frappe.utils.now_datetime() - if save: - work_order.save(ignore_permissions=True) - frappe.db.commit() return { "work_order": work_order.name, - "status": work_order.makap_manufacturing_readiness_status, + "status": status, "blockers": blockers, - "specification_status": spec_status, + "tooling_status": work_order.makap_tooling_readiness_status, } @frappe.whitelist() def evaluate_material_availability(work_order_name, save=True): """Compare standard Work Order required quantities with standard Bin actual quantity.""" + frappe.only_for(MATERIAL_CHECK_ROLES) work_order = _require_doc("Work Order", work_order_name) shortages = [] partially_available = False @@ -82,13 +89,16 @@ def evaluate_material_availability(work_order_name, save=True): else: status = "Available" - work_order.makap_material_availability_status = status - work_order.makap_material_availability_checked_by = frappe.session.user - work_order.makap_material_availability_checked_on = frappe.utils.now_datetime() - work_order.makap_material_availability_blocker_summary = "\n".join(shortages) - if save: - work_order.save(ignore_permissions=True) - frappe.db.commit() + if cint(save): + _record( + work_order, + { + "makap_material_availability_status": status, + "makap_material_availability_checked_by": frappe.session.user, + "makap_material_availability_checked_on": frappe.utils.now_datetime(), + "makap_material_availability_blocker_summary": "\n".join(shortages), + }, + ) return {"work_order": work_order.name, "status": status, "shortages": shortages} @@ -96,7 +106,10 @@ def evaluate_material_availability(work_order_name, save=True): @frappe.whitelist() def apply_quality_disposition(quality_inspection_name, disposition, quantity=None, reason=None): """Record bounded quality disposition evidence on a standard Quality Inspection.""" + frappe.only_for(CONCESSION_ROLES + DISPOSITION_ROLES if disposition == "Concession" else DISPOSITION_ROLES) inspection = _require_doc("Quality Inspection", quality_inspection_name) + if inspection.docstatus != 0: + frappe.throw("Disposition can only be recorded on a draft Quality Inspection; amend it to change a submitted one.") allowed = {"Pending", "Release", "Hold", "Reject", "Rework", "Scrap", "Concession"} if disposition not in allowed: frappe.throw("Invalid quality disposition.") @@ -114,14 +127,17 @@ def apply_quality_disposition(quality_inspection_name, disposition, quantity=Non inspection.makap_quality_eligibility = "Eligible" elif disposition in {"Hold", "Reject", "Rework", "Scrap"}: inspection.makap_quality_eligibility = "Hold" if disposition == "Hold" else "Ineligible" - inspection.save(ignore_permissions=True) - frappe.db.commit() + inspection.check_permission("write") + inspection.save() return {"quality_inspection": inspection.name, "disposition": disposition} @frappe.whitelist() def get_supply_assurance_snapshot(item_code, warehouse=None): """Return a derived supply view from standard stock, material requests, and purchase orders.""" + for doctype in ("Bin", "Material Request", "Purchase Order"): + if not frappe.has_permission(doctype, "read"): + frappe.throw(f"You do not have permission to read {doctype}.", frappe.PermissionError) bins = frappe.get_all( "Bin", filters={"item_code": item_code, **({"warehouse": warehouse} if warehouse else {})}, diff --git a/makap_backend/makap_backend/work_order_readiness.py b/makap_backend/makap_backend/work_order_readiness.py new file mode 100644 index 0000000..f9f2688 --- /dev/null +++ b/makap_backend/makap_backend/work_order_readiness.py @@ -0,0 +1,107 @@ +"""Work Order release rules, shared by the readiness check and the submit gate. + +Both callers run the same rules so a readiness result can never disagree with +what submission actually enforces. +""" + +import frappe + +RELEASABLE_SPEC_STATUS = "Approved for Manufacture" +READY_CYLINDER_STATUS = "Ready" +COMPATIBLE = {"Compatible", "Not Applicable"} + + +def _sales_order_item_for(work_order): + """Find the Sales Order Item a Work Order serves. + + Sub-assembly Work Orders (printed roll, laminated roll) carry no + sales_order_item of their own, so walk back through the Production Plan + to the finished-goods row that does. + """ + if work_order.get("sales_order_item"): + return work_order.sales_order_item + + plan_item = work_order.get("production_plan_item") + if not plan_item and work_order.get("production_plan_sub_assembly_item"): + plan_item = frappe.db.get_value( + "Production Plan Sub Assembly Item", + work_order.production_plan_sub_assembly_item, + "production_plan_item", + ) + if plan_item: + so_item = frappe.db.get_value("Production Plan Item", plan_item, "sales_order_item") + if so_item: + return so_item + + if work_order.get("sales_order") and work_order.get("production_item"): + matches = frappe.get_all( + "Sales Order Item", + filters={"parent": work_order.sales_order, "item_code": work_order.production_item}, + pluck="name", + ) + if len(matches) == 1: + return matches[0] + return None + + +def apply_specification(work_order): + """Copy the Packaging Specification from the Sales Order Item onto the Work Order.""" + if work_order.get("makap_packaging_specification"): + return + so_item = _sales_order_item_for(work_order) + if not so_item: + return + spec = frappe.db.get_value("Sales Order Item", so_item, "makap_packaging_specification") + if not spec: + return + work_order.makap_packaging_specification = spec + work_order.makap_specification_revision = frappe.db.get_value("Packaging Specification", spec, "revision") + work_order.makap_sales_order_item_reference = so_item + work_order.makap_customer = frappe.db.get_value("Packaging Specification", spec, "customer") + + +def tooling_status(spec_name): + """Return (status, blockers) for the print cylinders/plates a specification needs.""" + if not spec_name: + return "Not Required", [] + if not frappe.db.get_value("Packaging Specification", spec_name, "print_tooling_required"): + return "Not Required", [] + + cylinders = frappe.get_all( + "Print Cylinder", + filters={"packaging_specification": spec_name, "status": ["!=", "Retired"]}, + fields=["name", "status"], + ) + if not cylinders: + return "Blocked", ["Print tooling is required but no cylinders or plates are registered"] + not_ready = [c.name for c in cylinders if c.status != READY_CYLINDER_STATUS] + if not_ready: + return "Blocked", [f"Tooling not Ready: {', '.join(not_ready)}"] + return "Ready", [] + + +def release_blockers(work_order): + """Everything that stops this Work Order from being released to the floor.""" + blockers = [] + spec = work_order.get("makap_packaging_specification") + + if spec: + if frappe.db.get_value("Packaging Specification", spec, "specification_status") != RELEASABLE_SPEC_STATUS: + blockers.append("Packaging Specification is not Approved for Manufacture") + elif work_order.get("production_item") and frappe.db.get_value( + "Item", work_order.production_item, "makap_requires_packaging_specification" + ): + blockers.append("Production Item requires a Packaging Specification") + + if not work_order.get("bom_no"): + blockers.append("No BOM is selected") + elif work_order.get("makap_bom_compatibility_status") not in COMPATIBLE: + blockers.append("BOM compatibility is not confirmed") + + if work_order.get("makap_process_compatibility_status") not in COMPATIBLE: + blockers.append("Process compatibility is not confirmed") + + tooling, tooling_blockers = tooling_status(spec) + work_order.makap_tooling_readiness_status = tooling + blockers.extend(tooling_blockers) + return blockers