From 128d15ea8fceada05f1e3fd921d659630b794c68 Mon Sep 17 00:00:00 2001 From: Pedro Castro Date: Wed, 31 Dec 2025 16:30:26 -0300 Subject: [PATCH] fix: ensure proper async handling in delegation credentials (#25898) * fix: ensure proper async handling in ensureDefaultCalendars Replace forEach(async...) with Promise.allSettled to ensure: - Caller properly awaits completion - Errors are captured and logged - All users are processed even if some fail Adds unit tests for ensureDefaultCalendars * fix: use correct Jest assertion pattern for async test Co-Authored-By: keith@cal.com --------- Co-authored-by: Keith Williams Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- ...ions-delegation-credential.service.spec.ts | 138 ++++++++++++++++++ ...nizations-delegation-credential.service.ts | 43 ++++-- 2 files changed, 165 insertions(+), 16 deletions(-) create mode 100644 apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.spec.ts diff --git a/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.spec.ts b/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.spec.ts new file mode 100644 index 0000000000..31841d4669 --- /dev/null +++ b/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.spec.ts @@ -0,0 +1,138 @@ +import { + CALENDARS_QUEUE, + DEFAULT_CALENDARS_JOB, +} from "@/ee/calendars/processors/calendars.processor"; +import { OrganizationsDelegationCredentialRepository } from "@/modules/organizations/delegation-credentials/organizations-delegation-credential.repository"; +import { OrganizationsDelegationCredentialService } from "@/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service"; +import { Logger } from "@nestjs/common"; +import { getQueueToken } from "@nestjs/bull"; +import { Test, TestingModule } from "@nestjs/testing"; + +describe("OrganizationsDelegationCredentialService", () => { + let service: OrganizationsDelegationCredentialService; + let mockRepository: OrganizationsDelegationCredentialRepository; + let mockQueue: { getJob: jest.Mock; add: jest.Mock }; + + const orgId = 1; + const domain = "example.com"; + + beforeEach(async () => { + mockQueue = { + getJob: jest.fn().mockResolvedValue(null), + add: jest.fn().mockResolvedValue(undefined), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + OrganizationsDelegationCredentialService, + { + provide: OrganizationsDelegationCredentialRepository, + useValue: { + findDelegatedUserProfiles: jest.fn().mockResolvedValue([]), + }, + }, + { + provide: getQueueToken(CALENDARS_QUEUE), + useValue: mockQueue, + }, + ], + }).compile(); + + service = module.get( + OrganizationsDelegationCredentialService + ); + mockRepository = module.get( + OrganizationsDelegationCredentialRepository + ); + + jest.spyOn(Logger.prototype, "log").mockImplementation(); + jest.spyOn(Logger.prototype, "error").mockImplementation(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe("ensureDefaultCalendars", () => { + it("adds calendar jobs for each delegated user profile", async () => { + (mockRepository.findDelegatedUserProfiles as jest.Mock).mockResolvedValue([ + { userId: 1 }, + { userId: 2 }, + ]); + + await service.ensureDefaultCalendars(orgId, domain); + + expect(mockRepository.findDelegatedUserProfiles).toHaveBeenCalledWith(orgId, domain); + expect(mockQueue.add).toHaveBeenCalledTimes(2); + expect(mockQueue.add).toHaveBeenCalledWith( + DEFAULT_CALENDARS_JOB, + { userId: 1 }, + { jobId: `${DEFAULT_CALENDARS_JOB}_1`, removeOnComplete: true } + ); + expect(mockQueue.add).toHaveBeenCalledWith( + DEFAULT_CALENDARS_JOB, + { userId: 2 }, + { jobId: `${DEFAULT_CALENDARS_JOB}_2`, removeOnComplete: true } + ); + }); + + it("removes existing job before adding new one", async () => { + const mockExistingJob = { remove: jest.fn().mockResolvedValue(undefined) }; + (mockRepository.findDelegatedUserProfiles as jest.Mock).mockResolvedValue([{ userId: 1 }]); + mockQueue.getJob.mockResolvedValue(mockExistingJob); + + await service.ensureDefaultCalendars(orgId, domain); + + expect(mockExistingJob.remove).toHaveBeenCalled(); + expect(mockQueue.add).toHaveBeenCalledTimes(1); + }); + + it("skips profiles without userId", async () => { + (mockRepository.findDelegatedUserProfiles as jest.Mock).mockResolvedValue([ + { userId: 1 }, + { userId: null }, + { userId: 3 }, + ]); + + await service.ensureDefaultCalendars(orgId, domain); + + expect(mockQueue.add).toHaveBeenCalledTimes(2); + expect(mockQueue.add).toHaveBeenCalledWith(DEFAULT_CALENDARS_JOB, { userId: 1 }, expect.any(Object)); + expect(mockQueue.add).toHaveBeenCalledWith(DEFAULT_CALENDARS_JOB, { userId: 3 }, expect.any(Object)); + }); + + it("does not add jobs when profiles list is empty", async () => { + (mockRepository.findDelegatedUserProfiles as jest.Mock).mockResolvedValue([]); + + await service.ensureDefaultCalendars(orgId, domain); + + expect(mockQueue.add).not.toHaveBeenCalled(); + }); + + it("processes all profiles even when some fail (Promise.allSettled)", async () => { + (mockRepository.findDelegatedUserProfiles as jest.Mock).mockResolvedValue([ + { userId: 1 }, + { userId: 2 }, + { userId: 3 }, + ]); + mockQueue.add + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("Queue error")) + .mockResolvedValueOnce(undefined); + + await service.ensureDefaultCalendars(orgId, domain); + + // All 3 jobs were attempted despite the failure + expect(mockQueue.add).toHaveBeenCalledTimes(3); + }); + + it("does not throw when repository fails", async () => { + (mockRepository.findDelegatedUserProfiles as jest.Mock).mockRejectedValue( + new Error("Database error") + ); + + await expect(service.ensureDefaultCalendars(orgId, domain)).resolves.toBeUndefined(); + expect(mockQueue.add).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.ts b/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.ts index 6541b40ef9..6ddd955147 100644 --- a/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.ts +++ b/apps/api/v2/src/modules/organizations/delegation-credentials/services/organizations-delegation-credential.service.ts @@ -87,23 +87,34 @@ export class OrganizationsDelegationCredentialService { const delegatedUserProfiles = await this.organizationsDelegationCredentialRepository.findDelegatedUserProfiles(orgId, domain); - delegatedUserProfiles.forEach(async (profile) => { - if (profile.userId) { - const job = await this.calendarsQueue.getJob(`${DEFAULT_CALENDARS_JOB}_${profile.userId}`); - if (job) { - await job.remove(); - this.logger.log(`Removed default calendar job for user with id: ${profile.userId}`); + const results = await Promise.allSettled( + delegatedUserProfiles.map(async (profile) => { + if (profile.userId) { + const job = await this.calendarsQueue.getJob(`${DEFAULT_CALENDARS_JOB}_${profile.userId}`); + if (job) { + await job.remove(); + this.logger.log(`Removed default calendar job for user with id: ${profile.userId}`); + } + this.logger.log(`Adding default calendar job for user with id: ${profile.userId}`); + await this.calendarsQueue.add( + DEFAULT_CALENDARS_JOB, + { + userId: profile.userId, + } satisfies DefaultCalendarsJobDataType, + { jobId: `${DEFAULT_CALENDARS_JOB}_${profile.userId}`, removeOnComplete: true } + ); } - this.logger.log(`Adding default calendar job for user with id: ${profile.userId}`); - await this.calendarsQueue.add( - DEFAULT_CALENDARS_JOB, - { - userId: profile.userId, - } satisfies DefaultCalendarsJobDataType, - { jobId: `${DEFAULT_CALENDARS_JOB}_${profile.userId}`, removeOnComplete: true } - ); - } - }); + }) + ); + + const failures = results.filter( + (result): result is PromiseRejectedResult => result.status === "rejected" + ); + if (failures.length > 0) { + this.logger.error( + `Failed to ensure default calendars for ${failures.length} users in org ${orgId}: ${failures.map((f) => f.reason).join(", ")}` + ); + } } catch (err) { this.logger.error( err,