diff --git a/.changeset/system-hub-count-error-state-3679.md b/.changeset/system-hub-count-error-state-3679.md new file mode 100644 index 0000000000..b879e87f14 --- /dev/null +++ b/.changeset/system-hub-count-error-state-3679.md @@ -0,0 +1,23 @@ +--- +'@object-ui/console': patch +--- + +System Hub: a card count that failed to load no longer renders as `0` + +Each count on the System Hub fetched one object and caught its own failure with +an empty page, so a 500, a 401, a 403 or a dropped connection all produced the +same confident `0` as a table that really is empty — no error, no retry, and no +way to tell the two apart. The most reachable case was a permission denial on a +single object: an administrator who may open the hub but cannot read +`sys_audit_log` was shown "0 entries" rather than being told anything at all. + +A failed lookup now leaves that card's count unknown, and the badge — which +already renders only for a known count — is omitted, so the card shows no +number instead of a wrong one. The catch stays on each call rather than around +the batch, so one object's failure blanks only its own card and the cards beside +it keep the real numbers they received. + +Unchanged: an object the backend does not have still counts `0`. The adapter +resolves an unregistered object as an empty page by design (callers read empty +as "feature unavailable"), so that never was an error and is not treated as one +here. diff --git a/apps/console/src/pages/system/SystemHubPage.tsx b/apps/console/src/pages/system/SystemHubPage.tsx index 1e1e6927ea..c79ab2fe2a 100644 --- a/apps/console/src/pages/system/SystemHubPage.tsx +++ b/apps/console/src/pages/system/SystemHubPage.tsx @@ -55,6 +55,21 @@ interface HubCard { adminOnly?: boolean; } +/** + * One card's count from one `find` result: the row count when the lookup + * succeeded, `null` when it did not. + * + * `null` is not a formatting preference — it is the only shape this page has + * for "we do not know". The badge renders on `count !== null`, so an unknown + * count shows no badge at all, while `0` is a claim: the backend answered, and + * the answer was none. Collapsing a failed lookup into `0` prints a number + * nothing ever confirmed, and a 500 / 401 / 403 / offline then looks exactly + * like an empty table (objectui#3679). + */ +function countOrUnknown(result: { data?: unknown[] } | null): number | null { + return result === null ? null : (result.data?.length ?? 0); +} + export function SystemHubPage() { const navigate = useNavigate(); const { appName } = useParams(); @@ -104,22 +119,46 @@ export function SystemHubPage() { // MEASUREMENT case in this page's test rather than quietly re-aimed. // // TODO: Replace with count-specific API endpoint when available + // + // Each `.catch` resolves to `null`, NOT to an empty page. What these + // catches actually cover is the class the adapter does NOT absorb: it + // rethrows everything that is not a 404, so a 500 / 401 / 403 / offline + // / timeout lands here. Answering that with `{ data: [] }` used to print + // a confident `0` — the same pixel as "there really are none", with no + // error, no retry and no way for an administrator to tell the two apart + // (objectui#3679). `null` flows into `counts` and the badge's existing + // `count !== null` branch simply omits the badge. + // + // Per call rather than once around the `Promise.all`, because the most + // reachable failure is a permission denial on ONE object — an admin who + // may open this hub but cannot read `sys_audit_log` should lose that + // card's number only, not the four beside it that answered fine. + // + // A 404 still does not reach here and still renders `0`: the adapter + // resolves unregistered objects as an empty page on purpose (see above). + // That is its contract, not a failure — the one card still riding on it + // is Permissions, which is objectui#3655's decision to close. const [usersRes, orgsRes, positionsRes, permsRes, logsRes] = await Promise.all([ - dataSource.find('sys_user').catch(() => ({ data: [] })), - dataSource.find('sys_organization').catch(() => ({ data: [] })), - dataSource.find('sys_position').catch(() => ({ data: [] })), - dataSource.find('sys_permission').catch(() => ({ data: [] })), - dataSource.find('sys_audit_log').catch(() => ({ data: [] })), + dataSource.find('sys_user').catch(() => null), + dataSource.find('sys_organization').catch(() => null), + dataSource.find('sys_position').catch(() => null), + dataSource.find('sys_permission').catch(() => null), + dataSource.find('sys_audit_log').catch(() => null), ]); setCounts({ - users: usersRes.data?.length ?? 0, - orgs: orgsRes.data?.length ?? 0, - positions: positionsRes.data?.length ?? 0, - permissions: permsRes.data?.length ?? 0, - auditLogs: logsRes.data?.length ?? 0, + users: countOrUnknown(usersRes), + orgs: countOrUnknown(orgsRes), + positions: countOrUnknown(positionsRes), + permissions: countOrUnknown(permsRes), + auditLogs: countOrUnknown(logsRes), }); } catch { - // Keep nulls on failure + // Keep nulls on failure. Only a SYNCHRONOUS throw from `dataSource.find` + // can arrive here: the `.catch`es above are attached to the returned + // promises, so nothing makes `Promise.all` reject. That is why this + // branch is not where the fix went — leaving the counts untouched only + // says "unknown" because they are still `null`, and `setCounts` above is + // the only place a lookup's outcome is ever written. } finally { setLoading(false); } diff --git a/apps/console/src/pages/system/__tests__/SystemHubPage.counts.test.tsx b/apps/console/src/pages/system/__tests__/SystemHubPage.counts.test.tsx index 30e4d44719..fb11cb37a5 100644 --- a/apps/console/src/pages/system/__tests__/SystemHubPage.counts.test.tsx +++ b/apps/console/src/pages/system/__tests__/SystemHubPage.counts.test.tsx @@ -31,11 +31,22 @@ * `SystemHubPage` itself is the real component, as in the sibling * `SystemHubPage.metadataCards.test.tsx` — a transcribed copy of the card list * is precisely how a wrong name survives. + * + * ── objectui#3679 ────────────────────────────────────────────────────────── + * A second describe block was added below for the other half of the same + * pixel. The names above decide WHICH object is counted; the error handling + * decides whether a count that never arrived is allowed to be spelled `0`. It + * no longer is: a non-404 rejection (500 / 401 / 403 / offline — the only + * class the adapter does not absorb) leaves that card's count `null`, and the + * badge's existing `count !== null` branch drops the badge. The MEASUREMENT + * case that pinned the old collapse-into-0 was rewritten there, as its own + * comment asked for; the other two MEASUREMENTs still pin objectui#3655's gap + * and are untouched. */ import '@testing-library/jest-dom/vitest'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { render, screen, within, cleanup } from '@testing-library/react'; +import { render, screen, within, cleanup, waitFor } from '@testing-library/react'; import { MemoryRouter, Routes, Route } from 'react-router-dom'; // Hoisted so the vi.mock factories below can close over them, and so the @@ -143,6 +154,54 @@ async function badge(cardTestId: string, text: string) { return within(card).findByText(text); } +/** + * A card's count badge, or `null` when the card shows none — which is how this + * page says "unknown" (the badge renders only on `count !== null`). + * + * Anchored `^…$` on purpose: the card's title and description carry the same + * label word ("Manage system users and accounts"), so an unanchored match + * would find text on a card that has no badge at all. + */ +function countBadge(cardTestId: string, label: string) { + return within(screen.getByTestId(cardTestId)).queryByText( + new RegExp(`^\\d+\\s+${label}$`), + ); +} + +/** + * Settles the fetch when NO badge is expected to appear, so awaiting one is not + * an option. `fetchCounts` clears `loading` in its `finally`, so the spinner + * going away is the single anchor that holds whether the calls resolved, + * rejected, or threw synchronously. + */ +async function settleWithoutBadges() { + await waitFor(() => + expect(screen.queryByText('Loading statistics...')).not.toBeInTheDocument(), + ); +} + +/** Every card that carries a count, as [testid, countLabel]. */ +const COUNTED_CARDS: ReadonlyArray = [ + ['hub-card-users', 'users'], + ['hub-card-organizations', 'organizations'], + ['hub-card-positions', 'positions'], + ['hub-card-permissions', 'permissions'], + ['hub-card-audit-log', 'entries'], +]; + +/** + * The object names the page asks for, in call order — including + * `sys_permission`, which the framework does not register (objectui#3655) and + * so is absent from the fixture registry above. + */ +const QUERIED_OBJECT_NAMES = [ + 'sys_user', + 'sys_organization', + 'sys_position', + 'sys_permission', + 'sys_audit_log', +]; + describe('System Hub card counts — object names (objectui#3670)', () => { it('counts Organizations through sys_organization, the name the framework registers', async () => { renderHub(); @@ -183,10 +242,16 @@ describe('System Hub card counts — object names (objectui#3670)', () => { }); // ── MEASUREMENT ──────────────────────────────────────────────────────────── - // The three cases below pin the CURRENT behaviour, not the desired one. They + // The two cases below pin the CURRENT behaviour, not the desired one. They // exist so the remaining gap is visible in the suite instead of being read as // a missed line, and so whoever resolves objectui#3655 has a failing anchor // to rewrite rather than a silent pass. + // + // There were three. The third — "a non-404 failure is collapsed into 0 as + // well, with no error affordance" — named objectui#3679 as the work that + // would rewrite it, and that work is done: it now lives in the next describe + // block with its expectation inverted. These two stay measurements because + // objectui#3655 is still open. it('MEASUREMENT: Permissions still reads 0 while both candidate objects hold rows', async () => { renderHub(); @@ -216,20 +281,96 @@ describe('System Hub card counts — object names (objectui#3670)', () => { expect(within(screen.getByTestId('hub-card-permissions')).getByText('0 permissions')).toBeInTheDocument(); }); - it('MEASUREMENT: a non-404 failure is collapsed into 0 as well, with no error affordance', async () => { - // The 404 never reaches the page's `.catch` — the adapter ate it upstream. - // What that `.catch` really covers is this: a 500 (or 401 / 403 / offline) - // on ONE object, rendered as a confident `0` on that card while its - // neighbours show real numbers. Recorded here only; changing the error - // handling is a separate class of work, filed as objectui#3679. +}); + +describe('System Hub card counts — a lookup that failed is not a `0` (objectui#3679)', () => { + it('a 500 on one object blanks that card instead of collapsing it into 0', async () => { + // This is the rewrite of PR #3680's third MEASUREMENT ("a non-404 failure + // is collapsed into 0 as well, with no error affordance") — same fixture, + // inverted expectation. The 404 never reaches the page's `.catch`; the + // adapter ate it upstream. What that `.catch` really covers is this. state.failures.sys_user = Object.assign(new Error('Internal Server Error'), { status: 500, }); renderHub(); - expect(await badge('hub-card-users', '0 users')).toBeInTheDocument(); - // The per-call `.catch` also keeps `Promise.all` from rejecting, so the - // other four cards still resolve — including the one this PR fixed. - expect(within(screen.getByTestId('hub-card-organizations')).getByText('2 organizations')).toBeInTheDocument(); + // Organizations answered, so the whole wall has settled once its badge is + // up — all five counts land in one `setCounts`. + expect(await badge('hub-card-organizations', '2 organizations')).toBeInTheDocument(); + expect(countBadge('hub-card-users', 'users')).toBeNull(); + // Spelled out, because `0 users` is the exact string this issue is about. + expect(screen.queryByText('0 users')).not.toBeInTheDocument(); + }); + + it('blanks only the card that failed and leaves its neighbours their real counts', async () => { + // The issue's most reachable scenario, and the one an administrator is + // most likely to act on: someone who may open the hub but has no read + // permission on `sys_audit_log`. The backend answers 403, the adapter + // rethrows (it absorbs 404s only), and this card used to read "0 entries" + // — an audit log that looks empty to the person auditing it. + state.failures.sys_audit_log = Object.assign(new Error('Forbidden'), { + status: 403, + }); + renderHub(); + + await badge('hub-card-users', '3 users'); + expect(countBadge('hub-card-audit-log', 'entries')).toBeNull(); + // Single-card isolation — the reason the `.catch` stayed on each call + // instead of moving out around the `Promise.all`. The other four answered, + // so they still show their numbers. + expect(countBadge('hub-card-users', 'users')).toHaveTextContent('3 users'); + expect(countBadge('hub-card-organizations', 'organizations')).toHaveTextContent( + '2 organizations', + ); + expect(countBadge('hub-card-positions', 'positions')).toHaveTextContent('4 positions'); + expect(countBadge('hub-card-permissions', 'permissions')).toHaveTextContent( + '0 permissions', + ); + }); + + it('keeps `0` for the two things that really are zero, and blanks only the failure', async () => { + // All three rows of the issue's behaviour matrix in one render. Only the + // third moves; the first two are the adapter's contract and stay as they + // are (whether Permissions should be riding on row two at all is + // objectui#3655, not this change): + // sys_audit_log registered, genuinely empty -> `0 entries` + // sys_permission unregistered, adapter resolves empty -> `0 permissions` + // sys_position 403, adapter rethrows -> no badge + state.registry.sys_audit_log = []; + state.failures.sys_position = Object.assign(new Error('Forbidden'), { status: 403 }); + renderHub(); + + await badge('hub-card-users', '3 users'); + expect(countBadge('hub-card-audit-log', 'entries')).toHaveTextContent('0 entries'); + expect(countBadge('hub-card-permissions', 'permissions')).toHaveTextContent( + '0 permissions', + ); + expect(countBadge('hub-card-positions', 'positions')).toBeNull(); }); + + it('shows no counts at all when every lookup fails, and still renders the hub', async () => { + // Offline, or the backend down. Every counted card loses its badge rather + // than reporting a system with nothing in it; the cards that never carried + // a count are unaffected and every link still works. + for (const objectName of QUERIED_OBJECT_NAMES) { + state.failures[objectName] = Object.assign(new Error('Failed to fetch'), { + status: 0, + }); + } + renderHub(); + + await settleWithoutBadges(); + for (const [testId, label] of COUNTED_CARDS) { + expect(countBadge(testId, label)).toBeNull(); + } + expect(screen.getByTestId('hub-card-settings')).toBeInTheDocument(); + }); + + // No case is written for `fetchCounts`'s OUTER `catch`. It is reachable only + // by a synchronous throw from `dataSource.find` (the per-call `.catch`es keep + // `Promise.all` from ever rejecting), and on that path the counts are still + // at their initial `null` — so an assertion that "no card shows a number" + // would pass whatever the outer catch did, including nothing. It would be + // green for an empty reason rather than because the page is right, so the + // fix stayed where the outcome is actually written, and so did the tests. });