fix: ADR-079 D1 deletes the duplicate settings surface, and the eight E2E failures behind it - #458
Merged
Merged
Conversation
…rting surfaces that do not exist
NcSelect: THE PROP AND THE EVENT WERE BOTH GONE (4 components)
decidesk locks `@nextcloud/vue ^9.9.0`. In 9.9.0 NcSelect declares:
modelValue: { type: [String, Number, Object, Array], default: null },
emits: [" ", "update:modelValue"]
There is NO `value` prop and `input` is not in the emits list. Four components
still bound the v8 pair `:value` + `@input`, so the selection never reached the
component and the component's choice never reached the app. In
MeetingTranscriptionTab that is the failing test exactly: `selectedSource` stayed
null, so `:disabled="!selectedSource"` kept "Attach recording" disabled forever.
ActionItemDeckBoard (lane moves), GovernanceBodyRetentionTab (policy picker) and
one `@update:value` in the evaluations tab were silently broken the same way with
no test watching. All four move to `:model-value` / `@update:model-value`.
⚠️ This was checked against the WRONG VERSION first and "disproved". The shared
checkout's node_modules carries @nextcloud/vue 8.39.0, whose NcSelect does still
emit `input` — but package-lock.json pins 9.9.0, which is what CI installs. An
API check is only as good as the version it reads: verify against the LOCK, not
against whatever happens to be installed.
BOARD EVALUATION: THE RIGHT MEMBER, THE WRONG BODY (3 specs)
The card stayed at "0 of 3 responded" after a member submitted. Identity is
per-body, not per-person: a member serving on two boards has two Participant
objects, and the UID -> Participant lookup was unscoped, returning whichever the
store listed first. submitResponse() then compared an identity from one body
against the invited roster of another and rejected a genuinely invited member —
and the rejection reads as a legitimate "not invited to this cycle", so nothing
surfaced. The lookup is now scoped to the evaluation's own governance body; the
roster check remains the gate.
PUBLICATION: A SELECTOR THAT COULD NEVER RESOLVE
`getByTestId('publication-withdraw-reason').locator('textarea')` descends into an
NcTextArea that merges $attrs onto the inner <textarea> — the testid IS the
textarea, so there is nothing to descend into. `resolution-minutes.spec.ts:183`
already drives the identical NcTextArea construct with a direct `.fill()`, and
that spec passes. Matched to it.
TWO TESTS ASSERTED A SURFACE THAT DOES NOT EXIST — AND MUST NOT
`decision-management` clicked `getByRole('tab', { name: 'Lifecycle' })` and
`{ name: 'Voting results' }`. Neither is a tab. DecisionDetail's manifest lays
`decision-lifecycle` and `decision-voting` out as BODY WIDGETS, and its sidebar
declares exactly one tab: `audit`, labelled "History". So the click could never
have matched.
The tempting "fix" is to add `role="tab"` to those widget headers. That would be
a WCAG 4.1.2 regression — a tab role without a tablist and tabpanel — i.e. making
the gate green by breaking the thing it protects. Instead the tests assert the
widget the user actually lands on. Every substantive assertion below is
UNCHANGED: seven lifecycle states in machine order, exactly one current-state
marker, transition actions or the explicit empty notice, and the voting tab's
rounds/table/no-motion notice.
CRUD DELETE: ASSERT THE PHASE, DO NOT WAIT OUT A TIMER
CnDeleteDialog dwells on success — it shows "Item successfully deleted." and only
then arms a 2 s auto-close. The test sat out that timer inside a 20 s budget.
It now asserts the success phase explicitly
(`[data-testid-phase="result"]` + the success text — STRONGER than "something
vanished": a dialog stuck in `phase=confirm` or showing an error now fails here,
naming the phase) and dismisses via the Close button, as a user would. The
duplicate `gotoList` before the decision delete is dropped: the assertion above
it already leaves the browser on that list, and the reload was a second full page
load (~3.7 s measured) in a 20 s budget — which is where the test actually ran
out of clock.
No test is deleted, skipped, or given a longer timeout. No assertion is loosened.
FALSIFIABLE CLAIM FOR CI: transcription:220, publication:109,
decision-management:121 and :159, board-evaluation x3 and crud-persistence:202
flip. That is 8 of the 11, leaving integration-registry (upstream
openregister#2415), motion-amendment:34 and crud-persistence:318 — so 11 failed
-> 3 failed, with skips held at 59.
CI's PHPUnit job failed on this branch with no failing test:
Tests: 913, Assertions: 3215, Skipped: 37
Coverage current: 60.29% (9173/15214 statements)
Coverage merge base: 60.44% (9170/15172 statements)
FAIL: coverage dropped by 0.15% against the merge base.
This change adds 42 statements. Adding code without tests drops coverage.
A green test run under a red job is the coverage ratchet's signature, and it was
correct: the fix introduced ParticipantUuidLookup and
BoardEvaluationResponseService::resolveResponder and tested neither. The
behaviour was proven end to end by Playwright and not at all by a unit test,
which is exactly the gap the ratchet exists to catch.
Twelve tests, no threshold touched, no baseline edited:
ParticipantUuidLookupTest (7) — unscoped resolution, the null and `id`-fallback
shapes, and the defect itself: ONE user, TWO boards, TWO participant rows, and
the answer must depend on the body asked about. Also that belonging to no such
body resolves to null rather than to somebody else's identity, and that empty
inputs short-circuit before the store is touched.
BoardEvaluationResponderScopeTest (5) — the evaluation's `governanceBody` scopes
the lookup; the `@self.relations` form is read too; no body on the evaluation
degrades to the unscoped lookup; a store failure degrades rather than throwing
(the roster check in submitResponse() is still the gate); empty inputs never
query.
BOTH DIRECTIONS PROVEN. Disabling the body filter in
`forNextcloudUserInBody` — the exact shape of the original bug — turns three of
these red, the key one returning `participant-on-board-1` where the identity for
board 2 was asked for:
1) testForNextcloudUserInBodyPicksTheIdentityForThatBody
Failed asserting that two strings are identical.
3) testForNextcloudUserInBodyReturnsNullWhenNotOnThatBody
Failed asserting that 'participant-on-board-1' is null.
Reverted after measuring.
Suite: 913 -> 925 tests, 3215 -> 3230 assertions, 0 failures, skips held at 37.
phpcs on lib/ (CI's scope) unchanged at 0 errors / 105 warnings.
…he in-app one
The app shipped app-level configuration twice: at /settings/admin/decidesk,
rendered by lib/Settings/AdminSettings.php and authorized by Nextcloud
server-side before the section renders, AND at /apps/decidesk/settings, a
manifest `type:"settings"` page authorized by whatever the app remembered to
check. ADR-079 D1 calls the second one a defect. This deletes it.
Deletion, never a rename. Gate-63 compares a settings page's id and title
against a reserved word list, so renaming the page to anything else turns the
gate green while shipping the duplicate surface untouched — the gate would stop
reporting the exact thing it exists to find.
The page was also the collision ADR-079 Context (4) describes: menu-layout.json
lifted it into the NcAppNavigationSettings foldout, whose own button is labelled
"Settings", so the navigation read `Settings > Settings`. settingsSection is now
empty by decision, with the reason recorded next to it.
ONE SETTING WOULD HAVE BEEN SILENTLY ORPHANED, and this is the part ADR-079
assumes rather than states. Its Consequences say the 12 in-app pages are
"deleted, along with their routes", taking for granted that the app's
lib/Settings section already covers the same keys. Measured here, it covered two
of three: `ori_endpoint` and `email_voting_enabled` have sections on the admin
page; `organisatie_modus` had none. That key drives mode-specific labels across
the whole app (src/App.vue:137 via src/config/modeLabels.js), and the deleted
page was its only UI — after deletion it would have been writable only by occ,
with no gate able to notice. It is rehomed onto the admin page as a real
section, backed by an e2e test that selects a mode, saves, RELOADS and requires
the value to have survived the round trip. The backend needed nothing:
SettingsService already whitelists the key and both surfaces posted to the same
/api/settings endpoint.
The e2e file that owned the old surface is retargeted rather than dropped, and
it got stronger in the move. It carried a `test.fixme` for the register-mapping
actions because the nc-vue `version-info` and `register-mapping` settings
widgets crashed while rendering the manifest page (`TypeError: e is not a
function` at `Proxy.render`, twice per load) — so the in-app page had not merely
duplicated the admin page, it had been half-broken the whole time. The admin
page renders the mapping through CnAdminSettingsShell instead, so the scenario
is now asserted for real, and the console-error assertion that the old file had
to disable is switched back on.
Also deletes src/views/MeetingIntegrations.vue, which is genuinely dead — and
which is NOT one of the three files I was sent to delete. DecisionIntegrations,
AgendaItemIntegrations and MotionIntegrations are all live: each is imported and
registered in src/registry.js, named as the `component` of a manifest page on a
live route, and is the target of a manifest action handler. Deleting those three
would have blanked three routes. MeetingIntegrations is the opposite shape:
nothing imports it, its own docblock claims registration in
`src/customComponents.js` and that file does not exist, and its manifest page is
a fully declarative type:"detail" entry with widgets+layout and no `component`
key, so the renderer builds the route without ever touching the .vue. In a
manifest-driven app, liveness is answered by two registries at once — registry.js
and the manifest `component` key — and registry.js already carries a comment
recording the inverse bug shipping: MotionIntegrations was named by the manifest
but never registered, so "resolution fell through and the page rendered NOTHING".
DashboardEmptyState's "Set Up Body" button pushed `{ name: 'Settings' }`, which
would now resolve to nothing. Retargeted to GovernanceBodies — where a governing
body is actually created, which is what the button's label always promised.
Two Playwright tests are also corrected here, for a fault that has nothing to do
with settings but the same shape: they did
`getByRole('tab', { name: 'Lifecycle' }).click()` and `{ name: 'Voting results' }`
and timed out on every run. Neither is a tab. Both are `type:"custom"` entries
in the decision detail page's `config.widgets[]`, placed by `config.layout[]`
and rendered by CnDetailPage as a plain div + h3 with no role="tab"; the page's
sidebar declares exactly one tab, and the manifest _note says so outright:
"Sidebar is audit-trail only." Playwright was right to time out. The click is
removed and every assertion after it is kept, so the tests still fail if the
lifecycle timeline or the voting surface stops rendering. NcAppSidebarTab does
set role="tab" — checked in the built chunk — so this is not an ARIA gap in
nc-vue.
…d-adr079 # Conflicts: # tests/e2e/spec-coverage/decision-management.spec.ts
…l required a deleted surface gate-16 named two changed methods with no @SPEC — `scoreSummaryFor` (from the merged e2e-residue work) and `goGovernanceBodies` (from the ADR-079 D1 commit). Both get a real anchor into openspec/specs/, not an exclude: an exclude is how a coverage gate is talked out of the thing it measures. Verified by planting the absence back and watching gate-16 return FAIL — 1 changed method(s) missing @SPEC, then removing the plant. motion-amendment's create-dialog test asserted three things the product has never produced, which is why it failed identically on every run rather than flakily. - The heading is "Create Decision", and that is correct. Under ADR-005 there is no `motion` schema; a motion IS a Decision with decisionType=motion, and the Motions index is a filtered projection ("schema": "decision", "filter": {"decisionType": "motion"}). CnIndexPage mounts its create dialog without a `dialog-title`, so CnFormDialog falls back to `Create {schema.title}`. - Field labels are the schema property's `title`, never its key. The form renders "Motion type", "Co-signers" and "Status" — `motionType`, `coSigners` and `lifecycle` could not match anything. - `required` on Decision is exactly ["title","text","decisionType"], read out of lib/Settings/decidesk_register.json rather than assumed, so Title is the only one of these fields carrying an asterisk. `proposer *` and `lifecycle *` were asserting a required-ness the schema does not declare — the same mistake decision-management already corrected for decisionDate/outcome. The assertions are kept and corrected rather than dropped, so the test still fails if the motion form loses a field or gains a spurious required marker. app-navigation's REQ-NAV-002 still REQUIRED the surface ADR-079 D1 deletes: "and Beheer (the Settings entry, settings section)" plus "A settings link (Beheer) SHALL appear via the manifest `settings` section". Left alone, the repo would ship a spec demanding a page the same repo removed, and those clauses also describe the `Settings > Settings` collision ADR-079 Context (4) records — the gear foldout's own button is already labelled "Settings". Reconciled, with the ADR named as the reason and the removed wording quoted in place so the change is auditable rather than silent, and a scenario added asserting the app contributes no settings entry of its own. Flagging this one deliberately: editing a spec in a task about satisfying gates is the shape of "authoring the spec you were sent to check against". It is not that here — app-navigation is downstream of ADR-079, which is an approved company-wide decision this change executes, and no gate reads REQ-NAV-002. But it is a judgement call and it should be reviewed as one.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 11, 2026 12:31
test:l10n went red on eight keys, seven from the Organisation mode section moved onto the Nextcloud admin page under ADR-079 D1 and one from the reworded DashboardEmptyState description. This is the gate doing its job on a real omission in my own change, not a pre-existing finding. Written into en.json (key === English source) via the checker's own --write, and translated on the nl.json side by hand rather than left to fall back to English: the five organisatie_modus values are exactly the vocabulary this setting exists to switch, so shipping them untranslated would have made the Dutch admin page read half in English precisely where it explains Dutch label selection.
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ❌ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 548/548 | |||
| PHPUnit | ✅ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 12:34 UTC
Download the full PDF report from the workflow artifacts.
…s not work
Two nc-vue specifics that user-settings.spec.ts already documents, and that this
new test would have hit on its first CI run:
NcSelect does NOT set `inheritAttrs: false`, so a `data-testid` written on the
component lands on the WRAPPER rather than the combobox, and clicking the
wrapper does not open the dropdown. Click the inner input.
Options render through NcEllipsisedOption, which splits any label of 10+
characters into two spans inside a flex parent, so the option's ACCESSIBLE NAME
gains a space at the split point — `getByRole('option', { name: 'Association
(assoc)' })` can never match. Match on text content, which is unaffected.
Both fixed by copying the pattern from the passing display-preferences tests
rather than inventing a third one.
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 548/548 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 12:41 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 548/548 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 13:13 UTC
Download the full PDF report from the workflow artifacts.
…hange Reverts this branch's edit to openspec/specs/app-navigation/spec.md. The edit is correct and still needed — REQ-NAV-002 currently REQUIRES the settings entry ADR-079 D1 deletes, so the repo ships a spec demanding a page it removed — but touching that file pulls all 25 of its scenarios into gate-19's diff scope, and 23 of them are untouched legacy scenarios with no @e2e traceability. There are only three ways out and two of them are forbidden. Blanket-tagging 25 scenarios with `@e2e` would be fake coverage — gate-19 credits a file-level tag without reading the test body (.github#343), so the tags would assert traceability that does not exist. A reason-bearing `@e2e exclude` on scenarios I did not write is the same dodge wearing a justification. Writing 23 real e2e tests is correct and is not a rider on this change. So the spec reconciliation ships separately, with the traceability work it actually requires. Filed with the measured gate-19 consequence so the next person does not rediscover the cost. Recording a bad measurement of my own, because it nearly became a fleet-wide claim: I first "controlled" for this by reverting the file in the WORKING TREE and re-running the gate, saw 24 app-navigation scenarios still flagged while `git diff origin/development -- openspec/` printed nothing, and started writing this up as gate-19 ignoring diff scope. That control was invalid — the revert was uncommitted, and gate-19 diffs the COMMIT RANGE, so my committed change was still in scope. The gate was right. A control that does not change the input the tool actually reads is not a control.
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 548/548 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-11 13:51 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #457, whose commits are merged in here unchanged — its diagnosis was independently reproduced and its fixes are better than the ones I had written, so they are kept as authored.
The full-scope gate table this started from
decidesk's CI reported
ALL 25 APPLICABLE GATES PASSED. That is 25 of 64, over a push-scope diff of roughly one file. Dispatched at full scope (run 31488483644, job 93777112798):Five real failures, none of which the push-scoped green could see:
development@ full scope@e2eThis PR closes gate-63. The other four are inherited debt this PR does not touch and does not hide.
gate-63, closed by deletion — never by rename
Gate-63 compares a
type:"settings"page's id and title against a reserved word list. Renaming the page turns the gate green while shipping the duplicate surface untouched, so deletion is the only remedy that means anything.Positive control, both directions,
--fullon the same tree:development→[gate-63] settings-surface: FAIL — 2 settings-placement violation(s)[gate-63] settings-surface: PASSOne setting would have been silently orphaned. ADR-079 assumes the
lib/Settings/*Admin.phpsection already covers the same keys. Measured here it covered two of three:ori_endpointandemail_voting_enabledhad sections on the admin page,organisatie_modusdid not — and that key drives mode-specific labels app-wide. It is rehomed onto the admin page with an e2e test that selects, saves, reloads and requires the value to have survived.A claim I had to retract
I was asked to delete three dead
*Integrations.vuefiles. Measured,DecisionIntegrations,AgendaItemIntegrationsandMotionIntegrationsare all live — imported and registered insrc/registry.js, named as thecomponentof a manifest page on a live route, and targets of manifest action handlers. Deleting them would have blanked three routes.The genuinely dead file is the fourth,
src/views/MeetingIntegrations.vue: nothing imports it, its docblock claims registration insrc/customComponents.jswhich does not exist, and its manifest page is fully declarative with nocomponentkey. Only that one is deleted.In a manifest-driven app, liveness is answered by two registries at once —
registry.jsand the manifestcomponentkey.registry.jsalready carries a comment recording the inverse bug shipping.The E2E root causes — 11 failures, deterministic on three consecutive runs
Not the procest 41 MB-DOM shape: 102 passed, individual tests completing in 6–8 s, role queries terminating normally.
Product defects (4)
ParticipantUuidLookupresolved a Nextcloud UID to a Participant unscoped by governance body, returning the ci-seed chair, so the roster check rejected a genuinely invited member andrecordCompletion()never ran — "0 of 3 responded" forever.scoreSummaryFor()calledJSON.parse()on a value OpenRegister already returns decoded, so every real score summary becamenullbehind a silentcatchand thev-if'd results block never rendered.NcSelect/NcTextAreacall sites still used the v8:value/@inputpair.@nextcloud/vue9.9.0 has onlymodelValue/update:modelValue, so the pick never committed and Attach stayed disabled.CnDeleteDialogdwells ~2 s on success before auto-closing, which the delete test sat out inside a 20 s budget three page loads had already spent.Test defects (7) — all asserting surfaces the app does not have:
getByRole('tab')could never resolve; the sidebar declares one tab and the manifest_notesays so.NcAppSidebarTabdoes setrole="tab"— checked in the built chunk, so this is not an ARIA gap in nc-vue.publication-withdraw-reasonis the<textarea>:NcTextAreasetsinheritAttrs: falseand merges$attrsonto the inner element, so.locator('textarea')had no descendant to find.decisionType=motion— and field labels are schematitles, not keys.gotoListwas a byte-identical duplicate of the line seven above it.No timeouts widened, no tests skipped or deleted, no
mode: 'serial'. Every corrected assertion is kept so the tests still fail if the product regresses.Filed elsewhere, not worked around here
kvkandopencorporatesare advertised over OCS by openregister but were never registered in JS. Owner is nextcloud-vue; traceable to a missing "Vue registration" task in openregister's change. Includes the landmine that fixing it flips a currently-skipping decidesk test to failing.registerIntegration(and missedintegrations.register(, so it told decidesk and openregister they "register no integration leaves at all". Marked not-for-immediate-merge.~10 tests wrap a locator in
test.skip(!hasTab, 'deployed build predates sidebar tabs')whose condition can never be false, and 4 more are named for tabs but assert only thatapp-rootmounted. Both families carry@e2eannotations, so gate-19 credits coverage that is not exercised. Do not read102 passedas 102 assertions.🤖 Generated with Claude Code