fix(deelzaak): stop unlinkSubCases reporting success over a partial unlink (#793) - #796
Open
rubenvdlinde wants to merge 5 commits into
Open
fix(deelzaak): stop unlinkSubCases reporting success over a partial unlink (#793)#796rubenvdlinde wants to merge 5 commits into
rubenvdlinde wants to merge 5 commits into
Conversation
…nlink (#793) `unlinkSubCases()` is a synchronous fan-out of `saveObject()` on the request path — the openregister#2420 shape, invisible to gate-61 because that gate inspects post-event listeners and this is a plain service seam.⚠️ The remedy the issue proposes first — defer the whole thing to a background job — cannot be applied as-is, and that is the main finding here. `DeelzaakDeleteWarningModal::confirmDelete()` awaits this call and then deletes the parent. Enqueue-and-return would delete the parent while the children still point at it, so the "orphans survive" guarantee (REQ-DZS-006-B) would be broken by the very change meant to protect it. Deferral needs the parent delete moved into the same job; that is left to #793 and deliberately NOT done here. What IS fixed, because it is a correctness bug rather than a performance one: the implementation took `listSubCases()`'s `_limit => 200` page and reported plain success. A parent with more than 200 sub-cases had the remainder silently left linked, and the caller then deleted the parent — orphaning them under a dead reference while the API answered `200 OK` with a count that under-reported. Per-record failures were swallowed the same way. Changes - `unlinkSubCases()` pages to exhaustion and returns `{unlinked, failed, total, complete}`. A `$seen` set makes each sub-case count exactly once: a record whose save fails stays in the filter's result set, so without it the loop both spins forever and double-counts. Extracted the per-page work into `unlinkPage()` to keep the complexity within budget. - `DeelzaakController::unlink()` answers `207 Multi-Status` when the unlink is incomplete, so a partial result is distinguishable by status code alone. - `deelzaakApi.unlinkSubCases()` returns the whole result. `complete` defaults to false when the field is absent, so an older server that still answers with a bare count blocks the delete rather than silently permitting the failure mode this change closes. - `DeelzaakDeleteWarningModal` deletes the parent ONLY when `complete` is true, and otherwise tells the user how many sub-cases could not be detached. Tests `DeelzaakUnlinkSubCasesTest` — 6 cases over a stub that pages the way OpenRegister does. Includes the negative control (no sub-cases → complete zero, without which an always-incomplete implementation would pass the failure tests), the 450-records-over-a-200-page regression, and the all-failures case that hangs the suite if the no-progress guard is removed. That last test caught a real defect in my first implementation: failed records were re-queried on the next page and double-counted, reporting `failed: 2` for one failing record. composer check:strict: ALL CHECKS PASSED (1791 tests). vitest 13 passed. eslint 0 errors. USE_LOCAL_LIB=false npm run build: exit 0. Refs openregister#2420
rubenvdlinde
requested review from
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 11, 2026 18:40
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:57 UTC
Download the full PDF report from the workflow artifacts.
Three CI failures on this PR, all real and all mine. Each reproduced locally before fixing, and re-measured after. gate-16 spec-coverage — `deelzaakApi.js::unlinkSubCases` had a JSDoc but no `@spec` anchor. Reproduced against the canonical gate package (.github @81c8c97) with an explicit base: `HYDRA_GATE_BASE_REF=origin/development` → `count=1` naming that method, `count=0` after. Anchored at the existing 'Sub-case deletion protection' requirement, which already says the system MUST clear parentCase on ALL child cases — I did not write a requirement to justify the change; the requirement is what the change implements. l10n coverage / test:l10n — the new partial-unlink error string was missing from en.json and nl.json. Extracted and translated; the check now reports both key sets matching. Coverage ratchet — the PHPUnit cells failed with no failing test, which is the ratchet, not a broken suite. Covered the new controller code rather than touching the threshold: DeelzaakUnlinkControllerTest asserts 200 on a complete unlink, 207 on a partial one, 401 for an anonymous caller, and a clean 200 when there are no sub-cases. The 200 case is the negative control — without it, a controller that always answered 207 would satisfy the 207 test on its own.⚠️ The ratchet could not be reproduced locally: no code-coverage driver is installed here, so CI is the authority on whether this clears it. phpcs 0 errors, phpmd clean, PHPUnit 10 new tests pass, vitest 13 pass.
…ed risky phpunit.xml sets beStrictAboutCoverageMetadata="true". DeelzaakUnlinkSubCasesTest declared only @Covers ::unlinkSubCases, but the paging loop calls listSubCases() and the service is built with a REAL CaseObjectReader (as DeelzaakServiceTest does, for the same reason) — so both execute without being the subject and every case would be reported RISKY, failing the PHPUnit cell with no failing test. Caught by the identical failure on the sibling REQ-003 branch rather than by waiting for this PR's cells to come back red.⚠️ Not reproducible locally: the risky-test check only fires when coverage is collected, and there is no coverage driver in this environment, so every local run (--no-coverage) is silent about 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:28 UTC
Download the full PDF report from the workflow artifacts.
…notation Same defect the sibling REQ-003 branch hit, fixed here before this PR's cells reported it: the docblock prose explaining the @uses declarations contained a literal coverage-annotation token followed by the word 'so'. PHPUnit matches those tokens MID-LINE anywhere in a docblock, so it registered a coverage target literally named 'so' and failed the cell with an 'is invalid' warning while every test passed. Rewritten to describe the annotations without naming them. 🔑 Same family as the fleet's gate-19/gate-26 prose-parsing defect (.github#358): an explanatory comment is not inert. Checked the rest of tests/ for the shape — the three pre-existing occurrences are backtick-wrapped and demonstrably do not warn (CI's 16 warnings were all from the one file).
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:34 UTC
Download the full PDF report from the workflow artifacts.
…g risky Third and final shape of this one. After the prose fix the cell still failed: `Tests: 1787 … Risky: 5`, naming DeelzaakController and DeelzaakService as 'executed code that is not listed as code to be covered or used'. Cause: I declared coverage METHOD-scoped (::unlink, ::unlinkSubCases). Under beStrictAboutCoverageMetadata, constructing the subject runs `__construct` — and for the service also the SearchesObjects trait — none of which IS the named method, so they count as undeclared even though the collaborators were declared. Fixed by matching the convention already used throughout tests/Unit: DeelzaakServiceTest and InspectionChecklistControllerTest both declare at class scope. This also explains why the sibling #794 branch never hit it — its tests were class-scoped from the start, and its ConfiguredRegistryService trait usage was attributed to the class rather than reported. 🔑 Method-scoped coverage metadata is strictly harder to satisfy than class-scoped, and the difference only shows up when coverage is collected — which this environment cannot do.
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:41 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-11 20:05 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
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.
fix(deelzaak): stop unlinkSubCases reporting success over a partial unlink (#793)
unlinkSubCases()is a synchronous fan-out ofsaveObject()on the requestpath — the openregister#2420 shape, invisible to gate-61 because that gate
inspects post-event listeners and this is a plain service seam.
job — cannot be applied as-is, and that is the main finding here.
DeelzaakDeleteWarningModal::confirmDelete()awaits this call and then deletesthe parent. Enqueue-and-return would delete the parent while the children still
point at it, so the "orphans survive" guarantee (REQ-DZS-006-B) would be broken
by the very change meant to protect it. Deferral needs the parent delete moved
into the same job; that is left to #793 and deliberately NOT done here.
What IS fixed, because it is a correctness bug rather than a performance one:
the implementation took
listSubCases()'s_limit => 200page and reportedplain success. A parent with more than 200 sub-cases had the remainder silently
left linked, and the caller then deleted the parent — orphaning them under a
dead reference while the API answered
200 OKwith a count that under-reported.Per-record failures were swallowed the same way.
Changes
unlinkSubCases()pages to exhaustion and returns{unlinked, failed, total, complete}. A$seenset makes each sub-case countexactly once: a record whose save fails stays in the filter's result set, so
without it the loop both spins forever and double-counts. Extracted the
per-page work into
unlinkPage()to keep the complexity within budget.DeelzaakController::unlink()answers207 Multi-Statuswhen the unlink isincomplete, so a partial result is distinguishable by status code alone.
deelzaakApi.unlinkSubCases()returns the whole result.completedefaultsto false when the field is absent, so an older server that still answers with
a bare count blocks the delete rather than silently permitting the failure
mode this change closes.
DeelzaakDeleteWarningModaldeletes the parent ONLY whencompleteis true,and otherwise tells the user how many sub-cases could not be detached.
Tests
DeelzaakUnlinkSubCasesTest— 6 cases over a stub that pages the wayOpenRegister does. Includes the negative control (no sub-cases → complete zero,
without which an always-incomplete implementation would pass the failure
tests), the 450-records-over-a-200-page regression, and the all-failures case
that hangs the suite if the no-progress guard is removed. That last test caught
a real defect in my first implementation: failed records were re-queried on the
next page and double-counted, reporting
failed: 2for one failing record.composer check:strict: ALL CHECKS PASSED (1791 tests). vitest 13 passed.
eslint 0 errors. USE_LOCAL_LIB=false npm run build: exit 0.
Refs openregister#2420