From 14b3aeb5a8af2e25e0eebcc9da20732255627c92 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 16 May 2026 16:04:43 +0200 Subject: [PATCH 1/4] fix(wizard): namespace manifest config.schema alongside config.register (closes #75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `substituteRegisterSlug` only rewrote `config.register` to the per-version slug. The seeded `config.schema` value (`hello-message`) was left bare even though `provisionRegister` namespaces every seed schema as `{appSlug}-{versionSlug}-{originalSlug}`. Net effect: the manifest pointed at a schema slug that didn't exist in the per-version register, so the insights service couldn't resolve the schema-set and every tier's KPI cards read the same global aggregate (openbuilt#75 — all 3 pills showed `Object count: 246` regardless of selected tier). Generalise to `substituteVersionContext(manifest, registerSlug, schemaSlugPrefix)`. When the prefix is non-empty, every non-empty `config.schema` that doesn't already start with the prefix is rewritten to `{prefix}{originalSchemaSlug}`. The original `substituteRegisterSlug` entry point now delegates with an empty prefix (preserves existing unit tests / external callers). Also backfill the cascading test regressions from PR #74 / #76: - tests/store/schemas.spec.js — `registerObjectType` signature changed from 3-arg `(type, 'schemas', register)` to 4-arg with slugs map (`'api', { registerSlug: ... }`) when the schema store was retargeted at OR's `/api/schemas` CRUD. - tests/views/SchemaDesigner.spec.js — fixture slug needs the `{appSlug}-` prefix to survive the new client-side filter. - tests/Unit/Controller/ApplicationsControllerTest.php + tests/Unit/Controller/ApplicationsControllerDiffTest.php — add the `manifestResolver` constructor arg. - tests/Unit/Repair/MigrateToVersionedModelTest.php — rewrite the short-circuit test to match the new row-shape detection (the old "schema-exists" probe was the very bug fixed in #69). Verified: PHPUnit 204/204, Vitest 487/487. --- lib/Service/ApplicationCreationService.php | 45 +++++++++- .../Repair/MigrateToVersionedModelTest.php | 35 +++++++- .../ApplicationCreationServiceTest.php | 88 +++++++++++++++++++ tests/store/schemas.spec.js | 22 +++-- .../ApplicationsControllerDiffTest.php | 2 + .../Controller/ApplicationsControllerTest.php | 2 + tests/views/SchemaDesigner.spec.js | 13 ++- 7 files changed, 191 insertions(+), 16 deletions(-) diff --git a/lib/Service/ApplicationCreationService.php b/lib/Service/ApplicationCreationService.php index 4ff2707e..67b069f4 100644 --- a/lib/Service/ApplicationCreationService.php +++ b/lib/Service/ApplicationCreationService.php @@ -207,9 +207,10 @@ public function createApplication(array $payload): string $registerSlug = 'openbuilt-'.$appSlug.'-'.$versionSlug; // 3a: Create ApplicationVersion - $versionManifest = $this->substituteRegisterSlug( + $versionManifest = $this->substituteVersionContext( manifest: $defaultManifest, - registerSlug: $registerSlug + registerSlug: $registerSlug, + schemaSlugPrefix: $appSlug.'-'.$versionSlug.'-' ); $versionPayload = [ @@ -815,6 +816,35 @@ private function loadDefaultSchemas(): array */ public function substituteRegisterSlug(array $manifest, string $registerSlug): array { + return $this->substituteVersionContext( + manifest: $manifest, + registerSlug: $registerSlug, + schemaSlugPrefix: '' + ); + }//end substituteRegisterSlug() + + /** + * Substitute the per-version context tokens in a manifest template. + * + * Walks all `pages[*].config` blocks. Replaces the `{registerSlug}` + * token in `register` and rewrites every non-empty `config.schema` + * to the namespaced seed slug `{schemaSlugPrefix}{originalSchemaSlug}` + * so the manifest references the actual per-version schemas created + * by {@see provisionRegister()} (openbuilt#75 — without this the + * KPI / insights cards aggregated against a non-existent schema and + * leaked the same numbers across all tiers). + * + * @param array $manifest The manifest template blob + * @param string $registerSlug The per-version register slug + * @param string $schemaSlugPrefix Namespaced prefix for schema slugs (e.g. `permit-flow-development-`) + * + * @return array The manifest with tokens substituted + */ + public function substituteVersionContext( + array $manifest, + string $registerSlug, + string $schemaSlugPrefix + ): array { if (isset($manifest['pages']) === false || is_array($manifest['pages']) === false) { return $manifest; } @@ -831,11 +861,20 @@ public function substituteRegisterSlug(array $manifest, string $registerSlug): a if (isset($page['config']['register']) === true && $page['config']['register'] === '{registerSlug}') { $page['config']['register'] = $registerSlug; } + + if ($schemaSlugPrefix !== '' + && isset($page['config']['schema']) === true + && is_string($page['config']['schema']) === true + && $page['config']['schema'] !== '' + && str_starts_with($page['config']['schema'], $schemaSlugPrefix) === false + ) { + $page['config']['schema'] = $schemaSlugPrefix.$page['config']['schema']; + } } unset($page); return $manifest; - }//end substituteRegisterSlug() + }//end substituteVersionContext() /** * Get the UID of the currently authenticated user. diff --git a/tests/Unit/Repair/MigrateToVersionedModelTest.php b/tests/Unit/Repair/MigrateToVersionedModelTest.php index 9bcbe4ba..c3f2fc45 100644 --- a/tests/Unit/Repair/MigrateToVersionedModelTest.php +++ b/tests/Unit/Repair/MigrateToVersionedModelTest.php @@ -121,16 +121,43 @@ private function step(): MigrateToVersionedModel }//end step() /** - * Short-circuit: versioned schema already present → no deletions. + * Short-circuit: no Application rows carry pre-spec-C shape → no deletions. + * + * The previous check probed for the `applicationVersion` schema's + * existence, but `InitializeSettings` imports that schema BEFORE + * this step runs, so the probe always fired true and the migration + * was skipped (openbuilt#69). The new check inspects Application + * row shape and only proceeds when at least one row still carries + * a legacy `manifest` / `version` / `status` / `currentVersion` + * top-level field. * * @return void */ public function testShortCircuitsWhenVersionedSchemaPresent(): void { - // schemaMapper->find('applicationVersion', ...) succeeds (no throw) on first call. - $this->schemaMapper->method('find')->willReturn($this->createMock(Schema::class)); + // Register + schema lookups succeed. + $register = $this->getMockBuilder(Register::class) + ->disableOriginalConstructor() + ->addMethods(['getId']) + ->getMock(); + $register->method('getId')->willReturn(1); + $this->registerMapper->method('find')->willReturn($register); + + $this->schemaMapper->method('find')->willReturn( + $this->getMockBuilder(Schema::class) + ->disableOriginalConstructor() + ->addMethods(['getId']) + ->getMock() + ); + + // Application rows already match the post-C shape (no legacy keys). + $postCRows = [ + $this->mockEntity(['id' => 'uuid-a', 'slug' => 'app-a', 'name' => 'App A']), + ]; + $this->objectService->method('findAll')->willReturn($postCRows); - $this->objectService->expects(self::never())->method('findAll'); + // No deletions should fire: rows look post-C so the short-circuit + // returns true and `run()` exits before touching either service. $this->objectService->expects(self::never())->method('deleteObject'); $this->registerService->expects(self::never())->method('delete'); diff --git a/tests/Unit/Service/ApplicationCreationServiceTest.php b/tests/Unit/Service/ApplicationCreationServiceTest.php index cf494d33..b214abda 100644 --- a/tests/Unit/Service/ApplicationCreationServiceTest.php +++ b/tests/Unit/Service/ApplicationCreationServiceTest.php @@ -171,6 +171,94 @@ public function substituteRegisterSlugDoesNotTouchNonTokenFields(): void self::assertSame('some-other-register', $result['pages'][0]['config']['register']); }//end substituteRegisterSlugDoesNotTouchNonTokenFields() + /** + * substituteVersionContext rewrites both register AND schema. + * + * openbuilt#75 — without this the KPI / insights cards aggregate + * against `hello-message` (the un-namespaced template slug), which + * doesn't exist in the per-version register, so counts leak the + * same numbers across all tiers. + * + * @test + * + * @return void + */ + public function substituteVersionContextNamespacesSchemaSlugAlongsideRegister(): void + { + $manifest = [ + 'pages' => [ + ['id' => 'Dashboard', 'config' => ['widgets' => []]], + ['id' => 'Messages', 'config' => ['register' => '{registerSlug}', 'schema' => 'hello-message']], + ], + ]; + + $result = $this->service->substituteVersionContext( + manifest: $manifest, + registerSlug: 'openbuilt-permit-flow-development', + schemaSlugPrefix: 'permit-flow-development-' + ); + + self::assertSame('openbuilt-permit-flow-development', $result['pages'][1]['config']['register']); + self::assertSame('permit-flow-development-hello-message', $result['pages'][1]['config']['schema']); + }//end substituteVersionContextNamespacesSchemaSlugAlongsideRegister() + + /** + * substituteVersionContext is idempotent: re-running with the same + * prefix MUST NOT double-prefix the schema slug. + * + * @test + * + * @return void + */ + public function substituteVersionContextIsIdempotentOnAlreadyNamespacedSlugs(): void + { + $manifest = [ + 'pages' => [ + [ + 'id' => 'Messages', + 'config' => [ + 'register' => 'openbuilt-permit-flow-development', + 'schema' => 'permit-flow-development-hello-message', + ], + ], + ], + ]; + + $result = $this->service->substituteVersionContext( + manifest: $manifest, + registerSlug: 'openbuilt-permit-flow-development', + schemaSlugPrefix: 'permit-flow-development-' + ); + + // Schema must NOT become `permit-flow-development-permit-flow-development-hello-message`. + self::assertSame('permit-flow-development-hello-message', $result['pages'][0]['config']['schema']); + }//end substituteVersionContextIsIdempotentOnAlreadyNamespacedSlugs() + + /** + * substituteVersionContext leaves schema alone when no prefix is provided + * (backwards-compat with the legacy substituteRegisterSlug entry point). + * + * @test + * + * @return void + */ + public function substituteVersionContextLeavesSchemaAloneWhenPrefixEmpty(): void + { + $manifest = [ + 'pages' => [ + ['id' => 'Messages', 'config' => ['register' => '{registerSlug}', 'schema' => 'hello-message']], + ], + ]; + + $result = $this->service->substituteVersionContext( + manifest: $manifest, + registerSlug: 'openbuilt-my-app-production', + schemaSlugPrefix: '' + ); + + self::assertSame('hello-message', $result['pages'][0]['config']['schema']); + }//end substituteVersionContextLeavesSchemaAloneWhenPrefixEmpty() + // ------------------------------------------------------------------------- // resolveVersionChain // ------------------------------------------------------------------------- diff --git a/tests/store/schemas.spec.js b/tests/store/schemas.spec.js index 2a49e6ad..8e5b8074 100644 --- a/tests/store/schemas.spec.js +++ b/tests/store/schemas.spec.js @@ -76,7 +76,8 @@ describe('useSchemasStore — re-registers when register changes', () => { expect(registerObjectType).toHaveBeenCalledWith( 'schema', 'schemas', - 'openbuilt-hello-world-staging', + 'api', + { registerSlug: 'openbuilt-hello-world-staging' }, ) }) @@ -100,7 +101,8 @@ describe('useSchemasStore — re-registers when register changes', () => { expect(registerObjectType).toHaveBeenCalledWith( 'schema', 'schemas', - 'openbuilt-hello-world', + 'api', + { registerSlug: 'openbuilt-hello-world' }, ) }) @@ -122,14 +124,24 @@ describe('useSchemasStore — re-registers when register changes', () => { // First call — registers to staging useSchemasStore('hello-world', 'staging') expect(registerObjectType).toHaveBeenCalledTimes(1) - expect(registerObjectType).toHaveBeenLastCalledWith('schema', 'schemas', 'openbuilt-hello-world-staging') + expect(registerObjectType).toHaveBeenLastCalledWith( + 'schema', + 'schemas', + 'api', + { registerSlug: 'openbuilt-hello-world-staging' }, + ) // Simulate version change — objectTypeRegistry now has the old register recorded - fakeStore.objectTypeRegistry.schema = { register: 'openbuilt-hello-world-staging' } + fakeStore.objectTypeRegistry.schema = { slugs: { registerSlug: 'openbuilt-hello-world-staging' } } // Second call with different version — should re-register useSchemasStore('hello-world', 'production') expect(registerObjectType).toHaveBeenCalledTimes(2) - expect(registerObjectType).toHaveBeenLastCalledWith('schema', 'schemas', 'openbuilt-hello-world-production') + expect(registerObjectType).toHaveBeenLastCalledWith( + 'schema', + 'schemas', + 'api', + { registerSlug: 'openbuilt-hello-world-production' }, + ) }) }) diff --git a/tests/unit/Controller/ApplicationsControllerDiffTest.php b/tests/unit/Controller/ApplicationsControllerDiffTest.php index 8b8ad757..b60d8e6a 100644 --- a/tests/unit/Controller/ApplicationsControllerDiffTest.php +++ b/tests/unit/Controller/ApplicationsControllerDiffTest.php @@ -29,6 +29,7 @@ namespace OCA\OpenBuilt\Tests\Unit\Controller; use OCA\OpenBuilt\Controller\ApplicationsController; +use OCA\OpenBuilt\Service\ManifestResolverService; use OCA\OpenRegister\Db\ObjectEntity; use OCA\OpenRegister\Db\Register; use OCA\OpenRegister\Db\RegisterMapper; @@ -119,6 +120,7 @@ protected function setUp(): void schemaMapper: $schemaMapper, userSession: $userSession, groupManager: $groupManager, + manifestResolver: $this->createMock(ManifestResolverService::class), ); }//end setUp() diff --git a/tests/unit/Controller/ApplicationsControllerTest.php b/tests/unit/Controller/ApplicationsControllerTest.php index 0ee79927..af6ceff3 100644 --- a/tests/unit/Controller/ApplicationsControllerTest.php +++ b/tests/unit/Controller/ApplicationsControllerTest.php @@ -23,6 +23,7 @@ namespace OCA\OpenBuilt\Tests\Unit\Controller; use OCA\OpenBuilt\Controller\ApplicationsController; +use OCA\OpenBuilt\Service\ManifestResolverService; use OCA\OpenRegister\Db\AuditTrailMapper; use OCA\OpenRegister\Db\ObjectEntity; use OCA\OpenRegister\Db\Register; @@ -152,6 +153,7 @@ static function (string $callerUid, string $gid) use ($uid, $isAdmin): bool { schemaMapper: $schemaMapper, userSession: $this->userSession, groupManager: $this->groupManager, + manifestResolver: $this->createMock(ManifestResolverService::class), auditTrailMapper: $this->auditTrailMapper, ); }//end buildController() diff --git a/tests/views/SchemaDesigner.spec.js b/tests/views/SchemaDesigner.spec.js index 6dd7e9ff..8f24c823 100644 --- a/tests/views/SchemaDesigner.spec.js +++ b/tests/views/SchemaDesigner.spec.js @@ -116,7 +116,12 @@ function makeRouter({ slug = 'hello-world', schemaId = '', version = undefined } // equivalent but key-ordered-different JSON string and the // `JSON.stringify` diff in the SUT (correctly) reports a change. const persistedSchema = { - slug: 'hello', + // Schema slugs are namespaced `{appSlug}-{versionSlug}-{slug}` since + // PR #74 — the SchemaDesigner filters the org-wide schema collection + // down to ones owned by the active app+version. The default route is + // `hello-world` with no `_version`, so the fixture must carry the + // `hello-world-` prefix to survive the client-side filter. + slug: 'hello-world-hello', title: 'Hello', description: '', version: '0.1.0', @@ -150,7 +155,7 @@ describe('SchemaDesigner', () => { await wrapper.vm.$nextTick() expect(storeMocks.fetchCollection).toHaveBeenCalledWith('schema') expect(wrapper.vm.schemas).toHaveLength(1) - expect(wrapper.vm.schemas[0].slug).toBe('hello') + expect(wrapper.vm.schemas[0].slug).toBe('hello-world-hello') }) it('REQ-OBSD-001: surfaces a showError toast when the store reports a list error', async () => { @@ -231,7 +236,7 @@ describe('SchemaDesigner', () => { await wrapper.vm.$nextTick() expect(storeMocks.fetchObject).toHaveBeenCalledWith('schema', 'hello') expect(wrapper.vm.staged).toBeTruthy() - expect(wrapper.vm.staged.slug).toBe('hello') + expect(wrapper.vm.staged.slug).toBe('hello-world-hello') expect(wrapper.vm.staged.fields).toHaveLength(1) expect(wrapper.vm.staged.fields[0].name).toBe('subject') }) @@ -296,7 +301,7 @@ describe('SchemaDesigner', () => { expect(type).toBe('schema') expect(body).toMatchObject({ id: 'hello', - slug: 'hello', + slug: 'hello-world-hello', title: 'Hello renamed', version: '0.2.0', type: 'object', From 52df64f3ff8724789aee9129d4814d7d48d9a513 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 16 May 2026 17:04:29 +0200 Subject: [PATCH 2/4] fix(e2e): remove global OCS-APIRequest header + accept pretty-URL redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unrelated fixes to the Playwright config and login helpers that together unblock every spec from the Playwright baseline (24/26 failed → login itself worked but post-login waits/asserts timed out). 1. Drop `OCS-APIRequest: true` from `use.extraHTTPHeaders`. The header makes Nextcloud treat every browser request as an API call — including the HTML page loads — so the login form's redirect goes out as an empty JSON response with no Location header. Result: the browser stays on `/login` and every `waitForURL(/\/apps\//)` after login times out. Specs that need the header on their API calls (versionRouting, applicationDetailOverview, etc.) already set it explicitly on `request.fetch`; the global was redundant. 2. Relax the post-login `waitForURL` regex from `/index\.php\/apps\//` to `/\/apps\//` in template-gallery.spec.ts + export-zip.spec.ts. Pretty-URL rewrites in modern NC drop the `/index.php/` prefix on redirects. Net effect locally: full Playwright run now drives every spec to its content assertions instead of dying in `beforeEach`. Remaining failures are real test-logic regressions (e.g. `.template-gallery` selector mismatch) that this PR doesn't touch — they're tracked separately. --- playwright.config.ts | 10 ++++++---- tests/e2e/export-zip.spec.ts | 3 ++- tests/e2e/template-gallery.spec.ts | 5 ++++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/playwright.config.ts b/playwright.config.ts index d3516d7a..f0968c06 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -29,10 +29,12 @@ export default defineConfig({ username: process.env.NC_ADMIN_USER || 'admin', password: process.env.NC_ADMIN_PASSWORD || 'admin', }, - extraHTTPHeaders: { - // Nextcloud rejects unauthenticated OCS/API calls without this header. - 'OCS-APIRequest': 'true', - }, + // Note: do NOT set `OCS-APIRequest: true` here globally. The header + // makes Nextcloud treat every request as an API call — including the + // HTML page loads — which breaks the browser-based login redirect + // (no Location header is emitted, the page stays on /login). + // Specs that need OCS-APIRequest set it on their explicit `request` + // calls (e.g. versionRouting.spec.ts, applicationDetailOverview.spec.ts). trace: 'on-first-retry', screenshot: 'only-on-failure', video: 'retain-on-failure', diff --git a/tests/e2e/export-zip.spec.ts b/tests/e2e/export-zip.spec.ts index 207f5d3f..d7efb0f0 100644 --- a/tests/e2e/export-zip.spec.ts +++ b/tests/e2e/export-zip.spec.ts @@ -41,7 +41,8 @@ test.describe('OpenBuilt ZIP export', () => { await page.fill('input[name="user"]', ADMIN_USER) await page.fill('input[name="password"]', ADMIN_PASSWORD) await page.locator('button[type="submit"], input[type="submit"]').first().click() - await page.waitForURL(/\/index\.php\/apps\//, { timeout: 15_000 }) + // Pretty URLs in modern NC drop the `/index.php` prefix; accept both. + await page.waitForURL(/\/apps\//, { timeout: 15_000 }) } }) diff --git a/tests/e2e/template-gallery.spec.ts b/tests/e2e/template-gallery.spec.ts index b7495e49..406d2079 100644 --- a/tests/e2e/template-gallery.spec.ts +++ b/tests/e2e/template-gallery.spec.ts @@ -36,7 +36,10 @@ async function loginAsAdmin(page: Page): Promise { await page.locator('input[name="user"]').fill(ADMIN_USER) await page.locator('input[name="password"]').fill(ADMIN_PASS) await page.locator('button[type="submit"]').click() - await page.waitForURL(/index\.php\/apps\//, { timeout: 15_000 }) + // Accept both pretty URLs (`/apps/dashboard/`) and the legacy form + // (`/index.php/apps/dashboard/`) — pretty-URL rewrites are enabled in + // modern NC installs and the redirect drops the `/index.php` prefix. + await page.waitForURL(/\/apps\//, { timeout: 15_000 }) } test.describe('OpenBuilt template gallery', () => { From f16c85483ba8a1671b211db3c1ab3ad616529f2e Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 16 May 2026 17:07:59 +0200 Subject: [PATCH 3/4] test(newman): rewrite legacy CRUD folder for spec-C versioned model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'OpenRegister-backed Application CRUD' folder still POSTed manifest/version/status directly on the Application object — the pre-spec-C model where those lived on Application itself. Per ADR-002 the manifest + status + semver moved to ApplicationVersion, so 5 of the 6 assertions silently dropped to `undefined` and the flow died at the lifecycle transition. Rewritten flow: 1. POST plain Application (no manifest / status fields). 2. GET Application by uuid — assert slug round-trip only. 3. POST ApplicationVersion under that Application via the OpenBuilt version-aware endpoint (carries the manifest now). 4. PUT the Application's `productionVersion` pointer to the new version UUID. 5. POST publish transition on the VERSION (transitions live there under the new state machine). 6. GET /api/applications/{slug}/manifest — should resolve via ManifestResolverService to the version's manifest. 7. DELETE the version + per-version register via the OB endpoint. Note: step 5 currently returns 500 against a fresh dev container — likely a separate publish-from-newman-context bug worth filing, not a test-suite issue. Net Newman now goes 26/-5 (was 23/-6); the remaining 5 failures cascade from the 500 on step 5. --- .../openbuilt.postman_collection.json | 90 ++++++++++++++++--- 1 file changed, 76 insertions(+), 14 deletions(-) diff --git a/tests/integration/openbuilt.postman_collection.json b/tests/integration/openbuilt.postman_collection.json index 80f794ea..6f5bce84 100644 --- a/tests/integration/openbuilt.postman_collection.json +++ b/tests/integration/openbuilt.postman_collection.json @@ -149,10 +149,10 @@ ], "body": { "mode": "raw", - "raw": "{\n \"slug\": \"newman-roundtrip\",\n \"name\": \"Newman Round-trip\",\n \"description\": \"Synthetic Application created by the Newman CRUD round-trip\",\n \"version\": \"0.0.1\",\n \"status\": \"draft\",\n \"manifest\": {\n \"version\": \"1.0.0\",\n \"menu\": [],\n \"pages\": [\n {\n \"id\": \"Home\",\n \"route\": \"/\",\n \"type\": \"index\",\n \"title\": \"Home\",\n \"config\": { \"register\": \"openbuilt\", \"schema\": \"hello-message\", \"columns\": [\"title\"] }\n }\n ]\n }\n}" + "raw": "{\n \"slug\": \"newman-roundtrip\",\n \"name\": \"Newman Round-trip\",\n \"description\": \"Synthetic Application created by the Newman CRUD round-trip\"\n}" }, "url": "{{base_url}}/index.php/apps/openregister/api/objects/openbuilt/application", - "description": "Create a synthetic Application object via OR REST (ADR-022: CRUD goes to OR directly, not an OB wrapper)." + "description": "Create a synthetic Application object via OR REST. Per ADR-002 (versioned-app model) the `manifest`/`version`/`status` fields live on ApplicationVersion now, NOT the Application itself." }, "event": [ { @@ -187,7 +187,7 @@ { "key": "OCS-APIRequest", "value": "true" } ], "url": "{{base_url}}/index.php/apps/openregister/api/objects/openbuilt/application/{{created_uuid}}", - "description": "Read-back asserts the POST landed on disk and the uuid resolves." + "description": "Read-back asserts the POST landed on disk and the uuid resolves. Versioned model: no manifest at this layer; verified separately on the version record below." }, "event": [ { @@ -201,9 +201,45 @@ "const body = pm.response.json();", "pm.test('slug round-trips', function () {", " pm.expect(body.slug || (body['@self'] && body['@self'].slug)).to.eql('newman-roundtrip');", + "});" + ] + } + } + ] + }, + { + "name": "POST creates an ApplicationVersion under the new Application", + "request": { + "method": "POST", + "header": [ + { "key": "Accept", "value": "application/json" }, + { "key": "Content-Type", "value": "application/json" }, + { "key": "OCS-APIRequest", "value": "true" } + ], + "body": { + "mode": "raw", + "raw": "{\n \"name\": \"Newman Round-trip v1\",\n \"slug\": \"production\",\n \"manifest\": {\n \"version\": \"1.0.0\",\n \"menu\": [],\n \"pages\": [\n {\n \"id\": \"Home\",\n \"route\": \"/\",\n \"type\": \"index\",\n \"title\": \"Home\",\n \"config\": { \"register\": \"openbuilt\", \"schema\": \"hello-message\", \"columns\": [\"title\"] }\n }\n ]\n },\n \"register\": \"openbuilt-newman-roundtrip-production\",\n \"semver\": \"1.0.0\",\n \"status\": \"draft\",\n \"application\": \"{{created_uuid}}\"\n}" + }, + "url": "{{base_url}}/index.php/apps/openbuilt/api/applications/newman-roundtrip/versions", + "description": "Per ADR-002, the manifest + status + semver live on ApplicationVersion. POST against the version-aware OpenBuilt endpoint so the parent-relation guard runs." + }, + "event": [ + { + "listen": "test", + "script": { + "type": "text/javascript", + "exec": [ + "pm.test('returns 200 or 201', function () {", + " pm.expect([200, 201]).to.include(pm.response.code);", + "});", + "const body = pm.response.json();", + "const versionUuid = (body['@self'] && (body['@self'].id || body['@self'].uuid)) || body.uuid || body.id;", + "pm.test('version uuid is captured', function () {", + " pm.expect(versionUuid).to.be.a('string');", "});", - "pm.test('manifest is present and not empty', function () {", - " pm.expect(body.manifest, 'no manifest on GET').to.be.an('object');", + "pm.collectionVariables.set('created_version_uuid', versionUuid);", + "pm.test('manifest persisted on the version (not the application)', function () {", + " pm.expect(body.manifest).to.be.an('object');", "});" ] } @@ -211,7 +247,7 @@ ] }, { - "name": "PUT saves a manifest edit before publish (newman-roundtrip)", + "name": "PATCH set the Application's productionVersion pointer", "request": { "method": "PUT", "header": [ @@ -221,10 +257,10 @@ ], "body": { "mode": "raw", - "raw": "{\n \"slug\": \"newman-roundtrip\",\n \"name\": \"Newman Round-trip\",\n \"description\": \"Synthetic Application created by the Newman CRUD round-trip\",\n \"version\": \"0.0.2\",\n \"status\": \"draft\",\n \"manifest\": {\n \"version\": \"1.0.0\",\n \"menu\": [],\n \"pages\": [\n {\n \"id\": \"Home\",\n \"route\": \"/\",\n \"type\": \"index\",\n \"title\": \"Home\",\n \"config\": { \"register\": \"openbuilt\", \"schema\": \"hello-message\", \"columns\": [\"title\"] }\n }\n ]\n }\n}" + "raw": "{\n \"slug\": \"newman-roundtrip\",\n \"name\": \"Newman Round-trip\",\n \"description\": \"Synthetic Application created by the Newman CRUD round-trip\",\n \"productionVersion\": \"{{created_version_uuid}}\"\n}" }, "url": "{{base_url}}/index.php/apps/openregister/api/objects/openbuilt/application/{{created_uuid}}", - "description": "Save pending manifest edits while still in draft (mirrors ApplicationEditor.vue's publish() step 1). The publish happens via the lifecycle transition below, not by flipping status here — a raw status=published PUT would never fire ObjectTransitionedEvent and so the BuiltAppRoute would never be created (REQ-OBA-003 / REQ-OBA-004)." + "description": "ADR-002: the application's `productionVersion` field carries the UUID of the version that should be served as the live manifest." }, "event": [ { @@ -236,8 +272,9 @@ " pm.response.to.have.status(200);", "});", "const body = pm.response.json();", - "pm.test('still in draft (publish happens via lifecycle transition)', function () {", - " pm.expect(body.status || (body['@self'] && body['@self'].status)).to.eql('draft');", + "const prod = body.productionVersion || (body['@self'] && body['@self'].relations && body['@self'].relations.productionVersion);", + "pm.test('productionVersion points at the new version', function () {", + " pm.expect(prod).to.eql(pm.collectionVariables.get('created_version_uuid'));", "});" ] } @@ -245,7 +282,7 @@ ] }, { - "name": "POST the publish lifecycle transition (fires ObjectTransitionedEvent)", + "name": "POST publish lifecycle transition on the ApplicationVersion", "request": { "method": "POST", "header": [ @@ -257,8 +294,8 @@ "mode": "raw", "raw": "{ \"action\": \"publish\" }" }, - "url": "{{base_url}}/index.php/apps/openregister/api/objects/{{created_uuid}}/transition", - "description": "A raw PUT with status=published does NOT make OR fire ObjectTransitionedEvent, so the BuiltAppRoute (slug -> uuid) that GET /api/applications/{slug}/manifest resolves is never created. OR's lifecycle transition endpoint (POST /api/objects/{id}/transition with {action}) fires the event -> ApplicationVersionSnapshotListener upserts the BuiltAppRoute (REQ-OBA-003 / REQ-OBA-004). This is the server-side equivalent of ApplicationEditor.vue's publish() flow." + "url": "{{base_url}}/index.php/apps/openregister/api/objects/{{created_version_uuid}}/transition", + "description": "Publishing is a state machine on the VERSION now (ADR-002). The transition fires ObjectTransitionedEvent → ApplicationVersionSnapshotListener upserts the BuiltAppRoute." }, "event": [ { @@ -283,7 +320,7 @@ { "key": "OCS-APIRequest", "value": "true" } ], "url": "{{base_url}}/index.php/apps/openbuilt/api/applications/newman-roundtrip/manifest", - "description": "After the publish transition, the BuiltAppRoute is upserted and the manifest endpoint resolves the new slug." + "description": "After the publish transition + productionVersion pointer set, ManifestResolverService should resolve the slug to the version's manifest." }, "event": [ { @@ -307,6 +344,31 @@ } ] }, + { + "name": "DELETE cleans up the round-trip ApplicationVersion", + "request": { + "method": "DELETE", + "header": [ + { "key": "Accept", "value": "application/json" }, + { "key": "OCS-APIRequest", "value": "true" } + ], + "url": "{{base_url}}/index.php/apps/openbuilt/api/applications/newman-roundtrip/versions/production?strategy=hard", + "description": "Drop the version + its per-version register first so the parent Application can be safely deleted." + }, + "event": [ + { + "listen": "test", + "script": { + "type": "text/javascript", + "exec": [ + "pm.test('returns 200 or 204', function () {", + " pm.expect([200, 204]).to.include(pm.response.code);", + "});" + ] + } + } + ] + }, { "name": "DELETE cleans up the round-trip Application", "request": { From 62750f15ae0fc8c0503efdeadd74edf282022924 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 16 May 2026 17:19:21 +0200 Subject: [PATCH 4/4] fix(#77): owner-namespaced register slug on cross-user collision + Playwright session reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## #77 — cross-user register slug collision OR's register slug is organisation-wide unique. \`provisionPerAppRegister\` unconditionally used \`openbuilt-{slug}\`, which silently shared a register across users when two callers cloned the same template. Compatible fix: keep the legacy un-namespaced slug WHEN the existing register is owned by the caller (idempotent re-clones, single-tenant installs). When a different user owns the legacy slug, fall back to \`openbuilt-{ownerUid}-{slug}\` so the cross-user case provisions a fresh register instead of joining the other user's. Adds \`findOrCreateRegister\` + \`extractRegisterOwner\` helpers (tolerant of either an \`owner\` getter on the entity or an \`@self.owner\` block in the JSON shape). ## Playwright session reuse + path fix Two unrelated test-infrastructure issues that were blocking the whole e2e suite: 1. \`global-setup.ts\` logs in once at startup and writes the browser storage state to \`tests/e2e/.auth/admin.json\`. Every spec then reuses the session — no more per-spec \`loginAsAdmin\` helper wrestling with NC's brute-force throttle when workers ran in parallel. 2. \`fullyParallel: false\` + \`workers: 1\` — serial spec execution keeps the brute-force counter quiet. 3. applicationCard.spec.ts + iconUpload.spec.ts: \`page.goto('/apps/openbuilt')\` landed on the Dashboard widget page (route \`/\`), not the Virtual apps index (route \`/applications\`). Updated to \`/apps/openbuilt/applications\` so \`.ob-app-card\` actually renders. \`.auth/\` is gitignored. PHPUnit 204/204 still green. --- .gitignore | 1 + lib/Controller/ApplicationsController.php | 92 +++++++++++++++++++++-- playwright.config.ts | 13 +++- tests/e2e/applicationCard.spec.ts | 8 +- tests/e2e/global-setup.ts | 57 ++++++++++++++ tests/e2e/iconUpload.spec.ts | 10 ++- 6 files changed, 166 insertions(+), 15 deletions(-) create mode 100644 tests/e2e/global-setup.ts diff --git a/.gitignore b/.gitignore index 5b8b0d3c..ef68e426 100644 --- a/.gitignore +++ b/.gitignore @@ -96,3 +96,4 @@ openspec/test-site-results/**/*.webp /test-results/ /.playwright/ /playwright/.cache/ +tests/e2e/.auth/ diff --git a/lib/Controller/ApplicationsController.php b/lib/Controller/ApplicationsController.php index 4f7ca1d4..c1444182 100644 --- a/lib/Controller/ApplicationsController.php +++ b/lib/Controller/ApplicationsController.php @@ -1189,24 +1189,102 @@ private function buildRewriteMap(array $companions, string $newSlug): array */ private function provisionPerAppRegister(string $newSlug, string $ownerUid): \OCA\OpenRegister\Db\Register { - $registerSlug = 'openbuilt-'.$newSlug; + // Cross-user collision guard (#77): OR's register slugs are + // organisation-wide unique, so two callers wanting the same + // template+slug would otherwise share a register. The first + // attempt at scoping always namespaced by owner, but that + // breaks every existing single-tenant install (their existing + // registers are still at `openbuilt-{slug}`). So we keep the + // legacy un-namespaced slug AS LONG AS the existing register + // belongs to the caller; otherwise fall back to the + // owner-namespaced form. + $legacyRegisterSlug = 'openbuilt-'.$newSlug; try { - return $this->registerMapper->find($registerSlug, _multitenancy: false); + $existing = $this->registerMapper->find($legacyRegisterSlug, _multitenancy: false); + + $existingOwner = $this->extractRegisterOwner($existing); + if ($existingOwner === '' || $existingOwner === $ownerUid) { + return $existing; + } + + // Different user owns the org-wide slug — namespace ours. + return $this->findOrCreateRegister( + slug: 'openbuilt-'.$ownerUid.'-'.$newSlug, + appSlug: $newSlug, + ownerUid: $ownerUid, + ); + } catch (Throwable) { + // Legacy slug not taken — claim it for this user. + } + + return $this->findOrCreateRegister( + slug: $legacyRegisterSlug, + appSlug: $newSlug, + ownerUid: $ownerUid, + ); + }//end provisionPerAppRegister() + + /** + * Find or create a per-app register at an exact slug. + * + * @param string $slug The register slug to find/create + * @param string $appSlug The application slug (for the title) + * @param string $ownerUid The owner UID (for the description audit trail) + * + * @return \OCA\OpenRegister\Db\Register + */ + private function findOrCreateRegister( + string $slug, + string $appSlug, + string $ownerUid, + ): \OCA\OpenRegister\Db\Register { + try { + return $this->registerMapper->find($slug, _multitenancy: false); } catch (Throwable) { - // Register does not exist yet — create it. + // Not found — create it. } return $this->registerMapper->createFromArray( [ - 'slug' => $registerSlug, - 'title' => 'OpenBuilt — '.$newSlug, - 'description' => 'Per-app schema namespace for OpenBuilt app `'.$newSlug.'` (owner: '.$ownerUid.').', + 'slug' => $slug, + 'title' => 'OpenBuilt — '.$appSlug, + 'description' => 'Per-app schema namespace for OpenBuilt app `'.$appSlug.'` (owner: '.$ownerUid.').', 'version' => '0.1.0', 'schemas' => [], ] ); - }//end provisionPerAppRegister() + }//end findOrCreateRegister() + + /** + * Extract the owner UID from a Register entity, tolerating either an + * `owner` field on the entity itself or one inside an `@self` block. + * + * @param mixed $register The Register entity (or non-Register junk). + * + * @return string The owner UID, or empty string when not determinable. + */ + private function extractRegisterOwner(mixed $register): string + { + if (is_object($register) === true && method_exists($register, 'getOwner') === true) { + $owner = $register->getOwner(); + if (is_string($owner) === true) { + return $owner; + } + } + + if (is_object($register) === true && method_exists($register, 'jsonSerialize') === true) { + $data = $register->jsonSerialize(); + if (is_array($data) === true) { + $owner = ($data['owner'] ?? ($data['@self']['owner'] ?? null)); + if (is_string($owner) === true) { + return $owner; + } + } + } + + return ''; + }//end extractRegisterOwner() /** * Clone companion schemas into the per-app register. diff --git a/playwright.config.ts b/playwright.config.ts index f0968c06..de894437 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -18,13 +18,22 @@ export default defineConfig({ testDir: 'tests/e2e', timeout: 30_000, expect: { timeout: 5_000 }, - fullyParallel: true, + // Don't run specs in parallel: Nextcloud's brute-force throttle fires + // after a handful of near-simultaneous form logins from the same IP + // and every subsequent spec falls back to the /login page. Serial + // execution with one shared storageState (via globalSetup) avoids it. + fullyParallel: false, forbidOnly: !!process.env.CI, retries: process.env.CI ? 1 : 0, - workers: process.env.CI ? 1 : undefined, + workers: 1, + globalSetup: './tests/e2e/global-setup.ts', reporter: process.env.CI ? [['github'], ['html', { open: 'never' }]] : 'list', use: { baseURL: process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8080', + // Authenticated browser context populated by globalSetup. Empty + // when login fails — specs then surface the actual login page in + // their failure snapshots. + storageState: 'tests/e2e/.auth/admin.json', httpCredentials: { username: process.env.NC_ADMIN_USER || 'admin', password: process.env.NC_ADMIN_PASSWORD || 'admin', diff --git a/tests/e2e/applicationCard.spec.ts b/tests/e2e/applicationCard.spec.ts index 019ccdf9..bedd0979 100644 --- a/tests/e2e/applicationCard.spec.ts +++ b/tests/e2e/applicationCard.spec.ts @@ -26,7 +26,7 @@ import { test, expect } from '@playwright/test' test.describe('ApplicationCard — icon + productionVersion fields (spec A / spec C)', () => { test('index page renders ApplicationCards with icon elements', async ({ page }) => { - await page.goto('/index.php/apps/openbuilt') + await page.goto('/index.php/apps/openbuilt/applications') // Wait for the SPA to hydrate and the Applications list to appear. // The list renders one card per Application. The seeded hello-world @@ -46,7 +46,7 @@ test.describe('ApplicationCard — icon + productionVersion fields (spec A / spe }) test('hello-world ApplicationCard shows a status badge (not raw "Live" chip)', async ({ page }) => { - await page.goto('/index.php/apps/openbuilt') + await page.goto('/index.php/apps/openbuilt/applications') // Wait for at least the seeded hello-world card. await expect( @@ -66,7 +66,7 @@ test.describe('ApplicationCard — icon + productionVersion fields (spec A / spe }) test('hello-world ApplicationCard status badge is one of the known values', async ({ page }) => { - await page.goto('/index.php/apps/openbuilt') + await page.goto('/index.php/apps/openbuilt/applications') // Find the card for hello-world specifically. const helloCard = page.locator('[data-slug="hello-world"], .ob-app-card').first() @@ -85,7 +85,7 @@ test.describe('ApplicationCard — icon + productionVersion fields (spec A / spe }) test('hello-world ApplicationCard version chip shows semver or — placeholder', async ({ page }) => { - await page.goto('/index.php/apps/openbuilt') + await page.goto('/index.php/apps/openbuilt/applications') const helloCard = page.locator('[data-slug="hello-world"], .ob-app-card').first() await expect(helloCard).toBeVisible({ timeout: 15_000 }) diff --git a/tests/e2e/global-setup.ts b/tests/e2e/global-setup.ts new file mode 100644 index 00000000..4eb08d2a --- /dev/null +++ b/tests/e2e/global-setup.ts @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: EUPL-1.2 +// SPDX-FileCopyrightText: 2026 Conduction B.V. + +/** + * Playwright globalSetup — logs in to Nextcloud once and writes the + * authenticated browser context (cookies + localStorage) to + * `tests/e2e/.auth/admin.json`. Every spec then reuses that storage + * state via the project-level `storageState` config, so per-spec + * `loginAsAdmin` helpers are no longer required. + * + * Before this hook existed, specs without an explicit form-login step + * (applicationCard, builder-host, bootstrap-openbuilt, …) landed on + * `/login` and every locator timed out. Nextcloud's session is cookie- + * based; basic auth alone doesn't satisfy the SPA's auth check. + * + * The hook is no-op when login fails (e.g. brute-force throttle); the + * resulting empty storage state lets specs surface real errors with a + * clear "still on /login" snapshot in the report. + */ + +import { chromium, FullConfig } from '@playwright/test' +import { existsSync, mkdirSync } from 'fs' +import { dirname } from 'path' + +export default async function globalSetup(config: FullConfig): Promise { + const baseURL = (config.projects[0].use.baseURL as string) + || process.env.PLAYWRIGHT_BASE_URL + || 'http://localhost:8080' + const adminUser = process.env.NC_ADMIN_USER || 'admin' + const adminPassword = process.env.NC_ADMIN_PASSWORD || process.env.NC_ADMIN_PASS || 'admin' + const storagePath = 'tests/e2e/.auth/admin.json' + + if (existsSync(dirname(storagePath)) === false) { + mkdirSync(dirname(storagePath), { recursive: true }) + } + + const browser = await chromium.launch() + const context = await browser.newContext({ baseURL }) + const page = await context.newPage() + + try { + await page.goto('/index.php/login', { waitUntil: 'domcontentloaded' }) + await page.locator('input[name="user"]').fill(adminUser) + await page.locator('input[name="password"]').fill(adminPassword) + await page.locator('button[type="submit"], input[type="submit"]').first().click() + // Accept both pretty + index.php-prefixed redirects. + await page.waitForURL(/\/apps\//, { timeout: 20_000 }) + await context.storageState({ path: storagePath }) + // eslint-disable-next-line no-console + console.log(`[globalSetup] authenticated session stored at ${storagePath}`) + } catch (e) { + // eslint-disable-next-line no-console + console.warn(`[globalSetup] login failed — specs will run unauthenticated: ${(e as Error).message}`) + } finally { + await browser.close() + } +} diff --git a/tests/e2e/iconUpload.spec.ts b/tests/e2e/iconUpload.spec.ts index c597b1d5..038cfede 100644 --- a/tests/e2e/iconUpload.spec.ts +++ b/tests/e2e/iconUpload.spec.ts @@ -43,7 +43,10 @@ test.describe('Icon upload on Application detail page (spec A task 7.5)', () => test('Icon tab is accessible from the Application detail page', async ({ page }) => { // Navigate to the Applications list, then open the hello-world detail. - await page.goto('/index.php/apps/openbuilt') + // Land on the Virtual apps index — the ApplicationCards live at + // `/applications`, not at the app root (which redirects to the + // Dashboard widget page). + await page.goto('/index.php/apps/openbuilt/applications') await expect(page.locator('.ob-app-card, [data-testid*="app-card"]').first()).toBeVisible({ timeout: 15_000 }) // Navigate to the hello-world detail page. The card links to @@ -81,7 +84,10 @@ test.describe('Icon upload on Application detail page (spec A task 7.5)', () => }) test('uploading a minimal SVG updates the preview src', async ({ page }) => { - await page.goto('/index.php/apps/openbuilt') + // Land on the Virtual apps index — the ApplicationCards live at + // `/applications`, not at the app root (which redirects to the + // Dashboard widget page). + await page.goto('/index.php/apps/openbuilt/applications') await expect(page.locator('.ob-app-card, [data-testid*="app-card"]').first()).toBeVisible({ timeout: 15_000 }) // Open the hello-world detail.