refactor(procest): TermijnService/Controller/NotificationService -> Deadline* - #808
Closed
rubenvdlinde wants to merge 6 commits into
Closed
refactor(procest): TermijnService/Controller/NotificationService -> Deadline*#808rubenvdlinde wants to merge 6 commits into
rubenvdlinde wants to merge 6 commits into
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.
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.
…eadline* The core of the deadline cluster, across 23 files: TermijnService -> DeadlineService TermijnController -> DeadlineController TermijnNotificationService -> DeadlineNotificationService plus the controller's four Dutch action methods — pauze -> pause, hervat -> resume, verleng -> extend, voltooi -> complete — and the six route names that address them. Stacked on feat/english-vocabulary-migration (#807). Merge order: 797 -> 803 -> 804 -> 806 -> 807 -> this. THE URLs DO NOT MOVE, AND THAT IS DELIBERATE. Route NAMES follow the renamed controller and methods; /api/termijn/instances/{id}/pauze and its siblings stay exactly as published. Moving them is a breaking change for every consumer and is a separate decision. Nothing references the route names — the frontend calls the URLs directly, checked before renaming. ONE DUTCH STRING IN THE CONTROLLER IS A DATA KEY, NOT AN IDENTIFIER: $body['voltooiDatum'] is a request-body field, part of the request contract. The four method renames were done by matching on the trailing "(" precisely so that this key could not be caught by the same replacement — verified present and unchanged afterwards. VERIFIED - php -l clean on all 23 files; the Postman collection still parses. - phpunit, whole unit suite: 1777 tests, 6160 assertions, 5 skipped — green. - basename == class across lib/ AND tests/ for all five renamed files. - Route resolution: all six deadline#<action> names resolve to real public methods on the controller. Positive control: the same check against the four OLD action names (pauze, hervat, verleng, voltooi) reports MISSING METHOD, so it can tell a renamed route from a dangling one. - Every touched lib/ file is phpcs CLEAN. 18 violations appeared purely from the one-character length change (TermijnService -> DeadlineService shifting docblock parameter-type alignment) and were auto-fixed. - Residual grep over lib/, tests/, appinfo/ and src/: no live reference to any of the three old class names. NOT IN THIS COMMIT, AND NAMED SO IT IS NOT MISTAKEN FOR DONE. 29 occurrences of the injected property/parameter name $termijnService (and bare $termijn) remain across 15 files — they now hold a DeadlineService, which reads oddly but is harmless and type-correct. Checked for the named-argument hazard first: the only `termijn:`-shaped named arguments in the tree are `bewaartermijn:` and `procestermijn:`, which are different concepts, so no call site is at risk. Splitting here keeps this commit at a fully-verified boundary.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 11, 2026 21:10
This was referenced Aug 11, 2026
Base automatically changed from
feat/english-vocabulary-migration
to
development
August 11, 2026 23:47
Contributor
Author
|
Closing as superseded — and, importantly, because merging it now would be a regression rather than a no-op. The Deadline rename landed on The reason to close rather than resolve the conflicts is the migration itself. On Neither method exists. Checked with a positive control on the same greps so an empty result could not be a broken lookup:
So this code path is On the point that prompted the review: a property rename is a data migration, and this branch was right to carry one. That migration is on Branch left in place, not deleted, since nothing was merged from it. |
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
The core of procest's deadline cluster, across 23 files:
TermijnServiceDeadlineServiceTermijnControllerDeadlineControllerTermijnNotificationServiceDeadlineNotificationServiceplus the controller's four Dutch action methods —
pauze→pause,hervat→resume,verleng→extend,voltooi→complete— and the six route names addressing them.Stacked on #807. Merge order: #797 → #803 → #804 → #806 → #807 → this.
The URLs do not move, deliberately
Route names follow the renamed controller and methods.
/api/termijn/instances/{id}/pauzeand its siblings stay exactly as published — moving them breaks every consumer and is a separate decision.Nothing references the route names; the frontend calls the URLs directly. Checked before renaming.
One Dutch string in the controller is a data key
$body['voltooiDatum']is a request-body field — part of the request contract, not an identifier.The four method renames were done by matching on the trailing
(precisely so this key could not be caught by the same replacement. Verified present and unchanged afterwards.Verification
php -lclean on all 23 files; the Postman collection still parseslib/andtests/for all five renamed filesdeadline#<action>names resolve to real public methods. Positive control: the same check against the four old names (pauze,hervat,verleng,voltooi) reports MISSING METHOD, so it can tell a renamed route from a dangling onelib/file is phpcs CLEAN. 18 violations appeared purely from the one-character length change (TermijnService→DeadlineServiceshifting docblock alignment) and were auto-fixedlib/,tests/,appinfo/,src/— no live reference to any of the three old class namesNot in this PR — named so it isn't mistaken for done
29 occurrences of the injected property/parameter name
$termijnService(and bare$termijn) remain across 15 files. They now hold aDeadlineService— which reads oddly but is harmless and type-correct.Checked for the named-argument hazard first: the only
termijn:-shaped named arguments in the tree arebewaartermijn:andprocestermijn:, which are different concepts, so no call site is at risk.Splitting here keeps this PR at a fully-verified boundary rather than a half-applied one.