feat(desk-v2): contribute the Default Company navigation item kind

The first navigation item kind contributed by an app that is not frappe, which is
what charter point 6 of frappe/frappe#42226 promised and frappe/frappe#42424 asked
for: an app adds a kind through the same mechanism the framework's own eight go
through, and the framework grows no case for it.

The kind is one folder under `setup/navigation_item_type/default_company/`: the
`Navigation Item Type` record, which arrives at `bench migrate`; `frontend/item.js`
beside it, which arrives at `bench build`; and `default_company.py`, which
`hooks.py` names under `navigation_item_resolvers`. One row in the Setup sidebar
uses it, above the `Company` list.

It points at the company this site works in. That is a destination none of the
eight kinds can express: 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
renderer computes it from `boot.sysdefaults.company`, which every desk v2 boot
already sends, and the resolver reads the same key -- so the item cannot be
filtered against one company and drawn pointing at another.

The type declares the `Custom` permission rule rather than `Readable DocType`,
and that is the reason it carries server code at all. `Readable DocType` asks
whether a person may read `Company`; the honest question is whether they may open
*this* company, which a `User Permission` decides and which ERPNext sites use
routinely. Measured both ways on a real site: a System Manager pinned to another
company keeps the item under `Readable DocType` and loses it under `Custom`.

The shipped row carries no label on purpose, so the renderer's fallback names the
item after the company itself.

One thing the build found: a `Global Defaults` naming a company somebody has since
deleted makes `has_permission` raise. Left alone, the framework fails the kind
closed and writes an Error Log -- and would write another on every boot of every
session, because the cause is the site's data rather than a passing fault. It is
answered as the same "nothing to point at" an unfinished setup wizard gets. The
leak is invisible to Administrator, who never reaches the document.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
shariquerik
2026-09-02 20:53:22 +05:30
parent 7fe6b46a9e
commit 626d8b322c
9 changed files with 378 additions and 7 deletions

View File

@@ -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,

View File

@@ -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"
}

View File

@@ -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 []

View File

@@ -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;
},
};

View File

@@ -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,

View File

@@ -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.

View File

@@ -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
)