diff --git a/artifacts/api-server/src/routes/executive-meetings.ts b/artifacts/api-server/src/routes/executive-meetings.ts index bd8601c5..e7f79097 100644 --- a/artifacts/api-server/src/routes/executive-meetings.ts +++ b/artifacts/api-server/src/routes/executive-meetings.ts @@ -1,6 +1,7 @@ import { Router, type IRouter } from "express"; import { eq, + ne, asc, desc, inArray, @@ -377,10 +378,46 @@ async function nextDailyNumber( // order; cancelled meetings are pushed to the tail (N+1..N+M, by id). // The two-step "shift to negatives, then renumber" dance avoids violating // the `(meeting_date, daily_number)` unique index mid-update. +// #312: shape returned by renumberDayByStartTime so callers (PATCH, +// POST create, DELETE, duplicate) can capture how the visible +// (non-cancelled) row order changed and enrich their audit rows. The +// `before` / `after` arrays are the visible meeting IDs in +// `daily_number` order (i.e. what the user sees top-to-bottom in the +// schedule); `orderShifted` is just the boolean comparison of those +// two arrays. Cancelled rows are excluded because they are hidden in +// the UI and their tail-position is an implementation detail. +type DayOrderShift = { + date: string; + orderShifted: boolean; + before: number[]; + after: number[]; +}; + +async function snapshotVisibleDayOrder( + executor: DbExecutor, + date: string, +): Promise { + const rows = await executor + .select({ id: executiveMeetingsTable.id }) + .from(executiveMeetingsTable) + .where( + and( + eq(executiveMeetingsTable.meetingDate, date), + ne(executiveMeetingsTable.status, "cancelled"), + ), + ) + .orderBy(asc(executiveMeetingsTable.dailyNumber)); + return rows.map((r) => r.id); +} + async function renumberDayByStartTime( executor: DbExecutor, date: string, -): Promise { +): Promise { + // Snapshot the visible order BEFORE the renumber so callers can audit + // a true "from -> to" diff (#312). Cheap single SELECT inside the same + // transaction the caller passes in. + const before = await snapshotVisibleDayOrder(executor, date); // Step 1: temporarily move every row to a negative number so the // positives are free. The negatives stay unique because the previous // positives were unique. @@ -424,6 +461,13 @@ async function renumberDayByStartTime( ) AS sub WHERE em.id = sub.id `); + // Snapshot the visible order AFTER the renumber and compute the + // boolean "did the user-visible chronological order actually change?". + const after = await snapshotVisibleDayOrder(executor, date); + const orderShifted = + before.length !== after.length || + before.some((id, i) => id !== after[i]); + return { date, orderShifted, before, after }; } async function fetchMeetingWithAttendees(id: number) { @@ -759,14 +803,6 @@ router.patch( .where(eq(executiveMeetingAttendeesTable.meetingId, id)) .orderBy(asc(executiveMeetingAttendeesTable.sortOrder))); } - await logAudit(tx, { - action: "update", - entityType: "meeting", - entityId: id, - oldValue: existing, - newValue: { ...existing, ...updateValues, attendees }, - performedBy: userId, - }); // #312: Auto-sort the day(s) chronologically when the PATCH // touched a field that affects ordering or visibility. The user's // bug report — typing 13:00 on a row that sits above a 12:00 row @@ -786,21 +822,81 @@ router.patch( // - dailyNumber: a manual override; we still renumber so the // final state stays chronological (auto-sort wins over a // stray manual number coming from non-reorder paths). + // We run renumber BEFORE logAudit so the audit row can carry + // the post-renumber `dailyNumber` and an `orderShifted` flag — + // otherwise an admin reading the audit cannot tell that editing + // a meeting's start time also moved its row to a new position. const orderAffectingChange = data.startTime !== undefined || data.endTime !== undefined || data.meetingDate !== undefined || data.status !== undefined || data.dailyNumber !== undefined; + let primaryShift: DayOrderShift | null = null; + let sourceShift: DayOrderShift | null = null; + let finalDailyNumber: number | null = null; if (orderAffectingChange) { const newDate = updateValues.meetingDate ?? existing.meetingDate; - await renumberDayByStartTime(tx, newDate); + primaryShift = await renumberDayByStartTime(tx, newDate); if (newDate !== existing.meetingDate) { // Cross-day move: the source day also needs to close ranks // so its 1..N sequence stays gap-free. - await renumberDayByStartTime(tx, existing.meetingDate); + sourceShift = await renumberDayByStartTime(tx, existing.meetingDate); } + // Re-read this row's daily_number after auto-sort so the + // audit's newValue echoes the position the user actually + // sees, not the pre-renumber draft from `updateValues`. + const [row] = await tx + .select({ dailyNumber: executiveMeetingsTable.dailyNumber }) + .from(executiveMeetingsTable) + .where(eq(executiveMeetingsTable.id, id)); + finalDailyNumber = row?.dailyNumber ?? null; } + // Build the audit's newValue. When auto-sort moved this row, we + // surface (a) the row's final dailyNumber and (b) an + // `orderShifted` flag with `dayOrder` before/after arrays so an + // admin can read "the row jumped from #1 to #3 because the day + // re-sorted". `dayOrder` carries the visible (non-cancelled) ID + // sequence for the affected day(s). + const auditNewValue: Record = { + ...existing, + ...updateValues, + attendees, + }; + if (finalDailyNumber !== null) { + auditNewValue.dailyNumber = finalDailyNumber; + } + const shiftsForAudit: Array<{ + date: string; + before: number[]; + after: number[]; + }> = []; + if (primaryShift && primaryShift.orderShifted) { + shiftsForAudit.push({ + date: primaryShift.date, + before: primaryShift.before, + after: primaryShift.after, + }); + } + if (sourceShift && sourceShift.orderShifted) { + shiftsForAudit.push({ + date: sourceShift.date, + before: sourceShift.before, + after: sourceShift.after, + }); + } + if (shiftsForAudit.length > 0) { + auditNewValue.orderShifted = true; + auditNewValue.dayOrder = shiftsForAudit; + } + await logAudit(tx, { + action: "update", + entityType: "meeting", + entityId: id, + oldValue: existing, + newValue: auditNewValue, + performedBy: userId, + }); }); const updated = await fetchMeetingWithAttendees(id); // Realtime: when the meeting was rescheduled to a different day we diff --git a/artifacts/api-server/tests/executive-meetings.test.mjs b/artifacts/api-server/tests/executive-meetings.test.mjs index 35fc496e..06b5b84b 100644 --- a/artifacts/api-server/tests/executive-meetings.test.mjs +++ b/artifacts/api-server/tests/executive-meetings.test.mjs @@ -1640,6 +1640,103 @@ test("Auto-sort: POST /duplicate slots the cloned meeting at its chronological p "duplicate must slot at its chronological position on the target day"); }); +// #312: when a PATCH (e.g. typing 13:00 on the row currently sitting +// above a 12:00 row) triggers auto-sort and the row visibly moves to a +// new dailyNumber, the audit row for that PATCH must record both the +// post-renumber dailyNumber and an `orderShifted: true` flag with the +// before/after visible-row order. Without this an admin reading the +// audit cannot tell that the user's startTime edit also moved the row +// from `#1` to `#3`. +test("Auto-sort: PATCH that reorders the day records orderShifted + final dailyNumber + dayOrder in the audit row", async () => { + const d = "2050-07-12"; + const a = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "أ", titleEn: "A", meetingDate: d, + startTime: "09:00", endTime: "09:30", + }); + const b = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "ب", titleEn: "B", meetingDate: d, + startTime: "10:00", endTime: "10:30", + }); + const c = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "ج", titleEn: "C", meetingDate: d, + startTime: "11:00", endTime: "11: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); + // Sanity: starting visible order is A(09), B(10), C(11) at #1,#2,#3. + let visible = await assertDayChronological(d); + assert.deepEqual(visible.map((m) => m.id), [A.id, B.id, C.id]); + + // Move A from 09:00 -> 13:00 (the user's exact bug scenario). After + // auto-sort the visible order should become B(10), C(11), A(13) and + // A's dailyNumber should be 3. + const patch = await api(adminCookie, "PATCH", + `/api/executive-meetings/${A.id}`, + { startTime: "13:00", endTime: "13:30" }); + assert.equal(patch.status, 200); + visible = await assertDayChronological(d); + assert.deepEqual(visible.map((m) => m.id), [B.id, C.id, A.id]); + + // Read the audit row for THIS PATCH and assert the shift metadata. + const auditRows = await pool.query( + `SELECT action, entity_type, entity_id, new_value + FROM executive_meeting_audit_logs + WHERE action = 'update' + AND entity_type = 'meeting' + AND entity_id = $1 + ORDER BY id DESC LIMIT 1`, + [A.id], + ); + assert.equal(auditRows.rowCount, 1, "expected one update audit row for A"); + const newValue = auditRows.rows[0].new_value; + assert.ok(newValue, "audit new_value must be present"); + assert.equal(newValue.orderShifted, true, + "audit must flag orderShifted=true when auto-sort moved the row"); + assert.equal(newValue.dailyNumber, 3, + "audit's newValue.dailyNumber must echo the post-renumber position (#3), not the pre-renumber #1"); + assert.ok(Array.isArray(newValue.dayOrder), + "audit must include dayOrder array describing the visible-row shift"); + const shift = newValue.dayOrder.find((s) => s.date === d); + assert.ok(shift, `dayOrder must include an entry for the affected date ${d}`); + assert.deepEqual(shift.before, [A.id, B.id, C.id], + "dayOrder.before must be the visible row IDs in pre-renumber dailyNumber order"); + assert.deepEqual(shift.after, [B.id, C.id, A.id], + "dayOrder.after must be the visible row IDs in post-renumber dailyNumber order"); +}); + +// #312: a non-order-affecting PATCH (e.g. just changing the title) +// must NOT add orderShifted/dayOrder to the audit row, since auto-sort +// did not run. This guards against accidentally treating every PATCH +// as a reorder in the audit UI. +test("Auto-sort: PATCH that does not touch time/date/status leaves orderShifted absent from the audit", async () => { + const d = "2050-07-13"; + const a = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "أ", titleEn: "A", meetingDate: d, + startTime: "09:00", endTime: "09:30", + }); + const A = await a.json(); created.meetingIds.push(A.id); + + const patch = await api(adminCookie, "PATCH", + `/api/executive-meetings/${A.id}`, + { titleEn: "A renamed" }); + assert.equal(patch.status, 200); + + const auditRows = await pool.query( + `SELECT new_value FROM executive_meeting_audit_logs + WHERE action = 'update' AND entity_type = 'meeting' + AND entity_id = $1 + ORDER BY id DESC LIMIT 1`, + [A.id], + ); + assert.equal(auditRows.rowCount, 1); + const newValue = auditRows.rows[0].new_value; + assert.ok(!("orderShifted" in newValue), + "title-only PATCH must not add orderShifted to the audit"); + assert.ok(!("dayOrder" in newValue), + "title-only PATCH must not add dayOrder to the audit"); +}); + test("Reorder: user without executive role is denied (403)", async () => { const m = await api(adminCookie, "POST", "/api/executive-meetings", { titleAr: "ا", titleEn: "A", meetingDate: today,