fix(aggregations): translate the legacy DSL — batch 1 of the #1261 sweep - #1288
Merged
Merged
Conversation
…ransaction GLLine declared no fiscal-year property at all. Three segment-P&L roll-ups grouped by `GLLine.fiscalYearId` regardless, so every row landed in ONE null bucket — a plausible total rather than an error, which is why it survived. The gate had it waived with the note that it needed a schema decision. This is that decision. `periodId` was deliberately NOT reused. A period is a FINER grain than a year, so grouping by it would have silently changed what those roll-ups mean — twelve buckets where the consumer expects one — instead of fixing them. That is the same shape of defect, not a fix for it. ## What lands - `GLLine.fiscalYearId` (string, nullable), denormalised from the parent `GLTransaction.fiscalYearId`. The name and type match the six schemas that already declare it (GLTransaction, BalanceSheet, TrialBalance, ConsolidatedReport, ClosingEntry, kernGegevensConfig). - `GlLineFiscalYearBackfillMigrator` — the pure resolution core, modelled on GlLineAdministrationBackfillMigrator. Indexes each transaction under every identity a line may reference it by (id, @self.id, uuid, transactionNumber), because `transactionId` has been written as each of those at different points. - `BackfillGlLineFiscalYear` — the repair step, registered post-migration only (a fresh install has no historical lines to stamp). - The `GLLine.fiscalYearId` waiver is removed from AGGREGATION_REF_BASELINE. The dotted-reference gate now passes with NO baselined entries at all. - `AGG_BARE_REF_BASELINE` 120 -> 116. Declaring the property also resolved four BARE `fiscalYearId` references in other aggregations, which the ratchet caught and refused to let pass unrecorded. ## Reporting rather than gating, deliberately The administration backfill closes a config gate and refuses to reopen it unless a re-read proves completeness. That is right for `administrationId`: it is a tenant SCOPE, and a half-scoped ledger makes a filter return a silent zero. `fiscalYearId` is a GROUPING key. An unresolved line is not a leak and zeroes nothing — it appears as a null bucket, which is visible in the result. Aborting every resolvable line because one ancient row lost its transaction would trade a visible gap for no backfill at all. So the step stamps what resolves and REPORTS what did not, by re-reading the store afterwards and counting the whole set. The count is emitted even when it is zero, so "nothing left behind" is something the operator read rather than assumed. A line that already carries a year is never rewritten, even when its parent now disagrees — that disagreement is reported instead, because re-pointing a posted line is a bigger decision than a backfill gets to make. ## Verification - 10 new unit tests, 37 assertions - Mutation control: removing the no-overwrite guard makes the suite fail, and restoring it makes it pass again — the tests can detect the thing they claim - Full suite 4990 tests, 0 failures - validate-registers exits 0 with every gate at its baseline - Parsed-tree diff confirms the register edit added exactly the five `fiscalYearId` leaves and touched nothing else Refs #1261
…uild the JS check:schema-l10n went one over its baseline: the new GLLine.fiscalYearId description had no catalogue key, so it would have rendered in English inside an otherwise translated form. Both artefacts are updated, not just the source. l10n/*.json is what the check reads; l10n/*.js is what the browser actually loads, and a translation present only in the JSON reaches nobody.
Forty-one aggregations move from keys AggregationRunner never reads to keys it
does. None of them errored before; they returned an empty result or nothing at
all, under HTTP 200.
## 19 `@self` correlations become groupBy dimensions
`filter: { poolId: "@self.id" }` on an intra-schema aggregation does not
resolve. PlaceholderResolver acts only on `$`-prefixed values, so `@self.id`
stayed a LITERAL STRING and matched no row — an empty result that looks like
"no data" rather than "this never worked".
And it could not have worked: no caller supplies a parent row.
AggregationController, ReportRenderService and ThresholdEvaluationService are
the only three call sites of `run()` in OpenRegister, and none passes one.
Grouping by the same field needs no parent row and answers for every record at
once; a caller narrows to one through the `extraFilter` query params the REST
endpoint already passes through.
## 22 `operations` maps become `metrics`
`operations` is not an engine key either. Each entry is
`{field, operation, target}` — which is exactly `{field, metric, as}` — so the
translation is mechanical. `target` duplicated the map key; both became `as`.
Where `field` was written qualified (`RetainerDrawdown.drawdownAmount`) the
prefix is stripped, since `from` already resolves bare names on that schema.
Conditional entries carry through as `condition`, which the runner honours
since openregister #2917.
## What was deliberately NOT translated
- 33 `operations` maps containing an `expression` op. The engine has no
equivalent, and the conditions are SQL-ish STRINGS ("VATLine.type =
'collected'") where computeMetrics() takes a filter OBJECT. Guessing either
would produce a confident wrong number, which is the failure mode this whole
sweep exists to remove. `BtwAangifte.totalsByReturn` is pinned in its test as
still-untranslated so the gap stays visible.
- `ExpenseClaimEntry.settlementTotals`. It uses a multi-source `sources` array
whose "amountField"/"customerField" are CONFIG KEYS of a source descriptor,
not fields. An earlier pass of this change translated them into metrics; that
was nonsense and is reverted.
## Two defects the gates caught mid-change
Translating `operations` into `metrics` moved those field names into a key the
bare-reference gate can SEE, and the count rose from 102 to 111. That is not new
debt — it is debt that was previously invisible:
- `ProvisionDisclosureTabel.provisionDisclosureGeneration` kept `source` instead
of `from`, so its five movement fields resolved against the wrong schema.
- `IBProfitAttribution.innovatieboxAdministratie` summed
`kwalificerende_winst_na_nexus`; the schema declares
`qualifying_profit_after_nexus`. A Dutch/English mismatch that matched nothing.
- `GhgInventory.scope3ByCategory` tripped the string-groupBy gate the moment it
gained metrics — a STRING groupBy is silently dropped and yields one ungrouped
total.
All three are fixed here, so the bare-ref count returns to 102 with no baseline
raised.
## Ratchets
AGG_PLACEHOLDER_BASELINE 81 -> 62 (exactly the 19 converted)
AGG_NO_METRIC_BASELINE 206 -> 185
AGG_BARE_REF_BASELINE 102 -> 102 (unchanged, after the three fixes)
## Verification
- Parsed-tree diff against HEAD: exactly the intended aggregations changed,
ZERO non-aggregation leaf changes across all 14 files
- Only files whose JSON round-trips byte-identically were rewritten, so no file
was silently reformatted
- Five fragment tests updated from the old vocabulary to the engine's, each
asserting the inert key is GONE rather than only that the new one is present
- Full suite 4990 tests, 0 failures
- validate-registers exits 0 with every gate at its baseline
Stacked on #1287 (GLLine.fiscalYearId), whose baseline changes this builds on.
Refs #1261
gate-16 (spec-coverage) failed with 8 changed methods missing @SPEC. Found by running the real gate runner locally against origin/development as the delta base — the CI run was still queued behind a saturated runner pool, and without an explicit base the gate reports NOT APPLICABLE rather than passing, so it would have judged nothing. Tags point at openspec/specs/bookkeeping-cost-centers-dimensions/spec.md #req-cc-005 — the segment-P&L requirement that motivated the property — rather than at an openspec/changes/ directory. Archiving a change breaks every @SPEC tag pointing into it, and most existing tags in this repo have that shape. Also removes a tag that landed on the CLASS_UNRESOLVABLE constant docblock instead of a method, and adds the two the first pass placed on neighbouring docblocks (run, assertCountsMatch) — verified by re-running the gate, not by counting matches. gate-16 PASS, gate-98 PASS.
…tries The previous commit added ONE string and produced a 4,473-line diff on l10n/nl.json. The writer called dict(sorted(...)) on every catalogue, and these catalogues are not stored in sorted order — so the whole file was reordered and the single real addition was buried in it. That is not cosmetic. A reviewer cannot see a one-line change inside a four-thousand-line reordering, and the next person to touch l10n gets a conflict against every entry rather than against the line that moved. The catalogues are now written back in their ORIGINAL key order, re-sorting only a file that was already sorted. Net effect of this branch on l10n drops from ~4,500 changed lines to 8. Verified against origin/development, not against the branch tip: the previous commit looked small there too until the base was made explicit.
rubenvdlinde
changed the base branch from
feat/glline-fiscal-year
to
development
August 27, 2026 16:59
…n-legacy-dsl-batch-1 # Conflicts: # l10n/en.js # l10n/en.json # l10n/nl.js # l10n/nl.json # tests/validate-registers.js
Contributor
Quality Report — ConductionNL/shillinq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-manifest-shell | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-manifest-budget | ✅ | ||||
| check-markers | ✅ | ||||
| check-registers | ✅ | ||||
| check-seeds | ✅ | ||||
| check-fragment-required | ✅ | ||||
| check-nav-reachability | ✅ | ||||
| check-job-registration | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 106/106 | |||
| npm | ✅ | ✅ 551/551 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-27 20:39 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Forty-one aggregations move from keys
AggregationRunnernever reads to keys it does. None of them errored before — they returned an empty result, or nothing at all, under HTTP 200.19
@selfcorrelations → groupBy dimensionsfilter: { poolId: "@self.id" }on an intra-schema aggregation does not resolve.PlaceholderResolveracts only on$-prefixed values, so@self.idstayed a literal string and matched no row — an empty result that reads as "no data" rather than "this never worked".And it could not have worked: no caller supplies a parent row.
AggregationController,ReportRenderServiceandThresholdEvaluationServiceare the only three call sites ofrun()in OpenRegister, and none passes one.Grouping by the same field needs no parent row and answers for every record at once; a caller narrows to one through the
extraFilterquery params the REST endpoint already passes through.22
operationsmaps →metricsoperationsis not an engine key either. Each entry is{field, operation, target}— exactly{field, metric, as}— so the translation is mechanical.targetduplicated the map key; both becameas. Qualified fields (RetainerDrawdown.drawdownAmount) are stripped to bare names, sincefromalready resolves them. Conditional entries carry through ascondition, which the runner honours since openregister #2917.Deliberately NOT translated
operationsmaps containing anexpressionop. The engine has no equivalent, and their conditions are SQL-ish strings ("VATLine.type = 'collected'") wherecomputeMetrics()takes a filter object. Guessing either would produce a confident wrong number — the exact failure mode this sweep exists to remove.BtwAangifte.totalsByReturnis pinned in its test as still-untranslated so the gap stays visible.ExpenseClaimEntry.settlementTotals. It uses a multi-sourcesourcesarray whoseamountField/customerFieldare config keys of a source descriptor, not fields. An earlier pass of this change translated them into metrics; that was nonsense and is reverted.Two defects the gates caught mid-change
Moving those field names into a key the bare-reference gate can see pushed the count 102 → 111. That is not new debt — it is debt that was previously invisible:
ProvisionDisclosureTabel.provisionDisclosureGenerationsourceinstead offrom, so its five movement fields resolved against the wrong schemaIBProfitAttribution.innovatieboxAdministratiekwalificerende_winst_na_nexus; the schema declaresqualifying_profit_after_nexus— a Dutch/English mismatch that matched nothingGhgInventory.scope3ByCategorygroupByis silently dropped and yields one ungrouped totalAll three fixed here, so the count returns to 102 with no baseline raised.
Ratchets
Verification
validate-registersexits 0 with every gate at its baselineRemaining after this batch
28 of the original 55
@selfaggregations still need remodelling (missing fields,recursiveOver,@descendant, string expressions like"@self.periodYear-@self.periodQuarter"). Those are judgement calls per aggregation, not a mechanical pass, and each needs checking against real rows.Refs #1261