From ee565b2086dcdbff9a661f061fe4e6bf60b9516d Mon Sep 17 00:00:00 2001 From: Riyadh Date: Wed, 29 Apr 2026 18:27:15 +0000 Subject: [PATCH] Add automated tests for the Phase-2 Executive Meetings endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tests/executive-meetings.test.mjs | 319 ++++++++++++++++++ 1 file changed, 319 insertions(+) diff --git a/artifacts/api-server/tests/executive-meetings.test.mjs b/artifacts/api-server/tests/executive-meetings.test.mjs index db2a1814..a650ce03 100644 --- a/artifacts/api-server/tests/executive-meetings.test.mjs +++ b/artifacts/api-server/tests/executive-meetings.test.mjs @@ -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(!/
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=) 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);
+});