mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-23 07:26:30 +00:00
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.
This commit is contained in:
@@ -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<Pick<User, 'id' | 'email'>> {
|
||||
return { id: writer.userId, email: await this.notifications.actorEmail(writer.userId) };
|
||||
private async actorOf(writer: TripWriter): Promise<Pick<User, 'id' | 'username'>> {
|
||||
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<User, 'id' | 'email'>, preview?: string): Promise<void> {
|
||||
async notifyCollab(tripId: string, actor: Pick<User, 'id' | 'username'>, preview?: string): Promise<void> {
|
||||
// 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
|
||||
|
||||
@@ -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<string> {
|
||||
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<string> {
|
||||
return (await this.notificationsRepo.findUserBasic(userId))?.username ?? '';
|
||||
}
|
||||
|
||||
async deleteOne(notificationId: number, userId: number): Promise<boolean> {
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -891,7 +891,7 @@ export class PackingService {
|
||||
}
|
||||
|
||||
/** Fire-and-forget tag notification, mirroring the legacy dynamic import. */
|
||||
async notifyTagged(tripId: string, actor: Pick<User, 'id' | 'email'>, category: string, userIds: unknown): Promise<void> {
|
||||
async notifyTagged(tripId: string, actor: Pick<User, 'id' | 'username'>, category: string, userIds: unknown): Promise<void> {
|
||||
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
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -120,7 +120,7 @@ export class TripMembersService {
|
||||
/** Fire-and-forget trip-invite notification (mirrors the route's dynamic import). */
|
||||
notifyInvite(
|
||||
tripId: string,
|
||||
actor: Pick<User, 'id' | 'email'>,
|
||||
actor: Pick<User, 'id' | 'username'>,
|
||||
targetUserId: number,
|
||||
tripTitle: string,
|
||||
inviteeEmail: string,
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -406,7 +406,7 @@ describe('Tool: set_packing_category_assignees', () => {
|
||||
await new Promise<void>((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' });
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -413,7 +413,7 @@ describe('postNote / postMessage', () => {
|
||||
const send = vi.fn(async (_payload: { params: Record<string, string> }) => ({ 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) }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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<string, unknown> = {}, 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<string, unknown> = {}, 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();
|
||||
});
|
||||
|
||||
@@ -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> = {}): 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 () => {
|
||||
|
||||
@@ -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',
|
||||
|
||||
Reference in New Issue
Block a user