From 99afb357b1e8c8af06c6f189b31744a3b37c468b Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 3 Sep 2026 12:50:54 +0200 Subject: [PATCH] fix(decisions): feed the detail pages' type picker from the registry #1109 fixed the Decisions and Motions INDEX pickers through CnIndexPage's form-dialog slot and filed the rest as a known gap: CnDetailPage had no such slot, so every detail page that renders the Decision schema kept showing an empty required decisionType picker in its built-in Edit dialog. #1099 emptied properties.decisionType.enum on purpose, and the detail pages had nowhere to splice the vocabulary back in. nextcloud-vue#944 added the slot, deliberately with the same name and the same scope as CnIndexPage's, so one replacement serves both page types. This takes 2.34.0 and wires FOUR surfaces, not the one the gap named: DecisionDetail, MotionDetail, AmendmentDetail and DecisionIntegrations. The last is easy to miss and renders the same form: a `type: "detail"` page with a `config.schema` gets its Edit affordance from CnPageRenderer unconditionally, so the picker was empty there too. All four reuse #1109's DecisionFormDialog and its withDecisionTypeVocabulary() helper rather than a second copy of the vocabulary logic, so the six wirings cannot drift. One behavioural change in that dialog. CnDetailPage's slot `confirm` resolves to `{ success: true, data }` / `{ error }` because a replacement holds the only ref to its own CnFormDialog: the dialog raises `loading` on submit, only setResult() lowers it, and `no-close` is bound to `loading`. A failed edit leaves the form open, so dropping the result strands the user in a modal that can neither retry nor close. settleFormDialogResult() hands the result back and treats "resolved to nothing" as normal, which is what CnIndexPage's confirm still does, so the same component keeps working on the index pages. Tests (tests/vitest/decisionDetailFormDialog.spec.js, 15): - every detail page bound to the decision schema is wired, asserted as an EQUALITY on the page set so a fifth one cannot ship unwired - the picker's negative control is the shipped schema itself: stored decisionType carries no enum and is required, so the built-in dialog renders zero options; passing it through the helper yields the registry vocabulary, and the store's schema is not mutated - settleFormDialogResult closes on success, unlocks on failure, and stays out of the way for a confirm that resolves to nothing or a dialog that unmounted mid-save Verified locally: vitest 28 files / 407 tests green, eslint 0 errors, prettier clean, stylelint 0 errors, manifest Ajv PASS, schema-l10n and l10n parity at baseline, hydra gates v1.11.0 diff-scoped green on the diff (gate-16 and gate-69 findings are pre-existing and untouched here). Co-Authored-By: Claude Opus 5 (1M context) --- package-lock.json | 8 +- package.json | 2 +- src/dialogs/DecisionFormDialog.vue | 43 +++- src/dialogs/formDialogResult.js | 51 ++++ src/manifest.json | 9 + tests/vitest/decisionDetailFormDialog.spec.js | 217 ++++++++++++++++++ 6 files changed, 321 insertions(+), 9 deletions(-) create mode 100644 src/dialogs/formDialogResult.js create mode 100644 tests/vitest/decisionDetailFormDialog.spec.js diff --git a/package-lock.json b/package-lock.json index 205e141c..53a19b10 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "0.1.0", "license": "EUPL-1.2", "dependencies": { - "@conduction/nextcloud-vue": "^2.31.1", + "@conduction/nextcloud-vue": "^2.34.0", "@nextcloud/auth": "^2.6.0", "@nextcloud/axios": "~2.5.2", "@nextcloud/capabilities": "^1.2.1", @@ -562,9 +562,9 @@ } }, "node_modules/@conduction/nextcloud-vue": { - "version": "2.31.1", - "resolved": "https://registry.npmjs.org/@conduction/nextcloud-vue/-/nextcloud-vue-2.31.1.tgz", - "integrity": "sha512-tF1/7yNaBgxj5iHhpNuxVdUrPugfy2ePQZILl7y+fFJTq68tR7WPs8EqPin8aaCBc6ZxgWp1ciyY+NW9rXBMYA==", + "version": "2.34.0", + "resolved": "https://registry.npmjs.org/@conduction/nextcloud-vue/-/nextcloud-vue-2.34.0.tgz", + "integrity": "sha512-btUz646+ftoBZKnuARxTOWaOsUgYz3ih258BjV/IpJC/Zf3VCoUDepNOWFZNC30Uj+8xykMOgUcq/uqk4cUnCw==", "license": "EUPL-1.2", "dependencies": { "@ckpack/vue-color": "^1.6.0", diff --git a/package.json b/package.json index b11d234f..05d4476a 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "extends @nextcloud/browserslist-config" ], "dependencies": { - "@conduction/nextcloud-vue": "^2.31.1", + "@conduction/nextcloud-vue": "^2.34.0", "@nextcloud/auth": "^2.6.0", "@nextcloud/axios": "~2.5.2", "@nextcloud/capabilities": "^1.2.1", diff --git a/src/dialogs/DecisionFormDialog.vue b/src/dialogs/DecisionFormDialog.vue index c1377f1b..3869f131 100644 --- a/src/dialogs/DecisionFormDialog.vue +++ b/src/dialogs/DecisionFormDialog.vue @@ -5,9 +5,13 @@ Dialog: schema-driven create/edit form for Decision objects, with the decisionType picker fed from the registry. - This is a manifest `form-dialog` slot replacement for the Decisions and - Motions index pages (wired via each page's `slots` map in - src/manifest.json). The built-in dialog those pages otherwise render + This is a manifest `form-dialog` slot replacement for every decidiq + surface that renders the Decision schema in a form: the Decisions and + Motions INDEX pages, and the Decision, Motion, Amendment and Decision + integrations DETAIL pages (wired via each page's `slots` map in + src/manifest.json). CnIndexPage and CnDetailPage deliberately name the + slot the same and scope it the same, so one component serves both. + The built-in dialog those pages otherwise render builds its type picker from `properties.decisionType.enum` in the stored schema — and decision-types-as-configuration (#1099) deliberately emptied that enum, making the `decision_types` app config the only @@ -30,10 +34,11 @@ @@ -43,6 +48,7 @@ import { listDecisionTypes, withDecisionTypeVocabulary, } from '../integrations/decisionLink.js' +import { settleFormDialogResult } from './formDialogResult.js' export default { name: 'DecisionFormDialog', @@ -104,5 +110,34 @@ export default { async mounted() { this.decisionTypes = await listDecisionTypes() }, + + methods: { + /** + * Save through the page's own persistence path, then hand the + * outcome back to the dialog that submitted it. + * + * The result matters because this dialog is ours, not the page's: + * CnFormDialog raises `loading` on submit and only `setResult()` + * lowers it, with `no-close` bound to `loading`. On CnDetailPage a + * failed edit leaves the form open, so dropping the result would + * strand the user in a modal that can neither retry nor close. + * CnIndexPage's `confirm` resolves to nothing and closes the dialog + * by flipping `show` instead, which settleFormDialogResult() treats + * as a normal outcome rather than a fault, so this one component + * still serves both pages. + * + * @param {object} formData The submitted form data. + * @param {?object} extra CnFormDialog's second confirm argument + * (extension answers), passed through untouched. + * + * @return {Promise} + * + * @spec openspec/changes/decision-types-as-configuration/specs/decidesk-contract-decision-hub/spec.md + */ + async onConfirm(formData, extra) { + const result = await this.confirm(formData, extra) + settleFormDialogResult(this.$refs.dialog, result) + }, + }, } diff --git a/src/dialogs/formDialogResult.js b/src/dialogs/formDialogResult.js new file mode 100644 index 00000000..dd1ed770 --- /dev/null +++ b/src/dialogs/formDialogResult.js @@ -0,0 +1,51 @@ +/** + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + * + * Result plumbing for a `form-dialog` slot replacement. + * + * A page that replaces its built-in form dialog through the `form-dialog` + * slot renders its own `CnFormDialog`, and the host holds no ref to it. That + * makes the save result the only channel back: `CnFormDialog` raises its + * `loading` flag on submit and ONLY `setResult()` lowers it again, while + * `no-close` is bound to `loading`. A replacement that drops the result + * therefore locks its own modal open, on a successful save as well as on a + * failed one, with no error and nothing in the console. + * + * `CnDetailPage`'s slot `confirm` resolves to `{ success: true, data }` or + * `{ error }` for exactly this reason. `CnIndexPage`'s does not resolve to + * anything yet (nextcloud-vue#944 left that half alone deliberately), and it + * closes its dialog by flipping the slot's `show` instead — so this helper + * has to accept "no result" as a normal outcome rather than a fault, and one + * replacement component can serve both pages. + * + * Kept in a plain .js module because this repo's vitest runs on plain Vite + * with no @vitejs/plugin-vue, so a `.vue` file cannot be imported by a spec. + * The logic that must be tested lives here and the SFC calls it. + * + * @spec openspec/changes/decision-types-as-configuration/specs/decidesk-contract-decision-hub/spec.md + */ + +/** + * Hand a slot `confirm`'s resolved result back to the dialog that submitted it. + * + * @param {?object} dialog The CnFormDialog instance (a `$refs` entry), or null + * when the dialog has already unmounted because `show` went false. + * @param {?object} result What the slot's `confirm` resolved to: + * `{ success: true, data }` / `{ error }` on CnDetailPage, `undefined` on a + * page whose confirm returns nothing. + * + * @return {boolean} True when the dialog was settled, false when there was + * nothing to settle (no dialog, or no result to settle it with). + * + * @spec openspec/changes/decision-types-as-configuration/specs/decidesk-contract-decision-hub/spec.md + */ +export function settleFormDialogResult(dialog, result) { + // `setResult` reads `resultData.success`, so a null / undefined result + // would throw rather than close anything. A page whose confirm resolves + // to nothing closes its dialog itself; leave that path alone. + if (!result || typeof result !== 'object') return false + if (!dialog || typeof dialog.setResult !== 'function') return false + dialog.setResult(result) + return true +} diff --git a/src/manifest.json b/src/manifest.json index d899923e..709e1a00 100644 --- a/src/manifest.json +++ b/src/manifest.json @@ -640,6 +640,9 @@ "route": "/decisions/:id/integrations", "type": "detail", "title": "Decision integrations", + "slots": { + "form-dialog": "DecisionFormDialog" + }, "config": { "register": "decidiq", "schema": "decision", @@ -876,7 +879,9 @@ "id": "MotionDetail", "route": "/motions/:id", "type": "detail", + "_note": "decision-types-as-configuration: same wiring and same reason as DecisionDetail. This page renders the Decision schema too, so its built-in Edit dialog showed an empty decisionType picker until the form-dialog slot (nextcloud-vue#944) let DecisionFormDialog splice the registry vocabulary in.", "slots": { + "form-dialog": "DecisionFormDialog", "widget-motion-amendments": "MotionAmendmentsTab", "widget-motion-votes": "MotionVotesTab", "widget-motion-amendment-order": "MotionAmendmentOrderTab", @@ -960,7 +965,9 @@ }, "documentationUrl": "https://decidiq.conduction.nl" }, + "_note": "decision-types-as-configuration: same wiring and same reason as DecisionDetail. This page renders the Decision schema too, so its built-in Edit dialog showed an empty decisionType picker until the form-dialog slot (nextcloud-vue#944) let DecisionFormDialog splice the registry vocabulary in.", "slots": { + "form-dialog": "DecisionFormDialog", "widget-amend-diff": "AmendmentDiffTab", "widget-amend-parent": "AmendmentParentMotionTab" } @@ -1131,7 +1138,9 @@ "id": "DecisionDetail", "route": "/decisions/:id", "type": "detail", + "_note": "decision-types-as-configuration: this detail page's built-in Edit dialog renders the same Decision schema the index pages do, and #1099 deliberately emptied properties.decisionType.enum (the decision_types app config is the only authority), so its type picker offered nothing and the required field blocked the save. #1109 fixed the two index pages through CnIndexPage's form-dialog slot; CnDetailPage grew the same slot with the same scope in nextcloud-vue#944, so the same DecisionFormDialog is wired here rather than a second copy of the vocabulary logic. The note stays a SIBLING of slots: CnPageRenderer resolves every slots entry as a registry name and warns on one it cannot find.", "slots": { + "form-dialog": "DecisionFormDialog", "widget-decision-lifecycle": "DecisionLifecycleTab", "widget-decision-actions": "ActionItemsSurface", "widget-decision-related": "RelatedDecisionsTab", diff --git a/tests/vitest/decisionDetailFormDialog.spec.js b/tests/vitest/decisionDetailFormDialog.spec.js new file mode 100644 index 00000000..2167a1cc --- /dev/null +++ b/tests/vitest/decisionDetailFormDialog.spec.js @@ -0,0 +1,217 @@ +/** + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + * + * Unit tests for the decision-type picker on decidiq's DETAIL pages. + * + * The defect these pin: #1099 moved the `decisionType` vocabulary out of the + * stored schema's `enum` into the `decision_types` app config, and #1109 + * spliced it back into the two INDEX pages through CnIndexPage's + * `form-dialog` slot. CnDetailPage had no such slot, so every detail page + * that renders the Decision schema kept showing an EMPTY required picker in + * its built-in Edit dialog. nextcloud-vue#944 added the slot with the same + * name and the same scope; these tests hold the four decidiq surfaces that + * needed it to the wiring, and hold the wiring itself to the two things that + * make it work: the schema copy carries the vocabulary, and the save result + * gets back to the dialog that submitted it. + * + * Component mounting is deliberately absent: this repo's vitest runs on plain + * Vite with no @vitejs/plugin-vue, so a `.vue` file cannot be imported by a + * spec (see registerDetailWidgets.spec.js). The logic therefore lives in + * importable .js modules, and the manifest wiring is asserted against + * src/manifest.json itself. + * + * @spec openspec/changes/decision-types-as-configuration/specs/decidesk-contract-decision-hub/spec.md + */ + +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { describe, expect, it, vi } from 'vitest' +import { settleFormDialogResult } from '../../src/dialogs/formDialogResult.js' +import { + FALLBACK_DECISION_TYPES, + withDecisionTypeVocabulary, +} from '../../src/integrations/decisionLink.js' + +// decisionLink.js reaches @nextcloud/axios (and through it @nextcloud/auth, +// which touches `window`) at import time; these tests exercise its pure +// helpers, so the transport is stubbed rather than exercised. +vi.mock('@nextcloud/axios', () => ({ + default: { get: vi.fn(), post: vi.fn() }, +})) + +vi.mock('@nextcloud/l10n', () => ({ + translate: (app, text) => text, +})) + +/** + * Read a repo file relative to this spec. + * + * @param {string} relative Path relative to tests/vitest/. + * + * @return {string} The file contents. + */ +function read(relative) { + return readFileSync(fileURLToPath(new URL(relative, import.meta.url)), 'utf8') +} + +const manifest = JSON.parse(read('../../src/manifest.json')) +const registrySource = read('../../src/registry.js') + +/** Every page that renders the Decision schema through CnDetailPage. */ +const decisionDetailPages = manifest.pages.filter( + (page) => page.type === 'detail' && page.config?.schema === 'decision', +) + +/** + * The Decision schema exactly as the register ships it: what a detail page's + * built-in dialog renders, decisionType enum and all (there is none). + * + * @return {object} A fresh copy of the stored Decision schema. + */ +function storedDecisionSchema() { + return JSON.parse(read('../../lib/Settings/decidesk_register.json')).components + .schemas.Decision +} + +describe('the decision detail pages wire the form-dialog slot', () => { + it('covers every detail page bound to the decision schema', () => { + // Equality, not a subset: a new decision-backed detail page added + // without the wiring has exactly this defect, and the test that only + // checked the known four would stay green while it shipped. + expect(decisionDetailPages.map((page) => page.id).sort()).toEqual([ + 'AmendmentDetail', + 'DecisionDetail', + 'DecisionIntegrations', + 'MotionDetail', + ]) + }) + + it.each(decisionDetailPages.map((page) => page.id))( + '%s replaces its built-in form dialog with DecisionFormDialog', + (id) => { + const page = decisionDetailPages.find((entry) => entry.id === id) + + expect(page.slots?.['form-dialog']).toBe('DecisionFormDialog') + }, + ) + + it('the index pages keep the same wiring, so one component serves both', () => { + const indexPages = manifest.pages.filter( + (page) => page.type === 'index' && page.config?.schema === 'decision', + ) + + expect(indexPages.map((page) => page.id).sort()).toEqual([ + 'Decisions', + 'Motions', + ]) + for (const page of indexPages) { + expect(page.slots?.['form-dialog']).toBe('DecisionFormDialog') + } + }) + + it('DecisionFormDialog is registered, so the renderer can resolve the name', () => { + // CnPageRenderer resolves every `slots` value against the registry and + // only warns when it cannot: an unregistered name renders the built-in + // dialog again, which is the defect this change removes. + expect(registrySource).toContain( + "import DecisionFormDialog from './dialogs/DecisionFormDialog.vue'", + ) + expect(registrySource).toContain( + 'DecisionFormDialog: page(DecisionFormDialog)', + ) + }) +}) + +describe('the detail-page picker offers the registry vocabulary', () => { + it('renders no options at all without the wiring (negative control)', () => { + // This is what the built-in dialog renders from: the stored schema, + // whose decisionType is a free-text string by design since #1099. + const stored = storedDecisionSchema() + + expect(stored.properties.decisionType).toBeTruthy() + expect(stored.properties.decisionType.enum).toBeUndefined() + expect(stored.required).toContain('decisionType') + }) + + it('offers the registry types once the schema passes through the slot', () => { + const enriched = withDecisionTypeVocabulary(storedDecisionSchema(), [ + 'motion', + 'advice', + 'subsidie-besluit', + ]) + + expect(enriched.properties.decisionType.enum).toEqual([ + 'motion', + 'advice', + 'subsidie-besluit', + ]) + expect(enriched.properties.decisionType.enumLabels.motion).toBeTruthy() + }) + + it('offers the shipped seed while the registry has not answered', () => { + // The slot component fetches on mount, so the first open can land + // before the answer does. An empty picker there is the same defect. + expect( + withDecisionTypeVocabulary(storedDecisionSchema(), null).properties + .decisionType.enum, + ).toEqual(FALLBACK_DECISION_TYPES) + }) + + it('never mutates the store schema every other surface reads', () => { + const stored = storedDecisionSchema() + const enriched = withDecisionTypeVocabulary(stored, ['motion']) + + expect(stored.properties.decisionType.enum).toBeUndefined() + expect(enriched).not.toBe(stored) + expect(enriched.properties.decisionType.title).toBe( + stored.properties.decisionType.title, + ) + }) +}) + +describe('settleFormDialogResult (the locked-modal trap)', () => { + const fakeDialog = () => ({ setResult: vi.fn() }) + + it('closes the dialog on a successful save', () => { + const dialog = fakeDialog() + + expect( + settleFormDialogResult(dialog, { success: true, data: { id: '1' } }), + ).toBe(true) + expect(dialog.setResult).toHaveBeenCalledWith({ + success: true, + data: { id: '1' }, + }) + }) + + it('unlocks the dialog on a failed save so the user can retry or close', () => { + // CnFormDialog raises `loading` on submit and only setResult lowers + // it, with `no-close` bound to `loading`. On the error path the page + // leaves the form open, so a dropped result strands the user in a + // modal that can do neither. + const dialog = fakeDialog() + + expect(settleFormDialogResult(dialog, { error: 'Save failed' })).toBe(true) + expect(dialog.setResult).toHaveBeenCalledWith({ error: 'Save failed' }) + }) + + it('leaves a confirm that resolves to nothing alone', () => { + // CnIndexPage's confirm resolves to undefined and closes its dialog by + // flipping `show`. setResult reads `resultData.success`, so passing it + // nothing would throw where the index pages work today. + const dialog = fakeDialog() + + expect(settleFormDialogResult(dialog, undefined)).toBe(false) + expect(settleFormDialogResult(dialog, null)).toBe(false) + expect(settleFormDialogResult(dialog, 'saved')).toBe(false) + expect(dialog.setResult).not.toHaveBeenCalled() + }) + + it('survives the dialog having unmounted while the save was in flight', () => { + // A successful edit sets `show` false, which unmounts the replacement + // and empties the ref before the awaited confirm returns. + expect(() => settleFormDialogResult(null, { success: true })).not.toThrow() + expect(settleFormDialogResult({}, { success: true })).toBe(false) + }) +})