chore(quality): eslint and phpcs to zero, and fix the E2E test failing on development - #1808
Merged
Merged
Conversation
added 7 commits
September 4, 2026 07:35
…e real ones 166 functions carried TWO stacked JSDoc blocks: a complete one, and directly beneath it a bare stub that existed only to hold `@spec` and repeated every parameter with no type and no description. JSDoc takes the NEAREST preceding block, so the stub won. The real documentation was invisible to every tool that reads it, and eslint reported each parameter as undocumented. That is 527 of the repo's 823 warnings, and none of them described a missing description: they described a block nobody could see past. The `@spec` line moves into the real block and the stub goes. 599 deletions, zero insertions, because the tag simply ends up inside the block above it. Warnings 823 -> 296, eslint exits 0 (the suppressions file is pruned; entries for warnings that no longer occur are a hard error, not a warning). Two sites are left alone: their stub named a parameter the real block does not document, so merging would have silently dropped it. Those need a human. The script refused anything it could not prove safe, and the check that mattered was counting `@spec` before and after. An earlier revision of it LOST SEVEN TAGS: the pattern for the first block was not anchored, so it swallowed every earlier docblock in the file, and the "is this tag already present" test then matched a different function's `@spec`. The diff looked correct because the swallowed text was written back unchanged. Only the tally (1760 before, 1753 after) showed it.
vue/require-explicit-emits fires when $emit('x') has no matching entry in the
component's emits option. Nothing here is guessed: eslint names each event
because the component is already emitting it, and an undeclared emit falls
through to the native listener path in Vue 3 rather than being bound as a
component event.
13 components, 22 events, placed in style-guide position (after props, before
data/computed/methods).
eslint warnings 296 -> 244, lint and vitest exit 0.
An earlier automated pass bolted `@spec` onto these functions and dragged bare `@param name` lines along with it: no type, no description, no information. Deleting them is not a fix either, because jsdoc/require-param is enabled and simply reports the parameter as undeclared instead. 115 filled across 47 files. The type comes from the parameter's DEFAULT VALUE where the signature has one, otherwise from a closed table of name patterns (`*Uuid` string, `is*`/`has*` boolean, `*Count` number, plurals Array, and so on). 67 are deliberately left: `value`, `val`, `mode`, `tpl` and friends, whose type is whatever the call site passes. A guessed @PARAM type is a claim the next reader trusts, so those are worth reading one at a time rather than filling in bulk. Warnings 244 -> 109. lint, format and vitest all exit 0.
435 warnings from CustomSniffs.Commenting.SpecTag, on 172 classes and 263 public methods. PHPCS is now at zero. The target is DERIVED, never invented, and that distinction is the whole of the work. The sniff only checks that an @SPEC tag EXISTS: it never resolves the path. So the cheap way to clear 435 warnings is to paste any spec path onto every method, which satisfies the gate and leaves 435 claims nobody verified. A wrong @SPEC is worse than a missing one, because a missing one reads as "unlinked" and a wrong one reads as "linked and checked". So the tag comes from evidence in the file itself: - for a METHOD, the @SPEC of the nearest sibling method above it, since the methods of one class serve one capability; - for a CLASS, the most common @SPEC among its own methods; - 198 of the 206 files already carried at least one @SPEC, which is what made 411 of the 435 derivable this way. The remaining 8 files carried none, so there was nothing to derive from. Those 24 tags were resolved by reading each file's subject matter and matching it to a spec that exists: ZgwController -> zgw-api-mapping ZgwAuthException -> zgw-autorisaties-api ZgwAuthValidationException -> zgw-autorisaties-api TenantMiddleware -> tenant-isolation Iv3TaakveldController -> iv3-taakveld-2023-refinement BrokerAssertionResult -> zaakportaal-mijngemeente (the DigiD / eHerkenning SAML broker is specified there) SettingsSection -> admin-settings AdminSettings -> admin-settings PHPUnit, PHPStan, Psalm and PHPMD all exit 0, and php -l passes on every changed file.
59 more, in 27 files. These are the names the first two passes refused because no suffix rule covered them: ct, st, rt, def, duration, dir, out, mode, tpl and the rest. Each was read at its call site rather than pattern-matched, so `ct` is documented as a case type and `st` as a status type because that is what the functions taking them do with them. `value`, `val`, `newVal` and `v` are the payload of an input or update event. Their concrete type differs per call site, so they are typed as the union the call sites actually pass rather than as `*`, which jsdoc/reject-any-type flags for the same reason. eslint warnings 109 -> 50, and lint exits 0. What is left is not annotation debt: unused props and refs, deprecated OC globals, undefined components, one v-html. Those are code questions and get read one at a time.
The only failing E2E test on development, on every CI run since
2026-09-03:
installing the demo data reports HOW MUCH landed, not just success
Error: install failed: The demo import stored 0 of 444 object(s)
(24 refused by OpenRegister).
The step's own body tells the operator it is "safe to run more than
once", and `demo-data-setup-step.spec.ts:132` asserts the server keeps
that promise. It could not: an idempotent import necessarily stores zero
the second time, and the guard read `objects === 0` as failure.
Reproduced on a pristine rig. OpenRegister's debug log shows what is
actually happening to the 444:
840 [ImportHandler] Found 1 results
840 [ImportHandler] Skipped object update: imported version not higher
420 were already present at the same version and correctly left alone,
24 were refused with logged reasons, 0 needed storing.
So the question is whether anything SURVIVED, not whether anything moved.
`unchanged` is the importer's own count, added in openregister#3410,
which this reads. It is NOT computed here as
`requested - stored - refused`: that subtraction looks equivalent and
reclassifies an object the importer dropped without saying so as "already
present", which is the exact failure the guard exists to catch. An import
where everything failed still throws, and the three unit tests that pin
that behaviour still pass unchanged.
The operator message gains the count that explains a zero: "0 of 444
objects stored ... (24 refused, 420 already present)" rather than a bare
zero that reads as a broken install.
Verified against a pristine instance running both changes:
demo-data-setup-step.spec.ts goes from 1 failed + 1 flaky to 3 passed.
REQUIRES openregister#3410. Without it `unchanged` is absent, reads as 0,
and the guard behaves exactly as it does today: no crash, no change.
eslint is now at ZERO warnings, from 823 at the start of this branch.
The one that mattered: `CaseTypeList.vue` used <NcButton> three times and
neither imported nor registered it. Every other file that uses NcButton
imports it and there is no global registration, so those three buttons
could not render. vue/no-undef-components had been reporting it as a
warning among 822 others.
Real fixes:
- NcButton imported and registered in CaseTypeList.
- `OC.currentUser` replaced with `getCurrentUser()` from @nextcloud/auth at
four sites. Deprecated since Nextcloud 19, and three of the four already
called getCurrentUser() first and only fell back to the deprecated global.
- `caseRelationApi.addRelation` documented `params.notes`, a property the
function never had: it destructures `{targetId, aardRelatie, toelichting}`.
The tag now names the parameter that exists.
- One dead prop (`InspectionChecklistPanel.caseId`, passed by nobody, read by
nobody) and two dead template refs removed.
False positives, annotated with the reason rather than "fixed":
- The 7 dashboard widgets' `title` prop IS passed by their mount scripts, and
the Nextcloud host renders the heading. Removing the declaration would not
remove the prop, it would make it a fallthrough attribute and put a
title="" tooltip on the root element.
- `RedactionAssistDialog.open` and `InspectionPanel.canInspect` default to
true deliberately and nothing passes them. Flipping the default to satisfy
vue/no-boolean-default would change behaviour, not style: the dialog would
mount closed and inspection would be off everywhere.
- `EmailTemplateAdmin`'s v-html renders renderPreview(), which escapes &, <
and > in the body and emits only its own fixed-class <span> and <br>. No
caller-supplied markup can reach the DOM.
- `CnNotesTabComponent` is a computed component behind a v-if guard, which
vue/no-undef-components cannot resolve.
The rest were annotation: 33 files needed a blank line between a docblock
description and its first tag, six `{*}` types became the specific type each
doc already described in prose, and two `@type` tags were moved out of prose
where jsdoc read them as inline tags.
lint, format, stylelint, vitest, phpunit, phpcs, phpstan and psalm all exit 0.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
September 4, 2026 15:17
development gained #1806, which fixed the demo import from the OTHER end: it stops the import forking the schema set and renaming the register, which is what made 24 objects refuse as "ambiguous". That is the first-install half. This branch fixes the re-install half, so the two are complementary rather than competing, and both are needed for the spec to pass: #1806 makes the first install actually store its objects this branch stops "nothing needed storing" being reported as failure Resolved to keep both. The guard keeps development's much better diagnostic message (check the register import has run, a slug resolving to more than one row is refused as ambiguous) and gains the `unchanged === 0` condition, minus development's closing sentence about a re-import landing nothing: that case is now a success, so it no longer belongs in an error. The operator message keeps development's shape, which drops the schema count because a demo set defines no schemas, and keeps the "already present" count, which is what explains a zero. Also widened SetupControllerStatusTest's demo double, which returned an array without `unchanged` and made the controller read an undefined key. Same defect as the one that double already carries a comment about. PHPUnit 2944 green, phpcs, phpstan, psalm, lint, stylelint, prettier and vitest all exit 0.
Contributor
Quality Report — ConductionNL/dossiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 106/106 | |||
| npm | ✅ | ✅ 541/541 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-04 15:49 UTC
Download the full PDF report from the workflow artifacts.
This was referenced Sep 4, 2026
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
1 participant
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.
What this does
Clears dossiq's annotation debt to zero, and fixes the only E2E test failing on
development.demo-data-setup-stepphpunit,phpstan,psalm,phpmd,stylelint,prettier,vitestand all 79 applicable hydra gates exit 0.Three real defects, found by doing it
NcButtonwas never registered inCaseTypeList.vue. It is used three times in the template, imported nowhere, and there is no global registration. Those buttons could not render.vue/no-undef-componentshad been reporting it, as one warning among 822.The demo-data install refused every re-install. The only failing E2E test on
development, on every CI run since 2026-09-03:The step's own body promises it is "safe to run more than once", and the spec asserts the server keeps that promise. It could not: an idempotent import necessarily stores zero the second time, and the guard read
objects === 0as failure. Reproduced on a pristine rig, where OpenRegister's debug log shows what is really happening to the 444: 420 already present and correctly left alone, 24 refused, 0 needing storage.caseRelationApi.addRelationdocumented a parameter it does not have.@param params.notesagainst a function that destructures{targetId, aardRelatie, toelichting}.The
@spectags are derived, not invented435 of these, on 172 classes and 263 public methods, and the way they were produced matters more than the count.
CustomSniffs.Commenting.SpecTagonly checks that an@spectag exists; it never resolves the path. So the cheap way to clear 435 warnings is to paste any spec path onto every method, which satisfies the gate and leaves 435 claims nobody verified. A wrong@specis worse than a missing one: a missing one reads as "unlinked", a wrong one reads as "linked and checked".So the target comes from evidence in the file itself. For a method, the
@specof the nearest sibling method above it; for a class, the most common@specamong its own methods. 198 of the 206 files already carried at least one, which is what made 411 of the 435 derivable. The remaining 8 files carried none, so those 24 were resolved by reading each file's subject matter and matching it to a spec that exists (ZgwControllertozgw-api-mapping,BrokerAssertionResulttozaakportaal-mijngemeente, and so on).527 warnings were one bug, not 527
166 functions carried two stacked JSDoc blocks: a complete one, and directly beneath it a bare stub holding only
@specand repeating every parameter with no type and no description. JSDoc takes the nearest preceding block, so the stub won and the real documentation was invisible. 599 deletions, zero insertions, because the@specline simply ends up inside the block above it.False positives are annotated, not "fixed"
Where the rule was wrong, the reason is written down rather than the code bent to satisfy it. The 7 dashboard widgets'
titleprop is passed by their mount scripts and the host renders the heading; removing the declaration would make it a fallthrough attribute and put atitle=""tooltip on the root. Two boolean props default totruedeliberately, and flipping them would change behaviour, not style. Onev-htmlrenders output that is escaped by construction.Requires
openregister#3410, which adds the
unchanged.objectscounter this reads. Without it the counter is absent, reads as 0, and the guard behaves exactly as it does today: no crash, no change.🤖 Generated with Claude Code