From 36566a13ab4846828bbc1079dc1284bdf4123a93 Mon Sep 17 00:00:00 2001 From: Jeff Man Date: Fri, 24 Jul 2026 22:01:10 +0100 Subject: [PATCH] fix(api): close three retry/duplicate hazards in the API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Weekly digest has no duplicate-send guard despite being triggered by an external scheduler over HTTP, so a retry currently double-sends to every subscribed user. Add a per-user lastDigestSentAt timestamp with a 6-day resend window, mirroring the existing reminderSentAt pattern. UpdateApplicationUseCase logged a field_updated activity entry whenever a field was present in the input, not when it actually changed, unlike the correctly-guarded status_changed branch next to it — a retried no-op update polluted the activity timeline. Now compares against the current value before logging. BulkDeleteApplicationsUseCase used Promise.all, so retrying a partially failed bulk-delete failed again on the already-deleted items. Switch to Promise.allSettled and treat NOT_FOUND as an idempotent no-op; other per-item failures still surface as before. No API contract change. --- .../migration.sql | 2 + apps/api/prisma/schema.prisma | 1 + .../BulkDeleteApplicationsUseCase.test.ts | 20 ++- .../jobs/UpdateApplicationUseCase.test.ts | 114 ++++++++++++++++++ .../digest/SendWeeklyDigestUseCase.test.ts | 41 +++++++ .../api/src/__tests__/helpers/createTestDb.ts | 1 + apps/api/src/__tests__/helpers/mocks.ts | 2 + apps/api/src/constants.ts | 6 + apps/api/src/domain/user/User.ts | 1 + .../db/repositories/PrismaUserRepository.ts | 6 + .../digest/SendWeeklyDigestUseCase.ts | 14 ++- .../jobs/BulkDeleteApplicationsUseCase.ts | 14 ++- .../jobs/UpdateApplicationUseCase.ts | 36 ++++-- .../src/use-cases/ports/IUserRepository.ts | 1 + 14 files changed, 241 insertions(+), 18 deletions(-) create mode 100644 apps/api/prisma/migrations/20260724205821_add_last_digest_sent_at/migration.sql diff --git a/apps/api/prisma/migrations/20260724205821_add_last_digest_sent_at/migration.sql b/apps/api/prisma/migrations/20260724205821_add_last_digest_sent_at/migration.sql new file mode 100644 index 00000000..6e20c2c8 --- /dev/null +++ b/apps/api/prisma/migrations/20260724205821_add_last_digest_sent_at/migration.sql @@ -0,0 +1,2 @@ +-- AlterTable +ALTER TABLE "User" ADD COLUMN "lastDigestSentAt" DATETIME; diff --git a/apps/api/prisma/schema.prisma b/apps/api/prisma/schema.prisma index c96cac8a..c6049ca2 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -16,6 +16,7 @@ model User { targetRole String? emailVerifiedAt DateTime? weeklyDigestEnabled Boolean @default(true) + lastDigestSentAt DateTime? followUpRemindersEnabled Boolean @default(true) totpSecret String? totpEnabled Boolean @default(false) diff --git a/apps/api/src/__tests__/application/jobs/BulkDeleteApplicationsUseCase.test.ts b/apps/api/src/__tests__/application/jobs/BulkDeleteApplicationsUseCase.test.ts index 380d2766..be06dc14 100644 --- a/apps/api/src/__tests__/application/jobs/BulkDeleteApplicationsUseCase.test.ts +++ b/apps/api/src/__tests__/application/jobs/BulkDeleteApplicationsUseCase.test.ts @@ -45,7 +45,7 @@ describe('BulkDeleteApplicationsUseCase', () => { }); }); - it('propagates a per-item error (e.g. NOT_FOUND for a stale id)', async () => { + it('treats a NOT_FOUND item as an idempotent no-op (retried bulk-delete)', async () => { const deleteApplicationUseCase = stub({ execute: vi.fn().mockImplementation(({ applicationId }: { applicationId: string }) => { if (applicationId === 'app-2') { @@ -56,10 +56,26 @@ describe('BulkDeleteApplicationsUseCase', () => { }); const useCase = new BulkDeleteApplicationsUseCase({ deleteApplicationUseCase }); + await expect( + useCase.execute({ userId: 'user-1', applicationIds: ['app-1', 'app-2'] }), + ).resolves.toBeUndefined(); + }); + + it('still throws when an item fails for a real reason (e.g. FORBIDDEN)', async () => { + const deleteApplicationUseCase = stub({ + execute: vi.fn().mockImplementation(({ applicationId }: { applicationId: string }) => { + if (applicationId === 'app-2') { + return Promise.reject(Object.assign(new Error('Forbidden'), { code: 'FORBIDDEN' })); + } + return Promise.resolve(undefined); + }), + }); + const useCase = new BulkDeleteApplicationsUseCase({ deleteApplicationUseCase }); + const err = await useCase .execute({ userId: 'user-1', applicationIds: ['app-1', 'app-2'] }) .catch((e) => e); - expect((err as { code: string }).code).toBe('NOT_FOUND'); + expect((err as { code: string }).code).toBe('FORBIDDEN'); }); }); diff --git a/apps/api/src/__tests__/application/jobs/UpdateApplicationUseCase.test.ts b/apps/api/src/__tests__/application/jobs/UpdateApplicationUseCase.test.ts index 63dbed15..b7f6789f 100644 --- a/apps/api/src/__tests__/application/jobs/UpdateApplicationUseCase.test.ts +++ b/apps/api/src/__tests__/application/jobs/UpdateApplicationUseCase.test.ts @@ -4,6 +4,7 @@ import { makeApplicationRepository, makeApplication, makeTransactionManager, + makeActivityLogRepository, } from '@/__tests__/helpers/mocks.js'; describe('UpdateApplicationUseCase', () => { @@ -133,4 +134,117 @@ describe('UpdateApplicationUseCase', () => { useCase.execute({ userId: 'user-1', applicationId: 'app-1', company: 'Acme' }), ).resolves.toBeDefined(); }); + + it('does not log field_updated when the submitted value equals the current value', async () => { + const existing = makeApplication({ company: 'Acme' }); + const applicationRepository = makeApplicationRepository({ + findById: vi.fn().mockResolvedValue(existing), + update: vi.fn().mockResolvedValue(existing), + }); + const activityLogRepository = makeActivityLogRepository(); + + const useCase = new UpdateApplicationUseCase({ + applicationRepository, + activityLogRepository, + generateId: () => 'log-1', + }); + await useCase.execute({ userId: 'user-1', applicationId: 'app-1', company: 'Acme' }); + + expect(activityLogRepository.append).not.toHaveBeenCalled(); + }); + + it('logs field_updated only for fields that actually changed', async () => { + const existing = makeApplication({ company: 'Acme', role: 'Engineer' }); + const applicationRepository = makeApplicationRepository({ + findById: vi.fn().mockResolvedValue(existing), + update: vi.fn().mockResolvedValue(makeApplication({ company: 'Acme', role: 'Staff Eng' })), + }); + const activityLogRepository = makeActivityLogRepository(); + + const useCase = new UpdateApplicationUseCase({ + applicationRepository, + activityLogRepository, + generateId: () => 'log-1', + }); + await useCase.execute({ + userId: 'user-1', + applicationId: 'app-1', + company: 'Acme', + role: 'Staff Eng', + }); + + expect(activityLogRepository.append).toHaveBeenCalledOnce(); + expect(activityLogRepository.append).toHaveBeenCalledWith( + expect.objectContaining({ payload: JSON.stringify({ fields: ['role'] }) }), + ); + }); + + it('treats an identical followUpAt instant as unchanged even with a new Date object', async () => { + const existing = makeApplication({ followUpAt: new Date('2026-08-01T00:00:00.000Z') }); + const applicationRepository = makeApplicationRepository({ + findById: vi.fn().mockResolvedValue(existing), + update: vi.fn().mockResolvedValue(existing), + }); + const activityLogRepository = makeActivityLogRepository(); + + const useCase = new UpdateApplicationUseCase({ + applicationRepository, + activityLogRepository, + generateId: () => 'log-1', + }); + await useCase.execute({ + userId: 'user-1', + applicationId: 'app-1', + followUpAt: new Date('2026-08-01T00:00:00.000Z'), + }); + + expect(activityLogRepository.append).not.toHaveBeenCalled(); + }); + + it('logs field_updated when followUpAt actually changes', async () => { + const existing = makeApplication({ followUpAt: new Date('2026-08-01T00:00:00.000Z') }); + const applicationRepository = makeApplicationRepository({ + findById: vi.fn().mockResolvedValue(existing), + update: vi.fn().mockResolvedValue(existing), + }); + const activityLogRepository = makeActivityLogRepository(); + + const useCase = new UpdateApplicationUseCase({ + applicationRepository, + activityLogRepository, + generateId: () => 'log-1', + }); + await useCase.execute({ + userId: 'user-1', + applicationId: 'app-1', + followUpAt: new Date('2026-08-15T00:00:00.000Z'), + }); + + expect(activityLogRepository.append).toHaveBeenCalledWith( + expect.objectContaining({ payload: JSON.stringify({ fields: ['followUpAt'] }) }), + ); + }); + + it('still logs status_changed as before (regression guard)', async () => { + const existing = makeApplication({ status: 'draft' }); + const applicationRepository = makeApplicationRepository({ + findById: vi.fn().mockResolvedValue(existing), + update: vi.fn().mockResolvedValue(makeApplication({ status: 'applied' })), + }); + const activityLogRepository = makeActivityLogRepository(); + + const useCase = new UpdateApplicationUseCase({ + applicationRepository, + activityLogRepository, + generateId: () => 'log-1', + }); + await useCase.execute({ userId: 'user-1', applicationId: 'app-1', status: 'applied' }); + + expect(activityLogRepository.append).toHaveBeenCalledWith( + expect.objectContaining({ + eventType: 'status_changed', + payload: JSON.stringify({ from: 'draft', to: 'applied' }), + }), + ); + }); }); diff --git a/apps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.ts b/apps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.ts index 21e6402f..b447e3da 100644 --- a/apps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.ts +++ b/apps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.ts @@ -95,6 +95,47 @@ describe('SendWeeklyDigestUseCase', () => { expect(result).toEqual({ totalUsers: 2, sent: 1, skipped: 1 }); expect(emailService.sendWeeklyDigest).toHaveBeenCalledOnce(); expect(emailService.sendWeeklyDigest).toHaveBeenCalledWith('a@test.com', expect.any(Object)); + expect(userRepository.updateLastDigestSentAt).toHaveBeenCalledOnce(); + expect(userRepository.updateLastDigestSentAt).toHaveBeenCalledWith('u1', expect.any(Date)); + }); + + it('skips a user whose lastDigestSentAt is within the resend window', async () => { + const user = makeUser({ lastDigestSentAt: new Date(Date.now() - 1 * DAY_MS) }); + const userRepository = makeUserRepository({ findAll: vi.fn().mockResolvedValue([user]) }); + const applicationRepository = makeApplicationRepository({ + findAllByUserId: vi.fn().mockResolvedValue([makeApplication()]), + }); + const emailService = makeEmailService(); + + const result = await new SendWeeklyDigestUseCase({ + userRepository, + applicationRepository, + emailService, + }).execute(); + + expect(result).toEqual({ totalUsers: 1, sent: 0, skipped: 1 }); + expect(applicationRepository.findAllByUserId).not.toHaveBeenCalled(); + expect(emailService.sendWeeklyDigest).not.toHaveBeenCalled(); + expect(userRepository.updateLastDigestSentAt).not.toHaveBeenCalled(); + }); + + it('sends again once the resend window has elapsed', async () => { + const user = makeUser({ lastDigestSentAt: new Date(Date.now() - 7 * DAY_MS) }); + const userRepository = makeUserRepository({ findAll: vi.fn().mockResolvedValue([user]) }); + const applicationRepository = makeApplicationRepository({ + findAllByUserId: vi.fn().mockResolvedValue([makeApplication()]), + }); + const emailService = makeEmailService(); + + const result = await new SendWeeklyDigestUseCase({ + userRepository, + applicationRepository, + emailService, + }).execute(); + + expect(result).toEqual({ totalUsers: 1, sent: 1, skipped: 0 }); + expect(emailService.sendWeeklyDigest).toHaveBeenCalledOnce(); + expect(userRepository.updateLastDigestSentAt).toHaveBeenCalledWith(user.id, expect.any(Date)); }); it('categorises new applications created in the last 7 days', async () => { diff --git a/apps/api/src/__tests__/helpers/createTestDb.ts b/apps/api/src/__tests__/helpers/createTestDb.ts index a321559a..e94727cc 100644 --- a/apps/api/src/__tests__/helpers/createTestDb.ts +++ b/apps/api/src/__tests__/helpers/createTestDb.ts @@ -14,6 +14,7 @@ const SCHEMA_STATEMENTS = [ "targetRole" TEXT, "emailVerifiedAt" DATETIME, "weeklyDigestEnabled" INTEGER NOT NULL DEFAULT 1, + "lastDigestSentAt" DATETIME, "followUpRemindersEnabled" INTEGER NOT NULL DEFAULT 1, "totpSecret" TEXT, "totpEnabled" INTEGER NOT NULL DEFAULT 0, diff --git a/apps/api/src/__tests__/helpers/mocks.ts b/apps/api/src/__tests__/helpers/mocks.ts index 88329ace..a2c9b6fe 100644 --- a/apps/api/src/__tests__/helpers/mocks.ts +++ b/apps/api/src/__tests__/helpers/mocks.ts @@ -37,6 +37,7 @@ export const makeUserRepository = (overrides?: Partial): IUserR create: vi.fn(), update: vi.fn(), delete: vi.fn(), + updateLastDigestSentAt: vi.fn().mockResolvedValue(undefined), ...overrides, }); @@ -268,6 +269,7 @@ export const makeUser = (overrides?: Partial): User => ({ targetRole: null, emailVerifiedAt: null, weeklyDigestEnabled: true, + lastDigestSentAt: null, followUpRemindersEnabled: true, totpSecret: null, totpEnabled: false, diff --git a/apps/api/src/constants.ts b/apps/api/src/constants.ts index 59d14237..6a8a6722 100644 --- a/apps/api/src/constants.ts +++ b/apps/api/src/constants.ts @@ -249,6 +249,12 @@ export const REMINDER_WINDOW_MS = { RESEND_AFTER: 23 * 60 * 60 * 1000, // don't resend within 23h } as const; +/** Weekly-digest resend-guard window, in milliseconds. */ +export const DIGEST_WINDOW_MS = { + /** Don't resend the digest if the last send was within this window (digest cadence is 7 days). */ + RESEND_AFTER: 6 * 24 * 60 * 60 * 1000, // 6 days +} as const; + /** Default field values applied when the caller omits them. */ export const DEFAULTS = { APPLICATION_STATUS: 'draft', diff --git a/apps/api/src/domain/user/User.ts b/apps/api/src/domain/user/User.ts index b73d61f2..a2625a77 100644 --- a/apps/api/src/domain/user/User.ts +++ b/apps/api/src/domain/user/User.ts @@ -7,6 +7,7 @@ export interface User { targetRole: string | null; emailVerifiedAt: Date | null; weeklyDigestEnabled: boolean; + lastDigestSentAt: Date | null; followUpRemindersEnabled: boolean; totpSecret: string | null; totpEnabled: boolean; diff --git a/apps/api/src/infrastructure/db/repositories/PrismaUserRepository.ts b/apps/api/src/infrastructure/db/repositories/PrismaUserRepository.ts index b7d997c4..d928ef47 100644 --- a/apps/api/src/infrastructure/db/repositories/PrismaUserRepository.ts +++ b/apps/api/src/infrastructure/db/repositories/PrismaUserRepository.ts @@ -12,6 +12,7 @@ type PrismaUser = { targetRole: string | null; emailVerifiedAt: Date | null; weeklyDigestEnabled: boolean; + lastDigestSentAt: Date | null; followUpRemindersEnabled: boolean; totpSecret: string | null; totpEnabled: boolean; @@ -73,6 +74,10 @@ export class PrismaUserRepository implements IUserRepository { await this.db.user.delete({ where: { id } }); } + async updateLastDigestSentAt(id: string, sentAt: Date): Promise { + await this.db.user.update({ where: { id }, data: { lastDigestSentAt: sentAt } }); + } + private toEntity(row: PrismaUser): User { return { id: row.id, @@ -83,6 +88,7 @@ export class PrismaUserRepository implements IUserRepository { targetRole: row.targetRole, emailVerifiedAt: row.emailVerifiedAt, weeklyDigestEnabled: row.weeklyDigestEnabled, + lastDigestSentAt: row.lastDigestSentAt, followUpRemindersEnabled: row.followUpRemindersEnabled, totpSecret: row.totpSecret, totpEnabled: row.totpEnabled, diff --git a/apps/api/src/use-cases/digest/SendWeeklyDigestUseCase.ts b/apps/api/src/use-cases/digest/SendWeeklyDigestUseCase.ts index 95226207..b2ed0983 100644 --- a/apps/api/src/use-cases/digest/SendWeeklyDigestUseCase.ts +++ b/apps/api/src/use-cases/digest/SendWeeklyDigestUseCase.ts @@ -1,7 +1,7 @@ import type { IUserRepository } from '@/use-cases/ports/IUserRepository.js'; import type { IApplicationRepository } from '@/use-cases/ports/IApplicationRepository.js'; import type { IEmailService, WeeklyDigestData } from '@/use-cases/ports/IEmailService.js'; -import { DURATIONS_MS } from '@/constants.js'; +import { DURATIONS_MS, DIGEST_WINDOW_MS } from '@/constants.js'; interface Deps { userRepository: IUserRepository; @@ -32,13 +32,22 @@ export class SendWeeklyDigestUseCase { return; } + const now = new Date(); + + if ( + user.lastDigestSentAt && + user.lastDigestSentAt.getTime() > now.getTime() - DIGEST_WINDOW_MS.RESEND_AFTER + ) { + skipped++; + return; + } + const apps = await this.deps.applicationRepository.findAllByUserId(user.id); if (apps.length === 0) { skipped++; return; } - const now = new Date(); const weekAgo = new Date(now.getTime() - SEVEN_DAYS_MS); const nextWeek = new Date(now.getTime() + SEVEN_DAYS_MS); @@ -73,6 +82,7 @@ export class SendWeeklyDigestUseCase { }; await this.deps.emailService.sendWeeklyDigest(user.email, data); + await this.deps.userRepository.updateLastDigestSentAt(user.id, now); sent++; }), ); diff --git a/apps/api/src/use-cases/jobs/BulkDeleteApplicationsUseCase.ts b/apps/api/src/use-cases/jobs/BulkDeleteApplicationsUseCase.ts index 6ba577dd..5c547210 100644 --- a/apps/api/src/use-cases/jobs/BulkDeleteApplicationsUseCase.ts +++ b/apps/api/src/use-cases/jobs/BulkDeleteApplicationsUseCase.ts @@ -1,5 +1,6 @@ import type { IDeleteApplicationUseCase } from '@/use-cases/jobs/IDeleteApplicationUseCase.js'; import { assertValidBulkIds } from '@/use-cases/jobs/bulkValidation.js'; +import { ERROR_CODES } from '@/constants.js'; import type { IBulkDeleteApplicationsUseCase, BulkDeleteApplicationsInput, @@ -15,10 +16,21 @@ export class BulkDeleteApplicationsUseCase implements IBulkDeleteApplicationsUse async execute(input: BulkDeleteApplicationsInput): Promise { assertValidBulkIds(input.applicationIds); - await Promise.all( + const results = await Promise.allSettled( input.applicationIds.map((applicationId) => this.deps.deleteApplicationUseCase.execute({ userId: input.userId, applicationId }), ), ); + + // A NOT_FOUND item is already gone (e.g. a retried bulk-delete after a + // partial success) — treat it as an idempotent no-op, not a failure. + const realFailure = results.find( + (r): r is PromiseRejectedResult => + r.status === 'rejected' && (r.reason as { code?: string })?.code !== ERROR_CODES.NOT_FOUND, + ); + + if (realFailure) { + throw realFailure.reason; + } } } diff --git a/apps/api/src/use-cases/jobs/UpdateApplicationUseCase.ts b/apps/api/src/use-cases/jobs/UpdateApplicationUseCase.ts index 44b0f311..d98ce53e 100644 --- a/apps/api/src/use-cases/jobs/UpdateApplicationUseCase.ts +++ b/apps/api/src/use-cases/jobs/UpdateApplicationUseCase.ts @@ -56,19 +56,29 @@ export class UpdateApplicationUseCase implements IUpdateApplicationUseCase { payload: JSON.stringify({ from: app.status, to: input.status }), }); } else { - const changed = ( - [ - 'company', - 'role', - 'jobUrl', - 'location', - 'salaryRange', - 'description', - 'source', - 'followUpAt', - 'starred', - ] as const - ).filter((f) => input[f as keyof typeof input] !== undefined); + const primitiveFields = [ + 'company', + 'role', + 'jobUrl', + 'location', + 'salaryRange', + 'description', + 'source', + 'starred', + ] as const; + + const changed: string[] = primitiveFields.filter( + (f) => input[f] !== undefined && input[f] !== app[f], + ); + + if (input.followUpAt !== undefined) { + const nextTime = input.followUpAt ? input.followUpAt.getTime() : null; + const prevTime = app.followUpAt ? app.followUpAt.getTime() : null; + if (nextTime !== prevTime) { + changed.push('followUpAt'); + } + } + if (changed.length > 0) { await this.deps.activityLogRepository.append({ id: genId(), diff --git a/apps/api/src/use-cases/ports/IUserRepository.ts b/apps/api/src/use-cases/ports/IUserRepository.ts index 13acf838..f95a81d9 100644 --- a/apps/api/src/use-cases/ports/IUserRepository.ts +++ b/apps/api/src/use-cases/ports/IUserRepository.ts @@ -21,4 +21,5 @@ export interface IUserRepository { }, ): Promise; delete(id: string): Promise; + updateLastDigestSentAt(id: string, sentAt: Date): Promise; }