From 61e442493a0d655f2ec8be41a3299dff0bb66ae2 Mon Sep 17 00:00:00 2001 From: Alex Holliday Date: Thu, 23 Jul 2026 10:49:59 -0700 Subject: [PATCH] extact monitor children deletion to helper --- server/src/domain/monitors/monitor.service.ts | 29 ++++++++++--------- .../test/unit/services/monitorService.test.ts | 28 ++++++++++++++---- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/server/src/domain/monitors/monitor.service.ts b/server/src/domain/monitors/monitor.service.ts index e6d339b80..711fc9b9b 100644 --- a/server/src/domain/monitors/monitor.service.ts +++ b/server/src/domain/monitors/monitor.service.ts @@ -451,15 +451,7 @@ export class MonitorService implements IMonitorService { return { monitors, failedCount }; }; - deleteMonitor = async ({ teamId, monitorId }: { teamId: string; monitorId: string }): Promise => { - const monitor = await this.monitorsRepository.deleteById(monitorId, teamId); - await this.monitorStatsRepository.deleteByMonitorId(monitor.id).catch((err: unknown) => { - this.logger.warn({ - message: `Error deleting monitor stats for monitor ${monitor.id} with name ${monitor.name}`, - service: SERVICE_NAME, - stack: err instanceof Error ? err.stack : undefined, - }); - }); + private deleteMonitorChildren = async (monitor: Monitor, teamId: string) => { await this.checksRepository.deleteByMonitorId(monitor.id).catch((err: unknown) => { this.logger.warn({ message: `Error deleting checks for monitor ${monitor.id} with name ${monitor.name}`, @@ -491,6 +483,19 @@ export class MonitorService implements IMonitorService { }); }); + await this.monitorStatsRepository.deleteByMonitorId(monitor.id).catch((err: unknown) => { + this.logger.warn({ + message: `Error deleting monitor stats for monitor ${monitor.id} with name ${monitor.name}`, + service: SERVICE_NAME, + stack: err instanceof Error ? err.stack : undefined, + }); + }); + }; + + deleteMonitor = async ({ teamId, monitorId }: { teamId: string; monitorId: string }): Promise => { + const monitor = await this.monitorsRepository.deleteById(monitorId, teamId); + + await this.deleteMonitorChildren(monitor, teamId); await this.scheduler.deleteJob(monitor); return monitor; }; @@ -500,12 +505,8 @@ export class MonitorService implements IMonitorService { await Promise.all( monitors.map(async (monitor) => { try { + await this.deleteMonitorChildren(monitor, teamId); await this.scheduler.deleteJob(monitor); - await this.checksRepository.deleteByMonitorId(monitor.id); - await this.geoChecksRepository.deleteByMonitorId(monitor.id); - await this.statusPagesRepository.removeMonitorFromStatusPages(monitor.id); - await this.monitorStatsRepository.deleteByMonitorId(monitor.id); - await this.incidentsRepository.deleteByMonitorId(monitor.id, teamId); } catch (error: unknown) { this.logger.warn({ message: `Error deleting associated records for monitor ${monitor.id} with name ${monitor.name}`, diff --git a/server/test/unit/services/monitorService.test.ts b/server/test/unit/services/monitorService.test.ts index 2b6f72d1d..085b3322f 100644 --- a/server/test/unit/services/monitorService.test.ts +++ b/server/test/unit/services/monitorService.test.ts @@ -1443,12 +1443,15 @@ describe("MonitorService", () => { const statusPagesRepository = createStatusPagesRepositoryMock(); const geoChecksRepository = createGeoChecksRepositoryMock(); + const incidentsRepository = createIncidentsRepositoryMock(); + const monitors = [makeMonitor({ id: "m1" }), makeMonitor({ id: "m2" })]; (monitorsRepository.deleteByTeamId as jest.Mock).mockResolvedValue({ monitors, deletedCount: 2 }); (checksRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); (geoChecksRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); (statusPagesRepository.removeMonitorFromStatusPages as jest.Mock).mockResolvedValue(0); (monitorStatsRepository.deleteByMonitorId as jest.Mock).mockResolvedValue({}); + (incidentsRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); const { service, jobQueue } = createService({ monitorsRepository, @@ -1456,6 +1459,7 @@ describe("MonitorService", () => { monitorStatsRepository, statusPagesRepository, geoChecksRepository, + incidentsRepository, }); const result = await service.deleteAllMonitors({ teamId: TEAM_ID }); @@ -1466,26 +1470,33 @@ describe("MonitorService", () => { expect(geoChecksRepository.deleteByMonitorId).toHaveBeenCalledTimes(2); expect(statusPagesRepository.removeMonitorFromStatusPages).toHaveBeenCalledTimes(2); expect(monitorStatsRepository.deleteByMonitorId).toHaveBeenCalledTimes(2); + expect(incidentsRepository.deleteByMonitorId).toHaveBeenCalledTimes(2); }); - it("logs warning when associated record deletion fails with Error", async () => { + it("logs a per-child warning and continues when a child deletion fails with Error", async () => { const monitorsRepository = createMonitorsRepositoryMock(); const checksRepository = createChecksRepositoryMock(); const monitorStatsRepository = createMonitorStatsRepositoryMock(); const statusPagesRepository = createStatusPagesRepositoryMock(); const geoChecksRepository = createGeoChecksRepositoryMock(); + const incidentsRepository = createIncidentsRepositoryMock(); const monitors = [makeMonitor({ id: "m1" })]; (monitorsRepository.deleteByTeamId as jest.Mock).mockResolvedValue({ monitors, deletedCount: 1 }); (checksRepository.deleteByMonitorId as jest.Mock).mockRejectedValue(new Error("fail")); + (geoChecksRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); + (statusPagesRepository.removeMonitorFromStatusPages as jest.Mock).mockResolvedValue(0); + (monitorStatsRepository.deleteByMonitorId as jest.Mock).mockResolvedValue({}); + (incidentsRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); const logger = createMockLogger(); - const { service } = createService({ + const { service, jobQueue } = createService({ monitorsRepository, checksRepository, monitorStatsRepository, statusPagesRepository, geoChecksRepository, + incidentsRepository, logger, }); @@ -1494,21 +1505,28 @@ describe("MonitorService", () => { expect(result).toBe(1); expect(logger.warn).toHaveBeenCalledWith( expect.objectContaining({ - message: expect.stringContaining("Error deleting associated records"), + message: expect.stringContaining("Error deleting checks"), stack: expect.any(String), }) ); + expect(jobQueue.deleteJob).toHaveBeenCalledTimes(1); }); - it("logs warning when associated record deletion fails with non-Error", async () => { + it("logs warning when scheduler job deletion fails with non-Error", async () => { const monitorsRepository = createMonitorsRepositoryMock(); const checksRepository = createChecksRepositoryMock(); const monitorStatsRepository = createMonitorStatsRepositoryMock(); const statusPagesRepository = createStatusPagesRepositoryMock(); const geoChecksRepository = createGeoChecksRepositoryMock(); + const incidentsRepository = createIncidentsRepositoryMock(); const monitors = [makeMonitor({ id: "m1" })]; (monitorsRepository.deleteByTeamId as jest.Mock).mockResolvedValue({ monitors, deletedCount: 1 }); + (checksRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); + (geoChecksRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); + (statusPagesRepository.removeMonitorFromStatusPages as jest.Mock).mockResolvedValue(0); + (monitorStatsRepository.deleteByMonitorId as jest.Mock).mockResolvedValue({}); + (incidentsRepository.deleteByMonitorId as jest.Mock).mockResolvedValue(0); // deleteJob throws non-Error const jobQueue = createJobQueueMock(); (jobQueue.deleteJob as jest.Mock).mockRejectedValue("string error"); @@ -1523,7 +1541,7 @@ describe("MonitorService", () => { geoChecksRepository, monitorStatsRepository, statusPagesRepository, - incidentsRepository: createIncidentsRepositoryMock(), + incidentsRepository, }); const result = await service.deleteAllMonitors({ teamId: TEAM_ID });