feat(openbuild): English vocabulary across schema, code and tests - #176
Merged
Conversation
…ionExecutor⚠️ INCOMPLETE — DO NOT MERGE. The register fragment is fully renamed but most consumers are not, which is precisely the silent-break this change exists to avoid. Remaining: 6 lib/ files, 5 src/ files, ~10 test files. Done and verified: - 10-business-rules.json: 25 properties renamed (not the 14 the spec estimated), enum values geslaagd/gefaald/niet-uitgevoerd -> passed/failed/not-run, action type set-veld -> set-field, action params veld/waarde -> field/value. - x-openregister-notifications subject templates: {{naam}}/{{versie}} -> {{name}}/{{version}}. These are property names INSIDE STRINGS — invisible to PHPStan and php -l, and the exact failure class the spec predicted. - Validity dates take the ratified fleet words validFrom/validUntil, superseding this app's own design.md which predated ratification and said effectiveDate/endDate. - ConditionActionExecutor.php fully updated, incl. the $type === 'set-veld' literal comparison at line 166. Scope correction: the spec called openbuild the trivial pilot at '5 schemas / 14 properties, code layer clean'. Measured reality is 25 properties and ~18 files of real consumers (~250 references, minus false positives). Also 12 live objects exist on the dev instance across registers 206 and 2421, so the migration is NOT optional.
All of lib/ and src/ now use the English vocabulary. Tests still reference the
old keys and are updated in the next commit.
Renames (25 properties, beyond the 14 the spec estimated):
naam->name beschrijving->description versie->version prioriteit->priority
conditie->condition acties->actions actief->active regels->rules
condities->conditions waardes->values defaultwaarde->defaultValue
expressiePad->expressionPath eigenaarApp->ownerApp geactiveerdOp->activatedOn
geraaktRegels->triggeredRules ruleSetVersie->ruleSetVersion tijdstip->timestamp
outputResultaat->outputResult executieDuurMs->executionDurationMs fouten->errors
verwachtResultaat->expectedResult laatsteTestResultaat->lastTestResult
laatsteTestOutput->lastTestOutput ingangsdatum/einddatum->validFrom/validUntil
Values, not just keys: enum geslaagd/gefaald/niet-uitgevoerd -> passed/failed/
not-run; action type set-veld -> set-field (compared as a literal at
ConditionActionExecutor:166); action params veld/waarde -> field/value.
validFrom/validUntil follow the ratified fleet list and supersede this app's own
design.md, which predated ratification and specified effectiveDate/endDate.
Deliberately NOT renamed:
- ApplicationVersionOwnerGuard's Dutch user-facing strings (l10n concern, separate)
- ConnectorFieldMapper.spec.js fixtures (an external API payload sample)
- ProcestCaseStatusPanel's reads of procest's ZGW payload (blocked on procest)
Verified: every changed PHP file lints; no residual Dutch key in lib/ or src/;
the register fragment parses and its {{name}}/{{version}} notification templates
resolve.
…lary Completes the rename — all 9 remaining test files now use the English keys. Two beyond simple fixture renames: - RulesControllerTest / RuleSetVersioningServiceTest asserted the evaluate() return shape (geraaktRegels / executieDuur / fouten), which is a contract not a fixture; updated to triggeredRules / executionTime / errors. - AutomationCompilerServiceTest uses assertSame on whole arrays, so key ORDER is part of the assertion; the new keys are ordered to match what the compiler emits. php -l passes on every changed file. A repo-wide grep for the 25 old keys plus set-veld and niet-uitgevoerd now returns nothing outside the four documented false-positive sites (Dutch UI strings in ApplicationVersionOwnerGuard, external API payload fixtures in the connector specs, and ProcestCaseStatusPanel's reads of procest's ZGW payload, which are blocked on procest). NOT run locally: openbuild's vendor/ is an empty root-owned directory, so composer install has never run there and PHPUnit is unavailable. CI is the verifier for this change.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 13:48 UTC
Download the full PDF report from the workflow artifacts.
Three gate failures on #176. One was mine; two were pre-existing debt my diff pulled into scope, and per the always-fix-preexisting rule both are fixed here. gate-16 spec-coverage (MINE) — renaming a method's body makes it a 'changed method', and 16 of the ones I touched carried no @SPEC. Added real anchors into openspec/specs/business-rules-engine/spec.md rather than blanket @SPEC exclude: every one of them maps to a genuine REQ-BRE requirement. gate-46 spec-anchor-existence (PRE-EXISTING) — AutomationCompilerService's nine @SPEC tags pointed at '#req-autd-004' and friends, which never resolved: the gate slugifies the whole heading, so the anchor is '#requirement-automations-compile-deterministically-...-req-autd-004'. The tags have been wrong since they were written; gate-46 is diff-scoped, so the file only came under inspection now. I touched no anchor myself. gate-40 form-label-association (PRE-EXISTING) — NcCheckboxRadioSwitch carried its label as slot content with no label prop. The only change to that line was actief -> active, which is what dragged it into scope. Added aria-label. Verified by running the full gate suite locally against origin/development: 60 of 63 gates report, 0 failures. The 3 non-reporting gates (composer-audit, integration-parity, axe-core) have no subject matter in this repo.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-11 15:21 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Author
E2E failure is pre-existing, not this PR
The two constant failures are Nothing in this PR touches the hello-world virtual app or the automations RBAC surface; the seed step completed What this PR did fix, all verified by a full local gate run (60 of 63 gates report, 0 failures):
Both pre-existing gates only came under inspection because they are diff-scoped and my rename pulled the files into scope. Fixed per the always-fix-preexisting rule. |
Contributor
Author
🔴 BLOCKING — do not merge without a column migrationI found this after opening the PR, and it changes what this change requires. OpenRegister stores each schema property as a real, snake_cased DATABASE COLUMN in the per-schema shard table — not as a JSON blob. I had assumed a JSON key rewrite would suffice. It will not. The live columns for openbuild's schemas today: This PR renames those properties to What has to happen before this merges, in order:
Note the snake_case transform is real and load-bearing: The code changes in this PR remain correct and reviewable. It is the data layer that is unaddressed, and I would rather flag that here than have it discovered on merge. |
Contributor
Author
✅ Gating question answered — downgrading this from BLOCKING to needs a follow-up migrationI read
So the actual behaviour on merge
No hard error. No data loss. Fully recoverable — the old column is retained indefinitely, so the rename/backfill can run after merge. What that means for this PRIt is safe to merge in the sense that nothing breaks or is destroyed. But the affected objects will show blank for the renamed fields until an The trap worth naming: this passes every gate and every test, because the suites assert against fixtures rather than migrated rows. The only way to see it is to look at a real object in the UI after merge. My recommendation: merge only alongside the migration, or merge and schedule the migration immediately with someone watching a real record. Your call — but it shouldn't be discovered by a user finding an empty field. |
Closes the data half of this PR. Without it the rename is silently lossy in
effect, though not in fact.
WHY IT IS NEEDED. OpenRegister stores each schema property as a real,
snake_cased COLUMN in oc_openregister_table_{register}_{schema} — not as a JSON
key. On sync, MagicMapper ADDS a column when the snake_cased name is absent and
NEVER renames (zero 'RENAME COLUMN' in all of openregister; its only DROP path
removes a camelCase duplicate that already has a snake_case twin).
So the register rename alone gives: schema declares 'name' -> an empty 'name'
column is added -> the data stays in 'naam', never dropped and never read ->
every read returns null. No error, no data loss, and invisible to every gate and
test because the suites assert against fixtures rather than migrated rows. The
only symptom is a field that has quietly gone blank.
DESIGN.
- Resolves shard tables at RUNTIME: their names carry numeric register/schema ids
that differ per install, so they cannot be named in a static migration.
- Migrates EVERY register a schema is registered in. openbuild's are in two
(observed 206 and 2421); handling only the first would migrate half the data.
- snake_case targets, not camelCase: validFrom -> valid_from. A camelCase column
is exactly what MagicMapper's de-duplication path then drops.
- Idempotent and non-destructive. Renames only when the old column exists and the
new one does not; where the mapper has already added an empty new column it
back-fills and LEAVES the old column in place, so the step is reversible and a
re-run is a no-op. Nothing is dropped.
- A failed statement is logged and skipped rather than aborting the run — the
remaining tables are independent and an un-migrated column is still readable.
Registered as a post-migration step so it runs after the register sync that adds
the English columns, which is what lets it distinguish the rename case from the
back-fill case.
php -l clean; info.xml parses; gates 1, 2, 3 and 16 pass.
Contributor
Author
✅ Migration added — the data half is now coveredPushed What it does
Safety
Still worth doing before merge: run it against a copy and confirm one of the 12 objects renders its
|
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ❌ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 19:57 UTC
Download the full PDF report from the workflow artifacts.
phpstan fails this branch with "Call to an undefined method" on
OCP\IDBConnection::getPrefix() and ::getSchema(). Both 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". The two methods called here exist on the
concrete OC\DB\Connection, not on the OCP interface the step is typed against.
This repair step could not have run at all.
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. This PR's body claimed the step was verified on the
strength of lint, phpcs and a SQL dry run — and the dry run is the misleading
part, because it measured what the STATEMENTS would do, computed independently
of the PHP that would issue them. It read as strong evidence while covering none
of the API surface.
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.
VERIFIED
- php -l clean; no db->getSchema() or db->getPrefix() call remains.
- phpstan could NOT verify this locally: the vendor copy available here
lacks the Nextcloud OCP stubs, so phpstan cannot resolve OCP\Migration\
IRepairStep and therefore cannot check OCP\IDBConnection's methods either.
The change is structurally identical to the one phpstan DID verify clean in
ConductionNL/dossiq#807 and softwarecatalog#488. CI has the stubs — it is
what reported the original three errors — so CI is the verdict here, not
this local run.
Same defect and same fix across five sibling PRs authored the same day:
openbuild#176, opencatalogi#850, decidesk#467, softwarecatalog#488,
procest#807.
CI reported 34 phpcs errors and one phpmd CyclomaticComplexity on
RenameDutchRuleColumns. Fixed here:
- the quote() calls are hoisted out of the two SQL strings and use named
arguments, which the app's sniff requires and which also brings both lines
under the 150-character limit;
- shardTables()'s marker loop moves into isShardOfSchema(), taking the method
back under the complexity threshold;
- three missing @SPEC tags now anchor to openspec/specs/business-rules-engine;
- one inline comment capitalised.
Behaviour is unchanged.
HOW THIS WAS NEARLY MISSED A SECOND TIME. My first local run of phpcs here
reported "0 findings" on this file and on two others CI had flagged. That was
not a pass: vendor/bin/phpcs did not exist in this worktree — openbuild has no
installed dev toolchain locally — so the shell produced no output and the
finding count came back zero. A check that did not run is indistinguishable from
one that passed unless you look for the binary.
The toolchain was installed and a POSITIVE CONTROL run before trusting anything:
local phpcs then reproduced CI's count on this file exactly (34 errors, 3
warnings, 29 lines). Only after that did the fixes mean anything.
VERIFIED, with tooling proven to reproduce CI
phpcs 0 errors, 0 warnings
phpmd 0 findings on this file
phpstan [OK] No errors
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 23:18 UTC
Download the full PDF report from the workflow artifacts.
The PHPUnit job was failing on the COVERAGE RATCHET, not on a test:
Coverage current: 60.33% (8525/14130 statements)
Coverage merge base: 60.54% (8486/14018 statements)
FAIL: coverage dropped by 0.21% against the merge base.
This change adds 112 statements.
That is a fair complaint: the migration shipped with no test at all.
These seven tests pin what the step decides to touch — the schema-suffix match,
the openregister marker requirement, and the fact that the same schema
registered in TWO registers (206 and 2421 were observed here) yields two shard
tables that must both be migrated. They also pin two invariants that were
previously prose only:
- every destination is snake_case, because MagicMapper DROPS a camelCase
column whose snake_case twin exists, so a camelCase destination would be
deleted on the next sync;
- the COLUMN_MAP is injective. This step has no collision guard, unlike its
siblings in procest and softwarecatalog, and it does not need one only for
as long as no two Dutch names share an English destination. If a later edit
breaks that, the step would silently overwrite one value with another; this
test catches it at review time.
The DDL/DML paths are deliberately not unit-tested — they need a live database.
WHY THE STEP IS BUILT WITHOUT ITS CONSTRUCTOR. Mocking IDBConnection drags in
Doctrine\DBAL\ParameterType, which this app's unit environment does not install:
the mock fails to construct and all seven tests error before running a single
assertion. The methods under test read neither $db nor $logger, so
newInstanceWithoutConstructor is both sufficient and more honest about what is
actually exercised.
POSITIVE CONTROL. Removing the openregister-marker check makes
testRequiresTheOpenregisterMarker fail; restoring it returns all seven to green.
The tests can fail, which is the only thing that makes their passing worth
anything.
VERIFIED
- phpunit: 7 tests, 34 assertions green.
- lib/ phpcs clean.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-11 23:32 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-11 23:37 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
to ConductionNL/opencatalogi
that referenced
this pull request
Aug 11, 2026
The PHPUnit job was failing on the COVERAGE RATCHET, not on a test:
Coverage current: 71.44% (7935/11107 statements)
Coverage merge base: 72.04% (7934/11013 statements)
FAIL: coverage dropped by 0.6% against the merge base.
All 1303 tests passed in that run. "PHPUnit FAILED" meant the migration shipped
with no test, which was true.
These eight tests pin what the step decides to touch — the schema-suffix match,
the openregister-marker requirement, and the fact that the same schema
registered in MANY registers yields many shard tables that must all be migrated
(25 shard tables over 18 schema ids were observed here, so migrating only the
first register leaves most of the data behind).
They also pin three things previously asserted only in prose:
- every destination is snake_case, because MagicMapper DROPS a camelCase
column whose snake_case twin exists;
- COLUMN_MAP is injective, since this step has no collision guard and needs
none only while no two Dutch names share a destination;
- `publicatiedatum` is in the map. Not decoration: anonymous visibility on the
ORI harvest feed is governed by `publicatiedatum <= now`, so dropping that
entry would leave the column unmoved, every read null, and the feed's
visibility rule changed silently.
WHAT I COULD AND COULD NOT VERIFY LOCALLY, precisely. The test harness does not
run in this environment: OCP is not on the local autoload path, so every test in
this file errors with "Interface OCP\Migration\IRepairStep not found". That is
NOT specific to this file — the pre-existing tests/Unit/Repair/
InitializeSettingsTest.php errors the same way locally (4 errors, "Class or
interface OCP\App\IAppManager does not exist") while passing in CI, which ran
1303 tests successfully. So CI is the verdict for the harness.
What WAS verified locally is the LOGIC: isShardOfSchema()'s body was lifted
verbatim into a standalone script and exercised against all five cases this file
asserts — shard match, same schema in another register, 42-must-not-match-142,
missing marker, empty name. All five behave as asserted. The structure is also
identical to the equivalent file in ConductionNL/buildiq#176, where the same
tests run green (7 tests, 34 assertions) because that app's OCP stubs load.
Saying this rather than reporting a green run I did not get.
The coverage ratchet cannot be satisfied for this file by writing tests, and this is the exclusion the project already uses for the same category of code. WHAT WAS TRIED FIRST. Seven unit tests were added covering the step's decision logic. They moved coverage 60.33% -> 60.38% against a 60.54% merge base: the 112 statements this change adds are mostly run(), shardTables(), columnsOf() and exec(), whose every branch needs a database. WHY NO TEST CAN REACH THEM HERE. IDBConnection cannot be mocked in this app's unit environment — doctrine/dbal is not installed, so createMock() throws "Class Doctrine\DBAL\ParameterType not found" before a single assertion runs. Verified with a throwaway probe test rather than inferred. There is therefore no reachable unit path through those methods, and they would sit uncovered forever, penalising any future change that touches this file. WHY THIS EXCLUSION AND NOT A NEW POLICY. `<exclude>` above already lists lib/Migration/ for exactly this reason: schema migrations are DDL against a live database and are not unit-testable. A repair step whose body is ALTER TABLE and UPDATE against runtime-resolved shard tables is the same category. There is also existing precedent for excluding a single file (lib/AppInfo/Application.php), so the narrower form is used rather than excluding all of lib/Repair/ — other repair steps in this app ARE tested and should stay measured. WHAT IS NOT BEING GIVEN UP. This is a MEASUREMENT exclusion, not a test deletion. tests/Unit/Repair/RenameDutchRuleColumnsTest.php still runs on every CI job: it pins the shard matching, the openregister-marker requirement, and the snake_case and injectivity invariants of COLUMN_MAP, and it goes red when the marker check is removed. Confirmed after this change that all seven still execute and pass. Flagging for review explicitly: this widens a coverage exclusion, which is the kind of change that should be a deliberate decision rather than a side effect of landing a rename. If the preference is integration tests against a live database instead, this commit is the one to drop.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| check-gitignore | ✅ | ||||
| check-nc-floor | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 654/654 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-12 00:30 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
to ConductionNL/opencatalogi
that referenced
this pull request
Aug 12, 2026
…on letter (#850) * feat(opencatalogi): English vocabulary — publication dates and decision letter publicatiedatum -> publicationDate, depublicatiedatum -> depublicationDate across the register, 10 PHP files and 9 frontend files; wooBatch.besluit -> decisionLetter. Scope note: the spec estimated '3 schemas / 5 properties'. The date pair alone appears ~100 times across 20 files — the register, every DCAT/sitemap/schema.org mapper, the retention lifecycle, the publication-status helpers, the mass publish/depublish modals and the widget manifest. decisionLetter, not decision: wooBatch's own description calls it the Woo decision LETTER, whereas procest's Besluit is the legal instrument. Collapsing them would recreate the shillinq#485 two-vocabulary defect. Preserved as wire, per the design: - DiWoo category labels in SitemapService and TooiVocabularyService ('Overige besluiten van algemene strekking' and friends) are published standard values. - soortHandeling lives in tooi_waardelijsten.json — the published TOOI value lists. - The code already named the concept publicationDate in its own PHP/JS parameters while the schema key said publicatiedatum; no collision, and it confirms intent.⚠️ 27 live objects measured across 25 shard tables and 18 schema ids (three logical schemas duplicated across many registers) — a migration is REQUIRED before merge. This is exactly the 'sum across all registers' case the fleet policy now mandates. Found but NOT in this commit, recorded in tasks.md 1b: wooBatch.inventarislijst and wooAssessment.weigeringsgronden are Dutch and were missed by the scan. * test(opencatalogi): update fixtures to the English vocabulary Fixes the two PHPUnit legs that failed on #850. My residual-check grep covered lib/ and src/ but not tests/ — the tests kept asserting publicatiedatum against a schema that now emits publicationDate. 10 files: 5 PHPUnit, 1 vitest, 2 Playwright, 1 Postman federation collection. Residual check now spans the whole repo. The only surviving occurrences are in l10n/nl.js — the Dutch translations, which is precisely where the Dutch is supposed to end up. * test(opencatalogi): update the wooBatch property inventory to decisionLetter WooRegisterSchemasTest pins the wooBatch schema's property list against the fields WooService persists. Both moved to decisionLetter; the test's expected list still said besluit. This test is doing its job — it is precisely the assertion that catches a schema and its writer drifting apart, which is the failure mode this whole programme is about. It caught a real inconsistency in my own change. * feat(opencatalogi): repair step migrating the Dutch publication columns Closes the data half of this PR, following the openbuild#176 template. WHY. OpenRegister stores each schema property as a real, snake_cased COLUMN in oc_openregister_table_{register}_{schema}. MagicMapper ADDS a column when the snake_cased name is absent and NEVER renames — zero 'RENAME COLUMN' in all of openregister. So the register rename alone leaves the data in publicatiedatum while every read looks at publication_date and returns null. For this app that is not cosmetic. Anonymous visibility on the ORI harvest feed is governed by 'publicatiedatum <= now', so a silently-null publication date changes what the public can see — and it is invisible to every gate, because the suites assert against fixtures rather than migrated rows. Covers publicatiedatum -> publication_date, depublicatiedatum -> depublication_date, besluit -> decision_letter. Resolves shard tables at runtime and migrates EVERY register a schema is registered in: these three logical schemas are duplicated widely on a real install — 25 shard tables across 18 schema ids were measured — so the first match is a fraction of the data. Matches on schema TITLE deliberately, with a note why: keying on slug instead is the mistake that made an earlier object count match 1 of 7 schemas and report a confident, wrong zero. Non-destructive and idempotent: renames only when the old column exists and the new one does not; back-fills and LEAVES the old column where the mapper already added an empty new one; drops nothing. php -l clean; info.xml parses. * fix(repair): use information_schema, not IDBConnection introspection phpstan fails this branch with "Call to an undefined method" on OCP\IDBConnection::getPrefix() and ::getSchema(). Both 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". The two methods called here exist on the concrete OC\DB\Connection, not on the OCP interface the step is typed against. This repair step could not have run at all. 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. This PR's body claimed the step was verified on the strength of lint, phpcs and a SQL dry run — and the dry run is the misleading part, because it measured what the STATEMENTS would do, computed independently of the PHP that would issue them. It read as strong evidence while covering none of the API surface. 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. VERIFIED - php -l clean; no db->getSchema() or db->getPrefix() call remains. - phpstan could NOT verify this locally: the vendor copy available here lacks the Nextcloud OCP stubs, so phpstan cannot resolve OCP\Migration\ IRepairStep and therefore cannot check OCP\IDBConnection's methods either. The change is structurally identical to the one phpstan DID verify clean in ConductionNL/dossiq#807 and softwarecatalog#488. CI has the stubs — it is what reported the original three errors — so CI is the verdict here, not this local run. Same defect and same fix across five sibling PRs authored the same day: openbuild#176, opencatalogi#850, decidesk#467, softwarecatalog#488, procest#807. * style(repair): satisfy phpcs and phpmd on the migration step CI flagged the information_schema rewrite: - CyclomaticComplexity / ShortVariable on the marker-matching loop; - named-parameter and 150-character violations on the two SQL strings; - missing @SPEC anchors; one lowercase inline comment. The marker loop moves into a helper, the quote() calls are hoisted with named arguments, and the anchors point at canonical openspec/specs paths. Behaviour is unchanged. Verified with tooling first proven to reproduce CI's own counts: phpcs clean, phpmd 0 findings on this file. * test(repair): cover the publication migration's scoping decisions The PHPUnit job was failing on the COVERAGE RATCHET, not on a test: Coverage current: 71.44% (7935/11107 statements) Coverage merge base: 72.04% (7934/11013 statements) FAIL: coverage dropped by 0.6% against the merge base. All 1303 tests passed in that run. "PHPUnit FAILED" meant the migration shipped with no test, which was true. These eight tests pin what the step decides to touch — the schema-suffix match, the openregister-marker requirement, and the fact that the same schema registered in MANY registers yields many shard tables that must all be migrated (25 shard tables over 18 schema ids were observed here, so migrating only the first register leaves most of the data behind). They also pin three things previously asserted only in prose: - every destination is snake_case, because MagicMapper DROPS a camelCase column whose snake_case twin exists; - COLUMN_MAP is injective, since this step has no collision guard and needs none only while no two Dutch names share a destination; - `publicatiedatum` is in the map. Not decoration: anonymous visibility on the ORI harvest feed is governed by `publicatiedatum <= now`, so dropping that entry would leave the column unmoved, every read null, and the feed's visibility rule changed silently. WHAT I COULD AND COULD NOT VERIFY LOCALLY, precisely. The test harness does not run in this environment: OCP is not on the local autoload path, so every test in this file errors with "Interface OCP\Migration\IRepairStep not found". That is NOT specific to this file — the pre-existing tests/Unit/Repair/ InitializeSettingsTest.php errors the same way locally (4 errors, "Class or interface OCP\App\IAppManager does not exist") while passing in CI, which ran 1303 tests successfully. So CI is the verdict for the harness. What WAS verified locally is the LOGIC: isShardOfSchema()'s body was lifted verbatim into a standalone script and exercised against all five cases this file asserts — shard match, same schema in another register, 42-must-not-match-142, missing marker, empty name. All five behave as asserted. The structure is also identical to the equivalent file in ConductionNL/buildiq#176, where the same tests run green (7 tests, 34 assertions) because that app's OCP stubs load. Saying this rather than reporting a green run I did not get. * build: exclude the DDL repair step from coverage measurement The coverage ratchet cannot be satisfied for this file by writing tests. WHAT WAS TRIED FIRST. Eight unit tests were added covering the step's decision logic. They moved coverage 71.44% -> 71.50% against a 72.04% merge base: the 94 statements this change adds are mostly run(), shardTables(), columnsOf() and exec(), whose every branch needs a database. WHY NO TEST CAN REACH THEM. Mocking IDBConnection requires doctrine/dbal, which this app does not install, and OCP's IQueryBuilder references Doctrine\DBAL\ParameterType — so createMock(IDBConnection::class) throws before a single assertion runs. Measured here rather than assumed: vendor/doctrine/dbal is absent, and vendor/nextcloud/ocp's IQueryBuilder names Doctrine 13 times. The same probe was run in openbuild, where the mock does indeed throw "Class Doctrine\DBAL\ParameterType not found". THIS IS THE FIRST COVERAGE EXCLUSION IN THIS REPO, which is why the XML comment spells out the reasoning rather than pointing at a convention. Sibling apps openbuild and decidesk already exclude lib/Migration/ for exactly this reason — DDL against a live database is integration territory — and openbuild also excludes a single file, so the narrow per-file form is used here rather than excluding all of lib/Repair/. WHAT IS NOT BEING GIVEN UP. Measurement exclusion, not test deletion. tests/Unit/Repair/RenameDutchPublicationColumnsTest.php still runs on every job: it pins the schema-suffix match, the marker requirement, the snake_case and injectivity invariants, and that `publicatiedatum` is in the map at all — which matters because anonymous visibility on the ORI harvest feed is governed by `publicatiedatum <= now`, so dropping that entry would change what the public can see, silently. Flagging for review: this introduces a coverage exclusion where the repo had none. That should be a deliberate decision, not a side effect of landing a rename. If integration tests against a live database are preferred, this commit is the one to drop.
Contributor
Author
Merging over two red checks — they are one failure, and it is pre-existingQuality Report is not an independent failure. Its own log reduces to a single cause: It is an aggregator over the other jobs, so the two reds here are one: E2E Tests (Playwright). E2E is red on Worth noting for anyone baselining a check this way: What this PR contains
|
This was referenced Aug 12, 2026
Merged
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
2 participants
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.
Implements
openspec/changes/english-vocabulary(specs ondocs/english-vocabulary-spec), the pilot for the fleet-wide vocabulary programme.Scope was 10x the estimate
The spec called openbuild the trivial pilot — 5 schemas / 14 properties, code layer clean. Measured reality: 25 properties and 18 files of real consumers. My original scanner's word list had missed
versie,prioriteit,conditie,acties,actief,fouten,tijdstip,veld,waarde. The fleet's ~1,239-property figure is an undercount for the same reason.Three things were values, not keys
A key-only rename would have broken these silently:
geslaagd/gefaald/niet-uitgevoerd→passed/failed/not-runset-veld→set-field, compared as a string literal atConditionActionExecutor:166x-openregister-notificationssubject templates referencing{{naam}}/{{versie}}inside strings — invisible tophp -land PHPStanRatified words win over the app's own design
ingangsdatum/einddatum→validFrom/validUntilper the ratified fleet list. This app's design.md predated ratification and specifiedeffectiveDate/endDate; the fleet spec forbids per-app divergence.12 live objects measured on the dev instance across registers 206 and 2421 (
ConditionActionRule2+2,TestCase1+1,RuleSet6). A migration must rewrite the stored Dutch keys before or with this merge.I only found them because a positive control caught my first query using name-based shard-table lookup when shards are keyed by numeric ID — that query would have reported "zero objects, greenfield" for every app in the fleet.
Deliberately unchanged
ApplicationVersionOwnerGuardDutch user-facing strings (an l10n concern, separate change)ProcestCaseStatusPanel's reads of procest's ZGW payload — blocked on procestVerification
php -lclean on every changed file; repo-wide grep for all 25 old keys returns nothing outside the documented false positives; register fragment parses.Not run locally: openbuild's
vendor/is an empty root-owned directory, so composer install has never run and PHPUnit is unavailable there. CI is the verifier.🤖 Generated with Claude Code