fix(admin): implement the seven admin routes four settings tabs already called (#794) - #795
Merged
Merged
Conversation
…dy called (#794) Four admin settings tabs were entirely non-functional: Mandate Matrix Administration, Mandate Matrix System Settings, AWB Term Definitions and Consultation Management. They called procest API URLs that appinfo/routes.php never declared. Nextcloud answers an unmatched app URL with its own HTML page under HTTP 200, so nothing threw and nothing logged. Live-probed before the fix, printing the content-type rather than the status — all seven returned 200 text/html at 40,061 bytes, byte-identical to a nonsense control URL, while /api/settings and /api/vth/checklists returned application/json. Only ChecklistsTab (#784) was loud, because it used the string-admitting `x?.results || x || []`. The other seven use the correct guard, `Array.isArray(x) ? x : (x?.results || [])`, which properly discards the HTML string and thereby turned the identical defect into a silent empty tab. The guard is right and stays; a working guard masked a broken caller. What this adds - MandaatRegistryController — GET besluiten, POST/PATCH mandaten. - OrganisatieRolController — full CRUD for rollen (with the delete guard) and toewijzingen. Split from the above on the domain boundary; one class holding all four registries exceeded the public-method budget. - TermijnDefinitieController — GET/POST/PATCH definities (REQ-TERM-ADMIN-001). - ConfiguredRegistryService — generic list/save/delete over a config-key-named schema, with no authorization of its own, so every caller must carry a guard. - MandaatRegistryService — the referential-integrity guard the spec requires: a role held by a Mandaat or an active assignment refuses deletion with 409. - The two settings tabs now POST the app's own guarded /api/settings, their keys are registered in CONFIG_KEYS, and AdminSettings provides the initial state both tabs were already calling loadState() for but nothing supplied. Authorization posture, checked deliberately Every new method carries #[AuthorizedAdminSetting]. Repointing the frontend at OpenRegister's generic object route also resolves and also removes the symptom — that was the retracted first fix for #784, and it bypasses procest's admin authorization. Verified against a live instance: non-admin gets 403 on all seven new routes, while the @NoAdminRequired control still returns 200, so the probe distinguishes rather than uniformly failing. Pre-existing bugs found and fixed while verifying - InspectionChecklistService::deleteChecklist passed `id:` to ObjectService::deleteObject, whose parameter is `$uuid`. Every call raised "Unknown named parameter $id", was swallowed by the catch, and returned false — the admin Delete button 500'd on every checklist. Confirmed live before and after. - MandaatToewijzingenTable sent `{ totEnMet }` to end an assignment. The schema field is `validUntil`, and saveObject is PUT-semantic, so that payload would have nulled userId, rolId, toewijzingType and validFrom off the record. - An earlier draft of the delete guard checked organisatieRol/rol/rolId only. The Mandaat schema names the reference `gemandateerdeRol`, so the guard would have failed OPEN for every mandate. Read from the shipped schemas, not guessed. Verified on an isolated seeded rig (:8091): create/list/update/delete round trips for all four registries; the delete guard refuses a referenced role (409, naming what blocks it) and permits an unreferenced one (200); settings persist across a container restart; and an unregistered key is dropped by the CONFIG_KEYS allowlist, which is what makes registering the new keys load-bearing. composer check:strict: ALL CHECKS PASSED (1791 tests). eslint 0 errors. USE_LOCAL_LIB=false npm run build: exit 0. Refs #784, #786, #792
rubenvdlinde
requested review from
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 11, 2026 18:39
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ❌ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 552/552 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-11 18:58 UTC
Download the full PDF report from the workflow artifacts.
…n keys
Two CI failures on this PR, both real and both mine.
l10n coverage / test:l10n — the two new error strings ('Saving failed',
'Saving failed ({status})') were not in l10n/en.json. Extracted with
check-l10n.js --write and translated on the nl.json side, so no Dutch-locale
string falls back to the raw English key.
Coverage ratchet — 1777 tests passed and the PHPUnit cell still failed, which
is the ratchet, not a broken test: the change added 261 statements and coverage
fell 0.1% against the merge base. Fixed by covering the new code rather than by
touching the threshold.
- ConfiguredRegistryServiceTest — list/save/delete, both the numeric and the
slug resolution paths. Two are regression guards worth keeping: delete must
pass the identifier as `uuid` (passing `id:` is what made
InspectionChecklistService::deleteChecklist 500 on every call), and on the
numeric path register/schema must be NESTED under `@self` (a top-level
filter silently returns an empty set, which reads as 'there is no data'
rather than as a malformed query).
- MandateRegistryControllersTest — the three controllers: status codes, schema
routing, 409-with-a-reason on a referenced role, 422 vs 500, and that a
client-supplied `id` in the body cannot override the routed one.
Discovered while writing these: a live instance stores NUMERIC register/schema
ids (register 14, organisatie_rol_schema 153), so `searchObjects()` — not the
slug bridge — is the production path. The first draft of the test stubbed only
the slug bridge and returned an empty set, which is exactly the failure this
code has to survive.
⚠️ The ratchet could not be reproduced locally: no code-coverage driver is
installed here ('No code coverage driver available'), so CI is the authority on
whether this clears it.
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ❌ | ✅ 100/100 | |||
| npm | ✅ | ✅ 552/552 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-11 19:33 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 552/552 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-12 07:25 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
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.
fix(admin): implement the seven admin routes four settings tabs already called (#794)
Four admin settings tabs were entirely non-functional: Mandate Matrix
Administration, Mandate Matrix System Settings, AWB Term Definitions and
Consultation Management. They called procest API URLs that appinfo/routes.php
never declared. Nextcloud answers an unmatched app URL with its own HTML page
under HTTP 200, so nothing threw and nothing logged.
Live-probed before the fix, printing the content-type rather than the status —
all seven returned 200 text/html at 40,061 bytes, byte-identical to a nonsense
control URL, while /api/settings and /api/vth/checklists returned
application/json.
Only ChecklistsTab (#784) was loud, because it used the string-admitting
x?.results || x || []. The other seven use the correct guard,Array.isArray(x) ? x : (x?.results || []), which properly discards the HTMLstring and thereby turned the identical defect into a silent empty tab. The
guard is right and stays; a working guard masked a broken caller.
What this adds
toewijzingen. Split from the above on the domain boundary; one class holding
all four registries exceeded the public-method budget.
schema, with no authorization of its own, so every caller must carry a guard.
a role held by a Mandaat or an active assignment refuses deletion with 409.
keys are registered in CONFIG_KEYS, and AdminSettings provides the initial
state both tabs were already calling loadState() for but nothing supplied.
Authorization posture, checked deliberately
Every new method carries #[AuthorizedAdminSetting]. Repointing the frontend at
OpenRegister's generic object route also resolves and also removes the symptom
— that was the retracted first fix for #784, and it bypasses procest's admin
authorization. Verified against a live instance: non-admin gets 403 on all
seven new routes, while the @NoAdminRequired control still returns 200, so the
probe distinguishes rather than uniformly failing.
Pre-existing bugs found and fixed while verifying
id:toObjectService::deleteObject, whose parameter is
$uuid. Every call raised"Unknown named parameter $id", was swallowed by the catch, and returned
false — the admin Delete button 500'd on every checklist. Confirmed live
before and after.
{ totEnMet }to end an assignment. The schemafield is
validUntil, and saveObject is PUT-semantic, so that payload wouldhave nulled userId, rolId, toewijzingType and validFrom off the record.
The Mandaat schema names the reference
gemandateerdeRol, so the guard wouldhave failed OPEN for every mandate. Read from the shipped schemas, not guessed.
Verified on an isolated seeded rig (:8091): create/list/update/delete round
trips for all four registries; the delete guard refuses a referenced role (409,
naming what blocks it) and permits an unreferenced one (200); settings persist
across a container restart; and an unregistered key is dropped by the
CONFIG_KEYS allowlist, which is what makes registering the new keys load-bearing.
composer check:strict: ALL CHECKS PASSED (1791 tests). eslint 0 errors.
USE_LOCAL_LIB=false npm run build: exit 0.
Refs #784, #786, #792