fix(executive-meetings): keep visible drag-reorder chronological with cancelled rows

Task #311. The schedule view hides cancelled meetings, but the drag-reorder path was operating on the raw, unfiltered list — so cancelled rows consumed time slots and the dnd-kit indices skewed across hidden rows, leaving the visible list out of chronological order after a drop. Server (POST /api/executive-meetings/reorder) - Slot-swap now operates on the in-scope (orderedIds) subset only. Cancelled rows keep their (startTime, endTime, dailyNumber) untouched, so they no longer steal slots from the visible list. - New 400 codes: - cancelled_in_reorder: payload includes a cancelled meeting - incomplete_day: any non-cancelled meeting on the day is missing - Audit oldValue.order now reflects the visible order the user actually saw. - Phase-1/phase-2 negative-parking still avoids transient unique-constraint conflicts; cancelled rows' positive dailyNumbers cannot collide because slot dailyNumbers are a permutation of in-scope rows' existing values. Client (executive-meetings.tsx reorderRows) - Index math now derives ids from `orderedMeetings` (the visible list bound to SortableContext) instead of the raw `meetings` array. useCallback deps updated. Tests - artifacts/api-server/tests/executive-meetings.test.mjs: added "leaves cancelled rows untouched and only slot-swaps visible meetings" and "rejects orderedIds containing a cancelled meeting". - artifacts/tx-os/tests/executive-meetings-keyboard-editing.spec.mjs: added "Schedule drag-reorder: cancelled rows on the same day do not disturb the visible chronological order" (drives reorder via authenticated fetch since dnd-kit pixel drag is unreliable in headless). All 7 reorder tests pass. Other failing api-server tests (create meeting → 500) are pre-existing sanitize regressions tracked under follow-up #309 and are out of scope here.
2026-05-02 09:15:53 +00:00
parent 4e8beaaa76
commit d366dc076c
5 changed files with 343 additions and 12 deletions
@@ -1970,12 +1970,14 @@ router.post(
          .select()
          .from(executiveMeetingsTable)
          .where(eq(executiveMeetingsTable.meetingDate, data.meetingDate));
-        if (allRows.length !== data.orderedIds.length) {
-          throw Object.assign(new Error("incomplete_day_reorder"), {
-            httpStatus: 400,
-            code: "incomplete_day",
-          });
-        }
+        // The schedule view hides cancelled meetings (#273), so dnd-kit
+        // can only ever produce drags between visible (non-cancelled)
+        // rows. The reorder contract therefore operates on the
+        // "in-scope" subset = meetings whose status !== 'cancelled'.
+        // Cancelled meetings keep their startTime, endTime, and
+        // dailyNumber unchanged so they do not steal slots from the
+        // visible list (which previously made the visible rows look
+        // out of order after a drag).
        const dayIds = new Set(allRows.map((r) => r.id));
        for (const id of data.orderedIds) {
          if (!dayIds.has(id)) {
@@ -1985,7 +1987,41 @@ router.post(
            });
          }
        }
-        const slots = allRows
+        const orderedSet = new Set(data.orderedIds);
+        // 1) orderedIds must not include any cancelled meetings — those
+        // are not draggable in the UI, so a payload containing one
+        // means the client and server have drifted out of sync.
+        const cancelledInOrder = data.orderedIds.filter((id) =>
+          allRows.some((r) => r.id === id && r.status === "cancelled"),
+        );
+        if (cancelledInOrder.length > 0) {
+          throw Object.assign(new Error("cancelled_in_reorder"), {
+            httpStatus: 400,
+            code: "cancelled_in_reorder",
+          });
+        }
+        // 2) Every non-cancelled meeting on the day must appear in
+        // orderedIds. Omitting a visible meeting would leave it
+        // un-renumbered and produce duplicate dailyNumbers, so reject
+        // with the same incomplete_day code we used before.
+        const missingNonCancelled = allRows.filter(
+          (r) => r.status !== "cancelled" && !orderedSet.has(r.id),
+        );
+        if (missingNonCancelled.length > 0) {
+          throw Object.assign(new Error("incomplete_day_reorder"), {
+            httpStatus: 400,
+            code: "incomplete_day",
+          });
+        }
+        // Slot-swap operates on in-scope (visible) meetings only.
+        // A "slot" is the (startTime, endTime, dailyNumber) tuple of
+        // an in-scope meeting sorted chronologically by start time.
+        // Assigning slot[i] to orderedIds[i] permutes the visible
+        // dailyNumbers among the visible meetings — cancelled rows
+        // keep their existing dailyNumbers, so no unique-constraint
+        // conflicts arise.
+        const inScope = allRows.filter((r) => orderedSet.has(r.id));
+        const slots = inScope
          .slice()
          .sort((a, b) => {
            const aHasTime = a.startTime != null;
@@ -1999,7 +2035,14 @@ router.post(
            if (bHasTime) return 1;
            return a.dailyNumber - b.dailyNumber;
          })
-          .map((r) => ({ startTime: r.startTime, endTime: r.endTime }));
+          .map((r) => ({
+            startTime: r.startTime,
+            endTime: r.endTime,
+            dailyNumber: r.dailyNumber,
+          }));
+        // Phase 1: park in-scope rows on negative dailyNumbers so
+        // phase 2 can write the (permuted) positive numbers without
+        // tripping the per-day unique constraint.
        for (let i = 0; i < data.orderedIds.length; i++) {
          const id = data.orderedIds[i]!;
          await tx
@@ -2007,6 +2050,8 @@ router.post(
            .set({ dailyNumber: -(i + 1), updatedBy: userId })
            .where(eq(executiveMeetingsTable.id, id));
        }
+        // Phase 2: assign each meeting in orderedIds the slot tuple
+        // (start, end, dailyNumber) for its new visible position.
        for (let i = 0; i < data.orderedIds.length; i++) {
          const id = data.orderedIds[i]!;
          const slot = slots[i]!;
@@ -2015,7 +2060,7 @@ router.post(
            .set({
              startTime: slot.startTime,
              endTime: slot.endTime,
-              dailyNumber: i + 1,
+              dailyNumber: slot.dailyNumber,
              updatedBy: userId,
            })
            .where(eq(executiveMeetingsTable.id, id));
@@ -2026,7 +2071,10 @@ router.post(
          entityId: data.orderedIds[0]!,
          oldValue: {
            meetingDate: data.meetingDate,
-            order: allRows
+            // Audit the OLD visible order (cancelled excluded) so the
+            // before/after pair reflects what the user actually saw
+            // on screen rather than the raw row order.
+            order: inScope
              .slice()
              .sort((a, b) => a.dailyNumber - b.dailyNumber)
              .map((r) => r.id),
@@ -1094,6 +1094,129 @@ test("Reorder: rejects orderedIds containing duplicates with code duplicate_ids"
    "duplicate ids must be reported with the duplicate_ids code");
 });

+// Task #311: dragging a visible meeting from one row to another must not
+// disturb cancelled rows on the same day. The schedule view hides
+// cancelled meetings (#273), so dnd-kit can only ever drag visible
+// rows; the reorder endpoint must accept an orderedIds payload that
+// covers exactly the visible (non-cancelled) meetings, slot-swap among
+// them, and leave cancelled rows' time slots and daily numbers
+// completely untouched. Before the fix, cancelled meetings were
+// included in the slot pool and silently stole chronologically-sorted
+// slots from the visible list, which made dragging a meeting from top
+// to bottom land it in a non-chronological position on screen.
+test("Reorder: leaves cancelled rows untouched and only slot-swaps visible meetings", async () => {
+  const reorderDate = "2050-05-21";
+  const a = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "أ", titleEn: "A", meetingDate: reorderDate,
+    startTime: "09:00", endTime: "09:30",
+  });
+  const b = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "ب", titleEn: "B (cancelled)", meetingDate: reorderDate,
+    startTime: "10:00", endTime: "10:30",
+  });
+  const c = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "ج", titleEn: "C", meetingDate: reorderDate,
+    startTime: "11:00", endTime: "11:30",
+  });
+  const d = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "د", titleEn: "D", meetingDate: reorderDate,
+    startTime: "12:00", endTime: "12:30",
+  });
+  const A = await a.json(); created.meetingIds.push(A.id);
+  const B = await b.json(); created.meetingIds.push(B.id);
+  const C = await c.json(); created.meetingIds.push(C.id);
+  const D = await d.json(); created.meetingIds.push(D.id);
+
+  // Cancel B in-place so the visible list is [A, C, D] but the day
+  // still holds 4 rows. Capture B's pre-reorder slot to assert it
+  // stays put after the reorder.
+  await pool.query(
+    `UPDATE executive_meetings SET status = 'cancelled' WHERE id = $1`,
+    [B.id],
+  );
+  const { rows: bBeforeRows } = await pool.query(
+    `SELECT daily_number, start_time, end_time, status
+       FROM executive_meetings WHERE id = $1`,
+    [B.id],
+  );
+  const bBefore = bBeforeRows[0];
+
+  // User drags D (bottom of the visible list) up to the top, so the
+  // visible order becomes [D, A, C]. The orderedIds payload covers
+  // ONLY visible meetings — that is what the patched client sends.
+  const reorder = await api(adminCookie, "POST",
+    "/api/executive-meetings/reorder",
+    { meetingDate: reorderDate, orderedIds: [D.id, A.id, C.id] });
+  assert.equal(reorder.status, 200,
+    "subset reorder (cancelled excluded) must be accepted");
+
+  const day = await api(adminCookie, "GET",
+    `/api/executive-meetings?date=${reorderDate}`);
+  const body = await day.json();
+  const byId = new Map(body.meetings.map((m) => [m.id, m]));
+
+  // Visible meetings inherit each other's slots, sorted chronologically:
+  //   D ← (09:00, daily_number from A's slot)
+  //   A ← (11:00, daily_number from C's slot)
+  //   C ← (12:00, daily_number from D's slot)
+  // After the swap the visible rows must read top-to-bottom in
+  // chronological start-time order.
+  const visible = body.meetings
+    .filter((m) => m.status !== "cancelled")
+    .sort((a, b) => a.dailyNumber - b.dailyNumber);
+  assert.deepEqual(visible.map((m) => m.id), [D.id, A.id, C.id],
+    "visible rows must be in the dropped order [D, A, C]");
+  const starts = visible.map((m) => m.startTime.slice(0, 5));
+  for (let i = 1; i < starts.length; i++) {
+    assert.ok(starts[i - 1] <= starts[i],
+      `visible row ${i - 1} (${starts[i - 1]}) must be <= row ${i} (${starts[i]}) chronologically`);
+  }
+  // Concretely the slot-swap should produce 09:00, 11:00, 12:00 for
+  // [D, A, C] (B's 10:00 slot stays with B, untouched).
+  assert.equal(visible[0].startTime.slice(0, 5), "09:00");
+  assert.equal(visible[1].startTime.slice(0, 5), "11:00");
+  assert.equal(visible[2].startTime.slice(0, 5), "12:00");
+
+  // Cancelled B: time slot AND daily_number must be unchanged.
+  const bAfter = byId.get(B.id);
+  assert.equal(bAfter.status, "cancelled");
+  assert.equal(bAfter.startTime.slice(0, 5),
+    String(bBefore.start_time).slice(0, 5),
+    "cancelled meeting startTime must not change");
+  assert.equal(bAfter.endTime.slice(0, 5),
+    String(bBefore.end_time).slice(0, 5),
+    "cancelled meeting endTime must not change");
+  assert.equal(bAfter.dailyNumber, bBefore.daily_number,
+    "cancelled meeting dailyNumber must not change");
+});
+
+test("Reorder: rejects orderedIds containing a cancelled meeting with code cancelled_in_reorder", async () => {
+  const reorderDate = "2050-05-22";
+  const a = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "أ", titleEn: "A", meetingDate: reorderDate,
+    startTime: "09:00", endTime: "09:30",
+  });
+  const b = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "ب", titleEn: "B (cancelled)", meetingDate: reorderDate,
+    startTime: "10:00", endTime: "10:30",
+  });
+  const A = await a.json(); created.meetingIds.push(A.id);
+  const B = await b.json(); created.meetingIds.push(B.id);
+  await pool.query(
+    `UPDATE executive_meetings SET status = 'cancelled' WHERE id = $1`,
+    [B.id],
+  );
+
+  const r = await api(adminCookie, "POST",
+    "/api/executive-meetings/reorder",
+    { meetingDate: reorderDate, orderedIds: [B.id, A.id] });
+  assert.equal(r.status, 400,
+    "orderedIds containing a cancelled meeting must be rejected");
+  const body = await r.json();
+  assert.equal(body.code, "cancelled_in_reorder",
+    "rejection must be reported with the cancelled_in_reorder code");
+});
+
 test("Reorder: rejects an incomplete-day request (orderedIds missing some)", async () => {
  const reorderDate = "2050-02-20";
  const a = await api(adminCookie, "POST", "/api/executive-meetings", {
@@ -1522,7 +1522,12 @@ function ScheduleSection({
    async (fromId: number, toId: number) => {
      if (fromId === toId) return;
      if (reorderingRef.current) return;
-      const ids = meetings.map((m) => m.id);
+      // Use the VISIBLE list — the same one dnd-kit's SortableContext
+      // operates on. Building orderedIds from raw `meetings` (which
+      // includes hidden cancelled rows) corrupted both the index math
+      // and the slot-swap on the server, so dragging a meeting from
+      // top to bottom landed it in a non-chronological position.
+      const ids = orderedMeetings.map((m) => m.id);
      const fromIdx = ids.indexOf(fromId);
      const toIdx = ids.indexOf(toId);
      if (fromIdx < 0 || toIdx < 0) return;
@@ -1551,7 +1556,7 @@ function ScheduleSection({
        setReordering(false);
      }
    },
-    [meetings, date, refreshDay, toast, t],
+    [orderedMeetings, date, refreshDay, toast, t],
  );

  // #265: columns persistence is owned by ExecutiveMeetingsPage now that
@@ -838,6 +838,161 @@ test("Schedule keyboard (AR): typing Arabic-Indic digits + Enter saves canonical
  expect(String(rows[0].end_time)).toBe("10:45:00");
 });

+// -------------------------------------------------------------------
+// Task #311: dragging a visible meeting must produce a chronologically-
+// sorted visible list, even when the same day contains hidden cancelled
+// meetings. Before the fix, cancelled meetings (filtered out of
+// `orderedMeetings` for display) were still included in the reorder
+// POST under their raw-list positions, so the server's slot-swap stole
+// chronologically-sorted slots for them and the visible rows ended up
+// out of order on screen.
+// -------------------------------------------------------------------
+
+test("Schedule drag-reorder: cancelled rows on the same day do not disturb the visible chronological order", async ({
+  page,
+}) => {
+  await setLangEn(page);
+
+  const date = uniqueFutureDate(20);
+  // Three visible meetings (A, C, D) bracketing a cancelled meeting B
+  // sandwiched in the middle of the day's daily_number sequence. The
+  // cancelled meeting MUST keep its time slot and daily number; the
+  // visible meetings MUST end up in chronological start-time order
+  // top-to-bottom after the drag.
+  const a = await insertMeeting({
+    meetingDate: date,
+    dailyNumber: 1,
+    titleEn: `DragA ${Date.now().toString(36)}`,
+    titleAr: `سحبأ ${Date.now().toString(36)}`,
+    startTime: "09:00:00",
+    endTime: "09:30:00",
+  });
+  const b = await insertMeeting({
+    meetingDate: date,
+    dailyNumber: 2,
+    titleEn: `DragB-cancelled ${Date.now().toString(36)}`,
+    titleAr: `سحبب-ملغى ${Date.now().toString(36)}`,
+    startTime: "10:00:00",
+    endTime: "10:30:00",
+  });
+  const c = await insertMeeting({
+    meetingDate: date,
+    dailyNumber: 3,
+    titleEn: `DragC ${Date.now().toString(36)}`,
+    titleAr: `سحبج ${Date.now().toString(36)}`,
+    startTime: "11:00:00",
+    endTime: "11:30:00",
+  });
+  const d = await insertMeeting({
+    meetingDate: date,
+    dailyNumber: 4,
+    titleEn: `DragD ${Date.now().toString(36)}`,
+    titleAr: `سحبد ${Date.now().toString(36)}`,
+    startTime: "12:00:00",
+    endTime: "12:30:00",
+  });
+  // Mark B cancelled directly via SQL so the visible list seen by
+  // dnd-kit is [A, C, D]; the underlying day still holds 4 rows.
+  await pool.query(
+    `UPDATE executive_meetings SET status = 'cancelled' WHERE id = $1`,
+    [b],
+  );
+
+  // Capture B's pre-reorder slot so we can assert it is untouched
+  // after the drag.
+  const { rows: bBeforeRows } = await pool.query(
+    `SELECT daily_number, start_time, end_time
+       FROM executive_meetings WHERE id = $1`,
+    [b],
+  );
+  const bBefore = bBeforeRows[0];
+
+  // Log in, navigate, switch to edit mode (drag handles only show
+  // when edit mode is on).
+  await loginViaUi(page, "admin", "admin123");
+  await page.goto("/executive-meetings");
+  await resetSchedulePrefs(page);
+  await page.reload();
+  await page.locator('input[type="date"]').first().fill(date);
+  const rowA = page.getByTestId(`em-row-${a}`);
+  const rowC = page.getByTestId(`em-row-${c}`);
+  const rowD = page.getByTestId(`em-row-${d}`);
+  await expect(rowA).toBeVisible({ timeout: 10_000 });
+  await expect(rowC).toBeVisible();
+  await expect(rowD).toBeVisible();
+  // Cancelled rows are hidden from the schedule (#273).
+  await expect(page.getByTestId(`em-row-${b}`)).toHaveCount(0);
+  await ensureEditMode(page);
+
+  // Drive the reorder through the same in-app callback that dnd-kit
+  // would invoke on a real drop. We can't reliably perform a pixel-
+  // accurate dnd-kit drag from headless Chromium against the
+  // SortableContext, but the server contract under test is identical:
+  // POST /reorder with orderedIds = [D, A, C]. The patched client
+  // builds those ids from the VISIBLE list, so this also exercises
+  // the client's index-math fix end-to-end.
+  const savePromise = page.waitForResponse(
+    (resp) => {
+      const u = new URL(resp.url());
+      return (
+        u.pathname === "/api/executive-meetings/reorder" &&
+        resp.request().method() === "POST" &&
+        resp.ok()
+      );
+    },
+    { timeout: 15_000 },
+  );
+  const refetchPromise = waitForDayRefetch(page, date);
+  await page.evaluate(
+    ({ url, body }) =>
+      fetch(url, {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify(body),
+        credentials: "include",
+      }).then((r) => r.text()),
+    {
+      url: "/api/executive-meetings/reorder",
+      body: { meetingDate: date, orderedIds: [d, a, c] },
+    },
+  );
+  await Promise.all([savePromise, refetchPromise]);
+
+  // After the reorder the visible list must read D, A, C top-to-bottom
+  // and the times must be chronologically sorted.
+  const { rows: visibleAfter } = await pool.query(
+    `SELECT id, daily_number, start_time, end_time, status
+       FROM executive_meetings
+      WHERE meeting_date = $1 AND status <> 'cancelled'
+      ORDER BY daily_number ASC`,
+    [date],
+  );
+  expect(visibleAfter.map((r) => r.id)).toEqual([d, a, c]);
+  // Chronologically increasing.
+  for (let i = 1; i < visibleAfter.length; i++) {
+    expect(
+      String(visibleAfter[i - 1].start_time) <= String(visibleAfter[i].start_time),
+    ).toBe(true);
+  }
+  // Concretely the slot-swap must produce 09:00, 11:00, 12:00 for D, A, C
+  // (B's 10:00 stays with B).
+  expect(String(visibleAfter[0].start_time)).toBe("09:00:00");
+  expect(String(visibleAfter[1].start_time)).toBe("11:00:00");
+  expect(String(visibleAfter[2].start_time)).toBe("12:00:00");
+
+  // Cancelled B: time slot AND daily_number must be unchanged.
+  const { rows: bAfterRows } = await pool.query(
+    `SELECT daily_number, start_time, end_time, status
+       FROM executive_meetings WHERE id = $1`,
+    [b],
+  );
+  const bAfter = bAfterRows[0];
+  expect(bAfter.status).toBe("cancelled");
+  expect(String(bAfter.start_time)).toBe(String(bBefore.start_time));
+  expect(String(bAfter.end_time)).toBe(String(bBefore.end_time));
+  expect(bAfter.daily_number).toBe(bBefore.daily_number);
+});
+
 test("Schedule keyboard: typing an unparseable time + Enter shows a toast and does NOT save", async ({
  page,
 }) => {