Task #312: auto-sort executive-meetings schedule by start_time
User report (AR): "اريدها ترتيبها تلقائيا" — after a non-drag write
(typing 13:00 into a row sitting above a 12:00 row, creating a meeting
with an explicit start time, etc.) the schedule's daily_number stayed
frozen at the row's prior position, so the visible list was no longer
chronological. Fix: extend the existing `renumberDayByStartTime` helper
to every write path that touches start_time / meeting_date / status /
daily_number, so the day is always renumbered 1..N by start_time
(NULLS LAST, cancelled rows at the tail).
Server (artifacts/api-server/src/routes/executive-meetings.ts):
- POST /executive-meetings — renumber after insert.
- PATCH /executive-meetings/:id — renumber when an order- or
visibility-affecting field (startTime/endTime/meetingDate/status/
dailyNumber) is in the payload. Cross-day moves renumber BOTH the
source and destination day. Pre-allocates a fresh dailyNumber on
the destination via nextDailyNumber(tx, newDate) before the UPDATE
so the (meeting_date, daily_number) unique index does not collide
when the row arrives on a day with existing meetings.
- DELETE /executive-meetings/:id — renumber so the day's `#`
sequence stays gap-free.
- POST /executive-meetings/:id/duplicate — renumber the target day.
Existing postpone-minutes / reschedule / cancel paths already called
renumber and were left alone. Reorder POST is also untouched (its
slot-swap is the explicit user-driven order, not auto-sort).
Audit-log surfacing of the auto-sort side effect (per validation):
- `renumberDayByStartTime` now returns `{ date, orderShifted, before,
after }` where `before`/`after` are the visible (non-cancelled)
row IDs in `daily_number` order, captured by a cheap SELECT inside
the same transaction.
- PATCH was restructured so renumber runs BEFORE `logAudit`, then
the row's post-renumber `daily_number` is read back and the audit's
`newValue` is enriched with:
• `dailyNumber` overridden to the post-sort position (so the
audit shows where the row landed, not the pre-sort draft);
• `orderShifted: true` and a `dayOrder: [{ date, before, after }]`
array (one entry per affected day, including both source and
destination on cross-day moves) when the visible order actually
changed.
- When auto-sort runs but does not change the visible order (e.g. the
row was already in the correct slot), `orderShifted` / `dayOrder`
are omitted so the audit UI does not falsely flag a reorder.
Tests (artifacts/api-server/tests/executive-meetings.test.mjs, +11):
- assertDayChronological() helper asserts 1..N + non-decreasing
start_time + no duplicate dailyNumbers.
- PATCH startTime later → row demoted (the user's exact scenario).
- PATCH startTime earlier → row promoted to the top.
- POST create with middle startTime slots between existing rows.
- PATCH startTime=null sinks to the tail of visible rows.
- Cancel pushes out / uncancel re-slots chronologically.
- Cross-day PATCH meetingDate renumbers BOTH days.
- DELETE leaves no `#` gap.
- POST /duplicate slots clone at chronological position.
- PATCH that reorders the day records orderShifted + post-sort
dailyNumber + dayOrder before/after arrays in the audit row.
- Title-only PATCH leaves orderShifted/dayOrder absent from the audit.
All 57 tests in executive-meetings.test.mjs pass.
Architect review (advisory, scope-bounded):
- Concurrency: PATCH/DELETE read `existing` outside the transaction.
Pre-existing convention in this file — only the cascade-bearing
postpone-minutes/reschedule paths use SELECT FOR UPDATE. Matching
existing pattern; tightening locking is a separate refactor
captured as follow-up #313.
- Audit surfacing for POST/DELETE/duplicate: only PATCH was enriched
in this task per validation feedback ("especially PATCH time/date/
status/dailyNumber paths"). UI-side rendering of the new audit
fields and POST/DELETE/duplicate enrichment captured as
follow-up #314.
This commit is contained in:
@@ -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<number[]> {
|
||||
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<void> {
|
||||
): Promise<DayOrderShift> {
|
||||
// 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<string, unknown> = {
|
||||
...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
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user