Skip to content

fix(e2e): six of decidesk's seven reds — two OpenRegister null-writes and three locators - #469

Merged
rubenvdlinde merged 2 commits into
developmentfrom
fix/e2e-seven-reds
Aug 11, 2026
Merged

fix(e2e): six of decidesk's seven reds — two OpenRegister null-writes and three locators#469
rubenvdlinde merged 2 commits into
developmentfrom
fix/e2e-seven-reds

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes six of decidesk's seven pre-existing E2E Tests (Playwright) failures. The seventh is left red deliberately — see below.

All seven were pre-existing on development (443a77ed), established by the identical failure set across runs. None is a flake.

The evidence is HTTP, not source reading. The failing run (31521531981) uploaded playwright-traces; the HAR in it carries the request/response pairs the job log does not. That is what turned three of these from hypotheses into measurements.

Two OpenRegister null-writes

board-evaluation-workflow.spec.ts:84 and :133 — no likert response could ever be stored

POST /apps/decidesk/api/board-evaluations/1952ea2b-…/respond  -> 422
POST /apps/decidesk/api/board-evaluations/5774609c-…/respond  -> 422
{"message":"Submitting the response failed: Property 'answers.0.freeText' should be type 'string' but is 'null'."}

sanitiseAnswers() normalised every answer to the same four keys, so a likert answer carried freeText => null. EvaluationResponse.answers.items.freeText is declared {"type":"string"} with no nullable, and OpenRegister's validator rejects an explicit null rather than reading it as "absent" — so the whole saveObject failed, recordCompletion() was never reached, and respondedCount stayed at 0.

Nothing surfaced in the UI: the 422 lands in this.error, rendered by a CnNoteCard that is a sibling of the <ul>, outside the card the count lives in. So the symptom was "the number silently didn't move".

Fixed by omitting the key. Same treatment for likertValue on a free-text answer.

⚠️ This is not the cause diagnosed in #457, and #460 can be re-scoped. The 422 proves execution reached saveObject — past resolveResponder (whose failure is a 403 with "No participant profile…", not a 422) and past the roster check. #457 hardened a check that was already passing, which is exactly why its 434 passing unit tests left the symptom byte-identical.

board-evaluation-workflow.spec.ts:168 — publish answered 200 and persisted nothing

POST /publish -> 200   {"publishedPredicateSet": false, "overallScore": null, …}
GET  /api/objects/decidesk/board-evaluation/20c0cde2-… -> 200
     lifecycle:"closed"  ·  updated == created  ·  no publicatiedatum

Two type violations in one payload, both swallowed by a catch (\Throwable) that only logged a warning:

field declared written result
depublicatiedatum string / date-time, not nullable explicit null save rejected
scoreSummary string (JSON) array save rejected

scoreSummary is the interesting one. OpenRegister hands it back already parsed, so (string) $evaluation['scoreSummary'] yielded the literal "Array", json_decode returned null, and every aggregate fell back to null — the publish response carried overallScore: null while the stored object held 4. The Vue side already carried this exact fix, with its own live measurement, in GovernanceBodyEvaluationsTab::scoreSummaryFor(). The PHP side did not. Now both shapes are accepted on read, and the declared string shape is restored before the save.

The catch stays — a catalog-routing flow should not 500 because the predicate write failed — but it now logs at error level and carries the reason out in warning. A warning with no reader made total failure indistinguishable from success at HTTP 200.

Three locator defects

  • crud-persistence.spec.ts:218 / :334getByRole('dialog').getByRole('button', { name: 'Close' }) matched two buttons: CnDeleteDialog's result-phase action button and NcModal's chrome X (aria-label="Close"). Playwright retries a strict-mode violation, so it surfaced as a bare 20 s timeout rather than as an ambiguous locator. Scoped to .dialog__actions — verified against the published @nextcloud/vue@9.9.0 NcDialog, not the checkout's stale 8.39.0 node_modules — and pinned exact: true, since name matches a substring by default.
  • meeting-transcription-workflow.spec.ts:220NcCheckboxRadioSwitch merges $attrs onto the <input>, which v9.9.0 styles position: absolute; z-index: -1; opacity: 0 !important beneath its own content span. locator.click() can never land. Switched to the actuation the six passing NcCheckboxRadioSwitch tests in user-settings.spec.ts already use, and bracketed it with not.toBeChecked()toBeChecked()toBeEnabled() so a click that actuates nothing fails here by name.

The seventh is left red on purpose

integration-registry.spec.ts:342 — OCS advertises kvk and opencorporates; the JS registry declares neither. Positive-controlled: @conduction/nextcloud-vue ships no leaf for either (openproject matches 5 files under src/integrations/, those two match none). This is real cross-app drift, already filed as nextcloud-vue#630, and the test is right to catch it. Its fix does not live in decidesk, and weakening the assertion would delete a true finding.

Controls

Committed mutations, each reverted, each failing only its own target:

mutation result
scoreSummary array branch disabled 1 failure — Failed asserting that null is identical to 4, the production symptom exactly
depublicatiedatum restored to null 1 failure
sanitiseAnswers restored to writing nulls 2 failures, naming both keys

Two green-for-the-wrong-reason tests were also corrected, since both would otherwise have hidden the fix:

  • The existing publish test asserted depublicatiedatum was present and null — it pinned the defect and was green for exactly as long as the bug lived.
  • cycleLabel: 'E2E-Published' made toContainText(/published/i) unfailable, because the card renders label and lifecycle in one text run. A completely dead publish path read as working, and the test only failed four lines later. Renamed to E2E-Aggregate.

Verification

  • PHPUnit 940 tests / 3271 assertions, 0 failures
  • phpcs 0 errors (also fixed one pre-existing SPDX comment warning), phpmd clean on both changed files
  • ⚠️ psalm and phpstan could not be run locally — the vendor tree available here has an emptied vendor/nextcloud/ocp/OCP (an OCP.bak sits next to it), so phpstan reports 2457 "class not found" errors that are a wiring gap, not this diff. CI runs both properly.
  • Weakening checks all negative: no test.skip / test.fixme / .only / mode:'serial' / testIgnore / networkidle / force: true added, zero @e2e exclude added, and no removed line contains timeout — no budget was widened.

Also bumps phpcsstandards/phpcsutils 1.2.2 → 1.2.3 for CVE-2026-65954, exercised by CI running phpcs rather than only locked.

Expected result: E2E 7 failed → 1 failed (integration-registry.spec.ts:342). ⚠️ 58 tests are skipped in this suite, so "my test is not in the failure list" proves nothing — the six will be confirmed positively by their lines.

… locators

All seven were pre-existing on `development` and none is a flake. Every claim
below is a measurement; the HTTP evidence came out of the failing run's own
Playwright trace artifact (run 31521531981), not out of source reading.

## 1-2. board-evaluation :84 / :133 — no likert response could EVER be stored

Observed: `POST /api/board-evaluations/{id}/respond -> 422` on both, body
`Property 'answers.0.freeText' should be type 'string' but is 'null'`.

`sanitiseAnswers()` normalised every answer to four keys, writing `freeText =>
null` for a likert answer. The schema declares it `type: "string"`, not
nullable, and OpenRegister's validator rejects an explicit null rather than
reading it as "absent" — so the whole `saveObject` failed, `recordCompletion()`
was never reached, and `respondedCount` stayed 0. The UI showed nothing: the
error NoteCard renders outside the card the count lives in.

Omit the key instead. Same for `likertValue` on a free-text answer.

⚠️ This is NOT the unscoped-responder cause diagnosed in #457. The 422 proves
execution reached `saveObject`, i.e. it had already passed `resolveResponder`
(whose failure is a 403, not a 422) and the roster check. #457 hardened a check
that was already passing, which is why its 434 green unit tests changed nothing.

## 3. board-evaluation :168 — publish answered 200 and persisted nothing

Observed: `POST /publish -> 200` with `publishedPredicateSet: false`,
`overallScore: null`, and a follow-up GET showing `lifecycle: "closed"`,
`updated == created`, no `publicatiedatum`. Two type violations in one payload,
both swallowed by a `catch (\Throwable)` that only logged a warning:

- `depublicatiedatum` was written as an explicit `null` (declared
  `type: "string", format: "date-time"`, not nullable) — now omitted.
- `scoreSummary` is declared `type: "string"` but OpenRegister hands it back
  ALREADY PARSED as an array. `(string)` on an array yields the literal
  `"Array"`, `json_decode` returned null and every aggregate fell back to null —
  so the response carried `overallScore: null` while the stored object held 4 —
  and the array was written straight back, failing validation. Now accepts both
  shapes on read and restores the declared string shape before the save. The Vue
  side already carried this exact fix with its own live measurement
  (`GovernanceBodyEvaluationsTab::scoreSummaryFor`); the PHP side did not.

The catch stays — a catalog flow should not 500 because the predicate write
failed — but it now logs at error level and carries the reason out in `warning`,
because a warning nobody reads made total failure look identical to success.

## 4-5. crud-persistence :218 / :334 — two buttons named "Close"

`getByRole('dialog').getByRole('button', { name: 'Close' })` matched both
CnDeleteDialog's result-phase action button and NcModal's chrome X
(`aria-label="Close"`). Playwright RETRIES a strict-mode violation, so it
surfaced as a bare 20 s timeout, not as an ambiguous locator. Scoped to
`.dialog__actions` (verified in the published @nextcloud/vue 9.9.0 NcDialog) and
pinned `exact: true`, since `name` matches a substring by default.

## 6. meeting-transcription :220 — clicking an input that is behind its own label

`NcCheckboxRadioSwitch` merges `$attrs` onto the `<input>`, which v9.9.0 styles
`position: absolute; z-index: -1; opacity: 0 !important` under its own content
span. `locator.click()` can never land. Switched to the actuation the six
PASSING NcCheckboxRadioSwitch tests in `user-settings.spec.ts` already use, and
bracketed it with `not.toBeChecked()` / `toBeChecked()` / `toBeEnabled()` so a
click that actuates nothing fails by name.

## 7. integration-registry :342 — NOT FIXED, and must not be

OCS advertises `kvk` and `opencorporates`; the JS registry declares neither.
Confirmed with a positive control that `@conduction/nextcloud-vue` ships no leaf
for either (`openproject` matches 5 files, those two match none). That is real
cross-app drift, already filed as nextcloud-vue#630, and the test is correct to
catch it. Left red rather than weakened.

## Controls

Committed mutations, each reverted after:
- `scoreSummary` array branch disabled -> exactly 1 failure,
  `Failed asserting that null is identical to 4` — the production symptom.
- `depublicatiedatum` restored to `null` -> exactly 1 failure.
- `sanitiseAnswers` restored to writing nulls -> exactly 2 failures, naming both keys.

Also corrected a test that PINNED the defect: the existing publish test asserted
`depublicatiedatum` was present AND null, so it was green for exactly as long as
the bug lived.

And removed a false green in the same spec: `cycleLabel: 'E2E-Published'` made
`toContainText(/published/i)` unfailable, so a completely dead publish path read
as working. Renamed to `E2E-Aggregate`.

PHPUnit 940 tests / 3271 assertions, 0 failures. phpcs 0 errors (fixed one
pre-existing SPDX comment warning). phpmd clean on both changed files. Weakening
checks all negative: no skip/fixme/only/serial/testIgnore/networkidle/force
added, no `@e2e exclude` added, and no removed line contains `timeout`.

Also bumps `phpcsstandards/phpcsutils` 1.2.2 -> 1.2.3 for CVE-2026-65954,
exercised by CI's phpcs job rather than only locked.
… by an unobserved container

The previous form scoped to `.dialog__actions` — real in @nextcloud/vue 9.9.0's
NcDialog source, but never observed in this app's rendered DOM (0 hits across the
whole 508 KB failing job log, against 2 hits for `modal-container__close`). If it
is not a descendant of the `role=dialog` node the locator resolves to zero and
fails as another bare timeout, i.e. exactly the failure mode under repair, on an
inference rather than a measurement.

`.and(:not(.modal-container__close))` depends only on DOM this run printed.
Strictness is unchanged: a third button named "Close" still raises.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidesk @ 191c2f4

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 21:01 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidesk @ 1cc3d19

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 21:33 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 445776b into development Aug 11, 2026
28 of 31 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/e2e-seven-reds branch August 11, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants