diff --git a/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx new file mode 100644 index 00000000000..be79bdf52d0 --- /dev/null +++ b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The package door's save merges onto the PENDING DRAFT when one exists, not + * onto the published body (objectstack#26837). + * + * ## The defect these pins reach + * + * `doSave`'s package-door arm re-reads the set and merges this package's slice + * onto that read with `mergePermissionSlice`, which takes only the facets the + * editor can author from the edited body and everything else from the base. + * The base was `client.layered(...).effective` — the PUBLISHED body — while the + * save itself writes the package DRAFT. So a draft another door wrote (the + * card's reproduction: `PUT …?mode=draft&package=P` with `description` and + * `packageId`) lost those keys on the next Studio edit: the load displayed the + * draft, one checkbox autosaved, and the PUT carried the published values (or + * none) in their place. 200, no error. + * + * ## Why the pin drives the REAL editor + * + * `mergePermissionSlice` is correct for any base it is handed — its own pins + * (`permission-slice.authoredKeys.test.ts`, `permission-slice.facetDrift.test.ts`) + * call it directly and could never see this. The defect is the CHOICE of base, + * which only `doSave` makes, so these pins run the real page, the real load and + * the real autosave (objectui#11787: the package door has no Save button). + * + * ## The arms, and what each one's absence is read against + * + * 1. A pending draft whose non-authored keys differ from the published body: + * every one of them is on the wire byte-for-byte, out-of-scope rows come + * from the draft, and the edited row lands. + * 2. No pending draft: the base is still the published `effective`, so the + * published body's own keys and other packages' rows are on the wire — + * the control that the draft arm is not "the edited body, wholesale". + * 3. The save-time DRAFT read rejects: the save is refused on the same + * channel as objectui#9420's layered refusal, `client.save` is never + * called, and the control `getDraftCalls === 2` proves the read ran. + */ + +import '@testing-library/jest-dom/vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +/** + * The published record. `a_account` is the door's own package; `b_order` and + * `b_order.total` are another package's grants. + */ +const PUBLISHED = { + name: 'sales_perms', + label: 'Sales', + description: 'Published description', + objects: { + a_account: { allowRead: true, allowCreate: true }, + b_order: { allowRead: true }, + }, + fields: { 'b_order.total': { readable: true, editable: false } }, +}; + +/** + * The pending package draft another door wrote — every key the editor cannot + * author differs from {@link PUBLISHED}, and so does an out-of-scope row, so a + * value taken from the wrong base cannot pass for the right one. + */ +const DRAFT = { + name: 'sales_perms', + label: 'Sales', + description: 'Set through the API', + packageId: 'app.a', + objects: { + a_account: { allowRead: true, allowCreate: true }, + b_order: { allowRead: true, allowEdit: true, viewAllRecords: true }, + }, + fields: { 'b_order.total': { readable: true, editable: true } }, +}; + +/** The facets `EDITOR_AUTHORED_KEYS` does not name — the ones the base supplies. */ +const NON_AUTHORED = ['description', 'packageId'] as const; + +/** Distinctive slice of the refusal the author sees on the page's error strip. */ +const REFUSAL = /could not be re-read/; + +interface Server { + saved: Array>; + savedOpts: Array | undefined>; + /** `getDraft` calls seen so far — #1 is the load, #2 is `doSave`'s re-read. */ + getDraftCalls: number; +} + +function freshServer(): Server { + return { saved: [], savedOpts: [], getDraftCalls: 0 }; +} + +function makeClient(server: Server, opts: { draft: Record | null; rejectSaveTimeDraft?: boolean }) { + return { + layered: async () => ({ effective: PUBLISHED, code: null, overlay: null, overlayScope: null }), + getDraft: async () => { + server.getDraftCalls += 1; + if (opts.rejectSaveTimeDraft && server.getDraftCalls > 1) throw new Error('draft read failed'); + return opts.draft ? { type: 'permission', name: 'sales_perms', item: opts.draft, version: 'v1' } : null; + }, + // The package declares `a_account` only. + list: async (type: string) => (type === 'object' ? [{ item: { name: 'a_account' } }] : []), + get: async () => null, + save: async ( + _type: string, + _name: string, + payload: Record, + saveOpts?: Record, + ) => { + server.saved.push(payload); + server.savedOpts.push(saveOpts); + return payload; + }, + } as any; +} + +let clientImpl: any; + +vi.mock('./useMetadata', () => ({ + useMetadataClient: () => clientImpl, + useMetadataTypes: () => ({ + loading: false, + error: null, + entries: [{ type: 'permission', label: 'Permission', allowEnvironmentOverlay: true }], + }), +})); + +vi.mock('./AssignedUsersSection', () => ({ AssignedUsersSection: () => null })); + +import { PermissionMatrixEditPage } from './PermissionMatrixEditor'; + +afterEach(cleanup); + +function renderPackageDoor() { + return render( + + + , + ); +} + +/** One ordinary edit: revoke every grant on the door's own `a_account` row. */ +function editAccountRow() { + const row = screen.getByText('a_account').closest('tr')!; + fireEvent.click(within(row).getByRole('button', { name: 'None' })); +} + +/** The package door autosaves after the shared pause (1.5s). */ +const AUTOSAVE = { timeout: 4000 }; + +describe('PermissionMatrixEditPage — the PACKAGE door saves onto the pending draft (objectstack#26837)', () => { + it('keeps every key the editor does not author from the pending draft, byte-for-byte', async () => { + const server = freshServer(); + clientImpl = makeClient(server, { draft: DRAFT }); + renderPackageDoor(); + await screen.findByText('a_account'); + + editAccountRow(); + + await waitFor(() => expect(server.saved).toHaveLength(1), AUTOSAVE); + const body = server.saved[0] as any; + + // The edited row lands… + expect(body.objects.a_account).toEqual({}); + // …and the draft's non-authored keys survive, not the published ones. + for (const key of NON_AUTHORED) expect(body[key]).toEqual(DRAFT[key]); + // Out-of-scope rows come from the draft too: it is what this save replaces. + expect(body.objects.b_order).toEqual(DRAFT.objects.b_order); + expect(body.fields).toEqual(DRAFT.fields); + // CONTROL: the save-time draft read ran (#1 is the load). + expect(server.getDraftCalls).toBe(2); + expect(server.savedOpts[0]).toMatchObject({ mode: 'draft', packageId: 'app.a' }); + expect(screen.queryByText(REFUSAL)).toBeNull(); + }); + + it('CONTROL — with no pending draft the base is still the published body', async () => { + const server = freshServer(); + clientImpl = makeClient(server, { draft: null }); + renderPackageDoor(); + await screen.findByText('a_account'); + + editAccountRow(); + + await waitFor(() => expect(server.saved).toHaveLength(1), AUTOSAVE); + const body = server.saved[0] as any; + + expect(body.objects.a_account).toEqual({}); + expect(body.description).toBe(PUBLISHED.description); + expect('packageId' in body).toBe(false); + expect(body.objects.b_order).toEqual(PUBLISHED.objects.b_order); + expect(body.fields).toEqual(PUBLISHED.fields); + expect(screen.queryByText(REFUSAL)).toBeNull(); + }); + + it('REFUSES the save when the save-time draft read rejects, rather than fall back to the published body', async () => { + const server = freshServer(); + clientImpl = makeClient(server, { draft: DRAFT, rejectSaveTimeDraft: true }); + renderPackageDoor(); + await screen.findByText('a_account'); + + editAccountRow(); + + // Settle on either arm — the refusal on screen, or a PUT landing. + await waitFor( + () => expect(server.saved.length > 0 || screen.queryByText(REFUSAL) !== null).toBe(true), + AUTOSAVE, + ); + + // CONTROL: the save-time draft read really ran and really rejected. + expect(server.getDraftCalls).toBe(2); + expect(server.saved).toEqual([]); + expect(server.savedOpts).toEqual([]); + // The same error strip objectui#9420's layered refusal renders into. + expect(screen.getByText(REFUSAL)).toBeInTheDocument(); + expect(screen.queryByTestId('perm-autosaving')).toBeNull(); + }); +}); diff --git a/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx index 3fb2f78251f..ae1632ba59e 100644 --- a/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx +++ b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx @@ -266,6 +266,19 @@ function isArtifactBackedLayer( ); } +/** + * The editable body of a pending package draft, as `readDraftForSave` served + * it (the `{ type, name, item }` envelope, or a bare body), or `null` for "no + * draft". Read decorations come off here, at the unwrap, so neither the load + * nor the save's merge base carries `_diagnostics` / `_draft` (objectui#8181). + */ +function pendingDraftBody( + answer: { item?: PermissionSetDraft } | PermissionSetDraft | null, +): PermissionSetDraft | null { + if (!answer) return null; + return stripReadDecorations((answer as { item?: PermissionSetDraft }).item ?? answer) as PermissionSetDraft; +} + /** Localized short label for an OWD value; falls back to the raw value. */ function owdLabel(t: (k: string) => string, value: string): string { const key: Record = { @@ -867,13 +880,10 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved, // below re-bases on a fresh RAW `layered` read, which drops them — but // its `.catch(() => null)` arm falls back to this very body and // spreads it into `client.save`, so a failed layered read used to put - // `_diagnostics` / `_draft` on the wire. Strip at the unwrap, which is - // the one place the served envelope becomes an editable draft. - const draftBody = pendingDraft - ? (stripReadDecorations( - (pendingDraft as any).item ?? pendingDraft, - ) as PermissionSetDraft) - : null; + // `_diagnostics` / `_draft` on the wire. Strip at the unwrap, + // `pendingDraftBody`, the one function through which a served envelope + // becomes an editable body — here and in `doSave`'s merge base. + const draftBody = pendingDraftBody(pendingDraft); // Draft wins over the published baseline for display (D6). const effective: PermissionSetDraft = (draftBody ?? lay?.effective ?? lay?.code ?? { name, objects: {} }) as PermissionSetDraft; @@ -1280,19 +1290,36 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved, // purpose: a set that exists only as a package draft (what the Studio // Access pillar's "+ New" creates) has no published rows for anyone to // lose, and refusing there would block its first save. + // + // objectstack#26837 — and this door's save WRITES the pending draft, so + // when one exists it is the base, not the published body: the facets + // the editor cannot author (`description`, `packageId`, any key another + // door wrote) come from the draft, exactly as the load displayed it. + // Read through the load's own door, at the address this save writes. + // The pin the load recorded is left alone, so a draft that moved since + // the load is still refused as a version conflict. `readDraftForSave` + // resolves a REJECTED read as no answer AND no pin, while a read that + // found no draft pins `If-None-Match: *` — so the pair tells a failure + // apart from "no draft", and a failure refuses exactly like the + // layered re-read: falling back to the published body would drop the + // draft's values with a 200. let rereadFailed = false; - const fresh = await client - .layered(type, payload.name) - // Kept on `.catch` so the rejection can never escape as an unhandled - // one; the refusal is raised below, inside this `try`'s own body, and - // reaches the author on the same error channel a failed - // `client.save` already uses. - .catch(() => { - rereadFailed = true; - return null; - }); + const [fresh, draftRead] = await Promise.all([ + client + .layered(type, payload.name) + // Kept on `.catch` so the rejection can never escape as an + // unhandled one; the refusal is raised below, inside this `try`'s + // own body, and reaches the author on the same error channel a + // failed `client.save` already uses. + .catch(() => { + rereadFailed = true; + return null; + }), + readDraftForSave<{ item?: PermissionSetDraft } | PermissionSetDraft>(client, type, payload.name, packageId), + ]); + if (draftRead.answer === null && draftRead.pin === null) rereadFailed = true; if (rereadFailed) throw new Error(t('perm.save.rereadFailed')); - const base = (fresh?.effective ?? payload) as PermissionSetDraft; + const base = (pendingDraftBody(draftRead.answer) ?? fresh?.effective ?? payload) as PermissionSetDraft; toSave = mergePermissionSlice(base, payload, scope); } // ADR-0086 P2 (D6/D7). Package door → the set is metadata: write a DRAFT