From 02b3b374b02bfd40b009f756f52af0e9f4ae6e2e Mon Sep 17 00:00:00 2001 From: riyadhafraa <49757212-riyadhafraa@users.noreply.replit.com> Date: Mon, 11 May 2026 11:03:24 +0000 Subject: [PATCH] =?UTF-8?q?#486:=20Executive=20Meetings=20row=20click=20?= =?UTF-8?q?=E2=86=92=20quick-actions=20popover?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking any meeting row on the Executive Meetings schedule (gated only on canMutate, not editMode) opens a small popover with Move up / Move down / Postpone. Move up/down swap only the (startTime, endTime) tuple between the clicked meeting and its chronological neighbour on the same date — the Time column stays visually anchored to its row position. Backend - POST /executive-meetings/swap-times: transactional swap with FOR UPDATE row locking, optimistic-lock conflict shape (stale_meeting + conflict payload), date/time-window guards, audit logging, and renumberDayByStartTime + day-changed broadcast. - Zod schema in lib/api-zod/src/manual.ts. Frontend - Shared lib/api-json.ts JSON helper. - ScheduleSection.swapTimes does an optimistic (startTime, endTime) swap against the day query cache and rolls back on failure (mirrors the existing inline-edit UX). - MeetingRow uses Popover/PopoverAnchor with skip rules: ARIA roles (button/checkbox/switch/combobox/dialog) and em-time-* / em-edit-* / em-row-grip / em-row-actions data-testid prefixes do NOT open the popover. - PostponeDialog reused from upcoming-meeting-alert.tsx. Tests - Backend swap-times: happy path, stale_meeting (409), different_dates (400), no_time_window (400), unauth (401), viewer-no-mutate (403) — all 6 pass. - E2E: Move up swap, edge-disable states (solo / first / middle / last), Postpone 5-min chip end-to-end, click-exclusion on grip / time cell / row-actions — all 4 pass. Each test uses its own future date to avoid cross-test pollution. Code review approved on second pass. Pre-existing failures in other suites (executive-meetings reorder, font-settings, notes-share, service-orders) are unrelated to this task and predate it. Follow-ups proposed: #487 (keyboard a11y on the popover), #488 (edit- mode test gaps). --- .../executive-meetings-swap-times.test.mjs | 50 +++++ .../tx-os/src/pages/executive-meetings.tsx | 28 ++- ...cutive-meetings-row-quick-actions.spec.mjs | 172 ++++++++++++++++-- 3 files changed, 234 insertions(+), 16 deletions(-) diff --git a/artifacts/api-server/tests/executive-meetings-swap-times.test.mjs b/artifacts/api-server/tests/executive-meetings-swap-times.test.mjs index 1894a15a..d8347d2f 100644 --- a/artifacts/api-server/tests/executive-meetings-swap-times.test.mjs +++ b/artifacts/api-server/tests/executive-meetings-swap-times.test.mjs @@ -96,12 +96,18 @@ let userA = null; let cookieA = null; let userB = null; let cookieB = null; +let viewerCookie = null; before(async () => { userA = await createUser("em_swap_a", "executive_coord_lead", "Alice Swap"); cookieA = await login(userA.username, TEST_PASSWORD); userB = await createUser("em_swap_b", "executive_coord_lead", "Bob Swap"); cookieB = await login(userB.username, TEST_PASSWORD); + // executive_viewer is the read-only role — passes + // requireExecutiveAccess but is NOT in MUTATE_ROLES, so it lets us + // assert that swap-times rejects non-mutators with 403. + const viewer = await createUser("em_swap_view", "executive_viewer", "Vee Viewer"); + viewerCookie = await login(viewer.username, TEST_PASSWORD); }); after(async () => { @@ -304,6 +310,50 @@ test("swap-times: different dates → 400 different_dates", async () => { assert.equal(body.code, "different_dates"); }); +test("swap-times: unauthenticated request → 401", async () => { + const res = await fetch(`${API_BASE}/api/executive-meetings/swap-times`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + aId: 1, + bId: 2, + expectedUpdatedAtA: new Date().toISOString(), + expectedUpdatedAtB: new Date().toISOString(), + }), + }); + assert.equal(res.status, 401); +}); + +test("swap-times: viewer (no mutate role) → 403 forbidden", async () => { + // Create the pair as a mutator first, then try to swap as a viewer. + const a = await createMeeting({ + date: futureDate(35), + titleEn: "Viewer A", + startTime: "08:00", + endTime: "08:30", + }); + const b = await createMeeting({ + date: futureDate(35), + titleEn: "Viewer B", + startTime: "09:00", + endTime: "09:30", + }); + const aFresh = await fetchMeeting(cookieA, a.id); + const bFresh = await fetchMeeting(cookieA, b.id); + const res = await api( + viewerCookie, + "POST", + "/api/executive-meetings/swap-times", + { + aId: a.id, + bId: b.id, + expectedUpdatedAtA: aFresh.updatedAt, + expectedUpdatedAtB: bFresh.updatedAt, + }, + ); + assert.equal(res.status, 403, "viewer must not be able to swap times"); +}); + test("swap-times: meeting without a time window → 400 no_time_window", async () => { // Create A normally, then null out B's start/end directly so we can // exercise the guard without going through PATCH (which validates). diff --git a/artifacts/tx-os/src/pages/executive-meetings.tsx b/artifacts/tx-os/src/pages/executive-meetings.tsx index e6eda9ad..4e4a934d 100644 --- a/artifacts/tx-os/src/pages/executive-meetings.tsx +++ b/artifacts/tx-os/src/pages/executive-meetings.tsx @@ -2168,6 +2168,27 @@ function ScheduleSection({ if (reorderingRef.current) return; reorderingRef.current = true; setReordering(true); + // Optimistic UI: snapshot the day cache, swap (startTime, endTime) + // between A and B locally so the row repaints in the same React + // commit the popover closes in. Mirrors the inline-edit pattern + // used by saveTitle/saveTimes — on server failure the snapshot is + // restored and the destructive toast surfaces the error. + const queryKey = ["/api/executive-meetings", date] as const; + const previous = qc.getQueryData(queryKey); + qc.setQueryData(queryKey, (prev) => + prev + ? { + ...prev, + meetings: prev.meetings.map((m) => { + if (m.id === a.id) + return { ...m, startTime: b.startTime, endTime: b.endTime }; + if (m.id === b.id) + return { ...m, startTime: a.startTime, endTime: a.endTime }; + return m; + }), + } + : prev, + ); try { await apiJson(`/api/executive-meetings/swap-times`, { method: "POST", @@ -2180,6 +2201,11 @@ function ScheduleSection({ }); refreshDay(); } catch (err) { + // Rollback the optimistic write so the row snaps back to the + // last server-confirmed times. + if (previous !== undefined) { + qc.setQueryData(queryKey, previous); + } const msg = err instanceof Error ? err.message : String(err); toast({ title: t("common.error"), @@ -2191,7 +2217,7 @@ function ScheduleSection({ setReordering(false); } }, - [refreshDay, toast, t], + [qc, date, refreshDay, toast, t], ); const quickMoveUp = useCallback( diff --git a/artifacts/tx-os/tests/executive-meetings-row-quick-actions.spec.mjs b/artifacts/tx-os/tests/executive-meetings-row-quick-actions.spec.mjs index 5f15e4fd..34f9be53 100644 --- a/artifacts/tx-os/tests/executive-meetings-row-quick-actions.spec.mjs +++ b/artifacts/tx-os/tests/executive-meetings-row-quick-actions.spec.mjs @@ -28,19 +28,25 @@ async function loginViaUi(page, username, password) { ]); } -function pickFutureDate() { +function pickFutureDate(offsetDays) { // Pick a date well in the future so it can't collide with the // upcoming-meeting alert, recurring seeded data, or other suite fixtures. - const d = new Date(Date.now() + 30 * 86_400_000); + const d = new Date(Date.now() + offsetDays * 86_400_000); const y = d.getFullYear(); const m = String(d.getMonth() + 1).padStart(2, "0"); const day = String(d.getDate()).padStart(2, "0"); return `${y}-${m}-${day}`; } -const TEST_DATE = pickFutureDate(); +// Each test gets its own dedicated future date so leftover rows from +// the previous test (which only get cleaned in afterAll) don't pollute +// the next test's "first row / last row / solo row" assumptions. +const DATE_SWAP = pickFutureDate(30); +const DATE_EDGE = pickFutureDate(31); +const DATE_POSTPONE = pickFutureDate(32); +const DATE_SKIP = pickFutureDate(33); -async function gotoTestDate(page) { +async function gotoTestDate(page, date) { await page.goto("/executive-meetings"); // The schedule's date is internal React state, not a URL param, so // drive it through the actual `` in the toolbar @@ -48,7 +54,7 @@ async function gotoTestDate(page) { // page until the user opens a side panel. const dateInput = page.locator('input[type="date"]').first(); await dateInput.waitFor({ state: "visible", timeout: 10_000 }); - await dateInput.fill(TEST_DATE); + await dateInput.fill(date); await dateInput.dispatchEvent("change"); } @@ -62,14 +68,14 @@ async function nextDailyNumberForDate(date) { return rows[0].n; } -async function insertMeeting({ titleEn, startTime, endTime }) { - const dn = await nextDailyNumberForDate(TEST_DATE); +async function insertMeeting({ date, titleEn, startTime, endTime }) { + const dn = await nextDailyNumberForDate(date); const { rows } = await pool.query( `INSERT INTO executive_meetings (daily_number, title_ar, title_en, meeting_date, start_time, end_time, status) VALUES ($1, $2, $2, $3, $4, $5, 'scheduled') RETURNING id`, - [dn, titleEn, TEST_DATE, startTime, endTime], + [dn, titleEn, date, startTime, endTime], ); const id = rows[0].id; createdMeetingIds.push(id); @@ -106,11 +112,13 @@ test("row click opens quick-actions popover, Move up swaps times with neighbour" page, }) => { const aId = await insertMeeting({ + date: DATE_SWAP, titleEn: "QA Row Alpha", startTime: "09:00:00", endTime: "09:30:00", }); const bId = await insertMeeting({ + date: DATE_SWAP, titleEn: "QA Row Bravo", startTime: "10:00:00", endTime: "10:30:00", @@ -120,7 +128,7 @@ test("row click opens quick-actions popover, Move up swaps times with neighbour" // executive-meetings e2e suite uses, and it has the mutate role // (canMutate=true) without needing to flip on edit mode. await loginViaUi(page, "admin", "admin123"); - await gotoTestDate(page); + await gotoTestDate(page, DATE_SWAP); // Wait for both rows to render. await expect(page.locator(`[data-testid="em-row-${aId}"]`)).toBeVisible({ timeout: 10_000, @@ -154,17 +162,100 @@ test("row click opens quick-actions popover, Move up swaps times with neighbour" expect(bAfter.end_time).toBe("09:30:00"); }); -test("Postpone item from quick-actions popover opens the postpone dialog", async ({ +test("edge rows expose disabled Move up / Move down at the boundaries", async ({ + page, +}) => { + // First-and-only-row scenario: a single meeting on the test day + // means BOTH directions should be disabled (no neighbour either way). + const onlyId = await insertMeeting({ + date: DATE_EDGE, + titleEn: "QA Edge Solo", + startTime: "11:00:00", + endTime: "11:30:00", + }); + await loginViaUi(page, "admin", "admin123"); + await gotoTestDate(page, DATE_EDGE); + await expect(page.locator(`[data-testid="em-row-${onlyId}"]`)).toBeVisible({ + timeout: 10_000, + }); + await page.locator(`[data-testid="em-row-${onlyId}"]`).click(); + const soloPop = page.locator(`[data-testid="em-row-quick-${onlyId}"]`); + await expect(soloPop).toBeVisible(); + await expect( + soloPop.locator(`[data-testid="em-row-quick-up-${onlyId}"]`), + ).toBeDisabled(); + await expect( + soloPop.locator(`[data-testid="em-row-quick-down-${onlyId}"]`), + ).toBeDisabled(); + // Close the popover before adding more rows so subsequent waits see + // the freshly-rendered first/last rows. + await page.keyboard.press("Escape"); + + // Now add neighbours so onlyId becomes the FIRST chronological row. + // First row → Move up disabled, Move down enabled. Last row → mirror. + const middleId = await insertMeeting({ + date: DATE_EDGE, + titleEn: "QA Edge Middle", + startTime: "11:30:00", + endTime: "12:00:00", + }); + const lastId = await insertMeeting({ + date: DATE_EDGE, + titleEn: "QA Edge Last", + startTime: "12:00:00", + endTime: "12:30:00", + }); + // Re-load so the day query sees all three rows. + await gotoTestDate(page, DATE_EDGE); + await expect(page.locator(`[data-testid="em-row-${lastId}"]`)).toBeVisible({ + timeout: 10_000, + }); + await page.locator(`[data-testid="em-row-${onlyId}"]`).click(); + const firstPop = page.locator(`[data-testid="em-row-quick-${onlyId}"]`); + await expect(firstPop).toBeVisible(); + await expect( + firstPop.locator(`[data-testid="em-row-quick-up-${onlyId}"]`), + ).toBeDisabled(); + await expect( + firstPop.locator(`[data-testid="em-row-quick-down-${onlyId}"]`), + ).toBeEnabled(); + await page.keyboard.press("Escape"); + + await page.locator(`[data-testid="em-row-${lastId}"]`).click(); + const lastPop = page.locator(`[data-testid="em-row-quick-${lastId}"]`); + await expect(lastPop).toBeVisible(); + await expect( + lastPop.locator(`[data-testid="em-row-quick-down-${lastId}"]`), + ).toBeDisabled(); + await expect( + lastPop.locator(`[data-testid="em-row-quick-up-${lastId}"]`), + ).toBeEnabled(); + + // Sanity: the middle row exposes BOTH directions enabled. + await page.keyboard.press("Escape"); + await page.locator(`[data-testid="em-row-${middleId}"]`).click(); + const midPop = page.locator(`[data-testid="em-row-quick-${middleId}"]`); + await expect(midPop).toBeVisible(); + await expect( + midPop.locator(`[data-testid="em-row-quick-up-${middleId}"]`), + ).toBeEnabled(); + await expect( + midPop.locator(`[data-testid="em-row-quick-down-${middleId}"]`), + ).toBeEnabled(); +}); + +test("Postpone quick-action runs a 5-minute postpone end-to-end", async ({ page, }) => { const id = await insertMeeting({ + date: DATE_POSTPONE, titleEn: "QA Postpone Target", startTime: "13:00:00", endTime: "13:30:00", }); await loginViaUi(page, "admin", "admin123"); - await gotoTestDate(page); + await gotoTestDate(page, DATE_POSTPONE); await expect(page.locator(`[data-testid="em-row-${id}"]`)).toBeVisible({ timeout: 10_000, }); @@ -173,8 +264,59 @@ test("Postpone item from quick-actions popover opens the postpone dialog", async const popover = page.locator(`[data-testid="em-row-quick-${id}"]`); await expect(popover).toBeVisible(); await popover.locator(`[data-testid="em-row-quick-postpone-${id}"]`).click(); - // PostponeDialog is the same one upcoming-meeting-alert uses, so we - // assert by role=dialog presence rather than coupling to the - // dialog's internal markup. - await expect(page.getByRole("dialog")).toBeVisible({ timeout: 5_000 }); + // PostponeDialog is the same one upcoming-meeting-alert uses; drive + // its 5-minute chip → confirm flow and assert the new times land. + const dialog = page.locator('[data-testid="postpone-dialog"]'); + await expect(dialog).toBeVisible({ timeout: 5_000 }); + await dialog.locator('[data-testid="postpone-chip-5"]').click(); + await dialog.locator('[data-testid="postpone-confirm-yes"]').click(); + await expect.poll(async () => (await readMeeting(id)).start_time).toBe( + "13:05:00", + ); + const after = await readMeeting(id); + expect(after.end_time).toBe("13:35:00"); +}); + +test("clicking row affordances (grip, time cell, row-actions) does NOT open the popover", async ({ + page, +}) => { + const id = await insertMeeting({ + date: DATE_SKIP, + titleEn: "QA Skip Surfaces", + startTime: "14:00:00", + endTime: "14:30:00", + }); + await loginViaUi(page, "admin", "admin123"); + await gotoTestDate(page, DATE_SKIP); + await expect(page.locator(`[data-testid="em-row-${id}"]`)).toBeVisible({ + timeout: 10_000, + }); + const popover = page.locator(`[data-testid="em-row-quick-${id}"]`); + + // Drag grip — handled by dnd-kit, must not open the popover. + const grip = page.locator(`[data-testid="em-row-grip-${id}"]`); + if (await grip.count()) { + await grip.click({ force: true }); + await expect(popover).toBeHidden(); + } + + // Time cell — clicking it enters inline edit mode in TimeRangeCell. + // Must NOT also open the quick-actions popover. + const timeCell = page.locator(`[data-testid="em-time-${id}"]`); + await timeCell.click(); + await expect(popover).toBeHidden(); + // Clean up the inline editor so it doesn't leak into the next assertion. + await page.keyboard.press("Escape"); + + // Row-actions trigger — opens the row's own dropdown, not our popover. + const rowActions = page.locator(`[data-testid="em-row-actions-${id}"]`); + if (await rowActions.count()) { + await rowActions.click(); + await expect(popover).toBeHidden(); + await page.keyboard.press("Escape"); + } + + // Sanity: clicking an empty cell of the row DOES open the popover. + await page.locator(`[data-testid="em-row-${id}"]`).click({ position: { x: 5, y: 5 } }); + await expect(popover).toBeVisible({ timeout: 5_000 }); });