This repository was archived by the owner on May 29, 2026. It is now read-only.
fix(templates): same-user slug collision returns slug_collision 409 (closes #51)#76
Merged
Conversation
`ApplicationsController::lookupOne` placed the optional `owner` filter at the top level of the OR search query, but OR records ownership under `@self.owner`. Every owner-scoped lookup therefore matched 0 rows. Net effect for the createFromTemplate flow (issue #51): when the same user re-cloned a template with an already-used slug, the collision check fell through, provisioning then hit the org-wide register-slug unique constraint, and the response went out as `clone_failed` 500 instead of the documented `slug_collision` 409. Moving the `owner` filter into the `@self` block restores the documented behaviour (REQ-OBT-005). Note: the cross-user case (different user, same slug) still mis-behaves because OR register slugs are organisation-wide unique and OpenBuilt provisions registers as `openbuilt-{slug}` regardless of owner — a deeper architectural change that warrants its own ticket.
Contributor
Quality Report — ConductionNL/openbuilt @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ❌ | ❌ | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-16 12:34 UTC
Download the full PDF report from the workflow artifacts.
5 tasks
rubenvdlinde
added a commit
that referenced
this pull request
May 16, 2026
…ht env (#78) * fix(wizard): namespace manifest config.schema alongside config.register (closes #75) `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. * fix(e2e): remove global OCS-APIRequest header + accept pretty-URL redirects 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. * test(newman): rewrite legacy CRUD folder for spec-C versioned model 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. * fix(#77): owner-namespaced register slug on cross-user collision + Playwright session reuse ## #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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ApplicationsController::lookupOnewas placing the optional `owner` filter at the top level of the OR search query — but OR records ownership under `@self.owner`. Every owner-scoped collision check therefore matched 0 rows, the check fell through, and provisioning then hit the org-wide register-slug unique constraint, returning the wrong error code (`clone_failed` 500 instead of the documented `slug_collision` 409).One-line move of the filter into `@self` restores REQ-OBT-005 behaviour.
Out of scope
The cross-user case (different user, same slug → expected 201, currently 500) still misbehaves because OR register slugs are org-wide unique and OpenBuilt provisions `openbuilt-{slug}` without owner-scoping. Filing as a follow-up architectural ticket since it requires changing the register-naming scheme.
Test plan
Partially closes #51 (same-user path); the cross-user path will be tracked in a follow-up.