#245: narrow umbrella subset — toast polish, opt-out tests, Restore defaults

Picked the 3 most isolated items from the 7-item umbrella; deferred the rest
as #259/#260/#261.

#223 + #224 — singular toast + summary on partial failure (T001):
- my-orders.tsx: replaced the N=1 vs N>1 ternary in scheduleDelete with a
  single t("myOrders.clearedCount", { count }) so i18next picks _one /
  _other automatically. Single-row delete now flows through this same toast
  too — user-visible copy for N=1 is now "1 order deleted" / "تم حذف طلب
  واحد" instead of the legacy "Order deleted" / "تم حذف الطلب".
- my-orders.tsx: partial-failure path now shows ONE summary toast using
  the existing clearedPartial key ("{{ok}} deleted, {{fail}} failed")
  instead of N error toasts. Total failure (okCount===0) keeps deleteFailed.
- Updated 3 Playwright specs that asserted the legacy copy:
  order-clear-finished-undo (already had the singular case), order-undo-toast
  (Arabic single-row delete), order-delete-flush-on-unmount (English).
  Note: the legacy "myOrders.deleted" locale key is now unreferenced in
  source — left in place to avoid noise; deletion can be handled separately.

#238 — opt-out coverage in executive-meetings-notifications.test.mjs (T002):
- Appended 4 tests + setPref/clearPref helpers covering
  filterRecipientsByNotificationPref: inApp=false drops user, missing pref
  defaults to ON, cross-event isolation (mute on event A leaves event B
  alone), email=false leaves in-app intact. Helpers use ON CONFLICT on the
  verified unique index. Some scenarios overlap existing tests in
  executive-meetings.test.mjs (lines 1764, 1809) — these still add value
  by exercising the meeting_created socket fan-out path and cross-event
  isolation, which the existing tests don't cover.

#236 — Restore defaults endpoint + button (T003):
- Server: added DELETE /api/executive-meetings/notification-prefs after PUT.
  Scoped strictly to req.session.userId, returns {ok, count}. Reuses
  requireExecutiveAccess guard. Architect confirmed no cross-user leakage.
- Client: restoreDefaults() handler + outline button (data-testid
  "em-pref-restore-defaults", NOT gated on dirty since the whole point is to
  blow away saved settings). New i18n keys restoreDefaults / restored in
  both locales.
- Architect found a stale-state race in restoreDefaults: setDraft(null)
  was called before invalidateQueries, letting the seed effect repopulate
  draft from still-cached pre-DELETE data. Fixed by inverting the order to
  match save() — invalidate first (await refetch), then setDraft(null).
- Tests: appended 2 integration tests to executive-meetings.test.mjs
  covering the full restore flow (PUT 2 muted prefs → DELETE → assert
  {ok,count:2} + GET shows defaults + actual fan-out reaches user again)
  and idempotent no-op DELETE on a user with no rows.

Test results:
- executive-meetings.test.mjs: 47/47 pass (incl. 2 new DELETE tests)
- executive-meetings-notifications.test.mjs: 11/11 pass (incl. 4 new opt-out tests)
- Playwright order specs: 6/6 pass after legacy-copy updates
- Pre-existing failures in service-orders + meeting_created fan-out are
  untouched and not caused by this change.

Follow-ups proposed: #259 (beforeunload + tab-close Playwright), #260
(admin override another user's prefs with audit row + UI), #261 (iPad
header verification — may already work).
This commit is contained in:
riyadhafraa
2026-05-01 07:30:00 +00:00
parent 87b16fd256
commit c53641c721
10 changed files with 390 additions and 18 deletions
@@ -2371,6 +2371,23 @@ router.put(
},
);
// #236: DELETE wipes every pref row for the current user so all toggles
// fall back to the schema default (in_app=true, email=true). Cheaper than
// asking the client to PUT N "true" rows back, and avoids the partial-
// update gotcha where a missing event type would stay opted-out.
router.delete(
"/executive-meetings/notification-prefs",
requireExecutiveAccess,
async (req, res): Promise<void> => {
const userId = req.session.userId!;
const deleted = await db
.delete(executiveMeetingNotificationPrefsTable)
.where(eq(executiveMeetingNotificationPrefsTable.userId, userId))
.returning({ id: executiveMeetingNotificationPrefsTable.id });
res.json({ ok: true, count: deleted.length });
},
);
// =====================================================================
// PDF GENERATION (server-side)
// =====================================================================
@@ -727,3 +727,215 @@ test("task_completed: notifies the prior assignee, excludes actor, writes both t
await expectSocketEventsFor([coord2Sock], "task_completed", null);
});
// #238: opt-out coverage for filterRecipientsByNotificationPref. The HTTP
// surface gives us black-box access — set a pref row, trigger the event,
// assert delivery (or non-delivery) at the EMN + notifications + socket
// layer. Each test owns its own pref row(s) and cleans them up so the
// suite stays order-independent.
async function setPref(userId, notificationType, { inApp, email }) {
await pool.query(
`INSERT INTO executive_meeting_notification_prefs
(user_id, notification_type, in_app, email)
VALUES ($1, $2, $3, $4)
ON CONFLICT (user_id, notification_type)
DO UPDATE SET in_app = EXCLUDED.in_app, email = EXCLUDED.email`,
[userId, notificationType, inApp, email],
);
}
async function clearPref(userId, notificationType) {
await pool.query(
`DELETE FROM executive_meeting_notification_prefs
WHERE user_id = $1 AND notification_type = $2`,
[userId, notificationType],
);
}
test("pref opt-out: inApp=false drops the user from in-app fan-out (others unaffected)", async () => {
await setPref(approver1.id, "meeting_created", { inApp: false, email: true });
try {
for (const s of [approver1Sock, approver2Sock]) clearSocketEvents(s);
const before = await snapshotMaxIds();
const create = await api(adminCookie, "POST", "/api/executive-meetings", {
titleAr: "اختبار تعطيل",
titleEn: "Opt-out fan-out",
meetingDate: today,
attendees: [],
});
assert.equal(create.status, 201);
const meeting = await create.json();
created.meetingIds.push(meeting.id);
// Wait for approver2 (default-on) so we know fan-out completed before
// checking that approver1 was dropped.
await waitFor(
() =>
approver2Sock.events.created.some(
(p) => p.notificationType === "meeting_created",
),
{ timeoutMs: 2000 },
);
const diff = scopeDiff(await newRowsAfter(before), {
notificationType: "meeting_created",
meetingId: meeting.id,
relatedType: "executive_meeting",
relatedId: meeting.id,
});
assert.equal(
rowsForUser(diff.emn, approver1.id).length,
0,
"opted-out user must NOT get an executive_meeting_notifications row",
);
assert.equal(
rowsForUser(diff.notifications, approver1.id).length,
0,
"opted-out user must NOT get a notifications row",
);
assertRecipientGotOneOfEach(diff, approver2.id, {
notificationType: "meeting_created",
meetingId: meeting.id,
relatedType: "executive_meeting",
relatedId: meeting.id,
});
expectNoSocketEventFor(approver1Sock, "meeting_created");
} finally {
await clearPref(approver1.id, "meeting_created");
}
});
test("pref opt-out: missing pref row defaults to ON (user receives delivery)", async () => {
// No setPref call — approver1 has no row at all for this event type.
// This is the implicit default-on case. Assert it explicitly so a
// future regression (e.g. flipping the default to off) is caught.
await pool.query(
`DELETE FROM executive_meeting_notification_prefs
WHERE user_id = $1 AND notification_type = $2`,
[approver1.id, "meeting_created"],
);
for (const s of [approver1Sock, approver2Sock]) clearSocketEvents(s);
const before = await snapshotMaxIds();
const create = await api(adminCookie, "POST", "/api/executive-meetings", {
titleAr: "افتراضي مفعل",
titleEn: "Default-on fan-out",
meetingDate: today,
attendees: [],
});
assert.equal(create.status, 201);
const meeting = await create.json();
created.meetingIds.push(meeting.id);
await waitFor(
() =>
approver1Sock.events.created.some(
(p) => p.notificationType === "meeting_created",
),
{ timeoutMs: 2000 },
);
const diff = scopeDiff(await newRowsAfter(before), {
notificationType: "meeting_created",
meetingId: meeting.id,
relatedType: "executive_meeting",
relatedId: meeting.id,
});
assertRecipientGotOneOfEach(diff, approver1.id, {
notificationType: "meeting_created",
meetingId: meeting.id,
relatedType: "executive_meeting",
relatedId: meeting.id,
});
});
test("pref opt-out: inApp=false on event A does NOT mute event B", async () => {
// Approver1 mutes meeting_created but is still an approver who should
// receive request_submitted. Verifies the filter is keyed on event type.
await setPref(approver1.id, "meeting_created", { inApp: false, email: true });
try {
for (const s of [approver1Sock, approver2Sock, coord1Sock]) {
clearSocketEvents(s);
}
const before = await snapshotMaxIds();
const create = await api(
coord1.cookie,
"POST",
"/api/executive-meetings/requests",
{
requestType: "note",
requestDetails: { note: "cross-event opt-out" },
},
);
assert.equal(create.status, 201);
const request = await create.json();
created.requestIds.push(request.id);
await waitFor(
() =>
approver1Sock.events.created.some(
(p) => p.notificationType === "request_submitted",
),
{ timeoutMs: 2000 },
);
const diff = scopeDiff(await newRowsAfter(before), {
notificationType: "request_submitted",
relatedType: "executive_meeting_request",
relatedId: request.id,
});
assertRecipientGotOneOfEach(diff, approver1.id, {
notificationType: "request_submitted",
relatedType: "executive_meeting_request",
relatedId: request.id,
});
} finally {
await clearPref(approver1.id, "meeting_created");
}
});
test("pref opt-out: email=false does NOT affect the in-app channel", async () => {
// Channel independence — muting email must leave in-app delivery alone.
await setPref(approver1.id, "meeting_created", { inApp: true, email: false });
try {
for (const s of [approver1Sock, approver2Sock]) clearSocketEvents(s);
const before = await snapshotMaxIds();
const create = await api(adminCookie, "POST", "/api/executive-meetings", {
titleAr: "قناة منفصلة",
titleEn: "Channel independence",
meetingDate: today,
attendees: [],
});
assert.equal(create.status, 201);
const meeting = await create.json();
created.meetingIds.push(meeting.id);
await waitFor(
() =>
approver1Sock.events.created.some(
(p) => p.notificationType === "meeting_created",
),
{ timeoutMs: 2000 },
);
const diff = scopeDiff(await newRowsAfter(before), {
notificationType: "meeting_created",
meetingId: meeting.id,
relatedType: "executive_meeting",
relatedId: meeting.id,
});
assertRecipientGotOneOfEach(diff, approver1.id, {
notificationType: "meeting_created",
meetingId: meeting.id,
relatedType: "executive_meeting",
relatedId: meeting.id,
});
} finally {
await clearPref(approver1.id, "meeting_created");
}
});
@@ -1858,3 +1858,94 @@ test("Notification prefs: channels are independent (email-only mute leaves in-ap
assert.equal(rows[0].in_app, true);
assert.equal(rows[0].email, false);
});
test("Notification prefs: DELETE wipes the user's pref rows so GET reverts to defaults", async () => {
// #236: clicking "Restore defaults" in the UI hits DELETE. After it
// returns, every event type must read default-on regardless of what
// had been muted before, and a follow-up fan-out must reach the user
// again. Use a fresh approver so we don't disturb other prefs tests.
const fresh = await createUser("em_pref_restore", "executive_office_manager");
const cookie = await login(fresh.username, TEST_PASSWORD);
// Mute two different event types on different channels so we can
// prove the DELETE clears EVERY row, not just one.
const put = await api(cookie, "PUT",
"/api/executive-meetings/notification-prefs",
{
prefs: [
{ notificationType: "request_submitted", inApp: false, email: true },
{ notificationType: "meeting_created", inApp: true, email: false },
],
});
assert.equal(put.status, 200);
const { rows: pre } = await pool.query(
`SELECT COUNT(*)::int AS n
FROM executive_meeting_notification_prefs
WHERE user_id = $1`,
[fresh.id],
);
assert.equal(pre[0].n, 2, "PUT must have left 2 pref rows");
const del = await api(cookie, "DELETE",
"/api/executive-meetings/notification-prefs");
assert.equal(del.status, 200);
const delBody = await del.json();
assert.equal(delBody.ok, true);
assert.equal(delBody.count, 2, "DELETE must report wiping both rows");
const { rows: post } = await pool.query(
`SELECT COUNT(*)::int AS n
FROM executive_meeting_notification_prefs
WHERE user_id = $1`,
[fresh.id],
);
assert.equal(post[0].n, 0, "no pref rows must remain after DELETE");
// GET must now synthesize default-on for every type.
const after = await (await api(cookie, "GET",
"/api/executive-meetings/notification-prefs")).json();
for (const p of after.prefs) {
assert.equal(p.inApp, true, `${p.notificationType} must be default-on after DELETE`);
assert.equal(p.email, true, `${p.notificationType} must be default-on after DELETE`);
}
// And actual fan-out must reach the user again — a regression in the
// DELETE handler that left rows behind would silently re-mute them.
const beforeCount = await pool.query(
`SELECT COUNT(*)::int AS n
FROM executive_meeting_notifications
WHERE user_id = $1
AND notification_type = 'request_submitted'`,
[fresh.id],
);
const submit = await api(coordCookie, "POST",
"/api/executive-meetings/requests",
{ requestType: "note", requestDetails: { note: "post-restore-fanout" } });
assert.equal(submit.status, 201);
created.requestIds.push((await submit.json()).id);
const afterCount = await pool.query(
`SELECT COUNT(*)::int AS n
FROM executive_meeting_notifications
WHERE user_id = $1
AND notification_type = 'request_submitted'`,
[fresh.id],
);
assert.ok(
afterCount.rows[0].n > beforeCount.rows[0].n,
"user with no pref rows (post-DELETE) must receive new request_submitted rows",
);
});
test("Notification prefs: DELETE on a user with no rows is a 200 no-op", async () => {
// Cheap idempotence check — clicking Restore defaults twice in a row
// must not 500 or surface a misleading error.
const fresh = await createUser("em_pref_restore_noop", "executive_coordinator");
const cookie = await login(fresh.username, TEST_PASSWORD);
const del = await api(cookie, "DELETE",
"/api/executive-meetings/notification-prefs");
assert.equal(del.status, 200);
const body = await del.json();
assert.equal(body.ok, true);
assert.equal(body.count, 0);
});
+3 -1
View File
@@ -1334,7 +1334,9 @@
},
"save": "حفظ التفضيلات",
"reset": "إعادة",
"saved": "تم حفظ التفضيلات"
"saved": "تم حفظ التفضيلات",
"restoreDefaults": "استعادة الإعدادات الافتراضية",
"restored": "تمت استعادة الإعدادات الافتراضية"
}
}
}
+3 -1
View File
@@ -1235,7 +1235,9 @@
},
"save": "Save preferences",
"reset": "Reset",
"saved": "Preferences saved"
"saved": "Preferences saved",
"restoreDefaults": "Restore defaults",
"restored": "Preferences reset to defaults"
}
}
}
@@ -6336,6 +6336,31 @@ function NotificationPrefsCard({
setDraft(m);
}
// #236: wipe every server-side pref row for the current user. The GET
// handler then synthesizes the defaults (in_app=true, email=true) for
// every event type, so a single round-trip restores the factory state
// without the client having to PUT a full payload of `true` toggles.
async function restoreDefaults() {
try {
await apiJson("/api/executive-meetings/notification-prefs", {
method: "DELETE",
});
// #236: invalidate FIRST so the refetch lands before we drop the
// local draft. If we cleared draft first, the reseed effect at
// line ~6284 would fire against the still-cached pre-delete
// payload, repopulating draft from stale data; the later refetch
// would then no-op because draft is non-null again.
await qc.invalidateQueries({
queryKey: ["/api/executive-meetings/notification-prefs"],
});
setDraft(null);
toast({ title: t("executiveMeetings.notificationsPage.prefs.restored") });
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
toast({ title: msg, variant: "destructive" });
}
}
const types = data?.types ?? [];
return (
@@ -6429,6 +6454,16 @@ function NotificationPrefsCard({
>
{t("executiveMeetings.notificationsPage.prefs.reset")}
</Button>
{/* #236: restore-defaults wipes server rows even when the
local draft is clean, so it must NOT be gated on `dirty`. */}
<Button
size="sm"
variant="outline"
onClick={restoreDefaults}
data-testid="em-pref-restore-defaults"
>
{t("executiveMeetings.notificationsPage.prefs.restoreDefaults")}
</Button>
</div>
</>
)}
+12 -6
View File
@@ -438,7 +438,7 @@ export default function MyOrdersPage() {
const timer = setTimeout(() => {
ids.forEach((id) => timersRef.current.delete(id));
void performDelete(ids).then(({ failCount }) => {
void performDelete(ids).then(({ okCount, failCount }) => {
if (failCount > 0) {
// Restore failed deletions on error so the user sees them again.
setPendingDeleteIds((prev) => {
@@ -446,8 +446,14 @@ export default function MyOrdersPage() {
ids.forEach((id) => next.delete(id));
return next;
});
// #224: surface ONE summary toast instead of N error toasts. Total
// failure keeps the existing single-line copy; partial failure
// uses the existing `clearedPartial` key with both counts.
toast({
title: t("myOrders.deleteFailed"),
title:
okCount === 0
? t("myOrders.deleteFailed")
: t("myOrders.clearedPartial", { ok: okCount, fail: failCount }),
variant: "destructive",
});
} else {
@@ -477,10 +483,10 @@ export default function MyOrdersPage() {
toast({ title: t("myOrders.undone") });
};
const title =
ids.length === 1
? t("myOrders.deleted")
: t("myOrders.clearedCount", { count: ids.length });
// #223: lean on i18next pluralization (clearedCount_one / _other are
// already in both locales) so N=1 reads as "1 order deleted" instead
// of the generic "Order deleted" copy.
const title = t("myOrders.clearedCount", { count: ids.length });
const handle = toast({
title,
@@ -31,10 +31,10 @@
// 3. Bulk singular toast (Arabic):
// - Seed exactly 1 finished order.
// - Click the "مسح طلب منتهٍ" (Clear 1 finished) button + confirm.
// - When ids.length === 1 the bulk path uses myOrders.deleted, so the
// toast title is "تم حذف الطلب" (NOT clearedCount_one). This
// intentionally exercises the singular branch of the title selector
// in scheduleDelete().
// - #223: scheduleDelete() now always routes through clearedCount, so
// N=1 picks the i18next _one form. In Arabic that is
// "تم حذف طلب واحد" (singular plural form), not the generic
// "تم حذف الطلب".
// - Click Undo and verify the row remains in the DB and the card is
// visible again.
//
@@ -391,10 +391,11 @@ test.describe("My Orders bulk Clear-finished + undo flow", () => {
// Card filtered out while pending.
await expect(cards).toHaveCount(0);
// Singular branch in scheduleDelete uses myOrders.deleted (NOT
// clearedCount_one). In Arabic that is "تم حذف الطلب".
// #223: singular branch now uses clearedCount_one (the same key as the
// plural branch, with i18next pluralization picking the right form).
// In Arabic the _one form is "تم حذف طلب واحد".
const toast = toastRegion(page);
await expect(toast).toContainText("تم حذف الطلب");
await expect(toast).toContainText("تم حذف طلب واحد");
// Undo within the window. Arabic undo label = "تراجع".
await toast.getByRole("button", { name: "تراجع" }).click();
@@ -220,7 +220,10 @@ test.describe("My Orders unmount flushes pending deletes", () => {
// The Undo toast should be open. We deliberately do NOT click Undo —
// we want the cleanup path, not the cancel path.
const toast = toastRegion(page);
await expect(toast).toContainText("Order deleted");
// #223: single-row delete now reads as the `clearedCount_one` form
// ("1 order deleted") shared with bulk-clear instead of the legacy
// "Order deleted" copy.
await expect(toast).toContainText("1 order deleted");
await expect(toast.getByRole("button", { name: "Undo" })).toBeVisible();
// Sanity: nothing should have fired yet — the 7s timer is still pending.
@@ -315,9 +315,12 @@ test.describe("My Orders undo-toast flow", () => {
page.locator(".glass-panel.rounded-xl").filter({ hasText: serviceName }),
).toHaveCount(0);
// Toast title in Arabic = "تم حذف الطلب", action label = "تراجع".
// #223: single-row delete now goes through the same i18n-plural toast
// as bulk-clear, so N=1 reads as the `clearedCount_one` form
// ("تم حذف طلب واحد") instead of the legacy generic copy.
// Action label is unchanged ("تراجع").
const toast = toastRegion(page);
await expect(toast).toContainText("تم حذف الطلب");
await expect(toast).toContainText("تم حذف طلب واحد");
await toast.getByRole("button", { name: "تراجع" }).click();
// The card should come back to the list immediately after Undo.