diff --git a/.claude/skills/clean-env/SKILL.md b/.claude/skills/clean-env/SKILL.md index 498d3fdeb..ffa33b49c 100644 --- a/.claude/skills/clean-env/SKILL.md +++ b/.claude/skills/clean-env/SKILL.md @@ -5,14 +5,18 @@ description: Reset the OpenRegister development environment (stop, remove volume # Clean Environment -Run the `clean-env.sh` script to fully reset the OpenRegister development environment. +Reset the Conduction dev instance and bring it back up healthy. -This will: -1. Stop all containers from the OpenRegister docker-compose -2. Remove all containers and volumes (full data reset) -3. Start containers fresh -4. Wait for Nextcloud to become ready -5. Install core apps: openregister, opencatalogi, softwarecatalog, nldesign, mydash +> **This skill used to name a script that does not exist.** It said to run +> `bash .claude/scripts/clean-env.sh`; there is no such file in this repo or in +> any app checkout, and there never was one to find. Its documented app list was +> five entries long (`openregister opencatalogi softwarecatalog nldesign +> launchpad`) at a time when the fleet was twenty-one and three of those five +> names were app *directories* rather than app ids. Anyone following it got a +> "command not found" if they were lucky and a half-configured instance if they +> improvised past it. The real, maintained entry point is +> `.github/dev-up.sh` — it is the one place that knows about the mounts, the +> vendored PHP dependencies and the frontend bundles. **Model check — only apply when this skill is run standalone. Skip this section entirely if this skill was called from within another skill — the calling skill is responsible for model selection.** @@ -27,22 +31,70 @@ This will: ## Instructions -Run the clean-env script: +Work from the workspace root (the directory holding `.github/` and the app +checkouts as siblings). + +### Restart and heal — the common case + +Most "my environment is broken" reports need only this. It re-establishes +partial mounts, clears maintenance mode, runs pending migrations, installs any +missing `vendor/`, enables every mounted Conduction app and reports any app +whose frontend bundle would render blank: + +```bash +bash .github/dev-up.sh +``` + +### Full reset — destructive + +**This deletes the database and every volume.** Only run it when a full reset is +actually intended; `dev-up.sh` alone fixes most breakage. ```bash -bash .claude/scripts/clean-env.sh +docker compose -p openregister -f .github/docker-compose.yml down -v +bash .github/dev-up.sh ``` -**Important:** This is a destructive operation — it removes all database data and volumes. Only run when a full reset is intended. +## Verifying -After the script completes, verify the environment: -1. Check that Nextcloud is accessible at http://nextcloud.local -2. Log in with admin/admin -3. Confirm apps are listed and enabled +`dev-up.sh` ends in a status block — read it rather than assuming success: + +- `needsDbUpgrade: false` — an instance stuck at `true` serves a 503 the moment + anything trips maintenance mode. +- `apps visible: N/N` — a shortfall is named per app, and it says whether the + cause is an empty checkout (clone it) or a mount that did not attach. +- `apps enabled:` plus any `⚠` lines. **A `⚠ … need a frontend rebuild` line + means those apps are enabled and still render a blank page** — `occ` reports + them as perfectly healthy. Run the `npm ci && npm run build` command the + script prints for each one. + +Then open http://localhost:8080 (admin/admin) and confirm the apps appear in the +app menu. + +## If an app still fails to enable + +`dev-up.sh` prints the actual `occ` error per app; act on that rather than +retrying. The two failures that recur: + +- **`Class "…" not found`** — the app's `vendor/` is missing or half-installed. + `(cd && composer install --no-dev --ignore-platform-reqs)`. If + composer dies on `Could not delete …/vendor/…`, the tree contains root-owned + files from a container-side composer run; `dev-up.sh` heals that on its next + run. +- **`SKIP — empty checkout`** — the directory is mounted but empty. Clone + the repo into it, or drop its mount from `.github/docker-compose.yml`. + +To enable one by hand (note `-u www-data` — without it `occ` runs as root and +refuses): -If any app fails to enable, try running manually: ```bash -docker exec nextcloud php occ app-enable +docker exec -u www-data nextcloud php occ app:enable ``` +Use the app **id** (`integriq`, `filinq`, `dossiq`, `stackiq`, `keepiq`, +`larpinq`, `learniq`, `decidiq`, `buildiq`, `humaniq`), not the checkout +directory name (`openconnector`, `docudesk`, `procest`, …). The id is the `` +in `appinfo/info.xml`; a wrong name is not an error you will notice, because +`occ app:enable` on an unknown app just does nothing useful. + > 💡 If you switched models to run this command, don't forget to switch back to your preferred model with `/model ` (e.g. `/model default` or `/model sonnet`). diff --git a/.claude/skills/clean-env/examples/expected-output.md b/.claude/skills/clean-env/examples/expected-output.md index a7bd55386..28b59aae7 100644 --- a/.claude/skills/clean-env/examples/expected-output.md +++ b/.claude/skills/clean-env/examples/expected-output.md @@ -1,64 +1,85 @@ - + # Expected Output: clean-env +Captured from a real run on 2026-08-27, not written from memory. The previous +version of this file showed a script that does not exist writing "✓ All apps +installed" against five app names, at a URL (`http://nextcloud.local`) the +instance does not serve — an example nobody could ever have produced. + ## Successful run ``` -$ bash .claude/scripts/clean-env.sh - -[clean-env] Stopping containers... -Stopping nextcloud ... done -Stopping nextcloud_db ... done -✓ Containers stopped +$ bash .github/dev-up.sh -[clean-env] Removing containers and volumes... -Removing nextcloud ... done -Removing nextcloud_db ... done -Removing volume nextcloud_nextcloud_data ... done -Removing volume nextcloud_db_data ... done -✓ Containers and volumes removed +==> Starting stack (project=openregister) +==> Waiting for the DB +==> Waiting for all 36 app mounts inside the container + only 34/36 app dirs visible + collectives: EMPTY CHECKOUT at openregister/custom_apps/collectives -- clone it or drop its mount; restarting cannot help + zaakafhandelapp: EMPTY CHECKOUT at zaakafhandelapp -- clone it or drop its mount; restarting cannot help + every missing dir is an empty checkout, not a mount failure -- not restarting +==> Ensuring un-busted assets are not cached for 6 months + ok +==> Ensuring custom_apps is writable by www-data + ok +==> Clearing maintenance mode +==> Reconciling pending app upgrades (only if needed) + no upgrade needed +==> Healing PHP dependencies (vendor/) + stackiq: no vendor/autoload.php -- installing + ok +==> Ensuring Conduction apps are enabled + enabling keepiq (disabled) + ok + enabling integriq (disabled) + ok + enabling stackiq (disabled) + ok + SKIP zaakafhandelapp -- empty checkout at ../zaakafhandelapp (nothing to enable) +==> Re-reconciling upgrades (enabling an app can register a migration) + no upgrade needed +==> Checking frontend bundles match the app id + ok +==> Done. Status: + - installed: true + - version: 34.0.0.12 + - maintenance: false + - needsDbUpgrade: false + apps visible: 34/36 + apps enabled: 71 + UI: http://localhost:8080 (admin/admin) +``` -[clean-env] Starting fresh containers... -Creating network "openregister_default" with the default driver -Creating volume "nextcloud_db_data" ... done -Creating volume "nextcloud_nextcloud_data" ... done -Creating nextcloud_db ... done -Creating nextcloud ... done -✓ Containers started +## What a problem looks like -[clean-env] Waiting for Nextcloud to become ready... -.......... -✓ Nextcloud is ready at http://nextcloud.local +The script does not hide these; read to the end of the output. -[clean-env] Installing apps... -openregister installed and enabled. -opencatalogi installed and enabled. -softwarecatalog installed and enabled. -nldesign installed and enabled. -mydash installed and enabled. -✓ All apps installed +An app that is enabled and would still render a blank page — `occ` calls this +app perfectly healthy, so this section is the only thing that reports it: -[clean-env] Done! Environment is clean and ready. +``` +==> Checking frontend bundles match the app id + integriq: js/integriq-main.js missing (found openconnector-main.js -- stale, pre-rename) + fix: (cd ../openconnector && npm ci && npm run build) +... + ⚠ 5 app(s) need a frontend rebuild before their page renders ``` -## Post-script verification +An app that refused to enable, with the reason rather than a shrug: ``` -✅ Nextcloud accessible at http://nextcloud.local -✅ Logged in with admin/admin -✅ Apps enabled and active: - - openregister ✓ - - opencatalogi ✓ - - softwarecatalog ✓ - - nldesign ✓ - - mydash ✓ + enabling keepiq (disabled) + FAILED: An unhandled exception has been thrown: Error: Class "Ramsey\Uuid\Uuid" not found in …/keepiq/lib/Repair/SeedSecretTypes.php:115 + 1 app(s) could not be enabled -- see the errors above ``` -## If an app fails to enable +The stack failing to start at all — note that a **public** image answers +`denied: denied` when the local ghcr.io credential has expired, so the script +logs out and retries once before believing it: -```bash -# Re-enable manually: -docker exec nextcloud php occ app:enable openregister -docker exec nextcloud php occ app:enable opencatalogi +``` +==> Starting stack (project=openregister) + Error response from daemon: Head "https://ghcr.io/v2/conductionnl/n8n-nextcloud/manifests/latest": denied: denied + ghcr.io denied a pull -- logging out (credentials may be stale) and retrying once ``` diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index ccddb26ab..4fe99f0d9 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -2,35 +2,46 @@ name: Code Quality on: push: - # An ALLOW-LIST of branch prefixes is a gate with a hole in it, and the - # hole is SILENT: a branch matching nothing gets no CI at all, and its last - # visible status is whatever it inherited — indistinguishable, on every - # dashboard, from a branch that passed. + # DEFAULT BRANCHES ONLY. `pull_request` below carries every other branch. # - # Two live examples, both found 2026-08-14: `perf/**` was uncovered in - # openconnector, where a merge carrying unresolved conflict markers and 84 - # failing tests was pushed and nothing ran; and `feat/**` was uncovered in - # openregister — note the list said `feature/**`, so every branch anyone - # named `feat/...` had been running unchecked. + # This was an allow-list of branch prefixes, and that was a gate with a + # SILENT hole: a branch matching nothing got no CI at all, and its last + # visible status was whatever it inherited — indistinguishable, on every + # dashboard, from a branch that passed. Two live examples, both found + # 2026-08-14: `perf/**` was uncovered in openconnector, where a merge + # carrying unresolved conflict markers and 84 failing tests was pushed and + # nothing ran; and `feat/**` was uncovered in openregister, because the + # list said `feature/**`. # - # Prefixes are added rather than replaced with `**` because this workflow is - # expensive (PHPUnit matrix, Newman, Playwright). The fast structural checks - # DO run on `**` — see merge-hygiene.yml, added in the same change. + # The comment that stood here said adding prefixes was not the durable fix, + # and that the durable fix was to let the pull_request trigger gate it. + # THIS IS THAT CHANGE. # - # ⚠️ Adding prefixes is not the durable fix; the next invented one is - # uncovered again. The durable fix is branch protection requiring a PR into - # development, which the pull_request trigger below already gates correctly. + # What forced it now: a push to a branch with an open PR ran the SAME 34 + # jobs TWICE on the same commit. `concurrency` cannot dedupe them — the + # group is suffixed by event name deliberately (.github#540: a + # default-branch push carries jobs a PR run does not, and a dispatch must + # not be cancellable by a standing release PR), so the two events sit in + # different lanes BY DESIGN and both run to completion. Measured fleet-wide + # 2026-08-25..27, 659 of 2,106 Code Quality runs were that duplicate — 31% + # of the fleet's most expensive workflow, re-deciding a commit another run + # was already deciding. The account ceiling is 60 concurrent jobs (Team + # plan); the fleet was measured at 53 running with 1,528 jobs queued behind + # them, the oldest run 7 hours old and not yet started. + # + # NO BRANCH LOSES ITS FLOOR. merge-hygiene.yml runs on `'**'` — every + # branch anyone pushes, no prefix list to forget — and it is the check + # `development` actually requires. That is the smoke alarm; this workflow + # is the fire brigade and belongs on the PR. Of 668 feature-branch push + # runs in that window, only NINE were on a branch with no PR run beside + # them. + # + # The default branches STAY: their push runs are not duplicates, they are + # the only carrier of Coverage Baseline Check, SBOM and Features Extract, + # none of which run on a pull_request event. branches: - main - development - - feature/** - - feat/** - - bugfix/** - - hotfix/** - - perf/** - - refactor/** - - chore/** - - fix/** pull_request: branches: [main, master, development, beta] workflow_dispatch: @@ -109,7 +120,20 @@ concurrency: # A branch name is not a unique lane when two event types can each produce a # run for it, so the event is now always part of the key. group: quality-${{ github.head_ref || github.ref_name }}${{ github.event_name != 'pull_request' && format('-{0}', github.event_name) || '' }} - cancel-in-progress: true + + # PUSH RUNS ARE NOT CANCELLED — and this has to be said HERE, not only in the + # shared workflow. .github#597 set `cancel-in-progress` on quality.yml itself, + # but a caller's own concurrency cancels the whole run before the called + # workflow's setting can apply, so that fix reached only the apps that declare + # no concurrency of their own. Measured 2026-08-28 over push runs on + # `development` since #597: 0 of 11 cancelled where the caller was silent, 7 of + # 13 (54%) cancelled where the caller still said `true`. + # + # An integration branch needs a verdict per commit: the run being cancelled is + # the only thing that would have said whether what just landed is sound, and + # its replacement is cancelled too. `pull_request` keeps cancelling, where + # superseding really is correct. + cancel-in-progress: ${{ github.event_name != 'push' }} # Permission CEILING for the called quality pipeline. GitHub statically # validates the called workflow's declared job permissions against this diff --git a/.github/workflows/documentation.yml b/.github/workflows/documentation.yml index 2db235eda..dfc8b6300 100644 --- a/.github/workflows/documentation.yml +++ b/.github/workflows/documentation.yml @@ -1,13 +1,35 @@ name: Documentation +# Publishes the docs site to the Cloudflare Worker that serves it. +# +# TRIGGERS ON `development`, NOT ON A `documentation` BRANCH. This file used to +# listen on a branch called `documentation`; nobody has pushed to one since +# 2026-05-25, so the site simply stopped being rebuilt while every docs change +# merged to development satisfied its review and published nothing. on: push: - branches: [documentation] + branches: [development] pull_request: - branches: [documentation] + branches: [development] jobs: deploy: uses: ConductionNL/.github/.github/workflows/documentation.yml@main + # A reusable workflow receives NO secrets by default. Without this block the + # callee's publish step finds CF_API_TOKEN empty, skips itself on its own + # `if:` guard, and the run finishes GREEN having changed nothing -- the + # failure that left the fleet's docs sites on May builds. The names are the + # same on both sides; the org secrets really are CF_API_TOKEN/CF_ACCOUNT_ID. + secrets: + CF_API_TOKEN: ${{ secrets.CF_API_TOKEN }} + CF_ACCOUNT_ID: ${{ secrets.CF_ACCOUNT_ID }} with: cname: shillinq.conduction.nl + # EVERY host this worker answers on, in FULL: wrangler reconciles the + # worker's triggers against this list, so a host left out is REMOVED and + # goes dark. + docs-hosts: shillinq.conduction.nl + # PINNED. Deriving the name is how a deploy goes green and reaches + # nobody: wrangler creates the derived worker and publishes there while + # the custom domains keep routing to the real one. + worker-name: shillinq-docs diff --git a/appinfo/info.xml b/appinfo/info.xml index d62f61539..b5c72471c 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -26,7 +26,7 @@ Vrij en open source onder de EUPL-1.2-licentie. **Ondersteuning:** Voor ondersteuning, neem contact op via support@conduction.nl. ]]> - 0.2.2-beta.20260820202635 + 0.2.3-unstable.20260829195247 EUPL-1.2 Conduction Shillinq @@ -213,6 +213,19 @@ Vrij en open source onder de EUPL-1.2-licentie. SpendAnalytics refusing its GL-backed views rather than serving either cross-administration totals or a silently-zeroed scoped one. --> OCA\Shillinq\Repair\BackfillGlLineAdministration + + OCA\Shillinq\Repair\BackfillGlLineFiscalYear OCA\Shillinq\Repair\BackfillFiscalPeriods OCA\Shillinq\Repair\PeriodCloseBackfill OCA\Shillinq\Repair\LoadCbsSeedsStep @@ -220,7 +233,7 @@ Vrij en open source onder de EUPL-1.2-licentie. OCA\Shillinq\Repair\LoadDbaSeedsStep OCA\Shillinq\Repair\MigrateProductVendorMasterToPipelinq OCA\Shillinq\Repair\DelegateSigningMigrationRepair - + and been confirmed for at least one release, and the delete has itself been live-verified on real data. + + The tracker was #503 until now, which is the CODEBERG issue number. On GitHub #503 resolves to an + unrelated MERGED pull request, so following the old reference lands on finished work and reads as + though the hold were stale. It migrated to GitHub as #1094. Corrected because gate-98 + (repair-step-registration) flags this step as "written but never registered" and cannot tell a + deliberate hold from an oversight — its suggested remedy is to register it, which on a step that + deletes irreversible source rows is the wrong move. The comment is the only thing standing between + that gate's advice and real data, so its reference has to resolve. --> OCA\Shillinq\Repair\RematerialiseConvertedCalculations + + + + + diff --git a/tests/Unit/Consolidation/ConsolidationGuardTest.php b/tests/Unit/Consolidation/ConsolidationGuardTest.php index 2ba99f157..90f263c51 100644 --- a/tests/Unit/Consolidation/ConsolidationGuardTest.php +++ b/tests/Unit/Consolidation/ConsolidationGuardTest.php @@ -22,7 +22,7 @@ namespace OCA\Shillinq\Tests\Unit\Consolidation; -use OCA\Shillinq\Consolidation\ConsolidationGuard; +use OCA\Shillinq\Service\ConsolidationGuard; use OCA\Shillinq\Tests\Unit\Service\Support\DuckObjectServiceAdapter; use OCP\IAppConfig; use PHPUnit\Framework\MockObject\MockObject; diff --git a/tests/Unit/Controller/RecognitionControllerTest.php b/tests/Unit/Controller/RecognitionControllerTest.php index 305ace0fa..70111642f 100644 --- a/tests/Unit/Controller/RecognitionControllerTest.php +++ b/tests/Unit/Controller/RecognitionControllerTest.php @@ -23,7 +23,7 @@ namespace OCA\Shillinq\Tests\Unit\Controller; use OCA\Shillinq\Controller\RecognitionController; -use OCA\Shillinq\Recognition\RevenueRecognitionService; +use OCA\Shillinq\Service\RevenueRecognitionService; use OCA\Shillinq\Service\AdministrationContextService; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; diff --git a/tests/Unit/Controller/SetupControllerTest.php b/tests/Unit/Controller/SetupControllerTest.php index 6e278e047..c7500b640 100644 --- a/tests/Unit/Controller/SetupControllerTest.php +++ b/tests/Unit/Controller/SetupControllerTest.php @@ -30,6 +30,7 @@ namespace OCA\Shillinq\Tests\Unit\Controller; use OCA\Shillinq\Controller\SetupController; +use OCA\Shillinq\Service\DemoDataService; use OCA\Shillinq\Service\SettingsService; use OCP\AppFramework\Http; use OCP\IAppConfig; @@ -68,6 +69,13 @@ final class SetupControllerTest extends TestCase { * * @var SetupController */ + /** + * Mock DemoDataService. + * + * @var DemoDataService&MockObject + */ + private DemoDataService&MockObject $demoDataService; + private SetupController $controller; /** @@ -81,11 +89,13 @@ protected function setUp(): void { $this->request = $this->createMock(IRequest::class); $this->appConfig = $this->createMock(IAppConfig::class); $this->settingsService = $this->createMock(SettingsService::class); + $this->demoDataService = $this->createMock(DemoDataService::class); $this->controller = new SetupController( appName: 'shillinq', request: $this->request, appConfig: $this->appConfig, + demoDataService: $this->demoDataService, settingsService: $this->settingsService, ); @@ -210,6 +220,72 @@ public function testSaveConfigPersistsParamsAndSkipsRoute(): void { * * @return void */ + /** + * 🔴 THE COUNTS REACH THE OPERATOR. "Demo data installed" with no numbers + * cannot be told apart from an import that wrote nothing — the exact defect + * the openregister half of this programme shipped and had to fix. + * + * @return void + */ + public function testRunActionInstallDemoDataReportsWhatLanded(): void { + $this->demoDataService->expects($this->once()) + ->method('install') + ->willReturn(['objects' => 1497, 'schemas' => 499]); + + $this->appConfig->expects($this->once()) + ->method('setValueString') + ->with('shillinq', 'demo_data_decided', 'installed'); + + $response = $this->controller->runAction('install-demo-data'); + $data = $response->getData(); + + self::assertTrue($data['success']); + self::assertStringContainsString('1497', $data['message']); + self::assertStringContainsString('499', $data['message']); + + }//end testRunActionInstallDemoDataReportsWhatLanded() + + /** + * 🔴 A FAILED INSTALL LEAVES THE STEP UNDECIDED. + * + * The decision is recorded only after the import returns. Recording it first + * would let a failed install present as a finished step: the wizard would + * never offer it again and nobody would learn the demo data is absent. + * + * @return void + */ + public function testRunActionInstallDemoDataFailureLeavesTheStepUndecided(): void { + $this->demoDataService->method('install') + ->willThrowException(new \RuntimeException('openregister is not installed')); + + $this->appConfig->expects($this->never())->method('setValueString'); + + $data = $this->controller->runAction('install-demo-data')->getData(); + + self::assertFalse($data['success']); + self::assertStringContainsString('openregister', $data['message']); + + }//end testRunActionInstallDemoDataFailureLeavesTheStepUndecided() + + /** + * Skipping is a DECISION, not the absence of one: it records `skipped` so + * the optional wizard stops offering the step, and imports nothing. + * + * @return void + */ + public function testRunActionSkipDemoDataRecordsTheDecisionWithoutImporting(): void { + $this->demoDataService->expects($this->never())->method('install'); + + $this->appConfig->expects($this->once()) + ->method('setValueString') + ->with('shillinq', 'demo_data_decided', 'skipped'); + + $data = $this->controller->runAction('skip-demo-data')->getData(); + + self::assertTrue($data['success']); + + }//end testRunActionSkipDemoDataRecordsTheDecisionWithoutImporting() + public function testRunActionInitAdministrationPersistsSeedAdministrationCode(): void { $this->settingsService->expects($this->once())->method('loadConfigurationForced'); $this->settingsService->expects($this->once()) diff --git a/tests/Unit/Integration/TenderNedStatusSyncTest.php b/tests/Unit/Integration/TenderNedStatusSyncTest.php index 97b0e392b..8e0f0a3d6 100644 --- a/tests/Unit/Integration/TenderNedStatusSyncTest.php +++ b/tests/Unit/Integration/TenderNedStatusSyncTest.php @@ -35,7 +35,7 @@ namespace OCA\Shillinq\Tests\Unit\Integration; -use OCA\Shillinq\Integration\TenderNedStatusSync; +use OCA\Shillinq\Service\TenderNedStatusSync; use OCP\IAppConfig; use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; diff --git a/tests/Unit/Lifecycle/RechtmatigheidWorkflowTest.php b/tests/Unit/Lifecycle/RechtmatigheidWorkflowTest.php index 9cdb52f25..37b4b99dd 100644 --- a/tests/Unit/Lifecycle/RechtmatigheidWorkflowTest.php +++ b/tests/Unit/Lifecycle/RechtmatigheidWorkflowTest.php @@ -528,9 +528,19 @@ public function testQuarterlyReportFiltersOpenstaandeBevindingen(): void { actual: ($agg['groupBy'] ?? null), message: 'foutenPerBoekjaar must group by boekjaar.' ); + // `metrics`, not `sum`. AggregationRunner reads neither `sum` nor + // `source`, so this aggregation produced nothing at all — it did not + // fail, it just answered with no figures. + self::assertArrayNotHasKey('sum', $agg, '`sum` is not an engine key'); + $summed = []; + foreach (($agg['metrics'] ?? []) as $metric) { + self::assertSame('sum', $metric['metric']); + $summed[] = $metric['field']; + } + self::assertSame( expected: ['amount_error', 'amount_uncertainty'], - actual: ($agg['sum'] ?? null), + actual: $summed, message: 'foutenPerBoekjaar must sum bedrag_fout + bedrag_onzekerheid.' ); diff --git a/tests/Unit/Listener/OrderFulfilmentTransitionListenerTest.php b/tests/Unit/Listener/OrderFulfilmentTransitionListenerTest.php index f46a43c42..0c34bd010 100644 --- a/tests/Unit/Listener/OrderFulfilmentTransitionListenerTest.php +++ b/tests/Unit/Listener/OrderFulfilmentTransitionListenerTest.php @@ -34,7 +34,7 @@ use OCA\OpenRegister\Db\ObjectEntity; use OCA\OpenRegister\Event\ObjectTransitionedEvent; -use OCA\Shillinq\Integration\TenderNedStatusSync; +use OCA\Shillinq\Service\TenderNedStatusSync; use OCA\Shillinq\Listener\OrderFulfilmentTransitionListener; use OCA\Shillinq\Service\BudgetImpactEmitter; use OCA\Shillinq\Service\ListenerSchemaResolver; @@ -108,7 +108,19 @@ private function spyingSync(): TenderNedStatusSync { */ public array $syncCalls = []; + /** + * When true, syncCompletion() throws — so a test can show the + * listener's fail-soft catch keeps the transition alive. + * + * @var bool + */ + public bool $throwOnSync = false; + public function syncCompletion(array $oplevering): bool { + if ($this->throwOnSync === true) { + throw new \RuntimeException('TenderNed is unreachable'); + } + $this->syncCalls[] = $oplevering; return true; }//end syncCompletion() @@ -344,6 +356,75 @@ public function testUnapprovedEindopleveringDoesNotTriggerSync(): void { * * @return void */ + /** + * An event of another type is ignored. The dispatcher hands `Event`, so + * the instanceof guard is the only thing between this listener and a + * getTo() that does not exist on that class. + * + * @return void + */ + public function testAnEventOfAnotherTypeIsIgnored(): void { + $dispatcher = $this->recordingDispatcher(); + $sync = $this->spyingSync(); + $listener = new OrderFulfilmentTransitionListener( + new BudgetImpactEmitter($dispatcher, new NullLogger()), + $sync, + $this->resolver('OrderFulfilment'), + new NullLogger() + ); + + $listener->handle(new \OCP\EventDispatcher\GenericEvent()); + + $this->assertCount(0, $dispatcher->events); + $this->assertCount(0, $sync->syncCalls); + + }//end testAnEventOfAnotherTypeIsIgnored() + + /** + * THE FAIL-SOFT CONTRACT. This listener runs inside another object's + * transition, so a throw here would roll that transition back. A sync + * that blows up must be logged and swallowed, not propagated — and the + * milestone event that already fired must still stand. + * + * @return void + */ + public function testAThrowingSyncIsSwallowedAndTheMilestoneStillEmitted(): void { + $dispatcher = $this->recordingDispatcher(); + $sync = $this->spyingSync(); + $sync->throwOnSync = true; + + $listener = new OrderFulfilmentTransitionListener( + new BudgetImpactEmitter($dispatcher, new NullLogger()), + $sync, + $this->resolver('OrderFulfilment'), + new NullLogger() + ); + + $listener->handle( + new ObjectTransitionedEvent( + $this->entity('1201', [ + 'commitmentId' => 'TN-2026-0001', + 'milestoneId' => 'M-EIND', + 'deliveryType' => 'eindoplevering', + 'approved' => true, + ]), + 'afronden', + 'in-progress', + 'completed', + 'admin', + 'shillinq', + 'OrderFulfilment' + ) + ); + + $this->assertCount( + 1, + $dispatcher->events, + 'the milestone event fires before the sync, so it must survive the sync failing' + ); + + }//end testAThrowingSyncIsSwallowedAndTheMilestoneStillEmitted() + public function testNonCompletedTransitionIsIgnored(): void { $dispatcher = $this->recordingDispatcher(); $emitter = new BudgetImpactEmitter($dispatcher, new NullLogger()); diff --git a/tests/Unit/Recognition/RevenueRecognitionServiceTest.php b/tests/Unit/Recognition/RevenueRecognitionServiceTest.php index d15391684..3f208d510 100755 --- a/tests/Unit/Recognition/RevenueRecognitionServiceTest.php +++ b/tests/Unit/Recognition/RevenueRecognitionServiceTest.php @@ -22,7 +22,7 @@ namespace OCA\Shillinq\Tests\Unit\Recognition; -use OCA\Shillinq\Recognition\RevenueRecognitionService; +use OCA\Shillinq\Service\RevenueRecognitionService; use OCP\IAppConfig; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; diff --git a/tests/Unit/Service/DemoDataServiceTest.php b/tests/Unit/Service/DemoDataServiceTest.php new file mode 100644 index 000000000..b0d2f258f --- /dev/null +++ b/tests/Unit/Service/DemoDataServiceTest.php @@ -0,0 +1,226 @@ + + */ + +declare(strict_types=1); + +namespace OCA\Shillinq\Tests\Unit\Service; + +use OCA\Shillinq\Service\DemoDataService; +use OCP\App\IAppManager; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * Covers the demo import's decision table. + */ +class DemoDataServiceTest extends TestCase { + private IAppManager&MockObject $appManager; + + private ContainerInterface&MockObject $container; + + private DemoDataService $service; + + private string $appDir; + + protected function setUp(): void { + $this->appDir = sys_get_temp_dir() . '/shillinq-demo-' . uniqid(); + mkdir($this->appDir . '/lib/Settings', 0777, true); + + $this->appManager = $this->createMock(IAppManager::class); + $this->container = $this->createMock(ContainerInterface::class); + + $this->appManager->method('getAppPath')->willReturn($this->appDir); + $this->appManager->method('getAppVersion')->willReturn('1.2.3'); + $this->appManager->method('getInstalledApps')->willReturn(['shillinq', 'openregister']); + + $this->service = new DemoDataService( + $this->appManager, + $this->container, + $this->createMock(LoggerInterface::class) + ); + } + + protected function tearDown(): void { + $file = $this->appDir . '/lib/Settings/shillinq_mock_register.json'; + if (is_file($file) === true) { + unlink($file); + } + @rmdir($this->appDir . '/lib/Settings'); + @rmdir($this->appDir . '/lib'); + @rmdir($this->appDir); + } + + private function shipDescriptor(int $objects = 2): void { + file_put_contents( + $this->appDir . '/lib/Settings/shillinq_mock_register.json', + json_encode( + [ + 'x-openregister' => ['type' => 'mock', 'app' => 'shillinq'], + 'components' => [ + 'registers' => ['shillinq' => ['schemas' => ['Thing']]], + 'schemas' => ['Thing' => ['type' => 'object']], + 'objects' => array_fill(0, $objects, ['@self' => ['register' => 'shillinq', 'schema' => 'Thing']]), + ], + ] + ) + ); + } + + /** + * A stand-in for OpenRegister's importer that records how it was called. + * + * @return object The fake. + */ + private function importerSpy(): object { + return new class { + /** @var array */ + public array $seen = []; + + /** + * @param string $appId Config identity. + * @param array $data Descriptor. + * @param string $version App version. + * @param boolean $force Whether the version gate is bypassed. + * + * @return array + */ + public function importFromApp(string $appId, array $data, string $version, bool $force): array { + $this->seen = ['appId' => $appId, 'version' => $version, 'force' => $force]; + return ['registers' => ['shillinq'], 'schemas' => ['Thing']]; + } + }; + } + + public function testItImportsTheDescriptorAndReportsTheCounts(): void { + $this->shipDescriptor(objects: 5); + $spy = $this->importerSpy(); + $this->container->method('get')->willReturn($spy); + + $result = $this->service->install(); + + $this->assertSame(5, $result['objects']); + $this->assertSame(1, $result['registers']); + $this->assertSame(1, $result['schemas']); + } + + /** + * 🔴 THE IMPORT IS FORCED. OpenRegister version-gates a non-forced import + * and SKIPS silently when the version has not moved. An operator who asks + * for demo data and is told it worked, on an instance where nothing was + * written, has been lied to by a version compare. + */ + public function testTheImportIsForcedSoAVersionGateCannotSilentlySkipIt(): void { + $this->shipDescriptor(); + $spy = $this->importerSpy(); + $this->container->method('get')->willReturn($spy); + + $this->service->install(); + + $this->assertTrue($spy->seen['force'], 'a version gate must not be able to skip an explicit request'); + } + + /** + * Its own configuration identity, so the demo import and the real + * configuration import cannot mask one another's version gate. + */ + public function testItImportsUnderItsOwnConfigurationIdentity(): void { + $this->shipDescriptor(); + $spy = $this->importerSpy(); + $this->container->method('get')->willReturn($spy); + + $this->service->install(); + + $this->assertSame('shillinq.demo', $spy->seen['appId']); + } + + public function testAMissingDescriptorThrowsRatherThanReportingSuccess(): void { + $this->container->expects($this->never())->method('get'); + + $this->expectException(RuntimeException::class); + $this->service->install(); + } + + public function testUnparsableJsonThrowsRatherThanImportingNothing(): void { + file_put_contents($this->appDir . '/lib/Settings/shillinq_mock_register.json', 'not json'); + $this->container->expects($this->never())->method('get'); + + $this->expectException(RuntimeException::class); + $this->service->install(); + } + + /** + * 🔴 NAMES THE MISSING APP. "Something went wrong" on a cross-app lookup + * leaves an operator with nothing to act on; a cross-app class is a runtime + * lookup that finds nobody rather than erroring usefully. + */ + public function testWithoutOpenRegisterItRefusesAndSaysWhichAppIsMissing(): void { + $this->shipDescriptor(); + $appManager = $this->createMock(IAppManager::class); + $appManager->method('getAppPath')->willReturn($this->appDir); + $appManager->method('getAppVersion')->willReturn('1.2.3'); + $appManager->method('getInstalledApps')->willReturn(['shillinq']); + + $service = new DemoDataService($appManager, $this->container, $this->createMock(LoggerInterface::class)); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessageMatches('/OpenRegister/'); + $service->install(); + } + + /** + * 🔴 UNREADABLE IS NOT THE SAME AS ABSENT, AND NOT THE SAME AS EMPTY. + * + * `is_file()` passes on a descriptor whose contents cannot be read — a + * permission change, a truncated mount — and `file_get_contents()` then + * returns false. Without its own branch that false would flow into + * `json_decode(false)`, which yields null, and the operator would be told + * the dataset "is not valid JSON" when the real fault is that nothing could + * read it. Two different repairs, so two different messages. + * + * Skipped when the process can read anything regardless of mode (root in a + * container), because there the arm would assert nothing. + * + * @return void + */ + public function testAnUnreadableDescriptorSaysSoRatherThanBlamingTheJson(): void { + $this->shipDescriptor(); + $path = $this->appDir . '/lib/Settings/shillinq_mock_register.json'; + + if (chmod($path, 0000) === false || is_readable($path) === true) { + chmod($path, 0644); + self::markTestSkipped('this process reads regardless of mode; the arm would prove nothing'); + } + + try { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageMatches('/could not be read/'); + $this->service->install(); + } finally { + chmod($path, 0644); + } + + }//end testAnUnreadableDescriptorSaysSoRatherThanBlamingTheJson() + + public function testIsAvailableReflectsWhetherTheDescriptorShips(): void { + $this->assertFalse($this->service->isAvailable(), 'no descriptor on disk'); + $this->shipDescriptor(); + $this->assertTrue($this->service->isAvailable()); + } +} diff --git a/tests/Unit/Service/EmuReportingFragmentTest.php b/tests/Unit/Service/EmuReportingFragmentTest.php index f34a42603..4544d0648 100644 --- a/tests/Unit/Service/EmuReportingFragmentTest.php +++ b/tests/Unit/Service/EmuReportingFragmentTest.php @@ -152,9 +152,20 @@ public function testDebtPositionEsa2010ClassificationAndAggregation(): void { self::assertContains($cat, $enum, "categorieEurostat must include $cat"); } + // `metric`/`field`, not `sum`. AggregationRunner reads neither `sum` nor + // `source`, so this computed nothing at all. $agg = $schema['x-openregister-aggregations']['brutoSchuldPerCategorie']; self::assertTrue($agg['filter']['teltMeeInEmuDebt']); - self::assertContains('outstandingDebt', $agg['sum']); + self::assertSame('sum', $agg['metric']); + self::assertSame('outstandingDebt', $agg['field']); + self::assertArrayNotHasKey('sum', $agg, '`sum` is not an engine key'); + self::assertArrayNotHasKey('source', $agg, '`source` is not an engine key'); + + // The per-report correlation is a groupBy DIMENSION now: `reportId: + // "@self.reportId"` needed a parent row that no caller supplies, so it + // stayed a literal string and matched nothing. + self::assertContains('reportId', $agg['groupBy']); + self::assertArrayNotHasKey('reportId', $agg['filter']); }//end testDebtPositionEsa2010ClassificationAndAggregation() /** diff --git a/tests/Unit/Service/Migration/GlLineFiscalYearBackfillMigratorTest.php b/tests/Unit/Service/Migration/GlLineFiscalYearBackfillMigratorTest.php new file mode 100644 index 000000000..8d243358e --- /dev/null +++ b/tests/Unit/Service/Migration/GlLineFiscalYearBackfillMigratorTest.php @@ -0,0 +1,235 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://conduction.nl + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Shillinq\Tests\Unit\Service\Migration; + +use OCA\Shillinq\Service\Migration\GlLineFiscalYearBackfillMigrator; +use PHPUnit\Framework\TestCase; +use RuntimeException; + +/** + * No `covers` metadata, deliberately — `beStrictAboutCoverageMetadata="true"` + * discards the coverage of any test that touches a collaborator it did not + * name. + */ +class GlLineFiscalYearBackfillMigratorTest extends TestCase { + + private GlLineFiscalYearBackfillMigrator $migrator; + + protected function setUp(): void { + parent::setUp(); + $this->migrator = new GlLineFiscalYearBackfillMigrator(); + } + + /** + * @test + * A transaction carrying no fiscal year is not indexed at all. + * + * Indexing it against `''` would let classify() call a line resolved while + * stamping an empty year — a row that counts as backfilled and answers + * nothing, which is the exact failure the whole step exists to avoid. + */ + public function testTransactionWithoutFiscalYearIsNotIndexed(): void { + $index = $this->migrator->indexFiscalYearsByTransaction([ + ['id' => 'tx-1', 'fiscalYearId' => 'fy-2026-nl'], + ['id' => 'tx-2', 'fiscalYearId' => ''], + ['id' => 'tx-3'], + ['id' => 'tx-4', 'fiscalYearId' => ' '], + ]); + + $this->assertSame(['tx-1' => 'fy-2026-nl'], $index); + } + + /** + * @test + * A transaction is indexed under every identity a line might reference it by. + */ + public function testTransactionIndexedUnderEveryIdentity(): void { + $index = $this->migrator->indexFiscalYearsByTransaction([ + [ + 'id' => 'tx-1', + '@self' => ['id' => 'self-1'], + 'uuid' => 'uuid-1', + 'transactionNumber' => 'TX-0001', + 'fiscalYearId' => 'fy-2026-nl', + ], + ]); + + foreach (['tx-1', 'self-1', 'uuid-1', 'TX-0001'] as $identity) { + $this->assertArrayHasKey($identity, $index, "identity $identity must resolve"); + $this->assertSame('fy-2026-nl', $index[$identity]); + } + } + + /** + * @test + * A line with no resolvable parent is UNRESOLVABLE, not silently stamped. + */ + public function testUnresolvableLineIsClassifiedNotStamped(): void { + $index = $this->migrator->indexFiscalYearsByTransaction([ + ['id' => 'tx-1', 'fiscalYearId' => 'fy-2026-nl'], + ]); + + $this->assertSame( + GlLineFiscalYearBackfillMigrator::CLASS_UNRESOLVABLE, + $this->migrator->classify(['transactionId' => 'tx-missing'], $index) + ); + $this->assertSame( + GlLineFiscalYearBackfillMigrator::CLASS_UNRESOLVABLE, + $this->migrator->classify(['transactionId' => ''], $index) + ); + $this->assertSame( + GlLineFiscalYearBackfillMigrator::CLASS_RESOLVED, + $this->migrator->classify(['transactionId' => 'tx-1'], $index) + ); + } + + /** + * @test + * A line that already carries a fiscal year is never rewritten. + * + * Re-pointing a posted line to a different year is a bigger decision than + * a backfill gets to make, so the existing value wins even when the parent + * now disagrees. + */ + public function testExistingFiscalYearIsNeverOverwritten(): void { + $line = ['transactionId' => 'tx-1', 'fiscalYearId' => 'fy-2025-nl']; + + $stamped = $this->migrator->stampFiscalYearId($line, 'fy-2026-nl'); + + $this->assertSame('fy-2025-nl', $stamped['fiscalYearId']); + $this->assertSame($line, $stamped, 'the row must come back byte-identical'); + } + + /** + * @test + * Stamping preserves every other field on the row. + */ + public function testStampPreservesEveryOtherField(): void { + $line = [ + 'transactionId' => 'tx-1', + 'amount' => 125.50, + 'side' => 'debit', + 'accountNumber' => '4000', + ]; + + $stamped = $this->migrator->stampFiscalYearId($line, 'fy-2026-nl'); + + $this->assertSame('fy-2026-nl', $stamped['fiscalYearId']); + foreach ($line as $key => $value) { + $this->assertSame($value, $stamped[$key], "field $key must survive"); + } + } + + /** + * @test + * One unresolvable row does NOT abort the batch. + * + * This is the deliberate difference from the administration backfill, where + * an unclassifiable row aborts everything because a half-scoped ledger makes + * a tenant filter return a silent zero. A fiscal year is a GROUPING key: an + * unresolved row shows up as a null bucket, which is visible. Aborting here + * would trade a visible gap for no backfill at all. + */ + public function testUnresolvableRowDoesNotAbortTheBatch(): void { + $report = $this->migrator->backfillBatch( + [ + ['transactionId' => 'tx-1'], + ['transactionId' => 'tx-orphan'], + ['transactionId' => 'tx-1', 'fiscalYearId' => 'fy-2024-nl'], + ], + [['id' => 'tx-1', 'fiscalYearId' => 'fy-2026-nl']] + ); + + $this->assertSame(3, $report['seen']); + $this->assertSame(1, $report['stamped']); + $this->assertSame(1, $report['alreadyStamped']); + $this->assertSame(1, $report['unresolvable']); + + // Only the resolvable row is returned for writing, keyed by its source + // offset so the caller can pair it with the object it came from. + $this->assertSame([0], array_keys($report['lines'])); + $this->assertSame('fy-2026-nl', $report['lines'][0]['fiscalYearId']); + } + + /** + * @test + * A parent that disagrees with an already-stamped line is REPORTED. + * + * Silently keeping either value hides a real inconsistency in posted books. + */ + public function testDisagreementWithParentIsReported(): void { + $report = $this->migrator->backfillBatch( + [['transactionId' => 'tx-1', 'fiscalYearId' => 'fy-2024-nl']], + [['id' => 'tx-1', 'fiscalYearId' => 'fy-2026-nl']] + ); + + $this->assertSame(1, $report['alreadyStamped']); + $this->assertCount(1, $report['disagreements']); + $this->assertStringContainsString('fy-2024-nl', $report['disagreements'][0]); + $this->assertStringContainsString('fy-2026-nl', $report['disagreements'][0]); + $this->assertSame([], $report['lines'], 'nothing may be rewritten'); + } + + /** + * @test + * A second run over already-backfilled rows writes nothing. + */ + public function testSecondRunIsANoOp(): void { + $lines = [['transactionId' => 'tx-1']]; + $transactions = [['id' => 'tx-1', 'fiscalYearId' => 'fy-2026-nl']]; + + $first = $this->migrator->backfillBatch($lines, $transactions); + $this->assertSame(1, $first['stamped']); + + $second = $this->migrator->backfillBatch(array_values($first['lines']), $transactions); + + $this->assertSame(0, $second['stamped'], 'a re-run must write nothing'); + $this->assertSame(1, $second['alreadyStamped']); + $this->assertSame([], $second['lines']); + } + + /** + * @test + * countMissingFiscalYearId() reports a TOTAL, including blank strings. + */ + public function testCountMissingCountsBlanksAsMissing(): void { + $this->assertSame(3, $this->migrator->countMissingFiscalYearId([ + ['fiscalYearId' => 'fy-2026-nl'], + ['fiscalYearId' => ''], + ['fiscalYearId' => ' '], + [], + ])); + } + + /** + * @test + * A report that accounts for fewer rows than were seen is refused. + */ + public function testCountMismatchThrows(): void { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessageMatches('/classified/'); + + $this->migrator->assertCountsMatch(sourceCount: 10, classifiedCount: 9); + } +}//end class diff --git a/tests/Unit/Service/PayrollDetacheringFragmentTest.php b/tests/Unit/Service/PayrollDetacheringFragmentTest.php index 991aed47f..09ec852ca 100644 --- a/tests/Unit/Service/PayrollDetacheringFragmentTest.php +++ b/tests/Unit/Service/PayrollDetacheringFragmentTest.php @@ -139,11 +139,23 @@ public function testPayrollAggregations(): void { $payroll = $this->fragment()['components']['schemas']['Payroll']; $aggregations = $payroll['x-openregister-aggregations']; + // `from`, not `source`. AggregationRunner reads `from` and nothing else — + // `source` was an inert key it never consulted, so these aggregated the + // DECLARING schema instead of Deduction. self::assertArrayHasKey('netAmount', $aggregations); - self::assertSame('Deduction', $aggregations['netAmount']['source']); + self::assertSame('Deduction', $aggregations['netAmount']['from']); + self::assertArrayNotHasKey('source', $aggregations['netAmount'], '`source` is not an engine key'); + self::assertArrayHasKey('annualEmployeeDeductions', $aggregations); - self::assertSame('Deduction', $aggregations['annualEmployeeDeductions']['source']); + self::assertSame('Deduction', $aggregations['annualEmployeeDeductions']['from']); self::assertContains('deductionType', $aggregations['annualEmployeeDeductions']['groupBy']); + + // The `@self` correlation became a groupBy DIMENSION. No caller supplies a + // parent row, so `payrollId: "@self.id"` stayed a literal string and matched + // nothing — an empty result under HTTP 200. Grouping by the same field needs + // no parent row and is narrowed per record through extraFilter. + self::assertContains('payrollId', $aggregations['netAmount']['groupBy']); + self::assertArrayNotHasKey('payrollId', ($aggregations['netAmount']['filter'] ?? [])); }//end testPayrollAggregations() /** diff --git a/tests/Unit/Service/ProvinciesBbvFragmentTest.php b/tests/Unit/Service/ProvinciesBbvFragmentTest.php index 41116abf4..4023927f9 100644 --- a/tests/Unit/Service/ProvinciesBbvFragmentTest.php +++ b/tests/Unit/Service/ProvinciesBbvFragmentTest.php @@ -143,7 +143,16 @@ public function testGlLineOverlayDeclaresGuardAndAggregation(): void { self::assertArrayHasKey('programmeBudgetVsActuals', $glLine['x-openregister-aggregations']); $agg = $glLine['x-openregister-aggregations']['programmeBudgetVsActuals']; self::assertContains('programmeStructure', $agg['groupBy']); - self::assertArrayHasKey('spent', $agg['operations']); + + // `metrics`, not `operations`. AggregationRunner never read `operations`, + // so none of these figures were produced — the aggregation returned + // nothing rather than failing. + self::assertArrayNotHasKey('operations', $agg, '`operations` is not an engine key'); + $byAlias = []; + foreach ($agg['metrics'] as $metric) { + $byAlias[$metric['as']] = $metric; + } + self::assertArrayHasKey('spent', $byAlias); }//end testGlLineOverlayDeclaresGuardAndAggregation() diff --git a/tests/Unit/Service/RetainerBillingFragmentTest.php b/tests/Unit/Service/RetainerBillingFragmentTest.php index d4d24df7b..f68f8aa9c 100644 --- a/tests/Unit/Service/RetainerBillingFragmentTest.php +++ b/tests/Unit/Service/RetainerBillingFragmentTest.php @@ -157,10 +157,24 @@ public function testDrawdownBalanceIsDeclarativeAggregation(): void { self::assertArrayHasKey('drawdownsByPool', $agg); $drawdownsByPool = $agg['drawdownsByPool']; - self::assertSame('RetainerDrawdown', $drawdownsByPool['source']); - self::assertArrayHasKey('operations', $drawdownsByPool); - self::assertArrayHasKey('drawnAmount', $drawdownsByPool['operations']); - self::assertSame('sum', $drawdownsByPool['operations']['drawnAmount']['operation']); + + // `from`/`metrics`, not `source`/`operations`. Neither `source` nor + // `operations` is read by AggregationRunner, so this computed nothing. + self::assertSame('RetainerDrawdown', $drawdownsByPool['from']); + self::assertArrayNotHasKey('source', $drawdownsByPool, '`source` is not an engine key'); + self::assertArrayNotHasKey('operations', $drawdownsByPool, '`operations` is not an engine key'); + + self::assertArrayHasKey('metrics', $drawdownsByPool); + $byAlias = []; + foreach ($drawdownsByPool['metrics'] as $metric) { + $byAlias[$metric['as']] = $metric; + } + self::assertArrayHasKey('drawnAmount', $byAlias); + self::assertSame('sum', $byAlias['drawnAmount']['metric']); + self::assertSame('drawdownAmount', $byAlias['drawnAmount']['field'], 'field is bare — `from` resolves it'); + + // The pool correlation is a groupBy dimension now. + self::assertContains('poolId', $drawdownsByPool['groupBy']); }//end testDrawdownBalanceIsDeclarativeAggregation() diff --git a/tests/Unit/Service/VatBtwFilingFragmentTest.php b/tests/Unit/Service/VatBtwFilingFragmentTest.php index 458814710..fba1354da 100644 --- a/tests/Unit/Service/VatBtwFilingFragmentTest.php +++ b/tests/Unit/Service/VatBtwFilingFragmentTest.php @@ -154,23 +154,53 @@ public function testVatReturnDeclaresReconciliationAggregations(): void { self::assertArrayHasKey('totalsByReturn', $aggregations); $totals = $aggregations['totalsByReturn']; - self::assertSame('VATLine', $totals['source']); - self::assertArrayHasKey('operations', $totals); - self::assertArrayHasKey('totalVATCollected', $totals['operations']); - self::assertArrayHasKey('totalVATPaid', $totals['operations']); + // `from`, not `source`. AggregationRunner reads `from` and nothing else, + // so `source` never switched this onto VATLine at all. + self::assertSame('VATLine', $totals['from']); + self::assertArrayNotHasKey('source', $totals, '`source` is not an engine key'); + + // The per-return correlation is a groupBy DIMENSION now. `returnId: + // "@self.id"` needed a parent row that no caller supplies, so it stayed a + // literal string and matched nothing. + self::assertContains('returnId', $totals['groupBy']); + self::assertArrayNotHasKey('returnId', ($totals['filter'] ?? [])); + + // NOW TRANSLATED. This was pinned as untranslatable in the previous pass, + // for two reasons that have both since been removed: `vatBalance` is an + // `expression` op, which the engine had no equivalent for until derived + // metrics landed (openregister #2941), and the conditions were SQL-ish + // STRINGS where computeMetrics() takes a filter OBJECT. + self::assertArrayNotHasKey('operations', $totals, '`operations` is not an engine key'); + self::assertArrayHasKey('metrics', $totals); + + $byAlias = []; + foreach ($totals['metrics'] as $metric) { + $byAlias[$metric['as']] = $metric; + } + self::assertArrayHasKey('totalVATCollected', $byAlias); + self::assertArrayHasKey('totalVATPaid', $byAlias); + + // The string condition became a filter OBJECT, and the field lost its + // schema prefix — `from` resolves bare names on VATLine. + self::assertSame(['type' => 'collected'], $byAlias['totalVATCollected']['condition']); + self::assertSame('vatAmount', $byAlias['totalVATCollected']['field']); + + // The derived metric names only aliases declared BEFORE it. Anything + // else raises at run time, which validate-registers now refuses at + // declaration time. + self::assertArrayHasKey('vatBalance', $byAlias); + self::assertSame('expression', $byAlias['vatBalance']['metric']); + foreach (preg_split('/[^A-Za-z_]+/', $byAlias['vatBalance']['expression']) as $ident) { + if ($ident === '' || $ident === 'min' || $ident === 'max') { + continue; + } - // Sum operations aggregate over a VATLine.* field; the `vatBalance` - // operation is an expression op derived from the sum results. - foreach ($totals['operations'] as $operation) { - self::assertContains( - $operation['operation'], - ['sum', 'expression'], - 'aggregation operations are sum or expression' + self::assertArrayHasKey( + $ident, + $byAlias, + 'a derived metric may only name aliases this aggregation declares' ); - if ($operation['operation'] === 'sum') { - self::assertStringStartsWith('VATLine.', $operation['field']); - } } }//end testVatReturnDeclaresReconciliationAggregations() diff --git a/tests/Unit/Service/VoorzieningenClaimsFragmentTest.php b/tests/Unit/Service/VoorzieningenClaimsFragmentTest.php index 3ddbe7075..59f002638 100644 --- a/tests/Unit/Service/VoorzieningenClaimsFragmentTest.php +++ b/tests/Unit/Service/VoorzieningenClaimsFragmentTest.php @@ -314,10 +314,20 @@ public function testDisclosureTableAggregationIsDeclared(): void { $schema = $this->fragment()['components']['schemas']['ProvisionDisclosureTabel']; $agg = $schema['x-openregister-aggregations']['provisionDisclosureGeneration']; - self::assertSame('ProvisionMovement', $agg['source']); + // `from`/`metrics`, not `source`/`operations`. AggregationRunner reads + // neither of the old keys, so this aggregation produced none of the eight + // figures below — it just returned nothing, under HTTP 200. + self::assertSame('ProvisionMovement', $agg['from']); + self::assertArrayNotHasKey('source', $agg, '`source` is not an engine key'); + self::assertArrayNotHasKey('operations', $agg, '`operations` is not an engine key'); self::assertSame(['Provision.provisionType', 'ProvisionMovement.period'], $agg['groupBy']); + + $byAlias = []; + foreach ($agg['metrics'] as $metric) { + $byAlias[$metric['as']] = $metric; + } foreach (['openingBalance', 'additions', 'used', 'released', 'unwinding', 'estimatesChange', 'closingBalance', 'count'] as $bucket) { - self::assertArrayHasKey($bucket, $agg['operations'], "Disclosure aggregation must produce $bucket"); + self::assertArrayHasKey($bucket, $byAlias, "Disclosure aggregation must produce $bucket"); } }//end testDisclosureTableAggregationIsDeclared() diff --git a/tests/Unit/Settings/CostCentersDimensionsFragmentTest.php b/tests/Unit/Settings/CostCentersDimensionsFragmentTest.php index 959da124b..f03464441 100755 --- a/tests/Unit/Settings/CostCentersDimensionsFragmentTest.php +++ b/tests/Unit/Settings/CostCentersDimensionsFragmentTest.php @@ -124,12 +124,58 @@ public function testGlLineCarriesSegmentPnlAggregations(): void { foreach (['byCostCenter', 'byCostCenterHierarchy', 'byProject', 'byAnalyticalDimension'] as $key) { self::assertArrayHasKey($key, $aggs, 'Aggregation ' . $key . ' MUST be declared'); - self::assertSame('GLLine', $aggs[$key]['source'], 'Aggregation MUST source GLLine'); - self::assertSame(['amount'], $aggs[$key]['sum'], 'Aggregation MUST sum the amount field'); self::assertArrayHasKey('groupBy', $aggs[$key]); self::assertArrayHasKey('filter', $aggs[$key]); } + // `metric`/`field`, not `source`/`sum`. AggregationRunner reads only + // field/filter/from/groupBy/join/metric/metrics/select/where, so `sum` + // computed nothing and `source` was inert — it is NOT the engine's + // `from`, which switches the runner into its cross-schema path. These + // three declare GLLine ON GLLine, so dropping `source` is the whole + // translation. Verified live: byCostCenter returns CC-001=5000, + // CC-002=10000, null=24200, matching the rows exactly. + foreach (['byCostCenter', 'byProject'] as $key) { + self::assertSame('sum', $aggs[$key]['metric'], 'Aggregation MUST declare metric=sum'); + self::assertSame('amount', $aggs[$key]['field'], 'Aggregation MUST sum the amount field'); + self::assertArrayNotHasKey('source', $aggs[$key], '`source` is not an engine key'); + self::assertArrayNotHasKey('sum', $aggs[$key], '`sum` is not an engine key; use metric+field'); + } + + // byCostCenterHierarchy groups by `AnalyticalDimension.parentCode` — a + // field that exists only on the JOINED schema. applyJoin() runs AFTER + // grouping, so this used to group on a column every row lacks, giving + // one null bucket holding everything: a plausible total, not an error. + // OpenRegister #2916 projects joined group fields onto the rows first, + // which is what makes the declaration computable. + self::assertSame('sum', $aggs['byCostCenterHierarchy']['metric']); + self::assertSame('amount', $aggs['byCostCenterHierarchy']['field']); + self::assertArrayNotHasKey( + 'source', + $aggs['byCostCenterHierarchy'], + '`source` is not an engine key' + ); + + // The `on` SHORTHAND ("AnalyticalDimension.code") is refused by the + // joined-field grouping path: it names the joined side only, leaving the + // parent key to be inferred — and inferring the JOINED field produced a + // single '' bucket rather than one per region. The explicit map states + // both sides: parent GLLine.costCenterCode -> joined AnalyticalDimension.code. + self::assertSame( + ['costCenterCode' => 'code'], + $aggs['byCostCenterHierarchy']['join']['on'], + '`on` MUST be an explicit parent-field => joined-field map' + ); + + // byAnalyticalDimension is still untranslated: it groups by the wildcard + // `dimensions.*`, which no engine key expresses. Tracked in #1261 and + // pinned here so the gap stays visible. + self::assertArrayNotHasKey( + 'metric', + $aggs['byAnalyticalDimension'], + 'still untranslated — wildcard groupBy, see #1261' + ); + // After the ADIM merge (REQ-ADIM-101), byCostCenter joins through the unified // AnalyticalDimension schema (filtered by dimensionType=cost-center) rather than // the retired CostCenter schema. diff --git a/tests/Unit/Validation/BankReconciliationSchemaTest.php b/tests/Unit/Validation/BankReconciliationSchemaTest.php index 18af9ef1f..47c33baba 100644 --- a/tests/Unit/Validation/BankReconciliationSchemaTest.php +++ b/tests/Unit/Validation/BankReconciliationSchemaTest.php @@ -377,17 +377,21 @@ public function testApprovedAmountTotalAggregation(): void { $schema = $this->fragment['components']['schemas']['BankReconciliationMatch']; $agg = $schema['x-openregister-aggregations']['approvedAmountTotal']; - // `approvedAmountTotal` is NOT translated: it uses an `operations` dict - // whose custom alias (`approvedTotal`) the engine cannot express — its - // `metrics` form emits `sum_` keys, so translating it changes - // what every CONSUMER must read, not just the declaration. Its `filter` - // is also a SQL-ish string the engine does not parse. Both are tracked - // in #1261, and this test pins the shape as it stands so the debt stays - // visible rather than looking intentional. - self::assertSame("matchType = 'approved'", $agg['filter']); - self::assertSame('reconciliationId', $agg['groupBy']); - self::assertSame('bankTransactionAmount', $agg['operations']['approvedTotal']['field']); - self::assertSame('sum', $agg['operations']['approvedTotal']['operation']); + // `approvedAmountTotal` IS translated now. Both reasons it was pinned as + // untranslatable have been removed: the custom alias `approvedTotal` is + // expressible since `as` landed, and the SQL-ish string filter converts + // to the filter OBJECT the engine actually reads. + self::assertSame(['matchType' => 'approved'], $agg['filter'], 'filter is an object, not a string'); + self::assertSame(['reconciliationId'], $agg['groupBy'], 'groupBy is an ARRAY — a string is silently dropped'); + self::assertArrayNotHasKey('operations', $agg, '`operations` is not an engine key'); + + $byAlias = []; + foreach ($agg['metrics'] as $metric) { + $byAlias[$metric['as']] = $metric; + } + self::assertArrayHasKey('approvedTotal', $byAlias, 'the custom alias survives as `as`'); + self::assertSame('sum', $byAlias['approvedTotal']['metric']); + self::assertSame('bankTransactionAmount', $byAlias['approvedTotal']['field']); // `countByType` IS translated — a plain count over its own schema. $countByType = $schema['x-openregister-aggregations']['countByType']; diff --git a/tests/check-manifest-budget.js b/tests/check-manifest-budget.js index 40ea3429c..5a3d8d0f9 100644 --- a/tests/check-manifest-budget.js +++ b/tests/check-manifest-budget.js @@ -123,7 +123,27 @@ const MANIFEST_D_DIR = path.join(REPO_ROOT, 'src', 'manifest.d') // which is the honest consequence of the rule as written; if that is not the // intended policy, the fix is to decide a margin deliberately and say why, // not to round this constant up here. -const DEFAULT_BUDGET_BYTES = 1_128_750 +// +// Re-measured 2026-08-27 (ADR-111 demo-data setup step): 1,128,758 bytes. +// +// 🔴 THE HEADROOM WAS ALREADY GONE BEFORE THIS CHANGE. `development` had +// grown to 1,128,545 on its own, leaving 205 B — 0.018% — against the +// 0.58-0.88% this file says the ratio should be. So the tripwire was one +// small edit away from firing for whoever touched the manifest next, and +// what it caught was not a payload problem but its own exhausted margin. +// +// TRIMMED FIRST, then raised, per the note above. ADR-111 rule 4 requires +// `setup.steps[0]` to be the demo-data offer, so the step itself is not +// negotiable; its prose is. The body went from 200 characters to 76 +// ("Load example invoices, subscriptions and ledger entries. Skip on +// production."), recovering 124 B. What is left states the two things an +// operator needs: what lands, and not to run it on production. +// +// Raised to 1,135,500 — 6,742 B of headroom, 0.60%, back inside the ratio +// this file argues for and close to the 0.58% it ran with two bumps ago. +// Not more: the point of restating the ratio is that the next change +// re-measures rather than inheriting slack. +const DEFAULT_BUDGET_BYTES = 1_135_500 /** * Sum the byte size of every regular file in a directory (non-recursive), diff --git a/tests/e2e/ci-seed.sh b/tests/e2e/ci-seed.sh index 62a166e98..0120a57ca 100755 --- a/tests/e2e/ci-seed.sh +++ b/tests/e2e/ci-seed.sh @@ -50,6 +50,11 @@ set -euo pipefail SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" APP_DIR="$(cd -- "${SCRIPT_DIR}/../.." && pwd)" +# The Nextcloud server root, where `occ` lives. CI already runs with cwd set +# there, but derive it from the app's own location so the script also works +# when run from anywhere else — the same definition buildiq's ci-seed.sh uses. +SERVER_DIR="$(cd -- "${APP_DIR}/../.." && pwd)" + # ── Target resolution ──────────────────────────────────────────────────────── # The shared workflow's "Seed test data" step exports BASE_URL / NEXTCLOUD_URL / # NC_BASE_URL / ADMIN_USER / ADMIN_PASSWORD / NC_ADMIN_USER / NC_ADMIN_PASS. @@ -462,6 +467,42 @@ setup_post '/index.php/apps/shillinq/api/setup/config' \ setup_post '/index.php/apps/shillinq/api/setup/action/init-administration' '{}' setup_post '/index.php/apps/shillinq/api/setup/action/seed' '{}' +# The OPTIONAL step too, not just the required ones. +# +# `status()` recomputes every step from its own evidence and writes +# `setup_completed_version` only when the REQUIRED ones are done, so +# `completed: true` says nothing about `demo-data`. Since nextcloud-vue 2.21 an +# outstanding OPTIONAL step is enough to open the wizard on its own +# (nextcloud-vue#806 stopped it short-circuiting on `completed`), so an undone +# `demo-data` puts the dialog over the SPA and every click lands on the overlay. +# +# Measured: 59 failures across 18 unrelated spec files, every one +# `locator.click: Test timeout`, from the merge of #1295 onward. +# +# `demo_data_decided` is the app's own DEALT-WITH flag, not "demo objects +# exist" -- SetupController says re-offering the import every visit would make +# "no thanks" impossible to express. Writing `skipped` is what an operator who +# declined leaves behind, and it avoids seeding a dataset the specs do not +# expect. Same fix as buildiq#523. +# +# `./occ`, not "${SERVER_DIR}/occ": SERVER_DIR is never assigned anywhere in +# this script, and `set -euo pipefail` at the top makes reading it fatal -- +# "line 482: SERVER_DIR: unbound variable", which aborted the whole seed and +# took the E2E job down BEFORE Playwright ran. A guard meant to stop an +# overlay covering the SPA instead stopped the suite existing. +# +# This is the idiom the rest of the file already uses (see the +# htaccess.IgnoreFrontController block above): the workflow invokes this +# script with cwd at the Nextcloud server root, so `./occ` is the reachable +# path and its presence is the correct guard. +if [ -f "./occ" ]; then + if php ./occ config:app:set shillinq demo_data_decided --value=skipped; then + echo "[ci-seed] demo-data step marked decided (skipped)." + else + echo "::warning::could not set demo_data_decided; the wizard may reopen over the SPA." + fi +fi + # VERIFY, do not assume. `status()` is also what persists # `setup_completed_version`, which is the manifest's `completionConfigKey` — # so this call is both the assertion and the last write the wizard needs. @@ -480,8 +521,12 @@ if code != '200': print(raw[:500]) sys.exit(1) body = json.loads(raw) -if body.get('completed') is not True: - undone = [k for k, v in (body.get('steps') or {}).items() if not v.get('done')] +undone = [k for k, v in (body.get('steps') or {}).items() if not v.get('done')] +# EVERY step, not just `completed`. `completed` reflects the REQUIRED steps +# only, so it stayed true while `demo-data` sat undone -- and since +# nextcloud-vue 2.21 that alone reopens the wizard over the SPA. This guard +# passed straight through the exact failure it exists to prevent. +if body.get('completed') is not True or undone: print(f'::error::Shillinq first-time setup is NOT complete. Unfinished steps: {undone}') print('::error::The SPA will render a "Set up this app" dialog over every page and no spec can reach `main`.') sys.exit(1) diff --git a/tests/e2e/global-setup.ts b/tests/e2e/global-setup.ts index 2f18d732b..45577b60a 100644 --- a/tests/e2e/global-setup.ts +++ b/tests/e2e/global-setup.ts @@ -176,6 +176,48 @@ async function markWalkthroughSeen(page: Page): Promise { ) } +/** + * Stand the non-gating setup wizard down for every spec. + * + * CnAppRoot opens it whenever the server reports an OPTIONAL setup step as + * outstanding, and shillinq declares six. It renders as a modal over the + * shell, so a click on anything behind it does not fail fast — it waits out + * the full timeout. That surfaces as dozens of unrelated specs failing on + * their first interaction rather than as one cause. + * + * Seeded here, alongside the walkthrough, so the key travels with the + * storage state into every spec's context. Dismissing it reactively per + * test races the dialog's enter transition; seeding the key it reads does + * not. + * + * The key is versioned (`cn-setup-wizard-dismissed::`), + * so a RANGE is seeded: bumping manifest.setup.version must not silently + * re-open the wizard across the whole suite. + * + * @param page The authenticated page whose storage state is about to be captured. + */ +async function markSetupWizardDismissed(page: Page): Promise { + const keys = await page.evaluate(() => { + const written: string[] = [] + for (let v = 0; v <= 20; v++) { + const key = `cn-setup-wizard-dismissed:shillinq:${v}` + window.localStorage.setItem(key, '1') + written.push(key) + } + return written.filter((k) => window.localStorage.getItem(k) === '1') + }) + if (keys.length !== 21) { + throw new Error( + `Failed to seed the setup-wizard dismissal: wrote 21 keys, read back ${keys.length}. ` + + 'The setup wizard would render over the shell and intercept pointer events for the whole run.', + ) + } + // eslint-disable-next-line no-console + console.log( + '[playwright globalSetup] cn-setup-wizard-dismissed:shillinq:0..20 = 1 (setup wizard will not auto-open)', + ) +} + async function ensureNextcloudReachable(baseURL: string): Promise { const ctx = await request.newContext() try { @@ -250,6 +292,7 @@ export default async function globalSetup(config: FullConfig): Promise { // Seed the walkthrough as already-seen BEFORE the state is captured, so the // key travels with the storage state into every spec's context. await markWalkthroughSeen(page) + await markSetupWizardDismissed(page) // Persist the storage state so individual specs reuse the session. await context.storageState({ path: STORAGE_STATE }) diff --git a/tests/e2e/spec-coverage/demo-data-setup-step.spec.ts b/tests/e2e/spec-coverage/demo-data-setup-step.spec.ts new file mode 100644 index 000000000..16b774837 --- /dev/null +++ b/tests/e2e/spec-coverage/demo-data-setup-step.spec.ts @@ -0,0 +1,148 @@ +/** + * ADR-111 — the demo-data setup step, exercised against a running instance. + * + * WHY THIS EXISTS. The programme that added demo data to this app shipped a + * defect that every unit test passed: `occ openregister:descriptors:list + * --import=` printed `register "shillinq" imported.` and seeded ZERO of + * the descriptor's 30 objects. The unit tests could not see it — they mock the + * import service, so they validate the CALL and never its effect. + * + * So the assertion that matters here is not "the endpoint answers 200". It is + * that the response NAMES WHAT LANDED. A success message that cannot be told + * apart from an import that wrote nothing is exactly what let that defect + * through. + * + * WHY THE API AND NOT A CLICK-THROUGH. `CnAppRoot` opens the optional wizard + * only while an optional step is outstanding, and the CI seed deliberately + * settles those so the wizard stops covering the app in every test. The + * observable surface for this capability is therefore the contract the wizard + * calls — `GET /api/setup/status` and `POST /api/setup/action/{id}` — issued + * from inside the authenticated admin page so every call carries the real + * session and `OC.requestToken` through Nextcloud's `AuthorizedAdminSetting` + * middleware. A unit test with a mocked IAppConfig cannot show that middleware + * admitting the request; this can. + * + * WHAT THIS DELIBERATELY DOES NOT ASSERT. That `setup.steps[0]` is the + * demo-data step (ADR-111 rule 4) is a property of the manifest, which the app + * bundles rather than serves, so it is not observable from here. Gate 100 + * (`setup-demo-data-first`) checks it statically on every change. Claiming to + * prove it here would be asserting something this vantage point cannot see. + */ +import { test, expect, Page } from '@playwright/test' + +const BASE = '/apps/shillinq' + +/** One authenticated JSON call issued from inside the logged-in admin page. */ +async function api( + page: Page, + method: string, + path: string, +): Promise<{ status: number; json: any }> { + return await page.evaluate( + async ({ method, path }) => { + const res = await fetch(path, { + method, + headers: { + 'Content-Type': 'application/json', + // eslint-disable-next-line no-undef + requesttoken: (window as any).OC?.requestToken || '', + 'OCS-APIREQUEST': 'true', + }, + }) + let json: any = null + try { + json = await res.json() + } catch { + json = null + } + return { status: res.status, json } + }, + { method, path }, + ) +} + +test.describe.configure({ mode: 'serial' }) + +test.describe('ADR-111 demo data', () => { + test.beforeEach(async ({ page }) => { + await page.goto(`${BASE}/`, { waitUntil: 'domcontentloaded' }) + // The wizard's own contract lives behind the admin middleware; the page + // must be a real authenticated session before any call below. + await page.waitForFunction(() => (window as any).OC?.requestToken, null, { + timeout: 15000, + }) + }) + + test('setup status reports the demo-data step, so the wizard can offer it', async ({ + page, + }) => { + const res = await api(page, 'GET', `${BASE}/api/setup/status`) + + expect(res.status, 'setup/status must answer an authenticated admin').toBe( + 200, + ) + + // A step the endpoint never MENTIONS resolves to `done: false` forever — + // no operator action can clear it, and CnAppRoot then covers the app with + // the wizard in every fresh browser context. Absence is the defect here, + // not "not done". + expect( + Object.keys(res.json?.steps ?? {}), + 'setup/status must report a demo-data step', + ).toContain('demo-data') + }) + + test('installing the demo data reports HOW MUCH landed, not just success', async ({ + page, + }) => { + // 🔴 A REAL IMPORT, NOT A STUB. The largest app seeds 1497 objects; the + // call took 49.6s on one run and exceeded the 60s default on the next, so + // it sits on the boundary. The assertion is worth its cost: it is the only + // check that the install WROTE something. + test.slow() + + const res = await api( + page, + 'POST', + `${BASE}/api/setup/action/install-demo-data`, + ) + + expect(res.status, 'the action must pass the admin middleware').toBe(200) + expect( + res.json?.success, + `install failed: ${JSON.stringify(res.json)}`, + ).toBe(true) + + // 🔴 THE COUNTS ARE THE ASSERTION. "Demo data installed" with no numbers + // is indistinguishable from an import that wrote nothing — the exact + // defect this programme shipped in openregister and had to fix. A message + // carrying a positive object count is the only evidence that the demo + // data reached the instance. + const message = String(res.json?.message ?? '') + const numbers = (message.match(/\d+/g) ?? []).map(Number) + + expect( + numbers.some((n) => n > 0), + `the install message must name a non-zero object count; got: "${message}"`, + ).toBe(true) + }) + + test('re-installing is safe, because the step promises it is', async ({ + page, + }) => { + // The step body tells the operator it is "safe to run more than once". + // That sentence is a contract; this asserts the server keeps it rather + // than erroring or reporting failure on a second pass. + const again = await api( + page, + 'POST', + `${BASE}/api/setup/action/install-demo-data`, + ) + + expect(again.status).toBe(200) + expect( + again.json?.success, + `a second install must not fail: ${JSON.stringify(again.json)}`, + ).toBe(true) + }) +}) diff --git a/tests/validate-manifest.js b/tests/validate-manifest.js index ab23c0224..38bf78693 100644 --- a/tests/validate-manifest.js +++ b/tests/validate-manifest.js @@ -50,11 +50,21 @@ const SCHEMA_BASENAME = (() => { } })() +// THE INSTALLED SCHEMA WINS OVER THE VENDORED COPY. +// +// It used to be the other way round, and that ordering is a drift machine: the +// vendored copy shadows the one the shipped runtime actually enforces, so the +// check keeps passing against a snapshot of the rules while the rules move. +// Measured here — the vendored copy sat at 2.25.0 and rejected `type: "flow"`, +// which the installed package's schema (2.26.0) accepts and the runtime +// registers. The manifest was right and the check was reading an older grammar. +// +// A manifest has to satisfy the schema its own dependency ships. The vendored +// copy stays as a fallback for a tree with no node_modules — a fresh checkout, +// a CI leg that skips install — but it no longer gets to overrule what is +// installed. const SCHEMA_CANDIDATES = [ process.env.APP_MANIFEST_SCHEMA, - // Vendored canonical v2 schema (includes the setup block + metric cacheTtl); - // preferred so the gate does not depend on a fresh node_modules copy. - path.join(REPO_ROOT, 'tests', 'schemas', 'app-manifest-v2.schema.json'), path.join( REPO_ROOT, 'node_modules', @@ -64,6 +74,7 @@ const SCHEMA_CANDIDATES = [ 'schemas', SCHEMA_BASENAME, ), + path.join(REPO_ROOT, 'tests', 'schemas', 'app-manifest-v2.schema.json'), path.join(REPO_ROOT, '..', 'nextcloud-vue', 'src', 'schemas', SCHEMA_BASENAME), ].filter(Boolean) diff --git a/tests/validate-registers.js b/tests/validate-registers.js index 1c96e7199..cebda868c 100644 --- a/tests/validate-registers.js +++ b/tests/validate-registers.js @@ -543,17 +543,13 @@ function groupByRefs(agg) { // here (rather than deleting the check) is that the gate keeps protecting // every OTHER reference while these stay visible. // -// `GLLine.fiscalYearId`: GLLine has no fiscal-year property at all. Its -// nearest field is `periodId`, but a period is a FINER grain than a year, -// so substituting it would silently change what these P&L roll-ups group -// by — an architecture decision (add a fiscalYearId to GLLine, or join -// through GLTransaction/Period to derive the year), not a rename. -const AGGREGATION_REF_BASELINE = new Map([ - [ - 'GLLine.fiscalYearId', - 'GLLine declares no fiscal-year field; periodId is a finer grain, so this needs a schema decision, not a rename. Affects AnalyticalDimension.segmentPnl, AnalyticalDimension.segmentPnlByCostObject, Project.segmentPnl.', - ], -]) +// `GLLine.fiscalYearId` was here, waived, with the note that it needed a +// schema decision rather than a rename. The decision was taken: GLLine now +// DECLARES `fiscalYearId`, denormalised from the parent GLTransaction and +// backfilled by BackfillGlLineFiscalYear. `periodId` was deliberately not +// substituted — a period is a finer grain than a year, so it would have +// silently changed what the three P&L roll-ups group by. +const AGGREGATION_REF_BASELINE = new Map([]) // An aggregation without `metric`/`metrics` cannot produce a value. // @@ -585,7 +581,12 @@ const AGGREGATION_REF_BASELINE = new Map([ // runner into its cross-schema path, which needs a parent row and behaves // differently. Those are a redesign, not a rename, and they are the GL/tax ones // where a wrong number matters most. -const AGG_NO_METRIC_BASELINE = 219 +// 219 -> 216: the segment-P&L batch (GLLine byCostCenter / byCostObject / +// byProject). Each declared `source: "GLLine"` on GLLine itself — redundant, +// and NOT a `from`, which would have switched the runner into its cross-schema +// path — plus `sum: ["amount"]`, which is not an engine key. Verified live +// against the rows, not just for a non-empty response. +const AGG_NO_METRIC_BASELINE = 137 // A STRING `groupBy` is silently ignored, and the result is a WRONG NUMBER. // @@ -709,7 +710,7 @@ const AGG_PLACEHOLDER_TENANT_KEYS = new Set(['administrationId', 'organisationId // Measured 2026-08-26 by this check, after removing 67 tenant placeholders // across 22 files. Counted BY THE GATE, not by a one-off script — an earlier // estimate of 73 came from a narrower hand-written predicate and was wrong. -const AGG_PLACEHOLDER_BASELINE = 84 +const AGG_PLACEHOLDER_BASELINE = 62 function collectPlaceholders(node, path, out) { if (node === null || node === undefined) return @@ -734,6 +735,22 @@ function checkAggregationPlaceholders(registry) { for (const slug of Object.keys(registry).sort()) { for (const { aggName, agg, file } of registry[slug].aggregations) { + // `where` IS scanned on a cross-schema spec too. + // + // This check briefly exempted it, on the reasoning that + // runCrossSchema() substitutes `@self.` against the parent + // row and so the spelling works there. It resolves it, but nothing + // SUPPLIES a parent row: AggregationController, ReportRenderService + // and ThresholdEvaluationService all call AggregationRunner::run() + // without one, so `@self.code` became null — and a null is applied + // as a real filter VALUE, returning the target rows whose own field + // is null. For a segment P&L that is the unassigned total, handed + // back confidently for every parent record. + // + // OpenRegister now raises on an unresolvable `@self` rather than + // filtering on null, which makes these declarations loudly broken + // instead of quietly wrong. Either way they are defects, so the + // exemption is gone. const found = [] for (const key of ['filter', 'where', 'join', 'match']) { collectPlaceholders(agg[key], key, found) @@ -796,6 +813,189 @@ function checkAggregationPlaceholders(registry) { return ok } +// The BARE half of the same subject. +// +// checkAggregationFieldRefs() validates only `Schema.field` refs — it opens +// with `if (dot === -1) continue`, so a bare `fiscalYearId` was never looked +// at. That is the larger half: a bare groupBy on a property the target schema +// does not declare groups every row into ONE null bucket, and a bare filter +// key that resolves to nothing narrows to zero rows. Both answer HTTP 200 with +// a plausible figure, which is why none of them ever surfaced as a bug. +// +// The bare form was skipped because "the source schema would need resolving, +// which source/sourceSchema spell inconsistently". The engine settles it: +// AggregationRunner reads `from` and nothing else — `source` and +// `sourceSchema` are inert keys it never consults. So the target is `from` +// when present and the declaring schema otherwise, exactly as the runner +// computes it, and the ambiguity that justified skipping this is gone. +// 100 of the bare references checked resolve to nothing today. They are +// NOT waived — each returns a plausible figure (one null bucket, or zero rows) +// under HTTP 200, which is why the class went unnoticed. The ratchet keeps the +// number falling and refuses any new one. Classified in #1261; the bulk are +// declarations carrying the inert `source` key that MEANT another schema and +// therefore resolve their fields against the declaring schema instead. +const AGG_BARE_REF_BASELINE = 100 + +// A derived metric that names an alias which does not exist. +// +// `{"metric":"expression","expression":"a - b"}` reads the aliases of the +// metrics BESIDE it. OpenRegister raises when one is missing — deliberately, +// because resolving it to 0 would turn a typo into a plausible number. That +// raise happens at RUN time, on a dashboard, in front of whoever opened it. +// +// This catches it at declaration time. It matters more than it looks: the +// `operations` maps these were translated from keyed their entries one way and +// named their `target` another, so an expression written against the map KEY +// silently referred to an alias the translated metrics list does not have. A +// first pass of that translation produced 41 such references across 13 +// aggregations, every one of which would have raised on first use. +// +// Order matters and is checked: an expression may only name aliases declared +// BEFORE it, because that is the order the engine computes them in. +function checkAggregationExpressionAliases(registry) { + const offenders = [] + let checked = 0 + + for (const slug of Object.keys(registry).sort()) { + for (const { aggName, agg, file } of registry[slug].aggregations) { + if (Array.isArray(agg.metrics) === false) continue + + const declared = new Set() + for (const metric of agg.metrics) { + if (metric === null || typeof metric !== 'object') continue + + if (metric.metric === 'expression') { + checked++ + const expression = String(metric.expression ?? '') + if (expression.trim() === '') { + offenders.push( + `${slug}.${aggName} declares a derived metric with an empty expression` + + `\n declared in ${file}`, + ) + } + for (const ident of expression.match(/[A-Za-z_]\w*/g) || []) { + // `min`/`max` are the only functions the grammar has. + if (ident === 'min' || ident === 'max') continue + if (declared.has(ident) === false) { + offenders.push( + `${slug}.${aggName} expression "${expression}" names "${ident}", which is not ` + + `a metric declared before it (available: ${[...declared].join(', ') || 'none'})` + + `\n declared in ${file}`, + ) + } + } + } + + const alias = metric.as + if (typeof alias === 'string' && alias !== '') declared.add(alias) + } + } + } + + console.log( + `[validate-registers] derived-metric expressions checked: ${checked}`, + ) + if (offenders.length === 0) { + console.log( + '[validate-registers] PASS — every derived metric names only aliases declared before it', + ) + return true + } + console.error( + '[validate-registers] FAIL — derived metrics naming an alias that does not exist. ' + + 'OpenRegister raises on these at run time, on whatever dashboard opened them:', + ) + for (const o of offenders) console.error(` - ${o}`) + return false +} + +function checkAggregationBareRefs(registry) { + const offenders = [] + let checked = 0 + + for (const slug of Object.keys(registry).sort()) { + for (const { aggName, agg, file } of registry[slug].aggregations) { + // Resolve the target the way AggregationRunner does. + const targetSlug = + typeof agg.from === 'string' && agg.from !== '' ? agg.from : slug + const target = registry[targetSlug] + // An unresolvable target is a different defect class — and may + // legitimately live in another app's register. + if (!target) continue + + const refs = [] + const gb = Array.isArray(agg.groupBy) + ? agg.groupBy + : typeof agg.groupBy === 'string' + ? [agg.groupBy] + : [] + for (const g of gb) + if (typeof g === 'string') + refs.push({ key: 'groupBy', ref: g.trim() }) + for (const src of ['filter', 'where']) { + const v = agg[src] + if (v && typeof v === 'object' && Array.isArray(v) === false) + for (const k of Object.keys(v)) + refs.push({ key: src, ref: k.trim() }) + } + if (typeof agg.field === 'string' && agg.field !== '') + refs.push({ key: 'field', ref: agg.field.trim() }) + if (Array.isArray(agg.metrics)) + for (const m of agg.metrics) + if (m && typeof m.field === 'string' && m.field !== '') + refs.push({ key: 'metrics[].field', ref: m.field.trim() }) + + for (const { key, ref } of refs) { + // Dotted refs are checkAggregationFieldRefs()' subject. + if (ref.includes('.') === true) continue + // `_`-prefixed keys are OpenRegister control params, not + // properties; `@`-prefixed values are placeholders. + if (ref.startsWith('_') === true || ref.startsWith('@') === true) + continue + if (IMPLICIT_OBJECT_FIELDS.has(ref) === true) continue + checked++ + if (target.props.has(ref) === false) + offenders.push( + `${slug}.${aggName} ${key}="${ref}" — ${targetSlug} declares no such property` + + `\n declared in ${file}`, + ) + } + } + } + + console.log( + `[validate-registers] aggregation BARE field references checked: ${checked}`, + ) + // A check that examined nothing must not report success. + if (checked === 0) { + console.error( + '[validate-registers] FAIL — the bare aggregation reference check resolved ZERO references. ' + + 'That means it stopped seeing its own subject, not that the registers are clean.', + ) + return false + } + console.log( + `[validate-registers] aggregation bare refs that resolve to nothing: ${offenders.length} ` + + `(baseline ${AGG_BARE_REF_BASELINE}) — see #1261`, + ) + if (offenders.length > AGG_BARE_REF_BASELINE) { + console.error( + '[validate-registers] FAIL — bare aggregation references that cannot resolve: ' + + `${offenders.length}, above the baseline of ${AGG_BARE_REF_BASELINE}.`, + ) + for (const o of offenders) console.error(` - ${o}`) + return false + } + if (offenders.length < AGG_BARE_REF_BASELINE) { + console.error( + `[validate-registers] ${AGG_BARE_REF_BASELINE - offenders.length} better than baseline — ` + + `please lower AGG_BARE_REF_BASELINE to ${offenders.length}.`, + ) + return false + } + return true +} + function checkAggregationFieldRefs(registry) { const problems = [] const baselined = [] @@ -911,6 +1111,8 @@ function main() { const slugsOk = checkSlugCaseCollisions(registry) const sameSlugFullDefinitionOk = checkSameSlugFullDefinitionCollisions(registry) const aggregationRefsOk = checkAggregationFieldRefs(registry) + const aggregationBareRefsOk = checkAggregationBareRefs(registry) + const aggregationExpressionsOk = checkAggregationExpressionAliases(registry) const aggregationPlaceholdersOk = checkAggregationPlaceholders(registry) const aggregationMetricsOk = checkAggregationMetrics(registry) const aggregationGroupByOk = checkAggregationGroupByShape(registry) @@ -920,6 +1122,8 @@ function main() { slugsOk === false || sameSlugFullDefinitionOk === false || aggregationRefsOk === false + || aggregationBareRefsOk === false + || aggregationExpressionsOk === false || aggregationPlaceholdersOk === false || aggregationMetricsOk === false || aggregationGroupByOk === false diff --git a/tests/validate-seeds.js b/tests/validate-seeds.js index 4d5793796..8778472d7 100644 --- a/tests/validate-seeds.js +++ b/tests/validate-seeds.js @@ -159,7 +159,21 @@ function main() { } } - const schemas = (merged.components && merged.components.schemas) || {} + // BOTH shapes, exactly as the seed collector above reads both object + // buckets. ImportHandler merges `schemas` and `components.schemas` alike, + // and app registers use either — dossiq declares `stufEndpoint`, + // `stufMessage` and `zaaksysteemMapping` at the top level. + // + // Reading only `components.schemas` made this gate narrower than the set it + // collects, in both directions: a seed against a top-level schema was + // reported as naming a schema "no fragment declares" (a false alarm), and + // its `required` list was never checked at all (a false pass — the more + // expensive half, because this gate is a ratchet and an unseen class + // silently lowers the count it guards). + const schemas = { + ...((merged.components && merged.components.schemas) || {}), + ...(merged.schemas || {}), + } const offenders = new Map() let checked = 0 let unknownSchema = 0 diff --git a/webpack.config.js b/webpack.config.js index 70b7db002..29012fe90 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -102,7 +102,16 @@ webpackConfig.resolve = { // Deduplicate shared packages so the aliased library source uses // the same instances as the app (prevents dual-Pinia / dual-Vue bugs). vue$: path.resolve(__dirname, 'node_modules/vue'), - pinia$: path.resolve(__dirname, 'node_modules/pinia'), + // pinia@4 dropped `main` AND `module`, leaving only an `exports` map + // (`{".": "./dist/pinia.js"}`) — the same shape as @nextcloud/vue and + // @nextcloud/dialogs below. pinia@3 still had `main: index.cjs` / + // `module: dist/pinia.mjs`, which is why the Vue-2-era alias to the + // package DIRECTORY worked until now: a directory alias bypasses + // `exports` entirely and then looks for a main/index.js that no longer + // exists, so every `import { … } from 'pinia'` failed with + // "Can't resolve 'pinia'". Alias to the absolute FILE; the `$` + // (exact-match) form keeps deep imports going through the exports map. + pinia$: path.resolve(__dirname, 'node_modules/pinia/dist/pinia.js'), // @nextcloud/vue@9, @nextcloud/dialogs@7 and vue-router@5 are ESM-only: // their package.json has NO `main` and NO `module`, only an `exports` // map. A Vue-2-era alias to the package DIRECTORY bypasses `exports`