From 78d5fbaca4b94355ae5f514b194ae6d13ff08c77 Mon Sep 17 00:00:00 2001 From: Nabin Hait Date: Fri, 5 Jun 2026 12:29:55 +0530 Subject: [PATCH] refactor(stock): address layering/robustness review findings (#8, #9, #10) #8 ledger_preview: wrap the submit-in-memory dry run in a savepoint inside get_accounting_ledger_preview / get_stock_ledger_preview and roll back to it in a finally, so the preview never persists entries regardless of caller (previously only the whitelisted show_*_preview wrappers' full rollback made it safe). #9 exceptions: move BatchExpiredError and the QualityInspection* errors into a new erpnext/stock/exceptions.py and re-export them from stock_controller for backward compatibility (job_card and tests still import from the controller; identity is preserved). Services now import from the neutral module instead of back from the controller they were extracted out of. #10 quality inspection: extract the duplicated doctype->inspection-field map into a single INSPECTION_FIELDNAME_MAP constant in the service, consumed by both validate_inspection and check_item_quality_inspection. Verified: ledger snapshots, quality inspection suite, stock_entry batch-expiry test stay green; preview smoke-tested to persist nothing and not roll back the caller. --- erpnext/controllers/stock_controller.py | 36 +++++----------- erpnext/stock/exceptions.py | 27 ++++++++++++ erpnext/stock/services/ledger_preview.py | 42 ++++++++++++------- erpnext/stock/services/quality_inspection.py | 31 +++++++------- erpnext/stock/services/serial_batch_bundle.py | 2 +- 5 files changed, 83 insertions(+), 55 deletions(-) create mode 100644 erpnext/stock/exceptions.py diff --git a/erpnext/controllers/stock_controller.py b/erpnext/controllers/stock_controller.py index 67862520a83..857ce76eb84 100644 --- a/erpnext/controllers/stock_controller.py +++ b/erpnext/controllers/stock_controller.py @@ -23,25 +23,17 @@ from erpnext.setup.doctype.brand.brand import get_brand_defaults from erpnext.setup.doctype.item_group.item_group import get_item_group_defaults from erpnext.stock import get_warehouse_account_map from erpnext.stock.doctype.item.item import get_item_defaults + +# Re-exported for backward compatibility; canonical home is erpnext.stock.exceptions. +from erpnext.stock.exceptions import ( + BatchExpiredError, + QualityInspectionNotSubmittedError, + QualityInspectionRejectedError, + QualityInspectionRequiredError, +) from erpnext.stock.stock_ledger import get_items_to_be_repost -class QualityInspectionRequiredError(frappe.ValidationError): - pass - - -class QualityInspectionRejectedError(frappe.ValidationError): - pass - - -class QualityInspectionNotSubmittedError(frappe.ValidationError): - pass - - -class BatchExpiredError(frappe.ValidationError): - pass - - class StockController(AccountsController): def validate(self): super().validate() @@ -674,18 +666,12 @@ def repost_required_for_queue(doc: StockController) -> bool: @frappe.whitelist() def check_item_quality_inspection(doctype: str, docstatus: str | int, items: str | list[dict]): + from erpnext.stock.services.quality_inspection import INSPECTION_FIELDNAME_MAP + if isinstance(items, str): items = json.loads(items) - inspection_fieldname_map = { - "Purchase Receipt": "inspection_required_before_purchase", - "Purchase Invoice": "inspection_required_before_purchase", - "Subcontracting Receipt": "inspection_required_before_purchase", - "Sales Invoice": "inspection_required_before_delivery", - "Delivery Note": "inspection_required_before_delivery", - } - - inspection_fieldname = inspection_fieldname_map.get(doctype) + inspection_fieldname = INSPECTION_FIELDNAME_MAP.get(doctype) if inspection_fieldname is None: return [] diff --git a/erpnext/stock/exceptions.py b/erpnext/stock/exceptions.py new file mode 100644 index 00000000000..5ec9c6f1b28 --- /dev/null +++ b/erpnext/stock/exceptions.py @@ -0,0 +1,27 @@ +# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +"""Shared exceptions for stock transactions. + +Raised by the stock services (serial/batch bundle, quality inspection) and +re-exported from ``stock_controller`` for backward compatibility, so the services +do not have to import back from the controller they were extracted out of. +""" + +import frappe + + +class QualityInspectionRequiredError(frappe.ValidationError): + pass + + +class QualityInspectionRejectedError(frappe.ValidationError): + pass + + +class QualityInspectionNotSubmittedError(frappe.ValidationError): + pass + + +class BatchExpiredError(frappe.ValidationError): + pass diff --git a/erpnext/stock/services/ledger_preview.py b/erpnext/stock/services/ledger_preview.py index 20c533c8ddd..cdb8209da2f 100644 --- a/erpnext/stock/services/ledger_preview.py +++ b/erpnext/stock/services/ledger_preview.py @@ -30,17 +30,23 @@ def get_accounting_ledger_preview(doc, filters): "against_voucher", ] - doc.docstatus = 1 + # Dry run: submit in memory to materialise GL entries, read them, then roll back + # to the savepoint so the preview never persists anything, regardless of caller. + frappe.db.savepoint("ledger_preview") + try: + doc.docstatus = 1 - if doc.get("update_stock") or doc.doctype in ("Purchase Receipt", "Delivery Note", "Stock Entry"): - doc.update_stock_ledger() + if doc.get("update_stock") or doc.doctype in ("Purchase Receipt", "Delivery Note", "Stock Entry"): + doc.update_stock_ledger() - doc.make_gl_entries() - columns = get_gl_columns(filters) - gl_entries = get_gl_entries_for_preview(doc.doctype, doc.name, fields) + doc.make_gl_entries() + columns = get_gl_columns(filters) + gl_entries = get_gl_entries_for_preview(doc.doctype, doc.name, fields) - gl_columns = get_columns(columns, fields) - gl_data = get_data(fields, gl_entries) + gl_columns = get_columns(columns, fields) + gl_data = get_data(fields, gl_entries) + finally: + frappe.db.rollback(save_point="ledger_preview") return gl_columns, gl_data @@ -74,15 +80,21 @@ def get_stock_ledger_preview(doc, filters): ] if doc.get("update_stock") or doc.doctype in ("Purchase Receipt", "Delivery Note", "Stock Entry"): - doc.docstatus = 1 - doc.make_bundle_using_old_serial_batch_fields() - doc.update_stock_ledger() + # Dry run: submit in memory to materialise SLEs, read them, then roll back to + # the savepoint so the preview never persists anything, regardless of caller. + frappe.db.savepoint("ledger_preview") + try: + doc.docstatus = 1 + doc.make_bundle_using_old_serial_batch_fields() + doc.update_stock_ledger() - columns = get_sl_columns(filters) - sl_entries = get_sl_entries_for_preview(doc.doctype, doc.name, fields) + columns = get_sl_columns(filters) + sl_entries = get_sl_entries_for_preview(doc.doctype, doc.name, fields) - sl_columns = get_columns(columns, columns_fields) - sl_data = get_data(columns_fields, sl_entries) + sl_columns = get_columns(columns, columns_fields) + sl_data = get_data(columns_fields, sl_entries) + finally: + frappe.db.rollback(save_point="ledger_preview") return sl_columns, sl_data diff --git a/erpnext/stock/services/quality_inspection.py b/erpnext/stock/services/quality_inspection.py index 28fa8320dce..d9441bc6826 100644 --- a/erpnext/stock/services/quality_inspection.py +++ b/erpnext/stock/services/quality_inspection.py @@ -10,6 +10,22 @@ inspection have a present / submitted / non-rejected Quality Inspection. import frappe from frappe import _ +from erpnext.stock.exceptions import ( + QualityInspectionNotSubmittedError, + QualityInspectionRejectedError, + QualityInspectionRequiredError, +) + +# Doctype -> the document-level "inspection required" flag. Shared with +# check_item_quality_inspection in stock_controller so the two stay in sync. +INSPECTION_FIELDNAME_MAP = { + "Purchase Receipt": "inspection_required_before_purchase", + "Purchase Invoice": "inspection_required_before_purchase", + "Subcontracting Receipt": "inspection_required_before_purchase", + "Sales Invoice": "inspection_required_before_delivery", + "Delivery Note": "inspection_required_before_delivery", +} + class QualityInspectionService: def __init__(self, doc) -> None: @@ -17,14 +33,7 @@ class QualityInspectionService: def validate_inspection(self): """Checks if quality inspection is set/ is valid for Items that require inspection.""" - inspection_fieldname_map = { - "Purchase Receipt": "inspection_required_before_purchase", - "Purchase Invoice": "inspection_required_before_purchase", - "Subcontracting Receipt": "inspection_required_before_purchase", - "Sales Invoice": "inspection_required_before_delivery", - "Delivery Note": "inspection_required_before_delivery", - } - inspection_required_fieldname = inspection_fieldname_map.get(self.doc.doctype) + inspection_required_fieldname = INSPECTION_FIELDNAME_MAP.get(self.doc.doctype) # return if inspection is not required on document level if ( @@ -64,8 +73,6 @@ class QualityInspectionService: def validate_qi_presence(self, row): """Check if QI is present on row level. Warn on save and stop on submit if missing.""" - from erpnext.controllers.stock_controller import QualityInspectionRequiredError - if not row.quality_inspection: msg = _("Row #{0}: Quality Inspection is required for Item {1}").format( row.idx, frappe.bold(row.item_code) @@ -77,8 +84,6 @@ class QualityInspectionService: def validate_qi_submission(self, row): """Check if QI is submitted on row level, during submission""" - from erpnext.controllers.stock_controller import QualityInspectionNotSubmittedError - action = frappe.get_single_value("Stock Settings", "action_if_quality_inspection_is_not_submitted") qa_docstatus = frappe.db.get_value("Quality Inspection", row.quality_inspection, "docstatus") @@ -94,8 +99,6 @@ class QualityInspectionService: def validate_qi_rejection(self, row): """Check if QI is rejected on row level, during submission""" - from erpnext.controllers.stock_controller import QualityInspectionRejectedError - action = frappe.get_single_value("Stock Settings", "action_if_quality_inspection_is_rejected") qa_status = frappe.db.get_value("Quality Inspection", row.quality_inspection, "status") diff --git a/erpnext/stock/services/serial_batch_bundle.py b/erpnext/stock/services/serial_batch_bundle.py index ce8ca4c92f5..dce7be2beb3 100644 --- a/erpnext/stock/services/serial_batch_bundle.py +++ b/erpnext/stock/services/serial_batch_bundle.py @@ -102,8 +102,8 @@ class SerialBatchBundleService: ) def validate_serialized_batch(self): - from erpnext.controllers.stock_controller import BatchExpiredError from erpnext.stock.doctype.serial_no.serial_no import get_serial_nos + from erpnext.stock.exceptions import BatchExpiredError is_material_issue = False if self.doc.doctype == "Stock Entry" and self.doc.purpose in ["Material Issue", "Material Transfer"]: