Skip to content

Commit 07f3716

Browse files
fix(dashboard): link a tenant row to its detail page (#4004)
## What this PR does Fixes #3822 Tenant list showed name as plain text and the only link in a row was Edit button, so tenant detail page with its tabs and Delete was reachable only by opening edit form and cancelling out of it. Name cell is a link to that page now, it appears exactly where Edit appears and points to the same CR. Plain click switches selected tenant to the row's parent first, same as Edit does, because detail page takes namespace from selected tenant. Ctrl, cmd and shift click don't run that switch, so the tab you open a tenant from stays where it was. First commit fixes what the link depends on. Console recovers a tenant's parent from namespace name (`tenant-a-b` sits under `tenant-a`), but root is the one level that naming skips: tenant `acme` ordered in `tenant-root` owns `tenant-acme`, not `tenant-root-acme`. So `realParentNamespace` returned undefined for every top level tenant, which is most tenants on a normal cluster, and Edit button has been missing there for as long as it exists. Ancestor labels carry the parent that the name does not. Root level tenant named `root-x` resolves to CR `root-x` now, not `x`. ### Notes I first tried to make links carry their tenant in the url (`?tenant=`), so a detail page opened in new tab resolves on its own. That turned into questions about tenant context itself: what to do with a tenant in a link the user can't see, what localStorage may hold, and TenantNamespace LIST ignoring `labelSelector`. Detail route addresses a Tenant CR by relative name and takes namespace from selected tenant, and making that robust is a refactor of tenant context and routing, which should start with study and planning, not with patches on patches. So this PR only closes the UX gap (no way from tenant list into a tenant's page), going deeper is out of scope. Known limit that comes with it: link has no tenant in it, so opening it in a new tab (ctrl/cmd/middle click, open in new tab, copied url) resolves against the tenant last selected in any tab, because selection lives in shared localStorage. For a row directly under that tenant it opens the right page. For a row deeper in the tree the page says `Application <name> not found.` and there is no Delete. If that tenant also has a direct child tenant with the same relative name, new tab opens that other tenant's page with working Edit and Delete, and delete confirm says only `Delete Tenant "<name>"?`. Plain click in the same tab is not affected. Detail page's own tabs and Edit link already behave this way, it's how detail route resolves its namespace today and fixing it properly is the refactor above. A row links, and offers Edit, exactly when namespace holding its Tenant CR is one the apiserver returns to this user. It returns a tenant namespace only if the user is subject of a RoleBinding in it, so sub-tenant admin that cant act in `tenant-root` sees no link there. No row level Delete button. Issue allows either that or a route to detail page, I took the route so confirm and mutation stay in one place. Covered by tests, every source change here fails a test when reverted: - top level tenant, nested one, root level tenant named `root-x`, forest root without ancestor labels - bridged row whose real parent is invisible gets neither link nor Edit - plain click switches to the row's parent and opens detail page - ctrl click leaves selection and location alone ### Screenshots Tenant list under root on a dev cluster. Every row name is a link to tenant page now (`pr4078` is hovered), top level tenants had neither link nor Edit before. Host column redacted. <!-- SCREENSHOT: drag in 01-tenant-list-links.png here --> <img width="1440" height="900" alt="01-tenant-list-links" src="https://github.com/user-attachments/assets/39cd723a-1ac5-4d3b-8c36-b70ddc626961" /> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: All changed files are under `packages/system/dashboard/images/console`, console has no downstream copy (standalone UI repo is archived). ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(dashboard): tenant name in the tenant list links to its detail page now (where Delete is), and top level tenants get their Edit button back. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Root-level tenant names are now displayed without incorrectly removing prefixes, and tenants ordered directly under the root are correctly recognized in the hierarchy. * Tenant rows link to their detail pages when the parent tenant is accessible. Same-tab navigation updates the active tenant, while opening a link in another tab leaves the current selection unchanged. * Edit links and navigation now use consistent tenant routes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Backported-from: a2cc9a3
2 parents 3d68d41 + bd5e0b5 commit 07f3716

6 files changed

Lines changed: 321 additions & 21 deletions

File tree

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import { describe, it, expect } from "vitest"
2+
import type { MouseEvent } from "react"
3+
import { navigatesThisTab } from "./links.ts"
4+
5+
function click(overrides: Partial<MouseEvent> = {}): MouseEvent {
6+
return {
7+
button: 0,
8+
metaKey: false,
9+
altKey: false,
10+
ctrlKey: false,
11+
shiftKey: false,
12+
...overrides,
13+
} as MouseEvent
14+
}
15+
16+
describe("navigatesThisTab", () => {
17+
it("accepts a plain primary click", () => {
18+
expect(navigatesThisTab(click())).toBe(true)
19+
})
20+
21+
// Each of these hands the navigation to a new tab or window, so the tab the
22+
// click happened in keeps whatever tenant it was showing.
23+
it.each(["metaKey", "ctrlKey", "shiftKey", "altKey"] as const)(
24+
"refuses a click modified with %s",
25+
(modifier) => {
26+
expect(navigatesThisTab(click({ [modifier]: true }))).toBe(false)
27+
},
28+
)
29+
30+
it("refuses a non-primary button", () => {
31+
expect(navigatesThisTab(click({ button: 1 }))).toBe(false)
32+
})
33+
})
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import type { MouseEvent } from "react"
2+
3+
/**
4+
* Whether a click on an anchor will navigate the tab it happened in, rather
5+
* than open a new tab or window. React runs `onClick` either way, and a
6+
* cross-tenant link's handler aligns the tenant context of the tab it is
7+
* leaving — which is wrong when that tab is staying where it is. Same button
8+
* and modifier test react-router's own `Link` applies before it intercepts a
9+
* click, minus the `target` check these links have no attribute for.
10+
*/
11+
export function navigatesThisTab(e: MouseEvent): boolean {
12+
return e.button === 0 && !e.metaKey && !e.altKey && !e.ctrlKey && !e.shiftKey
13+
}
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import { describe, it, expect } from "vitest"
2+
import type { TenantNamespace } from "@cozystack/types"
3+
import { realParentNamespace } from "./tenant-tree.ts"
4+
5+
function tn(name: string, ancestors: string[]): TenantNamespace {
6+
const labels: Record<string, string> = {}
7+
for (const a of [...ancestors, name]) labels[`tenant.cozystack.io/${a}`] = ""
8+
return {
9+
apiVersion: "core.cozystack.io/v1alpha1",
10+
kind: "TenantNamespace",
11+
metadata: { name, labels },
12+
} as TenantNamespace
13+
}
14+
15+
describe("realParentNamespace", () => {
16+
it("takes the deepest ancestor that prefixes the namespace", () => {
17+
// The full chain is on the labels; only the longest prefix is the parent.
18+
expect(
19+
realParentNamespace(
20+
tn("tenant-acme-eu-dev", ["tenant-root", "tenant-acme", "tenant-acme-eu"]),
21+
),
22+
).toBe("tenant-acme-eu")
23+
})
24+
25+
it("names an inaccessible parent, which is the point of asking", () => {
26+
// Visibility is the caller's problem: the CR of a bridged row lives in a
27+
// namespace the user may not read, and the caller has to be able to tell.
28+
expect(
29+
realParentNamespace(tn("tenant-acme-eu", ["tenant-root", "tenant-acme"])),
30+
).toBe("tenant-acme")
31+
})
32+
33+
it("resolves a tenant ordered directly in the root", () => {
34+
// A tenant `acme` created in `tenant-root` owns `tenant-acme`, not
35+
// `tenant-root-acme`, so the prefix rule alone leaves every top-level
36+
// tenant — most tenants on a normal cluster — looking parentless.
37+
expect(realParentNamespace(tn("tenant-acme", ["tenant-root"]))).toBe(
38+
"tenant-root",
39+
)
40+
})
41+
42+
it("does not mistake a root-level name that starts with root", () => {
43+
// `tenant-root-x` is the tenant `root-x` ordered in the root, and the
44+
// prefix branch answers with the same parent the label does.
45+
expect(realParentNamespace(tn("tenant-root-x", ["tenant-root"]))).toBe(
46+
"tenant-root",
47+
)
48+
})
49+
50+
it("has no parent for the hierarchy root", () => {
51+
expect(realParentNamespace(tn("tenant-root", []))).toBeUndefined()
52+
})
53+
54+
it("has no parent when the ancestor labels are gone", () => {
55+
// A TenantNamespace carrying only its own label is either the root or
56+
// mislabelled; either way there is no CR namespace to derive.
57+
expect(realParentNamespace(tn("tenant-acme", []))).toBeUndefined()
58+
})
59+
})

‎packages/system/dashboard/images/console/apps/console/src/lib/tenant-tree.ts‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { TenantNamespace } from "@cozystack/types"
2+
import { ROOT_TENANT_NAMESPACE } from "./constants.ts"
23
import { tenantDisplayName } from "./tenant-context.tsx"
34

45
const ANCESTOR_LABEL_PREFIX = "tenant.cozystack.io/"
@@ -76,11 +77,18 @@ export function flattenTenantForest(
7677
/**
7778
* Row label relative to the visible parent: `tenant-whmcs-crpjxhwm` under
7879
* `tenant-whmcs` shows as `crpjxhwm`. Forest roots keep their full display
79-
* name — there is no parent on screen to be relative to.
80+
* name — there is no parent on screen to be relative to, and neither do the
81+
* root's own children, whose namespace never carries the parent's name: under
82+
* `tenant-root` the tenant `root-x` owns `tenant-root-x`, and stripping the
83+
* prefix there would label it `x` while its CR is `root-x`.
8084
*/
8185
export function relativeTenantName(node: TenantTreeNode): string {
8286
const ns = node.tn.metadata.name
83-
if (node.parentNs && ns.startsWith(`${node.parentNs}-`)) {
87+
if (
88+
node.parentNs &&
89+
node.parentNs !== ROOT_TENANT_NAMESPACE &&
90+
ns.startsWith(`${node.parentNs}-`)
91+
) {
8492
return ns.slice(node.parentNs.length + 1)
8593
}
8694
return tenantDisplayName(node.tn)
@@ -96,7 +104,16 @@ export function relativeTenantName(node: TenantTreeNode): string {
96104
*/
97105
export function realParentNamespace(tn: TenantNamespace): string | undefined {
98106
const ns = tn.metadata.name
99-
return ancestorNamespaces(tn)
107+
const ancestors = ancestorNamespaces(tn)
108+
const nested = ancestors
100109
.filter((ancestor) => ns.startsWith(`${ancestor}-`))
101110
.sort((a, b) => b.length - a.length)[0]
111+
if (nested) return nested
112+
// The root is the one level the naming rule skips: a tenant `x` ordered in
113+
// `tenant-root` owns `tenant-x`, not `tenant-root-x`, so the prefix test
114+
// never matches and every top-level tenant — most tenants on a normal
115+
// cluster — looked parentless. The ancestor labels still say otherwise.
116+
return ancestors.includes(ROOT_TENANT_NAMESPACE)
117+
? ROOT_TENANT_NAMESPACE
118+
: undefined
102119
}

‎packages/system/dashboard/images/console/apps/console/src/routes/TenantsPage.test.tsx‎

Lines changed: 148 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
import { describe, it, expect, vi, beforeAll } from "vitest"
2-
import { screen } from "@testing-library/react"
2+
import { fireEvent, screen } from "@testing-library/react"
33
import userEvent from "@testing-library/user-event"
4+
import { useLocation } from "react-router"
45
import {
56
K8sClient,
67
type K8sList,
78
type APIGroupList,
89
} from "@cozystack/k8s-client"
910
import { TenantsPage } from "./TenantsPage.tsx"
1011
import { TenantProvider } from "../lib/tenant-provider.tsx"
12+
import { useTenantContext } from "../lib/tenant-context.tsx"
1113
import { SELECTED_TENANT_KEY } from "../lib/constants.ts"
1214
import { renderWithK8sProvider } from "../test-utils/render.tsx"
1315

@@ -30,10 +32,19 @@ const VISIBLE = [
3032
tn("tenant-whmcs-a-b", ["tenant-root", "tenant-whmcs", "tenant-whmcs-a"]),
3133
]
3234

33-
function makeClient(): K8sClient {
35+
function LocationProbe() {
36+
const { pathname } = useLocation()
37+
return <div data-testid="location">{pathname}</div>
38+
}
39+
40+
function SelectedTenant() {
41+
return <output data-testid="selected-tenant">{useTenantContext().selectedTenant}</output>
42+
}
43+
44+
function makeClient(tenants: unknown[] = VISIBLE): K8sClient {
3445
const client = new K8sClient()
3546
vi.spyOn(client, "list").mockImplementation(async (_g, _v, plural) => {
36-
const items = plural === "tenantnamespaces" ? VISIBLE : []
47+
const items = plural === "tenantnamespaces" ? tenants : []
3748
return {
3849
apiVersion: "v1",
3950
kind: `${plural}List`,
@@ -89,6 +100,12 @@ describe("TenantsPage tree", () => {
89100
expect(editButtons).toHaveLength(1)
90101
expect(screen.getByTitle("Edit tenant whmcs-x")).toBeInTheDocument()
91102
expect(screen.queryByTitle("Edit tenant whmcs-a-b")).not.toBeInTheDocument()
103+
104+
// The name links to the detail page, where Delete is, exactly where Edit is
105+
// offered: a row whose CR namespace cannot be read stays plain text.
106+
expect(screen.getByRole("link", { name: "x" })).toHaveAttribute("href", "/admin/tenants/x")
107+
expect(screen.queryByRole("link", { name: "a-b" })).toBeNull()
108+
expect(screen.queryByRole("link", { name: "whmcs" })).toBeNull()
92109
})
93110

94111
it("collapses a subtree via the row toggle", async () => {
@@ -109,3 +126,131 @@ describe("TenantsPage tree", () => {
109126
expect(await screen.findByText("x")).toBeInTheDocument()
110127
})
111128
})
129+
130+
// Tenants ordered directly in the root are the common case on a normal
131+
// cluster, and they are the one level the hierarchical namespace naming skips:
132+
// `acme` created in `tenant-root` owns `tenant-acme`, not `tenant-root-acme`.
133+
const ROOT_VISIBLE = [
134+
tn("tenant-root", []),
135+
tn("tenant-acme", ["tenant-root"]),
136+
tn("tenant-acme-eu", ["tenant-root", "tenant-acme"]),
137+
// A root-level tenant whose own name starts with `root`, which is the case
138+
// that makes "strip the parent's prefix" the wrong rule rather than merely
139+
// an incomplete one.
140+
tn("tenant-root-x", ["tenant-root"]),
141+
]
142+
143+
function renderTenants(tenants: unknown[], selected: string) {
144+
localStorage.setItem(SELECTED_TENANT_KEY, selected)
145+
renderWithK8sProvider(
146+
<TenantProvider>
147+
<TenantsPage />
148+
<LocationProbe />
149+
<SelectedTenant />
150+
</TenantProvider>,
151+
{ client: makeClient(tenants), initialRoute: "/admin/tenants" },
152+
)
153+
}
154+
155+
describe("TenantsPage under a visible root", () => {
156+
it("offers Edit on a tenant ordered directly in the root", async () => {
157+
renderTenants(ROOT_VISIBLE, "root")
158+
159+
await userEvent.click(await screen.findByTitle("Edit tenant acme"))
160+
161+
expect(screen.getByTestId("location")).toHaveTextContent("/admin/tenants/acme/edit")
162+
})
163+
164+
it("names a nested tenant's CR relative to its parent", async () => {
165+
renderTenants(ROOT_VISIBLE, "root")
166+
167+
await userEvent.click(await screen.findByTitle("Edit tenant acme-eu"))
168+
169+
expect(screen.getByTestId("location")).toHaveTextContent("/admin/tenants/eu/edit")
170+
})
171+
172+
// `tenant-root-x` is the tenant `root-x`, not the tenant `x`: the row label
173+
// and the CR the row opens have to agree on that.
174+
it("keeps a root-level tenant named root-x whole", async () => {
175+
renderTenants(ROOT_VISIBLE, "root")
176+
177+
expect(await screen.findByText("root-x")).toBeInTheDocument()
178+
await userEvent.click(screen.getByTitle("Edit tenant root-x"))
179+
180+
expect(screen.getByTestId("location")).toHaveTextContent("/admin/tenants/root-x/edit")
181+
})
182+
})
183+
184+
// A forest root that carries no ancestor labels is self-referential: its CR
185+
// lives in its own namespace, and deriving the CR name must not subtract the
186+
// node's own name from itself and leave nothing.
187+
describe("TenantsPage self-referential root", () => {
188+
it("names the CR of a root that is not tenant-root", async () => {
189+
renderTenants([tn("tenant-whmcs", [])], "whmcs")
190+
191+
await userEvent.click(await screen.findByTitle("Edit tenant whmcs"))
192+
193+
expect(screen.getByTestId("location")).toHaveTextContent("/admin/tenants/whmcs/edit")
194+
})
195+
})
196+
197+
describe("TenantsPage row link", () => {
198+
it("links every row whose Tenant CR it can name", async () => {
199+
renderTenants(ROOT_VISIBLE, "root")
200+
201+
// The root's CR is self-referential: `root` inside its own namespace.
202+
expect(await screen.findByRole("link", { name: "root" })).toHaveAttribute(
203+
"href",
204+
"/admin/tenants/root",
205+
)
206+
expect(screen.getByRole("link", { name: "acme" })).toHaveAttribute("href", "/admin/tenants/acme")
207+
expect(screen.getByRole("link", { name: "eu" })).toHaveAttribute("href", "/admin/tenants/eu")
208+
expect(screen.getByRole("link", { name: "root-x" })).toHaveAttribute(
209+
"href",
210+
"/admin/tenants/root-x",
211+
)
212+
})
213+
214+
it("links a self-referential root to its own CR", async () => {
215+
renderTenants([tn("tenant-whmcs", [])], "whmcs")
216+
217+
expect(await screen.findByRole("link", { name: "whmcs" })).toHaveAttribute(
218+
"href",
219+
"/admin/tenants/whmcs",
220+
)
221+
})
222+
223+
// The detail page resolves `eu` in the selected tenant's namespace, and eu's
224+
// CR lives in acme's, so the click has to land the tab on acme.
225+
it("switches to the row's parent and opens the detail page", async () => {
226+
renderTenants(ROOT_VISIBLE, "root")
227+
228+
fireEvent.click(await screen.findByRole("link", { name: "eu" }))
229+
230+
expect(screen.getByTestId("selected-tenant").textContent).toBe("acme")
231+
expect(screen.getByTestId("location")).toHaveTextContent("/admin/tenants/eu")
232+
})
233+
234+
// Edit shares the route with the link, and its form resolves the CR the same
235+
// way, so it has to land on the parent too.
236+
it("switches to the row's parent before opening the edit form", async () => {
237+
renderTenants(ROOT_VISIBLE, "root")
238+
239+
await userEvent.click(await screen.findByTitle("Edit tenant acme-eu"))
240+
241+
expect(screen.getByTestId("selected-tenant").textContent).toBe("acme")
242+
})
243+
244+
// A ctrl-click opens a new tab; the tab it happened in stays where it is.
245+
it("leaves the current tab alone on a ctrl-click", async () => {
246+
renderTenants(ROOT_VISIBLE, "root")
247+
const link = await screen.findByRole("link", { name: "eu" })
248+
// jsdom cannot open the new tab; the handler under test has run by then.
249+
link.addEventListener("click", (e) => e.preventDefault())
250+
251+
fireEvent.click(link, { ctrlKey: true })
252+
253+
expect(screen.getByTestId("selected-tenant").textContent).toBe("root")
254+
expect(screen.getByTestId("location")).toHaveTextContent(/^\/admin\/tenants$/)
255+
})
256+
})

0 commit comments

Comments
 (0)