From f78683c14b4e226c659a6bf2793c2ab7f3b539d3 Mon Sep 17 00:00:00 2001 From: Bibin <17405044+bibinqcs@users.noreply.github.com> Date: Sun, 21 Jun 2026 15:33:26 +0000 Subject: [PATCH] fix(UAE Regional): address greptile review findings - Gate generate_faf() and mark_as_submitted() on write permission so REST callers without write access can no longer trigger state changes via the whitelisted endpoints. - Drop test_generate_faf_excise_not_yet_implemented; the Excise file type is no longer a valid Select option, so doc.insert() now fails before generate_faf() is reached. - Stream GL Entry rows in pages of GL_PAGE_SIZE to bound memory on multi-year exports against large companies; running balance, account-name cache, and totals carry across batches so output is byte-identical to the single-fetch implementation. - Move the VAT 201 helper cache from a module-level dict to frappe.local so concurrent requests on threaded workers no longer race or leak data across users. --- .../doctype/fta_audit_file/fta_audit_file.py | 146 +++++++++++------- .../fta_audit_file/test_fta_audit_file.py | 17 -- .../report/uae_vat_201/uae_vat_201.py | 26 +++- 3 files changed, 109 insertions(+), 80 deletions(-) diff --git a/erpnext/regional/doctype/fta_audit_file/fta_audit_file.py b/erpnext/regional/doctype/fta_audit_file/fta_audit_file.py index 366705c3079..e228b67d00f 100644 --- a/erpnext/regional/doctype/fta_audit_file/fta_audit_file.py +++ b/erpnext/regional/doctype/fta_audit_file/fta_audit_file.py @@ -37,6 +37,12 @@ DEFAULT_COUNTRY = "United Arab Emirates" DEFAULT_DATE = "31-12-9999" PRODUCT_VERSION = "ERPNext" +# Number of GL Entry rows read into memory per batch when streaming the +# General Ledger section. Large UAE companies routinely produce hundreds +# of thousands of GL entries per year; reading them all into a single list +# would push the background worker over its memory limit. +GL_PAGE_SIZE = 5000 + # Inputs that define what the FAF file represents. Once the file has been # Generated or Submitted, changing any of these would silently desync the # attached CSV from the form, so we lock them. @@ -105,6 +111,11 @@ class FTAAuditFile(Document): Under ``frappe.flags.in_test`` the job runs synchronously so tests can assert on the post-generation state without polling. """ + # `@frappe.whitelist()` gates network access but not document write + # permission; without this explicit check, a user with read-only + # access could still trigger generation via REST. + self.check_permission("write") + # Re-read status from DB so two concurrent button clicks can't both # enqueue a job — the second one sees Queued/Generating and bails. current_status = frappe.db.get_value(self.doctype, self.name, "status", for_update=True) @@ -138,6 +149,7 @@ class FTAAuditFile(Document): @frappe.whitelist() def mark_as_submitted(self): """Mark the FAF as submitted to the FTA portal (manual record).""" + self.check_permission("write") if self.status != "Generated": frappe.throw(_("Only Generated files can be marked as Submitted")) self.status = "Submitted" @@ -481,42 +493,34 @@ class FTAAuditFile(Document): return line_count def _write_gl_listing(self, writer): - """Emit General Ledger per Appendix 5 with end-of-table totals row.""" + """Emit General Ledger per Appendix 5 with end-of-table totals row. + + GL Entry rows are streamed in pages of ``GL_PAGE_SIZE`` to keep + memory bounded for multi-year exports on large companies; the + running-balance, account-name, and totals state survives across + pages so the output is identical to a single-fetch implementation. + """ writer.writerow(["GLDataStart"]) company_currency = _company_currency(self.company) + base_filters = { + "company": self.company, + "posting_date": ["between", [self.from_date, self.to_date]], + "is_cancelled": 0, + } - entries = frappe.get_all( - "GL Entry", - filters={ - "company": self.company, - "posting_date": ["between", [self.from_date, self.to_date]], - "is_cancelled": 0, - }, - fields=[ - "name", - "posting_date", - "account", - "remarks", - "against", - "voucher_no", - "voucher_type", - "debit", - "credit", - ], - order_by="posting_date asc, creation asc", - ) - if not entries: - writer.writerow(["GLDataEnd", _money(0), _money(0), 0, company_currency]) - return 0 - - account_names = list({e.account for e in entries if e.account}) - account_name_map = _bulk_party_field("Account", account_names, "account_name") - + # Opening balances need every account that posts in the period up + # front; without that flag we cache account names lazily as we + # encounter them in each batch. if self.include_opening_balance: - running_balance = _opening_balances_by_account(self.company, self.from_date, account_names) + accounts_in_period = frappe.get_all( + "GL Entry", filters=base_filters, pluck="account", distinct=True + ) + running_balance = _opening_balances_by_account(self.company, self.from_date, accounts_in_period) + account_name_map = _bulk_party_field("Account", accounts_in_period, "account_name") else: running_balance = {} + account_name_map = {} source_type_map = { "Sales Invoice": "AR", @@ -531,34 +535,66 @@ class FTAAuditFile(Document): total_debit = 0.0 total_credit = 0.0 count = 0 + start = 0 - for entry in entries: - account_name = account_name_map.get(entry.account) or entry.account - source_type = source_type_map.get(entry.voucher_type, entry.voucher_type or "") - debit = flt(entry.debit, 2) - credit = flt(entry.credit, 2) - - running_balance[entry.account] = running_balance.get(entry.account, 0.0) + debit - credit - balance = flt(running_balance[entry.account], 2) - - writer.writerow( - [ - _format_date(entry.posting_date), - entry.account, - _clean(account_name), - _clean(entry.remarks or ""), - _clean(entry.against or ""), - entry.voucher_no, - entry.voucher_no, - source_type, - _money(debit), - _money(credit), - _money(balance), - ] + while True: + batch = frappe.get_all( + "GL Entry", + filters=base_filters, + fields=[ + "name", + "posting_date", + "account", + "remarks", + "against", + "voucher_no", + "voucher_type", + "debit", + "credit", + ], + order_by="posting_date asc, creation asc", + limit_start=start, + limit_page_length=GL_PAGE_SIZE, ) - total_debit += debit - total_credit += credit - count += 1 + if not batch: + break + + # Backfill the account-name cache for accounts new to this batch. + new_accounts = [e.account for e in batch if e.account and e.account not in account_name_map] + if new_accounts: + account_name_map.update(_bulk_party_field("Account", new_accounts, "account_name")) + + for entry in batch: + account_name = account_name_map.get(entry.account) or entry.account + source_type = source_type_map.get(entry.voucher_type, entry.voucher_type or "") + debit = flt(entry.debit, 2) + credit = flt(entry.credit, 2) + + running_balance[entry.account] = running_balance.get(entry.account, 0.0) + debit - credit + balance = flt(running_balance[entry.account], 2) + + writer.writerow( + [ + _format_date(entry.posting_date), + entry.account, + _clean(account_name), + _clean(entry.remarks or ""), + _clean(entry.against or ""), + entry.voucher_no, + entry.voucher_no, + source_type, + _money(debit), + _money(credit), + _money(balance), + ] + ) + total_debit += debit + total_credit += credit + count += 1 + + if len(batch) < GL_PAGE_SIZE: + break + start += GL_PAGE_SIZE writer.writerow( [ diff --git a/erpnext/regional/doctype/fta_audit_file/test_fta_audit_file.py b/erpnext/regional/doctype/fta_audit_file/test_fta_audit_file.py index 22dba1e02d5..2754c53cbba 100644 --- a/erpnext/regional/doctype/fta_audit_file/test_fta_audit_file.py +++ b/erpnext/regional/doctype/fta_audit_file/test_fta_audit_file.py @@ -213,23 +213,6 @@ class TestFTAAuditFile(FrappeTestCase): self.assertNotIn("SuppDataEnd,0.0,", csv_content) self.assertNotIn("GLDataEnd,0.0,", csv_content) - def test_generate_faf_excise_not_yet_implemented(self): - """Excise FAF (Appendix 6) should error cleanly until implemented.""" - doc = frappe.get_doc( - { - "doctype": "FTA Audit File", - "company": self.company, - "from_date": "2099-03-01", - "to_date": "2099-03-31", - "file_type": "Excise", - } - ) - doc.insert() - - self.assertRaises(frappe.ValidationError, doc.generate_faf) - doc.reload() - self.assertEqual(doc.status, "Error") - def test_mark_as_submitted_workflow(self): """Generated docs can be marked submitted; non-Generated cannot.""" doc = frappe.get_doc( diff --git a/erpnext/regional/report/uae_vat_201/uae_vat_201.py b/erpnext/regional/report/uae_vat_201/uae_vat_201.py index e795e7709e9..e7979283f0f 100644 --- a/erpnext/regional/report/uae_vat_201/uae_vat_201.py +++ b/erpnext/regional/report/uae_vat_201/uae_vat_201.py @@ -12,10 +12,19 @@ from frappe.utils import flt from erpnext import get_region -# Per-execution memoization cache for the helper functions below. -# Cleared at the start of every execute() call so each report run gets -# fresh data; within a single run, repeated calls reuse the result. -_cache = {} +# Per-request memoization cache for the helper functions below. Stored on +# ``frappe.local`` so concurrent requests under gevent/threaded workers +# never share or race on this state; cleared at the start of every +# ``execute()`` so each report run gets fresh data. +_CACHE_ATTR = "_uae_vat_201_cache" + + +def _get_cache(): + cache = getattr(frappe.local, _CACHE_ATTR, None) + if cache is None: + cache = {} + setattr(frappe.local, _CACHE_ATTR, cache) + return cache def _drill_down_link(text, filters, **extra): @@ -39,10 +48,11 @@ def _drill_down_link(text, filters, **extra): def _cached(fn): def wrapper(filters, *args, **kwargs): + cache = _get_cache() key = (fn.__name__, tuple(sorted((filters or {}).items()))) - if key not in _cache: - _cache[key] = fn(filters, *args, **kwargs) - return _cache[key] + if key not in cache: + cache[key] = fn(filters, *args, **kwargs) + return cache[key] return wrapper @@ -50,7 +60,7 @@ def _cached(fn): def execute(filters=None): filters = filters or {} validate_company_region(filters) - _cache.clear() + _get_cache().clear() columns = get_columns() data = get_data(filters) return columns, data