refactor(procest): rename the daily-scan and seed-data deadline classes - #806
Merged
Merged
Conversation
…tionDispatchJob A code-only slice of openspec/changes/english-vocabulary, and the first procest slice — chosen because it is one of the very few procest identifiers that is neither ZGW protocol vocabulary nor wired by string. 'termijn' here is a STATUTORY DEADLINE being monitored, not a contract term, so it becomes Deadline. That reading is app-specific on purpose: hrmq uses 'termijn' for notice periods (aanzegtermijn) and decidesk for terms of office (TermijnRegeling). Three apps, three meanings, one Dutch word — it must never become a fleet word. Referenced only via ::class from TermijnNotificationService, so a missed reference is a compile error rather than a job that silently never dispatches. No route entry, no register-fragment wiring, no baseline suppression keyed to the old path — all four checked, not assumed. TermijnNotificationService itself keeps its name in this commit; it belongs with the termijnbewaking slice, where the Termijn* schemas (termijnDefinitie, termijnInstance, termijnGebeurtenis) move together with their register fragments. Archived changes under openspec/changes/archive/ keep the old name — they are a record of what was built at the time. php -l clean; class name matches file name; use statement still ordered.
TermijnReportingController -> DeadlineReportingController and TermijnReportingService -> DeadlineReportingService, with their two Dutch controller methods (kwartaalrapport -> quarterlyReport, jaarrekening -> annualStatement), every Dutch parameter and local ($periode -> $period, $afdeling -> $department, $jaar -> $year, $jaarPrefix -> $yearPrefix), the two named-argument call sites, the three route names, the test, and the Postman collection's description. Stacked on feat/english-vocabulary-deadline-job (#797) because that branch already renames TermijnNotificationDispatchJob; branching from development would have conflicted. FOUR DUTCH CONTRACTS ARE DELIBERATELY LEFT ALONE, and each is annotated in place so the next reader does not "finish the job" and break it: 1. $row['afdeling'] is a SCHEMA PROPERTY. OpenRegister materialises it as a real column, so it moves with a data migration, not with a rename. 2. 'periode', 'afdeling' and 'jaar' as RESPONSE KEYS are the published shape of the reports. src/views/dashboard/TermijnDashboard.vue reads annual.jaar directly, so they move only with a coordinated frontend change. 3. getParam('periode') and getParam('jaar') are QUERY-STRING parameter names — the request contract. 4. The /api/termijn/* URLs. Route NAMES follow the renamed class and methods; URLs do not. Moving them is a breaking change for every consumer and is a separate decision. NAMED ARGUMENTS. resolveQuarter(periode:) and aggregateByType(afdeling:) are called with named arguments, which PHP resolves at call time — a parameter rename that misses one is a runtime fatal that `php -l` reports as clean. Both moved with their declarations. VERIFIED - php -l clean on all four PHP files; the Postman collection still parses. - phpunit: 5 tests, 14 assertions green. - Route resolution: each 'deadlineReporting#<action>' route name was checked against the controller's actual public methods — all three resolve. Positive control: the same check against the OLD action names (kwartaalrapport, jaarrekening) reports MISSING METHOD, so it can distinguish a renamed route from a dangling one. - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to either old class name or the old route prefix. PHPCS SCOPE, MEASURED RATHER THAN ASSUMED. phpcs.xml declares <file>lib</file> and nothing else, so tests/ and appinfo/ are outside the standard entirely — appinfo/routes.php carries 373 violations on development and every sibling test file is equally "dirty". Both files this change touches inside lib/ are CLEAN, including two pre-existing missing-@SPEC errors fixed here (each now anchors to openspec/specs/termijn-reporting/spec.md, the canonical spec, not the change directory the file headers point at). An earlier pass ran phpcbf over the test file and produced a 94-line diff for what is a 6-line rename. That was churn against a gate that never reads the file, so it was reverted; the test diff is now the rename alone.
…ner -> Deadline* Four more classes in the deadline cluster: TermijnEscalationService -> DeadlineEscalationService TermijnPauseService -> DeadlinePauseService TermijnExtensionService -> DeadlineExtensionService TermijnCaseCreatedListener -> DeadlineCaseCreatedListener plus the test file, across 11 files: the DI listener registration, the daily scan job and service, TermijnController, and three test suites. Stacked on feat/english-vocabulary-deadline-reporting (#803), which is itself stacked on #797. Merge order: 797 -> 803 -> this. THE ONE DEFECT THIS SLICE PRODUCED, AND WHAT CAUGHT IT. Renaming the test FILE TermijnPauseExtensionServiceTest.php left the class inside it still called TermijnPauseExtensionServiceTest — neither of the two service-name replacements matched that token, because it is a third name. PHPUnit refused to load it ("Class DeadlinePauseExtensionServiceTest cannot be found"). The basename == class invariant I ran before committing covered only the four lib/ files and would have missed it; it now covers the tests too. Same failure mode as hrmq's provider discovery, reached from the opposite direction. LISTENER WIRING IS SAFE BY CONSTRUCTION. WorkflowListenerRegistrar registers the listener as TermijnCaseCreatedListener::class, not as a string literal, so the rename is resolved at compile time rather than silently failing to match at runtime. Checked before renaming rather than after. VERIFIED - php -l clean on every changed file. - phpunit, whole unit suite: 1777 tests, 6160 assertions, 5 skipped — green. - Every lib/ file this change touches is phpcs CLEAN. Three pre-existing issues were fixed along the way: docblock parameter-type alignment in TermijnController and TermijnDailyScanService (shifted by the longer type names), and two missing class-level @SPEC tags. Those now anchor to canonical specs — termijn-binding + termijn-pause-extension for the controller, which spans both, and termijn-escalation for the daily scan — rather than to the change directories the file headers still point at. - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to any of the four old names. phpcs.xml scopes to <file>lib</file> only, so tests/ and appinfo/ are outside the standard; no formatting was applied to them.
TermijnDailyScanService -> DeadlineDailyScanService TermijnbewakingSeedDataService -> DeadlineMonitoringSeedDataService DailyTermijnScanJob -> DailyDeadlineScanJob SeedTermijnbewakingData -> SeedDeadlineMonitoringData TermijnbewakingEndToEndTest -> DeadlineMonitoringEndToEndTest Stacked on feat/english-vocabulary-deadline-services (#804). Merge order: 797 -> 803 -> 804 -> this. TWO OF THESE ARE WIRED IN appinfo/info.xml, NOT BY IMPORT. The job is declared as <job>OCA\Procest\BackgroundJob\DailyTermijnScanJob</job> and the seeder as <step>OCA\Procest\Repair\SeedTermijnbewakingData</step>. These fail differently from each other, which is why both were checked rather than assumed: - an unresolvable JOB is silently reaped. Nextcloud's JobList::buildJob() logs "failed to create instance of background job" and calls removeById(), so the daily termijn sweep would simply stop running with no error surface. - an unresolvable REPAIR STEP throws during upgrade. Both entries move here. Verified by walking info.xml and resolving every class it names to a file on disk: all 13 registered jobs plus the repair step resolve. Positive control: moving DailyDeadlineScanJob.php away makes that same walk report MISSING, so the check can fail. TEST CLASS NAMES. Three of the four test classes are prefixed by the service name they cover, so the service replacement carried them; only TermijnbewakingEndToEndTest was an independent token and was renamed explicitly. The basename == class invariant was run over lib/ AND tests/ this time — the previous slice (#804) ran it over lib/ only and missed exactly this class of defect. VERIFIED - php -l clean on every changed file; info.xml parses. - phpunit, whole unit suite: 1777 tests, 6160 assertions, 5 skipped — green. - Every lib/ file this change touches is phpcs CLEAN. Pre-existing issues fixed along the way: docblock parameter-type alignment in the job and the repair step (shifted by the longer injected type names), and three missing @SPEC tags now anchored to canonical specs (termijn-escalation for the scan job, termijnbewaking-schemas for the seeder). - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to any of the five old names. This caught two prose references in unrelated files (VthSeedDataRepairStep and KccWerkplekSeedDataService) that name the seeder in comments; both updated so the comments stay true.
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
August 11, 2026 20:54
This was referenced Aug 11, 2026
Base automatically changed from
feat/english-vocabulary-deadline-services
to
development
August 11, 2026 22:18
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn and
remko48
as code owners
August 11, 2026 22:18
rubenvdlinde
changed the base branch from
feat/english-vocabulary-deadline-services
to
development
August 11, 2026 22:18
…ocabulary-deadline-scan # Conflicts: # tests/Unit/Service/DeadlineDailyScanServiceTest.php # tests/Unit/Service/DeadlineMonitoringEndToEndTest.php
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 22:39 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 11, 2026
* refactor(procest): TermijnNotificationDispatchJob -> DeadlineNotificationDispatchJob A code-only slice of openspec/changes/english-vocabulary, and the first procest slice — chosen because it is one of the very few procest identifiers that is neither ZGW protocol vocabulary nor wired by string. 'termijn' here is a STATUTORY DEADLINE being monitored, not a contract term, so it becomes Deadline. That reading is app-specific on purpose: hrmq uses 'termijn' for notice periods (aanzegtermijn) and decidesk for terms of office (TermijnRegeling). Three apps, three meanings, one Dutch word — it must never become a fleet word. Referenced only via ::class from TermijnNotificationService, so a missed reference is a compile error rather than a job that silently never dispatches. No route entry, no register-fragment wiring, no baseline suppression keyed to the old path — all four checked, not assumed. TermijnNotificationService itself keeps its name in this commit; it belongs with the termijnbewaking slice, where the Termijn* schemas (termijnDefinitie, termijnInstance, termijnGebeurtenis) move together with their register fragments. Archived changes under openspec/changes/archive/ keep the old name — they are a record of what was built at the time. php -l clean; class name matches file name; use statement still ordered. * refactor(procest): TermijnReporting* -> DeadlineReporting* TermijnReportingController -> DeadlineReportingController and TermijnReportingService -> DeadlineReportingService, with their two Dutch controller methods (kwartaalrapport -> quarterlyReport, jaarrekening -> annualStatement), every Dutch parameter and local ($periode -> $period, $afdeling -> $department, $jaar -> $year, $jaarPrefix -> $yearPrefix), the two named-argument call sites, the three route names, the test, and the Postman collection's description. Stacked on feat/english-vocabulary-deadline-job (#797) because that branch already renames TermijnNotificationDispatchJob; branching from development would have conflicted. FOUR DUTCH CONTRACTS ARE DELIBERATELY LEFT ALONE, and each is annotated in place so the next reader does not "finish the job" and break it: 1. $row['afdeling'] is a SCHEMA PROPERTY. OpenRegister materialises it as a real column, so it moves with a data migration, not with a rename. 2. 'periode', 'afdeling' and 'jaar' as RESPONSE KEYS are the published shape of the reports. src/views/dashboard/TermijnDashboard.vue reads annual.jaar directly, so they move only with a coordinated frontend change. 3. getParam('periode') and getParam('jaar') are QUERY-STRING parameter names — the request contract. 4. The /api/termijn/* URLs. Route NAMES follow the renamed class and methods; URLs do not. Moving them is a breaking change for every consumer and is a separate decision. NAMED ARGUMENTS. resolveQuarter(periode:) and aggregateByType(afdeling:) are called with named arguments, which PHP resolves at call time — a parameter rename that misses one is a runtime fatal that `php -l` reports as clean. Both moved with their declarations. VERIFIED - php -l clean on all four PHP files; the Postman collection still parses. - phpunit: 5 tests, 14 assertions green. - Route resolution: each 'deadlineReporting#<action>' route name was checked against the controller's actual public methods — all three resolve. Positive control: the same check against the OLD action names (kwartaalrapport, jaarrekening) reports MISSING METHOD, so it can distinguish a renamed route from a dangling one. - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to either old class name or the old route prefix. PHPCS SCOPE, MEASURED RATHER THAN ASSUMED. phpcs.xml declares <file>lib</file> and nothing else, so tests/ and appinfo/ are outside the standard entirely — appinfo/routes.php carries 373 violations on development and every sibling test file is equally "dirty". Both files this change touches inside lib/ are CLEAN, including two pre-existing missing-@SPEC errors fixed here (each now anchors to openspec/specs/termijn-reporting/spec.md, the canonical spec, not the change directory the file headers point at). An earlier pass ran phpcbf over the test file and produced a 94-line diff for what is a 6-line rename. That was churn against a gate that never reads the file, so it was reverted; the test diff is now the rename alone. * refactor(procest): Termijn{Escalation,Pause,Extension}Service + listener -> Deadline* Four more classes in the deadline cluster: TermijnEscalationService -> DeadlineEscalationService TermijnPauseService -> DeadlinePauseService TermijnExtensionService -> DeadlineExtensionService TermijnCaseCreatedListener -> DeadlineCaseCreatedListener plus the test file, across 11 files: the DI listener registration, the daily scan job and service, TermijnController, and three test suites. Stacked on feat/english-vocabulary-deadline-reporting (#803), which is itself stacked on #797. Merge order: 797 -> 803 -> this. THE ONE DEFECT THIS SLICE PRODUCED, AND WHAT CAUGHT IT. Renaming the test FILE TermijnPauseExtensionServiceTest.php left the class inside it still called TermijnPauseExtensionServiceTest — neither of the two service-name replacements matched that token, because it is a third name. PHPUnit refused to load it ("Class DeadlinePauseExtensionServiceTest cannot be found"). The basename == class invariant I ran before committing covered only the four lib/ files and would have missed it; it now covers the tests too. Same failure mode as hrmq's provider discovery, reached from the opposite direction. LISTENER WIRING IS SAFE BY CONSTRUCTION. WorkflowListenerRegistrar registers the listener as TermijnCaseCreatedListener::class, not as a string literal, so the rename is resolved at compile time rather than silently failing to match at runtime. Checked before renaming rather than after. VERIFIED - php -l clean on every changed file. - phpunit, whole unit suite: 1777 tests, 6160 assertions, 5 skipped — green. - Every lib/ file this change touches is phpcs CLEAN. Three pre-existing issues were fixed along the way: docblock parameter-type alignment in TermijnController and TermijnDailyScanService (shifted by the longer type names), and two missing class-level @SPEC tags. Those now anchor to canonical specs — termijn-binding + termijn-pause-extension for the controller, which spans both, and termijn-escalation for the daily scan — rather than to the change directories the file headers still point at. - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to any of the four old names. phpcs.xml scopes to <file>lib</file> only, so tests/ and appinfo/ are outside the standard; no formatting was applied to them. * refactor(procest): rename the daily-scan and seed-data deadline classes TermijnDailyScanService -> DeadlineDailyScanService TermijnbewakingSeedDataService -> DeadlineMonitoringSeedDataService DailyTermijnScanJob -> DailyDeadlineScanJob SeedTermijnbewakingData -> SeedDeadlineMonitoringData TermijnbewakingEndToEndTest -> DeadlineMonitoringEndToEndTest Stacked on feat/english-vocabulary-deadline-services (#804). Merge order: 797 -> 803 -> 804 -> this. TWO OF THESE ARE WIRED IN appinfo/info.xml, NOT BY IMPORT. The job is declared as <job>OCA\Procest\BackgroundJob\DailyTermijnScanJob</job> and the seeder as <step>OCA\Procest\Repair\SeedTermijnbewakingData</step>. These fail differently from each other, which is why both were checked rather than assumed: - an unresolvable JOB is silently reaped. Nextcloud's JobList::buildJob() logs "failed to create instance of background job" and calls removeById(), so the daily termijn sweep would simply stop running with no error surface. - an unresolvable REPAIR STEP throws during upgrade. Both entries move here. Verified by walking info.xml and resolving every class it names to a file on disk: all 13 registered jobs plus the repair step resolve. Positive control: moving DailyDeadlineScanJob.php away makes that same walk report MISSING, so the check can fail. TEST CLASS NAMES. Three of the four test classes are prefixed by the service name they cover, so the service replacement carried them; only TermijnbewakingEndToEndTest was an independent token and was renamed explicitly. The basename == class invariant was run over lib/ AND tests/ this time — the previous slice (#804) ran it over lib/ only and missed exactly this class of defect. VERIFIED - php -l clean on every changed file; info.xml parses. - phpunit, whole unit suite: 1777 tests, 6160 assertions, 5 skipped — green. - Every lib/ file this change touches is phpcs CLEAN. Pre-existing issues fixed along the way: docblock parameter-type alignment in the job and the repair step (shifted by the longer injected type names), and three missing @SPEC tags now anchored to canonical specs (termijn-escalation for the scan job, termijnbewaking-schemas for the seeder). - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to any of the five old names. This caught two prose references in unrelated files (VthSeedDataRepairStep and KccWerkplekSeedDataService) that name the seeder in comments; both updated so the comments stay true. * feat(repair): migrate procest's Dutch deadline columns to English Adds RenameDutchDeadlineColumns, the data-migration half of procest's English vocabulary slice. No property is renamed in this commit — the migration lands first, because TermijnService and TermijnController cannot be renamed until the stored data can follow them. Stacked on feat/english-vocabulary-deadline-scan (#806). Merge order: 797 -> 803 -> 804 -> 806 -> this. WHY A MIGRATION IS NEEDED AT ALL. OpenRegister does not store an object as a JSON blob keyed by property name. Each schema property is a real, snake_cased COLUMN in oc_openregister_table_{register}_{schema}. MagicMapper ADDS a column on sync and never renames. A register-only rename therefore leaves the data in the Dutch column while every read looks at the English one and finds null. THIS APP HAS TWO REGISTERS, AND THE OBVIOUS SCOPING MISSES ONE. Measured on the reference install: `procest` holds 1051 rows and `procest-default` holds 107. The sibling step in decidesk resolves a single exact slug. Applying that shape here migrates 40 columns and silently leaves 29 behind — measured both ways: exact slug = 'procest' -> 40 columns slug LIKE 'procest%' -> 69 columns so the register set is resolved by PREFIX and every match is migrated. The failure that was avoided is the worst kind available here: a step that reports success having covered only part of the data. zaaktype IS EXEMPT AND DELIBERATELY ABSENT FROM THE MAP. It is the field name in the statutory ZGW wire format this app both consumes and emits, so it is exempt under the fleet rule — even though at 14 shard tables it is the second most widespread Dutch column here, which is exactly why leaving it out needed saying rather than looking like an oversight. COLLISIONS ARE REFUSED, NOT MERGED. `omschrijving` and `beschrijving` both mean `description`. Measured: they do not co-occur in any shard table on this install, so the guard is a safety net rather than an active path. A later fragment could introduce a pair, and a silent merge would destroy one of two values, so the step migrates neither and logs. SOFT-DELETED ROWS ARE MIGRATED TOO. The back-fill does not filter on `_deleted` — a restored row must not come back with a null subject. 36 non-null values exist across the mapped columns, 35 of them live. VERIFIED - php -l clean; info.xml parses; phpcs CLEAN under the app's standard, including its @SPEC anchor requirement. - Dry run of the step's exact resolution: 40 renames under `procest`, 29 under `procest-default`, 0 back-fills, 0 ambiguous. zaaktype untouched on all 14 tables carrying it. NOT VERIFIED. The step has not been executed against a database — only its resolution and its statement set were computed. It runs on the next upgrade; its idempotence and its back-fill branch are reasoned from the code and the sibling steps, not observed. Say so rather than let "migration verified" stand in for it. * fix(repair): use information_schema, not IDBConnection introspection phpstan failed this branch with three errors, all in the new repair step: Call to an undefined method OCP\IDBConnection::getPrefix(). Call to an undefined method OCP\IDBConnection::getSchema(). (x2) They are real. Read from the running server's own lib/public/IDBConnection.php, the interface exposes getQueryBuilder, getTypedQueryBuilder, getError, getDatabasePlatform, getDatabaseProvider, getShardDefinition and getCrossShardMoveHelper — and nothing else beginning with "get". Both methods I called exist on the concrete OC\DB\Connection, not on the OCP interface this step is typed against. WHY EVERY OTHER CHECK PASSED. `php -l` parses a call to a method that does not exist, and phpcs is a style tool. A nonexistent method on an injected interface is invisible to both. Only phpstan sees it, and I did not run phpstan locally before opening the PR — I described the step as verified on the strength of lint + phpcs + a SQL dry run, none of which touch the PHP API surface. The dry run in particular measured what the STATEMENTS would do, which is exactly why it looked convincing while the code that would issue them could not run at all. THE FIX follows openregister's own RegisterService::magicTableNames(), which solves the same problem: query information_schema and anchor the match on the `openregister_table_` MARKER rather than a computed prefix. That file documents why the obvious alternative fails — getQueryBuilder()->getTableName('') returns the literal `*PREFIX*` placeholder, resolved only when a query executes through the NC DB layer, which a raw information_schema string never is; a LIKE built from it matches zero tables and silently reports every register empty. Column introspection moves to information_schema.columns for the same reason. Matching is tightened while here: the suffix after `openregister_table_<id>_` must be all digits, so register 17 cannot match register 170's shard tables. VERIFIED - phpstan: [OK] No errors, whole-project run, same command as CI. - php -l clean; no `db->getSchema()` or `db->getPrefix()` call remains. THE SAME DEFECT IS IN FOUR SIBLING PRs, all authored today and all failing phpstan for this identical reason: openbuild#176, opencatalogi#850, decidesk#467 and softwarecatalog#488. Each needs this same change. None can merge while red, so nothing shipped — but each of their PR bodies claims verification that never covered the API surface. * style(repair): split shardTables() and drop the short variable phpmd flagged two things the information_schema rewrite introduced: CyclomaticComplexity shardTables() is at 10; the threshold is 10. ShortVariable $at is below the 3-character minimum. The marker-matching loop moves into isShardOf(), which fixes both: the extraction takes shardTables() back under the threshold, and $at becomes $offset. Behaviour is unchanged — same marker anchoring, same digits-only suffix check that keeps register 17 from matching register 170's tables. Also capitalises one inline comment, which phpcs requires. ALL THREE GATES RUN LOCALLY THIS TIME, which is the point. The original defect in this file shipped because I ran php -l and phpcs and called it verified; phpstan was the only gate that could see a call to a nonexistent interface method, and I had not run it. This round: phpcs CLEAN phpstan [OK] No errors phpmd 0 findings on this file * test(repair): cover the migration's scoping decisions The PHPUnit job was failing on the COVERAGE RATCHET, not on a test: Coverage current: 30.52% (14092/46169 statements) Coverage merge base: 30.55% (14000/45824 statements) FAIL: coverage dropped by 0.03% against the merge base. This change adds 345 statements. Adding code without tests drops coverage. Both suite legs reported 1803 and 1777 tests with zero failures, so "PHPUnit FAILED" here meant "you added untested code" — which was true. The migration shipped with no test at all. These ten tests pin the two decisions that determine what the step touches — which shard tables are in scope, and when a rename is refused — plus two invariants that are load-bearing but were only asserted in prose: that `zaaktype` stays out of the map because it is the statutory ZGW wire name, and that every destination is snake_case (MagicMapper DROPS a camelCase column whose snake_case twin exists, so a camelCase destination would be deleted). The DDL/DML paths are deliberately not unit-tested: they need a live database. A POSITIVE CONTROL FOUND A FALSE CLAIM IN MY OWN COMMENT. The first version of the shard-matching test asserted that register 17 must not match register 170's tables, quoting the comment above the guard. Deleting the ctype_digit guard and re-running left that test GREEN — it could not fail, because the marker already ends in '_', so `openregister_table_17_` is not a substring of `openregister_table_170_85`. The trailing underscore handles 17-vs-170; the digits check guards something else entirely: derived tables like `…_17_85_backup` and non-shards like `…_17_audit`, both of which contain the marker and would otherwise receive an ALTER TABLE. The test now asserts that case and goes RED when the guard is removed. The code comment, which stated the wrong reason, is corrected in the same commit. VERIFIED - phpunit: 10 tests, 24 assertions green; whole unit suite 1787/6184. - Positive control: guard removed -> 1 failure; restored -> 10 pass. - phpcs clean, phpstan [OK] No errors.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 12, 2026
…l six owners (#816) * refactor(mandaat): rename the mandateringsBesluit schema to mandateDecision Applies the ratified besluit = decision equivalence to procest's mandate schema: the `mandateringsBesluit` schema (key, slug, title, description) becomes `mandateDecision`, with five of its properties and the cross-schema reference that names it. besluitNummer -> decisionNumber besluitNaam -> decisionName inWerkingtreding -> effectiveFrom vervalDatum -> expiryDate vorigBesluit -> previousDecision mandaat.mandateringsBesluit -> mandaat.mandateDecision THERE WAS NEVER A SCHEMA MERGE TO DO. This slice was previously described as blocked because procest "already declares a decision schema, so renaming Besluit onto it is a merge". Measured: procest declares exactly one `decision` schema, already fully English with twelve English properties, and there is no bare `Besluit` schema anywhere to collide with it. What carries the word is two differently-named schemas and five properties. The premise was wrong. WHAT IS DELIBERATELY NOT RENAMED `wettelijkeGrondslag` stays. It is owned by FIVE other procest schemas (dwangsomUitbetaling, mandaat, subsidieBeschikking, termijnDefinitie, terugvordering) that still declare the Dutch spelling, and the column migration is REGISTER-scoped: renaming it here would migrate their data out from under their own declarations. A shared property half-renamed is worse than one not renamed at all. It moves fleet-wide, with them, in one slice. `legalBasis` already exists on another procest schema, which is a second reason not to introduce a partial second source for it. The `status` enum VALUES (concept / vastgesteld / vervallen) stay. Enum values are stored data, not identifiers — rewriting them rewrites every row. Same call as openconnector#1213. The SchemaSlugMap VALUE stays `mandaterings_besluit_schema`. Only the key is the slug; the value is the app-config key under which this schema's numeric id is already stored on every existing install. Renaming it would orphan that id and the schema would silently resolve to nothing. The import endpoint accepts BOTH spellings. Its request-body keys are a published contract; the English names are preferred and the Dutch ones still read, so a client that has not been updated does not break. MIGRATION. Six columns added to the existing register-scoped repair step. Each was verified to be owned by exactly one schema before being added — that check is what kept wettelijke_grondslag out, and it is the same hazard decidesk's step avoids by scoping to a register, only here it is within one app. THREE PRE-EXISTING UI BUGS FOUND, DELIBERATELY NOT "FIXED" BY RENAMING - MandaatEditor.vue binds form.inWerkingtreding onto a MANDAAT, which declares validFrom/validUntil and has no such property. - MandaatMatrixTable.vue reads b.vervaldatum — lowercase d — matching no schema property at all. - BevoegdhedenPanel.vue reads both off API rows whose shape is not verifiable without running it. Renaming these would have cemented bindings that already point at nothing. Only MandaatMatrixWidget.vue, which reads the renamed reference property, was updated. The three need their own investigation. VERIFIED - phpunit: 1867 tests, 6346 assertions green (5 skipped, same as baseline). - php -l clean; the fragment parses as JSON. - Named-argument parity: findPriorBesluit -> findPriorDecision, parameter besluitNummer -> decisionNumber, both call sites moved and diffed against the declaration. - phpcs: 0 errors on every file this change touches except MandaatImportServiceTest, which carries 46 — identical count on the unmodified file, measured by stashing. * refactor(procest): rename wettelijkeGrondslag to legalBasis, all six owners Renames the property across EVERY schema that declares it, in one change: dwangsomUitbetaling, mandaat, mandateDecision, subsidieBeschikking, termijnDefinitie and terugvordering — plus the seed data, six services, three Vue files and the affected test fixtures. WHY ALL SIX AT ONCE. This was deliberately held out of the mandate-decision slice. The column migration is REGISTER-scoped: it walks every shard table in the procest registers and renames any column whose name is in its map. Renaming `wettelijkeGrondslag` for one schema would therefore have moved the data of the other five out from under declarations that still used the Dutch name — a silent null-read on each. A shared property is renamed for all its owners or for none. With all six moved, `wettelijke_grondslag => legal_basis` is safe to add to the map, and it is added here. MY FIRST MEASUREMENT OF THE FOOTPRINT WAS SHORT. It ran against the shared checkout, where TermijnReportingService.php had not yet been renamed to DeadlineReportingService.php by #806, so that consumer did not appear. Three Vue files were missed the same way. The residual grep after editing is what surfaced them; the count went from 27 references to 40. NOT EVERY VUE BINDING WAS REAL. The sibling slice found MandaatEditor binding `inWerkingtreding` onto a mandaat that has no such property. This one is different and was checked rather than assumed: `mandaat` genuinely declares wettelijkeGrondslag, so its editor field, its validator and the matrix widget's legalLink() all bind something that exists, and all three move. THE CSV HEADER STAYS READABLE. MandaatImportService reads the legal basis from an operator-supplied CSV column. That header is an external input format, so the new spelling is preferred and the old one still accepted — an existing import file does not break. VERIFIED - phpunit: 1867 tests, 6347 assertions green (5 skipped, same as baseline). - Two tests failed on the first run and were fixed: an assertion in DwangsomUitbetalingServiceTest reading the old key, and four fixtures in TermijnServiceTest. Both now pass. - php -l clean on every changed file; all four JSON files parse. - phpcs: TermijnServiceTest ends at 47 errors against a 74-error baseline — my edit first pushed it to 96 through misaligned array arrows, and phpcbf both corrected that and cleared pre-existing debt. Every other changed file is at or below its baseline. * style(mandaat): realign the import payload array after the legalBasis rename phpcs failed #816 with 7 'Array double arrow not aligned correctly' errors, all in the payload built by buildMandaatPayload(). The cause is mine: 'legalBasis' is shorter than 'wettelijkeGrondslag', so every arrow in that array needed re-aligning and I did not re-run phpcs on this file after the edit. phpcbf-applied; the file is back to 0 errors. No behaviour change — whitespace only, confirmed by the diff being 7 insertions and 7 deletions on the same lines, and by MandaatImportServiceTest still passing. * docs(mandaat): add @SPEC anchors to the two methods this PR changed gate-16 (spec-coverage) failed #816 with '2 changed method(s) missing @SPEC'. The gate is diff-scoped and it is right: buildMandaatPayload() and collectChangedFields() were both edited by the legalBasis rename and neither carried an anchor. gate-16 PASSES on development, so this is caused by this branch, not inherited. Both now point at the change that introduced them. NOT FIXED HERE: gate-53 (effective-manifest-crossref, 8 failures). Measured against development's own latest Code Quality run — it reports the IDENTICAL 8 cross-reference failures there, so it is pre-existing and untouched by this branch. src/manifest.json contains no reference to any property this PR renamed (grep for wettelijkeGrondslag returns 0).
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
TermijnDailyScanServiceDeadlineDailyScanServiceTermijnbewakingSeedDataServiceDeadlineMonitoringSeedDataServiceDailyTermijnScanJobDailyDeadlineScanJobSeedTermijnbewakingDataSeedDeadlineMonitoringDataTermijnbewakingEndToEndTestDeadlineMonitoringEndToEndTestStacked on #804. Merge order: #797 → #803 → #804 → this.
Two of these are wired in
appinfo/info.xml, not by importThey fail differently from each other, which is why both were checked rather than assumed:
JobList::buildJob()logs "failed to create instance of background job" and callsremoveById(). The daily termijn sweep would simply stop running, with no error surface.Both entries move here. Verified by walking
info.xmland resolving every class it names to a file on disk: all 13 registered jobs plus the repair step resolve. Positive control: movingDailyDeadlineScanJob.phpaway makes that same walk report MISSING, so the check can fail.Test class names
Three of the four test classes are prefixed by the service they cover, so the service replacement carried them; only
TermijnbewakingEndToEndTestwas an independent token and was renamed explicitly.The basename-equals-class invariant ran over
lib/andtests/this time. #804 ran it overlib/only and missed exactly this class of defect — PHPUnit caught it there instead.Verification
php -lclean on every changed file;info.xmlparseslib/file this change touches is phpcs CLEANlib/,tests/,appinfo/,src/— no live reference to any of the five old namesPre-existing issues fixed along the way: docblock parameter-type alignment in the job and repair step (shifted by the longer injected type names), and three missing
@spectags now anchored to canonical specs (termijn-escalationfor the scan job,termijnbewaking-schemasfor the seeder).The residual grep also caught two prose references in unrelated files —
VthSeedDataRepairStepandKccWerkplekSeedDataServicename the seeder in comments. Both updated so the comments stay true.