Add automated tests for the Phase-2 Executive Meetings endpoints

Task #112 — locks in RBAC, transactional safety, and the router.param numeric-id guard for the Executive Meetings module so future regressions fail loudly instead of silently. What was added (all in artifacts/api-server/tests/executive-meetings.test.mjs): 1. "Meeting CRUD permissions: coordinator forbidden, lead allowed, admin allowed" — confirms requireMutate denies executive_coordinator on POST/PATCH/DELETE while still letting them GET, and that executive_coord_lead and admin can mutate. 2. "Requests: coordinator can submit + withdraw their own request" — covers the coordinator-as-requester path, asserts only the original requester can withdraw, and that withdraw on an already-withdrawn request returns 409 / code:bad_state instead of crashing. 3. "Requests: admin can reject; rejected requests cannot be re-reviewed" — covers the rejection branch of PATCH /requests/:id, blocks non-approvers, and asserts that re-reviewing or late-withdrawing a reviewed request returns 409. 4. "Tasks: assignee can update status; non-assignee non-mutator gets 403" — the assignedTo carve-out works for status flips, mutator-only fields are silently dropped for the assignee, and a sibling coordinator who isn't the assignee is rejected. 5. "Font settings: PUT then GET returns the user-scoped row roundtrip" — covers PUT and the PATCH alias, then GETs and asserts the saved values are echoed back. 6. "router.param: non-numeric :id returns 404 across endpoints (no crash)" — exhaustively walks the GET/PATCH/DELETE/PUT/POST routes with non-digit ids ("abc", "123abc", "-1") and asserts each returns 404 instead of crashing inside Number(req.params.id). 7. "Transactional safety: a failing audit insert rolls back the parent DELETE" — installs a temporary BEFORE INSERT trigger on executive_meeting_audit_logs that raises only for this specific meeting's delete audit row, then DELETEs the meeting and asserts 500 + the row is still in the database. Trigger is dropped in a finally so other tests are unaffected. Side note: \`pnpm install\` was needed to land pdfkit + bidi-js so the API server could build (those packages were missing from the on-disk node_modules). The two pre-existing PDF-download tests still fail with 500 in this env — captured as follow-up #172, not within scope here.
2026-04-29 18:27:15 +00:00
parent a85a8f5d5f
commit ee565b2086
1 changed files with 319 additions and 0 deletions
@@ -914,3 +914,322 @@ test("Reorder: user without executive role is denied (403)", async () => {
    { meetingDate: today, orderedIds: [M.id] });
  assert.equal(r.status, 403);
 });
+
+// ---------------------------------------------------------------------
+// Phase-2 task #112 additions:
+//   - per-role meeting CRUD permissions
+//   - request reject / withdraw flows
+//   - task assignee-only status updates
+//   - font-settings PATCH/GET roundtrip
+//   - non-numeric :id returns 404 (router.param guard) instead of crashing
+//   - audit-insert failure rolls back the parent mutation (transactional)
+// ---------------------------------------------------------------------
+
+test("Meeting CRUD permissions: coordinator forbidden, lead allowed, admin allowed", async () => {
+  // executive_coordinator is in REQUEST_ROLES but NOT in MUTATE_ROLES, so
+  // a direct POST/PATCH/DELETE on /executive-meetings must come back 403.
+  const coordCreate = await api(coordCookie, "POST", "/api/executive-meetings", {
+    titleAr: "م", titleEn: "M", meetingDate: today,
+  });
+  assert.equal(coordCreate.status, 403,
+    "executive_coordinator must not be able to POST a meeting");
+
+  // executive_coord_lead is in MUTATE_ROLES, so the same call must succeed.
+  const leadCreate = await api(leadCookie, "POST", "/api/executive-meetings", {
+    titleAr: "ل", titleEn: "L", meetingDate: today,
+  });
+  assert.equal(leadCreate.status, 201,
+    "executive_coord_lead must be able to POST a meeting");
+  const leadMeeting = await leadCreate.json();
+  created.meetingIds.push(leadMeeting.id);
+
+  // Coordinator can still GET (read role) but cannot PATCH or DELETE.
+  const coordRead = await api(coordCookie, "GET",
+    `/api/executive-meetings/${leadMeeting.id}`);
+  assert.equal(coordRead.status, 200,
+    "coordinator should still be allowed to read meeting details");
+
+  const coordPatch = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/${leadMeeting.id}`, { notes: "no" });
+  assert.equal(coordPatch.status, 403,
+    "coordinator must not PATCH meetings directly");
+
+  const coordDelete = await api(coordCookie, "DELETE",
+    `/api/executive-meetings/${leadMeeting.id}`);
+  assert.equal(coordDelete.status, 403,
+    "coordinator must not DELETE meetings directly");
+
+  // Lead can mutate.
+  const leadPatch = await api(leadCookie, "PATCH",
+    `/api/executive-meetings/${leadMeeting.id}`, { notes: "ok" });
+  assert.equal(leadPatch.status, 200,
+    "executive_coord_lead must be able to PATCH meetings");
+});
+
+test("Requests: coordinator can submit + withdraw their own request", async () => {
+  const reqRes = await api(coordCookie, "POST",
+    "/api/executive-meetings/requests", {
+      requestType: "note",
+      requestDetails: { note: "coord submitting" },
+    });
+  assert.equal(reqRes.status, 201,
+    "coordinator (REQUEST_ROLES) must be able to POST a request");
+  const reqRow = await reqRes.json();
+  assert.equal(reqRow.status, "new");
+  assert.equal(reqRow.requestedBy, coordUserId);
+  created.requestIds.push(reqRow.id);
+
+  // A different coordinator cannot withdraw someone else's request.
+  const otherCoord = await createUser("em_coord_other", "executive_coordinator");
+  const otherCookie = await login(otherCoord.username, TEST_PASSWORD);
+  const otherWithdraw = await api(otherCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { action: "withdraw" });
+  assert.equal(otherWithdraw.status, 403,
+    "non-requester must not be able to withdraw someone else's request");
+
+  // The original requester can withdraw while still 'new'.
+  const withdraw = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { action: "withdraw" });
+  assert.equal(withdraw.status, 200);
+  const withdrawn = await withdraw.json();
+  assert.equal(withdrawn.status, "withdrawn");
+
+  // Once withdrawn, repeated withdraw must 409 (bad_state), not crash.
+  const again = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { action: "withdraw" });
+  assert.equal(again.status, 409);
+  const againBody = await again.json();
+  assert.equal(againBody.code, "bad_state");
+});
+
+test("Requests: admin can reject; rejected requests cannot be re-reviewed", async () => {
+  const reqRes = await api(coordCookie, "POST",
+    "/api/executive-meetings/requests", {
+      requestType: "note",
+      requestDetails: { note: "to be rejected" },
+    });
+  assert.equal(reqRes.status, 201);
+  const reqRow = await reqRes.json();
+  created.requestIds.push(reqRow.id);
+
+  // Coordinator (no APPROVE_ROLES) cannot reject.
+  const coordReject = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { status: "rejected", reviewNotes: "not allowed" });
+  assert.equal(coordReject.status, 403,
+    "non-approver must not be able to reject a request");
+
+  // Admin can reject. The response must reflect the new status and reviewer.
+  const reject = await api(adminCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { status: "rejected", reviewNotes: "no" });
+  assert.equal(reject.status, 200);
+  const rejected = await reject.json();
+  assert.equal(rejected.status, "rejected");
+  assert.equal(rejected.reviewedBy, adminUserId);
+  assert.equal(rejected.reviewNotes, "no");
+
+  // After review the original requester can no longer withdraw.
+  const lateWithdraw = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { action: "withdraw" });
+  assert.equal(lateWithdraw.status, 409,
+    "withdrawing an already-reviewed request must 409, not crash");
+
+  // Trying to re-review must also 409 with code:bad_state.
+  const reReview = await api(adminCookie, "PATCH",
+    `/api/executive-meetings/requests/${reqRow.id}`,
+    { status: "approved", reviewNotes: "flip" });
+  assert.equal(reReview.status, 409);
+  const reBody = await reReview.json();
+  assert.equal(reBody.code, "bad_state");
+});
+
+test("Tasks: assignee can update status; non-assignee non-mutator gets 403", async () => {
+  // Admin creates a task assigned to coord.
+  const create = await api(adminCookie, "POST",
+    "/api/executive-meetings/tasks", {
+      taskType: "follow_up",
+      assignedTo: coordUserId,
+      notes: "assignee-only test",
+    });
+  assert.equal(create.status, 201);
+  const task = await create.json();
+  created.taskIds.push(task.id);
+
+  // The assignee (coord) can flip the status even though they are NOT in
+  // MUTATE_ROLES. This is the assignedTo carve-out in the PATCH handler.
+  const assigneeUpdate = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/tasks/${task.id}`, { status: "in_progress" });
+  assert.equal(assigneeUpdate.status, 200,
+    "assignee must be able to update their own task status");
+  const updated = await assigneeUpdate.json();
+  assert.equal(updated.status, "in_progress");
+
+  // The assignee CANNOT change non-status fields (mutator-only).
+  const assigneeReassign = await api(coordCookie, "PATCH",
+    `/api/executive-meetings/tasks/${task.id}`,
+    { assignedTo: leadUserId });
+  assert.equal(assigneeReassign.status, 200,
+    "assignee patching a mutator-only field returns the row but ignores it");
+  const stillCoord = await assigneeReassign.json();
+  assert.equal(stillCoord.assignedTo, coordUserId,
+    "assignee may not reassign a task to someone else");
+
+  // A different coordinator (not the assignee, not a mutator) is rejected.
+  const otherCoord = await createUser("em_other_coord", "executive_coordinator");
+  const otherCookie = await login(otherCoord.username, TEST_PASSWORD);
+  const nonAssignee = await api(otherCookie, "PATCH",
+    `/api/executive-meetings/tasks/${task.id}`, { status: "completed" });
+  assert.equal(nonAssignee.status, 403,
+    "non-assignee non-mutator must be denied by the PATCH handler");
+
+  // Mutator (lead) is always allowed.
+  const mutator = await api(leadCookie, "PATCH",
+    `/api/executive-meetings/tasks/${task.id}`, { status: "completed" });
+  assert.equal(mutator.status, 200);
+});
+
+test("Font settings: PUT then GET returns the user-scoped row roundtrip", async () => {
+  const put = await api(adminCookie, "PUT",
+    "/api/executive-meetings/font-settings", {
+      scope: "user",
+      fontFamily: "Tajawal",
+      fontSize: 18,
+      fontWeight: "bold",
+      alignment: "center",
+    });
+  assert.equal(put.status, 200);
+
+  const get = await api(adminCookie, "GET",
+    "/api/executive-meetings/font-settings");
+  assert.equal(get.status, 200);
+  const body = await get.json();
+  assert.ok(body.user, "GET must include the user-scoped row");
+  assert.equal(body.user.scope, "user");
+  assert.equal(body.user.fontFamily, "Tajawal");
+  assert.equal(body.user.fontSize, 18);
+  assert.equal(body.user.fontWeight, "bold");
+  assert.equal(body.user.alignment, "center");
+
+  // PATCH alias must hit the same upsert handler.
+  const patch = await api(adminCookie, "PATCH",
+    "/api/executive-meetings/font-settings", {
+      scope: "user",
+      fontFamily: "Cairo",
+      fontSize: 14,
+      fontWeight: "regular",
+      alignment: "start",
+    });
+  assert.equal(patch.status, 200);
+
+  const get2 = await api(adminCookie, "GET",
+    "/api/executive-meetings/font-settings");
+  const body2 = await get2.json();
+  assert.equal(body2.user.fontFamily, "Cairo");
+  assert.equal(body2.user.fontSize, 14);
+  assert.equal(body2.user.fontWeight, "regular");
+  assert.equal(body2.user.alignment, "start");
+});
+
+test("router.param: non-numeric :id returns 404 across endpoints (no crash)", async () => {
+  // The router.param("id", ...) guard in executive-meetings.ts must reject
+  // non-digit ids by calling next("route"), so Express falls through to 404
+  // instead of attempting Number("abc") and crashing further down.
+  const cases = [
+    ["GET",    "/api/executive-meetings/abc"],
+    ["PATCH",  "/api/executive-meetings/abc"],
+    ["DELETE", "/api/executive-meetings/abc"],
+    ["GET",    "/api/executive-meetings/123abc"],
+    ["GET",    "/api/executive-meetings/-1"],
+    ["POST",   "/api/executive-meetings/abc/duplicate"],
+    ["PUT",    "/api/executive-meetings/abc/attendees"],
+    ["POST",   "/api/executive-meetings/abc/requests"],
+    ["PATCH",  "/api/executive-meetings/requests/abc"],
+    ["PATCH",  "/api/executive-meetings/tasks/abc"],
+    ["DELETE", "/api/executive-meetings/tasks/abc"],
+  ];
+  for (const [method, path] of cases) {
+    const res = await api(adminCookie, method, path,
+      method === "GET" || method === "DELETE" ? undefined : {});
+    assert.equal(res.status, 404,
+      `${method} ${path} should 404 (got ${res.status})`);
+    // Express's default 404 fallthrough returns an HTML "Cannot METHOD
+    // /path" page (text/html), so we assert that the body is *not* the
+    // pretty-printed JSON of an unhandled exception. Whichever Express
+    // produces, it must not be a 5xx HTML error stack trace.
+    const body = await res.text();
+    assert.ok(!/<pre>Error:/i.test(body),
+      `${method} ${path} body must not contain a thrown Error stack`);
+    assert.ok(!/TypeError|ReferenceError/i.test(body),
+      `${method} ${path} body must not contain a JS exception name`);
+  }
+});
+
+test("Transactional safety: a failing audit insert rolls back the parent DELETE", async () => {
+  // Create a meeting we can target.
+  const create = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "اختبار-صفقة", titleEn: "Tx Rollback Target", meetingDate: today,
+  });
+  assert.equal(create.status, 201);
+  const meeting = await create.json();
+  // Do NOT push to created.meetingIds yet — we want to verify the row is
+  // still there after the deliberately-failing DELETE, then clean it up
+  // ourselves.
+  const meetingId = meeting.id;
+
+  // Install a BEFORE INSERT trigger on the audit log table that raises an
+  // exception only when our specific (entity_type='meeting', action='delete',
+  // entity_id=<this meeting>) row is being inserted. Other audit inserts —
+  // including the ones that other tests run in parallel/sequence — are
+  // unaffected.
+  const triggerName = `tx_rollback_test_${meetingId}`;
+  const fnName = `tx_rollback_test_fn_${meetingId}`;
+  await pool.query(`
+    CREATE OR REPLACE FUNCTION ${fnName}() RETURNS trigger AS $$
+    BEGIN
+      IF NEW.entity_type = 'meeting'
+         AND NEW.action = 'delete'
+         AND NEW.entity_id = ${meetingId} THEN
+        RAISE EXCEPTION 'forced audit failure for tx rollback test';
+      END IF;
+      RETURN NEW;
+    END;
+    $$ LANGUAGE plpgsql;
+  `);
+  await pool.query(`
+    CREATE TRIGGER ${triggerName}
+    BEFORE INSERT ON executive_meeting_audit_logs
+    FOR EACH ROW EXECUTE FUNCTION ${fnName}();
+  `);
+
+  let deleteStatus = null;
+  try {
+    const del = await api(adminCookie, "DELETE",
+      `/api/executive-meetings/${meetingId}`);
+    deleteStatus = del.status;
+  } finally {
+    // Always tear the trigger + function down so the rest of the test file
+    // (and any other test runs) are not poisoned.
+    await pool.query(`DROP TRIGGER IF EXISTS ${triggerName} ON executive_meeting_audit_logs`);
+    await pool.query(`DROP FUNCTION IF EXISTS ${fnName}()`);
+  }
+
+  assert.equal(deleteStatus, 500,
+    "DELETE must surface a 500 when the audit insert raises");
+
+  // The meeting row must still exist — the surrounding db.transaction()
+  // should have rolled the DELETE back when the audit insert failed.
+  const stillThere = await pool.query(
+    `SELECT id FROM executive_meetings WHERE id = $1`,
+    [meetingId],
+  );
+  assert.equal(stillThere.rowCount, 1,
+    "parent DELETE must be rolled back when audit insert fails");
+
+  // Now clean up for real — register the id so afterAll deletes it.
+  created.meetingIds.push(meetingId);
+});