diff --git a/erpnext/hooks.py b/erpnext/hooks.py index a6918a3f50b..1b214979e9f 100644 --- a/erpnext/hooks.py +++ b/erpnext/hooks.py @@ -20,6 +20,18 @@ app_home = "/desk/home" # non-modular prefix has no module route at all. app_modular = True +# The server half of ERPNext's own navigation item kind, keyed by type name. The other two +# halves are files: the `Navigation Item Type` record, which arrives at migrate, and the +# renderer beside it, which arrives at build. Server code is optional for a kind, and this +# one needs it because the type declares the `Custom` permission rule -- the framework +# appends `.can_see` to this path and hands it every item of the kind at once +# (frappe/frappe#42424). +# +# desk v2 only, and harmless on `develop`, where nothing reads the hook. +navigation_item_resolvers = { + "Default Company": "erpnext.setup.navigation_item_type.default_company.default_company" +} + add_to_apps_screen = [ { "name": app_name, diff --git a/erpnext/setup/navigation_item_type/__init__.py b/erpnext/setup/navigation_item_type/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/erpnext/setup/navigation_item_type/default_company/__init__.py b/erpnext/setup/navigation_item_type/default_company/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/erpnext/setup/navigation_item_type/default_company/default_company.json b/erpnext/setup/navigation_item_type/default_company/default_company.json new file mode 100644 index 00000000000..88dba0fdcd4 --- /dev/null +++ b/erpnext/setup/navigation_item_type/default_company/default_company.json @@ -0,0 +1,15 @@ +{ + "creation": "2026-09-03 10:00:00.000000", + "docstatus": 0, + "doctype": "Navigation Item Type", + "idx": 0, + "label": "Default Company", + "modified": "2026-09-03 10:00:00.000000", + "modified_by": "Administrator", + "module": "Setup", + "name": "Default Company", + "owner": "Administrator", + "permission_rule": "Custom", + "target_doctype": "Company", + "type_name": "Default Company" +} diff --git a/erpnext/setup/navigation_item_type/default_company/default_company.py b/erpnext/setup/navigation_item_type/default_company/default_company.py new file mode 100644 index 00000000000..4667103d93b --- /dev/null +++ b/erpnext/setup/navigation_item_type/default_company/default_company.py @@ -0,0 +1,71 @@ +# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +"""The server half of the `Default Company` navigation item kind. + +Optional, and here because this kind needs it: `permission_rule` on the type record is +`Custom`, so the framework hands every item of the kind to `can_see` and takes the list +it returns (`frappe/shell/navigation_filter.py`). The path is named in `hooks.py` under +`navigation_item_resolvers`, keyed by the type name; the framework appends `.can_see`. + +It sits beside the record and the renderer rather than in a navigation module of its +own, because a kind is one contribution and not a scattering across three folders. +""" + +from typing import TYPE_CHECKING + +import frappe + +if TYPE_CHECKING: + from frappe.shell.navigation_filter import NavigationContext + + +def default_company() -> str | None: + """The company this site works in, or None on a site still in the setup wizard. + + The same key the renderer reads out of `boot.sysdefaults`, which is this function's + output for the whole site -- so the item cannot be filtered against one company and + then drawn pointing at another. + """ + return frappe.defaults.get_defaults().get("company") + + +def can_see(items: list[dict], context: "NavigationContext") -> list[dict]: + """Which of these items this user may follow. + + `Custom` rather than `Readable DocType`, and that is the whole reason the kind carries + server code at all. The item points at one company *document*, and restricting a user + to a subset of companies with `User Permission` is ordinary on an ERPNext site -- so + the doctype-level bucket would leave the item on the rail for somebody whose only + company is a different one. frappe/frappe#42231 accepted that leak for `Record` items + and wrote it down as a cost; a kind with exactly one destination can afford the honest + check instead. + + One permission call for the batch rather than one per item. Every item of this kind + shares the site's default company, so there is one question to ask however many rows + name it -- which is what the batched signature is for (frappe/frappe#42231 measured 553 + per-item checks at 3,594 ms against 25 ms for a single pass, inside boot's blocking + fetch). + """ + company = default_company() + + # Nothing to point at. Dropped here rather than left for the renderer so the row stays + # out of boot entirely, instead of being sent and then declined by the browser. + if not company: + return [] + + try: + permitted = frappe.has_permission("Company", doc=company, user=context.user) + except frappe.DoesNotExistError: + # Global Defaults naming a company somebody has since deleted. Left to raise, the + # framework catches it, fails the kind closed and writes one Error Log -- and it would + # write another on the next boot, and on every other person's, because the cause is a + # site's data rather than a passing fault. It is the same "nothing to point at" as an + # unfinished setup wizard, so it is answered the same way. + # + # Caught rather than checked ahead of time: `has_permission` already loads the document + # (lazily, without child tables), so an existence check would be a second query on every + # boot to save one on almost none. + return [] + + return items if permitted else [] diff --git a/erpnext/setup/navigation_item_type/default_company/frontend/item.js b/erpnext/setup/navigation_item_type/default_company/frontend/item.js new file mode 100644 index 00000000000..bdcc476767f --- /dev/null +++ b/erpnext/setup/navigation_item_type/default_company/frontend/item.js @@ -0,0 +1,35 @@ +// The `Default Company` kind: the one company this site works in. +// +// ERPNext's own kind, and the first one contributed by an app that is not frappe +// (frappe/frappe#42424). It exists because the eight framework kinds cannot express it: +// a `Record` item names its document, and the company's name is chosen during the setup +// wizard, so ERPNext has no name to ship. The destination is therefore computed here, +// out of a value the boot already carries. +// +// `boot.sysdefaults` is `frappe.defaults.get_defaults()`, which every desk v2 boot sends +// and which holds `company` on any ERPNext site past setup. Nothing new rides in boot for +// this, and the server half reads the same key when it decides who may see the item, so +// the two cannot point at different companies. + +import { routeFor } from "@shell"; + +export default { + render(item, { boot }) { + const company = boot.sysdefaults?.company; + + // A site still in the setup wizard has no default company. The server drops the item + // before boot for exactly this case; this is the second fence, and it is here rather + // than trusted away because a renderer runs on whatever the browser was handed. + if (!company) return null; + + return { to: routeFor("Company", company) }; + }, + + // No authored label on the shipped row, deliberately: the company's name is the most + // useful thing this item can say, and it is not knowable when the row is written. An + // authored label would still win (frappe/frappe#42230), so a site that prefers a fixed + // word can set one. + label(item, { boot }) { + return boot.sysdefaults?.company; + }, +}; diff --git a/erpnext/sidebar/module_def_setup/module_def_setup.json b/erpnext/sidebar/module_def_setup/module_def_setup.json index 2e7cafcb3ee..b8746bb877c 100644 --- a/erpnext/sidebar/module_def_setup/module_def_setup.json +++ b/erpnext/sidebar/module_def_setup/module_def_setup.json @@ -1,13 +1,13 @@ { "app": "erpnext", - "creation": "2026-09-02 22:15:00.000000", + "creation": "2026-09-02 22:15:00", "docstatus": 0, "doctype": "Sidebar", "idx": 0, "items": [], "link_doctype": "Module Def", "link_to": "Setup", - "modified": "2026-09-02 22:15:00.000000", + "modified": "2026-09-02 20:36:43.873382", "modified_by": "Administrator", "name": "module_def_setup", "navigation_items": [ @@ -243,6 +243,17 @@ "link_doctype": "", "switches_app": 0 }, + { + "added": 0, + "collapsible": 0, + "hidden": 0, + "item_type": "Default Company", + "keep_closed": 0, + "key": "default-company", + "link_doctype": "Company", + "parent_key": "section-organization", + "switches_app": 0 + }, { "added": 0, "collapsible": 0, diff --git a/erpnext/tests/test_navigation.py b/erpnext/tests/test_navigation.py index 0d87a2e2ce0..f5eb54bbb75 100644 --- a/erpnext/tests/test_navigation.py +++ b/erpnext/tests/test_navigation.py @@ -3,17 +3,20 @@ """ERPNext's shipped desk v2 navigation: one Rail record and eighteen module Sidebars. -These are fixture tests. ERPNext ships no navigation code at all -- the rows arrive as JSON at -`bench migrate` and the framework resolves them -- so what there is to get wrong is the content -of those rows: a rail item naming a sidebar nobody ships, a row pointing at a doctype this site -does not have, or two rows claiming one key. Each of those resolves to a quietly shorter list -rather than to an error, which is why they are asserted here. +These are fixture tests. The rows arrive as JSON at `bench migrate` and the framework resolves +them, so what there is to get wrong is the content of those rows: a rail item naming a sidebar +nobody ships, a row pointing at a doctype this site does not have, or two rows claiming one key. +Each of those resolves to a quietly shorter list rather than to an error, which is why they are +asserted here. This is the module-primary half of charter point 2 (frappe/frappe#42226): the same table, the same resolver and the same item kinds that give CRM a doctype-primary rail, giving ERPNext one made of modules. The rail is a `Sidebar` item per module that has an authored sidebar and a `Module` item per module that does not. +The one piece of ERPNext navigation code lives elsewhere: `test_navigation_kind.py` covers the +`Default Company` item kind, which is the app contributing a kind rather than authoring rows. + Every test runs as somebody other than Administrator. The permission filter short-circuits for an administrator (`frappe/shell/navigation_filter.py`), so an Administrator suite would pass against a rail that is not being filtered at all. diff --git a/erpnext/tests/test_navigation_kind.py b/erpnext/tests/test_navigation_kind.py new file mode 100644 index 00000000000..74582a8e45c --- /dev/null +++ b/erpnext/tests/test_navigation_kind.py @@ -0,0 +1,224 @@ +# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +"""The `Default Company` navigation item kind: ERPNext's own, and the first one an app has +contributed (frappe/frappe#42424). + +A kind is two files and, optionally, a hook. The record at +`setup/navigation_item_type/default_company/default_company.json` arrives at `bench migrate`; +the renderer beside it at `frontend/item.js` arrives at `bench build`; and because the record +declares the `Custom` permission rule, `hooks.py` points the framework at a `can_see` in the +same folder. Those are three independent channels, and the failure they share is quiet: a kind +missing its renderer is skipped with a console line, and a kind declaring `Custom` with no +`can_see` fails closed. Both read as an item that is simply not there. + +So the pairing is asserted as much as the behaviour. +""" + +import os +from unittest.mock import patch + +import frappe +from frappe.shell.navigation import resolve_navigation +from frappe.tests import IntegrationTestCase + +from erpnext.setup.navigation_item_type.default_company import default_company as kind + +TYPE_NAME = "Default Company" +FOLDER = os.path.join(frappe.get_app_path("erpnext"), "setup", "navigation_item_type", "default_company") + +# Where the item is authored: ERPNext's Setup sidebar, beside the `Company` list. +SIDEBAR = "module_def_setup" +KEY = "default-company" + +# Any name will do where the permission call is stubbed: `can_see` passes the string through +# and never reads the document itself. +COMPANY = "A Company This Site May Or May Not Have" + + +class TestDefaultCompanyKind(IntegrationTestCase): + """What the app ships, and whether the three halves agree with each other.""" + + def test_the_type_record_arrives_at_migrate(self): + row = frappe.db.get_value( + "Navigation Item Type", TYPE_NAME, ["permission_rule", "target_doctype", "module"], as_dict=True + ) + + self.assertIsNotNone(row, "the type record did not arrive; `bench migrate` imports it") + self.assertEqual(row.permission_rule, "Custom") + self.assertEqual(row.target_doctype, "Company") + self.assertEqual(row.module, "Setup") + + def test_the_renderer_ships_beside_the_record(self): + """The pair is the kind. Ship one without the other and the item vanishes quietly -- + with no renderer it is skipped and logged to the console, which nobody is reading.""" + self.assertTrue(os.path.isfile(os.path.join(FOLDER, "default_company.json"))) + self.assertTrue(os.path.isfile(os.path.join(FOLDER, "frontend", "item.js"))) + + def test_the_hook_names_a_can_see_that_imports(self): + """A `Custom` type whose hook points at nothing fails closed: every item of the kind is + dropped and one line goes to the Error Log. This is the same lookup the framework does.""" + paths = frappe.get_hooks("navigation_item_resolvers", default={}).get(TYPE_NAME) + + self.assertTrue(paths, "hooks.py contributes no resolver for the kind") + self.assertTrue(callable(frappe.get_attr(f"{paths[-1]}.can_see"))) + + def test_the_shipped_row_carries_no_label(self): + """Deliberate: the renderer's fallback names the item after the company, which is not + knowable when the row is authored. An authored label would win over it.""" + row = frappe.db.get_value( + "Navigation Item", + {"parenttype": "Sidebar", "parent": SIDEBAR, "key": KEY}, + ["item_type", "label", "parent_key"], + as_dict=True, + ) + + self.assertIsNotNone(row, f"{SIDEBAR} does not ship the {KEY} row") + self.assertEqual(row.item_type, TYPE_NAME) + self.assertFalse(row.label) + self.assertEqual(row.parent_key, "section-organization") + + def test_the_type_is_code_owned(self): + """Charter point 5: a type row is app content, so nobody may mint one on a site. + + Read off the permission rows rather than by asking `has_permission`, which answers True + for Administrator whatever the rows say -- the account most likely to be running this. + """ + grants = frappe.get_meta("Navigation Item Type").permissions + + self.assertTrue(grants, "the table grants nothing at all, which is a different bug") + for grant in grants: + self.assertFalse(grant.create, grant.role) + self.assertFalse(grant.write, grant.role) + + +class TestDefaultCompanyVisibility(IntegrationTestCase): + """`can_see`, which is the whole reason the kind declares `Custom` rather than a bucket.""" + + def setUp(self): + self.items = [{"key": KEY, "item_type": TYPE_NAME}] + self.context = _Context(frappe.session.user) + + def test_a_site_with_no_default_company_ships_nothing(self): + """A site still in the setup wizard. The item has nothing to point at, and it is dropped + here rather than sent and then declined by the renderer.""" + with patch.object(kind, "default_company", return_value=None): + self.assertEqual(kind.can_see(self.items, self.context), []) + + def test_the_check_is_on_the_document_and_not_the_doctype(self): + """The difference the kind exists for. `Readable DocType` asks whether this user may read + `Company` at all; the honest question is whether they may open *this* company, which is + what a `User Permission` decides on an ordinary ERPNext site.""" + with patch.object(kind, "default_company", return_value=COMPANY): + with patch.object(frappe, "has_permission", return_value=True) as allowed: + self.assertEqual(kind.can_see(self.items, self.context), self.items) + + self.assertEqual(allowed.call_args.args, ("Company",)) + self.assertEqual(allowed.call_args.kwargs["doc"], COMPANY) + + with patch.object(frappe, "has_permission", return_value=False): + self.assertEqual(kind.can_see(self.items, self.context), []) + + def test_a_default_company_that_no_longer_exists_ships_nothing(self): + """Global Defaults naming a deleted company. Left to raise, this would fail the kind + closed *and* write an Error Log on every boot of every session, since the cause is the + site's data rather than a passing fault. + + As somebody other than Administrator, who never reaches the document: the permission + system answers True for them before it looks anything up, so this leak is invisible to + the account most likely to be testing it. + """ + context = _Context(_make_user("default.company.kind@example.com")) + + with patch.object(kind, "default_company", return_value="A Company Nobody Has"): + self.assertEqual(kind.can_see(self.items, context), []) + + def test_one_permission_call_however_many_items(self): + """Batched by contract (frappe/frappe#42231): every item of this kind shares one + destination, so there is one question to ask. A loop here is what the 3,594 ms + measurement was made on.""" + many = [{"key": f"{KEY}-{n}", "item_type": TYPE_NAME} for n in range(20)] + + with patch.object(kind, "default_company", return_value=COMPANY): + with patch.object(frappe, "has_permission", return_value=True) as allowed: + self.assertEqual(kind.can_see(many, self.context), many) + + self.assertEqual(allowed.call_count, 1) + + def test_a_restricted_user_loses_the_item_from_the_resolved_sidebar(self): + """End to end, through the framework rather than by calling `can_see`: the same person + keeps or loses the row on nothing but a `User Permission` naming another company. + + Run against the companies the site already has rather than ERPNext's `Company` test + records, which cannot be created on a site that already keeps books -- their fiscal years + collide with the real ones. So this is the one case that needs a second company and skips + without one. + """ + companies = frappe.get_all("Company", pluck="name", limit=2) + if len(companies) < 2: + self.skipTest("needs two companies: one to be the default, one to be restricted to") + + default, other = companies + user = _make_user("default.company.kind@example.com") + + with patch.object(kind, "default_company", return_value=default): + self.assertIn(KEY, _sidebar_keys(user)) + + _restrict(user, other) + self.assertNotIn(KEY, _sidebar_keys(user)) + + +class _Context: + """Stands in for `NavigationContext`, of which `can_see` reads one attribute. + + A real one would resolve the whole permission pass for a user this test does not otherwise + need, and the narrowness is the point: a resolver that reached for more would be a resolver + recomputing what the framework already paid for. + """ + + def __init__(self, user: str): + self.user = user + + +def _sidebar_keys(user: str) -> list[str]: + frappe.set_user(user) + try: + frappe.clear_cache(user=user) + rows = resolve_navigation("erpnext")["sidebars"].get(SIDEBAR, []) + return [row["key"] for row in rows] + finally: + frappe.set_user("Administrator") + + +def _restrict(user: str, company: str): + """Pin somebody to one company, which is how an ERPNext site says it.""" + frappe.set_user("Administrator") + frappe.get_doc( + { + "doctype": "User Permission", + "user": user, + "allow": "Company", + "for_value": company, + "apply_to_all_doctypes": 1, + } + ).insert(ignore_permissions=True) + frappe.clear_cache(user=user) + + +def _make_user(email: str) -> str: + if frappe.db.exists("User", email): + return email + + return ( + frappe.get_doc( + { + "doctype": "User", + "email": email, + "first_name": "Default Company Kind", + "user_type": "System User", + "roles": [{"role": "System Manager"}], + } + ) + .insert(ignore_permissions=True) + .name + )