diff --git a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.js b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.js index c304c7f17eb8..3ca9518a1e88 100644 --- a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.js +++ b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.js @@ -22,27 +22,50 @@ frappe.ui.form.on("Repost Accounting Ledger", { }, refresh: function (frm) { - frm.add_custom_button(__("Show Preview"), () => { - frm.call({ - method: "generate_preview", - doc: frm.doc, - freeze: true, - freeze_message: __("Generating Preview"), - callback: function (r) { - if (r && r.message) { - let content = r.message; - let opts = { - title: "Preview", - subtitle: "preview", - content: content, - print_settings: { orientation: "landscape" }, - columns: [], - data: [], - }; - frappe.render_grid(opts); - } - }, + // the server refuses only while the job is alive, so a dead one can be restarted here + if (frm.doc.docstatus == 1 && !["Completed", "Cancelled"].includes(frm.doc.status)) { + frm.add_custom_button(__("Start Reposting"), () => { + frm.events.start_repost(frm); }); + } + + if (frm.doc.docstatus != 2) { + frm.add_custom_button(__("Show Preview"), () => { + frm.events.generate_preview(frm); + }); + } + }, + + generate_preview: function (frm) { + frm.call({ + method: "generate_preview", + doc: frm.doc, + freeze: true, + freeze_message: __("Generating Preview"), + callback: function (r) { + if (r && r.message) { + let content = r.message; + let opts = { + title: "Preview", + subtitle: "preview", + content: content, + print_settings: { orientation: "landscape" }, + columns: [], + data: [], + }; + frappe.render_grid(opts); + } + }, + }); + }, + + start_repost: function (frm) { + frm.call({ + method: "start_repost", + doc: frm.doc, + callback: function (r) { + frm.reload_doc(); + }, }); }, }); diff --git a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.json b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.json index 0d74b2150f7f..50adc5101e1a 100644 --- a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.json +++ b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.json @@ -1,5 +1,6 @@ { "actions": [], + "allow_bulk_edit": 1, "creation": "2023-07-04 13:07:32.923675", "default_view": "List", "doctype": "DocType", @@ -7,16 +8,24 @@ "engine": "InnoDB", "field_order": [ "company", - "column_break_vpup", "delete_cancelled_entries", + "column_break_vpup", + "status", "section_break_metl", "vouchers", - "amended_from" + "error_section", + "error_log", + "miscellaneous_section", + "amended_from", + "column_break_hrah", + "scheduled_job" ], "fields": [ { "fieldname": "company", "fieldtype": "Link", + "in_list_view": 1, + "in_standard_filter": 1, "label": "Company", "options": "Company" }, @@ -48,12 +57,54 @@ "fieldname": "delete_cancelled_entries", "fieldtype": "Check", "label": "Delete Cancelled Ledger Entries" + }, + { + "fieldname": "error_section", + "fieldtype": "Section Break", + "label": "Error" + }, + { + "fieldname": "error_log", + "fieldtype": "Code", + "label": "Error Log", + "no_copy": 1, + "print_hide": 1, + "read_only": 1 + }, + { + "fieldname": "miscellaneous_section", + "fieldtype": "Section Break", + "label": "Miscellaneous" + }, + { + "fieldname": "column_break_hrah", + "fieldtype": "Column Break" + }, + { + "depends_on": "eval:doc.docstatus >= 1;", + "fieldname": "status", + "fieldtype": "Select", + "in_list_view": 1, + "in_standard_filter": 1, + "label": "Status", + "no_copy": 1, + "options": "\nQueued\nIn Progress\nPartially Reposted\nCompleted\nFailed\nCancelled", + "read_only": 1 + }, + { + "fieldname": "scheduled_job", + "fieldtype": "Link", + "hidden": 1, + "label": "Scheduled Job", + "no_copy": 1, + "options": "RQ Job", + "read_only": 1 } ], "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2024-06-03 17:30:37.012593", + "modified": "2026-07-28 00:56:50.290314", "modified_by": "Administrator", "module": "Accounts", "name": "Repost Accounting Ledger", @@ -80,4 +131,4 @@ "sort_order": "DESC", "states": [], "track_changes": 1 -} \ No newline at end of file +} diff --git a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py index f11aeda4383c..ebf79077cc84 100644 --- a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py +++ b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py @@ -7,7 +7,14 @@ from frappe import _, qb from frappe.desk.form.linked_with import get_child_tables_of_doctypes from frappe.model.document import Document +from frappe.utils.background_jobs import create_job_id, is_job_enqueued from frappe.utils.data import comma_and +from frappe.utils.scheduler import is_scheduler_inactive + +# a batch has to finish well within the timeout of the job reposting it +MAX_VOUCHERS_PER_REPOST = 50 + +HANDLED_VOUCHER_STATUSES = ("Reposted", "Skipped") from erpnext.stock import get_warehouse_account_map @@ -28,6 +35,11 @@ class RepostAccountingLedger(Document): amended_from: DF.Link | None company: DF.Link | None delete_cancelled_entries: DF.Check + error_log: DF.Code | None + scheduled_job: DF.Link | None + status: DF.Literal[ + "", "Queued", "In Progress", "Partially Reposted", "Completed", "Failed", "Cancelled" + ] vouchers: DF.Table[RepostAccountingLedgerItems] # end: auto-generated types @@ -37,6 +49,11 @@ def __init__(self, *args, **kwargs): def validate(self): self.validate_vouchers() + self.validate_repost_preconditions() + + def validate_repost_preconditions(self): + """The checks a repost queued days ago could have outlived, re-run before it touches + the ledger. Vouchers cancelled since are skipped one by one while reposting.""" self.validate_for_closed_fiscal_year() self.validate_for_deferred_accounting() @@ -73,8 +90,52 @@ def validate_for_closed_fiscal_year(self): frappe.throw(_("Cannot Resubmit Ledger entries for vouchers in Closed fiscal year.")) def validate_vouchers(self): - if self.vouchers: - validate_docs_for_voucher_types([x.voucher_type for x in self.vouchers]) + if not self.vouchers: + frappe.throw(_("Add atleast one voucher to repost.")) + + if len(self.vouchers) > MAX_VOUCHERS_PER_REPOST: + frappe.throw( + _("Cannot repost more than {0} vouchers at once. Split them into multiple documents.").format( + MAX_VOUCHERS_PER_REPOST + ) + ) + + validate_docs_for_voucher_types([x.voucher_type for x in self.vouchers]) + + self.validate_no_duplicate_vouchers() + self.validate_vouchers_are_submitted() + + def validate_no_duplicate_vouchers(self): + vouchers = [(x.voucher_type, x.voucher_no) for x in self.vouchers] + + if len(vouchers) != len(set(vouchers)): + frappe.throw(_("Duplicate vouchers found. Remove the duplicate vouchers to continue to repost.")) + + def validate_vouchers_are_submitted(self): + voucher_type_wise_map = {} + for d in self.vouchers: + voucher_type_wise_map.setdefault(d.voucher_type, []) + voucher_type_wise_map[d.voucher_type].append(d.voucher_no) + + non_submitted_vouchers = [] + for key in voucher_type_wise_map.keys(): + non_submitted_vouchers.extend( + frappe.get_all( + key, + filters={"name": ["in", voucher_type_wise_map[key]], "docstatus": ["!=", 1]}, + pluck="name", + ) + ) + + if non_submitted_vouchers: + frappe.throw( + _("The following vouchers are not submitted: {0}").format( + comma_and(non_submitted_vouchers, add_quotes=True) + ) + ) + + def on_discard(self): + self.db_set("status", "Cancelled") def get_existing_ledger_entries(self): vouchers = [x.voucher_no for x in self.vouchers] @@ -139,80 +200,245 @@ def generate_preview(self): return rendered_page def on_submit(self): - if len(self.vouchers) > 5: - job_name = "repost_accounting_ledger_" + self.name - frappe.enqueue( - method="erpnext.accounts.doctype.repost_accounting_ledger.repost_accounting_ledger.start_repost", - account_repost_doc=self.name, - is_async=True, - job_name=job_name, - enqueue_after_commit=True, + self.start_repost() + + def before_cancel(self): + self._raise_error_if_reposting_in_progress() + + def on_cancel(self): + self.db_set("status", "Cancelled") + + def _raise_error_if_reposting_in_progress(self): + if self.scheduled_job and is_job_enqueued(_repost_job_id(self.name)): + frappe.throw(_("Reposting is still in progress in background.")) + + @frappe.whitelist() + def start_repost(self): + if self.docstatus != 1: + frappe.throw(_("Reposting can be started only for submitted document.")) + + # under a row lock, so two concurrent starts cannot both get past here + status = frappe.db.get_value(self.doctype, self.name, "status", for_update=True) + if status in ("Completed", "Cancelled"): + frappe.throw(_("Reposting cannot be started when status is {0}.").format(status)) + + # `Queued` and `In Progress` are held back by the job, not by the status: a worker that + # died leaves the status behind and the document has to stay restartable + self._raise_error_if_reposting_in_progress() + + self.check_permission("write") + + # workers pick up enqueued jobs whether or not the scheduler runs, so this is a warning + if is_scheduler_inactive(): + frappe.msgprint( + _("Scheduler is inactive. Reposting will only run once background jobs are processed."), + alert=True, + indicator="orange", ) - frappe.msgprint(_("Repost has started in the background")) - else: - start_repost(self.name) + self.db_set({"status": "Queued", "scheduled_job": create_job_id(_repost_job_id(self.name))}) + _enqueue_repost(self.name) + frappe.msgprint(_("Repost has started in the background"), alert=True, indicator="blue") + + +def _repost_job_id(repost_doc_name: str) -> str: + """Derived from the document, so a repost can only ever have one job.""" + return f"repost_accounting_ledger::{repost_doc_name}" + + +def _enqueue_repost(repost_doc_name: str) -> None: + """Hand the repost to a background worker. + + Tests run it in the foreground, inside their own transaction: documents edited after submit + repost themselves through `repost_accounting_entries`, and tests across apps assert on the + ledger right after doing so. + """ + frappe.enqueue( + method="erpnext.accounts.doctype.repost_accounting_ledger.repost_accounting_ledger.repost", + repost_doc_name=repost_doc_name, + commit=not frappe.flags.in_test, + queue="long", + timeout=1500, + job_id=_repost_job_id(repost_doc_name), + deduplicate=True, + enqueue_after_commit=True, + now=frappe.flags.in_test, + ) -@frappe.whitelist() -def start_repost(account_repost_doc: str | None = None) -> None: - from erpnext.accounts.general_ledger import make_reverse_gl_entries + +def _lock_vouchers(vouchers) -> dict: + """Lock every voucher up front so a concurrent repost cannot touch the same GL entries. + + Returns them keyed by voucher, so reposting does not load them again. These are file locks + under the site directory: they serialise nothing across hosts that do not share it, and a + worker killed outright leaves them behind until they expire. + """ + locked_docs = {} + try: + for x in vouchers: + doc = frappe.get_doc(x.voucher_type, x.voucher_no) + doc.lock() + locked_docs[(x.voucher_type, x.voucher_no)] = doc + except Exception: + for doc in locked_docs.values(): + doc.unlock() + raise + return locked_docs + + +def repost(repost_doc_name: str, commit: bool = True): + """Repost every voucher of the document, one transaction at a time. + + `commit` says whether this call owns the transaction. The background job does, and commits + after every voucher so progress survives a crash; a caller inside its own passes `False`. + """ + from erpnext.accounts.utils import _delete_accounting_ledger_entries, _delete_adv_pl_entries frappe.flags.through_repost_accounting_ledger = True - if account_repost_doc: - repost_doc = frappe.get_doc("Repost Accounting Ledger", account_repost_doc) - repost_doc.check_permission("write") - if repost_doc.docstatus == 1: - # Prevent repost on invoices with deferred accounting - repost_doc.validate_for_deferred_accounting() + repost_doc = frappe.get_doc("Repost Accounting Ledger", repost_doc_name) + locked_docs = {} - for x in repost_doc.vouchers: - doc = frappe.get_doc(x.voucher_type, x.voucher_no) + try: + repost_doc.validate_repost_preconditions() + + # a retry leaves the vouchers it is done with alone: they are not locked, not loaded + # and not reposted again + pending = [x for x in repost_doc.vouchers if x.status not in HANDLED_VOUCHER_STATUSES] + locked_docs = _lock_vouchers(pending) + + repost_doc.db_set("status", "In Progress", commit=commit) + + for position, x in enumerate(pending, start=1): + frappe.publish_progress( + position * 100 / len(pending), + doctype=repost_doc.doctype, + docname=repost_doc.name, + description=_("Reposting {0} {1}").format(x.voucher_type, x.voucher_no), + ) + + save_point = "reposting" + frappe.db.savepoint(save_point=save_point) + try: + doc = locked_docs[(x.voucher_type, x.voucher_no)] + + if doc.docstatus == 2: + x.db_set({"status": "Skipped", "traceback": ""}) + continue if repost_doc.delete_cancelled_entries: - frappe.db.delete( - "GL Entry", filters={"voucher_type": doc.doctype, "voucher_no": doc.name} - ) - frappe.db.delete( - "Payment Ledger Entry", filters={"voucher_type": doc.doctype, "voucher_no": doc.name} - ) - frappe.db.delete( - "Advance Payment Ledger Entry", - filters={"voucher_type": doc.doctype, "voucher_no": doc.name}, - ) + _delete_accounting_ledger_entries(doc.doctype, doc.name) + _delete_adv_pl_entries(doc.doctype, doc.name) + + _repost_vouchers(doc, repost_doc.delete_cancelled_entries) + except Exception: + frappe.db.rollback(save_point=save_point) + + x.db_set({"status": "Failed", "traceback": frappe.get_traceback()}) + else: + x.db_set({"status": "Reposted", "traceback": ""}) + finally: + if commit: + frappe.db.commit() # nosemgrep + + except Exception: + if commit: + frappe.db.rollback() + + _record_repost_failure(repost_doc, commit=commit) + raise + else: + repost_doc.db_set({"status": _derive_status(repost_doc), "error_log": ""}, notify=True) + finally: + for doc in locked_docs.values(): + doc.unlock() + if commit: + frappe.db.commit() # nosemgrep + + +def _derive_status(repost_doc) -> str: + """Vouchers are committed one by one, so the status follows what was actually handled.""" + handled = sum(1 for voucher in repost_doc.vouchers if voucher.status in HANDLED_VOUCHER_STATUSES) + + if handled == len(repost_doc.vouchers): + return "Completed" + elif handled == 0: + return "Failed" + + return "Partially Reposted" + + +def _record_repost_failure(repost_doc, commit=False) -> None: + """Persist the traceback of a run that could not finish, without discarding its progress.""" + # the traceback with frame locals goes to the Error Log, which is permissioned separately + traceback = frappe.get_traceback() + + frappe.log_error( + title=_("Unable to Repost Accounting Ledger"), + reference_doctype=repost_doc.doctype, + reference_name=repost_doc.name, + ) + + frappe.db.set_value( + repost_doc.doctype, repost_doc.name, {"error_log": traceback, "status": _derive_status(repost_doc)} + ) + + if commit: + frappe.db.commit() + - if doc.doctype in ["Sales Invoice", "Purchase Invoice"]: - if not repost_doc.delete_cancelled_entries: - doc.docstatus = 2 - doc.make_gl_entries_on_cancel(from_repost=True) - - doc.docstatus = 1 - if doc.doctype == "Sales Invoice": - doc.force_set_against_income_account() - else: - doc.force_set_against_expense_account() - doc.make_gl_entries() - - elif doc.doctype == "Purchase Receipt": - if not repost_doc.delete_cancelled_entries: - doc.docstatus = 2 - doc.make_gl_entries_on_cancel(from_repost=True) - - doc.docstatus = 1 - doc.make_gl_entries(from_repost=True) - - elif doc.doctype in ["Payment Entry", "Journal Entry", "Expense Claim"]: - if not repost_doc.delete_cancelled_entries: - doc.make_gl_entries(1) - doc.make_gl_entries() - elif doc.doctype in frappe.get_hooks("repost_allowed_doctypes"): - if hasattr(doc, "make_gl_entries") and callable(doc.make_gl_entries): - if not repost_doc.delete_cancelled_entries: - if "cancel" in inspect.getfullargspec(doc.make_gl_entries): - doc.make_gl_entries(cancel=1) - else: - make_reverse_gl_entries(voucher_type=doc.doctype, voucher_no=doc.name) - doc.make_gl_entries() +def _repost_vouchers(doc, delete_cancelled_entries: bool | int | None): + if doc.doctype in ["Sales Invoice", "Purchase Invoice"]: + _repost_invoices(doc, delete_cancelled_entries) + + elif doc.doctype == "Purchase Receipt": + _repost_purchase_receipt(doc, delete_cancelled_entries) + + elif doc.doctype in ["Payment Entry", "Journal Entry"]: + _repost_pe_je(doc, delete_cancelled_entries) + + elif doc.doctype in frappe.get_hooks("repost_allowed_doctypes"): + _repost_allowed_hook_doctypes(doc, delete_cancelled_entries) + + +def _repost_invoices(invoice_doc, delete_cancelled_entries): + if not delete_cancelled_entries: + invoice_doc.docstatus = 2 + invoice_doc.make_gl_entries_on_cancel(from_repost=True) + + invoice_doc.docstatus = 1 + if invoice_doc.doctype == "Sales Invoice": + invoice_doc.force_set_against_income_account() + else: + invoice_doc.force_set_against_expense_account() + invoice_doc.make_gl_entries() + + +def _repost_purchase_receipt(receipt_doc, delete_cancelled_entries): + if not delete_cancelled_entries: + receipt_doc.docstatus = 2 + receipt_doc.make_gl_entries_on_cancel(from_repost=True) + + receipt_doc.docstatus = 1 + receipt_doc.make_gl_entries(from_repost=True) + + +def _repost_pe_je(entry_doc, delete_cancelled_entries): + if not delete_cancelled_entries: + entry_doc.make_gl_entries(cancel=1) + entry_doc.make_gl_entries() + + +def _repost_allowed_hook_doctypes(repost_doc, delete_cancelled_entries: bool | int | None): + from erpnext.accounts.general_ledger import make_reverse_gl_entries + + if hasattr(repost_doc, "make_gl_entries") and callable(repost_doc.make_gl_entries): + if not delete_cancelled_entries: + if "cancel" in inspect.getfullargspec(repost_doc.make_gl_entries).args: + repost_doc.make_gl_entries(cancel=1) + else: + make_reverse_gl_entries(voucher_type=repost_doc.doctype, voucher_no=repost_doc.name) + repost_doc.make_gl_entries() def get_allowed_types_from_settings(child_doc: bool = False): diff --git a/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger_list.js b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger_list.js new file mode 100644 index 000000000000..0ecdca3843cc --- /dev/null +++ b/erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger_list.js @@ -0,0 +1,16 @@ +frappe.listview_settings["Repost Accounting Ledger"] = { + add_fields: ["status"], + // drafts and cancelled documents are coloured by the framework before it gets here + get_indicator: function (doc) { + if (!doc.status) return; + + const status_color = { + Queued: "yellow", + "In Progress": "blue", + "Partially Reposted": "orange", + Completed: "green", + Failed: "red", + }; + return [__(doc.status), status_color[doc.status] || "gray", "status,=," + doc.status]; + }, +}; diff --git a/erpnext/accounts/doctype/repost_accounting_ledger/test_repost_accounting_ledger.py b/erpnext/accounts/doctype/repost_accounting_ledger/test_repost_accounting_ledger.py index 7ed999831cdc..49554914ee1f 100644 --- a/erpnext/accounts/doctype/repost_accounting_ledger/test_repost_accounting_ledger.py +++ b/erpnext/accounts/doctype/repost_accounting_ledger/test_repost_accounting_ledger.py @@ -1,20 +1,35 @@ # Copyright (c) 2023, Frappe Technologies Pvt. Ltd. and Contributors # See license.txt +from contextlib import contextmanager +from unittest.mock import patch + import frappe from frappe import qb from frappe.query_builder.functions import Sum from frappe.tests.utils import FrappeTestCase, change_settings from frappe.utils import add_days, nowdate, today +from erpnext.accounts.doctype.journal_entry.test_journal_entry import make_journal_entry from erpnext.accounts.doctype.payment_entry.payment_entry import get_payment_entry from erpnext.accounts.doctype.payment_request.payment_request import make_payment_request +from erpnext.accounts.doctype.repost_accounting_ledger.repost_accounting_ledger import ( + _lock_vouchers, + _record_repost_failure, + _repost_allowed_hook_doctypes, + _repost_job_id, + _repost_vouchers, + repost, +) from erpnext.accounts.doctype.sales_invoice.test_sales_invoice import create_sales_invoice from erpnext.accounts.test.accounts_mixin import AccountsTestMixin from erpnext.accounts.utils import get_fiscal_year from erpnext.stock.doctype.item.test_item import make_item from erpnext.stock.doctype.purchase_receipt.test_purchase_receipt import get_gl_entries, make_purchase_receipt +REPOST_MODULE = "erpnext.accounts.doctype.repost_accounting_ledger.repost_accounting_ledger" +SIMULATED_FAILURE = "Simulated repost failure" + class TestRepostAccountingLedger(AccountsTestMixin, FrappeTestCase): def setUp(self): @@ -26,8 +41,8 @@ def setUp(self): def tearDown(self): frappe.db.rollback() - def test_01_basic_functions(self): - si = create_sales_invoice( + def make_invoice(self, **kwargs): + return create_sales_invoice( item=self.item, company=self.company, customer=self.customer, @@ -35,8 +50,71 @@ def test_01_basic_functions(self): parent_cost_center=self.cost_center, cost_center=self.cost_center, rate=100, + **kwargs, ) + def make_invoice_and_payment(self): + si = self.make_invoice() + pe = get_payment_entry(si.doctype, si.name) + pe.save().submit() + return si, pe + + def create_repost_doc(self, vouchers, delete_cancelled_entries=False, submit=False): + ral = frappe.new_doc("Repost Accounting Ledger") + ral.company = self.company + ral.delete_cancelled_entries = delete_cancelled_entries + for voucher in vouchers: + ral.append("vouchers", {"voucher_type": voucher.doctype, "voucher_no": voucher.name}) + + ral.save() + if submit: + ral.submit() + ral.reload() + return ral + + @contextmanager + def patched_repost(self, fail_for=()): + """Yield the vouchers handed over to `_repost_vouchers`, failing the given types.""" + reposted = [] + + def repost_voucher(doc, delete_cancelled_entries): + reposted.append(doc.name) + if doc.doctype in fail_for: + frappe.throw(SIMULATED_FAILURE) + _repost_vouchers(doc, delete_cancelled_entries) + + with patch(f"{REPOST_MODULE}._repost_vouchers", new=repost_voucher): + yield reposted + + def make_period_closing_voucher(self): + fy = get_fiscal_year(today(), company=self.company) + pcv = frappe.get_doc( + { + "doctype": "Period Closing Voucher", + "transaction_date": today(), + "period_start_date": fy[1], + "period_end_date": today(), + "company": self.company, + "fiscal_year": fy[0], + "cost_center": self.cost_center, + "closing_account_head": self.retained_earnings, + "remarks": "test", + } + ) + return pcv.save().submit() + + def get_gl_totals(self, voucher_no, is_cancelled=0): + gl = qb.DocType("GL Entry") + return ( + qb.from_(gl) + .select(Sum(gl.debit).as_("debit"), Sum(gl.credit).as_("credit")) + .where((gl.voucher_no == voucher_no) & (gl.is_cancelled == is_cancelled)) + .run() + )[0] + + def test_01_basic_functions(self): + si = self.make_invoice() + preq = frappe.get_doc( make_payment_request( dt=si.doctype, @@ -70,51 +148,24 @@ def test_01_basic_functions(self): gle = frappe.db.get_all("GL Entry", filters={"voucher_no": si.name, "account": self.debit_to}) frappe.db.set_value("GL Entry", gle[0], "debit", 90) - gl = qb.DocType("GL Entry") - res = ( - qb.from_(gl) - .select(gl.voucher_no, Sum(gl.debit).as_("debit"), Sum(gl.credit).as_("credit")) - .where((gl.voucher_no == si.name) & (gl.is_cancelled == 0)) - .run() - ) - # Assert incorrect ledger balance - self.assertNotEqual(res[0], (si.name, 100, 100)) + self.assertNotEqual(self.get_gl_totals(si.name), (100, 100)) # Submit repost document ral.save().submit() - res = ( - qb.from_(gl) - .select(gl.voucher_no, Sum(gl.debit).as_("debit"), Sum(gl.credit).as_("credit")) - .where((gl.voucher_no == si.name) & (gl.is_cancelled == 0)) - .run() - ) - # Ledger should reflect correct amount post repost - self.assertEqual(res[0], (si.name, 100, 100)) + self.assertEqual(self.get_gl_totals(si.name), (100, 100)) def test_02_deferred_accounting_valiations(self): - si = create_sales_invoice( - item=self.item, - company=self.company, - customer=self.customer, - debit_to=self.debit_to, - parent_cost_center=self.cost_center, - cost_center=self.cost_center, - rate=100, - do_not_submit=True, - ) + si = self.make_invoice(do_not_submit=True) si.items[0].enable_deferred_revenue = True si.items[0].deferred_revenue_account = self.deferred_revenue si.items[0].service_start_date = nowdate() si.items[0].service_end_date = add_days(nowdate(), 90) si.save().submit() - ral = frappe.new_doc("Repost Accounting Ledger") - ral.company = self.company - ral.append("vouchers", {"voucher_type": si.doctype, "voucher_no": si.name}) - self.assertRaises(frappe.ValidationError, ral.save) + self.assertRaises(frappe.ValidationError, self.create_repost_doc, [si]) @change_settings("Accounts Settings", {"delete_linked_ledger_entries": 1}) def test_04_pcv_validation(self): @@ -122,86 +173,29 @@ def test_04_pcv_validation(self): gl = frappe.qb.DocType("GL Entry") qb.from_(gl).delete().where(gl.company == self.company).run() - si = create_sales_invoice( - item=self.item, - company=self.company, - customer=self.customer, - debit_to=self.debit_to, - parent_cost_center=self.cost_center, - cost_center=self.cost_center, - rate=100, - ) - fy = get_fiscal_year(today(), company=self.company) - pcv = frappe.get_doc( - { - "doctype": "Period Closing Voucher", - "transaction_date": today(), - "period_start_date": fy[1], - "period_end_date": today(), - "company": self.company, - "fiscal_year": fy[0], - "cost_center": self.cost_center, - "closing_account_head": self.retained_earnings, - "remarks": "test", - } - ) - pcv.save().submit() + si = self.make_invoice() + pcv = self.make_period_closing_voucher() - ral = frappe.new_doc("Repost Accounting Ledger") - ral.company = self.company - ral.append("vouchers", {"voucher_type": si.doctype, "voucher_no": si.name}) - self.assertRaises(frappe.ValidationError, ral.save) + self.assertRaises(frappe.ValidationError, self.create_repost_doc, [si]) pcv.reload() pcv.cancel() pcv.delete() def test_03_deletion_flag_and_preview_function(self): - si = create_sales_invoice( - item=self.item, - company=self.company, - customer=self.customer, - debit_to=self.debit_to, - parent_cost_center=self.cost_center, - cost_center=self.cost_center, - rate=100, - ) - - pe = get_payment_entry(si.doctype, si.name) - pe.save().submit() + si, pe = self.make_invoice_and_payment() # with deletion flag set - ral = frappe.new_doc("Repost Accounting Ledger") - ral.company = self.company - ral.delete_cancelled_entries = True - ral.append("vouchers", {"voucher_type": si.doctype, "voucher_no": si.name}) - ral.append("vouchers", {"voucher_type": pe.doctype, "voucher_no": pe.name}) - ral.save().submit() + self.create_repost_doc([si, pe], delete_cancelled_entries=True, submit=True) self.assertIsNone(frappe.db.exists("GL Entry", {"voucher_no": si.name, "is_cancelled": 1})) self.assertIsNone(frappe.db.exists("GL Entry", {"voucher_no": pe.name, "is_cancelled": 1})) def test_05_without_deletion_flag(self): - si = create_sales_invoice( - item=self.item, - company=self.company, - customer=self.customer, - debit_to=self.debit_to, - parent_cost_center=self.cost_center, - cost_center=self.cost_center, - rate=100, - ) - - pe = get_payment_entry(si.doctype, si.name) - pe.save().submit() + si, pe = self.make_invoice_and_payment() # without deletion flag set - ral = frappe.new_doc("Repost Accounting Ledger") - ral.company = self.company - ral.delete_cancelled_entries = False - ral.append("vouchers", {"voucher_type": si.doctype, "voucher_no": si.name}) - ral.append("vouchers", {"voucher_type": pe.doctype, "voucher_no": pe.name}) - ral.save().submit() + self.create_repost_doc([si, pe], submit=True) self.assertIsNotNone(frappe.db.exists("GL Entry", {"voucher_no": si.name, "is_cancelled": 1})) self.assertIsNotNone(frappe.db.exists("GL Entry", {"voucher_no": pe.name, "is_cancelled": 1})) @@ -247,11 +241,7 @@ def test_06_repost_purchase_receipt(self): another_provisional_account, ) - repost_doc = frappe.new_doc("Repost Accounting Ledger") - repost_doc.company = self.company - repost_doc.delete_cancelled_entries = True - repost_doc.append("vouchers", {"voucher_type": pr.doctype, "voucher_no": pr.name}) - repost_doc.save().submit() + repost_doc = self.create_repost_doc([pr], delete_cancelled_entries=True, submit=True) pr_gles_after_repost = get_gl_entries(pr.doctype, pr.name, skip_cancelled=True) expected_pr_gles_after_repost = [ @@ -272,6 +262,279 @@ def test_06_repost_purchase_receipt(self): company.default_provisional_account = None company.save() + def test_07_voucher_validations(self): + submitted_si = self.make_invoice() + draft_si = self.make_invoice(do_not_submit=True) + cancelled_si = self.make_invoice() + cancelled_si.cancel() + + for vouchers, exception, message in ( + ([], frappe.ValidationError, "Add atleast one voucher"), + ([submitted_si, submitted_si], frappe.ValidationError, "Duplicate vouchers found"), + ([draft_si], frappe.ValidationError, f"not submitted.*{draft_si.name}"), + # cancelled vouchers don't make it past link validation + ([cancelled_si], frappe.CancelledLinkError, "Cannot link cancelled document"), + ): + with self.subTest(vouchers=[x.name for x in vouchers]): + self.assertRaisesRegex(exception, message, self.create_repost_doc, vouchers) + + self.create_repost_doc([submitted_si]) + + def test_08_voucher_count_limit(self): + si, pe = self.make_invoice_and_payment() + another_si = self.make_invoice() + + with patch(f"{REPOST_MODULE}.MAX_VOUCHERS_PER_REPOST", 2): + self.create_repost_doc([si, pe]) + self.assertRaisesRegex( + frappe.ValidationError, + "Cannot repost more than 2 vouchers", + self.create_repost_doc, + [si, pe, another_si], + ) + + def test_09_status_lifecycle(self): + si, pe = self.make_invoice_and_payment() + + ral = self.create_repost_doc([si, pe]) + self.assertEqual(ral.status, "") + + ral.submit() + ral.reload() + + self.assertEqual(ral.status, "Completed") + self.assertFalse(ral.error_log) + for voucher in ral.vouchers: + self.assertEqual(voucher.status, "Reposted") + self.assertFalse(voucher.traceback) + + ral.cancel() + ral.reload() + self.assertEqual(ral.status, "Cancelled") + + # the `discard` flow (and the `on_discard` hook it triggers) only exists on v16, + # so there is nothing to assert here on v15 + + def test_10_start_repost_guards(self): + si = self.make_invoice() + ral = self.create_repost_doc([si]) + + self.assertRaisesRegex(frappe.ValidationError, "only for submitted document", ral.start_repost) + + ral.submit() + ral.reload() + self.assertRaisesRegex( + frappe.ValidationError, "cannot be started when status is Completed", ral.start_repost + ) + + # a document left behind by a worker that died mid-repost + ral.db_set("status", "In Progress") + + with patch(f"{REPOST_MODULE}.is_job_enqueued", return_value=True): + self.assertRaisesRegex( + frappe.ValidationError, "still in progress in background", ral.start_repost + ) + self.assertRaisesRegex(frappe.ValidationError, "still in progress in background", ral.cancel) + + # `cancel` flips docstatus in memory before running `before_cancel` + ral.reload() + + with patch(f"{REPOST_MODULE}.is_job_enqueued", return_value=False): + # the job is gone, so `In Progress` must not keep the document stuck + ral.start_repost() + + ral.reload() + self.assertEqual(ral.status, "Completed") + + def test_11_repost_job_is_tied_to_the_document(self): + si = self.make_invoice() + ral = self.create_repost_doc([si], submit=True) + ral.db_set("status", "Failed") + + with patch(f"{REPOST_MODULE}.frappe.enqueue") as enqueue: + ral.start_repost() + + kwargs = enqueue.call_args.kwargs + self.assertEqual(kwargs["repost_doc_name"], ral.name) + self.assertEqual(kwargs["job_id"], _repost_job_id(ral.name)) + # a second start cannot queue a second job for the same document + self.assertTrue(kwargs["deduplicate"]) + + def test_12_voucher_failures_are_isolated_and_retried(self): + si, pe = self.make_invoice_and_payment() + pe_gl_entries = frappe.db.count("GL Entry", {"voucher_no": pe.name}) + + # the deletion flag drops the existing entries before reposting them + ral = self.create_repost_doc([si, pe], delete_cancelled_entries=True) + with self.patched_repost(fail_for=["Payment Entry"]): + ral.submit() + + ral.reload() + self.assertEqual(ral.status, "Partially Reposted") + + si_row, pe_row = ral.vouchers + self.assertEqual((si_row.status, pe_row.status), ("Reposted", "Failed")) + self.assertFalse(si_row.traceback) + self.assertIn(SIMULATED_FAILURE, pe_row.traceback) + + # the failed voucher is rolled back to its savepoint, so its entries are back + self.assertEqual(frappe.db.count("GL Entry", {"voucher_no": pe.name}), pe_gl_entries) + + # a retry only picks up the vouchers that are not reposted yet, and leaves the rest + # alone entirely: they are not locked or loaded either + with ( + patch(f"{REPOST_MODULE}._lock_vouchers", side_effect=_lock_vouchers) as lock_vouchers, + self.patched_repost() as retried, + ): + ral.start_repost() + + self.assertEqual(retried, [pe.name]) + self.assertEqual([x.voucher_no for x in lock_vouchers.call_args.args[0]], [pe.name]) + + ral.reload() + self.assertEqual(ral.status, "Completed") + for voucher in ral.vouchers: + self.assertEqual(voucher.status, "Reposted") + self.assertFalse(voucher.traceback) + + def test_13_status_of_a_run_that_could_not_finish(self): + si, pe = self.make_invoice_and_payment() + + ral = self.create_repost_doc([si, pe]) + with self.patched_repost(fail_for=["Payment Entry"]): + ral.submit() + + ral.reload() + + # the job dies after the loop committed the invoice, e.g. killed or timed out + try: + frappe.throw(SIMULATED_FAILURE) + except frappe.ValidationError: + _record_repost_failure(ral) + + ral.reload() + + # progress already committed must not be reported as a total failure + self.assertEqual(ral.status, "Partially Reposted") + self.assertIn(SIMULATED_FAILURE, ral.error_log) + self.assertTrue( + frappe.db.exists("Error Log", {"reference_doctype": ral.doctype, "reference_name": ral.name}) + ) + + @change_settings("Accounts Settings", {"delete_linked_ledger_entries": 1}) + def test_14_period_closed_after_the_repost_was_started(self): + gl = qb.DocType("GL Entry") + qb.from_(gl).delete().where(gl.company == self.company).run() + + si = self.make_invoice() + ral = self.create_repost_doc([si], submit=True) + ral.db_set("status", "Failed") + ral.vouchers[0].db_set("status", "Pending") + + # the period is closed between the repost being started and the job running + self.make_period_closing_voucher() + + gl_entries = frappe.db.count("GL Entry", {"voucher_no": si.name}) + self.assertRaisesRegex(frappe.ValidationError, "Closed fiscal year", repost, ral.name, commit=False) + + ral.reload() + self.assertEqual(ral.status, "Failed") + self.assertIn("Closed fiscal year", ral.error_log) + + # the ledger is left exactly as it was + self.assertEqual(frappe.db.count("GL Entry", {"voucher_no": si.name}), gl_entries) + self.assertEqual(ral.vouchers[0].status, "Pending") + + def test_15_failed_repost_skips_cancelled_voucher(self): + si = self.make_invoice() + + ral = self.create_repost_doc([si]) + with self.patched_repost(fail_for=["Sales Invoice"]): + ral.submit() + + ral.reload() + self.assertEqual(ral.status, "Failed") + + si.reload() + si.cancel() + + ral.start_repost() + ral.reload() + + # nothing was reposted, but there is nothing left to repost either + self.assertEqual(ral.status, "Completed") + self.assertEqual(ral.vouchers[0].status, "Skipped") + self.assertFalse(ral.vouchers[0].traceback) + + def test_16_concurrent_repost_is_blocked_by_voucher_lock(self): + si, pe = self.make_invoice_and_payment() + ral = self.create_repost_doc([si, pe]) + + # a concurrent repost holding the lock on the second voucher + locked_pe = frappe.get_doc(pe.doctype, pe.name) + locked_pe.lock() + try: + self.assertRaises(frappe.DocumentLockedError, ral.submit) + + # vouchers locked before the failure are released again + self.assertFalse(frappe.get_doc(si.doctype, si.name).is_locked) + finally: + locked_pe.unlock() + + def test_17_journal_entry_repost(self): + je = make_journal_entry("_Test Bank - _TC", "_Test Cash - _TC", 500, submit=True) + je = frappe.get_doc("Journal Entry", je.name) + + self.assertEqual(self.get_gl_totals(je.name), (500.0, 500.0)) + + # without the deletion flag the 2 original entries are marked as cancelled, + # along with the 2 reverse entries booked against them + for delete_cancelled_entries, cancelled_entries in ((False, 4), (True, 0)): + with self.subTest(delete_cancelled_entries=delete_cancelled_entries): + ral = self.create_repost_doc( + [je], delete_cancelled_entries=delete_cancelled_entries, submit=True + ) + + self.assertEqual(ral.status, "Completed") + self.assertEqual(self.get_gl_totals(je.name), (500.0, 500.0)) + self.assertEqual( + frappe.db.count("GL Entry", {"voucher_no": je.name, "is_cancelled": 1}), + cancelled_entries, + ) + + def test_18_hook_allowed_doctype_repost(self): + class VoucherWithCancelArg: + doctype = "Test Repost Voucher" + name = "TRV-00001" + + def __init__(self): + self.calls = [] + + def make_gl_entries(self, cancel=0): + self.calls.append(cancel) + + class VoucherWithoutCancelArg(VoucherWithCancelArg): + def make_gl_entries(self): + self.calls.append("repost") + + # vouchers that can reverse their own entries are asked to do so first + doc = VoucherWithCancelArg() + _repost_allowed_hook_doctypes(doc, delete_cancelled_entries=False) + self.assertEqual(doc.calls, [1, 0]) + + # nothing to reverse when the old entries are deleted + doc = VoucherWithCancelArg() + _repost_allowed_hook_doctypes(doc, delete_cancelled_entries=True) + self.assertEqual(doc.calls, [0]) + + # the rest fall back to the generic reversal + doc = VoucherWithoutCancelArg() + with patch("erpnext.accounts.general_ledger.make_reverse_gl_entries") as make_reverse_gl_entries: + _repost_allowed_hook_doctypes(doc, delete_cancelled_entries=False) + + make_reverse_gl_entries.assert_called_once_with(voucher_type=doc.doctype, voucher_no=doc.name) + self.assertEqual(doc.calls, ["repost"]) + def update_repost_settings(): allowed_types = [ diff --git a/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.json b/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.json index 4a2041f88c6b..d3329dde45bc 100644 --- a/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.json +++ b/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.json @@ -1,5 +1,6 @@ { "actions": [], + "allow_bulk_edit": 1, "allow_rename": 1, "creation": "2023-07-04 14:14:01.243848", "doctype": "DocType", @@ -7,28 +8,63 @@ "engine": "InnoDB", "field_order": [ "voucher_type", - "voucher_no" + "column_break_ndex", + "voucher_no", + "reposting_status_section", + "status", + "traceback" ], "fields": [ { + "columns": 5, "fieldname": "voucher_type", "fieldtype": "Link", "in_list_view": 1, "label": "Voucher Type", - "options": "DocType" + "options": "DocType", + "reqd": 1 }, { + "fieldname": "column_break_ndex", + "fieldtype": "Column Break" + }, + { + "columns": 5, "fieldname": "voucher_no", "fieldtype": "Dynamic Link", "in_list_view": 1, "label": "Voucher No", - "options": "voucher_type" + "options": "voucher_type", + "reqd": 1 + }, + { + "fieldname": "reposting_status_section", + "fieldtype": "Section Break", + "label": "Reposting Status" + }, + { + "columns": 2, + "default": "Pending", + "fieldname": "status", + "fieldtype": "Select", + "in_list_view": 1, + "label": "Status", + "no_copy": 1, + "options": "Pending\nReposted\nSkipped\nFailed", + "read_only": 1 + }, + { + "fieldname": "traceback", + "fieldtype": "Code", + "label": "Traceback", + "no_copy": 1, + "read_only": 1 } ], "index_web_pages_for_search": 1, "istable": 1, "links": [], - "modified": "2023-07-04 14:15:51.165584", + "modified": "2026-07-29 02:41:00.000000", "modified_by": "Administrator", "module": "Accounts", "name": "Repost Accounting Ledger Items", @@ -37,4 +73,4 @@ "sort_field": "modified", "sort_order": "DESC", "states": [] -} \ No newline at end of file +} diff --git a/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.py b/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.py index 6e02e3a6b98d..a895e218e4d5 100644 --- a/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.py +++ b/erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.py @@ -17,8 +17,10 @@ class RepostAccountingLedgerItems(Document): parent: DF.Data parentfield: DF.Data parenttype: DF.Data - voucher_no: DF.DynamicLink | None - voucher_type: DF.Link | None + status: DF.Literal["Pending", "Reposted", "Skipped", "Failed"] + traceback: DF.Code | None + voucher_no: DF.DynamicLink + voucher_type: DF.Link # end: auto-generated types pass diff --git a/erpnext/accounts/report/tax_withholding_details/tax_withholding_details.py b/erpnext/accounts/report/tax_withholding_details/tax_withholding_details.py index 99ac592097b7..72f80b62dd1c 100644 --- a/erpnext/accounts/report/tax_withholding_details/tax_withholding_details.py +++ b/erpnext/accounts/report/tax_withholding_details/tax_withholding_details.py @@ -7,6 +7,7 @@ from frappe.utils import flt, getdate from pypika import Tuple +from erpnext.accounts.report.utils import validate_mandatory_date_range from erpnext.accounts.utils import get_currency_precision @@ -33,9 +34,7 @@ def execute(filters=None): def validate_filters(filters): """Validate if dates are properly set""" - filters = frappe._dict(filters or {}) - if filters.from_date > filters.to_date: - frappe.throw(_("From Date must be before To Date")) + validate_mandatory_date_range(filters or {}) def get_result(filters, tds_accounts, tax_category_map, net_total_map): diff --git a/erpnext/accounts/report/tds_computation_summary/tds_computation_summary.py b/erpnext/accounts/report/tds_computation_summary/tds_computation_summary.py index a1b9c22f63f7..5e7c5d46a988 100644 --- a/erpnext/accounts/report/tds_computation_summary/tds_computation_summary.py +++ b/erpnext/accounts/report/tds_computation_summary/tds_computation_summary.py @@ -5,6 +5,7 @@ get_result, get_tds_docs, ) +from erpnext.accounts.report.utils import validate_mandatory_date_range from erpnext.accounts.utils import get_fiscal_year @@ -33,8 +34,7 @@ def execute(filters=None): def validate_filters(filters): """Validate if dates are properly set and lie in the same fiscal year""" - if filters.from_date > filters.to_date: - frappe.throw(_("From Date must be before To Date")) + validate_mandatory_date_range(filters) from_year = get_fiscal_year(filters.from_date)[0] to_year = get_fiscal_year(filters.to_date)[0] diff --git a/erpnext/accounts/report/utils.py b/erpnext/accounts/report/utils.py index 8d1730ab294e..3661e787f41f 100644 --- a/erpnext/accounts/report/utils.py +++ b/erpnext/accounts/report/utils.py @@ -1,4 +1,5 @@ import frappe +from frappe import _ from frappe.query_builder.custom import ConstantColumn from frappe.query_builder.functions import Sum from frappe.utils import flt, formatdate, get_datetime_str, get_table_name @@ -16,6 +17,19 @@ __exchange_rates = {} +def validate_mandatory_date_range(filters, from_field="from_date", to_field="to_date"): + from_date = filters.get(from_field) + to_date = filters.get(to_field) + + if not from_date or not to_date: + frappe.throw( + _("{0} and {1} are mandatory").format(frappe.bold(_("From Date")), frappe.bold(_("To Date"))) + ) + + if from_date > to_date: + frappe.throw(_("From Date must be before To Date")) + + def get_currency(filters): """ Returns a dictionary containing currency information. The keys of the dict are diff --git a/erpnext/patches.txt b/erpnext/patches.txt index 02103c8071a4..022b33cac10e 100644 --- a/erpnext/patches.txt +++ b/erpnext/patches.txt @@ -445,3 +445,4 @@ erpnext.patches.v16_0.backfill_pick_list_transferred_qty erpnext.patches.v16_0.access_control_for_project_users erpnext.patches.v16_0.rename_ar_ap_ageing_filter erpnext.patches.v15_0.fix_titles +erpnext.patches.v16_0.backfill_repost_accounting_ledger_status diff --git a/erpnext/patches/v16_0/backfill_repost_accounting_ledger_status.py b/erpnext/patches/v16_0/backfill_repost_accounting_ledger_status.py new file mode 100644 index 000000000000..f5a240a506c0 --- /dev/null +++ b/erpnext/patches/v16_0/backfill_repost_accounting_ledger_status.py @@ -0,0 +1,25 @@ +import frappe +from frappe.query_builder.functions import Coalesce + + +def execute(): + """Backfill the statuses of documents reposted before those fields existed. + + Without it they show up as drafts and are offered a `Start Reposting` button that would + repost vouchers which are already reposted. + """ + ral = frappe.qb.DocType("Repost Accounting Ledger") + items = frappe.qb.DocType("Repost Accounting Ledger Items") + + reposted = ( + frappe.qb.from_(ral).select(ral.name).where((ral.docstatus == 1) & (Coalesce(ral.status, "") == "")) + ) + frappe.qb.update(items).set(items.status, "Reposted").where(items.parent.isin(reposted)).run() + + for docstatus, status in ((1, "Completed"), (2, "Cancelled")): + ( + frappe.qb.update(ral) + .set(ral.status, status) + .where((ral.docstatus == docstatus) & (Coalesce(ral.status, "") == "")) + .run() + ) diff --git a/erpnext/regional/italy/utils.py b/erpnext/regional/italy/utils.py index d996ff9b2acb..8e2166448572 100644 --- a/erpnext/regional/italy/utils.py +++ b/erpnext/regional/italy/utils.py @@ -238,7 +238,7 @@ def get_invoice_summary(items, taxes): # Preflight for successful e-invoice export. def sales_invoice_validate(doc): # Validate company - if doc.doctype != "Sales Invoice": + if doc.doctype != "Sales Invoice" or doc.is_opening == "Yes": return if not doc.company_address: @@ -322,7 +322,7 @@ def sales_invoice_validate(doc): # Ensure payment details are valid for e-invoice. def sales_invoice_on_submit(doc, method): # Validate payment details - if get_company_country(doc.company) not in [ + if doc.is_opening == "Yes" or get_company_country(doc.company) not in [ "Italy", "Italia", "Italian Republic", @@ -388,7 +388,7 @@ def generate_single_invoice(docname): # Delete e-invoice attachment on cancel. def sales_invoice_on_cancel(doc, method): - if get_company_country(doc.company) not in [ + if doc.is_opening == "Yes" or get_company_country(doc.company) not in [ "Italy", "Italia", "Italian Republic", diff --git a/erpnext/stock/doctype/stock_entry/stock_entry.py b/erpnext/stock/doctype/stock_entry/stock_entry.py index d601dc093b82..f93c41bacc5a 100644 --- a/erpnext/stock/doctype/stock_entry/stock_entry.py +++ b/erpnext/stock/doctype/stock_entry/stock_entry.py @@ -1321,6 +1321,7 @@ def set_basic_rate(self, reset_outgoing_rate=True, raise_error_if_no_rate=True): # Set rate for outgoing items outgoing_items_cost = self.set_rate_for_outgoing_items(reset_outgoing_rate, raise_error_if_no_rate) finished_item_qty = sum(d.transfer_qty for d in self.items if d.is_finished_item) + has_consumption_basis = self.has_consumption_basis() items = [] # Set basic rate for incoming items @@ -1328,6 +1329,8 @@ def set_basic_rate(self, reset_outgoing_rate=True, raise_error_if_no_rate=True): if d.s_warehouse or d.set_basic_rate_manually: continue + rate_derived_from_consumption = False + if d.allow_zero_valuation_rate: d.basic_rate = 0.0 items.append(d.item_code) @@ -1335,12 +1338,17 @@ def set_basic_rate(self, reset_outgoing_rate=True, raise_error_if_no_rate=True): elif d.is_finished_item: if self.purpose == "Manufacture": d.basic_rate = self.get_basic_rate_for_manufactured_item( - finished_item_qty, outgoing_items_cost + finished_item_qty, outgoing_items_cost, has_consumption_basis ) + rate_derived_from_consumption = has_consumption_basis elif self.purpose == "Repack": d.basic_rate = self.get_basic_rate_for_repacked_items(d.transfer_qty, outgoing_items_cost) + # Repack rate comes from consumed source-warehouse rows, not consumption entries + rate_derived_from_consumption = any(item.s_warehouse for item in self.get("items")) - if not d.basic_rate and not d.allow_zero_valuation_rate: + # A rate of zero derived from the consumed items is their actual cost, not a missing + # rate. Falling back to the item's valuation here would value free inputs as output. + if not d.basic_rate and not d.allow_zero_valuation_rate and not rate_derived_from_consumption: if self.is_new(): raise_error_if_no_rate = False @@ -1375,6 +1383,31 @@ def set_basic_rate(self, reset_outgoing_rate=True, raise_error_if_no_rate=True): frappe.msgprint(message, alert=True) + def has_consumption_basis(self) -> bool: + """Whether the cost of the consumed items is known, even when that cost is zero.""" + if any(d.s_warehouse for d in self.get("items")): + return True + + settings = frappe.get_single("Manufacturing Settings") + if settings.material_consumption and settings.get_rm_cost_from_consumption_entry and self.work_order: + return bool(self.get_consumption_entries()) + + return False + + def get_consumption_entries(self) -> list[str]: + # Cached: queried in both has_consumption_basis() and get_basic_rate_for_manufactured_item() + if getattr(self, "_consumption_entries", None) is None: + self._consumption_entries = frappe.get_all( + "Stock Entry", + filters={ + "docstatus": 1, + "work_order": self.work_order, + "purpose": "Material Consumption for Manufacture", + }, + pluck="name", + ) + return self._consumption_entries + def set_rate_for_outgoing_items(self, reset_outgoing_rate=True, raise_error_if_no_rate=True): outgoing_items_cost = 0.0 for d in self.get("items"): @@ -1428,21 +1461,16 @@ def get_basic_rate_for_repacked_items(self, finished_item_qty, outgoing_items_co ) return flt(outgoing_items_cost / total_fg_qty) - def get_basic_rate_for_manufactured_item(self, finished_item_qty, outgoing_items_cost=0) -> float: + def get_basic_rate_for_manufactured_item( + self, finished_item_qty, outgoing_items_cost=0, has_consumption_basis=False + ) -> float: settings = frappe.get_single("Manufacturing Settings") scrap_items_cost = sum([flt(d.basic_amount) for d in self.get("items") if d.is_scrap_item]) if settings.material_consumption: if settings.get_rm_cost_from_consumption_entry and self.work_order: # Validate only if Material Consumption Entry exists for the Work Order. - if frappe.db.exists( - "Stock Entry", - { - "docstatus": 1, - "work_order": self.work_order, - "purpose": "Material Consumption for Manufacture", - }, - ): + if self.get_consumption_entries(): for item in self.items: if not item.is_finished_item and not item.is_scrap_item: label = frappe.get_meta(settings.doctype).get_label( @@ -1489,7 +1517,9 @@ def get_basic_rate_for_manufactured_item(self, finished_item_qty, outgoing_items ) ).run()[0][0] or 0 - elif not outgoing_items_cost: + # Estimate from the BOM only when nothing was consumed. A consumed cost of zero is a + # real cost, so substituting BOM rates would value free inputs as output. + elif not outgoing_items_cost and not has_consumption_basis: bom_items = self.get_bom_raw_materials(finished_item_qty) outgoing_items_cost = sum([flt(row.qty) * flt(row.rate) for row in bom_items.values()]) diff --git a/erpnext/stock/doctype/stock_entry/test_stock_entry.py b/erpnext/stock/doctype/stock_entry/test_stock_entry.py index b344b772cbb7..bcaa90e104f5 100644 --- a/erpnext/stock/doctype/stock_entry/test_stock_entry.py +++ b/erpnext/stock/doctype/stock_entry/test_stock_entry.py @@ -2574,6 +2574,149 @@ def test_transferred_qty_in_material_transfer(self): material_request.reload() self.assertEqual(material_request.transfer_status, "Completed") + def test_manufacture_with_zero_valued_raw_material(self): + # A finished good produced from free inputs is worth nothing. Falling back to the item's + # own valuation would create value out of nothing and inflate it on every production run. + fg_item = make_item(properties={"is_stock_item": 1}).name + rm_item = make_item(properties={"is_stock_item": 1}).name + warehouse = "_Test Warehouse - _TC" + fg_warehouse = "Finished Goods - _TC" + + rm_receipt = make_stock_entry(item_code=rm_item, target=warehouse, qty=100, rate=0, do_not_save=True) + rm_receipt.items[0].allow_zero_valuation_rate = 1 + rm_receipt.save() + rm_receipt.submit() + + # the finished good already carries a valuation in the target warehouse + make_stock_entry(item_code=fg_item, target=fg_warehouse, qty=10, rate=100) + + se = frappe.new_doc("Stock Entry") + se.purpose = se.stock_entry_type = "Manufacture" + se.company = "_Test Company" + se.append( + "items", + {"item_code": rm_item, "s_warehouse": warehouse, "qty": 10, "conversion_factor": 1}, + ) + se.append( + "items", + { + "item_code": fg_item, + "t_warehouse": fg_warehouse, + "qty": 10, + "is_finished_item": 1, + "conversion_factor": 1, + }, + ) + se.save() + + self.assertEqual(se.items[0].basic_amount, 0) + self.assertEqual(se.items[1].basic_rate, 0) + self.assertEqual(se.items[1].basic_amount, 0) + + se.submit() + + fg_sle = frappe.db.get_value( + "Stock Ledger Entry", + {"voucher_no": se.name, "item_code": fg_item, "is_cancelled": 0}, + ["incoming_rate", "stock_value_difference"], + as_dict=True, + ) + + self.assertEqual(fg_sle.incoming_rate, 0) + self.assertEqual(fg_sle.stock_value_difference, 0) + + def _make_wo_for_free_raw_material(self, rm_item, fg_item, bom_no): + from erpnext.manufacturing.doctype.work_order.test_work_order import make_wo_order_test_record + from erpnext.manufacturing.doctype.work_order.work_order import ( + make_stock_entry as make_stock_entry_from_wo, + ) + + receipt = make_stock_entry(item_code=rm_item, target="Stores - _TC", qty=10, rate=0, do_not_save=True) + receipt.items[0].allow_zero_valuation_rate = 1 + receipt.save() + receipt.submit() + + wo = make_wo_order_test_record(production_item=fg_item, bom_no=bom_no, qty=10) + + transfer = frappe.get_doc(make_stock_entry_from_wo(wo.name, "Material Transfer for Manufacture", 10)) + transfer.items[0].s_warehouse = "Stores - _TC" + transfer.insert().submit() + + return wo + + @change_settings( + "Manufacturing Settings", {"material_consumption": 1, "get_rm_cost_from_consumption_entry": 0} + ) + def test_manufacture_does_not_fall_back_to_bom_cost_for_free_raw_material(self): + # The BOM is only an estimate for when nothing was consumed. Items that were consumed and + # cost nothing are a real cost, so a BOM rate must not stand in for them. + from erpnext.manufacturing.doctype.production_plan.test_production_plan import make_bom + from erpnext.manufacturing.doctype.work_order.work_order import ( + make_stock_entry as make_stock_entry_from_wo, + ) + + rm_item = make_item(properties={"is_stock_item": 1}).name + fg_item = make_item(properties={"is_stock_item": 1}).name + + frappe.get_doc( + { + "doctype": "Item Price", + "item_code": rm_item, + "price_list": "_Test Price List India", + "price_list_rate": 150, + "buying": 1, + } + ).insert() + + # price the BOM off the price list so that it carries a rate the free stock does not + bom = make_bom(item=fg_item, raw_materials=[rm_item], do_not_save=True) + bom.rm_cost_as_per = "Price List" + bom.buying_price_list = "_Test Price List India" + bom.currency = "INR" + bom.save() + bom.submit() + + wo = self._make_wo_for_free_raw_material(rm_item, fg_item, bom.name) + + manufacture = frappe.get_doc(make_stock_entry_from_wo(wo.name, "Manufacture", 10)) + manufacture.save() + + fg_row = next(d for d in manufacture.items if d.is_finished_item) + self.assertEqual(fg_row.basic_rate, 0) + self.assertEqual(fg_row.basic_amount, 0) + + @change_settings( + "Manufacturing Settings", {"material_consumption": 1, "get_rm_cost_from_consumption_entry": 1} + ) + def test_manufacture_with_zero_valued_consumption_entry(self): + # The raw material is consumed by a separate entry, so the Manufacture entry carries no + # consumed rows of its own. Its cost is still known, and it is zero. + from erpnext.manufacturing.doctype.production_plan.test_production_plan import make_bom + from erpnext.manufacturing.doctype.work_order.work_order import ( + make_stock_entry as make_stock_entry_from_wo, + ) + + rm_item = make_item(properties={"is_stock_item": 1}).name + fg_item = make_item(properties={"is_stock_item": 1}).name + + # the finished good already carries a valuation in the work order's target warehouse + make_stock_entry(item_code=fg_item, target="_Test Warehouse 1 - _TC", qty=10, rate=100) + + bom = make_bom(item=fg_item, raw_materials=[rm_item]).name + wo = self._make_wo_for_free_raw_material(rm_item, fg_item, bom) + + consumption = frappe.get_doc( + make_stock_entry_from_wo(wo.name, "Material Consumption for Manufacture", 10) + ) + consumption.insert().submit() + + manufacture = frappe.get_doc(make_stock_entry_from_wo(wo.name, "Manufacture", 10)) + manufacture.save() + + fg_row = next(d for d in manufacture.items if d.is_finished_item) + self.assertEqual(fg_row.basic_rate, 0) + self.assertEqual(fg_row.basic_amount, 0) + def test_disassemble_entry_without_wo(self): from erpnext.manufacturing.doctype.production_plan.test_production_plan import make_bom diff --git a/erpnext/stock/report/batch_wise_balance_history/batch_wise_balance_history.py b/erpnext/stock/report/batch_wise_balance_history/batch_wise_balance_history.py index e5cb69ff8162..a50061de1e3e 100644 --- a/erpnext/stock/report/batch_wise_balance_history/batch_wise_balance_history.py +++ b/erpnext/stock/report/batch_wise_balance_history/batch_wise_balance_history.py @@ -8,6 +8,7 @@ from frappe.utils.deprecations import deprecated from pypika import functions as fn +from erpnext.accounts.report.utils import validate_mandatory_date_range from erpnext.stock.doctype.warehouse.warehouse import apply_warehouse_filter SLE_COUNT_LIMIT = 100_000 @@ -29,8 +30,7 @@ def execute(filters=None): _("Please select either the Item or Warehouse or Warehouse Type filter to generate the report.") ) - if filters.from_date > filters.to_date: - frappe.throw(_("From Date must be before To Date")) + validate_mandatory_date_range(filters) float_precision = cint(frappe.db.get_default("float_precision")) or 3 diff --git a/erpnext/stock/report/cogs_by_item_group/cogs_by_item_group.py b/erpnext/stock/report/cogs_by_item_group/cogs_by_item_group.py index 000aca9f43e0..afae69c6ce01 100644 --- a/erpnext/stock/report/cogs_by_item_group/cogs_by_item_group.py +++ b/erpnext/stock/report/cogs_by_item_group/cogs_by_item_group.py @@ -9,6 +9,7 @@ from frappe.utils import date_diff from erpnext.accounts.report.general_ledger.general_ledger import get_gl_entries +from erpnext.accounts.report.utils import validate_mandatory_date_range Filters = frappe._dict Row = frappe._dict @@ -34,8 +35,7 @@ def update_filters_with_account(filters: Filters) -> None: def validate_filters(filters: Filters) -> None: - if filters.from_date > filters.to_date: - frappe.throw(_("From Date must be before To Date")) + validate_mandatory_date_range(filters) def get_columns() -> Columns: