From 8b93d2582482035745cfdfac1b02882d3f6ac1fe Mon Sep 17 00:00:00 2001 From: shariquerik Date: Wed, 2 Sep 2026 19:32:06 +0530 Subject: [PATCH] test: assert the shipped rows, not what the resolver made of them Greptile P2, and right: the fixture checks read `resolve_navigation`, which has already dropped a row naming a missing doctype, a heading over nothing and a linked item whose sidebar emptied. Against that output they were asserting that the resolver works, which is frappe's test. They now read the standard `Rail` and `Sidebar` rows as authored. Checked that each can fail: an orphan `parent_key` and an empty section both insert happily. A missing doctype does not -- `link_to` is a Dynamic Link and `_validate_links` refuses the row at import -- so that test now says what it is actually for, a doctype removed after the rows were imported. Adds one: every `parent_key` names a section beside it. An orphan is promoted to the top level rather than dropped, so it loses its nesting in silence. Co-Authored-By: Claude Opus 5 (1M context) --- erpnext/tests/test_navigation.py | 100 +++++++++++++++++++++---------- 1 file changed, 69 insertions(+), 31 deletions(-) diff --git a/erpnext/tests/test_navigation.py b/erpnext/tests/test_navigation.py index d9208718461..0d87a2e2ce0 100644 --- a/erpnext/tests/test_navigation.py +++ b/erpnext/tests/test_navigation.py @@ -108,17 +108,18 @@ class TestERPNextNavigation(IntegrationTestCase): def test_a_linked_item_names_a_sidebar_this_app_ships(self): """A typo in `link_to` costs the item its panel and nothing else, so nothing reports it. - `Sidebar` declares the `Derived From Children` permission rule, so an item naming an - address that resolves to no rows is dropped by the server's own cascade -- silently, and - correctly, because that is also what an emptied sidebar has to do. + Read off the **shipped rows**, not the resolved payload. `Sidebar` declares the + `Derived From Children` permission rule, so an item naming an address that resolves to no + rows is dropped by the server's own cascade -- silently, and correctly, because that is + also what an emptied sidebar has to do. Asserting against the resolved list would + therefore be asserting that the cascade works, which is frappe's test and not this one. """ - frappe.set_user("Administrator") - navigation = resolve_navigation(APP) - linked = [item for item in navigation["rail"] if item["item_type"] == "Sidebar"] + linked = [row for row in shipped("Rail", "erpnext") if row.item_type == "Sidebar"] + addresses = set(frappe.get_all("Sidebar", filters={"app": APP, "standard": 1}, pluck="name")) - self.assertEqual(tuple(item["key"] for item in linked), LINKED) - for item in linked: - self.assertIn(item["link_to"], navigation["sidebars"]) + self.assertEqual(tuple(row.key for row in linked), LINKED) + for row in linked: + self.assertIn(row.link_to, addresses, row.key) def test_an_independent_item_is_a_module_and_opens_nothing(self): """Charter point 1 makes independent a first-class state, and needs no field to say so: @@ -142,35 +143,45 @@ class TestERPNextNavigation(IntegrationTestCase): self.assertEqual(name, frappe.scrub(f"Module Def {module}")) def test_every_destination_is_a_doctype_this_site_has(self): - """A row naming a doctype that is gone is dropped by the filter, not refused at import.""" - frappe.set_user("Administrator") - navigation = resolve_navigation(APP) - rows = [*navigation["rail"], *(row for rows in navigation["sidebars"].values() for row in rows)] + """Read off the shipped rows rather than the resolved payload, which has already dropped + any row it could not place. - for row in rows: - if row["item_type"] == "DocType": - self.assertTrue(frappe.db.exists("DocType", row["link_to"]), row["key"]) + A typo cannot get this far: `link_to` is a Dynamic Link, so `_validate_links` refuses the + row at import with `Could not find Row #N: Link To` -- checked, not assumed. What is left + for this to catch is a doctype removed *after* the rows were imported, which arrives as a + quietly shorter sidebar and nothing else. + """ + for container, address in every_container(): + for row in shipped(container, address): + if row.item_type == "DocType": + self.assertTrue(frappe.db.exists("DocType", row.link_to), f"{address}/{row.key}") def test_keys_are_unique_within_each_container(self): """Every site and user edit is filed against a key, so two rows sharing one collide.""" - frappe.set_user("Administrator") - navigation = resolve_navigation(APP) - - for rows in (navigation["rail"], *navigation["sidebars"].values()): - keys = [row["key"] for row in rows] - self.assertEqual(len(keys), len(set(keys))) + for container, address in every_container(): + keys = [row.key for row in shipped(container, address)] + self.assertEqual(len(keys), len(set(keys)), address) def test_no_section_is_shipped_over_nothing(self): - """A heading with no rows under it is dropped by the cascade at read time, so shipping - one would mean shipping a row that can never render. They are left out at authoring.""" - frappe.set_user("Administrator") - navigation = resolve_navigation(APP) - - for rows in navigation["sidebars"].values(): - parents = {row.get("parent_key") for row in rows} + """A heading with no rows under it is dropped by the cascade at read time, so shipping one + would mean shipping a row that can never render. Left out at authoring instead -- and read + here off the shipped rows, since the cascade would have hidden it either way.""" + for container, address in every_container(): + rows = shipped(container, address) + parents = {row.parent_key for row in rows} for row in rows: - if row["item_type"] == "Section": - self.assertIn(row["key"], parents, row["key"]) + if row.item_type == "Section": + self.assertIn(row.key, parents, f"{address}/{row.key}") + + def test_every_parent_key_names_a_section_beside_it(self): + """A row filed under a heading that is not there loses its nesting silently: the resolver + promotes an orphan to the top level rather than dropping it.""" + for container, address in every_container(): + rows = shipped(container, address) + sections = {row.key for row in rows if row.item_type == "Section"} + for row in rows: + if row.parent_key: + self.assertIn(row.parent_key, sections, f"{address}/{row.key}") def test_a_real_user_sees_fewer_modules_than_administrator(self): """The filter is doing something, which is the one thing an Administrator suite cannot @@ -220,6 +231,33 @@ class TestERPNextNavigation(IntegrationTestCase): ) +def every_container() -> list[tuple[str, str]]: + """Every standard record ERPNext ships, as `(doctype, name)`.""" + return [("Rail", "erpnext")] + [ + ("Sidebar", name) + for name in frappe.get_all("Sidebar", filters={"app": APP, "standard": 1}, pluck="name") + ] + + +def shipped(container: str, address: str) -> list: + """The rows as authored, before the resolver has filtered or cascaded anything away. + + The distinction matters for every fixture assertion below: `resolve_navigation` drops a row + naming a doctype that is gone, a heading over nothing and a linked item whose sidebar + emptied. Read against its output, a test for those is asserting that the resolver works. + """ + return frappe.get_all( + "Navigation Item", + filters={ + "parenttype": container, + "parent": address, + "parentfield": "items" if container == "Rail" else "navigation_items", + }, + fields=["key", "parent_key", "item_type", "link_doctype", "link_to"], + order_by="idx asc", + ) + + def block(module: str, user: str): """Withdraw a module from one person, the way an administrator does on the User form.