diff --git a/ordinis-admin-ui/src/components/schema/DictionaryEditorDialog.tsx b/ordinis-admin-ui/src/components/schema/DictionaryEditorDialog.tsx index 7e7de59..b4d2456 100644 --- a/ordinis-admin-ui/src/components/schema/DictionaryEditorDialog.tsx +++ b/ordinis-admin-ui/src/components/schema/DictionaryEditorDialog.tsx @@ -52,6 +52,42 @@ const SCOPE_OPTIONS = (t: (k: string) => string): SelectOption[] => [ type EditorTab = 'metadata' | 'schema' | 'preview' | 'events' +/** + * Decide whether the editor's Save button should be blocked because the + * change has to go through the schema-draft workflow instead of inline PUT. + * Mirrors the three backend gate clauses in DictionaryDefinitionService + * exactly so the UX matches the server contract. + * + * Returns false (don't block) for create-mode, for non-approval-required + * dicts, and for pure metadata edits (display name / description / locales + * etc.) that leave schema + approval config alone. + * + * Exported for unit testability — the three-clause logic is the load-bearing + * piece, easier to verify in isolation than mounting the full editor with + * its query and mutation deps. + */ +export function computeRequiresDraftFlow({ + isEdit, + initialApprovalRequired, + initialMinRole, + currentApprovalRequired, + currentMinRole, + schemaChangeCount, +}: { + isEdit: boolean + initialApprovalRequired: boolean + initialMinRole: string | null + currentApprovalRequired: boolean + currentMinRole: string | null + schemaChangeCount: number +}): boolean { + if (!isEdit || !initialApprovalRequired) return false + const schemaChanged = schemaChangeCount > 0 + const approvalToggledOff = !currentApprovalRequired + const minRoleChanged = initialMinRole !== currentMinRole + return schemaChanged || approvalToggledOff || minRoleChanged +} + export const DictionaryEditorDialog = ({ open, mode, onClose, onSuccess }: Props) => { const { t } = useTranslation() const createMut = useCreateDictionary() @@ -150,17 +186,17 @@ export const DictionaryEditorDialog = ({ open, mode, onClose, onSuccess }: Props // 422 approval_required, but it's much better UX to disable Save up front // and tell the user to open a draft instead. Mirror the three backend // gate clauses exactly so the UX matches the server contract. - const wasApprovalRequired = - Boolean((initial as { approvalRequired?: boolean } | null)?.approvalRequired) - const initialMinRole = - ((initial as { approvalMinRole?: string } | null)?.approvalMinRole ?? '') || null - const currentMinRole = approvalMinRole.trim() === '' ? null : approvalMinRole.trim() - const schemaChangedFromInitial = isEdit && changes.length > 0 - const approvalToggledOff = isEdit && wasApprovalRequired && !approvalRequired - const minRoleChanged = isEdit && wasApprovalRequired && initialMinRole !== currentMinRole - const requiresDraftFlow = - isEdit && wasApprovalRequired - && (schemaChangedFromInitial || approvalToggledOff || minRoleChanged) + const requiresDraftFlow = computeRequiresDraftFlow({ + isEdit, + initialApprovalRequired: Boolean( + (initial as { approvalRequired?: boolean } | null)?.approvalRequired, + ), + initialMinRole: + ((initial as { approvalMinRole?: string } | null)?.approvalMinRole ?? '') || null, + currentApprovalRequired: approvalRequired, + currentMinRole: approvalMinRole.trim() === '' ? null : approvalMinRole.trim(), + schemaChangeCount: changes.length, + }) const activeMutation = isEdit ? updateMut : createMut const serverError = serverErrorMessage(activeMutation.error) @@ -490,9 +526,13 @@ export const DictionaryEditorDialog = ({ open, mode, onClose, onSuccess }: Props detail={initial} initialProposedSchema={schemaJson} onCreated={() => { + // Cascade close: dismiss the draft modal AND the editor. Once + // the draft is submitted, the in-memory edit in the editor is + // stale by design — the draft is now the source of truth until + // a reviewer decides. Leaving the editor open would let the + // maker silently mutate further state that can't go anywhere + // (Save still disabled until the draft resolves). setDraftHandoffOpen(false) - // Close the editor too — the maker's done editing live, the draft - // is now the source of truth until a reviewer decides. onClose() }} /> diff --git a/ordinis-admin-ui/src/components/schema/computeRequiresDraftFlow.test.ts b/ordinis-admin-ui/src/components/schema/computeRequiresDraftFlow.test.ts new file mode 100644 index 0000000..bf8af2d --- /dev/null +++ b/ordinis-admin-ui/src/components/schema/computeRequiresDraftFlow.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from 'vitest' +import { computeRequiresDraftFlow } from './DictionaryEditorDialog' + +const base = { + isEdit: true, + initialApprovalRequired: true, + initialMinRole: 'ordinis:reviewer' as string | null, + currentApprovalRequired: true, + currentMinRole: 'ordinis:reviewer' as string | null, + schemaChangeCount: 0, +} + +describe('computeRequiresDraftFlow', () => { + it('returns false in create mode (no initial dict)', () => { + expect(computeRequiresDraftFlow({ ...base, isEdit: false })).toBe(false) + }) + + it('returns false for non-approval-required dicts even with schema changes', () => { + expect( + computeRequiresDraftFlow({ + ...base, + initialApprovalRequired: false, + schemaChangeCount: 5, + }), + ).toBe(false) + }) + + it('returns false for pure metadata edits on approval-required dict', () => { + // Editing display name / description / locales doesn't touch schema or + // approval config; backend allows inline. UX should NOT block Save here. + expect(computeRequiresDraftFlow(base)).toBe(false) + }) + + it('returns true when schema changed on approval-required dict', () => { + expect( + computeRequiresDraftFlow({ ...base, schemaChangeCount: 1 }), + ).toBe(true) + }) + + it('returns true when approvalRequired toggled off (backdoor block)', () => { + expect( + computeRequiresDraftFlow({ ...base, currentApprovalRequired: false }), + ).toBe(true) + }) + + it('returns true when approvalMinRole changed', () => { + expect( + computeRequiresDraftFlow({ ...base, currentMinRole: 'ordinis:admin' }), + ).toBe(true) + }) + + it('returns true when approvalMinRole cleared', () => { + // Edge case from MR !182 audit: clearing the min-role would let any + // generic reviewer approve restricted-dict drafts. Must route through + // draft workflow. + expect( + computeRequiresDraftFlow({ ...base, currentMinRole: null }), + ).toBe(true) + }) + + it('handles all three clauses together (worst-case schema rewrite + governance flip)', () => { + expect( + computeRequiresDraftFlow({ + ...base, + schemaChangeCount: 3, + currentApprovalRequired: false, + currentMinRole: null, + }), + ).toBe(true) + }) +}) diff --git a/ordinis-admin-ui/src/components/workflow/CreateSchemaDraftModal.tsx b/ordinis-admin-ui/src/components/workflow/CreateSchemaDraftModal.tsx index 7df5241..a435eaf 100644 --- a/ordinis-admin-ui/src/components/workflow/CreateSchemaDraftModal.tsx +++ b/ordinis-admin-ui/src/components/workflow/CreateSchemaDraftModal.tsx @@ -55,6 +55,34 @@ type Props = { * вернёт detail в message. * */ +/** + * Resolve the schema JSON to seed the editor with. Three-tier priority: + * + * 1. edit-mode draft — `editDraft.proposedSchema` + * Maker is revising their own draft after CHANGES_REQUESTED; the + * draft itself is the source of truth. + * + * 2. caller pre-fill — `initialProposedSchema` prop + * DictionaryEditorDialog's inline CTA passes in-progress edits so + * the maker doesn't retype them. + * + * 3. live HEAD — `detailSchemaJson` + * First-time draft on this dict with no pre-fill; start from the + * current published schema. + * + * Exported so the priority can be unit-tested without rendering the + * whole modal (mutations + queries make the render test setup heavy). + */ +export function pickSeedSchema( + editDraft: SchemaDraft | undefined, + initialProposedSchema: unknown, + detailSchemaJson: unknown, +): unknown { + if (editDraft) return editDraft.proposedSchema + if (initialProposedSchema !== undefined) return initialProposedSchema + return detailSchemaJson +} + export function CreateSchemaDraftModal({ open, onClose, @@ -65,17 +93,10 @@ export function CreateSchemaDraftModal({ }: Props) { const { t } = useTranslation() const isEdit = Boolean(editDraft) - // Seed order: edit-mode draft → caller-provided initialProposedSchema → - // live HEAD. Caller pre-fill wins over live HEAD so the maker's in-progress - // edits from DictionaryEditorDialog flow through. const initialJson = useMemo( () => JSON.stringify( - editDraft - ? editDraft.proposedSchema - : initialProposedSchema !== undefined - ? initialProposedSchema - : detail.schemaJson, + pickSeedSchema(editDraft, initialProposedSchema, detail.schemaJson), null, 2, ), diff --git a/ordinis-admin-ui/src/components/workflow/pickSeedSchema.test.ts b/ordinis-admin-ui/src/components/workflow/pickSeedSchema.test.ts new file mode 100644 index 0000000..1e80c5b --- /dev/null +++ b/ordinis-admin-ui/src/components/workflow/pickSeedSchema.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from 'vitest' +import type { SchemaDraft } from '@/api/client' +import { pickSeedSchema } from './CreateSchemaDraftModal' + +const draft = (proposed: unknown): SchemaDraft => + ({ proposedSchema: proposed } as unknown as SchemaDraft) + +describe('pickSeedSchema', () => { + it('edit-mode draft wins over pre-fill and live HEAD', () => { + const seed = pickSeedSchema(draft({ type: 'edit' }), { type: 'pre-fill' }, { type: 'head' }) + expect(seed).toEqual({ type: 'edit' }) + }) + + it('caller pre-fill wins over live HEAD when no edit-mode draft', () => { + const seed = pickSeedSchema(undefined, { type: 'pre-fill' }, { type: 'head' }) + expect(seed).toEqual({ type: 'pre-fill' }) + }) + + it('falls back to live HEAD when neither edit draft nor pre-fill provided', () => { + const seed = pickSeedSchema(undefined, undefined, { type: 'head' }) + expect(seed).toEqual({ type: 'head' }) + }) +}) diff --git a/ordinis-admin-ui/src/i18n.ts b/ordinis-admin-ui/src/i18n.ts index af13827..6e70d0f 100644 --- a/ordinis-admin-ui/src/i18n.ts +++ b/ordinis-admin-ui/src/i18n.ts @@ -128,6 +128,9 @@ i18n 'reviews.drawer.commentPlaceholder': 'Для approve опционально, для reject обязательно', 'schema.approvalRequired.label': 'Требовать approval (maker-checker)', 'schema.approvalRequired.hint': 'Approval Workflow v2. Все CRUD операции пойдут через draft → review → approve. Защита от operator error на критичных справочниках. Существующие маршруты (POST/PUT/DELETE) вернут 409 draft_required с pointer на /drafts API.', + 'schema.approvalRequired.editorBlockTitle': 'Изменения требуют ревью', + 'schema.approvalRequired.editorBlock': 'Этот словарь требует maker-checker review. Откройте черновик — ваши текущие изменения будут предзаполнены, вам нужно только указать причину и отправить на согласование.', + 'schema.approvalRequired.openDraftCta': 'Создать черновик с моими изменениями', 'schema.approvalMinRole.label': 'Минимальная роль ревьюера (опционально)', 'schema.approvalMinRole.hint': 'Например ordinis:internal или ordinis:restricted. Если задано — backend требует чтобы ревьюер имел эту роль (поверх стандартных ordinis:record:reviewer / ordinis:schema:reviewer) для approve / reject / publish. Пусто = достаточно стандартной reviewer role. ordinis:admin проходит всегда.', 'search.title': 'Smart search', @@ -577,8 +580,8 @@ i18n // === Schema review queue panel === 'reviews.tab.records': 'Записи', 'reviews.tab.schemas': 'Схемы', - 'reviews.schema.empty': 'Нет схем на ревью', - 'reviews.schema.emptyDescription': 'Когда maker отправит schema draft на ревью, он появится здесь.', + 'reviews.schema.empty': 'Очередь пуста', + 'reviews.schema.emptyDescription': 'Схемы на ревью появятся здесь.', 'reviews.schema.title': 'Изменения схем', 'reviews.schema.label': 'Schema workflow', 'reviews.schema.queueTotal_one': '{{count}} draft ждёт ревью', @@ -843,6 +846,9 @@ i18n 'reviews.drawer.commentPlaceholder': 'Optional for approve, required for reject', 'schema.approvalRequired.label': 'Require approval (maker-checker)', 'schema.approvalRequired.hint': 'Approval Workflow v2. All CRUD goes through draft → review → approve. Protects critical dictionaries from operator error. Existing endpoints (POST/PUT/DELETE) return 409 draft_required pointing to /drafts API.', + 'schema.approvalRequired.editorBlockTitle': 'Changes require review', + 'schema.approvalRequired.editorBlock': 'This dictionary requires maker-checker review. Open a draft — your current edits will be pre-filled, you only need to add a reason and submit for review.', + 'schema.approvalRequired.openDraftCta': 'Create draft with my changes', 'schema.approvalMinRole.label': 'Minimum reviewer role (optional)', 'schema.approvalMinRole.hint': 'e.g. ordinis:internal or ordinis:restricted. When set, backend requires reviewer to hold this role (on top of standard ordinis:record:reviewer / ordinis:schema:reviewer) for approve / reject / publish. Empty = standard reviewer role suffices. ordinis:admin always passes.', 'search.title': 'Smart search', @@ -1272,8 +1278,8 @@ i18n // === Schema review queue panel === 'reviews.tab.records': 'Records', 'reviews.tab.schemas': 'Schemas', - 'reviews.schema.empty': 'No schemas under review', - 'reviews.schema.emptyDescription': 'Once a maker submits a schema draft for review, it will appear here.', + 'reviews.schema.empty': 'Queue is empty', + 'reviews.schema.emptyDescription': 'Schemas under review will appear here.', 'reviews.schema.title': 'Schema changes', 'reviews.schema.label': 'Schema workflow', 'reviews.schema.queueTotal_one': '{{count}} draft awaiting review', diff --git a/ordinis-admin-ui/src/lib/diff.test.ts b/ordinis-admin-ui/src/lib/diff.test.ts new file mode 100644 index 0000000..084cec5 --- /dev/null +++ b/ordinis-admin-ui/src/lib/diff.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest' +import { shallowDiffSummary, isEmptyDiffSummary } from './diff' + +describe('shallowDiffSummary', () => { + it('returns null when both inputs are null', () => { + expect(shallowDiffSummary(null, null)).toBeNull() + expect(shallowDiffSummary(undefined, undefined)).toBeNull() + expect(shallowDiffSummary(null, undefined)).toBeNull() + }) + + it('reports added keys when before is empty/null', () => { + const s = shallowDiffSummary(null, { code: 'A', name: 'Alpha' }) + expect(s).toEqual({ added: ['code', 'name'], removed: [], changed: [] }) + }) + + it('reports removed keys when after is empty/null', () => { + const s = shallowDiffSummary({ code: 'A', name: 'Alpha' }, null) + expect(s).toEqual({ added: [], removed: ['code', 'name'], changed: [] }) + }) + + it('reports changed keys when values differ', () => { + const s = shallowDiffSummary({ code: 'A', count: 1 }, { code: 'A', count: 2 }) + expect(s).toEqual({ added: [], removed: [], changed: ['count'] }) + }) + + it('handles mixed +/-/~ in one call', () => { + const s = shallowDiffSummary( + { code: 'A', count: 1, oldField: 'x' }, + { code: 'A', count: 2, newField: 'y' }, + ) + expect(s).toEqual({ + added: ['newField'], + removed: ['oldField'], + changed: ['count'], + }) + }) + + it('treats nested-object change as one changed entry on the outer key', () => { + const s = shallowDiffSummary( + { code: 'A', meta: { lat: 55, lon: 37 } }, + { code: 'A', meta: { lat: 55, lon: 38 } }, + ) + expect(s).toEqual({ added: [], removed: [], changed: ['meta'] }) + }) + + it('reports empty buckets when payloads are deeply equal', () => { + const s = shallowDiffSummary({ code: 'A', count: 1 }, { code: 'A', count: 1 }) + expect(s).toEqual({ added: [], removed: [], changed: [] }) + expect(s && isEmptyDiffSummary(s)).toBe(true) + }) +}) + +describe('isEmptyDiffSummary', () => { + it('returns true when all three buckets are empty', () => { + expect(isEmptyDiffSummary({ added: [], removed: [], changed: [] })).toBe(true) + }) + + it('returns false when any bucket has entries', () => { + expect(isEmptyDiffSummary({ added: ['x'], removed: [], changed: [] })).toBe(false) + expect(isEmptyDiffSummary({ added: [], removed: ['x'], changed: [] })).toBe(false) + expect(isEmptyDiffSummary({ added: [], removed: [], changed: ['x'] })).toBe(false) + }) +}) diff --git a/ordinis-admin-ui/src/lib/diff.ts b/ordinis-admin-ui/src/lib/diff.ts new file mode 100644 index 0000000..aa1ed69 --- /dev/null +++ b/ordinis-admin-ui/src/lib/diff.ts @@ -0,0 +1,61 @@ +/** + * Shallow JSON-record diff utilities for UI summary chips. + * + *

Lives in `src/lib` (not in a route file) so that callers beyond the + * record-draft ReviewDrawer can reuse it — RecordHistoryDrawer, audit row + * summary, future audit-trail tooltips. Pre-emptive DRY: surfaced during + * the 2026-05-14 eng review of MR !186. + * + *

Shallow on purpose: ordinis business records are flat-ish at the top + * level (business fields, no deep object trees that would require a real + * structural diff). Nested-object changes are reported as a single + * "changed" entry on the outer key. Good enough for "what's the gist" UI + * chips. For full schema-level diff with line-by-line highlighting, see + * `ChangelogDiffModal` + its backend `summary.added/removed/changed` + * payload. + * + *

Equality is checked via `JSON.stringify`. Order-sensitive — relies on + * V8/JSC preserving insertion order for plain objects (true on every + * modern runtime ordinis ships against). Sufficient for record data which + * is always object-literal serialized; not safe for Map/Set/Date/RegExp, + * none of which appear in record payloads. + */ + +export type ShallowDiffSummary = { + added: string[] + removed: string[] + changed: string[] +} + +/** + * @returns null when both inputs are null/undefined (e.g. CLOSE op draft + * with no live record). Caller can skip rendering entirely. + * Otherwise: per-bucket key lists, never null. + */ +export function shallowDiffSummary( + before: Record | null | undefined, + after: Record | null | undefined, +): ShallowDiffSummary | null { + if (!before && !after) return null + const b = before ?? {} + const a = after ?? {} + const added: string[] = [] + const removed: string[] = [] + const changed: string[] = [] + for (const k of Object.keys(a)) { + if (!(k in b)) { + added.push(k) + } else if (JSON.stringify(a[k]) !== JSON.stringify(b[k])) { + changed.push(k) + } + } + for (const k of Object.keys(b)) { + if (!(k in a)) removed.push(k) + } + return { added, removed, changed } +} + +/** Convenience: empty when all three buckets are empty. */ +export function isEmptyDiffSummary(s: ShallowDiffSummary): boolean { + return s.added.length === 0 && s.removed.length === 0 && s.changed.length === 0 +} diff --git a/ordinis-admin-ui/src/routes/reviews.tsx b/ordinis-admin-ui/src/routes/reviews.tsx index 4ea7b1d..bf2d2e8 100644 --- a/ordinis-admin-ui/src/routes/reviews.tsx +++ b/ordinis-admin-ui/src/routes/reviews.tsx @@ -29,6 +29,7 @@ import { useDraft, useLiveRecord, useReviewQueue, useSchemaReviewQueue } from '@ import { useApproveDraft, useRejectDraft } from '@/api/mutations' import type { DraftOperation, DraftResponse } from '@/api/client' import { UserCell } from '@/lib/useUserDisplay' +import { shallowDiffSummary, isEmptyDiffSummary } from '@/lib/diff' export const Route = createFileRoute('/reviews')({ component: ReviewsPage, @@ -41,43 +42,57 @@ const operationVariant = (op: DraftOperation): 'info' | 'success' | 'warning' => } /** - * Shallow diff summary between two record JSON payloads. Reviewer sees three - * counts above the side-by-side dump — "+N added, −M removed, ~K changed" — - * so they don't have to eyeball-diff long records to find the actual edit. + * Diff summary chips for the ReviewDrawer — "+N added (fields), −M removed, + * ~K changed" above the side-by-side JSON panes. Reviewer sees the gist + * without eyeball-diffing long records. * - * Shallow on purpose: ordinis records are flat-ish dicts at the top level - * (business fields, no deep object trees). Nested object changes count as - * one "changed" entry on the outer key. Good enough for reviewer at-a-glance. - * - * Returns null when before/after both null (CLOSE op, or first CREATE) — - * caller can skip the summary row entirely. + * Backed by `shallowDiffSummary` from `@/lib/diff` (extracted there during + * the 2026-05-14 eng review of MR !186 for reuse — RecordHistoryDrawer and + * audit row summaries will plug into the same helper). Memo'd against the + * three actual inputs so it doesn't recompute on unrelated state changes + * (textarea typing, drawer resize, etc.). */ -type DraftDiffSummary = { - added: string[] - removed: string[] - changed: string[] -} -function shallowDiffSummary( - before: Record | null | undefined, - after: Record | null | undefined, -): DraftDiffSummary | null { - if (!before && !after) return null - const b = before ?? {} - const a = after ?? {} - const added: string[] = [] - const removed: string[] = [] - const changed: string[] = [] - for (const k of Object.keys(a)) { - if (!(k in b)) { - added.push(k) - } else if (JSON.stringify(a[k]) !== JSON.stringify(b[k])) { - changed.push(k) - } - } - for (const k of Object.keys(b)) { - if (!(k in a)) removed.push(k) - } - return { added, removed, changed } +function DraftDiffSummary({ + live, + proposed, + operation, +}: { + live: Record | null | undefined + proposed: Record | null | undefined + operation: DraftOperation +}) { + const summary = useMemo(() => { + // CLOSE op has no proposed payload by design — skip without computing. + if (operation === 'CLOSE') return null + return shallowDiffSummary(live, proposed) + }, [live, proposed, operation]) + + if (!summary || isEmptyDiffSummary(summary)) return null + return ( +

+ {summary.added.length > 0 && ( + + + + {summary.added.length} + ({summary.added.join(', ')}) + + )} + {summary.removed.length > 0 && ( + + + {summary.removed.length} + ({summary.removed.join(', ')}) + + )} + {summary.changed.length > 0 && ( + + ~ + {summary.changed.length} + ({summary.changed.join(', ')}) + + )} +
+ ) } /** @@ -591,55 +606,11 @@ function ReviewDrawer({ draftId, onClose }: DrawerProps) { )} - {/* Diff summary chips above the side-by-side dump — reviewer sees - "+1 added, ~2 changed" before eyeballing the JSON. Skipped for - CLOSE op (no proposed) and when both sides are null. */} - {(() => { - const summary = - draft.operation === 'CLOSE' - ? null - : shallowDiffSummary( - liveQ.data?.data as Record | null | undefined, - draft.data as Record | null | undefined, - ) - if (!summary) return null - const empty = - summary.added.length === 0 && - summary.removed.length === 0 && - summary.changed.length === 0 - if (empty) return null - return ( -
- {summary.added.length > 0 && ( - - + - {summary.added.length} - - ({summary.added.join(', ')}) - - - )} - {summary.removed.length > 0 && ( - - - {summary.removed.length} - - ({summary.removed.join(', ')}) - - - )} - {summary.changed.length > 0 && ( - - ~ - {summary.changed.length} - - ({summary.changed.join(', ')}) - - - )} -
- ) - })()} + | null | undefined} + proposed={draft.data as Record | null | undefined} + operation={draft.operation} + />