From 4bc3420b21710d831ec0bab0c42021f7075f95e9 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Fri, 19 Jun 2026 10:02:59 +0530 Subject: [PATCH] refactor(postgres): address Greptile review on Assets conversions - cancel_movement_entries: filter the parent Asset Movement's docstatus via a qb join (as the original SQL did), instead of the child Asset Movement Item.docstatus. Behaviour is identical in normal flows (child docstatus is synced) but this is exactly faithful. - get_maintenance_log: add a both-engine test for this previously-untested whitelisted endpoint. Confirms the frappe v16 dict aggregate field spec ({"COUNT": ...}) runs and returns correct per-status counts (no runtime crash). Co-Authored-By: Claude Opus 4.8 (1M context) --- erpnext/assets/doctype/asset/asset.py | 14 ++++++--- .../test_asset_maintenance.py | 30 +++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/erpnext/assets/doctype/asset/asset.py b/erpnext/assets/doctype/asset/asset.py index e9d3b38a667..1f0bee0f021 100644 --- a/erpnext/assets/doctype/asset/asset.py +++ b/erpnext/assets/doctype/asset/asset.py @@ -735,10 +735,16 @@ class Asset(AccountsController): frappe.throw(_("Asset cannot be cancelled, as it is already {0}").format(self.status)) def cancel_movement_entries(self): - movements = frappe.get_all( - "Asset Movement Item", - filters={"asset": self.name, "docstatus": 1}, - fields=["parent as name"], + # filter the parent Asset Movement's docstatus (as the original SQL did), not the child row's + asm = frappe.qb.DocType("Asset Movement") + asm_item = frappe.qb.DocType("Asset Movement Item") + movements = ( + frappe.qb.from_(asm_item) + .inner_join(asm) + .on(asm_item.parent == asm.name) + .select(asm.name) + .where((asm_item.asset == self.name) & (asm.docstatus == 1)) + .run(as_dict=True) ) for movement in movements: diff --git a/erpnext/assets/doctype/asset_maintenance/test_asset_maintenance.py b/erpnext/assets/doctype/asset_maintenance/test_asset_maintenance.py index 068c6cff9c9..7411349b029 100644 --- a/erpnext/assets/doctype/asset_maintenance/test_asset_maintenance.py +++ b/erpnext/assets/doctype/asset_maintenance/test_asset_maintenance.py @@ -18,6 +18,36 @@ class TestAssetMaintenance(ERPNextTestSuite): self.asset_name = frappe.db.get_value("Asset", {"purchase_receipt": self.pr.name}, "name") self.asset_doc = frappe.get_doc("Asset", self.asset_name) + def test_get_maintenance_log_counts_by_status(self): + """get_maintenance_log uses a v16 dict aggregate field spec + ({"COUNT": "asset_name", "as": "count"}); confirm it runs and returns correct per-status counts + on both engines (the whitelisted endpoint was previously untested).""" + from erpnext.assets.doctype.asset_maintenance.asset_maintenance import get_maintenance_log + + self.asset_doc.available_for_use_date = nowdate() + self.asset_doc.purchase_date = nowdate() + self.asset_doc.save() + + frappe.get_doc( + { + "doctype": "Asset Maintenance", + "asset_name": self.asset_name, + "maintenance_team": "Team Awesome", + "company": "_Test Company", + "asset_maintenance_tasks": get_maintenance_tasks(), + } + ).insert() + + rows = get_maintenance_log(self.asset_name) + # the dict aggregate spec did not crash and returned grouped rows... + self.assertTrue(rows) + self.assertTrue(all("maintenance_status" in r for r in rows)) + # ...and the per-status counts sum to the total number of logs for this asset + self.assertEqual( + sum(r["count"] for r in rows), + frappe.db.count("Asset Maintenance Log", {"asset_name": self.asset_name}), + ) + def test_create_asset_maintenance_with_log(self): month_end_date = get_last_day(nowdate())