From 50da9b3bca48da0b50c1b29b01030402fcbbd099 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Tue, 9 Jun 2026 22:25:32 -0400 Subject: [PATCH] fix(05-review): IN-01 resolve actor display name in eventChangeDispatcher for D-02/D-03 --- apps/api/src/lib/eventChangeDispatcher.ts | 51 +++++--- .../tests/lib/eventChangeDispatcher.test.ts | 116 +++++++++++++----- 2 files changed, 115 insertions(+), 52 deletions(-) diff --git a/apps/api/src/lib/eventChangeDispatcher.ts b/apps/api/src/lib/eventChangeDispatcher.ts index 5496ee6..fc9db9c 100644 --- a/apps/api/src/lib/eventChangeDispatcher.ts +++ b/apps/api/src/lib/eventChangeDispatcher.ts @@ -15,9 +15,9 @@ * Fire-and-forget: sync correctness does not depend on push success. */ -import { ne } from 'drizzle-orm' +import { eq, ne } from 'drizzle-orm' import { db } from '../db/client.js' -import { pushSubscriptions } from '../db/schema.js' +import { users, pushSubscriptions } from '../db/schema.js' import { dispatchPush } from './pushDispatcher.js' // ── Types ──────────────────────────────────────────────────────────────────── @@ -71,32 +71,32 @@ export function isMeaningfulChange(change: EventChange): boolean { } /** - * Builds the notification title for a change. - * Actor name is omitted here (we don't have it from userId alone in the fast path); - * the copy follows the D-02 spec using generic "A member" as fallback. For the - * two-person household the non-actor will always be informed by the other member's - * action, so the body context is sufficient. + * Builds the notification copy for a change (D-02/D-03). * - * Note: actorName resolution (querying users table) is a future D-02 enhancement. - * For MVP, use generic copy that still satisfies the acceptance criteria. + * @param change - The detected calendar event change. + * @param actorName - Display name of the actor (D-03: named in every notification). + * Falls back to 'A family member' when the users row is absent. */ function buildCopy( change: EventChange, + actorName: string, ): { notifTitle: string; notifBody: string; navigate: string } { const eventTitle = change.title ?? change.uid let notifTitle: string let notifBody: string + // D-02: event notifications show specifics — actor + title. + // D-03: name the actor in every change notification. if (change.operation === 'create') { - notifTitle = 'New calendar event' + notifTitle = `${actorName} added an event` notifBody = eventTitle } else if (change.operation === 'delete') { - notifTitle = 'Calendar event removed' + notifTitle = `${actorName} removed an event` notifBody = eventTitle } else { // update - notifTitle = 'Calendar event updated' + notifTitle = `${actorName} updated an event` notifBody = eventTitle } @@ -128,13 +128,24 @@ export async function dispatchEventChange( return } - // D-13: query push_subscriptions from MariaDB only - // D-03: ne() filter excludes the actor at DB level; application-level filter - // below provides defence-in-depth (also makes the mock-based tests deterministic). - const allSubs = await db - .select() - .from(pushSubscriptions) - .where(ne(pushSubscriptions.userId, actorUserId)) + // IN-01: resolve actor display name for D-02/D-03 notification copy. + // Runs in parallel with subscription query for minimal latency. + const [actorRows, allSubs] = await Promise.all([ + db + .select({ displayName: users.displayName }) + .from(users) + .where(eq(users.id, actorUserId)) + .limit(1), + // D-13: query push_subscriptions from MariaDB only + // D-03: ne() filter excludes the actor at DB level; application-level filter + // below provides defence-in-depth (also makes the mock-based tests deterministic). + db + .select() + .from(pushSubscriptions) + .where(ne(pushSubscriptions.userId, actorUserId)), + ]) + + const actorName: string = actorRows[0]?.displayName ?? 'A family member' // D-03: additional application-level actor exclusion (defence-in-depth) const subs = allSubs.filter((s) => s.userId !== actorUserId) @@ -143,7 +154,7 @@ export async function dispatchEventChange( return } - const { notifTitle, notifBody, navigate } = buildCopy(change) + const { notifTitle, notifBody, navigate } = buildCopy(change, actorName) // Fan out to all non-actor subscriptions (D-03 already enforced by ne() filter) for (const sub of subs) { diff --git a/apps/api/tests/lib/eventChangeDispatcher.test.ts b/apps/api/tests/lib/eventChangeDispatcher.test.ts index 9b0e25b..24ec519 100644 --- a/apps/api/tests/lib/eventChangeDispatcher.test.ts +++ b/apps/api/tests/lib/eventChangeDispatcher.test.ts @@ -1,13 +1,14 @@ /** - * RED scaffold — eventChangeDispatcher (Plan 05-05 turns this GREEN). + * eventChangeDispatcher tests (D-04, D-02/D-03 actor naming — IN-01 fix). * * Asserts that dispatchEventChange: * - Fires for new events (operation='create') * - Fires for updated events with meaningful changes: time/date/title/location (D-04) * - Does NOT fire for description-only edits (D-04) * - Excludes the actor's own push subscriptions (D-03) + * - Names the actor in the notification title (IN-01 / D-02/D-03) + * - Falls back to 'A family member' when the actor row is missing (IN-01) * - * These tests fail now because eventChangeDispatcher.ts does not yet exist. * Run: pnpm --filter @familysync/api exec vitest run tests/lib/eventChangeDispatcher.test.ts */ @@ -16,11 +17,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' // Mock DB vi.mock('../../src/db/client.js', () => ({ db: { - select: vi.fn().mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([]), - }), - }), + select: vi.fn(), }, })) @@ -29,6 +26,37 @@ vi.mock('../../src/lib/pushDispatcher.js', () => ({ dispatchPush: vi.fn().mockResolvedValue(undefined), })) +// --------------------------------------------------------------------------- +// Helper: build a mock db.select chain. +// dispatchEventChange now calls Promise.all([actorQuery, subsQuery]). +// The first select is for the actor name (users table, returns [{displayName}]), +// the second is for push subscriptions (returns sub rows). +// --------------------------------------------------------------------------- +function mockDbSelect( + db: { select: ReturnType }, + actorDisplayName: string | null, + subRows: Array<{ id: number; userId: number; endpoint: string; p256dh: string; auth: string }>, +) { + // Two sequential .select() calls via Promise.all: + // First call → actor name query (select.from.where.limit → [{ displayName }]) + // Second call → subscriptions query (select.from.where → subRows) + db.select + .mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue( + actorDisplayName !== null ? [{ displayName: actorDisplayName }] : [], + ), + }), + }), + }) + .mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue(subRows), + }), + }) +} + describe('eventChangeDispatcher — trigger conditions (D-04)', () => { beforeEach(() => { vi.clearAllMocks() @@ -41,11 +69,7 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => { const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js') const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' } - vi.mocked(db.select).mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([otherUserSub]), - }), - } as never) + mockDbSelect(vi.mocked(db), 'Lucas', [otherUserSub]) await dispatchEventChange( { uid: 'new-event-uid', title: 'Doctor appointment', operation: 'create' }, @@ -61,11 +85,7 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => { const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js') const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' } - vi.mocked(db.select).mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([otherUserSub]), - }), - } as never) + mockDbSelect(vi.mocked(db), 'Lucas', [otherUserSub]) await dispatchEventChange( { @@ -85,13 +105,11 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => { const { dispatchPush } = await import('../../src/lib/pushDispatcher.js') const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js') - vi.mocked(db.select).mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([ - { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' }, - ]), - }), - } as never) + // isMeaningfulChange returns false before DB is queried — no select needed. + // But set up a mock just in case (won't be called). + mockDbSelect(vi.mocked(db), 'Lucas', [ + { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' }, + ]) await dispatchEventChange( { @@ -111,14 +129,12 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => { const { dispatchPush } = await import('../../src/lib/pushDispatcher.js') const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js') - // DB returns only the actor's own subscription (userId 1) - vi.mocked(db.select).mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([ - { id: 1, userId: 1, endpoint: 'https://push.example.com/sub/1', p256dh: 'x', auth: 'y' }, - ]), - }), - } as never) + // DB returns only the actor's own subscription (userId 1). + // The ne() filter in the SQL should exclude it, but here we simulate that + // the application-level filter also catches it. + mockDbSelect(vi.mocked(db), 'Lucas', [ + { id: 1, userId: 1, endpoint: 'https://push.example.com/sub/1', p256dh: 'x', auth: 'y' }, + ]) await dispatchEventChange( { uid: 'event-uid-3', title: 'Soccer practice', operation: 'create' }, @@ -128,4 +144,40 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => { // No subscriptions remain after excluding the actor — nothing dispatched expect(vi.mocked(dispatchPush)).not.toHaveBeenCalled() }) + + it('names the actor in the notification title (IN-01 / D-02/D-03)', async () => { + const { db } = await import('../../src/db/client.js') + const { dispatchPush } = await import('../../src/lib/pushDispatcher.js') + const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js') + + const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' } + mockDbSelect(vi.mocked(db), 'Lucas', [otherUserSub]) + + await dispatchEventChange( + { uid: 'event-uid-4', title: 'Dentist', operation: 'create' }, + /* actorUserId */ 1, + ) + + expect(vi.mocked(dispatchPush)).toHaveBeenCalled() + const calledWith = vi.mocked(dispatchPush).mock.calls[0][1] + expect(calledWith.title).toContain('Lucas') + }) + + it('falls back to "A family member" when actor row is missing (IN-01)', async () => { + const { db } = await import('../../src/db/client.js') + const { dispatchPush } = await import('../../src/lib/pushDispatcher.js') + const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js') + + const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' } + mockDbSelect(vi.mocked(db), null, [otherUserSub]) // actor row absent → null + + await dispatchEventChange( + { uid: 'event-uid-5', title: 'Soccer', operation: 'create' }, + /* actorUserId */ 99, // non-existent user + ) + + expect(vi.mocked(dispatchPush)).toHaveBeenCalled() + const calledWith = vi.mocked(dispatchPush).mock.calls[0][1] + expect(calledWith.title).toContain('A family member') + }) })