diff --git a/src/views/WorkspaceApp.vue b/src/views/WorkspaceApp.vue index bf177638..51067257 100644 --- a/src/views/WorkspaceApp.vue +++ b/src/views/WorkspaceApp.vue @@ -511,8 +511,10 @@ export default { * `null` undims everything; an array dims every item whose id is * NOT present (an empty array therefore dims everything). * - * @param {Array|null} matchIds the current matching ids. + * @param {Array|null} matchIds the current matching ids + * (numbers off the API row, strings once normalised below). * @return {void} + * @spec openspec/specs/tile-quick-search/spec.md */ applySearchDimming(matchIds) { if (typeof document === 'undefined') { @@ -522,13 +524,26 @@ export default { if (!items) { return } + /* + * A PLACEMENT ID IS A NUMBER; A DOM ATTRIBUTE IS A STRING. + * `matchIds` comes from `searchableTiles()`, which copies + * `placement.id` straight off the API row — an integer. The value + * read back out of a rendered cell is `getAttribute()`, which is + * always a string. `Array.prototype.includes` compares with + * SameValueZero, i.e. no coercion at all, so `[7].includes('7')` + * is `false` — EVERY tile was dimmed on every query, including + * the matches the user was looking for (launchpad#95). + * Normalising both sides to strings makes the comparison one + * between two values of the same type. + */ + const wanted = matchIds === null ? null : matchIds.map((id) => String(id)) items.forEach((el) => { - if (matchIds === null) { + if (wanted === null) { el.classList.remove('launchpad-grid-item--dimmed') return } const id = el.getAttribute('data-placement-id') - el.classList.toggle('launchpad-grid-item--dimmed', matchIds.includes(id) === false) + el.classList.toggle('launchpad-grid-item--dimmed', wanted.includes(id) === false) }) }, @@ -539,12 +554,28 @@ export default { * REQ-QSEARCH-003 "honouring its configured link target"). Non-tile * placements without a link are focused instead, best-effort. * - * @param {{id: string, placement: object}} item the opened search - * result. + * @param {{id: (string|number), placement: object}} item the opened + * search result. `placement.id` is an INTEGER off the API row. * @return {void} + * @spec openspec/specs/tile-quick-search/spec.md */ activateSearchResult(item) { - const placementId = item?.placement?.id + /* + * `String(...)`, not the raw value. `placement.id` is an INTEGER + * off the API row, and the line below used to call + * `placementId.replace(...)` on it. `Number.prototype.replace` + * does not exist, so this threw a `TypeError` on every single + * activation — inside a Vue event handler, where nothing surfaces + * it, so pressing Enter on a search result silently did nothing + * (launchpad#95). The truthiness guard above did not catch it: a + * non-zero integer is truthy. + * + * `?? ''` rather than a bare cast so that `null`/`undefined` + * become the empty string and are rejected by the guard, instead + * of being stringified into the literal `"null"` and sent to + * `querySelector` as a real id to look for. + */ + const placementId = String(item?.placement?.id ?? '') if (!placementId || !this.$el) { return } diff --git a/src/views/__tests__/WorkspaceApp.spec.js b/src/views/__tests__/WorkspaceApp.spec.js index 00f57d3c..62a05768 100644 --- a/src/views/__tests__/WorkspaceApp.spec.js +++ b/src/views/__tests__/WorkspaceApp.spec.js @@ -334,6 +334,97 @@ describe('WorkspaceApp', () => { expect(clickSpy).toHaveBeenCalled() }) + /* + * launchpad#95 — TWO DEFECTS, ONE ROOT CAUSE, AND A FIXTURE THAT HID + * BOTH. + * + * Every test above seeds a placement id as a STRING (`'p1'`, + * `'match'`). `WidgetPlacement` rows arrive from + * `GET /api/dashboard/{id}` with an INTEGER `id` — the column is an + * auto-increment primary key — so no fixture in this file had ever + * exercised the type the product actually handles, and both defects + * below were invisible to a green suite: + * + * 1. `applySearchDimming()` compared `getAttribute()` (always a + * string) against the raw ids with `Array.includes`, which does + * not coerce. With integer ids nothing ever matched, so EVERY + * tile was dimmed — including the ones the user searched for. + * 2. `activateSearchResult()` called `.replace()` on the id. + * `Number.prototype.replace` does not exist, so Enter threw a + * `TypeError` inside a Vue event handler and silently did + * nothing. + * + * These two tests are the regression guard, and they are written + * against the production shape on purpose. Both are RED on the code + * as it stood before the fix: the first because `matchEl` is dimmed, + * the second because the call throws before reaching the link. + */ + it('onSearchFilter leaves an INTEGER-id match undimmed (launchpad#95)', () => { + const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) + const grid = wrapper.find('.workspace-shell__grid').element + const matchEl = document.createElement('div') + matchEl.className = 'launchpad-grid-item' + matchEl.setAttribute('data-placement-id', '7') + const otherEl = document.createElement('div') + otherEl.className = 'launchpad-grid-item' + otherEl.setAttribute('data-placement-id', '8') + grid.appendChild(matchEl) + grid.appendChild(otherEl) + + // The ids are numbers, exactly as `searchableTiles()` copies them + // off the API row — NOT the strings the older fixtures used. + wrapper.vm.onSearchFilter([7]) + + expect( + matchEl.classList.contains('launchpad-grid-item--dimmed'), + 'a matching tile must not be de-emphasised', + ).toBe(false) + expect( + otherEl.classList.contains('launchpad-grid-item--dimmed'), + 'CONTROL: a non-matching tile must still be de-emphasised, or the assertion above is satisfied by "nothing is ever dimmed"', + ).toBe(true) + }) + + it('onSearchOpen activates a tile whose placement id is an INTEGER (launchpad#95)', () => { + const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) + const grid = wrapper.find('.workspace-shell__grid').element + const el = document.createElement('div') + el.className = 'launchpad-grid-item' + el.setAttribute('data-placement-id', '7') + const link = document.createElement('a') + link.setAttribute('href', '#deck') + el.appendChild(link) + grid.appendChild(el) + + const scrollSpy = vi.fn() + el.scrollIntoView = scrollSpy + const clickSpy = vi.spyOn(link, 'click') + + wrapper.vm.onSearchOpen({ id: 7, label: 'Deck', placement: { id: 7 } }) + + expect(scrollSpy, 'the matched tile must be scrolled into view').toHaveBeenCalled() + expect(clickSpy, 'Enter must activate the tile\'s rendered link').toHaveBeenCalled() + }) + + it('onSearchOpen ignores a placement with no id rather than looking for the string "null"', () => { + const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) + const grid = wrapper.find('.workspace-shell__grid').element + const el = document.createElement('div') + el.className = 'launchpad-grid-item' + // A cell literally labelled "null" — the shape a bare String() + // cast would go looking for, and find. + el.setAttribute('data-placement-id', 'null') + const link = document.createElement('a') + link.setAttribute('href', '#deck') + el.appendChild(link) + grid.appendChild(el) + const clickSpy = vi.spyOn(link, 'click') + + wrapper.vm.onSearchOpen({ id: null, label: 'Deck', placement: { id: null } }) + + expect(clickSpy, 'a null id must not activate the tile that happens to be called "null"').not.toHaveBeenCalled() + }) + it('onSearchFallback opens a web-search URL in a new tab', () => { const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) const openSpy = vi.spyOn(window, 'open').mockImplementation(() => {}) diff --git a/tests/e2e/conditional-visibility-editor.spec.ts b/tests/e2e/conditional-visibility-editor.spec.ts new file mode 100644 index 00000000..ec1391a1 --- /dev/null +++ b/tests/e2e/conditional-visibility-editor.spec.ts @@ -0,0 +1,1016 @@ +/* + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + * + * The conditional-visibility rule builder — the "Visibility rules…" modal + * reached from a placement's right-click context menu + * (conditional-visibility-editor REQ-CVUI-001..005). + * + * WHY A BROWSER + * ============= + * Almost every claim in this spec is either about the DOM (which section a + * row sits in, whether the include/exclude distinction survives without + * colour, what the empty state says) or about the exact request body a row + * emits when the author presses Save. The request body half is the + * interesting one: REQ-CVUI-001 says the editor MUST NOT introduce a new + * persistence path or alter the stored rule shape, and the only way to check + * that is to watch what the browser actually sends. So these tests assert on + * intercepted request bodies rather than on the row re-rendering, which a + * purely local component could fake. + * + * WHAT IS DELIBERATELY NOT HERE, AND WHY IT IS NOT AN @e2e exclude + * ================================================================ + * Five scenarios in this spec need the author to PICK A GROUP, and the group + * picker has no options to pick from — `Views.vue` renders + * `` and never passes + * `available-groups`, so the prop keeps its `default: () => []`. Measured in + * a browser: the combobox reports "No results", and typing a group name and + * pressing Enter selects nothing (it is not taggable). The same empty list + * feeds the preview-as-audience picker, so "preview as groups [marketing]" + * cannot be performed either. + * + * REQ-CVUI-001 add-a-group-inclusion-rule-through-the-ui + * REQ-CVUI-002 group-row-operands + * REQ-CVUI-004 preview-shows-visible-for-a-matching-audience + * REQ-CVUI-004 preview-shows-hidden-for-a-non-matching-audience + * REQ-CVUI-004 preview-reflects-an-exclude-override + * + * Those five carry NO `@e2e exclude`. An exclusion states "a browser cannot + * observe this scenario", which is false — a browser observes it perfectly + * well, and what it observes is that the feature is not wired. Per + * `.github#345` the gate scores an exclusion as POSITIVE coverage, so + * excluding them would buy five findings with a false statement. Filed + * instead as launchpad#97. + * + * The group RULE ITSELF works end to end when seeded through the API — it is + * only the picker's option list that is missing — which is why the tests + * below can still load, render, preview and evaluate group rules. + * + * A SECOND BLOCKER, FOUND WHILE WRITING THESE TESTS: NO EXCLUDE RULE CAN BE + * CREATED AT ALL (launchpad#96) + * ========================================================================== + * `POST /api/widgets/{id}/rules` with `isInclude: false` answers **HTTP 400 + * `{"error":"Operation failed"}`**. The identical body with `isInclude: true` + * answers 201. `PUT /api/rules/{id}` with `{"isInclude": false}` is refused + * the same way, so there is no path to an exclude rule — not through the API, + * not through this editor. + * + * Root cause is a type mismatch: `oc_launchpad_conditional_rules.is_include` + * is `smallint NOT NULL DEFAULT 1`, while `ConditionalRule` declares + * `protected bool $isInclude` plus `addType('isInclude', 'boolean')`, so + * `QBMapper::getParameterTypeForProperty()` binds `PARAM_BOOL`. On PostgreSQL + * — the shared workflow's DEFAULT database, so this is the CI fixture too — + * a boolean will not go into a smallint column. The controller reports it + * through `ResponseHelper::error()` WITHOUT a logger, so the real exception is + * written nowhere and the client sees only the generic message. + * + * That costs `include-rules-grouped-under-an-or-heading`, which needs a saved + * exclude rule to exist. It is left uncovered and unexcluded for the same + * reason as the five above. + * + * `includeexclude-toggle` IS still covered, deliberately and narrowly: that + * requirement is about what the ROW emits and where the row moves to, and + * both are observable before the server ever sees the request. The test + * asserts exactly that and does not assert persistence, which would be + * asserting #97 is fixed. + * + * @spec openspec/specs/conditional-visibility-editor/spec.md + */ + +import { expect, request, test, type APIRequestContext, type Locator, type Page } from '@playwright/test' + +const ADMIN = { + user: process.env.ADMIN_USER ?? process.env.NC_ADMIN_USER ?? 'admin', + pass: process.env.ADMIN_PASSWORD ?? process.env.NC_ADMIN_PASS ?? 'admin', +} +const ENV_BASE_URL = (process.env.BASE_URL ?? process.env.NC_BASE_URL ?? '').replace(/\/$/, '') + +const APP_URL = '/index.php/apps/launchpad' +const SETTINGS = '/index.php/apps/launchpad/api/admin/settings' + +const EDITOR = '[data-test="conditional-visibility-editor"]' +const ROW_INCLUDE = '[data-test="visibility-rule-row-include"]' +const ROW_EXCLUDE = '[data-test="visibility-rule-row-exclude"]' +const ADD_RULE = '[data-test="add-rule"]' +const EMPTY_STATE = '[data-test="visibility-empty-state"]' +const INCLUDE_SECTION = '[data-test="include-section"]' +const EXCLUDE_SECTION = '[data-test="exclude-section"]' +const RUN_PREVIEW = '[data-test="run-preview"]' +const PREVIEW_VERDICT = '[data-test="preview-verdict-text"]' + +/* + * `VisibilityRuleRow.vue`'s own root carries `data-test="visibility-rule-row"`, + * but the parent passes `data-test="visibility-rule-row-include"` / + * `-exclude` on the `` tag — and a fallthrough attribute + * OVERWRITES the child's own. So `visibility-rule-row` never appears in the + * DOM at all. Written down because a selector that matches nothing fails as a + * timeout, which reads like the modal not opening. + */ + +/** The rule-type options, in the order `typeOptions` declares them. */ +const TYPE = { group: 0, time: 1, date: 2, attribute: 3 } as const + +const STAMP = `${Date.now()}` + +/* + * A `startDate` that is open-ended and already past, so a rule carrying it + * MATCHES right now. + * + * This is load-bearing, not decoration. Under include=OR a placement with + * include rules and no match is HIDDEN — correctly — and a hidden placement + * cannot be right-clicked, so the editor cannot be reopened to inspect the + * very rules that hid it. Any fixture that seeds a non-matching include rule + * must therefore seed a matching one alongside it, or it locks itself out of + * the surface under test. (Measured: seeding a lone `groups:["marketing"]` + * rule as admin made the tile vanish and the next right-click time out.) + */ +const MATCHING_START_DATE = `${new Date().getUTCFullYear() - 1}-01-01` +const KEEP_VISIBLE = { + ruleType: 'date', + ruleConfig: { startDate: MATCHING_START_DATE }, + isInclude: true, +} as const + +function basic(user: string, pass: string): string { + return `Basic ${Buffer.from(`${user}:${pass}`).toString('base64')}` +} + +/* + * An API context that is unambiguously the admin. The jar is spelled + * `{ cookies: [], origins: [] }` and NOT `storageState: undefined` — option + * merging reads an explicit `undefined` as "not supplied" and falls back to + * the project default, which is the very session this is meant to drop. + */ +async function adminApi(): Promise { + return request.newContext({ + baseURL: ENV_BASE_URL, + storageState: { cookies: [], origins: [] }, + extraHTTPHeaders: { + 'OCS-APIRequest': 'true', + Authorization: basic(ADMIN.user, ADMIN.pass), + }, + }) +} + +let dashboardId = 0 +let dashboardUuid = '' +let placementId = 0 +let priorAllowUserDash = true + +/** Every rule currently stored against the shared placement. */ +async function storedRules(api: APIRequestContext): Promise>> { + const res = await api.get(`/index.php/apps/launchpad/api/widgets/${placementId}/rules`) + expect(res.status(), `reading stored rules: ${await res.text()}`).toBe(200) + return (await res.json()).rules ?? [] +} + +/** Remove every rule on the shared placement, so each test starts clean. */ +async function clearRules(api: APIRequestContext): Promise { + for (const rule of await storedRules(api)) { + const res = await api.delete(`/index.php/apps/launchpad/api/rules/${rule.id}`) + expect(res.status(), `clearing rule ${rule.id}: ${await res.text()}`).toBeLessThan(300) + } + expect(await storedRules(api), 'the placement must start each test with no rules').toHaveLength(0) +} + +/** Seed one rule directly, bypassing the UI. Returns its server id. */ +async function seedRule( + api: APIRequestContext, + rule: { ruleType: string, ruleConfig: Record, isInclude: boolean }, +): Promise { + const res = await api.post(`/index.php/apps/launchpad/api/widgets/${placementId}/rules`, { data: rule }) + expect(res.status(), `seeding ${rule.ruleType} rule: ${await res.text()}`).toBeLessThan(300) + const body = await res.json() + return Number(body.id ?? body.rule?.id) +} + +test.beforeAll(async () => { + const api = await adminApi() + + const before = await api.get(SETTINGS) + expect(before.status(), await before.text()).toBe(200) + priorAllowUserDash = (await before.json()).allowUserDashboards === true + const enable = await api.put(SETTINGS, { data: { allowUserDash: true } }) + expect(enable.status(), await enable.text()).toBeLessThan(300) + + const created = await api.post('/index.php/apps/launchpad/api/dashboard', { + data: { name: `E2E Visibility ${STAMP}` }, + }) + expect(created.status(), await created.text()).toBeLessThan(300) + const body = await created.json() + const dash = body.dashboard ?? body.data?.dashboard ?? body + dashboardId = Number(dash.id) + dashboardUuid = String(dash.uuid) + expect(dashboardId, `no dashboard id: ${JSON.stringify(body)}`).toBeTruthy() + + const tile = await api.post(`/index.php/apps/launchpad/api/dashboard/${dashboardId}/tile`, { + data: { + title: `Visibility subject ${STAMP}`, + linkType: 'url', + linkValue: `https://example.invalid/${STAMP}`, + gridX: 0, + gridY: 0, + gridWidth: 3, + gridHeight: 2, + }, + }) + expect(tile.status(), `seeding the subject tile: ${await tile.text()}`).toBeLessThan(300) + placementId = Number((await tile.json()).id) + expect(placementId, 'no placement id for the subject tile').toBeTruthy() + + /* + * Activate through BOTH mechanisms. `/activate` sets the id-based + * `is_active` column; `POST /api/dashboards/active` sets the per-user UUID + * PREFERENCE, and the shell resolves through the preference. With only the + * first called, a preference left behind by an earlier spec in this serial + * job wins and every assertion below runs against someone else's dashboard. + */ + const activate = await api.post(`/index.php/apps/launchpad/api/dashboard/${dashboardId}/activate`) + expect(activate.status(), await activate.text()).toBeLessThan(300) + const preference = await api.post('/index.php/apps/launchpad/api/dashboards/active', { + data: { uuid: dashboardUuid }, + }) + expect(preference.status(), await preference.text()).toBeLessThan(300) + + await api.dispose() +}) + +test.afterAll(async () => { + const api = await adminApi() + if (dashboardId) { + await api.delete(`/index.php/apps/launchpad/api/dashboard/${dashboardId}`) + } + if (priorAllowUserDash === false) { + await api.put(SETTINGS, { data: { allowUserDash: false } }) + } + await api.dispose() +}) + +/** Load the workspace on the seeded dashboard, in edit mode. */ +async function openWorkspaceInEditMode(page: Page): Promise { + await page.goto(APP_URL) + + /* + * Gate on THIS suite's own tile, not on a bare grid-item count. A + * dashboard seeded with the default bundle clears a count gate easily, so + * when the shell resolves a different dashboard a count passes and every + * later assertion fails downstream with a symptom three steps from its + * cause. + */ + await expect( + page.locator('.launchpad-grid-item').filter({ hasText: STAMP }).first(), + `the active dashboard must be the one this suite seeded (tile stamped ${STAMP})`, + ).toBeVisible({ timeout: 30_000 }) + + /* + * IDEMPOTENT ON PURPOSE. Edit mode is server-side state that SURVIVES a + * reload, so on every test after the first the dashboard is already in it + * and the cog menu offers "Stop editing" rather than "Edit dashboard". + * The first draft of this helper clicked unconditionally, and every test + * but the first then failed at the *next* step — the context menu never + * opened — which reads like a context-menu bug and is not one. + */ + if (await page.locator('.launchpad-edit-mode').count() === 0) { + await page.locator('.launchpad-sidebar-toggle').first().click() + await page.waitForSelector('.dashboard-switcher-sidebar.open', { timeout: 10_000 }) + /* + * `.active`, NOT `.first()`. The sidebar lists every personal + * dashboard, and the first one is whichever sorts first — not + * necessarily this suite's. Opening a different row's cog menu and + * choosing "Edit dashboard" SWITCHES the active dashboard, so the + * grid then holds someone else's tiles and the right-click below + * cannot find the stamped one. + * + * Measured: with `.first()` the suite entered edit mode on a stale + * dashboard left behind by an earlier run, and failed at the + * right-click with "locator not found" — three steps from the cause, + * and reading as a context-menu defect. + */ + const activeRow = page.locator('[data-source="user"].dashboard-switcher-sidebar__item.active').first() + await expect( + activeRow, + 'the active dashboard must be a personal one this suite owns before edit mode is entered', + ).toBeVisible({ timeout: 10_000 }) + await activeRow.locator('.dashboard-row-actions button').first().click() + await page.getByRole('menuitem', { name: /edit dashboard/i }).click() + await page.waitForSelector('.launchpad-edit-mode', { timeout: 10_000 }) + + // Close the sidebar so it cannot occlude the grid for the right-click. + const closeBtn = page.locator('.dashboard-switcher-sidebar__close').first() + if (await closeBtn.isVisible().catch(() => false)) { + await closeBtn.click() + await page.waitForFunction( + () => !document.querySelector('.dashboard-switcher-sidebar.open'), + { timeout: 5_000 }, + ).catch(() => null) + } + } + + await expect( + page.locator('.launchpad-edit-mode'), + 'the grid must be in edit mode, or the placement context menu never opens', + ).toHaveCount(1) + + // Re-gate AFTER edit mode. Entering it goes through the dashboard switcher, + // which is exactly the step that can change which dashboard is active, so + // the gate taken before it is not still true here. + await expect( + page.locator('.launchpad-grid-item').filter({ hasText: STAMP }).first(), + `edit mode must have been entered on THIS suite's dashboard (tile stamped ${STAMP})`, + ).toBeVisible({ timeout: 30_000 }) +} + +/** Open the Visibility rules modal on the seeded placement. */ +async function openVisibilityEditor(page: Page): Promise { + await openWorkspaceInEditMode(page) + await page.locator('.launchpad-grid-item').filter({ hasText: STAMP }).first().click({ button: 'right' }) + await page.locator('[data-testid="ctx-visibility-rules"]').click() + await expect(page.locator(EDITOR), 'the visibility editor must mount').toBeVisible({ timeout: 15_000 }) + // The editor renders its loading spinner in place of the body, so waiting + // for the Add-rule button is what says the rules fetch has settled. + await expect(page.locator(ADD_RULE)).toBeVisible({ timeout: 15_000 }) +} + +/** + * Record every non-GET launchpad API request the page makes, with its body. + * The whole point of REQ-CVUI-001 is which endpoint and which shape, so the + * wire is the evidence — not the re-rendered row. + */ +function recordWrites(page: Page): Array<{ method: string, url: string, body: string }> { + const writes: Array<{ method: string, url: string, body: string }> = [] + page.on('request', (req) => { + if (req.method() !== 'GET' && req.url().includes('/apps/launchpad/api/')) { + writes.push({ method: req.method(), url: req.url(), body: req.postData() ?? '' }) + } + }) + return writes +} + +/** + * Activate an `NcCheckboxRadioSwitch` (radio, switch or checkbox). + * + * The component's `data-test` lands on its ``, and that input is + * VISUALLY HIDDEN. Playwright resolves it happily and then waits forever for + * it to become clickable, which times out as + * `locator.click: Timeout ... exceeded` — indistinguishable from the control + * being missing or disabled. + * + * AND THERE IS NO `