From 53b14de383a95900e0d3956dcca54e2b58bedbfd Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 18:25:54 +0200 Subject: [PATCH 1/3] test(e2e): 21 real Playwright tests + 4 exclusions close gate-19 137 -> 112 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gate-19 (e2e-coverage) was softwarecatalog's only failing gate. Baseline reproduced locally byte-identical to CI — 137 — against the CANONICAL gate package (freshly cloned .github@main), scoped HYDRA_GATE_BASE_REF=origin/beta, which is what CI resolves for the open development -> beta PR #197. start 137 -> end 112 (21 tests + 4 exclusions) Negative control, run on the final tree: with suite-wizard.spec.ts 112 file removed 118 (+6 = its exactly-6 @e2e anchors) file restored 112 Every one of the 14 new tests was proven able to FAIL by a planted true positive, each plant chosen so only its own test went red — e.g. `applicaties: []` in buildSuitePayload reddened only the submit test; renaming the third wizard step reddened only the step-labels test. Two needed the plant to remove TWO layers: Nextcloud core's SecurityMiddleware rejects an unauthenticated request before the controller body runs, so the anon-POST test only reddened once #[PublicPage] + #[NoCSRFRequired] were added as well. Full suite on an isolated single-owner rig: 90 passed, 4 skipped, 0 failed. The 4 skipped are pre-existing test.fixme cases, untouched. WHAT THE TESTS FOUND (all filed, none worked around) * #481 — every write through src/utils/adminApi.js failed CSRF. The helper sent no Nextcloud requesttoken, and none of its target controllers is #[NoCSRFRequired], so SEVEN shipped actions were unreachable from the UI: review submit, moderation approve/reject, federation peer add/remove/pull, EOL config save and "Sync now". GET is exempt, so every settings section rendered fine and only its buttons were dead. Fixed here, because the coverage could not exist without it: the same unchanged test goes green once the token is sent. src/store/modules/facets.js already used @nextcloud/axios and was never affected. * #482 — the suite wizard's success result is never rendered. onSuiteCreated sets showWizard = false in the same synchronous tick setResult() runs, and the dialog is under v-if, so the result phase is destroyed before any render flush. Not a race. The test asserts the two THENs that hold and DISCLOSES the third in a comment block rather than quietly dropping it. * #483 — two gemma-faceted-search requirements are unimplemented: empty facet dimensions are never disabled, and the facet cache is TTL-only with no invalidation path. Both are browser-observable, so both are left IN the count rather than excluded. * #484 — sbom-import.spec.ts carries 16 file-level @e2e tags in a docblock that says, verbatim, "excluded from Playwright coverage below". gate-19 credits them (.github#343), so the honest debt is 153, not 137. Deliberately NOT "fixed" here — converting 16 positive claims into exclusions inside a PR that lowers the gate count is indistinguishable from gaming it. * #485 — 23 whole-spec @e2e exclude markers retire 212 of 696 scenarios (30%), several on reasons that do not hold ("no live deployment in this pass"; Vue modals excluded from browser testing; "is an interaction"). THE 4 EXCLUSIONS All scenario-level, all in gemma-faceted-search, each with its own reason, and each verified not to leak: the sibling scenario sharing a requirement with the code-path-identity exclusion is still counted as uncovered (.github#356). They are the cases where no observation could decide the claim — a PHP array key between two services (the endpoint's _meta echoes no query), paging vs. scanning (byte-identical response, signal is a log line), code-path IDENTITY (two paths agreeing on every output are indistinguishable from one), and l10n KEY names (the browser only ever sees resolved values). Refused the exclusion where the requirement is merely broken or absent — those are issues above, not waivers. DEAD CODE src/views/gemmaviews/GemmaViewIndex.vue deleted. Not concluded from "no references found" — that is an absence claim. Proven positively: the bundle built WITH the file present is BYTE-IDENTICAL to the bundle built without it (md5 b6cc3b81..., 5,215,895 bytes), so webpack could not reach it from any entry. Positive control in the same bundle: OrganisationMergePanel 9 hits, "Preview merge" 3, SuitesIndexView 6 — the search does find live components. GemmaViewIndex: 0, and its unique string "No GEMMA views are available": 0. Cross-checked against the six ways a component can be reached in these apps — registry.js, customComponents.js, every string-keyed component/widgetKey/ target/handler value in manifest.json + manifest.d/*.json (13 names, enumerated in full), router.js, dynamic imports, and the bundle. The only surviving mention was a proposal that had already flagged it as orphaned. src/store/modules/view.js is now its only-consumer-less sibling and is also absent from the bundle; left in place deliberately — it fronts a backend view-enrichment API and deserves its own decision. Refs #481 #482 #483 #484 #485 --- openspec/specs/gemma-faceted-search/spec.md | 8 + src/utils/adminApi.js | 31 ++ src/views/gemmaviews/GemmaViewIndex.vue | 273 ----------- .../e2e/spec-coverage/catalog-ratings.spec.ts | 435 ++++++++++++++++++ tests/e2e/spec-coverage/suite-wizard.spec.ts | 293 ++++++++++++ 5 files changed, 767 insertions(+), 273 deletions(-) delete mode 100644 src/views/gemmaviews/GemmaViewIndex.vue create mode 100644 tests/e2e/spec-coverage/catalog-ratings.spec.ts create mode 100644 tests/e2e/spec-coverage/suite-wizard.spec.ts diff --git a/openspec/specs/gemma-faceted-search/spec.md b/openspec/specs/gemma-faceted-search/spec.md index 9940476c..88032f7c 100644 --- a/openspec/specs/gemma-faceted-search/spec.md +++ b/openspec/specs/gemma-faceted-search/spec.md @@ -74,6 +74,8 @@ Every OpenRegister `searchObjects()` (or equivalent aggregate) call issued by th #### Scenario: Facet aggregation query sets an explicit limit +@e2e exclude The subject is a PHP array key (`$pagedQuery['_limit']`) that exists only between `FacetService` and `ObjectService` inside one request; no browser or HTTP response ever carries it. Verified against the live endpoint rather than assumed: `GET /apps/softwarecatalog/api/facets/module` returns `_meta` = `{totalMatched, processingTimeMs, cached, matchedObjectIds}` — the query array is not echoed in any form, so there is nothing a Playwright assertion could read. Covered instead by a unit test on `FacetService`'s query construction, which can inspect the array directly. + - GIVEN `FacetService` builds a query to aggregate `referentiecomponent` values across the `module` schema - WHEN the query array is constructed - THEN it MUST include an explicit `_limit` value @@ -81,6 +83,8 @@ Every OpenRegister `searchObjects()` (or equivalent aggregate) call issued by th #### Scenario: A register too large for one bounded page pages instead of scanning unbounded +@e2e exclude Distinguishes "paged through `searchObjectsPaginated()`" from "one unbounded `searchObjects()`" — two implementations that produce the BYTE-IDENTICAL response. Verified: the endpoint's `_meta` (`totalMatched, processingTimeMs, cached, matchedObjectIds`) discloses no paging state, and the overflow path's only external signal is a `LoggerInterface::warning` line. A browser cannot read a log file, and a response that is the same either way cannot be asserted on. Covered by a unit test that counts calls to the paginated verb. + - GIVEN the `module` register has more objects than fit in one bounded facet aggregation page - WHEN facet counts are computed - THEN the service MUST page through results via `searchObjectsPaginated()` (or a documented `_limit` ceiling) to reach a complete count @@ -99,6 +103,8 @@ Facet aggregation SHALL count only objects the requesting user is authorized to #### Scenario: Facet aggregation uses the same authorization path as the object list +@e2e exclude Asserts CODE-PATH IDENTITY ("the identical scoping ... MUST NOT use a separate, unscoped counting code path"), not an outcome. Two code paths that happen to agree on every observable output are indistinguishable from one shared path by any black-box observation, so no browser assertion can decide this proposition — even a perfect test of the OUTCOME would leave the requirement unproven, which is why this is not merely "hard to test". The observable half (a restricted user's counts reflect only their own scope) is a SEPARATE scenario in this same requirement and is NOT excluded. + - GIVEN the module index page's own object list query is scoped by RBAC/organisation context - WHEN the facet aggregation query is built - THEN it MUST apply the identical RBAC/tenant scoping as the object list query @@ -210,6 +216,8 @@ All facet dimension labels, facet value display strings sourced from the UI laye #### Scenario: Translation keys are in English +@e2e exclude The subject is the KEY side of the `l10n/*.json` source files. A browser is only ever served the resolved VALUE — by the time any string reaches the DOM the key has been substituted away, so a rendered page is identical whether the key was `facetSaveAsView` or a Dutch literal. This is a repository-file property, enforceable only by reading `l10n/` (which the sibling scenario "Facet panel renders in the user's selected language" does NOT substitute for: that one asserts the values). + - GIVEN the softwarecatalog `l10n` translation files - WHEN the facet panel's translation keys are inspected - THEN each key MUST be an English identifier (e.g. `facetSaveAsView`), not a Dutch string, with the Dutch translation supplied as the `nl` value diff --git a/src/utils/adminApi.js b/src/utils/adminApi.js index d6d8c523..16ce3238 100644 --- a/src/utils/adminApi.js +++ b/src/utils/adminApi.js @@ -16,6 +16,8 @@ * SPDX-License-Identifier: EUPL-1.2 */ +import { getRequestToken } from '@nextcloud/auth' + const API_BASE = '/index.php/apps/softwarecatalog/api' /** @@ -50,11 +52,40 @@ export async function apiRequest(path, options = {}, fetchImpl = undefined) { throw new Error('No fetch implementation available') } + // ⚠️ `requesttoken` is NOT optional on a state-changing request. + // + // Nextcloud's CSRF middleware rejects any cookie-authenticated request + // whose method is not GET/HEAD unless the controller declares + // `#[NoCSRFRequired]` OR the request carries the session's request token. + // `X-Requested-With: XMLHttpRequest` does NOT satisfy it — that header was + // dropped as a CSRF signal long ago. + // + // Without this header EVERY write that goes through this helper failed + // with a rendered "CSRF check failed" alert and no server-side effect: + // SubmitReviewModal POST reviews + // ModerationQueue POST moderation/{uuid}/approve|reject + // FederationSettings POST/DELETE federation/peers, POST federation/pull + // EolSyncSettings POST eol-sync/config, POST eol-sync/trigger + // None of those controllers is `#[NoCSRFRequired]`, so none of them was + // reachable from the UI. Measured, not inferred: a Playwright run driving + // the real "Write a review" modal captured the alert text `CSRF check + // failed` in the dialog, and the same flow passes once this header is sent. + // + // Reads are unaffected (GET is exempt), which is why the settings sections + // rendered correctly and only their WRITE actions were dead — a failure + // mode that looks like "the button does nothing". + // + // `getRequestToken()` reads the token @nextcloud/auth keeps in sync with + // the `data-requesttoken` head meta, so it stays correct across NC's + // token rotation. This is the same source `@nextcloud/axios` uses; the + // sibling store `src/store/modules/facets.js` already goes through axios + // and was never affected. const init = { method: options.method || 'GET', headers: { 'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest', + requesttoken: getRequestToken() ?? '', }, } if (options.body !== undefined && options.body !== null) { diff --git a/src/views/gemmaviews/GemmaViewIndex.vue b/src/views/gemmaviews/GemmaViewIndex.vue deleted file mode 100644 index c96ad643..00000000 --- a/src/views/gemmaviews/GemmaViewIndex.vue +++ /dev/null @@ -1,273 +0,0 @@ - - - - - - diff --git a/tests/e2e/spec-coverage/catalog-ratings.spec.ts b/tests/e2e/spec-coverage/catalog-ratings.spec.ts new file mode 100644 index 00000000..b0508e02 --- /dev/null +++ b/tests/e2e/spec-coverage/catalog-ratings.spec.ts @@ -0,0 +1,435 @@ +// SPDX-License-Identifier: EUPL-1.2 +// SPDX-FileCopyrightText: 2026 Conduction B.V. +/** + * Behavioural e2e coverage for catalog ratings & reviews. + * + * Surfaces under test: + * src/components/reviews/ReviewsPanel.vue body widget `md-reviews` on + * ModuleDetail (/modules/:id) + * src/modals/SubmitReviewModal.vue "Write a review" + * src/views/settings/sections/ModerationQueue.vue + * reused for type="beoordeeling" + * at /settings/admin/softwarecatalog + * manifest page `Reviews` (/reviews) the reviews index + * + * The moderation lifecycle is driven END TO END through the UI: submit in the + * module panel, approve/reject in the admin queue, then re-read the panel. The + * aggregate is *not* re-derived by the test — it is read off the rendered + * panel, so a change that stops the panel consuming the aggregate fails here. + * + * Two scenarios are asserted at the HTTP layer rather than the DOM, and both + * are genuinely unreachable through the shipped UI, not shortcuts: + * - "an unauthenticated request cannot create a review": the modal is only + * rendered to a logged-in user, so the anonymous case has no UI at all. + * - "a client-supplied author is ignored": SubmitReviewModal never sends an + * `auteur` field, so the rogue payload cannot be produced by any click. + * Playwright's APIRequestContext is still the browser's own HTTP stack, and + * every probe below prints its STATUS CODE into the assertion message. + * + * @spec openspec/specs/catalog-ratings/spec.md + */ +import { test, expect, request as playwrightRequest, type Page } from '@playwright/test' +import type { APIRequestContext } from '@playwright/test' +import { APP_BASE, APP_MAIN, APP_SHELL, collectAppErrors, dismissSupportDialog, dismissWalkthrough, expectNoAppErrors, gotoAppRoute, navClickTo } from './_helpers' +import { + BASE_URL, + RUN_ID, + createObject, + findAll, + newApiContext, + resolveConfig, + deleteObject, + type VoorzieningenConfig, +} from '../workflows/_fixtures' + +const MODULE_NAME = `Review subject ${RUN_ID}` +/** Unique per test so the moderation queue row this test acts on is its own. */ +const title = (suffix: string): string => `Review ${suffix} ${RUN_ID}` + +let ctx: APIRequestContext +let config: VoorzieningenConfig +// OpenRegister's object API returns the object's UUID as `id` (verified on the +// live instance: `id` === `@self.id` === a v4 uuid; there is no separate +// `uuid` key). The detail ROUTE takes that same value, so `createObject`'s +// return value is directly the deep-link segment — no lookup needed. +let moduleUuid = '' + +test.beforeAll(async () => { + ctx = await newApiContext() + config = await resolveConfig(ctx) + moduleUuid = await createObject(ctx, config.register, 'module', { naam: MODULE_NAME }) +}) + +test.afterAll(async () => { + if (!ctx || !config) return + for (const schema of ['beoordeeling', 'module']) { + const rows = await findAll(ctx, config.register, schema) + for (const row of rows) { + if (JSON.stringify(row).includes(RUN_ID)) { + const id = String(row.id ?? (row as { '@self'?: { id?: string } })['@self']?.id ?? '') + if (id) await deleteObject(ctx, config.register, schema, id) + } + } + } + await ctx.dispose() +}) + +/** + * A genuinely UNAUTHENTICATED APIRequestContext. + * + * ⚠️ `request.newContext({ baseURL })` is NOT anonymous under this config. + * Playwright merges the project's `use` options into the new context, and this + * project sets `use.storageState` to the admin cookie jar — so a context + * created "with no credentials" silently runs AS ADMIN. Measured on this rig: + * `GET /ocs/v2.php/cloud/user` from such a context returned **200 with + * `"id":"admin"`**, and an "anonymous" `POST /api/reviews` returned **202 + * Accepted**. Passing `storageState: undefined` explicitly returns **401 + * `Current user is not logged in`** for both. + * + * This is a control that fails silently in the dangerous direction: a test + * asserting "unauthenticated is rejected" would be firing an AUTHENTICATED + * request and could only pass by accident. It did, briefly — an earlier draft + * of this file "passed" on a 400 `empty payload` that had nothing to do with + * authentication at all. + * + * So the helper asserts its own premise before returning: if the context is + * not anonymous, it fails here rather than letting a caller draw a conclusion + * from it. + */ +async function newAnonymousContext(): Promise { + const anon = await playwrightRequest.newContext({ + baseURL: BASE_URL, + storageState: undefined, + extraHTTPHeaders: {}, + }) + const whoami = await anon.get('/ocs/v2.php/cloud/user?format=json', { + headers: { 'OCS-APIREQUEST': 'true' }, + }) + expect( + whoami.status(), + `the "anonymous" context is authenticated (whoami returned ${whoami.status()}) — ` + + 'every anonymous assertion made through it would be meaningless', + ).toBe(401) + return anon +} + +/** Open the seeded module's detail page and wait for the reviews panel. */ +async function openModuleReviews(page: Page): Promise { + await page.goto(`${APP_BASE}#/modules/${moduleUuid}`, { waitUntil: 'domcontentloaded' }) + await page.locator(APP_SHELL).first().waitFor({ state: 'attached', timeout: 30000 }) + await page.locator(APP_MAIN).first().waitFor({ state: 'visible', timeout: 30000 }) + await dismissSupportDialog(page) + await dismissWalkthrough(page) + await expect(page.locator('.reviews-panel').first()).toBeVisible({ timeout: 30000 }) +} + +/** Submit a review through the real modal. */ +async function submitReview(page: Page, reviewTitle: string, rating: string): Promise { + await page.getByRole('button', { name: 'Write a review', exact: true }).first().click() + const dialog = page.getByRole('dialog').filter({ hasText: 'Write a review' }).first() + await expect(dialog).toBeVisible({ timeout: 15000 }) + await dialog.getByRole('textbox', { name: /^Title/ }).first().fill(reviewTitle) + const rater = dialog.locator('.vs__search').first() + await rater.click() + await page.locator('.vs__dropdown-option', { hasText: new RegExp(`^${rating}$`) }).first().click() + await dialog.getByRole('textbox', { name: /^Testimonial/ }).first().fill(`Body for ${reviewTitle}`) + await dialog.getByRole('button', { name: 'Submit review', exact: true }).click() + await expect(dialog).toBeHidden({ timeout: 30000 }) +} + +/** Open the admin settings page and wait for the review moderation queue. */ +async function openReviewQueue(page: Page) { + await page.goto('/index.php/settings/admin/softwarecatalog', { waitUntil: 'domcontentloaded' }) + const queue = page.locator('section, .settings-section') + .filter({ hasText: 'Review moderation' }).first() + await expect(queue).toBeVisible({ timeout: 30000 }) + return queue +} + +/** Act on one named row in a moderation queue. */ +async function moderate(page: Page, reviewTitle: string, action: 'Approve' | 'Reject'): Promise { + const queue = await openReviewQueue(page) + const row = queue.locator('li.moderation-item').filter({ hasText: reviewTitle }).first() + await expect(row, `no pending queue row titled "${reviewTitle}"`).toBeVisible({ timeout: 30000 }) + await row.getByRole('button', { name: action, exact: true }).click() + // The row leaves the pending queue once the verdict lands. + await expect(row).toBeHidden({ timeout: 30000 }) +} + +// @e2e catalog-ratings::an-authenticated-catalog-user-can-submit-a-review +// @e2e catalog-ratings::a-submission-lands-pending-and-is-not-yet-public +test('reviews: an authenticated submission lands pending and is not yet public', async ({ page }) => { + const bag = collectAppErrors(page) + const reviewTitle = title('pending') + + await openModuleReviews(page) + await submitReview(page, reviewTitle, '8') + + // NOT yet public: the panel re-fetches the aggregate on submit, and the + // new review must not be in it. + const panel = page.locator('.reviews-panel').first() + await expect(panel.locator('.reviews-panel__list').getByText(reviewTitle)).toHaveCount(0) + + // It IS waiting in the admin moderation queue. + const queue = await openReviewQueue(page) + const row = queue.locator('li.moderation-item').filter({ hasText: reviewTitle }).first() + await expect(row, `submitted review is not in the pending queue`).toBeVisible({ timeout: 30000 }) + + // NOTE: the queue row shows only the title. `moderationItemSubtitle()` + // (src/utils/moderationItem.js) picks its subtitle from + // email/contactEmail/url/website/beschrijving/description — a `beoordeeling` + // carries none of those, so the subtitle is ''. The server-bound author is + // therefore NOT observable here; it is asserted on the Reviews index below, + // which does render the `auteur` column. + + expectNoAppErrors(bag) +}) + +// @e2e catalog-ratings::admin-approval-makes-the-review-public +// @e2e catalog-ratings::aggregate-reflects-only-approved-reviews +// @e2e catalog-ratings::an-approved-review-is-publicly-readable +test('reviews: admin approval publishes the review and moves the aggregate', async ({ page }) => { + const bag = collectAppErrors(page) + const reviewTitle = title('approve') + + await openModuleReviews(page) + // Before: this module has no approved review, so the average renders "—". + await expect(page.locator('.reviews-panel__average-value--empty')).toBeVisible() + + await submitReview(page, reviewTitle, '9') + await moderate(page, reviewTitle, 'Approve') + + // After approval the panel lists it and the aggregate counts it. + await openModuleReviews(page) + const panel = page.locator('.reviews-panel').first() + await expect(panel.locator('.reviews-panel__list').getByText(reviewTitle).first()) + .toBeVisible({ timeout: 30000 }) + await expect(panel.locator('.reviews-panel__average-value').first()).toContainText('9') + await expect(panel.locator('.reviews-panel__count')).toContainText('1 review') + + // Publicly readable: the aggregate endpoint is #[PublicPage], so an + // ANONYMOUS context (no credentials at all) must see the approved review. + const anon = await newAnonymousContext() + const res = await anon.get( + `/index.php/apps/softwarecatalog/api/reviews/aggregate?subjectType=module&subjectId=${moduleUuid}`, + ) + expect(res.status(), `anonymous aggregate returned ${res.status()}`).toBe(200) + const body = await res.json() + expect(JSON.stringify(body), 'approved review absent from the anonymous aggregate') + .toContain(reviewTitle) + await anon.dispose() + + expectNoAppErrors(bag) +}) + +// @e2e catalog-ratings::admin-rejection-keeps-the-review-hidden +// @e2e catalog-ratings::a-rejected-review-is-not-publicly-readable +// @e2e catalog-ratings::a-pending-review-is-not-publicly-readable +test('reviews: a rejected review stays hidden, and so does a pending one', async ({ page }) => { + const bag = collectAppErrors(page) + const rejected = title('reject') + const pending = title('stillpending') + + // The REJECTED one is submitted through the real modal — that is the leg + // this test is about. The still-PENDING one is seeded through the API: + // it is a fixture (a second row that must stay invisible), not the + // behaviour under test, and driving the modal twice in one page session + // re-opens it with the previous submission's state still in the form. + const seeded = await ctx.post('/index.php/apps/softwarecatalog/api/reviews', { + data: { + review: { naam: pending, waardering: 3, beschrijvingLang: 'Still pending' }, + subjectType: 'module', + subjectId: moduleUuid, + }, + }) + expect(seeded.status(), `seeding the pending review returned ${seeded.status()}`).toBeLessThan(300) + + await openModuleReviews(page) + await submitReview(page, rejected, '2') + + await moderate(page, rejected, 'Reject') + + // Neither the rejected nor the still-pending review is in the panel. + await openModuleReviews(page) + const panel = page.locator('.reviews-panel').first() + await expect(panel.getByText(rejected)).toHaveCount(0) + await expect(panel.getByText(pending)).toHaveCount(0) + + // Nor in the anonymous (public) aggregate. + const anon = await newAnonymousContext() + const res = await anon.get( + `/index.php/apps/softwarecatalog/api/reviews/aggregate?subjectType=module&subjectId=${moduleUuid}`, + ) + expect(res.status(), `anonymous aggregate returned ${res.status()}`).toBe(200) + const text = await res.text() + expect(text, 'rejected review leaked into the public aggregate').not.toContain(rejected) + expect(text, 'pending review leaked into the public aggregate').not.toContain(pending) + await anon.dispose() + + expectNoAppErrors(bag) +}) + +// @e2e catalog-ratings::aggregate-with-no-approved-reviews +test('reviews: a module with no approved reviews shows the empty aggregate, not a zero score', async ({ page }) => { + const bag = collectAppErrors(page) + // A module of its own, so no other test's approval can reach it. + const isolatedName = `Unreviewed module ${RUN_ID}` + const uuid = await createObject(ctx, config.register, 'module', { naam: isolatedName }) + expect(uuid, 'isolated module fixture has no uuid').not.toBe('') + + await page.goto(`${APP_BASE}#/modules/${uuid}`, { waitUntil: 'domcontentloaded' }) + await page.locator(APP_MAIN).first().waitFor({ state: 'visible', timeout: 30000 }) + await dismissSupportDialog(page) + await dismissWalkthrough(page) + + const panel = page.locator('.reviews-panel').first() + await expect(panel).toBeVisible({ timeout: 30000 }) + // The average is the em-dash placeholder — NOT "0/10". + await expect(panel.locator('.reviews-panel__average-value--empty')).toBeVisible() + await expect(panel.locator('.reviews-panel__average-value')).not.toContainText('0/10') + await expect(panel.getByText('No reviews yet')).toBeVisible() + + expectNoAppErrors(bag) +}) + +// @e2e catalog-ratings::an-admin-moderates-pending-reviews-through-the-existing-queue-ui +// @e2e catalog-ratings::the-default-unparameterised-organisatie-moderation-path-is-unchanged +test('reviews: moderation reuses the ONE queue component, and the organisatie queue is unchanged', async ({ page }) => { + const bag = collectAppErrors(page) + await page.goto('/index.php/settings/admin/softwarecatalog', { waitUntil: 'domcontentloaded' }) + + // The review queue exists… + const reviewQueue = page.locator('section, .settings-section') + .filter({ hasText: 'Review moderation' }).first() + await expect(reviewQueue).toBeVisible({ timeout: 30000 }) + // …rendered by the SAME component as the organisatie queue: both expose the + // component's own "Refresh queue" affordance and its `moderation-list`/ + // empty-state markup. A second, bespoke review-moderation mechanism would + // not carry these. + await expect(reviewQueue.getByRole('button', { name: /Refresh queue/i }).first()) + .toBeVisible() + + // The DEFAULT (organisatie) queue still renders alongside it, untouched. + const orgQueue = page.locator('section, .settings-section') + .filter({ hasText: /Organisation moderation|Organisatie moderation|Pending registrations/i }) + .first() + await expect(orgQueue, 'the default organisatie moderation queue disappeared') + .toBeVisible({ timeout: 30000 }) + await expect(orgQueue.getByRole('button', { name: /Refresh queue/i }).first()) + .toBeVisible() + + // And the unparameterised endpoint still answers for organisatie. + const res = await ctx.get('/index.php/apps/softwarecatalog/api/moderation/pending') + expect(res.status(), `default moderation/pending returned ${res.status()}`).toBe(200) + + expectNoAppErrors(bag) +}) + +// @e2e catalog-ratings::every-configured-column-resolves-to-a-real-schema-property +// @e2e catalog-ratings::the-stored-review-carries-the-authenticated-users-name +test('reviews index: each configured column renders this row\'s real value', async ({ page }) => { + const bag = collectAppErrors(page) + + // ⚠️ Asserting the column HEADINGS would test the manifest, not the render: + // a column naming a property the `beoordeeling` schema does not have still + // renders its heading perfectly — it is the CELL that comes back empty. + // (That is exactly the failure the spec names: the page was configured with + // `titel`/`score`/`datum`, none of which exist on the schema.) So this test + // seeds a row with a DISTINCT value per configured column and asserts each + // value reaches the page. + const reviewTitle = title('columns') + const created = await ctx.post('/index.php/apps/softwarecatalog/api/reviews', { + data: { + review: { naam: reviewTitle, waardering: 6, beschrijvingLang: 'Column probe' }, + subjectType: 'module', + subjectId: moduleUuid, + }, + }) + expect(created.status(), `seed POST /api/reviews returned ${created.status()}: ${await created.text()}`) + .toBeLessThan(300) + + await navClickTo(page, 'Reviews') + const main = page.locator(APP_MAIN).first() + await expect(main).toBeVisible({ timeout: 30000 }) + + // Located BY THIS RUN'S UNIQUE TITLE, so a pre-existing review cannot + // satisfy the assertion — no search box interaction needed, and none is + // used: a search control whose selector drifts would turn a real coverage + // failure into a locator timeout. + const row = main.locator('tr, li, [class*="card"]').filter({ hasText: reviewTitle }).first() + await expect(row, `no Reviews row for "${reviewTitle}"`).toBeVisible({ timeout: 30000 }) + + // `naam` — the row's own title. `auteur` — bound server-side to the session + // user. `waardering` — the rating we sent. `status` — `pending`, since the + // submission has not been moderated. All four are the manifest's configured + // columns; a key that resolved to nothing would leave its value absent. + await expect(row).toContainText(reviewTitle) + await expect(row).toContainText(/admin/i) + await expect(row).toContainText('6') + await expect(row).toContainText(/pending/i) + + expectNoAppErrors(bag) +}) + +// @e2e catalog-ratings::an-unauthenticated-request-cannot-create-a-review +test('reviews: an anonymous POST cannot create a review', async () => { + // No UI path exists for this: the "Write a review" button is only rendered + // inside the authenticated SPA. Asserted at the HTTP layer for that reason. + const anon = await newAnonymousContext() + // Body shape mirrors the modal's own `buildReviewSubmission()`: + // `{ review: {...}, subjectType, subjectId }` — the controller signature is + // `submit(array $review, string $subjectType, string $subjectId)`, so a + // FLAT body is rejected with `{"message":"empty payload"}` before auth is + // ever considered, which would make this test pass for the wrong reason. + const res = await anon.post('/index.php/apps/softwarecatalog/api/reviews', { + data: { + review: { naam: `Anon review ${RUN_ID}`, waardering: 10 }, + subjectType: 'module', + subjectId: moduleUuid, + }, + headers: { 'OCS-APIREQUEST': 'true' }, + }) + expect( + res.status(), + `anonymous POST /api/reviews returned ${res.status()} — expected 401/403`, + ).toBeGreaterThanOrEqual(400) + expect(res.status()).toBeLessThan(500) + await anon.dispose() + + // And nothing was persisted. + const rows = await findAll(ctx, config.register, 'beoordeeling', `Anon review ${RUN_ID}`) + expect( + rows.filter(r => String(r.naam ?? '').includes('Anon review')).length, + 'an anonymous request created a review object', + ).toBe(0) +}) + +// @e2e catalog-ratings::a-client-supplied-author-is-ignored +test('reviews: a client-supplied auteur/status is stripped, not stored', async () => { + // No UI path: SubmitReviewModal never sends `auteur` or `status`, so this + // payload cannot be produced by any sequence of clicks. + const reviewTitle = title('rogue') + const res = await ctx.post('/index.php/apps/softwarecatalog/api/reviews', { + data: { + review: { + naam: reviewTitle, + waardering: 7, + // The rogue fields. Neither is sent by SubmitReviewModal. + auteur: 'Someone Else Entirely', + status: 'approved', + }, + subjectType: 'module', + subjectId: moduleUuid, + }, + }) + expect(res.status(), `POST /api/reviews returned ${res.status()}: ${await res.text()}`).toBeLessThan(300) + + const rows = await findAll(ctx, config.register, 'beoordeeling', reviewTitle) + const stored = rows.find(r => String(r.naam ?? '') === reviewTitle) + expect(stored, 'the review was not persisted at all').toBeTruthy() + // The client-supplied author was IGNORED — the session identity won. + expect(String(stored?.auteur ?? ''), 'client-supplied auteur was stored') + .not.toBe('Someone Else Entirely') + // …and the client-supplied `approved` status was ignored too. + expect(String(stored?.status ?? '')).toBe('pending') +}) diff --git a/tests/e2e/spec-coverage/suite-wizard.spec.ts b/tests/e2e/spec-coverage/suite-wizard.spec.ts new file mode 100644 index 00000000..0f2e68be --- /dev/null +++ b/tests/e2e/spec-coverage/suite-wizard.spec.ts @@ -0,0 +1,293 @@ +// SPDX-License-Identifier: EUPL-1.2 +// SPDX-FileCopyrightText: 2026 Conduction B.V. +/** + * Behavioural e2e coverage for the guided suite-creation wizard. + * + * Components under test: + * src/views/suites/SuitesIndexView.vue (manifest page `Suites`, /suites) + * src/dialogs/SuiteWizardDialog.vue (CnWizardDialog host) + * src/dialogs/SuiteWizard/Step1Details.vue + * src/dialogs/SuiteWizard/Step2Applications.vue + * src/dialogs/SuiteWizard/Step3Confirm.vue + * + * Everything here is asserted against RENDERED DOM after real clicks. The two + * `module` objects the wizard attaches are seeded through the OpenRegister + * object API in `beforeAll` and removed in `afterAll` — fixture setup may use + * the API, assertions may not (the gate-19 honest-coverage rule). + * + * DOM handles come from @conduction/nextcloud-vue's CnWizardDialog itself, not + * from text we could accidentally match elsewhere on the page: + * [data-testid-modal="cn-wizard-dialog"] the dialog + * [data-testid-phase="form"|"result"] wizard phase vs. result phase + * [data-step-id=""] the CURRENT step's body + * `data-step-id` is the load-bearing one: it is rendered on the single mounted + * step body, so asserting it equals `details` is a positive statement about + * which step is showing, not merely that a details field exists somewhere. + * + * @spec openspec/specs/suite-wizard/spec.md + */ +import { test, expect, type Page } from '@playwright/test' +import { APP_MAIN, collectAppErrors, expectNoAppErrors, navClickTo } from './_helpers' +import { + RUN_ID, + cleanupByToken, + createObject, + newApiContext, + resolveConfig, + type VoorzieningenConfig, +} from '../workflows/_fixtures' +import type { APIRequestContext } from '@playwright/test' + +const APP_A = `Suite member A ${RUN_ID}` +const APP_B = `Suite member B ${RUN_ID}` +const SUITE_NAME = `Centric Leefomgeving ${RUN_ID}` +const SUITE_SHORT = 'Bundled leefomgeving product' + +let ctx: APIRequestContext +let config: VoorzieningenConfig + +test.beforeAll(async () => { + ctx = await newApiContext() + config = await resolveConfig(ctx) + // Two REAL modules for the picker to offer. `module` is resolved by slug + // (not by a `*_schema` config key) exactly as the wizard itself does. + await createObject(ctx, config.register, 'module', { naam: APP_A }) + await createObject(ctx, config.register, 'module', { naam: APP_B }) +}) + +test.afterAll(async () => { + if (ctx && config) { + // `suite` is not in cleanupByToken's schema list, so remove it here. + const res = await ctx.get( + `/index.php/apps/openregister/api/objects/${config.register}/suite?_limit=500`, + ) + if (res.ok()) { + const rows = (await res.json())?.results ?? [] + for (const row of rows) { + if (JSON.stringify(row).includes(RUN_ID)) { + const id = String(row.id ?? row['@self']?.id ?? '') + if (id) { + await ctx.delete( + `/index.php/apps/openregister/api/objects/${config.register}/suite/${id}`, + ) + } + } + } + } + await cleanupByToken(ctx, config, RUN_ID) + await ctx.dispose() + } +}) + +/** The wizard dialog, scoped so nothing outside it can satisfy an assertion. */ +function wizard(page: Page) { + return page.locator('[data-testid-modal="cn-wizard-dialog"]').first() +} + +/** Open the Suites page and launch the wizard via the real "New suite" button. */ +async function openWizard(page: Page): Promise { + await navClickTo(page, 'Suites') + await page.locator(APP_MAIN).first() + .getByRole('button', { name: 'New suite', exact: true }).first().click() + await expect(wizard(page)).toBeVisible({ timeout: 30000 }) +} + +/** + * Fill the details step with valid values so `Next` is not blocked by it. + * + * Scoped to `.suite-wizard-step1` rather than to a global accessible name: + * "Name" is far too common a label to match uniquely across a dialog, and the + * component renders the label as `Name *` (the asterisk is part of the string, + * not a separate required marker), so an exact-name lookup would miss it. + */ +async function fillDetails(page: Page): Promise { + const step = wizard(page).locator('.suite-wizard-step1') + await step.getByRole('textbox', { name: /^Name/ }).first().fill(SUITE_NAME) + await step.getByRole('textbox', { name: /^Short description/ }).first().fill(SUITE_SHORT) +} + +/** Attach one seeded module through the real NcSelect multi-picker. */ +async function attachApplication(page: Page, name: string): Promise { + const w = wizard(page) + const picker = w.locator('.suite-wizard-step2 .vs__search').first() + await picker.click() + await picker.fill(name) + // The option list is teleported to body by vue-select, so it is queried on + // the page rather than inside the dialog. + await page.locator('.vs__dropdown-option', { hasText: name }).first() + .click({ timeout: 30000 }) +} + +// @e2e suite-wizard::opening-the-wizard-starts-on-the-details-step +test('suite wizard: opens on the details step showing all three step labels', async ({ page }) => { + const bag = collectAppErrors(page) + await openWizard(page) + + const w = wizard(page) + // The wizard phase is showing (not the post-submit result phase). + await expect(w.locator('[data-testid-phase="form"]')).toBeVisible() + + // `data-step-id` is rendered on the ONE mounted step body, so this asserts + // which step is current — not merely that a details control exists. + await expect(w.locator('.cn-wizard-dialog__step-body')).toHaveAttribute('data-step-id', 'details') + + // The progress indicator lists exactly the three specified steps, in order. + const labels = w.locator('.cn-wizard-dialog__progress-label') + await expect(labels).toHaveCount(3) + await expect(labels).toHaveText(['Details', 'Applications', 'Confirm']) + + expectNoAppErrors(bag) +}) + +// @e2e suite-wizard::the-applications-step-only-offers-modules-that-already-exist +test('suite wizard: the applications step offers existing modules and no create control', async ({ page }) => { + const bag = collectAppErrors(page) + await openWizard(page) + await fillDetails(page) + + const w = wizard(page) + await w.getByRole('button', { name: 'Next', exact: true }).click() + await expect(w.locator('.cn-wizard-dialog__step-body')).toHaveAttribute('data-step-id', 'applications') + + // The picker offers a module that genuinely exists in the register — the + // one this run seeded. If the step invented its own options, or fetched + // nothing, this fails. + const picker = w.locator('.suite-wizard-step2 .vs__search').first() + await picker.click() + await picker.fill(APP_A) + await expect(page.locator('.vs__dropdown-option', { hasText: APP_A }).first()) + .toBeVisible({ timeout: 30000 }) + + // AND there is no control to create a new module from this step. Asserted + // over the step body, so a "New suite"/"Add" button elsewhere in the app + // chrome cannot accidentally satisfy — or accidentally fail — it. + const stepBody = w.locator('.cn-wizard-dialog__step-body') + await expect(stepBody.getByRole('button', { name: /new|add|create/i })).toHaveCount(0) + + expectNoAppErrors(bag) +}) + +// @e2e suite-wizard::advancing-with-zero-applications-is-blocked +test('suite wizard: Next with zero applications is blocked and explains why', async ({ page }) => { + const bag = collectAppErrors(page) + await openWizard(page) + await fillDetails(page) + + const w = wizard(page) + await w.getByRole('button', { name: 'Next', exact: true }).click() + await expect(w.locator('.cn-wizard-dialog__step-body')).toHaveAttribute('data-step-id', 'applications') + + // Click Next with nothing selected. + await w.getByRole('button', { name: 'Next', exact: true }).click() + + // It MUST NOT advance… + await expect(w.locator('.cn-wizard-dialog__step-body')).toHaveAttribute('data-step-id', 'applications') + // …and MUST say at least one application is required. + await expect( + w.getByText(/at least one existing application/i).first(), + ).toBeVisible({ timeout: 15000 }) + + expectNoAppErrors(bag) +}) + +// @e2e suite-wizard::advancing-with-one-or-more-applications-succeeds +test('suite wizard: Next with an application attached reaches confirm and lists it', async ({ page }) => { + const bag = collectAppErrors(page) + await openWizard(page) + await fillDetails(page) + + const w = wizard(page) + await w.getByRole('button', { name: 'Next', exact: true }).click() + await attachApplication(page, APP_A) + await w.getByRole('button', { name: 'Next', exact: true }).click() + + await expect(w.locator('.cn-wizard-dialog__step-body')).toHaveAttribute('data-step-id', 'confirm') + // The confirm step lists the selected application BY NAME. + await expect(w.locator('.suite-wizard-step3__apps').getByText(APP_A, { exact: true })) + .toBeVisible({ timeout: 15000 }) + + expectNoAppErrors(bag) +}) + +// @e2e suite-wizard::successful-submission-creates-the-suite-with-its-members +test('suite wizard: submit creates the suite with both attached modules', async ({ page }) => { + const bag = collectAppErrors(page) + await openWizard(page) + await fillDetails(page) + + const w = wizard(page) + await w.getByRole('button', { name: 'Next', exact: true }).click() + await attachApplication(page, APP_A) + await attachApplication(page, APP_B) + await w.getByRole('button', { name: 'Next', exact: true }).click() + await expect(w.locator('.cn-wizard-dialog__step-body')).toHaveAttribute('data-step-id', 'confirm') + + await w.getByRole('button', { name: 'Create suite', exact: true }).click() + + // ⚠️ PARTIAL COVERAGE — DISCLOSED, NOT PAPERED OVER. + // + // The scenario has three THENs. This test proves the first two (the suite + // is created; `applicaties` holds both module ids). It does NOT prove the + // third — "AND the wizard MUST show a success result" — because that + // result is IMPOSSIBLE to observe in the shipped build, and the assertion + // was removed only after establishing why: + // + // SuiteWizardDialog.onSubmit() calls `wizard.setResult({success:true})` + // and then `$emit('created')`. SuitesIndexView.onSuiteCreated() responds + // by setting `showWizard = false`, and the dialog is rendered under + // `v-if="show"` — so it UNMOUNTS in the same synchronous tick in which + // the result phase was set. `[data-testid-phase="result"]` is never + // committed to the DOM. This is deterministic, not a race: both updates + // land before the next render flush. + // + // Measured: the first version of this test waited the full 30s for + // `[data-testid-phase="result"]` and the page snapshot at failure showed + // no dialog in the DOM at all. Filed as a product defect; the success + // banner the spec requires is not reachable by any user either. + // + // What a user DOES see on success is the navigation to the new suite's + // detail page, so that is asserted instead — a real, observable outcome + // rather than a weakened one. + await expect(page).toHaveURL(/#\/suites\/[^/]+$/, { timeout: 30000 }) + + // The suite really was persisted, with BOTH modules in `applicaties`. + // Read back through the register the UI wrote to. + const res = await ctx.get( + `/index.php/apps/openregister/api/objects/${config.register}/suite?_limit=500`, + ) + expect(res.status(), `suite collection read: ${res.status()}`).toBe(200) + const rows: Array> = (await res.json())?.results ?? [] + const created = rows.find(r => String(r.naam ?? '') === SUITE_NAME) + expect(created, `no suite named "${SUITE_NAME}" was persisted`).toBeTruthy() + expect(String(created?.beschrijvingKort ?? '')).toBe(SUITE_SHORT) + // `applicaties` holds the attached modules' ids — two of them. + const attached = created?.applicaties as unknown[] | undefined + expect(Array.isArray(attached), 'applicaties is not an array').toBe(true) + expect(attached?.length, `applicaties = ${JSON.stringify(attached)}`).toBe(2) + + expectNoAppErrors(bag) +}) + +// @e2e suite-wizard::the-suites-nav-entry-opens-the-suite-index +test('suite wizard: the Suites nav entry opens the suite index listing suites', async ({ page }) => { + const bag = collectAppErrors(page) + // Reached by CLICKING the real nav entry, not by deep-linking. + await navClickTo(page, 'Suites') + + const main = page.locator(APP_MAIN).first() + await expect(main).toBeVisible({ timeout: 30000 }) + await expect(main.getByRole('heading', { name: 'Suites', exact: true }).first()) + .toBeVisible({ timeout: 30000 }) + + // The list body mounted — the "Showing N of M" header, or the empty state. + // Proves the self-fetch against register=voorzieningen/schema=suite ran. + const populated = main.getByText(/Showing\s+\d+\s+of\s+\d+/i).first() + const empty = main.getByText('No items found', { exact: false }).first() + await expect(populated.or(empty)).toBeVisible({ timeout: 30000 }) + + // The guided wizard is the page's primary creation action. + await expect(main.getByRole('button', { name: 'New suite', exact: true }).first()) + .toBeVisible() + + expectNoAppErrors(bag) +}) From c06bb09a1b5b09200bff448e286040b05b468fe0 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 19:23:27 +0200 Subject: [PATCH 2/3] test(e2e): 5 facet-contract tests; defer the 4 exclusions so this PR stays honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit on this branch. Two changes: 1. ADDS tests/e2e/spec-coverage/gemma-faceted-search.spec.ts — 5 tests covering the facet CONTRACT and the panel's presence: - all four GEMMA dimensions present, empty ones as [] not omitted - an unsupported schema is rejected 400, naming the supported set - a text query narrows the aggregated set - a repeated identical request is served from cache; a different one is not - the GEMMA panel renders beside the still-working free-text search box 2. REVERTS the four @e2e exclude markers added to openspec/specs/gemma-faceted-search/spec.md in the previous commit. WHY THE EXCLUSIONS ARE BEING DEFERRED Editing that spec file pulls ALL 25 of its scenarios into this PR's diff scope (gate-19 scopes by FILE, not by hunk). CI proved it: the previous commit made this PR's own gate-19 report FAIL - 21, reproduced locally byte-identical. Two of those 25 are unimplemented outright (#483: empty facet dimensions are never disabled; the facet cache is TTL-only with no invalidation path). Both are browser-observable, so neither can honestly carry an `@e2e exclude` — an exclusion asserts "a browser cannot observe this", and for these two that sentence is false. And no passing test can be written against behaviour that does not exist. So no honest version of this PR can be green while it edits that file. The four exclusions — which are good, scenario-level, and individually reasoned — land together with the #483 fixes instead. Cost: 4 scenarios of headline number. Benefit: the gate keeps meaning what it says. With the spec file untouched, gate-19 for this PR SKIPs as `na` ("the diff touched NO spec file"). That is not a pass and is not claimed as one; the honest figure remains the development -> beta scope. beta scope: 137 -> 111 (26 scenarios, 19 test cases, 0 exclusions) WHAT THE FACET FIXTURES CANNOT DO, AND WHY THAT IS RECORDED The facet VALUE scenarios (per-dimension counts, OR-within / AND-across, URL round-tripping of a selection, saved views) are NOT claimed. POSTing a module with {"standaardVersies":["x"],"referentieComponenten":["y"]} returns 200 with BOTH arrays silently emptied — they are relation fields and OpenRegister drops bare strings without erroring. Seeding them needs real `element` objects plus `relation` rows. A facet test seeded with data the product cannot produce would prove nothing, so those scenarios stay uncovered and counted. `no-text-query-returns-facets-over-the-full-rbac-scoped-set` is also left uncovered on purpose: it needs the unparameterised aggregate, which is the exact entry the cache never invalidates. Measured — findAll saw 3 modules while the endpoint answered {"cached":true,"totalMatched":1}. The three available ways to make it green (wait out the 1800s TTL, assert >= instead of ==, or add a cache-busting param and test a different scenario) were all rejected. Refs #483 --- openspec/specs/gemma-faceted-search/spec.md | 8 - .../gemma-faceted-search.spec.ts | 226 ++++++++++++++++++ 2 files changed, 226 insertions(+), 8 deletions(-) create mode 100644 tests/e2e/spec-coverage/gemma-faceted-search.spec.ts diff --git a/openspec/specs/gemma-faceted-search/spec.md b/openspec/specs/gemma-faceted-search/spec.md index 88032f7c..9940476c 100644 --- a/openspec/specs/gemma-faceted-search/spec.md +++ b/openspec/specs/gemma-faceted-search/spec.md @@ -74,8 +74,6 @@ Every OpenRegister `searchObjects()` (or equivalent aggregate) call issued by th #### Scenario: Facet aggregation query sets an explicit limit -@e2e exclude The subject is a PHP array key (`$pagedQuery['_limit']`) that exists only between `FacetService` and `ObjectService` inside one request; no browser or HTTP response ever carries it. Verified against the live endpoint rather than assumed: `GET /apps/softwarecatalog/api/facets/module` returns `_meta` = `{totalMatched, processingTimeMs, cached, matchedObjectIds}` — the query array is not echoed in any form, so there is nothing a Playwright assertion could read. Covered instead by a unit test on `FacetService`'s query construction, which can inspect the array directly. - - GIVEN `FacetService` builds a query to aggregate `referentiecomponent` values across the `module` schema - WHEN the query array is constructed - THEN it MUST include an explicit `_limit` value @@ -83,8 +81,6 @@ Every OpenRegister `searchObjects()` (or equivalent aggregate) call issued by th #### Scenario: A register too large for one bounded page pages instead of scanning unbounded -@e2e exclude Distinguishes "paged through `searchObjectsPaginated()`" from "one unbounded `searchObjects()`" — two implementations that produce the BYTE-IDENTICAL response. Verified: the endpoint's `_meta` (`totalMatched, processingTimeMs, cached, matchedObjectIds`) discloses no paging state, and the overflow path's only external signal is a `LoggerInterface::warning` line. A browser cannot read a log file, and a response that is the same either way cannot be asserted on. Covered by a unit test that counts calls to the paginated verb. - - GIVEN the `module` register has more objects than fit in one bounded facet aggregation page - WHEN facet counts are computed - THEN the service MUST page through results via `searchObjectsPaginated()` (or a documented `_limit` ceiling) to reach a complete count @@ -103,8 +99,6 @@ Facet aggregation SHALL count only objects the requesting user is authorized to #### Scenario: Facet aggregation uses the same authorization path as the object list -@e2e exclude Asserts CODE-PATH IDENTITY ("the identical scoping ... MUST NOT use a separate, unscoped counting code path"), not an outcome. Two code paths that happen to agree on every observable output are indistinguishable from one shared path by any black-box observation, so no browser assertion can decide this proposition — even a perfect test of the OUTCOME would leave the requirement unproven, which is why this is not merely "hard to test". The observable half (a restricted user's counts reflect only their own scope) is a SEPARATE scenario in this same requirement and is NOT excluded. - - GIVEN the module index page's own object list query is scoped by RBAC/organisation context - WHEN the facet aggregation query is built - THEN it MUST apply the identical RBAC/tenant scoping as the object list query @@ -216,8 +210,6 @@ All facet dimension labels, facet value display strings sourced from the UI laye #### Scenario: Translation keys are in English -@e2e exclude The subject is the KEY side of the `l10n/*.json` source files. A browser is only ever served the resolved VALUE — by the time any string reaches the DOM the key has been substituted away, so a rendered page is identical whether the key was `facetSaveAsView` or a Dutch literal. This is a repository-file property, enforceable only by reading `l10n/` (which the sibling scenario "Facet panel renders in the user's selected language" does NOT substitute for: that one asserts the values). - - GIVEN the softwarecatalog `l10n` translation files - WHEN the facet panel's translation keys are inspected - THEN each key MUST be an English identifier (e.g. `facetSaveAsView`), not a Dutch string, with the Dutch translation supplied as the `nl` value diff --git a/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts b/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts new file mode 100644 index 00000000..905712e2 --- /dev/null +++ b/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts @@ -0,0 +1,226 @@ +// SPDX-License-Identifier: EUPL-1.2 +// SPDX-FileCopyrightText: 2026 Conduction B.V. +/** + * Behavioural e2e coverage for GEMMA faceted search. + * + * Surfaces under test: + * lib/Controller/FacetController.php GET /api/facets/{schema} + * lib/Service/FacetService.php aggregation + per-caller cache + * src/views/FacetedCatalogIndexView.vue manifest pages `Modules` (/modules) + * and `Diensten` (/diensten) + * + * ⚠️ SCOPE OF THIS FILE — READ BEFORE ADDING TO IT. + * + * The scenarios about facet VALUES (counts per referentiecomponent, OR-within- + * a-dimension, AND-across-dimensions, URL round-tripping of a selection, saved + * views) are NOT covered here, and not because they are hard: the fixtures + * cannot be built through the object API. Measured on a live instance — + * POSTing a module with + * {"standaardVersies":["StUF-ZKN-x"],"referentieComponenten":["RC-x"]} + * returns 200 with BOTH arrays silently emptied (`"standaardVersies":[]`, + * `"referentieComponenten":[]`). They are relation fields; OpenRegister drops + * bare strings without erroring. Seeding them needs real `element` objects in + * the AMEF register plus `relation` rows, which is a fixture layer this file + * deliberately does not fake — a facet test seeded with data the product + * cannot produce would prove nothing. + * + * So this file covers the facet CONTRACT and the panel's presence, and leaves + * the value-dependent scenarios uncovered and counted. That is the honest + * split; see the PR for the two scenarios that are unimplemented outright. + * + * @spec openspec/specs/gemma-faceted-search/spec.md + */ +import { test, expect } from '@playwright/test' +import type { APIRequestContext } from '@playwright/test' +import { APP_MAIN, collectAppErrors, expectNoAppErrors, navClickTo } from './_helpers' +import { + RUN_ID, + createObject, + deleteObject, + newApiContext, + resolveConfig, + type VoorzieningenConfig, +} from '../workflows/_fixtures' + +const FACETS = '/index.php/apps/softwarecatalog/api/facets' +/** The four GEMMA dimensions the endpoint must always describe. */ +const DIMENSIONS = ['referentiecomponent', 'standaard', 'applicatieservice', 'domein'] as const + +/** Unique per run so the cache — keyed by params — is cold on first use. */ +const TOKEN = `facet${RUN_ID.replace(/[^a-z0-9]/gi, '')}` + +let ctx: APIRequestContext +let config: VoorzieningenConfig +const seeded: string[] = [] + +test.beforeAll(async () => { + ctx = await newApiContext() + config = await resolveConfig(ctx) + // Two modules whose names contain TOKEN, so a text query can select + // exactly them out of whatever else the instance holds. + for (const n of [1, 2]) { + seeded.push(await createObject(ctx, config.register, 'module', { naam: `${TOKEN} module ${n}` })) + } +}) + +test.afterAll(async () => { + if (!ctx || !config) return + for (const id of seeded) { + await deleteObject(ctx, config.register, 'module', id) + } + await ctx.dispose() +}) + +// @e2e gemma-faceted-search::facet-response-covers-all-four-gemma-dimensions +test('facets: the response carries all four GEMMA dimensions, empty ones as [] not omitted', async () => { + // ⚠️ A CACHE-COLD parameter set, and that is load-bearing. + // + // The unparameterised `GET /api/facets/module` is served from an entry the + // product never invalidates (the facet cache is TTL-only), so it can answer + // with a count — and a response shape — that predates the running code. + // Measured: `findAll` saw 3 modules while the unparameterised call returned + // `{"cached":true,"totalMatched":1}`. A unique `search` term forces a fresh + // computation, so this assertion is made against live output. + // + // The `cached === false` guard below is not decoration: without it this test + // would silently degrade into "some earlier response had four dimensions". + const res = await ctx.get(`${FACETS}/module?search=${encodeURIComponent(`${TOKEN}-dims`)}`) + expect(res.status(), `GET ${FACETS}/module returned ${res.status()}`).toBe(200) + const body = await res.json() + expect(body?._meta?.cached, 'this assertion needs a freshly computed response, but got a cached one').toBe(false) + + // This is the load-bearing half of the scenario: "a dimension with no + // matching objects MUST be present as an empty array, not omitted". On this + // instance the GEMMA link fields are unpopulated, so every dimension IS + // empty — which makes this the exact condition the scenario describes, + // rather than a weaker version of it. + for (const dim of DIMENSIONS) { + expect(Object.prototype.hasOwnProperty.call(body, dim), `dimension "${dim}" is missing from the response`).toBe(true) + expect(Array.isArray(body[dim]), `dimension "${dim}" is not an array: ${JSON.stringify(body[dim])}`).toBe(true) + } + // And no extra top-level dimension keys crept in beyond the four + _meta. + expect(Object.keys(body).sort()).toEqual([...DIMENSIONS].sort().concat('_meta').sort()) +}) + +// @e2e gemma-faceted-search::unsupported-schema-is-rejected +test('facets: an unsupported schema is rejected with 400 naming the supported ones', async () => { + const res = await ctx.get(`${FACETS}/contract`) + expect(res.status(), `GET ${FACETS}/contract returned ${res.status()} — expected 400`).toBe(400) + + const body = await res.json() + const message = String(body?.message ?? '') + // The scenario requires the error to NAME the supported schemas, not merely + // to reject — so both names are asserted, not just a non-2xx. + expect(message, `error message did not name the supported schemas: ${message}`).toMatch(/module/) + expect(message).toMatch(/dienst/) + + // The supported set is also machine-readable, and must be exactly the two. + expect(body?.supportedSchemas?.sort?.()).toEqual(['dienst', 'module']) + + // Control: the same endpoint shape with a SUPPORTED schema is a 200, so the + // 400 above is about the schema and not about the route being broken. + const ok = await ctx.get(`${FACETS}/dienst`) + expect(ok.status(), `GET ${FACETS}/dienst returned ${ok.status()}`).toBe(200) +}) + +// ⚠️ `no-text-query-returns-facets-over-the-full-rbac-scoped-set` IS DELIBERATELY +// NOT CLAIMED HERE, and it is not an oversight. +// +// The scenario needs the UNPARAMETERISED aggregate, and that is exactly the +// cache entry the product never invalidates. Measured during the first run of +// this file: `findAll` saw 3 modules while `GET /api/facets/module` answered +// `{"cached":true,"totalMatched":1}` — the entry had been populated when the +// instance held one module and did not move when two more were created. The +// assertion `totalMatched === visibleModules` therefore fails for a REAL +// product reason (filed: facet cache is TTL-only, no invalidation path). +// +// Three ways to make it green were available and all three were rejected: +// waiting out the 1800 s TTL (a timing-dependent test), asserting `>=` instead +// of `===` (an assertion that cannot fail), or adding a cache-busting param +// (which makes it a different scenario — a FILTERED aggregate). Leaving the +// scenario uncovered and counted is the honest outcome; it becomes coverable +// the moment invalidation exists. + +// @e2e gemma-faceted-search::text-query-narrows-facet-counts +test('facets: a text query narrows the aggregated set', async () => { + // Both comparands use CACHE-COLD parameter sets. The unparameterised entry + // is unusable as a baseline here (see the block above), so "narrows" is + // asserted between a broad query and a strictly-narrower one, both of which + // are computed fresh and therefore both accurate. + const broad = await ctx.get(`${FACETS}/module?search=${encodeURIComponent(TOKEN)}`) + expect(broad.status(), `GET with the broad search returned ${broad.status()}`).toBe(200) + const totalBroad = (await broad.json())?._meta?.totalMatched + expect(totalBroad, `the run token matched ${totalBroad}, expected the 2 seeded modules`).toBe(2) + + // One character more specific — matches exactly one of the two. + const narrow = await ctx.get(`${FACETS}/module?search=${encodeURIComponent(`${TOKEN} module 1`)}`) + expect(narrow.status()).toBe(200) + const totalNarrow = (await narrow.json())?._meta?.totalMatched + expect(totalNarrow, `the narrower query matched ${totalNarrow}, expected 1`).toBe(1) + + // Strictly fewer: that is what "narrows" means, and both numbers were + // computed rather than served stale. + expect(totalBroad).toBeGreaterThan(totalNarrow) + + // A term matching nothing narrows all the way to zero, and still returns + // all four dimensions rather than erroring. + const miss = await ctx.get(`${FACETS}/module?search=${encodeURIComponent(TOKEN)}zzzznomatch`) + expect(miss.status()).toBe(200) + const missBody = await miss.json() + expect(missBody?._meta?.totalMatched).toBe(0) + for (const dim of DIMENSIONS) { + expect(Array.isArray(missBody[dim]), `dimension "${dim}" absent on an empty result`).toBe(true) + } +}) + +// @e2e gemma-faceted-search::repeated-identical-facet-request-is-served-from-cache +test('facets: a repeated identical request is served from cache, a different one is not', async () => { + // A parameter combination nothing has asked for yet, so the first call is + // guaranteed to be a cache MISS. Using the run token rather than the bare + // endpoint matters: the unparameterised call is warmed by the other tests + // in this file, and asserting `cached === false` on it would fail for a + // reason that has nothing to do with the behaviour. + const url = `${FACETS}/module?search=${encodeURIComponent(TOKEN)}-cachecheck` + + const first = await ctx.get(url) + expect(first.status()).toBe(200) + expect((await first.json())?._meta?.cached, 'first request for a fresh parameter set was already cached').toBe(false) + + const second = await ctx.get(url) + expect(second.status()).toBe(200) + expect((await second.json())?._meta?.cached, 'the repeated identical request was NOT served from cache').toBe(true) + + // A DIFFERENT parameter set is a different cache entry — proving the flag + // tracks the request rather than being stuck on after any first call. + const other = await ctx.get(`${url}-variant`) + expect(other.status()).toBe(200) + expect((await other.json())?._meta?.cached, 'a different parameter set reused another entry\'s cache').toBe(false) +}) + +// @e2e gemma-faceted-search::facet-panel-renders-alongside-the-existing-index-page-toolbar +test('facets: the GEMMA panel renders beside the search box on the module index', async ({ page }) => { + const bag = collectAppErrors(page) + await navClickTo(page, 'Applications') + + const main = page.locator(APP_MAIN).first() + await expect(main).toBeVisible({ timeout: 30000 }) + + // The facet sidebar is present… + const sidebar = page.locator('.cn-facet-sidebar').first() + await expect(sidebar, 'the GEMMA facet sidebar did not render').toBeVisible({ timeout: 30000 }) + await expect(sidebar.locator('.cn-facet-sidebar__title')).toContainText(/GEMMA/i) + + // …and it lists all four dimensions as filter groups. + await expect(sidebar.locator('.cn-facet-sidebar__group')).toHaveCount(DIMENSIONS.length) + + // …AND the pre-existing toolbar still works alongside it, which is the + // second half of the scenario ("the existing free-text search box ... MUST + // continue to render and function unchanged"). + const search = page.locator('.faceted-catalog-index__search').first() + await expect(search, 'the free-text search box disappeared').toBeVisible() + await search.getByRole('textbox').first().fill(TOKEN) + // The list re-fetches and settles on this run's seeded modules. + await expect(main.getByText(`${TOKEN} module 1`).first()).toBeVisible({ timeout: 30000 }) + + expectNoAppErrors(bag) +}) From d35596ce0d3143cbd21df8aedf2cde0f734d8e0d Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 19:43:36 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix(e2e):=20drop=20the=20facet-cache=20test?= =?UTF-8?q?=20=E2=80=94=20the=20CI=20instance=20configures=20no=20cache=20?= =?UTF-8?q?backend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this, not the dev rig: the test passed locally and failed on the runner, on the first attempt and its retry. The facet cache is `ICacheFactory::createDistributed(...)`, which degrades to a NULL cache when Nextcloud has no memcache backend configured — nothing is stored, so `_meta.cached` is always false and "a repeated identical request is served from cache" has no observable behaviour there. Measured on both sides rather than assumed: dev rig `occ config:system:get memcache.local` -> \OC\Memcache\APCu, APCu present; the flag flips false -> true reliably. CI the shared workflow configures no memcache at all (zero mentions of memcache/apcu in the job log); both calls returned `cached: false`. The two tempting repairs were rejected. A `test.skip` guard on "is caching available" can never be false on CI, so it would credit coverage that never executes — the exact shape flagged fleet-wide. Asserting only that `cached` is a boolean is an assertion that cannot fail. So the scenario is left uncovered and COUNTED, with the reasoning recorded at the point where the test used to be. It becomes coverable when the CI instance configures a cache backend. beta scope: 137 -> 112 (25 scenarios, 18 test cases, 0 exclusions) Negative control on the final tree: with tests/e2e/spec-coverage/gemma-faceted-search.spec.ts 112 file removed 116 (+4 = its 4 anchors) file restored 112 Refs #483 --- .../gemma-faceted-search.spec.ts | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts b/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts index 905712e2..7a6e4788 100644 --- a/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts +++ b/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts @@ -173,29 +173,27 @@ test('facets: a text query narrows the aggregated set', async () => { } }) -// @e2e gemma-faceted-search::repeated-identical-facet-request-is-served-from-cache -test('facets: a repeated identical request is served from cache, a different one is not', async () => { - // A parameter combination nothing has asked for yet, so the first call is - // guaranteed to be a cache MISS. Using the run token rather than the bare - // endpoint matters: the unparameterised call is warmed by the other tests - // in this file, and asserting `cached === false` on it would fail for a - // reason that has nothing to do with the behaviour. - const url = `${FACETS}/module?search=${encodeURIComponent(TOKEN)}-cachecheck` - - const first = await ctx.get(url) - expect(first.status()).toBe(200) - expect((await first.json())?._meta?.cached, 'first request for a fresh parameter set was already cached').toBe(false) - - const second = await ctx.get(url) - expect(second.status()).toBe(200) - expect((await second.json())?._meta?.cached, 'the repeated identical request was NOT served from cache').toBe(true) - - // A DIFFERENT parameter set is a different cache entry — proving the flag - // tracks the request rather than being stuck on after any first call. - const other = await ctx.get(`${url}-variant`) - expect(other.status()).toBe(200) - expect((await other.json())?._meta?.cached, 'a different parameter set reused another entry\'s cache').toBe(false) -}) +// ⚠️ `repeated-identical-facet-request-is-served-from-cache` IS NOT CLAIMED HERE. +// +// It passed locally and FAILED ON CI, and the difference is the environment, +// not the code. The facet cache is `ICacheFactory::createDistributed(...)`, +// which degrades to a NULL cache when Nextcloud has no memcache backend +// configured — nothing is stored and `_meta.cached` is therefore always false. +// +// Measured on both sides: +// dev rig — `occ config:system:get memcache.local` -> \OC\Memcache\APCu, +// APCu present; the flag flips false -> true reliably. +// CI — the shared workflow configures no memcache at all (zero +// mentions of memcache/apcu in the job log); both calls returned +// `cached: false` and the assertion failed on the first run and +// its retry. +// +// So on the instance where this suite actually runs, the scenario has no +// observable behaviour. The tempting fixes are both wrong: a `test.skip` guard +// on "is caching available" would never be false on CI, so it would credit +// coverage that never executes; and asserting only that `cached` is a boolean +// is an assertion that cannot fail. Left uncovered and counted until the CI +// instance configures a cache backend. // @e2e gemma-faceted-search::facet-panel-renders-alongside-the-existing-index-page-toolbar test('facets: the GEMMA panel renders beside the search box on the module index', async ({ page }) => {