From ab02794ff20417dfd20764f123406a17486a3f37 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 22 Oct 2026 21:51:44 +0000 Subject: [PATCH] fix(notifications): notifications sent from the services name their actor by name The routes on dev now name the actor of a notification by username instead of the address. The chat, note, invite and packing notifications the services send for both surfaces read the name too, so they no longer go out with an empty actor. --- server/src/nest/collab/collab.service.ts | 6 +++--- .../notifications/notifications.service.ts | 6 +++--- .../nest/packing/packing-writes.service.ts | 12 +++++------ server/src/nest/packing/packing.service.ts | 2 +- .../trip-member-writes.service.ts | 4 ++-- .../nest/trip-members/trip-members.service.ts | 2 +- .../unit/mcp/tools-collab-polls-chat.test.ts | 2 +- server/tests/unit/mcp/tools-notes.test.ts | 2 +- .../unit/mcp/tools-packing-advanced.test.ts | 2 +- .../tests/unit/mcp/tools-trip-members.test.ts | 4 ++-- server/tests/unit/nest/collab.service.test.ts | 4 ++-- .../unit/nest/packing-writes.service.test.ts | 20 +++++++++---------- .../unit/nest/packing.controller.test.ts | 12 +++++------ .../nest/trip-member-writes.service.test.ts | 4 ++-- 14 files changed, 41 insertions(+), 41 deletions(-) diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index 12de88b3e..02286bc7b 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -254,8 +254,8 @@ export class CollabService { } /** The writer as a notification names them: by id, and by the address the route read off its session. */ - private async actorOf(writer: TripWriter): Promise> { - return { id: writer.userId, email: await this.notifications.actorEmail(writer.userId) }; + private async actorOf(writer: TripWriter): Promise> { + return { id: writer.userId, username: await this.notifications.actorName(writer.userId) }; } /* ------------------------------------------------------------------ */ @@ -907,7 +907,7 @@ export class CollabService { } /** Fire-and-forget collab notification (mirrors the legacy route's dynamic import). */ - async notifyCollab(tripId: string, actor: Pick, preview?: string): Promise { + async notifyCollab(tripId: string, actor: Pick, preview?: string): Promise { // Injected, not a lazy import of the old notifications bridge. The laziness bought // nothing the module graph does not already give — NotificationsModule // reaches nothing in this direction — and it hid the edge while handing the diff --git a/server/src/nest/notifications/notifications.service.ts b/server/src/nest/notifications/notifications.service.ts index 1da3483c5..4edd66f3c 100644 --- a/server/src/nest/notifications/notifications.service.ts +++ b/server/src/nest/notifications/notifications.service.ts @@ -662,9 +662,9 @@ export class NotificationsService { return true; } - /** The address a notification names its actor by, as the routes read it off their session user. */ - async actorEmail(userId: number): Promise { - return (await this.mailer.getUserEmail(userId)) ?? ''; + /** The name a notification gives its actor, as the routes read it off their session user. */ + async actorName(userId: number): Promise { + return (await this.notificationsRepo.findUserBasic(userId))?.username ?? ''; } async deleteOne(notificationId: number, userId: number): Promise { diff --git a/server/src/nest/packing/packing-writes.service.ts b/server/src/nest/packing/packing-writes.service.ts index 0bb8019b9..65a7bcd68 100644 --- a/server/src/nest/packing/packing-writes.service.ts +++ b/server/src/nest/packing/packing-writes.service.ts @@ -2,7 +2,7 @@ import type { User } from '../../types'; import { isUpdateConflict } from '../common/conflictResult'; import { DomainError } from '../common/domain-error'; import { requireTripWrite, type TripWriter } from '../common/trip-writer'; -import { MailerService } from '../notifications/mailer/mailer.service'; +import { NotificationsService } from '../notifications/notifications.service'; import { PermissionsService } from '../permissions/permissions.service'; import { PackingService, isInvalidBagRef, type PackingVisibility } from './packing.service'; import { Injectable } from '@nestjs/common'; @@ -56,7 +56,7 @@ export class PackingWritesService { constructor( private readonly packing: PackingService, private readonly permissions: PermissionsService, - private readonly mailer: MailerService, + private readonly notifications: NotificationsService, ) {} /** @@ -209,10 +209,10 @@ export class PackingWritesService { const assignees = await this.packing.updateCategoryAssignees(tripId, category, userIds); writer.events.emit('packing:assignees', { category, assignees }); if (userIds.length > 0) { - // The notification names its actor by email. A writer carries only the id, - // so the address is read here; a guest never signs in, so never writes. - const email = await this.mailer.getUserEmail(writer.userId); - if (email) await this.packing.notifyTagged(String(tripId), { id: writer.userId, email }, category, userIds); + // The notification names its actor by name. A writer carries only the id, + // so the name is read here; an account that is gone notifies nobody. + const username = await this.notifications.actorName(writer.userId); + if (username) await this.packing.notifyTagged(String(tripId), { id: writer.userId, username }, category, userIds); } return assignees; } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 1889be4a1..232a6af5a 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -891,7 +891,7 @@ export class PackingService { } /** Fire-and-forget tag notification, mirroring the legacy dynamic import. */ - async notifyTagged(tripId: string, actor: Pick, category: string, userIds: unknown): Promise { + async notifyTagged(tripId: string, actor: Pick, category: string, userIds: unknown): Promise { if (!Array.isArray(userIds) || userIds.length === 0) return; // Injected, not a lazy import of the old notifications bridge. The laziness bought // nothing the module graph does not already give — NotificationsModule diff --git a/server/src/nest/trip-members/trip-member-writes.service.ts b/server/src/nest/trip-members/trip-member-writes.service.ts index 6222f36ce..54cc1321d 100644 --- a/server/src/nest/trip-members/trip-member-writes.service.ts +++ b/server/src/nest/trip-members/trip-member-writes.service.ts @@ -38,8 +38,8 @@ export class TripMemberWritesService { async addMember(tripId: string | number, identifier: string, tripOwnerId: number, writer: TripWriter) { const result = await this.roster.addMember(tripId, identifier, tripOwnerId, writer.userId); writer.events.emit('member:added', { member: result.member }); - // The invitation names who sent it by address, as the route always did with its session user. - const actor = { id: writer.userId, email: (await this.users.getEmail(writer.userId)) ?? '' }; + // The invitation names who sent it by name, as the route does with its session user. + const actor = { id: writer.userId, username: (await this.users.findById(writer.userId))?.username ?? '' }; this.roster.notifyInvite(String(tripId), actor, result.targetUserId, result.tripTitle, result.member.email); return result; } diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 717f78241..4c5939717 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -120,7 +120,7 @@ export class TripMembersService { /** Fire-and-forget trip-invite notification (mirrors the route's dynamic import). */ notifyInvite( tripId: string, - actor: Pick, + actor: Pick, targetUserId: number, tripTitle: string, inviteeEmail: string, diff --git a/server/tests/unit/mcp/tools-collab-polls-chat.test.ts b/server/tests/unit/mcp/tools-collab-polls-chat.test.ts index ca0d715e8..0d4b33b56 100644 --- a/server/tests/unit/mcp/tools-collab-polls-chat.test.ts +++ b/server/tests/unit/mcp/tools-collab-polls-chat.test.ts @@ -553,7 +553,7 @@ describe('send_collab_message, as the chat posts it', () => { event: 'collab_message', actorId: user.id, targetId: trip.id, - params: expect.objectContaining({ actor: user.email, preview: 'Dinner at 8?' }), + params: expect.objectContaining({ actor: user.username, preview: 'Dinner at 8?' }), }), ); } finally { diff --git a/server/tests/unit/mcp/tools-notes.test.ts b/server/tests/unit/mcp/tools-notes.test.ts index aeb7f2c49..2b8541291 100644 --- a/server/tests/unit/mcp/tools-notes.test.ts +++ b/server/tests/unit/mcp/tools-notes.test.ts @@ -817,7 +817,7 @@ describe('create_collab_note, as the Collab tab posts it', () => { event: 'collab_message', actorId: user.id, targetId: trip.id, - params: expect.objectContaining({ actor: user.email }), + params: expect.objectContaining({ actor: user.username }), }), ); } finally { diff --git a/server/tests/unit/mcp/tools-packing-advanced.test.ts b/server/tests/unit/mcp/tools-packing-advanced.test.ts index 96b2c9625..207cb7ad3 100644 --- a/server/tests/unit/mcp/tools-packing-advanced.test.ts +++ b/server/tests/unit/mcp/tools-packing-advanced.test.ts @@ -406,7 +406,7 @@ describe('Tool: set_packing_category_assignees', () => { await new Promise((resolve) => setImmediate(resolve)); const sent = await findRow(orm, Notifications, { recipient: member.id }); expect(sent?.text_key).toBe('notif.packing_tagged.text'); - expect(JSON.parse(String(sent?.text_params))).toMatchObject({ actor: user.email, category: 'Clothing' }); + expect(JSON.parse(String(sent?.text_params))).toMatchObject({ actor: user.username, category: 'Clothing' }); }); }); diff --git a/server/tests/unit/mcp/tools-trip-members.test.ts b/server/tests/unit/mcp/tools-trip-members.test.ts index 8abd91bea..0bc6a6b96 100644 --- a/server/tests/unit/mcp/tools-trip-members.test.ts +++ b/server/tests/unit/mcp/tools-trip-members.test.ts @@ -987,7 +987,7 @@ describe('Guest tools stay owner-only', () => { // --------------------------------------------------------------------------- describe('Roster writes, as REST makes them', () => { - it('MCP-MEMBERS-001: add_trip_member sends the invitation the planner sends, naming the inviter by address', async () => { + it('MCP-MEMBERS-001: add_trip_member sends the invitation the planner sends, naming the inviter by name', async () => { const { user: owner } = createUser(testDb); const { user: invitee } = createUser(testDb); const trip = createTrip(testDb, owner.id); @@ -1004,7 +1004,7 @@ describe('Roster writes, as REST makes them', () => { actorId: owner.id, scope: 'user', targetId: invitee.id, - params: expect.objectContaining({ actor: owner.email, invitee: invitee.email, tripId: String(trip.id) }), + params: expect.objectContaining({ actor: owner.username, invitee: invitee.email, tripId: String(trip.id) }), }), ); } finally { diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index 0a8e36da0..baa896f83 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -413,7 +413,7 @@ describe('postNote / postMessage', () => { const send = vi.fn(async (_payload: { params: Record }) => ({ attempted: 0, delivered: 0 })); const notifications = { send, - actorEmail: vi.fn(async () => 'owner@collab.test'), + actorName: vi.fn(async () => 'owner'), } as unknown as NotificationsService; const service = await buildCollabService(collabFx.storage, rateLimit, notifications, realtime); return { service, realtime, send }; @@ -440,7 +440,7 @@ describe('postNote / postMessage', () => { actorId: user1.id, scope: 'trip', targetId: trip.id, - params: expect.objectContaining({ actor: 'owner@collab.test', tripId: String(trip.id) }), + params: expect.objectContaining({ actor: 'owner', tripId: String(trip.id) }), }), ); }); diff --git a/server/tests/unit/nest/packing-writes.service.test.ts b/server/tests/unit/nest/packing-writes.service.test.ts index 38319eda1..13dee98d3 100644 --- a/server/tests/unit/nest/packing-writes.service.test.ts +++ b/server/tests/unit/nest/packing-writes.service.test.ts @@ -1,6 +1,6 @@ import { DomainError } from '../../../src/nest/common/domain-error'; import { McpToolGuardsService } from '../../../src/nest/mcp-shared/mcp-tool-guards.service'; -import type { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; +import type { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { PackingWritesService } from '../../../src/nest/packing/packing-writes.service'; import type { PackingService } from '../../../src/nest/packing/packing.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; @@ -64,8 +64,8 @@ function setup(overrides: Record = {}, opts: { allowed?: boolea const permissions = { checkPermission: vi.fn().mockResolvedValue(opts.allowed ?? true), } as unknown as PermissionsService; - const mailer = { getUserEmail: vi.fn().mockResolvedValue(user.email) } as unknown as MailerService; - const writes = new PackingWritesService(packing, permissions, mailer); + const notifications = { actorName: vi.fn().mockResolvedValue('u') } as unknown as NotificationsService; + const writes = new PackingWritesService(packing, permissions, notifications); const guards = new McpToolGuardsService( { getOwnerId: vi.fn().mockResolvedValue(1) } as never, { getRole: vi.fn().mockResolvedValue('user') } as never, @@ -82,7 +82,7 @@ function setup(overrides: Record = {}, opts: { allowed?: boolea await fn(writer); return { names: names(), calls: [...realtime.broadcast.mock.calls] }; }; - return { writes, packing, permissions, mailer, realtime, run }; + return { writes, packing, permissions, notifications, realtime, run }; } describe('packing item events: REST == MCP', () => { @@ -274,19 +274,19 @@ describe('packing list use cases: REST == MCP', () => { const mcp = await s.run('mcp', (w) => s.writes.setCategoryAssignees(5, 'Clothes', [2], w)); expect(mcp.names).toEqual(rest.names); expect(rest.calls).toEqual([['5', 'packing:assignees', { category: 'Clothes', assignees }, 'sock']]); - // The MCP tool used to tell none of them; both name the actor by its email. - const actor = { id: 1, email: user.email }; + // The MCP tool used to tell none of them; both name the actor by name. + const actor = { id: 1, username: 'u' }; expect(s.packing.notifyTagged).toHaveBeenNthCalledWith(1, '5', actor, 'Clothes', [2]); expect(s.packing.notifyTagged).toHaveBeenNthCalledWith(2, '5', actor, 'Clothes', [2]); - expect(s.mailer.getUserEmail).toHaveBeenCalledWith(1); + expect(s.notifications.actorName).toHaveBeenCalledWith(1); }); - it('PACK-LIST-006: clearing a category notifies nobody, and an actor without an address is not named', async () => { + it('PACK-LIST-006: clearing a category notifies nobody, and an actor without a name is not named', async () => { const s = setup({ updateCategoryAssignees: vi.fn().mockResolvedValue([]), notifyTagged: vi.fn() }); await s.run('mcp', (w) => s.writes.setCategoryAssignees(5, 'Clothes', [], w)); - expect(s.mailer.getUserEmail).not.toHaveBeenCalled(); + expect(s.notifications.actorName).not.toHaveBeenCalled(); - vi.mocked(s.mailer.getUserEmail).mockResolvedValueOnce(null); + vi.mocked(s.notifications.actorName).mockResolvedValueOnce(''); await s.run('mcp', (w) => s.writes.setCategoryAssignees(5, 'Clothes', [2], w)); expect(s.packing.notifyTagged).not.toHaveBeenCalled(); }); diff --git a/server/tests/unit/nest/packing.controller.test.ts b/server/tests/unit/nest/packing.controller.test.ts index c987ca115..c576f86bc 100644 --- a/server/tests/unit/nest/packing.controller.test.ts +++ b/server/tests/unit/nest/packing.controller.test.ts @@ -1,4 +1,4 @@ -import type { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; +import type { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { PackingWritesService } from '../../../src/nest/packing/packing-writes.service'; import { PackingController as RoutePackingController } from '../../../src/nest/packing/packing.controller'; import type { PackingService } from '../../../src/nest/packing/packing.service'; @@ -63,13 +63,13 @@ function makeService(overrides: Partial = {}): PackingService { * The route over the real item use cases (PackingWritesService), with the * permission granted: the 403 lives in trip-access.guard.test.ts, and these * cases pin what the route answers and broadcasts through the service mock. - * The mailer answers the actor's email the assignee notification names. + * The notifications answer the actor's name the assignee notification gives. */ const allowAll = { checkPermission: vi.fn().mockResolvedValue(true) } as unknown as PermissionsService; -const mailer = { getUserEmail: vi.fn().mockResolvedValue(user.email) } as unknown as MailerService; +const notifications = { actorName: vi.fn().mockResolvedValue('u') } as unknown as NotificationsService; class PackingController extends RoutePackingController { constructor(svc: PackingService) { - super(svc, new PackingWritesService(svc, allowAll, mailer)); + super(svc, new PackingWritesService(svc, allowAll, notifications)); } } @@ -797,8 +797,8 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r { category: 'Clothes', assignees: [{ user_id: 2 }] }, 'sock', ); - // The actor is named by the email on its account row, as the MCP tool names it. - expect(notifyTagged).toHaveBeenCalledWith('5', { id: user.id, email: user.email }, 'Clothes', [2]); + // The actor is named by the name on its account row, as the MCP tool names it. + expect(notifyTagged).toHaveBeenCalledWith('5', { id: user.id, username: 'u' }, 'Clothes', [2]); }); it('notifies nobody when the category is cleared', async () => { diff --git a/server/tests/unit/nest/trip-member-writes.service.test.ts b/server/tests/unit/nest/trip-member-writes.service.test.ts index a95775947..8c091679f 100644 --- a/server/tests/unit/nest/trip-member-writes.service.test.ts +++ b/server/tests/unit/nest/trip-member-writes.service.test.ts @@ -57,7 +57,7 @@ afterAll(async () => { }); describe('TripMemberWritesService', () => { - it('MEMBER-WRITES-001: an inviter whose address cannot be read still sends the invitation, with an empty address', async () => { + it('MEMBER-WRITES-001: an inviter whose name cannot be read still sends the invitation, with an empty name', async () => { const member = { id: 21, email: 'invitee@example.test' }; roster.addMember.mockResolvedValue({ member, targetUserId: 21, tripTitle: 'Lisbon' }); const writer = writerFor(987654); @@ -69,7 +69,7 @@ describe('TripMemberWritesService', () => { expect(writer.events.emit).toHaveBeenCalledWith('member:added', { member }); expect(roster.notifyInvite).toHaveBeenCalledWith( '7', - { id: 987654, email: '' }, + { id: 987654, username: '' }, 21, 'Lisbon', 'invitee@example.test',