From e4b9a618ae968f3b51f00d99cd4a3a324f76e324 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 07:00:40 +0000 Subject: [PATCH] fix(console): approvals drawer summary renders the record page's field faces The request drawer's record summary read labels from the snapshot (`payload_labels` or a title-cased key) and printed values through a type-blind formatter, so option values, dates and currency showed as stored. It now reads the request object's field declarations and draws each declared field through the record page's own cell dispatch (`resolveCellRendererType` and `getCellRenderer`), under the declared label. Undeclared snapshot keys and unresolved references take no row. Without metadata the card falls back to the snapshot rendering. The injected `owner_id` column is now dropped as bookkeeping via `isSystemManagedField`, which replaces the page's private six-name list, so an object with its own `owner` field no longer shows two "Owner" rows. Rows are keyed by field key instead of label, which removes React's duplicate-key warning, and any label two rows still share is qualified with the field key. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01FngvPpdrnhHMdHHq6vwwju --- .changeset/11677-approvals-summary-faces.md | 15 + ...alsInboxPage.characterizationPins.test.tsx | 27 +- ...pprovalsInboxPage.hiddenFieldTrim.test.tsx | 23 +- ...ApprovalsInboxPage.rawPayloadGate.test.tsx | 3 +- ...ovalsInboxPage.summaryFaces-11677.test.tsx | 485 ++++++++++++++++++ .../src/pages/system/ApprovalsInboxPage.tsx | 287 +++++++++-- 6 files changed, 783 insertions(+), 57 deletions(-) create mode 100644 .changeset/11677-approvals-summary-faces.md create mode 100644 apps/console/src/pages/system/ApprovalsInboxPage.summaryFaces-11677.test.tsx diff --git a/.changeset/11677-approvals-summary-faces.md b/.changeset/11677-approvals-summary-faces.md new file mode 100644 index 0000000000..aba889b577 --- /dev/null +++ b/.changeset/11677-approvals-summary-faces.md @@ -0,0 +1,15 @@ +--- +'@object-ui/console': patch +--- + +The approvals inbox's request drawer shows its record summary the way the record page shows the record (objectui#11677). + +The summary card now reads the field declarations of the request's object. It asks the same cached metadata read the drawer already makes for `hidden: true`, so no network request is added. With those declarations: + +- **Labels.** Each row takes its field's declared label, translated as the record page translates it, instead of the server's snapshot label or a title-cased key. +- **Values.** Each value is drawn by the record page's own cell for its field type. An option shows its label (`Sent`, `EMEA` instead of `sent`, `emea`), a date shows as the date cell shows it, and a currency field shows in its currency. A lookup still shows the record title the server resolved. The lead amount at the top of the card follows the same rule. A `summary` roll-up shows as the record page shows it: the plain stored number, without digit grouping. +- **Rows.** A snapshot key the object does not declare gets no row. Neither does a reference the server could not resolve to a title, because its value is an id. + +If the object's metadata cannot be read, the card renders the snapshot as before. + +Two rows never share a label. The card now drops the platform's injected ownership column `owner_id` (label "Owner") as bookkeeping, the same way the default list columns do, so an object that also declares its own `owner` field no longer shows two "Owner" rows. If two remaining fields still share a label, each of those rows adds its field key in parentheses. The rows are keyed by field key rather than by label, so React no longer warns about two children with the same key. diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.characterizationPins.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.characterizationPins.test.tsx index 284dfed87e..402428877f 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.characterizationPins.test.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.characterizationPins.test.tsx @@ -18,7 +18,7 @@ * plus the "Flow-initiated" cell, instead of a bare person icon + em dash * (#2803). * 3. **Record-card copy and emphasis** — `prettifyKey()` drops a trailing - * `id` token (`owner_id` → "Owner") and `decisionAmountEntry()` promotes + * `id` token (`vendor_id` → "Vendor") and `decisionAmountEntry()` promotes * the decision amount to a lead figure, excluded from the generic field * grid so it renders exactly once (#2803). * @@ -41,7 +41,7 @@ * two people to conflate; * - a **`flow:`-prefixed submitter id** with no `submitter_name` — the shape * a real flow-initiated request arrives in; - * - a payload carrying **both `owner_id` and `amount`** — `owner_id` proves + * - a payload carrying **both `vendor_id` and `amount`** — `vendor_id` proves * the id-token trim (a bare `id` key, or one with a resolvable non-`_id` * name, would not exercise `prettifyKey`'s trim branch at all), `amount` * proves both the lead-figure promotion and the single-render exclusion. @@ -71,9 +71,9 @@ const APP = 'com.objectstack.account'; // all imports) — so the two display-value fixtures live INSIDE it and are // destructured out below, rather than being plain top-level `const`s the // hoisted block would close over before they existed. -const { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, OWNER_DISPLAY, AMOUNT_DISPLAY } = vi.hoisted(() => { - /** The server-resolved display value for `owner_id` — never opaque-dropped. */ - const OWNER_DISPLAY = 'Jordan Lee'; +const { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, VENDOR_DISPLAY, AMOUNT_DISPLAY } = vi.hoisted(() => { + /** The server-resolved display value for `vendor_id` — never opaque-dropped. */ + const VENDOR_DISPLAY = 'Northwind Labs'; /** The server-formatted decision amount, as `payload_display` would send it. */ const AMOUNT_DISPLAY = 'USD 42,000.00'; @@ -100,11 +100,14 @@ const { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, OWNER_DISPLAY, A // Case 3 fixture: an `_id`-suffixed key (prettifyKey's trim branch) and // an amount-like key (decisionAmountEntry's lead figure), together. payload: { - owner_id: 'usr_9f3ma2xk1pqz88a', + // `owner_id` stood here until objectui#11677. It is the platform's + // injected ownership column, which the card now drops as bookkeeping + // (`isSystemManagedField`), so it never reaches `prettifyKey`'s trim. + vendor_id: 'vnd_9f3ma2xk1pqz88a', amount: 42000, }, payload_display: { - owner_id: OWNER_DISPLAY, + vendor_id: VENDOR_DISPLAY, amount: AMOUNT_DISPLAY, }, }; @@ -129,7 +132,7 @@ const { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, OWNER_DISPLAY, A language: 'en', }; - return { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, OWNER_DISPLAY, AMOUNT_DISPLAY }; + return { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, VENDOR_DISPLAY, AMOUNT_DISPLAY }; }); vi.mock('@object-ui/i18n', async (importOriginal) => ({ @@ -228,13 +231,13 @@ describe('Approvals drawer — a flow-initiated request names its origin (object }); describe('Approvals drawer — record-card copy and emphasis (objectui#2803, #6395)', () => { - it('reads `owner_id` as "Owner" and leads with the decision amount exactly once', async () => { + it('reads `vendor_id` as "Vendor" and leads with the decision amount exactly once', async () => { const dialog = await openedDrawer(); // prettifyKey: the trailing `id` token is dropped, so the field label - // reads "Owner", not "Owner Id". - expect(within(dialog).getByText('Owner')).toBeInTheDocument(); - expect(within(dialog).getByText(OWNER_DISPLAY)).toBeInTheDocument(); + // reads "Vendor", not "Vendor Id". + expect(within(dialog).getByText('Vendor')).toBeInTheDocument(); + expect(within(dialog).getByText(VENDOR_DISPLAY)).toBeInTheDocument(); // decisionAmountEntry: the amount is promoted to the lead figure AND // excluded from the generic field grid, so its display value renders diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.hiddenFieldTrim.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.hiddenFieldTrim.test.tsx index d88f2528f5..9b29cd9bb1 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.hiddenFieldTrim.test.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.hiddenFieldTrim.test.tsx @@ -194,6 +194,15 @@ async function openedDrawer(): Promise { return dialog; } +/** + * Every snapshot key declared as a text field. Since objectui#11677 a snapshot + * key the object's metadata does not declare takes no row once that metadata + * has answered, so a fixture that wants a key on the card declares it. + */ +const EVERY_PAYLOAD_FIELD: Record> = Object.fromEntries( + Object.keys(ROW.payload as Record).map((k) => [k, { type: 'text' }]), +); + /** The card really rendered its business content — run by every denial case. */ function expectSummaryCardIntact(dialog: HTMLElement): void { for (const value of SIBLING_VALUES) { @@ -216,11 +225,7 @@ afterEach(cleanup); describe('Approvals drawer summary card — `hidden: true` trim (objectui#5565)', () => { it('drops the hidden field and promotes the seventh into its slot', async () => { getObjectSchema.mockResolvedValue({ - fields: { - subject: { type: 'text' }, - diagnosis_code: { type: 'text', hidden: true }, - justification: { type: 'text' }, - }, + fields: { ...EVERY_PAYLOAD_FIELD, diagnosis_code: { type: 'text', hidden: true } }, }); const dialog = await openedDrawer(); @@ -238,9 +243,7 @@ describe('Approvals drawer summary card — `hidden: true` trim (objectui#5565)' }); it('COUNTER-PROBE: same fixture, same position, nothing declared hidden — it renders', async () => { - getObjectSchema.mockResolvedValue({ - fields: { subject: { type: 'text' }, diagnosis_code: { type: 'text' } }, - }); + getObjectSchema.mockResolvedValue({ fields: EVERY_PAYLOAD_FIELD }); const dialog = await openedDrawer(); await waitFor(() => expect(getObjectSchema).toHaveBeenCalledWith(OBJECT)); @@ -277,7 +280,7 @@ describe('Approvals drawer lead amount — same card, same trim (objectui#5565)' it('does not lead with an amount field the object declares hidden', async () => { useAmountFixture(); - getObjectSchema.mockResolvedValue({ fields: { total_amount: { hidden: true } } }); + getObjectSchema.mockResolvedValue({ fields: { total_amount: { hidden: true }, subject: { type: 'text' } } }); const dialog = await openedDrawer(); await waitFor(() => expect(getObjectSchema).toHaveBeenCalledWith(OBJECT)); @@ -290,7 +293,7 @@ describe('Approvals drawer lead amount — same card, same trim (objectui#5565)' it('COUNTER-PROBE: the same amount, undeclared, still leads the card', async () => { useAmountFixture(); - getObjectSchema.mockResolvedValue({ fields: { total_amount: {} } }); + getObjectSchema.mockResolvedValue({ fields: { total_amount: {}, subject: { type: 'text' } } }); const dialog = await openedDrawer(); await waitFor(() => expect(getObjectSchema).toHaveBeenCalledWith(OBJECT)); diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.rawPayloadGate.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.rawPayloadGate.test.tsx index f5bd2d52fe..8409f6e328 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.rawPayloadGate.test.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.rawPayloadGate.test.tsx @@ -27,7 +27,8 @@ * * ## The witnesses * - * `created_by` and `organization_id` are in the page's `PAYLOAD_SYSTEM_KEYS`, + * `created_by` and `organization_id` are bookkeeping columns to the page's + * `isBookkeepingKey` (`@object-ui/types`' `isSystemManagedField`, objectui#11677), * so the summary card already drops them: their values can reach the DOM only * through the raw panel. That makes them exact witnesses for "the panel * rendered", not proxies. (Both are also long enough to trip the summary's diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.summaryFaces-11677.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.summaryFaces-11677.test.tsx new file mode 100644 index 0000000000..6bb1e29447 --- /dev/null +++ b/apps/console/src/pages/system/ApprovalsInboxPage.summaryFaces-11677.test.tsx @@ -0,0 +1,485 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Approvals drawer — the record summary card renders the object's field + * metadata: its labels, and each value through the record page's own cell + * face (objectui#11677). + * + * ## The defect, as the 2026-10-06 showcase dogfood measured it + * + * The summary card titled every row with `payload_labels[key] ?? prettifyKey(key)` + * and printed every value through `formatPayloadValue`, which knows no field + * type. So "Invoice Dual Sign-off" read `Status sent` and `Region emea` (the + * stored option values, where the invoice list shows the "Sent" / "EMEA" + * labels) and `Issued On 2026-07-23` (the stored date string). Its card also + * carried two rows labelled "Owner": the author's `owner` text field and the + * platform-injected `owner_id` lookup — both DECLARE the label "Owner", and the + * row `key` was that label, so React logged "two children with the same key". + * + * ## What the two fixtures are + * + * The two requests the dogfood opened. Their SHAPE was read from a live + * showcase boot: `GET /api/v1/approvals/requests` and + * `GET /api/v1/meta/object/:name`. That covers the snapshot key order (the + * injected system columns first), the nulls, the hydrated `account`, + * `payload_display` resolving `owner_id` alone, and `payload_labels` giving + * both `owner` and `owner_id` the label "Owner". The metadata read answers + * `total` as "Total" (the showcase translation bundle) while `payload_labels` + * says "Subtotal" (the object's own label). The VALUES are the invoice the + * dogfood opened: `INV-1010`, `emea`, issued 2026-07-23. A fresh boot picks a + * different `sent` invoice. + * + * ⚠️ One reading in the card was corrected by the fixture, not carried into + * it: the expense card's `Approved 0` is not a boolean. `showcase_expense_report` + * declares no boolean at all; "Approved" is the label of `approved_amount`, a + * `summary` roll-up whose value for the demo report is 0. Its record-page face + * is the summary cell, and that is what is pinned below. + * + * ## How "the same face" is asserted + * + * Every value is compared with what the record page's own dispatch draws for + * the same declaration and the same stored value — `getCellRenderer( + * resolveCellRendererType(def))` from `@object-ui/fields`, the chain + * `DetailSection` renders a field through — rendered standalone in this same + * environment. Literal anchors (`EMEA`, `Sent`, the raw strings' absence) sit + * beside each comparison, so a face that renders nothing cannot pass by + * agreeing with an oracle that also renders nothing. + * + * ## Why `setup.access` + * + * Same reason as `ApprovalsInboxPage.hiddenFieldTrim.test.tsx`: a real grant + * that is not platform-admin-only, so the raw-JSON panel (objectui#5553) never + * renders and the summary card is the only door under test. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, within, waitFor } from '@testing-library/react'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; +import { MePermissionsProvider, type MePermissionsResponse } from '@object-ui/permissions'; +import { getCellRenderer, resolveCellRendererType } from '@object-ui/fields'; + +const APP = 'com.example.showcase'; + +const { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, INVOICE, EXPENSE, SCHEMAS } = vi.hoisted(() => { + /** The system columns the registry injects on a business object. */ + const injected = { + owner_id: { type: 'lookup', reference: 'sys_user', label: 'Owner', readonly: false, system: true }, + owning_business_unit_id: { + type: 'lookup', reference: 'sys_business_unit', label: 'Owning Business Unit', + hidden: true, readonly: true, system: true, + }, + created_at: { type: 'datetime', label: 'Created At', readonly: true, system: true }, + created_by: { type: 'lookup', reference: 'sys_user', label: 'Created By', readonly: true, system: true }, + updated_at: { type: 'datetime', label: 'Last Modified At', readonly: true, system: true }, + updated_by: { type: 'lookup', reference: 'sys_user', label: 'Last Modified By', readonly: true, system: true }, + }; + + const SCHEMAS: Record = { + showcase_invoice: { + name: 'showcase_invoice', + label: 'Invoice', + fields: { + ...injected, + name: { type: 'text', label: 'Invoice Number' }, + account: { type: 'lookup', reference: 'showcase_account', label: 'Account' }, + contact: { type: 'lookup', reference: 'showcase_contact', label: 'Contact' }, + owner: { type: 'text', label: 'Owner' }, + region: { + type: 'select', label: 'Region', + options: [ + { label: 'AMER', value: 'amer' }, + { label: 'EMEA', value: 'emea' }, + { label: 'APAC', value: 'apac' }, + ], + }, + status: { + type: 'select', label: 'Status', + options: [ + { label: 'Draft', value: 'draft', color: '#94A3B8' }, + { label: 'Sent', value: 'sent', color: '#3B82F6' }, + { label: 'Paid', value: 'paid', color: '#10B981' }, + { label: 'Void', value: 'void', color: '#EF4444' }, + ], + }, + issued_on: { type: 'date', label: 'Issued On' }, + tax_rate: { type: 'number', label: 'Tax Rate (%)' }, + paid_on: { type: 'date', label: 'Paid On' }, + // The metadata read serves the translation bundle's label; the + // server's `payload_labels` carries the object's own "Subtotal". + total: { type: 'summary', label: 'Total' }, + }, + }, + showcase_expense_report: { + name: 'showcase_expense_report', + label: 'Expense Report', + fields: { + ...injected, + name: { type: 'text', label: 'Report Title' }, + employee: { type: 'text', label: 'Employee' }, + status: { + type: 'select', label: 'Status', + options: [ + { label: 'Draft', value: 'draft' }, + { label: 'Submitted', value: 'submitted' }, + { label: 'Approved', value: 'approved' }, + { label: 'Reimbursed', value: 'reimbursed' }, + ], + }, + submitted_on: { type: 'date', label: 'Submitted On' }, + total_amount: { type: 'summary', label: 'Total' }, + approved_amount: { type: 'summary', label: 'Approved' }, + reimbursable_amount: { type: 'summary', label: 'Reimbursable' }, + line_count: { type: 'summary', label: 'Lines' }, + rejected_count: { type: 'summary', label: 'Rejected' }, + over_limit_count: { type: 'summary', label: 'Over $500' }, + }, + }, + }; + + /** The label per payload key the server sends (`resolveFieldLabels`). */ + const labelsOf = (object: string, payload: Record) => { + const fields = (SCHEMAS[object] as { fields: Record }).fields; + const out: Record = {}; + for (const k of Object.keys(payload)) if (fields[k]?.label) out[k] = fields[k].label as string; + return out; + }; + + /** The injected columns, first in the snapshot, as the live read returned them. */ + const head = (id: string) => ({ + id, + created_at: '2026-10-06T06:44:23.359Z', + updated_at: '2026-10-06T06:44:24.469Z', + organization_id: null, + created_by: null, + updated_by: null, + owner_id: 'rOrg4X8TWQKqZDDLH3dH7JDhSZ69dEwm', + owning_business_unit_id: null, + }); + + const invoicePayload = { + ...head('8XdUnlTpGrRx6nqZ'), + name: 'INV-1010', + account: { id: 'da67WkM9ylVrUi96', name: 'Fabrikam', status: 'active' }, + contact: null, + owner: 'grace@example.com', + region: 'emea', + status: 'sent', + issued_on: '2026-07-23', + tax_rate: 20, + paid_on: null, + total: 0, + }; + + const expensePayload = { + ...head('H7m0glScJ9bWJGrJ'), + name: 'EXP-DEMO', + employee: 'Grace Hopper', + status: 'submitted', + submitted_on: '2026-10-05', + total_amount: 8900, + approved_amount: 0, + reimbursable_amount: 8900, + line_count: 4, + rejected_count: 0, + over_limit_count: 4, + }; + + const base = { + status: 'pending', + pending_approvers: ['u_1', 'u_2'], + submitter_id: 'u_9', + submitter_name: 'Dev Admin', + submitted_at: '2026-10-06T05:00:00.000Z', + }; + + const INVOICE: Record = { + ...base, + id: 'req_invoice', + process_name: 'showcase_invoice_signoff', + process_label: 'Invoice Dual Sign-off', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: invoicePayload.id, + record_title: 'INV-1010', + payload: invoicePayload, + payload_labels: { ...labelsOf('showcase_invoice', invoicePayload), total: 'Subtotal' }, + payload_display: { owner_id: 'Dev Admin' }, + }; + + const EXPENSE: Record = { + ...base, + id: 'req_expense', + process_name: 'showcase_committee_quorum', + process_label: 'High-Value Expense — Committee Quorum', + object_name: 'showcase_expense_report', + object_label: 'Expense Report', + record_id: expensePayload.id, + record_title: 'EXP-DEMO', + payload: expensePayload, + payload_labels: labelsOf('showcase_expense_report', expensePayload), + payload_display: { owner_id: 'Dev Admin' }, + }; + + const getObjectSchema = vi.fn(async (name: string): Promise => SCHEMAS[name] ?? { fields: {} }); + + const approvalsApiStub = { + listRequests: vi.fn(async () => ({ data: [INVOICE], total: 1 })), + getRequest: vi.fn(async () => ({ data: INVOICE })), + listActions: vi.fn(async () => ({ data: [] })), + approve: vi.fn(async () => ({ data: INVOICE, finalized: true })), + reject: vi.fn(async () => ({ data: INVOICE, finalized: true })), + }; + + // STABLE singletons: a mocked hook handing back a fresh object per render + // re-runs the page's load effect forever and the drawer never settles. + const ADAPTER = { find: vi.fn(async () => ({ data: [] })), getObjectSchema }; + const AUTH = { user: { id: 'u_1', email: 'approver@example.com' } }; + const I18N = { + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + language: 'en', + }; + + return { approvalsApiStub, getObjectSchema, ADAPTER, AUTH, I18N, INVOICE, EXPENSE, SCHEMAS }; +}); + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => I18N, +})); + +vi.mock('@object-ui/auth', async (importOriginal) => { + const authFetch = vi.fn(async () => new Response('{}', { status: 200 })); + return { + ...(await importOriginal>()), + useAuth: () => AUTH, + createAuthenticatedFetch: () => authFetch, + TokenStorage: { get: () => null }, + }; +}); + +vi.mock('@object-ui/app-shell', async (importOriginal) => ({ + ...(await importOriginal>()), + useAdapter: () => ADAPTER, + DeclaredActionsBar: () => null, + isViaOverrideRow: () => false, +})); + +vi.mock('../../services/approvalsApi', async (importOriginal) => ({ + ...(await importOriginal>()), + approvalsApi: approvalsApiStub, +})); + +// Imported after the mocks so the page picks them up. +import { ApprovalsInboxPage } from './ApprovalsInboxPage'; + +function permissionsPayload(): MePermissionsResponse { + return { + authenticated: true, + userId: 'u_1', + tenantId: 't_1', + roles: [], + permissionSets: [], + objects: {}, + fields: {}, + systemPermissions: ['setup.access'], + }; +} + +/** Every `console.error` the page logged — React's duplicate-key warning lands here. */ +let errors: string[] = []; + +beforeEach(() => { + errors = []; + vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + errors.push(args.map((a) => String(a)).join(' ')); + }); + getObjectSchema.mockClear(); + getObjectSchema.mockImplementation(async (name: string) => SCHEMAS[name] ?? { fields: {} }); + for (const fn of Object.values(approvalsApiStub)) fn.mockClear(); +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +function useRequest(row: Record) { + approvalsApiStub.listRequests.mockResolvedValue({ data: [row], total: 1 }); + approvalsApiStub.getRequest.mockResolvedValue({ data: row }); +} + +/** Open the drawer the way a notification opens it — the `?request=` deep link. */ +async function openedDrawer(row: Record): Promise { + render( + + + + } /> + + + , + ); + const dialog = await screen.findByRole('dialog'); + await within(dialog).findByText(String(row.process_label)); + return dialog; +} + +/** The summary card's rows, as `{ field key → { label, value } }`. */ +function summaryRows(dialog: HTMLElement): Map { + const out = new Map(); + for (const row of dialog.querySelectorAll('[data-summary-field]')) { + const [label, value] = Array.from(row.children); + out.set(row.getAttribute('data-summary-field') as string, { + label: (label?.textContent ?? '').trim(), + value: (value?.textContent ?? '').trim(), + }); + } + return out; +} + +/** + * What the record page draws for this declaration and this stored value — the + * `DetailSection` dispatch, rendered standalone in this environment. + */ +function recordPageFace(object: string, field: string, value: unknown): string { + const def = { ...(SCHEMAS[object] as { fields: Record> }).fields[field], name: field }; + const Cell = getCellRenderer(resolveCellRendererType(def as { type?: string; format?: string })); + const { container, unmount } = render(); + const text = (container.textContent ?? '').trim(); + unmount(); + return text; +} + +const duplicateKeyWarnings = () => errors.filter((e) => /same key/i.test(e)); + +describe('Approvals drawer summary — field metadata faces (objectui#11677)', () => { + it('Invoice Dual Sign-off: option labels and the date face, under the declared labels', async () => { + useRequest(INVOICE); + const dialog = await openedDrawer(INVOICE); + // Settlement: the option LABEL can only appear once the schema answered. + await waitFor(() => expect(summaryRows(dialog).get('region')?.value).toBe('EMEA')); + const rows = summaryRows(dialog); + + expect(rows.get('region')).toEqual({ label: 'Region', value: recordPageFace('showcase_invoice', 'region', 'emea') }); + expect(rows.get('status')).toEqual({ label: 'Status', value: 'Sent' }); + expect(rows.get('status')?.value).toBe(recordPageFace('showcase_invoice', 'status', 'sent')); + expect(rows.get('issued_on')?.label).toBe('Issued On'); + expect(rows.get('issued_on')?.value).toBe(recordPageFace('showcase_invoice', 'issued_on', '2026-07-23')); + expect(rows.get('name')).toEqual({ label: 'Invoice Number', value: 'INV-1010' }); + expect(rows.get('owner')).toEqual({ label: 'Owner', value: 'grace@example.com' }); + + // The raw stored values are gone from the card. + for (const raw of ['emea', 'sent', '2026-07-23']) { + expect(within(dialog).queryByText(raw)).not.toBeInTheDocument(); + } + }); + + it('Invoice Dual Sign-off: one "Owner", and no duplicate-key warning', async () => { + useRequest(INVOICE); + const dialog = await openedDrawer(INVOICE); + await waitFor(() => expect(summaryRows(dialog).get('region')?.value).toBe('EMEA')); + const labels = [...summaryRows(dialog).values()].map((r) => r.label); + + expect(labels.filter((l) => l === 'Owner')).toHaveLength(1); + expect(new Set(labels).size).toBe(labels.length); + // `owner_id` is the platform's injected ownership column — bookkeeping, not + // a business field of the record — so the card does not spend a row on it. + expect(summaryRows(dialog).has('owner_id')).toBe(false); + expect(duplicateKeyWarnings()).toEqual([]); + }); + + it('High-Value Expense — Committee Quorum: "Approved" is the summary face of `approved_amount`', async () => { + useRequest(EXPENSE); + const dialog = await openedDrawer(EXPENSE); + await waitFor(() => expect(summaryRows(dialog).get('status')?.value).toBe('Submitted')); + const rows = summaryRows(dialog); + + expect(rows.get('approved_amount')).toEqual({ + label: 'Approved', + value: recordPageFace('showcase_expense_report', 'approved_amount', 0), + }); + expect(rows.get('status')?.value).toBe(recordPageFace('showcase_expense_report', 'status', 'submitted')); + expect(rows.get('submitted_on')?.value).toBe(recordPageFace('showcase_expense_report', 'submitted_on', '2026-10-05')); + expect(within(dialog).queryByText('2026-10-05')).not.toBeInTheDocument(); + + const labels = [...rows.values()].map((r) => r.label); + expect(new Set(labels).size).toBe(labels.length); + expect(duplicateKeyWarnings()).toEqual([]); + }); + + it('keeps the lead amount under its declared label, through the same face', async () => { + useRequest(EXPENSE); + const dialog = await openedDrawer(EXPENSE); + await waitFor(() => expect(summaryRows(dialog).get('status')?.value).toBe('Submitted')); + + const lead = dialog.querySelector('[data-summary-lead]'); + expect(lead?.getAttribute('data-summary-lead')).toBe('total_amount'); + expect(lead?.children[0]?.textContent).toBe('Total'); + expect((lead?.children[1]?.textContent ?? '').trim()) + .toBe(recordPageFace('showcase_expense_report', 'total_amount', 8900)); + // Led once, never again as an ordinary row. + expect(summaryRows(dialog).has('total_amount')).toBe(false); + }); + + it('labels the lead from the metadata read, as the record page does — not from `payload_labels`', async () => { + useRequest(INVOICE); + const dialog = await openedDrawer(INVOICE); + await waitFor(() => expect(summaryRows(dialog).get('region')?.value).toBe('EMEA')); + + const lead = dialog.querySelector('[data-summary-lead]'); + expect(lead?.getAttribute('data-summary-lead')).toBe('total'); + // The record page reads "Total" for this field; the snapshot's server + // label ("Subtotal") is what the card showed before. + expect(lead?.children[0]?.textContent).toBe('Total'); + expect((lead?.children[1]?.textContent ?? '').trim()).toBe(recordPageFace('showcase_invoice', 'total', 0)); + }); + + it('a payload key the object does not declare takes no row once the schema answered', async () => { + const row = { + ...INVOICE, + payload: { ...(INVOICE.payload as Record), legacy_note: 'snapshot-era column' }, + }; + useRequest(row); + const dialog = await openedDrawer(row); + await waitFor(() => expect(summaryRows(dialog).get('region')?.value).toBe('EMEA')); + + expect(summaryRows(dialog).has('legacy_note')).toBe(false); + expect(within(dialog).queryByText('snapshot-era column')).not.toBeInTheDocument(); + }); + + it('two DECLARED fields sharing one label are told apart, and keyed apart', async () => { + const schema = SCHEMAS.showcase_invoice as { fields: Record }; + getObjectSchema.mockImplementation(async (name: string) => (name === 'showcase_invoice' + ? { ...schema, fields: { ...schema.fields, region: { ...(schema.fields.region as object), label: 'Status' } } } + : SCHEMAS[name])); + useRequest(INVOICE); + const dialog = await openedDrawer(INVOICE); + await waitFor(() => expect(summaryRows(dialog).get('region')?.value).toBe('EMEA')); + const rows = summaryRows(dialog); + + const labels = [...rows.values()].map((r) => r.label); + expect(new Set(labels).size).toBe(labels.length); + expect(rows.get('region')?.label).not.toBe(rows.get('status')?.label); + // Both still carry the declared label they share. + expect(rows.get('region')?.label).toContain('Status'); + expect(rows.get('status')?.label).toContain('Status'); + expect(duplicateKeyWarnings()).toEqual([]); + }); + + it('FAILS OPEN: a source that cannot describe the object renders the snapshot as before, still one label per row', async () => { + getObjectSchema.mockRejectedValue(Object.assign(new Error('Forbidden'), { status: 403 })); + useRequest(INVOICE); + const dialog = await openedDrawer(INVOICE); + await waitFor(() => expect(getObjectSchema).toHaveBeenCalledWith('showcase_invoice')); + await waitFor(() => expect(summaryRows(dialog).get('region')?.value).toBe('emea')); + const rows = summaryRows(dialog); + + // Unknown metadata: the server's labels and the stored text, as today. + expect(rows.get('region')).toEqual({ label: 'Region', value: 'emea' }); + expect(rows.get('owner')).toEqual({ label: 'Owner', value: 'grace@example.com' }); + const labels = [...rows.values()].map((r) => r.label); + expect(labels.filter((l) => l === 'Owner')).toHaveLength(1); + expect(new Set(labels).size).toBe(labels.length); + expect(duplicateKeyWarnings()).toEqual([]); + }); +}); diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index 131528a125..ad56146727 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -24,9 +24,9 @@ * an input is focused. */ -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from 'react'; import { Link, useParams, useSearchParams } from 'react-router-dom'; -import { DeclaredActionsBar, isViaOverrideRow } from '@object-ui/app-shell'; +import { DeclaredActionsBar, isViaOverrideRow, useAdapter } from '@object-ui/app-shell'; import { createAuthenticatedFetch } from '@object-ui/auth'; import { Button, @@ -76,7 +76,10 @@ import { import { toast } from 'sonner'; import { useAuth } from '@object-ui/auth'; import { usePermissions } from '@object-ui/permissions'; -import { useDisplayLocale, useObjectTranslation } from '@object-ui/i18n'; +import { useDisplayLocale, useObjectTranslation, useSafeFieldLabel } from '@object-ui/i18n'; +import { getCellRenderer, resolveCellRendererType } from '@object-ui/fields'; +import { isExpandableFieldType } from '@object-ui/core'; +import { isSystemManagedField, type FieldMetadata } from '@object-ui/types'; import { APPROVAL_STATUS_LABELS } from '@objectstack/spec/contracts'; import { CheckCircle2, @@ -115,7 +118,7 @@ import { isUnresolvableRecordReference, useUnresolvableRecordReferenceLabel, } from './unresolvableRecordReference'; -import { useHiddenFieldsByObject } from './hiddenFields'; +import { useHiddenFieldsByObject, type HiddenFieldsSource } from './hiddenFields'; import { holdsStudioAccess } from '../../components/studioEntry'; type TabKey = 'pending' | 'submitted' | 'all'; @@ -341,9 +344,141 @@ function slaState(dueAt: string | null | undefined, now: number): { overdue: boo return { overdue: due < now, ms: Math.abs(now - due) }; } -const PAYLOAD_SYSTEM_KEYS = new Set([ - 'id', 'created_at', 'updated_at', 'created_by', 'updated_by', 'organization_id', -]); +/** + * Whether a snapshot key is a framework-managed column rather than a business + * field of the record — `@object-ui/types`' `isSystemManagedField`, the one + * classifier the default list columns, record pickers and related lists use + * to keep bookkeeping out of business surfaces. It reads the spec `system` + * flag the registry stamps on every column it injects, then its name list. + * + * This card used a private six-name copy of that list. The copy left out + * `owner_id`, the injected ownership column, and every business object's + * metadata labels it "Owner". So an object with its own `owner` field + * (showcase's invoice keeps a text `owner` as its row-level-security anchor) + * showed two rows labelled "Owner" (objectui#11677). + */ +function isBookkeepingKey(key: string, def: FieldDef | undefined): boolean { + return isSystemManagedField(key, def as { system?: boolean } | undefined); +} + +/** + * One field's declaration as the object's metadata serves it. The index below + * adds `name` to each one, as the record page's field defs carry it. + */ +type FieldDef = Readonly>; + +/** An object's field declarations, keyed by field name. */ +type FieldDefs = Readonly>; + +/** + * Index an object schema's `fields` by name (objectui#11677). Reads both + * shapes the platform serves: the record shape (`{ name: def }`) and the array + * shape (`[{ name, ...def }]`), as `hiddenFieldNames` does. + * + * Returns `null` when the schema declares no field at all, and `null` is the + * UNKNOWN answer. A real object always declares fields: the registry injects + * its system columns on every business object. So an empty answer says the + * source did not describe the object, and the card stays on today's snapshot + * rendering rather than dropping every row as undeclared. + */ +function indexFieldDefs(schema: unknown): FieldDefs | null { + const fields = (schema as { fields?: unknown } | null | undefined)?.fields; + if (!fields || typeof fields !== 'object') return null; + const entries: Array<[unknown, unknown]> = Array.isArray(fields) + ? fields.map((def) => [(def as { name?: unknown } | null)?.name, def]) + : Object.entries(fields as Record); + const out: Record = {}; + for (const [name, def] of entries) { + if (typeof name !== 'string' || name === '' || !def || typeof def !== 'object') continue; + out[name] = { ...(def as Record), name }; + } + return Object.keys(out).length > 0 ? out : null; +} + +/** + * The field declarations of the open request's object (objectui#11677). + * Returns `null` until the read answers, and `null` again when it cannot + * answer: no `getObjectSchema`, a transport error, or a principal without + * metadata read. + * + * It is the same `getObjectSchema` read `useHiddenFieldsByObject` makes for + * this object. It lands on the adapter's `MetadataCache`, which de-duplicates + * in-flight reads, so it adds no round trip. `null` fails OPEN, in the same + * direction as that hook: the card renders today's snapshot rows, because a + * metadata error must not empty the approver's decision surface. + */ +function useRequestFieldDefs(objectName: string | null | undefined): FieldDefs | null { + const adapter = useAdapter() as HiddenFieldsSource | null | undefined; + const [answer, setAnswer] = useState<{ object: string; defs: FieldDefs | null } | null>(null); + useEffect(() => { + if (!objectName || typeof adapter?.getObjectSchema !== 'function') return; + let cancelled = false; + adapter.getObjectSchema(objectName).then( + (schema) => { if (!cancelled) setAnswer({ object: objectName, defs: indexFieldDefs(schema) }); }, + // Deliberately silent, as `readHiddenFields` is: a viewer without + // metadata access hits this on every drawer open. + () => { if (!cancelled) setAnswer({ object: objectName, defs: null }); }, + ); + return () => { cancelled = true; }; + }, [adapter, objectName]); + // An answer for the PREVIOUS request's object is never applied to this one. + return answer !== null && answer.object === objectName ? answer.defs : null; +} + +/** + * A declared field's stored value, drawn the way the record page draws it + * (objectui#11677). `resolveCellRendererType` → `getCellRenderer` is the chain + * `DetailSection` renders a field through, and the option labels are + * translated as it translates them. So the card shows `Sent` and `EMEA` where + * the record page does, a date as the date cell shows it, and a currency + * field in its currency. + * + * ⛔ Not a second formatter. This passes the declaration and the stored value + * to the record page's own cell and adds nothing. A face that looks wrong here + * looks wrong on the record page as well, and the fix belongs in the cell. + * + * Module scope on purpose, for the same reason as the row cells below + * (objectui#5348). + */ +function RecordFieldFace({ objectName, def, value }: { objectName: string; def: FieldDef; value: unknown }) { + const { translateOptions } = useSafeFieldLabel(); + const options = def.options; + const field = Array.isArray(options) && options.length > 0 + ? { ...def, options: translateOptions(objectName, def.name as string, options) } + : def; + const Cell = getCellRenderer(resolveCellRendererType(field as { type?: string; format?: string })); + // eslint-disable-next-line react-hooks/static-components -- getCellRenderer returns the registry's module-level renderer for the type, not a component created during render + return ; +} + +/** One row of the drawer's summary card. `key` is the snapshot key, so it is unique. */ +interface SummaryRow { + key: string; + label: string; + value: ReactNode; + /** Hover text, set only when the value is plain text. */ + title?: string; +} + +/** + * Two rows never share a label (objectui#11677). The labels come from the + * object's metadata, and metadata can give two fields the same label: an + * author field and an injected column, or two author fields. Two identical + * labels on one card read as one field shown twice. Every row whose label is + * shared therefore names its field key as well, and none of them is dropped. + * + * Field keys are unique within one snapshot, so the qualified labels are + * unique. If a qualified label still matches another row's plain label (an + * author who labelled a field `Owner (owner_id)`), every row is qualified. + */ +function withDistinctLabels(rows: SummaryRow[]): SummaryRow[] { + const qualify = (pick: (r: SummaryRow) => boolean) => + rows.map((r) => (pick(r) ? { ...r, label: `${r.label} (${r.key})` } : r)); + const count = new Map(); + for (const r of rows) count.set(r.label, (count.get(r.label) ?? 0) + 1); + const once = qualify((r) => (count.get(r.label) ?? 0) > 1); + return new Set(once.map((r) => r.label)).size === once.length ? once : qualify(() => true); +} function prettifyKey(k: string): string { const tokens = k.split('_').filter(Boolean); @@ -390,34 +525,77 @@ const OPAQUE_ID_RE = /^[A-Za-z0-9_-]{15,}$/; * An empty `hiddenKeys` means "nothing known to be hidden" — including the case * where the metadata read has not answered — and renders today's card. See * `hiddenFields.ts` on why this presentation filter fails open. + * + * `meta` is the object's field declarations (objectui#11677). With it, the + * card follows the record page: + * - each row takes its field's declared label, translated as the record page + * translates it, and never a title-cased key; + * - each value is drawn by {@link RecordFieldFace}, the record page's own + * cell, unless the server resolved a display value for it; + * - a snapshot key the object does not declare gets no row, because it has + * no label and no face to show; + * - a reference the server could not resolve gets no row, because its value + * is an id. + * Without `meta` (not answered, failed, or undescribed) the card falls back to + * the snapshot as it rendered before: the server's labels, then the + * title-cased key, and the stored text. In both modes two rows never share a + * label ({@link withDistinctLabels}). */ function payloadSummary( payload: unknown, locale: string, - display?: Record, - labels?: Record, - max = 6, - excludeKey?: string, - hiddenKeys?: ReadonlySet, -): Array<[string, string]> { + display: Record | undefined, + labels: Record | undefined, + max: number, + excludeKey: string | undefined, + hiddenKeys: ReadonlySet | undefined, + meta: SummaryMeta | null, +): SummaryRow[] { if (!payload || typeof payload !== 'object' || Array.isArray(payload)) return []; - const out: Array<[string, string]> = []; + const out: SummaryRow[] = []; for (const [k, v] of Object.entries(payload as Record)) { - if (PAYLOAD_SYSTEM_KEYS.has(k)) continue; + const def = meta?.defs[k]; + if (isBookkeepingKey(k, def)) continue; if (hiddenKeys?.has(k)) continue; // author declared `hidden: true` (#5565) if (excludeKey && k === excludeKey) continue; // shown as the lead amount if (v == null || typeof v === 'object') continue; if (String(v).trim() === '') continue; const resolved = display?.[k]; - if (!resolved && typeof v === 'string' && OPAQUE_ID_RE.test(v.trim()) && !/^\d+$/.test(v.trim())) { - continue; + if (meta) { + if (!def) continue; // not a field of this object (#11677) + if (!resolved && isExpandableFieldType(def)) continue; // an unresolved reference id + out.push({ + key: k, + label: declaredLabel(meta, k, def), + value: resolved ?? , + title: resolved, + }); + } else { + if (!resolved && typeof v === 'string' && OPAQUE_ID_RE.test(v.trim()) && !/^\d+$/.test(v.trim())) { + continue; + } + // Prefer the server-resolved field label (the target object's own label, + // already localized for a single-locale project) over a title-cased key. + const text = resolved ?? formatPayloadValue(k, v, locale); + out.push({ key: k, label: labels?.[k] ?? prettifyKey(k), value: text, title: text }); } - // Prefer the server-resolved field label (the target object's own label, - // already localized for a single-locale project) over a title-cased key. - out.push([labels?.[k] ?? prettifyKey(k), resolved ?? formatPayloadValue(k, v, locale)]); if (out.length >= max) break; } - return out; + return withDistinctLabels(out); +} + +/** What {@link payloadSummary} needs to render the record page's faces. */ +interface SummaryMeta { + objectName: string; + defs: FieldDefs; + /** `useSafeFieldLabel().fieldLabel` — the record page's label translator. */ + fieldLabel: (objectName: string, fieldName: string, fallback: string) => string; +} + +/** A declared field's label, as the record page reads it: `field.label || field.name`, translated. */ +function declaredLabel(meta: SummaryMeta, key: string, def: FieldDef): string { + const label = typeof def.label === 'string' && def.label !== '' ? def.label : key; + return meta.fieldLabel(meta.objectName, key, label); } /** @@ -445,17 +623,24 @@ const AMOUNT_KEY_RE = /(amount|total|price|value|cost|sum|budget|salary|fee|reve * An empty set means "nothing known to be hidden" — including the case where * the metadata read has not answered — and yields today's amount. See * `hiddenFields.ts` on why this presentation filter fails open. + * + * `defs` is the object's field declarations, which only the drawer has + * (objectui#11677). When given, a key the object does not declare is not a + * candidate, by the same rule as the drawer's field grid. The queue passes + * nothing, which keeps today's choice. */ function decisionAmountEntry( r: ApprovalRequestRow, hiddenKeys: ReadonlySet, locale: string, + defs: FieldDefs | null = null, ): { key: string; label: string; value: number; display: string } | null { const payload = r.payload; if (!payload || typeof payload !== 'object' || Array.isArray(payload)) return null; for (const [k, v] of Object.entries(payload as Record)) { - if (PAYLOAD_SYSTEM_KEYS.has(k)) continue; + if (isBookkeepingKey(k, defs?.[k])) continue; if (hiddenKeys.has(k)) continue; // author declared `hidden: true` (#5565) + if (defs && !defs[k]) continue; // not a field of this object (#11677) if (!AMOUNT_KEY_RE.test(k)) continue; const num = typeof v === 'number' ? v @@ -905,6 +1090,14 @@ export function ApprovalsInboxPage() { ); const hiddenFields = useHiddenFieldsByObject(hiddenFieldObjects); const hiddenPayloadKeys = hiddenFields.forObject(selected?.object_name); + /** + * objectui#11677 — the open request's field declarations, so the drawer's + * summary card shows the record page's labels and value faces. `null` until + * the read answers or when it fails; the card then renders the snapshot as + * before. See `useRequestFieldDefs` for the cost and the fail-open choice. + */ + const requestFieldDefs = useRequestFieldDefs(selected?.object_name); + const { fieldLabel } = useSafeFieldLabel(); // Approve/reject/reassign/send-back/… are server-declared actions rendered by // DeclaredActionsBar (objectui#2697 + framework#3300); their param dialog // collects the comment and — since the shared upload-widget renderer (#2700/ @@ -2072,8 +2265,31 @@ export function ApprovalsInboxPage() { // the same `hidden` trim (objectui#5565) — otherwise a hidden // amount-like field would simply move from the field grid to the // bold lead figure at the top of the very card being fixed. - const drawerAmount = decisionAmountEntry(selected, hiddenPayloadKeys, displayLocale); - const summary = payloadSummary(selected.payload, displayLocale, selected.payload_display, selected.payload_labels, 6, drawerAmount?.key, hiddenPayloadKeys); + // objectui#11677: with the object's field declarations, both + // halves take the declared label and the record page's face. + const summaryMeta: SummaryMeta | null = requestFieldDefs + ? { objectName: selected.object_name, defs: requestFieldDefs, fieldLabel } + : null; + const drawerAmount = decisionAmountEntry(selected, hiddenPayloadKeys, displayLocale, requestFieldDefs); + const leadDef = drawerAmount ? summaryMeta?.defs[drawerAmount.key] : undefined; + const lead: SummaryRow | null = !drawerAmount + ? null + : summaryMeta && leadDef + ? { + key: drawerAmount.key, + label: declaredLabel(summaryMeta, drawerAmount.key, leadDef), + value: selected.payload_display?.[drawerAmount.key] + ?? ( + )[drawerAmount.key]} + /> + ), + title: selected.payload_display?.[drawerAmount.key], + } + : { key: drawerAmount.key, label: drawerAmount.label, value: drawerAmount.display, title: drawerAmount.display }; + const summary = payloadSummary(selected.payload, displayLocale, selected.payload_display, selected.payload_labels, 6, drawerAmount?.key, hiddenPayloadKeys, summaryMeta); return ( @@ -2137,20 +2353,23 @@ export function ApprovalsInboxPage() { - {drawerAmount && ( -
-
{drawerAmount.label}
-
- {drawerAmount.display} + {lead && ( +
+
{lead.label}
+
+ {lead.value}
)} {summary.length > 0 && ( -
- {summary.map(([k, v]) => ( -
-
{k}
-
{v}
+
+ {/* Keyed by FIELD key (objectui#11677): it was keyed by + the label, and two fields labelled "Owner" logged + React's duplicate-key warning on every drawer render. */} + {summary.map((row) => ( +
+
{row.label}
+
{row.value}
))}