feat(inspection): build ChecklistService, REQ-003 of the canonical spec (#786) - #798
Merged
Merged
Conversation
…ec (#786) REQ-003 of `openspec/specs/inspection-checklists/spec.md` was never built. This adds it: `OCA\Procest\Service\ChecklistService`, with the four methods the requirement names — `completeItem()`, `getProgress()`, `validateCompletion()` and `getConformitySummary()` — all pure with respect to the checklist payload, so the same calculation runs server-side, in a dry-run preview, and in a unit test.⚠️ I did NOT write the requirement. REQ-001, REQ-002 and REQ-003 already exist verbatim in the canonical spec under `## Execution Surface (retrofit)`, and the spec's frontmatter lists them under `retrofit_extensions`. The implementation follows that text rather than the other way round.⚠️ This is deliberately NOT a rename of `lib/Service/Inspection/ChecklistService.php`. The spec's own Notes block keeps the two apart on purpose — "the top-level service handles per-run progress, the namespaced one handles templates. Consolidation deferred to a future refactor change." Merging them would contradict the spec, not tidy it. Consistency with the existing implementation Answer semantics are taken from `Inspection\ChecklistService` so the two can never disagree about which answer is a failure: `ja_nee_nvt` → `nvt` is not-applicable and `nee` is non-conforming; `fotoRequired` is the tri-state `nooit`/`bij_nee`/`altijd`, not a boolean; an item's id is `id`, else `order`, else its position. Those SHAPE rules moved into `ChecklistPayloadReader` so they are stated once rather than restated per consumer — and so `ChecklistService` stays under the class-complexity budget. Two decisions worth reviewing rather than assuming - A response row that exists but carries no usable value is NOT an answer. Counting it would let a run of blank required items report itself complete. - `completeItem()` enforces the photo gate at answer time, not only at submission. REQ-001's scenario requires the item to REMAIN INCOMPLETE when a mandatory photo is missing, which validating only at submission would not do. Tests: 21 cases, including negative controls that would pass against a wrong implementation — an optional item may be left blank (a validator flagging every unanswered item would also satisfy the "reports both required items" test), an orphan response does not inflate progress, and an empty checklist is 100% rather than a division by zero. Still outstanding on #786, deliberately not attempted here: REQ-001 (`InspectionController`) and REQ-002 (`InspectionService`). REQ-001 delegates to this service, so it is now unblocked. composer lint/phpcs/phpmd/psalm/phpstan: clean. PHPUnit: 1793 tests, 6192 assertions, 0 failures. Refs #786
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
August 11, 2026 19:01
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:17 UTC
Download the full PDF report from the workflow artifacts.
… own suite The PHPUnit cells failed with 1793 tests passing and 8 RISKY — not the coverage ratchet and not a broken test. phpunit.xml sets `beStrictAboutCoverageMetadata="true"`, and ChecklistServiceTest declared only `@covers \OCA\Procest\Service\ChecklistService` while every case also executes ChecklistPayloadReader, which the service composes directly (REQ-003 requires it to be constructible with no dependencies, so the reader cannot be injected and mocked away). Fixed the way this repo already handles it — DeelzaakServiceTest carries the same `@uses` for CaseObjectReader for the same reason — plus a real `@covers` suite for the reader rather than leaving it covered only by association.⚠️ I did not catch this locally because every local run used `--no-coverage`: the risky-test check only fires when coverage is collected, and there is no coverage driver installed in this environment. A green `--no-coverage` run is not evidence about the cells that failed. ChecklistPayloadReaderTest also pins one thing worth keeping: PHP coerces a numeric-string array key to an int, so an item keyed by `order: 4` lands under int 4. Lookups coerce the same way so behaviour is correct, but an assertSame on array_keys() does not see what you wrote — the first draft of that test asserted the wrong thing and failed. phpcs 0 errors. 30 tests, 61 assertions pass.
…notation The @uses fix worked — 8 RISKY became 0 — and introduced 16 PHPUnit warnings, which fail the cell just as hard, with all 1802 tests passing: "@Covers suite" is invalid The docblock prose explaining the fix contained a literal coverage-annotation token followed by the word 'suite'. PHPUnit scans the whole docblock and matches those tokens MID-LINE, not just at the start of one, so it dutifully registered a coverage target literally named 'suite'. Rewritten to describe the annotations without naming them.⚠️ The first attempt at THIS fix reintroduced the bug, because the warning I wrote about the token contained the token. 🔑 Same family as the fleet's gate-19/gate-26 prose-parsing defect (.github#358): an explanatory comment is not inert, and documenting a parser-driven defect can re-trigger the defect you are documenting.
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.
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:51 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:23 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.
feat(inspection): build ChecklistService, REQ-003 of the canonical spec (#786)
REQ-003 of
openspec/specs/inspection-checklists/spec.mdwas never built. Thisadds it:
OCA\Procest\Service\ChecklistService, with the four methods therequirement names —
completeItem(),getProgress(),validateCompletion()and
getConformitySummary()— all pure with respect to the checklist payload,so the same calculation runs server-side, in a dry-run preview, and in a unit
test.
verbatim in the canonical spec under
## Execution Surface (retrofit), and thespec's frontmatter lists them under
retrofit_extensions. The implementationfollows that text rather than the other way round.
lib/Service/Inspection/ChecklistService.php.The spec's own Notes block keeps the two apart on purpose — "the top-level
service handles per-run progress, the namespaced one handles templates.
Consolidation deferred to a future refactor change." Merging them would
contradict the spec, not tidy it.
Consistency with the existing implementation
Answer semantics are taken from
Inspection\ChecklistServiceso the two cannever disagree about which answer is a failure:
ja_nee_nvt→nvtisnot-applicable and
neeis non-conforming;fotoRequiredis the tri-statenooit/bij_nee/altijd, not a boolean; an item's id isid, elseorder,else its position. Those SHAPE rules moved into
ChecklistPayloadReaderso theyare stated once rather than restated per consumer — and so
ChecklistServicestays under the class-complexity budget.
Two decisions worth reviewing rather than assuming
Counting it would let a run of blank required items report itself complete.
completeItem()enforces the photo gate at answer time, not only atsubmission. REQ-001's scenario requires the item to REMAIN INCOMPLETE when a
mandatory photo is missing, which validating only at submission would not do.
Tests: 21 cases, including negative controls that would pass against a wrong
implementation — an optional item may be left blank (a validator flagging every
unanswered item would also satisfy the "reports both required items" test), an
orphan response does not inflate progress, and an empty checklist is 100% rather
than a division by zero.
Still outstanding on #786, deliberately not attempted here: REQ-001
(
InspectionController) and REQ-002 (InspectionService). REQ-001 delegates tothis service, so it is now unblocked.
composer lint/phpcs/phpmd/psalm/phpstan: clean. PHPUnit: 1793 tests, 6192
assertions, 0 failures.
Refs #786