refactor: convert lucky user to service class #23329
Conversation
WalkthroughThis change refactors LuckyUser functionality into a class-based, dependency-injected service and wires it across API, libs, and tests. It introduces DI containers/modules and tokens, adds Prisma-backed repositories for Host and Attribute, and re-exports these via platform libraries. Core logic in getLuckyUser is migrated to LuckyUserService with public methods for selection and ordering. New repository methods support required queries (user last booking, hosts created in interval, OOO entries in interval, attribute weights). Call sites in features, TRPC handlers, and tests are updated to use getLuckyUserService(). A dependency bump to @calcom/platform-libraries 0.0.333 is included. Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
keithwillcode
added
core
ThyMinimalDev
changed the title
ThyMinimalDev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (48)
turbo.json (2)
269-273: Redundant env entry on a non-cached task (no functional effect).
@calcom/prisma#db-seedalready has"cache": false, andDAILY_API_KEYis present inglobalEnv(Line 41). The per-task"env": ["DAILY_API_KEY"]neither forwards the variable nor affects caching here, so it’s just config noise.
- If seeding truly requires
DAILY_API_KEY, enforce it in the seed script (fail fast with a clear error) rather than via Turbo config.Proposed minimal cleanup:
"@calcom/prisma#db-seed": { "cache": false, - "dependsOn": ["@calcom/prisma#db-migrate"], - "env": ["DAILY_API_KEY"] + "dependsOn": ["@calcom/prisma#db-migrate"] },
405-407: Make top-level db-seed non-cached to avoid accidental skips.The top-level
db-seedtask currently lacks"cache": false. Seeding should always run (or at least be explicit about caching), otherwise Turbo might skip it based on cache state. Keeping the explicitenvis fine for readability, but note it’s already inglobalEnv.
- Add
"cache": falseto ensure deterministic execution across CI and local runs.Suggested change:
- "db-seed": { - "env": ["DAILY_API_KEY"] - }, + "db-seed": { + "cache": false, + "env": ["DAILY_API_KEY"] + },packages/lib/server/repository/host.ts (3)
18-39: Use Prisma select to minimize payloadPer repo guidelines, avoid returning full rows. Select only fields the LuckyUserService actually needs (e.g., id, userId, eventTypeId, createdAt, isFixed).
async findHostsCreatedInInterval({ @@ }) { - return await this.prismaClient.host.findMany({ + return await this.prismaClient.host.findMany({ where: { @@ }, + select: { + id: true, + userId: true, + eventTypeId: true, + isFixed: true, + createdAt: true, + }, }); }
18-26: Guard against empty userIds inputShort-circuit to avoid an unnecessary query with IN [] on some drivers.
async findHostsCreatedInInterval({ @@ }: { @@ }) { + if (!userIds?.length) return []; return await this.prismaClient.host.findMany({
18-26: Method name suggests a closed interval but accepts only startDateEither rename to findHostsCreatedSince or accept an endDate and constrain accordingly.
- async findHostsCreatedInInterval({ - eventTypeId, - userIds, - startDate, - }: { + async findHostsCreatedSince({ + eventTypeId, + userIds, + startDate, + }: {Or:
async findHostsCreatedInInterval({ - eventTypeId, - userIds, - startDate, + eventTypeId, + userIds, + startDate, + endDate, }: { @@ - startDate: Date; + startDate: Date; + endDate: Date; }) { return await this.prismaClient.host.findMany({ where: { @@ - createdAt: { - gte: startDate, - }, + createdAt: { gte: startDate, lte: endDate }, },packages/lib/server/repository/PrismaAttributeRepository.ts (1)
59-98: Optional: allow narrowing assignedUsers to a candidate setIf LuckyUserService evaluates a known subset of users, pass userIds to reduce the cardinality of assignedUsers and lower memory/CPU.
Example shape (requires a new optional parameter):
- async findUniqueWithWeights({ teamId, attributeId, isWeightsEnabled = true }: + async findUniqueWithWeights({ teamId, attributeId, isWeightsEnabled = true, userIds }: { teamId: number; attributeId: string; isWeightsEnabled?: boolean; + userIds?: number[] }) { @@ - assignedUsers: { + assignedUsers: { + where: userIds?.length ? { member: { userId: { in: userIds } } } : undefined, select: { member: { select: { userId: true } }, weight: true, }, },Happy to wire this through DI if you want.
packages/lib/server/repository/user.ts (2)
1032-1069: Name clarityConsider findUsersWithLastAcceptedBooking to reflect the status filter explicitly.
-async findUsersWithLastBooking({ userIds, eventTypeId }: +async findUsersWithLastAcceptedBooking({ userIds, eventTypeId }:
1032-1069: Add missing composite indexes to Booking and Attendee for this query patternTo support efficient retrieval of each user’s most-recent accepted booking by event type (with no-show filters), we need these additional indexes in your Prisma schema:
• In packages/prisma/schema.prisma (Booking model, around lines 757–831):
- Composite index on (userId, eventTypeId, status, createdAt desc) to cover the where-clauses and ORDER BY.
- Single-column index on noShowHost to cover the OR filter.
• In packages/prisma/schema.prisma (Attendee model, around lines 733–747):
- Composite index on (bookingId, noShow) to cover the nested attendees.some filter.
Proposed diff:
model Booking { … noShowHost Boolean? @default(false) + @@index([userId, eventTypeId, status, createdAt(sort: Desc)]) + @@index([noShowHost]) @@index([eventTypeId]) @@index([userId]) @@index([destinationCalendarId]) @@index([recurringEventId]) @@index([uid]) @@index([status]) @@index([startTime, endTime, status]) } model Attendee { … noShow Boolean? @default(false) + @@index([bookingId, noShow]) @@index([email]) @@index([bookingId]) }packages/lib/server/repository/ooo.ts (1)
150-177: Interval method looks good; minimal selection and correct predicateThe overlap condition is correct and the selected fields are minimal. Consider selecting id as well if callers need to reference specific entries; otherwise LGTM.
select: { + id: true, start: true, end: true, userId: true, },packages/lib/di/modules/Attribute.ts (1)
6-9: Bind AttributeRepository to the read-only Prisma client for least-privilege.Attribute repo methods appear read-only. Prefer READ_ONLY_PRISMA_CLIENT to reduce risk and route read traffic appropriately.
Apply this diff:
-attributeRepositoryModule - .bind(DI_TOKENS.ATTRIBUTE_REPOSITORY) - .toClass(PrismaAttributeRepository, [DI_TOKENS.PRISMA_CLIENT]); +attributeRepositoryModule + .bind(DI_TOKENS.ATTRIBUTE_REPOSITORY) + .toClass(PrismaAttributeRepository, [DI_TOKENS.READ_ONLY_PRISMA_CLIENT]);packages/features/ee/round-robin/roundRobinReassignment.ts (2)
156-162: Pass meetingStartTime to LuckyUserService for consistent fairness logic.HandleNewBooking provides meetingStartTime; do the same here so reassignment follows the same selection semantics.
Apply this diff:
const reassignedRRHost = await luckyUserService.getLuckyUser({ availableUsers, eventType, allRRHosts: eventTypeHosts.filter((host) => !host.isFixed), // todo: only use hosts from virtual queue - routingFormResponse: null, + routingFormResponse: null, + meetingStartTime: booking.startTime, });
156-162: Option: pre-instantiate service outside hot paths.Minor micro-optimization: if this function starts doing more than one service call later, consider moving
const luckyUserService = getLuckyUserService();closer to the top of the function to avoid repeated container.get calls in future refactors.packages/features/bookings/lib/handleNewBooking.ts (2)
906-925: Hoist service resolution and cache enriched hosts to avoid repeated work inside loops.Right now we resolve the service and enrich hosts inside the group loop. Given this runs per group and potentially per retry, hoist both to reduce overhead and log noise.
Apply this diff within the loop body to use a precomputed array:
- const luckyUserService = getLuckyUserService(); const newLuckyUser = await luckyUserService.getLuckyUser({ // find a lucky user that is not already in the luckyUsers array availableUsers: freeUsers, // only hosts from the same group - allRRHosts: ( - await enrichHostsWithDelegationCredentials({ - orgId: firstUserOrgId ?? null, - hosts: eventTypeWithUsers.hosts, - }) - ).filter( + allRRHosts: enrichedRRHosts.filter( (host) => !host.isFixed && userIdsSet.has(host.user.id) && (host.groupId === groupId || (!host.groupId && groupId === DEFAULT_GROUP_ID)) ), eventType, routingFormResponse, meetingStartTime: new Date(reqBody.start), });And add these initializations once before entering the group iteration (outside this changed hunk):
// before: for (const [groupId, luckyUserPool] of Object.entries(luckyUserPools)) { const luckyUserService = getLuckyUserService(); const firstUserOrgId = await getOrgIdFromMemberOrTeamId({ memberId: eventTypeWithUsers.users[0].id ?? null, teamId: eventType.teamId, }); // resolve once; used by all groups const enrichedRRHosts = await enrichHostsWithDelegationCredentials({ orgId: firstUserOrgId ?? null, hosts: eventTypeWithUsers.hosts, });
906-925: Optional: precompute userIdsSet once per group, not per attempt.If the inner while-loop iterates multiple times, keep userIdsSet outside that loop to avoid re-allocations. Minor, but cheap to tidy up.
packages/lib/di/containers/LuckyUser.ts (2)
23-25: Add explicit return type to public DI accessor.Tiny DX improvement: annotate the return type for
getLuckyUserServiceto surface the API contract at call sites.-export function getLuckyUserService() { - return container.get<LuckyUserService>(DI_TOKENS.LUCKY_USER_SERVICE); -} +export function getLuckyUserService(): LuckyUserService { + return container.get<LuckyUserService>(DI_TOKENS.LUCKY_USER_SERVICE); +}
14-21: Consider exposing a factory for isolated containers (tests/serverless).The module-level singleton container is fine for prod, but tests and serverless environments sometimes need isolation to avoid cross-test state. Optional: provide a helper to build a fresh container on demand.
const container = createContainer(); container.load(DI_TOKENS.PRISMA_MODULE, prismaModule); @@ container.load(DI_TOKENS.LUCKY_USER_SERVICE_MODULE, luckyUserServiceModule); export function getLuckyUserService(): LuckyUserService { return container.get<LuckyUserService>(DI_TOKENS.LUCKY_USER_SERVICE); } + +// Optional: handy for tests or ephemeral workers needing a fresh container +export function createLuckyUserContainer() { + const c = createContainer(); + c.load(DI_TOKENS.PRISMA_MODULE, prismaModule); + c.load(DI_TOKENS.BOOKING_REPOSITORY_MODULE, bookingRepositoryModule); + c.load(DI_TOKENS.HOST_REPOSITORY_MODULE, hostRepositoryModule); + c.load(DI_TOKENS.OOO_REPOSITORY_MODULE, oooRepositoryModule); + c.load(DI_TOKENS.USER_REPOSITORY_MODULE, userRepositoryModule); + c.load(DI_TOKENS.ATTRIBUTE_REPOSITORY_MODULE, attributeRepositoryModule); + c.load(DI_TOKENS.LUCKY_USER_SERVICE_MODULE, luckyUserServiceModule); + return c; +}apps/api/v2/src/lib/repositories/prisma-host.repository.ts (2)
9-11: Use a read‐only Prisma client for HostRepositoryVerified that
packages/lib/server/repository/host.tsdefines no write operations (nocreate,update,upsert,delete, ortransactioncalls). To offload read traffic from the primary database, inject thePrismaReadServiceinstead of the write service.• File: apps/api/v2/src/lib/repositories/prisma-host.repository.ts
• Lines: 9–11 (constructor)Suggested diff:
- constructor(private readonly dbWrite: PrismaWriteService) { - super(dbWrite.prisma as unknown as PrismaClient); - } + constructor(private readonly dbRead: PrismaReadService) { + super(dbRead.prisma as unknown as PrismaClient); + }
9-11: PrismaClient cast simplification – confirm cross-package alignmentIt looks like every
Prisma*Repositoryinapps/api/v2/src/lib/repositoriesis currently doing:super(dbWrite.prisma as unknown as PrismaClient);If
@calcom/prismasimply re-exports or aliases the samePrismaClientfrom@prisma/client, you can drop theunknownhop and assert directly:- super(dbWrite.prisma as unknown as PrismaClient); + super(dbWrite.prisma as PrismaClient);Please verify that:
@calcom/prisma’sPrismaClientis structurally compatible (or aliased) to the one in@prisma/clientso that a direct cast compiles without error.- All repository subclasses still type-check after you remove the
unknowncast.Affected files (pattern):
• apps/api/v2/src/lib/repositories/prisma-*.repository.tsSuggested batch replace once you’ve confirmed compatibility:
rg -l 'super(dbWrite.prisma as unknown as PrismaClient)' \ apps/api/v2/src/lib/repositories \ | xargs sed -i 's/dbWrite.prisma as unknown as PrismaClient/dbWrite.prisma as PrismaClient/g'If you encounter any TS errors about mismatched PrismaClient types, keep the double-cast as is.
apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (2)
9-11: Same double-cast note as host repository.Prefer dropping the
unknownhop if type aliases align; otherwise keep it for compatibility.- super(dbWrite.prisma as unknown as PrismaClient); + super(dbWrite.prisma as PrismaClient);
1-12: Confirm file naming convention for new repositories.As with the Host repo, please verify team-wide guidance on “.repository.ts” for new files to keep consistency across packages.
packages/lib/server/getLuckyUser.integration-test.ts (2)
205-215: Reduce duplication in repeated getLuckyUser invocations.All call sites pass nearly identical shapes (availableUsers, eventType with isRRWeightsEnabled=false, allRRHosts=[], routingFormResponse=null). A tiny helper will trim ~100 LOC and make future param changes safer.
Example helper (add near top of file):
function callGetLuckyUser(availableUsers: any[], overrides?: Partial<Parameters<typeof luckyUserService.getLuckyUser>[0]>) { return luckyUserService.getLuckyUser({ availableUsers, eventType: { id: commonEventTypeId, isRRWeightsEnabled: false, team: {}, ...overrides?.eventType }, allRRHosts: overrides?.allRRHosts ?? [], routingFormResponse: overrides?.routingFormResponse ?? null, }); }Then replace sites like:
await callGetLuckyUser([organizerThatShowedUp, organizerThatDidntShowUp]);Also applies to: 265-276, 362-376, 429-440, 472-482, 485-496, 527-537, 547-557, 625-629, 708-712, 784-788
116-121: Prisma query in tests: avoid include and avoid loading credentials.
- Our guidelines recommend using select over include and fetching only fields needed.
- Loading user.credentials risks pulling sensitive fields (e.g., credential.key) into memory/logs even in tests.
Suggested change (sketch):
-include: { - bookings: true, - credentials: true, -}, +select: { + id: true, + email: true, + createdDate: true, + bookings: { + select: { + id: true, + uid: true, + title: true, + eventTypeId: true, + createdAt: true, + noShowHost: true, + attendees: { select: { noShow: true } }, + }, + }, + // omit credentials entirely +},If you want, I can scan the repo for any test code that fetches credentials with include and prepare a follow-up PR.
Also applies to: 154-161
apps/api/v2/src/lib/services/lucky-user.service.ts (2)
19-25: Use object property shorthand in the super(...) call.Minor readability nit; same values can be expressed more succinctly.
- super({ - bookingRepository: bookingRepository, - hostRepository: hostRepository, - oooRepository: oooRepository, - userRepository: userRepository, - attributeRepository: attributeRepository, - }); + super({ + bookingRepository, + hostRepository, + oooRepository, + userRepository, + attributeRepository, + });
10-12: File naming vs. repo guideline.Repo guidance discourages new “.service.ts” suffixes. NestJS typically prefers them, so consider:
- Keep current name for Nest parity, or
- Rename to lucky-user.ts for repo consistency and map via module providers.
Given this is a new file, confirm the intended convention for api/v2. I’m ok either way if we align consistently.
Want me to inventory “.service.ts” files under apps/api/v2 to see prevailing convention?
packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (3)
79-104: Replace Prisma include with select to comply with data-minimization policy.Guideline: “For Prisma queries, only select data you need; never use include.” Here you can explicitly select the few fields used later.
Sketch (adjust if getSerializableForm requires more scalars):
- const form = await prisma.app_RoutingForms_Form.findFirst({ + const form = await prisma.app_RoutingForms_Form.findFirst({ where: { id: formId, ...entityPrismaWhereClause({ userId: user.id }), }, - include: { - team: { - select: { - parentId: true, - parent: { select: { slug: true } }, - metadata: true, - }, - }, - user: { - select: { id: true, username: true, movedToProfileId: true }, - }, - }, + select: { + id: true, + teamId: true, + team: { + select: { + parentId: true, + parent: { select: { slug: true } }, + metadata: true, + }, + }, + user: { + select: { id: true, username: true, movedToProfileId: true }, + }, + }, });If you’re unsure which extra scalars getSerializableForm needs, I can scan its usage and generate an exact select shape.
331-331: Prefer named export over default export.Repo guidance discourages default exports. Recommend:
-export default findTeamMembersMatchingAttributeLogicOfRouteHandler; +export { findTeamMembersMatchingAttributeLogicOfRouteHandler };Follow-up will need to update import sites.
I can prepare a codemod to update imports across the repo if you want to adopt this.
305-306: Avoid unconditional console logging in server paths.The
_enablePerfflag-gated Server-Timing log is fine, but the previous console.log runs unconditionally. Wrap it with the same flag or drop it.- console.log("_enablePerf, _concurrency", _enablePerf, _concurrency); - if (_enablePerf) { + if (_enablePerf) { + console.log("_enablePerf, _concurrency", _enablePerf, _concurrency); const serverTimingHeader = getServerTimingHeader(timeTaken); ctx.res?.setHeader("Server-Timing", serverTimingHeader); console.log("Server-Timing", serverTimingHeader); }packages/lib/server/getLuckyUser.test.ts (2)
64-75: Introduce a tiny helper to DRY repeated getLuckyUser calls in tests.Same benefit as in the integration test: fewer args at call sites, easier future changes.
Example:
function gu(availableUsers: any[], overrides?: Partial<Parameters<typeof luckyUserService.getLuckyUser>[0]>) { return luckyUserService.getLuckyUser({ availableUsers, eventType: { id: 1, // or commonEventTypeId if applicable in this file isRRWeightsEnabled: !!overrides?.eventType?.isRRWeightsEnabled, team: overrides?.eventType?.team ?? { rrResetInterval: RRResetInterval.MONTH, rrTimestampBasis: RRTimestampBasis.CREATED_AT }, includeNoShowInRRCalculation: overrides?.eventType?.includeNoShowInRRCalculation ?? false, }, allRRHosts: overrides?.allRRHosts ?? [], routingFormResponse: overrides?.routingFormResponse ?? null, }); }Also applies to: 117-131, 177-188, 239-251, 341-353, 460-471, 579-590, 685-697, 806-817, 913-924, 943-954, 1482-1497
430-434: Duplicate booking ids in test fixtures.Several mocked bookings use the same id (e.g., id: 4). While not functionally relevant here, unique ids reduce confusion and make future assertions safer. Either omit id or ensure uniqueness.
- buildBooking({ id: 4, userId: 2, createdAt: ... }) + buildBooking({ id: 40, userId: 2, createdAt: ... })Also applies to: 549-552, 666-681, 789-803
packages/lib/bookings/filterHostsByLeadThreshold.test.ts (6)
10-10: Avoid binding your spy to a potentially different DI instanceSpying on a service instance created at module load (
const luckyUserService = getLuckyUserService()) is brittle if the DI container is not a strict singleton or is reset between tests. Prefer spying on the DI factory or injecting a stub into the subject under test so the code under test and the spy refer to the same instance.Option A (test-only): Spy on the container function and return a stable stub.
+import * as LuckyUserContainer from "@calcom/lib/di/containers/LuckyUser"; ... -let getOrderedListOfLuckyUsersMock: Mock; +let getOrderedListOfLuckyUsersMock: vi.Mock; +let serviceStub: Pick<ReturnType<typeof LuckyUserContainer.getLuckyUserService>, "getOrderedListOfLuckyUsers">; beforeEach(() => { vi.clearAllMocks(); - // Spy on the real method and explicitly cast it as a Mock - getOrderedListOfLuckyUsersMock = vi.spyOn(luckyUserService, "getOrderedListOfLuckyUsers") as Mock; + serviceStub = { getOrderedListOfLuckyUsers: vi.fn() }; + getOrderedListOfLuckyUsersMock = serviceStub.getOrderedListOfLuckyUsers as unknown as vi.Mock; + vi.spyOn(LuckyUserContainer, "getLuckyUserService").mockReturnValue(serviceStub as any); });This decouples the test from DI lifetime and makes intent explicit.
22-30: Strengthen typing: avoidas Mock; use a properly typed Spy/MockedFunctionCasting to
Mockloses type safety and hid that your mock didn’t return the full shape the service promises. Usevi.MockedFunctionorSpyInstancetyped to the actual method signature.Apply:
-import type { Mock } from "vitest"; +import type { MockedFunction } from "vitest"; ... -let getOrderedListOfLuckyUsersMock: Mock; +let getOrderedListOfLuckyUsersMock: MockedFunction<typeof luckyUserService.getOrderedListOfLuckyUsers>; ... - getOrderedListOfLuckyUsersMock = vi.spyOn(luckyUserService, "getOrderedListOfLuckyUsers") as Mock; + getOrderedListOfLuckyUsersMock = vi.spyOn(luckyUserService, "getOrderedListOfLuckyUsers");This will also force the mocked return to include required fields like
usersandisUsingAttributeWeights.
25-30: Clearing all mocks wipes the module-scope prisma spy
vi.clearAllMocks()resets call counts and mocked implementations, including the module-scopevi.spyOn(prisma.booking, "groupBy"). If that spy is still needed, re-apply it inbeforeEach; otherwise, delete the dead mocking code entirely since this suite doesn’t rely on it anymore.If keeping it:
beforeEach(() => { vi.clearAllMocks(); vi.spyOn(prisma.booking, "groupBy").mockImplementation(prismaMock.booking.groupBy); ... });If not needed, remove the module-scope spy/imports to reduce noise.
110-115: Return full service shape from the mock for robustnessThe real
getOrderedListOfLuckyUsersreturns{ users, isUsingAttributeWeights, perUserData }. Returning onlyperUserDataworks here by coincidence (loose typing), but will break once the spy is strictly typed (see prior comment).Apply:
- getOrderedListOfLuckyUsersMock.mockResolvedValue({ - perUserData: { - bookingsCount: { 1: 10, 2: 6 }, - }, - }); + getOrderedListOfLuckyUsersMock.mockResolvedValue({ + users: [], + isUsingAttributeWeights: false, + perUserData: { + bookingsCount: { 1: 10, 2: 6 }, + bookingShortfalls: null, + calibrations: null, + weights: null, + }, + });And for the weighted case, set
isUsingAttributeWeights: true.Also applies to: 167-175
38-66: Assert the service is not called when the filter is skippedSince
maxLeadThreshold: nullshort-circuits, add an assertion that LuckyUserService wasn’t invoked. This guards against regressions.Example:
await expect(filterHostsByLeadThreshold({...})).resolves.toStrictEqual(hosts); expect(getOrderedListOfLuckyUsersMock).not.toHaveBeenCalled();
176-189: Remove stray, unused promise invocation
const test = filterHostsByLeadThreshold(...)produces an unawaited promise and duplicates the call. Remove it to avoid side effects and flakiness.- const test = filterHostsByLeadThreshold({ - hosts, - maxLeadThreshold: 3, - eventType: { - id: 1, - isRRWeightsEnabled: true, - team: { - parentId: null, - rrResetInterval: RRResetInterval.MONTH, - }, - }, - routingFormResponse: null, - });packages/lib/bookings/filterHostsByLeadThreshold.ts (6)
6-8: Import types: avoid coupling to the class just to derive a return typeUsing
LuckyUserService.prototypein a conditional type is more complex than necessary and ties this module to the concrete class. Prefer the instance type or a public API interface for the return shape.-import type { LuckyUserService, RoutingFormResponse } from "../server/getLuckyUser"; +import type { LuckyUserService, RoutingFormResponse } from "../server/getLuckyUser";And for the alias below, switch to the instance method:
-type PerUserData = Awaited< - ReturnType<(typeof LuckyUserService.prototype)["getOrderedListOfLuckyUsers"]> ->["perUserData"]; +type PerUserData = Awaited<ReturnType<LuckyUserService["getOrderedListOfLuckyUsers"]>>["perUserData"];If possible, expose a dedicated
LuckyUserServiceAPIinterface and reference that instead of the class.
49-55: Non-inclusive and confusing log messageMessage uses “made him exceed” (gendered) and says “filtered out” inside the allow-branch. Tighten and neutralize wording.
- log.debug( - `Host ${userIdStr} has been filtered out because the amount of bookings made him exceed the thresholds. Shortfall: ${shortfall}, Max Shortfall: ${maxShortfall}` - ); + log.debug( + `Host ${userIdStr} filtered out: bookings exceed lead threshold. Shortfall=${shortfall}, MaxShortfall=${maxShortfall}` + );
71-76: Contradictory “filtered out” message in the allow-branchMessage says “…has been filtered out…” while the code is allowing the host. Fix the wording.
- log.debug( - `Host Allowed ${userIdStr} has been filtered out because the given data made them exceed the thresholds. BookingsCount: ${bookingsCount}, MinBookings: ${minBookings}, MaxLeadThreshold: ${maxLeadThreshold}` - ); + log.debug( + `Host allowed ${userIdStr}. BookingsCount=${bookingsCount}, MinBookings=${minBookings}, MaxLeadThreshold=${maxLeadThreshold}` + );
72-73: UseNumber()overparseIntfor numeric object-keysYou’re converting object keys that are already numeric ids stringified by JS.
Number(userIdStr)is faster and safer thanparseInt(userIdStr, 10).- filteredUserIds.push(parseInt(userIdStr, 10)); + filteredUserIds.push(Number(userIdStr));Apply in both with-weights and without-weights helpers.
Also applies to: 53-54
120-123: Dependency injection from inside the function complicates testingAcquiring the service from the container inside the hot path makes unit testing rely on container lifetime and complicates stubbing. Consider an optional parameter with a sensible default to improve testability and reduce DI coupling.
-export const filterHostsByLeadThreshold = async <T extends BaseHost<BaseUser>>({ +export const filterHostsByLeadThreshold = async <T extends BaseHost<BaseUser>>({ hosts, maxLeadThreshold, eventType, routingFormResponse, }: { ... -}) => { +}, deps: { luckyUserService?: LuckyUserService } = {}) => { ... - const luckyUserService = getLuckyUserService(); + const luckyUserService = deps.luckyUserService ?? getLuckyUserService();Tests can now pass
{ luckyUserService: stub }without spying on container state.
142-161: Hard failure on partial per-user data could be handled more gracefullyThrowing when any of
calibrations,weights, orbookingShortfallsis null may be correct logically, but it will bubble to callers and break booking flows. Consider logging an error and falling back to the non-weighted path (or bypass filter) to keep the system resilient.Example fallback:
- if ( + if ( perUserData.calibrations === null || perUserData.weights === null || perUserData.bookingShortfalls === null ) { - throw new Error("Calibrations, weights, or booking shortfalls are null"); + log.error("Weighted filtering requested but per-user data was null; falling back to non-weighted."); + return filterHostsByLeadThresholdWithoutWeights(perUserData, maxLeadThreshold); }packages/lib/server/getLuckyUser.ts (7)
836-849: Hot path does heavy dayjs.tz work and uses DST-unsafe full-day checkThe per-busy-time loop constructs timezone offsets via
dayjs(...).tz(...).utcOffset()and then checks full-day with a fixed 24h. This is both heavy and incorrect near DST changes.
- Use the new
isFullDayEventInTZ(start, end, timeZone)helper directly.- Avoid manual offset math; rely on dayjs.tz only for the boundary checks (cheaper than recomputing offset twice per event).
- Cache the parsed
responseand/or computed results if this runs frequently.-const timezoneOffset = dayjs(busyTime.start).tz(busyTime.timeZone).utcOffset() * 60000; -let start = new Date(new Date(busyTime.start).getTime() + timezoneOffset); -const end = new Date(new Date(busyTime.end).getTime() + timezoneOffset); +const start = new Date(busyTime.start); +const end = new Date(busyTime.end); ... -return end.getTime() < new Date().getTime() && isFullDayEvent(start, end); +return end.getTime() < Date.now() && isFullDayEventInTZ(start, end, busyTime.timeZone);This fixes DST issues and simplifies the code.
187-205:ILuckyUserServiceis actually dependency bag; rename to avoid confusionThe interface named
ILuckyUserServiceonly describes constructor dependencies and exposes no service API. This misleads consumers and complicates typing (e.g., deriving return types elsewhere).Consider:
-export interface ILuckyUserService { +export interface LuckyUserServiceDeps { bookingRepository: BookingRepository; hostRepository: HostRepository; oooRepository: PrismaOOORepository; userRepository: UserRepository; attributeRepository: PrismaAttributeRepository; } -export class LuckyUserService implements ILuckyUserService { +export class LuckyUserService { public bookingRepository: BookingRepository; ... - constructor(deps: ILuckyUserService) { + constructor(deps: LuckyUserServiceDeps) { this.bookingRepository = deps.bookingRepository; ... }Optionally, expose a separate
LuckyUserServiceAPItype for the public methods to decouple call sites from the class.
253-256: Unreachable check
userIdAndAtCreatedPairis always a truthy object. This branch never triggers.- if (!userIdAndAtCreatedPair) { - throw new Error("Unable to find users by availableUser ids."); - } + // All available users are represented; nothing to throw here.
257-264: Tie-breaking and stability in least-recently-booked selectionWhen timestamps tie, sorting returns 0 and order depends on runtime sort stability. Add a deterministic tiebreaker (e.g., by id) to avoid nondeterminism across environments.
- const leastRecentlyBookedUser = availableUsers.sort((a, b) => { - if (userIdAndAtCreatedPair[a.id] > userIdAndAtCreatedPair[b.id]) return 1; - else if (userIdAndAtCreatedPair[a.id] < userIdAndAtCreatedPair[b.id]) return -1; - else return 0; - })[0]; + const leastRecentlyBookedUser = availableUsers.sort((a, b) => { + const da = userIdAndAtCreatedPair[a.id]; + const db = userIdAndAtCreatedPair[b.id]; + if (da > db) return 1; + if (da < db) return -1; + return a.id - b.id; + })[0];
520-555: Avoid using.mapfor side effects; prefer loops for clarity
fieldValueArray.mapis used only for side effects to buildaverageWeightsHosts, which hurts readability.- fieldValueArray.map((obj) => { + for (const obj of fieldValueArray) { ... - }); + }
1101-1108:bookingsCountexcludes attendee participation; verify intended semanticsYou compute
perUserBookingsCountusingbooking.userId === luckyUser.idonly. Elsewhere, weights/shortfalls treat a user as “booked” if they were organizer or attendee. If lead-threshold logic expects parity with shortfall calculation, counts should include attendee participation for consistency.- perUserBookingsCount[luckyUser.id] = bookingsOfAvailableUsersOfInterval.filter( - (booking) => booking.userId === luckyUser.id - ).length; + perUserBookingsCount[luckyUser.id] = bookingsOfAvailableUsersOfInterval.filter( + (booking) => + booking.userId === luckyUser.id || + booking.attendees.some((att) => att.email === luckyUser.email) + ).length;If organizer-only counts are intentional for lead threshold, add a comment stating so and ensure tests cover both organizer and attendee scenarios.
108-129: Interval helpers returnnew Date()for CREATED_AT end; confirm intent
getIntervalEndDatereturnsnew Date()forRRTimestampBasis.CREATED_AT. This differs from DAY/MONTH end-of-interval semantics and may produce uneven windows. If this is intentional, add a comment; otherwise consider usingendOfDay()/endOfMonth()similar toSTART_TIME.Briefly document the business rule for CREATED_AT windows.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
apps/api/v2/package.json(1 hunks)apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts(1 hunks)apps/api/v2/src/lib/repositories/prisma-host.repository.ts(1 hunks)apps/api/v2/src/lib/services/lucky-user.service.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(2 hunks)packages/features/ee/round-robin/roundRobinReassignment.ts(2 hunks)packages/lib/bookings/filterHostsByLeadThreshold.test.ts(4 hunks)packages/lib/bookings/filterHostsByLeadThreshold.ts(3 hunks)packages/lib/di/containers/LuckyUser.ts(1 hunks)packages/lib/di/modules/Attribute.ts(1 hunks)packages/lib/di/modules/Host.ts(1 hunks)packages/lib/di/modules/LuckyUser.ts(1 hunks)packages/lib/di/tokens.ts(1 hunks)packages/lib/server/getLuckyUser.integration-test.ts(12 hunks)packages/lib/server/getLuckyUser.test.ts(27 hunks)packages/lib/server/getLuckyUser.ts(3 hunks)packages/lib/server/repository/PrismaAttributeRepository.ts(1 hunks)packages/lib/server/repository/host.ts(1 hunks)packages/lib/server/repository/ooo.ts(1 hunks)packages/lib/server/repository/user.ts(2 hunks)packages/platform/libraries/bookings.ts(1 hunks)packages/platform/libraries/repositories.ts(2 hunks)packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts(2 hunks)turbo.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/di/containers/LuckyUser.tspackages/lib/di/modules/Host.tsapps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/lib/server/repository/user.tspackages/lib/di/tokens.tspackages/lib/di/modules/Attribute.tsapps/api/v2/src/lib/services/lucky-user.service.tspackages/lib/di/modules/LuckyUser.tspackages/platform/libraries/bookings.tspackages/lib/server/repository/host.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/platform/libraries/repositories.tspackages/lib/server/repository/PrismaAttributeRepository.tspackages/lib/server/getLuckyUser.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsByLeadThreshold.test.tspackages/lib/server/repository/ooo.tspackages/lib/server/getLuckyUser.integration-test.tspackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/getLuckyUser.test.tspackages/lib/bookings/filterHostsByLeadThreshold.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/di/containers/LuckyUser.tspackages/lib/di/modules/Host.tsapps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/lib/server/repository/user.tspackages/lib/di/tokens.tspackages/lib/di/modules/Attribute.tsapps/api/v2/src/lib/services/lucky-user.service.tspackages/lib/di/modules/LuckyUser.tspackages/platform/libraries/bookings.tspackages/lib/server/repository/host.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/platform/libraries/repositories.tspackages/lib/server/repository/PrismaAttributeRepository.tspackages/lib/server/getLuckyUser.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsByLeadThreshold.test.tspackages/lib/server/repository/ooo.tspackages/lib/server/getLuckyUser.integration-test.tspackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/getLuckyUser.test.tspackages/lib/bookings/filterHostsByLeadThreshold.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/di/containers/LuckyUser.tspackages/lib/di/modules/Host.tsapps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/lib/server/repository/user.tspackages/lib/di/tokens.tspackages/lib/di/modules/Attribute.tsapps/api/v2/src/lib/services/lucky-user.service.tspackages/lib/di/modules/LuckyUser.tspackages/platform/libraries/bookings.tspackages/lib/server/repository/host.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/platform/libraries/repositories.tspackages/lib/server/repository/PrismaAttributeRepository.tspackages/lib/server/getLuckyUser.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsByLeadThreshold.test.tspackages/lib/server/repository/ooo.tspackages/lib/server/getLuckyUser.integration-test.tspackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/getLuckyUser.test.tspackages/lib/bookings/filterHostsByLeadThreshold.ts
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
apps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tsapps/api/v2/src/lib/services/lucky-user.service.ts
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/lib/server/repository/PrismaAttributeRepository.ts
🧠 Learnings (2)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*Repository.ts : Repository files must include `Repository` suffix, prefix with technology if applicable (e.g., `PrismaAppRepository.ts`), and use PascalCase matching the exported class
Applied to files:
apps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/platform/libraries/repositories.ts
📚 Learning: 2025-08-21T13:44:06.784Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.784Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/lib/server/repository/user.tspackages/lib/server/getLuckyUser.ts
🧬 Code graph analysis (15)
packages/lib/di/containers/LuckyUser.ts (9)
packages/lib/di/tokens.ts (1)
DI_TOKENS(1-54)packages/prisma/prisma.module.ts (1)
prismaModule(7-7)packages/lib/di/modules/Booking.ts (1)
bookingRepositoryModule(6-6)packages/lib/di/modules/Host.ts (1)
hostRepositoryModule(6-6)packages/lib/di/modules/Ooo.ts (1)
oooRepositoryModule(6-6)packages/lib/di/modules/User.ts (1)
userRepositoryModule(6-6)packages/lib/di/modules/Attribute.ts (1)
attributeRepositoryModule(6-6)packages/lib/di/modules/LuckyUser.ts (1)
luckyUserServiceModule(6-6)packages/lib/server/getLuckyUser.ts (1)
LuckyUserService(195-1135)
packages/lib/di/modules/Host.ts (3)
packages/lib/di/tokens.ts (1)
DI_TOKENS(1-54)packages/lib/server/repository/host.ts (1)
HostRepository(3-40)packages/platform/libraries/repositories.ts (1)
HostRepository(12-12)
apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (3)
apps/api/v2/src/lib/repositories/prisma-host.repository.ts (1)
Injectable(7-12)apps/api/v2/src/lib/services/lucky-user.service.ts (1)
Injectable(10-27)packages/lib/server/repository/PrismaAttributeRepository.ts (1)
PrismaAttributeRepository(3-99)
apps/api/v2/src/lib/repositories/prisma-host.repository.ts (3)
apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (1)
Injectable(7-12)apps/api/v2/src/lib/services/lucky-user.service.ts (1)
Injectable(10-27)packages/prisma/index.ts (1)
PrismaClient(83-83)
packages/lib/di/modules/Attribute.ts (3)
packages/lib/di/tokens.ts (1)
DI_TOKENS(1-54)packages/lib/server/repository/PrismaAttributeRepository.ts (1)
PrismaAttributeRepository(3-99)packages/platform/libraries/repositories.ts (1)
PrismaAttributeRepository(1-1)
apps/api/v2/src/lib/services/lucky-user.service.ts (4)
apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (1)
Injectable(7-12)apps/api/v2/src/lib/repositories/prisma-host.repository.ts (1)
Injectable(7-12)packages/lib/server/getLuckyUser.ts (1)
LuckyUserService(195-1135)packages/lib/server/repository/ooo.ts (1)
PrismaOOORepository(3-178)
packages/lib/di/modules/LuckyUser.ts (2)
packages/lib/di/tokens.ts (1)
DI_TOKENS(1-54)packages/lib/server/getLuckyUser.ts (1)
LuckyUserService(195-1135)
packages/features/ee/round-robin/roundRobinReassignment.ts (1)
packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
packages/lib/server/getLuckyUser.ts (7)
packages/lib/raqb/raqbUtils.ts (1)
acrossQueryValueCompatiblity(219-224)packages/lib/server/repository/host.ts (1)
HostRepository(3-40)packages/lib/server/repository/PrismaAttributeRepository.ts (1)
PrismaAttributeRepository(3-99)packages/app-store/routing-forms/types/types.d.ts (1)
FormResponse(19-27)packages/lib/raqb/zod.ts (1)
raqbQueryValueSchema(120-133)packages/lib/CalendarManager.ts (1)
getBusyCalendarTimes(216-278)packages/lib/date-ranges.ts (1)
mergeOverlappingRanges(449-467)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
packages/lib/bookings/filterHostsByLeadThreshold.test.ts (1)
packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
packages/lib/server/getLuckyUser.integration-test.ts (1)
packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (1)
packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
packages/lib/server/getLuckyUser.test.ts (1)
packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
packages/lib/bookings/filterHostsByLeadThreshold.ts (2)
packages/lib/server/getLuckyUser.ts (1)
LuckyUserService(195-1135)packages/lib/di/containers/LuckyUser.ts (1)
getLuckyUserService(23-25)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Production builds / Build Web App
🔇 Additional comments (22)
apps/api/v2/package.json (1)
41-41: Verify Release Notes and Surface Compatibility for @calcom/platform-libraries 0.0.330
- Confirmed that both dependencies and peerDependencies for 0.0.330 match those in 0.0.321 (only
@calcom/liband@calcom/features)—no new deps introduced.- No CHANGELOG.md is packaged in either version—please review the GitHub release notes or commit diff between tags v0.0.321…v0.0.330 to catch any undocumented breaking changes.
- A quick export scan did not reveal renamed or removed repository/service exports, but DI surfaces can be subtle—manually sanity-check any custom bindings or overrides in your code.
packages/lib/server/repository/user.ts (1)
1032-1069: Query shape is sound and uses select correctlyFilters Accepted bookings, excludes no-shows, and returns only the latest createdAt per user. Complies with “select only what you need.”
packages/platform/libraries/bookings.ts (1)
1-2: Re-export looks good; aligns with the new DI-based service usage.No functional risk here. This centralizes access to LuckyUserService via the platform library.
packages/lib/di/modules/Attribute.ts (1)
1-9: Attribute module wiring verified
I confirmed thatattributeRepositoryModuleis imported inpackages/lib/di/containers/LuckyUser.ts(line 7) and loaded viacontainer.load(DI_TOKENS.ATTRIBUTE_REPOSITORY_MODULE, attributeRepositoryModule);(line 20), ensuring the token lookup will succeed at runtime.
packages/lib/di/modules/Host.ts (2)
6-7: Host repository DI binding looks correct.HostRepository performs updates as well, so PRISMA_CLIENT binding is appropriate.
6-7: Host repository module registration confirmedThe
hostRepositoryModuleis correctly defined and bound inpackages/lib/di/modules/Host.tsand explicitly loaded into the LuckyUser container:
- In packages/lib/di/modules/Host.ts –
export const hostRepositoryModule = createModule();on lines 6–7- In packages/lib/di/containers/LuckyUser.ts –
container.load(DI_TOKENS.HOST_REPOSITORY_MODULE, hostRepositoryModule);on line 17No further action is needed.
packages/features/ee/round-robin/roundRobinReassignment.ts (1)
23-23: Good: switched to DI entry point for LuckyUserService.This keeps parity with the broader refactor and simplifies testing.
packages/features/bookings/lib/handleNewBooking.ts (2)
57-57: Import via DI container is correct.Keeps LuckyUser lookup consistent with the service-based approach throughout the codebase.
906-925: ✅ No directgetLuckyUser(...)calls remain
All occurrences ofgetLuckyUserin the codebase are invoked via the DI-provided service (getLuckyUserService().getLuckyUser(...)), confirming that there are no legacy free-function calls bypassing the service API.• packages/features/bookings/lib/handleNewBooking.ts – uses
getLuckyUserService().getLuckyUser
• packages/features/ee/round-robin/roundRobinReassignment.ts – usesgetLuckyUserService().getLuckyUser
• All integration tests and unit tests reference the service method, not a standalone functionpackages/platform/libraries/repositories.ts (2)
1-1: Re-export looks good and keeps platform-libraries’ surface consistent.Adding
PrismaAttributeRepositoryhere matches the “Prisma*Repository” naming and avoids default exports. No issues spotted.
12-12: Alias for Host repository is appropriate and improves consistency.Re-exporting
HostRepositoryasPrismaHostRepositoryaligns API v2 expectations with platform-libraries. Nice touch.apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (1)
1-12: Prisma query shape reminder (for future changes inside the base repository).This class delegates to a base repository that performs Prisma queries. When adding or modifying queries, follow our rule: select only needed fields; avoid
include. No action needed now—just a heads-up for future edits.packages/lib/di/modules/LuckyUser.ts (2)
6-13: Binding map matches constructor contract; DI wiring looks correct.The token-to-constructor-arg map aligns with
LuckyUserService’s deps. This will enable clean, isolated testing and simpler composition.
4-4: No currentLuckyUserServiceambiguity
I searched for imports ofLuckyUserServiceacross bothpackagesandappsand found only the single occurrence inpackages/lib/di/modules/LuckyUser.ts. There is no second class namedLuckyUserServicebeing imported in this codebase, so aliasing isn’t necessary at this time. If in the future a second definition enters scope alongside the existing one, then introducing distinct names (e.g.BaseLuckyUserServicevs.ApiLuckyUserService) would be worthwhile.Likely an incorrect or invalid review comment.
packages/lib/di/tokens.ts (1)
48-53: New DI tokens look good and follow existing naming.The symbols are unique and consistent with the rest of the container. No collisions spotted.
packages/lib/server/getLuckyUser.integration-test.ts (1)
3-7: Switching tests to DI-backed LuckyUserService is the right move.
- Centralizing calls through the service increases fidelity with production usage and keeps tests aligned with real wiring.
- Minor: if LuckyUserService gains internal state later, consider instantiating it in a beforeEach and/or exposing a reset hook from the DI container to guarantee isolation across tests.
Would you like a quick helper to reset the DI container between tests if needed?
apps/api/v2/src/lib/services/lucky-user.service.ts (1)
8-27: NestJS wrapper cleanly delegates to the base service.Constructor wiring is correct and keeps responsibilities in the platform library. Good separation.
packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (3)
13-14: DI-based LuckyUserService import is correct.This aligns the route with the new service surface and central DI. Nice.
272-301: Service call integration LGTM.
- Good perf timing around the call.
- Inputs (availableUsers, eventType, allRRHosts, routingFormResponse) match the service API.
319-324: User mapping is safe (no credentials or secrets exposed).Only id, name, email are returned. This complies with “do not expose credential.key” guidance.
packages/lib/server/getLuckyUser.test.ts (2)
8-15: Tests now exercise the DI-backed service surface—good alignment.This keeps unit tests closer to production behavior. Minor: if DI wiring relies on a real Prisma client by default, ensure the DI container in test env is bound to prismaMock.
Do you want me to check the DI container bindings in packages/lib/di/containers/LuckyUser.ts to ensure it uses the mocked Prisma in tests?
12-15: Interval helper tests: nice coverage.Importing and validating getIntervalStartDate/getIntervalEndDate against mocked system time is clear and deterministic.
Also applies to: 1511-1575
| import { PrismaWriteService } from "@/modules/prisma/prisma-write.service"; | ||
| import { Injectable } from "@nestjs/common"; | ||
|
|
||
| import { PrismaHostRepository as PrismaHostRepositoryLib } from "@calcom/platform-libraries/repositories"; | ||
| import { PrismaClient } from "@calcom/prisma"; | ||
|
|
||
| @Injectable() | ||
| export class PrismaHostRepository extends PrismaHostRepositoryLib { | ||
| constructor(private readonly dbWrite: PrismaWriteService) { | ||
| super(dbWrite.prisma as unknown as PrismaClient); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm naming convention for new repository files.
Guidelines say to avoid “.repository.ts” for new files, while learnings indicate repository classes must carry a Repository suffix (class name). This file follows the latter and NestJS ecosystem norms. Please confirm which rule is canonical for this codebase going forward to avoid drift.
🏁 Script executed:
#!/bin/bash
# Inspect prevalent patterns to inform the decision
rg -nP --type=ts -C2 '(class\s+\w+Repository\b|\.repository\.ts\b)' packages apps | sed -n '1,200p'Length of output: 14544
Enforce consistent repository file naming across the codebase
Our established conventions require that repository files be named in PascalCase to match their exported class and avoid dot-suffixes like .repository.ts for new files. In the packages/lib/server/repository folder, new files uniformly use names such as PrismaAppRepository.ts (not prisma-app.repository.ts) to mirror their export class PrismaAppRepository definitions—no file uses a kebab-case or dot-suffix pattern .
To align this new API module with those conventions, please:
- Rename
apps/api/v2/src/lib/repositories/prisma-host.repository.ts
to
apps/api/v2/src/lib/repositories/PrismaHostRepository.ts - Update all imports accordingly, for example:
// before import { PrismaHostRepository } from "./prisma-host.repository"; // after import { PrismaHostRepository } from "./PrismaHostRepository";
This ensures file names are immediately discoverable and consistent with class names across the entire repo.
🤖 Prompt for AI Agents
In apps/api/v2/src/lib/repositories/prisma-host.repository.ts (lines 1-12), the
file name uses a kebab/dot-suffix style that doesn’t match our PascalCase
convention; rename the file to
apps/api/v2/src/lib/repositories/PrismaHostRepository.ts and update every import
site to reference "./PrismaHostRepository" instead of "./prisma-host.repository"
(search the repo for occurrences and replace), ensuring the exported class name
remains PrismaHostRepository and that any build or lint paths are updated
accordingly.
| function isFullDayEvent(date1: Date, date2: Date) { | ||
| const MILLISECONDS_IN_A_DAY = 24 * 60 * 60 * 1000; | ||
| const difference = Math.abs(date1.getTime() - date2.getTime()); | ||
|
|
||
| return leastRecentlyBookedUser; | ||
| if (difference % MILLISECONDS_IN_A_DAY === 0) return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Full-day detection is DST-unsafe
Using a constant 24h day length misclassifies full-day events across DST boundaries (23h/25h days). This can corrupt OOO calibration.
Replace with timezone-aware, boundary-based detection:
-function isFullDayEvent(date1: Date, date2: Date) {
- const MILLISECONDS_IN_A_DAY = 24 * 60 * 60 * 1000;
- const difference = Math.abs(date1.getTime() - date2.getTime());
- if (difference % MILLISECONDS_IN_A_DAY === 0) return true;
-}
+function isFullDayEventInTZ(start: Date, end: Date, timeZone: string) {
+ // Full-day if starts at local midnight and ends at local midnight of a later day (exclusive end)
+ const s = dayjs.tz(start, timeZone);
+ const e = dayjs.tz(end, timeZone);
+ const startsAtMidnight = s.millisecond() === 0 && s.second() === 0 && s.minute() === 0 && s.hour() === 0;
+ const endsAtMidnight = e.millisecond() === 0 && e.second() === 0 && e.minute() === 0 && e.hour() === 0;
+ return startsAtMidnight && endsAtMidnight && e.isAfter(s, "day");
+}And pass busyTime.timeZone at the call site (see next comment).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/lib/server/getLuckyUser.ts around lines 157 to 162, the
isFullDayEvent function currently uses a fixed 24-hour millisecond constant
which fails across DST transitions; replace this with a timezone-aware,
boundary-based check that converts the two Date values to local date boundaries
in the provided timeZone (e.g., compute the start-of-day for each date in the
event timeZone and verify the event spans exactly whole-day boundaries or that
the start equals the start-of-day and the end equals the next start-of-day), and
update the call site to pass busyTime.timeZone into isFullDayEvent so the
function can perform its DST-safe comparisons.
| const oooCalibration = new Map<number, number>(); | ||
|
|
||
| return hosts.map((host) => ({ | ||
| ...host, | ||
| calibration: | ||
| (newHostsWithCalibration.get(host.userId)?.calibration ?? 0) + (oooCalibration.get(host.userId) ?? 0), | ||
| })); | ||
| } | ||
| oooData.forEach(({ userId, oooEntries }) => { | ||
| let calibration = 0; | ||
|
|
||
| function getUsersWithHighestPriority<T extends PartialUser & { priority?: number | null }>({ | ||
| availableUsers, | ||
| }: { | ||
| availableUsers: T[]; | ||
| }) { | ||
| const highestPriority = Math.max(...availableUsers.map((user) => user.priority ?? 2)); | ||
| const usersWithHighestPriority = availableUsers.filter( | ||
| (user) => user.priority === highestPriority || (user.priority == null && highestPriority === 2) | ||
| ); | ||
| if (!isNonEmptyArray(usersWithHighestPriority)) { | ||
| throw new Error("Internal Error: Highest Priority filter should never return length=0."); | ||
| } | ||
| oooEntries.forEach((oooEntry) => { | ||
| const bookingsInTimeframe = existingBookings.filter( | ||
| (booking) => | ||
| booking.createdAt >= oooEntry.start && | ||
| booking.createdAt <= oooEntry.end && | ||
| booking.userId !== userId | ||
| ); | ||
| calibration += bookingsInTimeframe.length / (hosts.length - 1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Divide-by-zero when only one host exists
bookingsInTimeframe.length / (hosts.length - 1) will divide by zero if there’s only one host, producing Infinity and corrupting downstream weight/shortfall math.
Apply guard:
- calibration += bookingsInTimeframe.length / (hosts.length - 1);
+ if (hosts.length > 1) {
+ calibration += bookingsInTimeframe.length / (hosts.length - 1);
+ }Also consider early-exiting getHostsWithCalibration with zero calibrations when hosts.length <= 1.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const oooCalibration = new Map<number, number>(); | |
| return hosts.map((host) => ({ | |
| ...host, | |
| calibration: | |
| (newHostsWithCalibration.get(host.userId)?.calibration ?? 0) + (oooCalibration.get(host.userId) ?? 0), | |
| })); | |
| } | |
| oooData.forEach(({ userId, oooEntries }) => { | |
| let calibration = 0; | |
| function getUsersWithHighestPriority<T extends PartialUser & { priority?: number | null }>({ | |
| availableUsers, | |
| }: { | |
| availableUsers: T[]; | |
| }) { | |
| const highestPriority = Math.max(...availableUsers.map((user) => user.priority ?? 2)); | |
| const usersWithHighestPriority = availableUsers.filter( | |
| (user) => user.priority === highestPriority || (user.priority == null && highestPriority === 2) | |
| ); | |
| if (!isNonEmptyArray(usersWithHighestPriority)) { | |
| throw new Error("Internal Error: Highest Priority filter should never return length=0."); | |
| } | |
| oooEntries.forEach((oooEntry) => { | |
| const bookingsInTimeframe = existingBookings.filter( | |
| (booking) => | |
| booking.createdAt >= oooEntry.start && | |
| booking.createdAt <= oooEntry.end && | |
| booking.userId !== userId | |
| ); | |
| calibration += bookingsInTimeframe.length / (hosts.length - 1); | |
| }); | |
| const oooCalibration = new Map<number, number>(); | |
| oooData.forEach(({ userId, oooEntries }) => { | |
| let calibration = 0; | |
| oooEntries.forEach((oooEntry) => { | |
| const bookingsInTimeframe = existingBookings.filter( | |
| (booking) => | |
| booking.createdAt >= oooEntry.start && | |
| booking.createdAt <= oooEntry.end && | |
| booking.userId !== userId | |
| ); | |
| if (hosts.length > 1) { | |
| calibration += bookingsInTimeframe.length / (hosts.length - 1); | |
| } | |
| }); | |
| // …rest of logic that uses `calibration` | |
| }); |
🤖 Prompt for AI Agents
In packages/lib/server/getLuckyUser.ts around lines 303 to 317, the computation
calibration += bookingsInTimeframe.length / (hosts.length - 1) can divide by
zero when hosts.length === 1; change the logic to guard the division (e.g. only
perform the division when hosts.length > 1, or use a safe denominator like
Math.max(1, hosts.length - 1)), and also add an early exit in
getHostsWithCalibration that returns zero calibrations (or an empty/neutral
result) when hosts.length <= 1 to avoid corrupting downstream weight/shortfall
math.
| async findUserOOODays({ userId, dateTo, dateFrom }: { userId: number; dateTo: string; dateFrom: string }) { | ||
| return this.prismaClient.outOfOfficeEntry.findMany({ | ||
| where: { | ||
| userId, | ||
| OR: [ | ||
| // outside of range | ||
| // (start <= 'dateTo' AND end >= 'dateFrom') | ||
| { | ||
| start: { | ||
| lte: dateTo, | ||
| }, | ||
| end: { | ||
| gte: dateFrom, | ||
| }, | ||
| }, | ||
| // start is between dateFrom and dateTo but end is outside of range | ||
| // (start <= 'dateTo' AND end >= 'dateTo') | ||
| { | ||
| start: { | ||
| lte: dateTo, | ||
| }, | ||
|
|
||
| end: { | ||
| gte: dateTo, | ||
| }, | ||
| }, | ||
| // end is between dateFrom and dateTo but start is outside of range | ||
| // (start <= 'dateFrom' OR end <= 'dateTo') | ||
| { | ||
| start: { | ||
| lte: dateFrom, | ||
| }, | ||
|
|
||
| end: { | ||
| lte: dateTo, | ||
| }, | ||
| }, | ||
| ], | ||
| where: { | ||
| userId, | ||
| OR: [ | ||
| // outside of range | ||
| // (start <= 'dateTo' AND end >= 'dateFrom') | ||
| { | ||
| start: { | ||
| lte: dateTo, | ||
| }, | ||
| end: { | ||
| gte: dateFrom, | ||
| }, | ||
| }, | ||
| // start is between dateFrom and dateTo but end is outside of range | ||
| // (start <= 'dateTo' AND end >= 'dateTo') | ||
| { | ||
| start: { | ||
| lte: dateTo, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| start: true, | ||
| end: true, | ||
| user: { | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| }, | ||
| }, | ||
| toUser: { | ||
| select: { | ||
| id: true, | ||
| username: true, | ||
| name: true, | ||
| }, | ||
| }, | ||
| reason: { | ||
| select: { | ||
| id: true, | ||
| emoji: true, | ||
| reason: true, | ||
| }, | ||
| }, | ||
|
|
||
| end: { | ||
| gte: dateTo, | ||
| }, | ||
| }); | ||
| }, | ||
| // end is between dateFrom and dateTo but start is outside of range | ||
| // (start <= 'dateFrom' OR end <= 'dateTo') | ||
| { | ||
| start: { | ||
| lte: dateFrom, | ||
| }, | ||
|
|
||
| end: { | ||
| lte: dateTo, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| select: { | ||
| id: true, | ||
| start: true, | ||
| end: true, | ||
| user: { | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| }, | ||
| }, | ||
| toUser: { | ||
| select: { | ||
| id: true, | ||
| username: true, | ||
| name: true, | ||
| }, | ||
| }, | ||
| reason: { | ||
| select: { | ||
| id: true, | ||
| emoji: true, | ||
| reason: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify overlap logic and use Date types to avoid timezone/string pitfalls
The triple-OR can return false positives and is harder to reason about. Use the canonical overlap predicate and accept Date inputs for consistent comparisons (align with findOOOEntriesInInterval).
- async findUserOOODays({ userId, dateTo, dateFrom }: { userId: number; dateTo: string; dateFrom: string }) {
- return this.prismaClient.outOfOfficeEntry.findMany({
- where: {
- userId,
- OR: [
- {
- start: { lte: dateTo },
- end: { gte: dateFrom },
- },
- {
- start: { lte: dateTo },
- end: { gte: dateTo },
- },
- {
- start: { lte: dateFrom },
- end: { lte: dateTo },
- },
- ],
- },
+ async findUserOOODays({ userId, dateTo, dateFrom }: { userId: number; dateTo: Date; dateFrom: Date }) {
+ return this.prismaClient.outOfOfficeEntry.findMany({
+ where: {
+ userId,
+ // canonical interval overlap: [start, end] intersects [dateFrom, dateTo]
+ start: { lte: dateTo },
+ end: { gte: dateFrom },
+ },
select: {
id: true,
start: true,
end: true,
user: { select: { id: true, name: true } },
toUser: { select: { id: true, username: true, name: true } },
reason: { select: { id: true, emoji: true, reason: true } },
},
});
}If callers must pass strings, coerce them once at the boundary and keep repository APIs strongly typed with Date.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async findUserOOODays({ userId, dateTo, dateFrom }: { userId: number; dateTo: string; dateFrom: string }) { | |
| return this.prismaClient.outOfOfficeEntry.findMany({ | |
| where: { | |
| userId, | |
| OR: [ | |
| // outside of range | |
| // (start <= 'dateTo' AND end >= 'dateFrom') | |
| { | |
| start: { | |
| lte: dateTo, | |
| }, | |
| end: { | |
| gte: dateFrom, | |
| }, | |
| }, | |
| // start is between dateFrom and dateTo but end is outside of range | |
| // (start <= 'dateTo' AND end >= 'dateTo') | |
| { | |
| start: { | |
| lte: dateTo, | |
| }, | |
| end: { | |
| gte: dateTo, | |
| }, | |
| }, | |
| // end is between dateFrom and dateTo but start is outside of range | |
| // (start <= 'dateFrom' OR end <= 'dateTo') | |
| { | |
| start: { | |
| lte: dateFrom, | |
| }, | |
| end: { | |
| lte: dateTo, | |
| }, | |
| }, | |
| ], | |
| where: { | |
| userId, | |
| OR: [ | |
| // outside of range | |
| // (start <= 'dateTo' AND end >= 'dateFrom') | |
| { | |
| start: { | |
| lte: dateTo, | |
| }, | |
| end: { | |
| gte: dateFrom, | |
| }, | |
| }, | |
| // start is between dateFrom and dateTo but end is outside of range | |
| // (start <= 'dateTo' AND end >= 'dateTo') | |
| { | |
| start: { | |
| lte: dateTo, | |
| }, | |
| select: { | |
| id: true, | |
| start: true, | |
| end: true, | |
| user: { | |
| select: { | |
| id: true, | |
| name: true, | |
| }, | |
| }, | |
| toUser: { | |
| select: { | |
| id: true, | |
| username: true, | |
| name: true, | |
| }, | |
| }, | |
| reason: { | |
| select: { | |
| id: true, | |
| emoji: true, | |
| reason: true, | |
| }, | |
| }, | |
| end: { | |
| gte: dateTo, | |
| }, | |
| }); | |
| }, | |
| // end is between dateFrom and dateTo but start is outside of range | |
| // (start <= 'dateFrom' OR end <= 'dateTo') | |
| { | |
| start: { | |
| lte: dateFrom, | |
| }, | |
| end: { | |
| lte: dateTo, | |
| }, | |
| }, | |
| ], | |
| }, | |
| select: { | |
| id: true, | |
| start: true, | |
| end: true, | |
| user: { | |
| select: { | |
| id: true, | |
| name: true, | |
| }, | |
| }, | |
| toUser: { | |
| select: { | |
| id: true, | |
| username: true, | |
| name: true, | |
| }, | |
| }, | |
| reason: { | |
| select: { | |
| id: true, | |
| emoji: true, | |
| reason: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } | |
| async findUserOOODays({ userId, dateTo, dateFrom }: { userId: number; dateTo: Date; dateFrom: Date }) { | |
| return this.prismaClient.outOfOfficeEntry.findMany({ | |
| where: { | |
| userId, | |
| // canonical interval overlap: [start, end] intersects [dateFrom, dateTo] | |
| start: { lte: dateTo }, | |
| end: { gte: dateFrom }, | |
| }, | |
| select: { | |
| id: true, | |
| start: true, | |
| end: true, | |
| user: { select: { id: true, name: true } }, | |
| toUser: { select: { id: true, username: true, name: true } }, | |
| reason: { select: { id: true, emoji: true, reason: true } }, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/server/repository/ooo.ts around lines 83-148, replace the
fragile triple-OR interval logic with the canonical interval-overlap predicate
and use Date-typed inputs: change the method signature to accept Date objects
(userId: number, dateFrom: Date, dateTo: Date), simplify the Prisma where clause
to a single condition that checks start <= dateTo AND end >= dateFrom (i.e. the
canonical overlap), remove the three OR branches, and update any callers to
coerce incoming string dates to Date at the API/boundary layer (not inside this
repository) so comparisons are done with Date objects and Prisma date fields.
| async findUniqueWithWeights({ | ||
| teamId, | ||
| attributeId, | ||
| isWeightsEnabled = true, | ||
| }: { | ||
| teamId: number; | ||
| attributeId: string; | ||
| isWeightsEnabled?: boolean; | ||
| }) { | ||
| return await this.prismaClient.attribute.findUnique({ | ||
| where: { | ||
| id: attributeId, | ||
| teamId, | ||
| isWeightsEnabled, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| slug: true, | ||
| type: true, | ||
| options: { | ||
| select: { | ||
| id: true, | ||
| value: true, | ||
| slug: true, | ||
| assignedUsers: { | ||
| select: { | ||
| member: { | ||
| select: { | ||
| userId: true, | ||
| }, | ||
| }, | ||
| weight: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
findUnique where-clause is invalid; use findFirst (or narrow to unique field only)
Prisma findUnique only accepts unique/composite-unique fields. Adding teamId and isWeightsEnabled to where alongside id will not type-check and won’t execute. Use findFirst with all filters, or use findUnique({ where: { id } }) and validate tenant flags in code.
- return await this.prismaClient.attribute.findUnique({
- where: {
- id: attributeId,
- teamId,
- isWeightsEnabled,
- },
+ return await this.prismaClient.attribute.findFirst({
+ where: {
+ id: attributeId,
+ teamId,
+ isWeightsEnabled,
+ },
select: {
id: true,
name: true,
slug: true,
type: true,
options: {
select: {
id: true,
value: true,
slug: true,
assignedUsers: {
select: {
member: {
select: {
userId: true,
},
},
weight: true,
},
},
},
},
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async findUniqueWithWeights({ | |
| teamId, | |
| attributeId, | |
| isWeightsEnabled = true, | |
| }: { | |
| teamId: number; | |
| attributeId: string; | |
| isWeightsEnabled?: boolean; | |
| }) { | |
| return await this.prismaClient.attribute.findUnique({ | |
| where: { | |
| id: attributeId, | |
| teamId, | |
| isWeightsEnabled, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| slug: true, | |
| type: true, | |
| options: { | |
| select: { | |
| id: true, | |
| value: true, | |
| slug: true, | |
| assignedUsers: { | |
| select: { | |
| member: { | |
| select: { | |
| userId: true, | |
| }, | |
| }, | |
| weight: true, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } | |
| async findUniqueWithWeights({ | |
| teamId, | |
| attributeId, | |
| isWeightsEnabled = true, | |
| }: { | |
| teamId: number; | |
| attributeId: string; | |
| isWeightsEnabled?: boolean; | |
| }) { | |
| return await this.prismaClient.attribute.findFirst({ | |
| where: { | |
| id: attributeId, | |
| teamId, | |
| isWeightsEnabled, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| slug: true, | |
| type: true, | |
| options: { | |
| select: { | |
| id: true, | |
| value: true, | |
| slug: true, | |
| assignedUsers: { | |
| select: { | |
| member: { | |
| select: { | |
| userId: true, | |
| }, | |
| }, | |
| weight: true, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/server/repository/PrismaAttributeRepository.ts around lines 59
to 98, the current findUnique call uses non-unique fields (teamId and
isWeightsEnabled) in the where clause which Prisma disallows; replace this with
findFirst using the same filters (where: { id: attributeId, teamId,
isWeightsEnabled }) to allow multiple-field filtering, or alternatively call
findUnique({ where: { id: attributeId } }) and then enforce/validate teamId and
isWeightsEnabled in code after fetching; update the method to use one of these
approaches and keep the same select projection.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (11)
45-70: Stubbing email prototypes globally is fine; add restoration to avoid cross-spec leakageThese spies persist for the whole runtime. To keep the test suite hermetic and prevent bleed-over into other specs, restore all mocks in
afterAll.Apply this diff in the teardown block (see also my separate teardown comment for related cleanup):
afterAll(async () => { - // Clean up database records + // Clean up mocks & database records + jest.restoreAllMocks();
71-78: Include oauthClientId in EmailSetup for proper teardownYou’re creating per-env OAuth clients but never delete them. Expose the created client id in
EmailSetupso you can delete it inafterAll.type EmailSetup = { team: Team; member1: User; member2: User; collectiveEventType: { id: number }; roundRobinEventType: { id: number }; + oauthClientId: string; };
100-104: Strengthen the type guard to actually protect uses of uid/hostsCurrent guard only checks presence of
id, but tests immediately access.uid(and sometimes.hosts). Make the guard validate those keys so future changes don’t cause silent runtime errors.-// Utility function to check response data type -const responseDataIsBooking = (data: any): data is BookingOutput_2024_08_13 => { - return !Array.isArray(data) && typeof data === "object" && data && "id" in data; -}; +// Utility function to check response data type +const responseDataIsBooking = (data: unknown): data is BookingOutput_2024_08_13 => { + if (!data || Array.isArray(data) || typeof data !== "object") return false; + return "id" in data && "uid" in data && "hosts" in data; +};Optionally, switch to an assertion helper to eliminate downstream casts.
146-184: Return oauthClientId to enable deletion; minor nit: flatten eventType idsYou already create per-env OAuth clients and teams. Return the client id for teardown, and consider flattening
collectiveEventType/roundRobinEventTypeto plain ids to avoid.idnoise.-async function setupTestEnvironment(emailsEnabled: boolean): Promise<EmailSetup> { +async function setupTestEnvironment(emailsEnabled: boolean): Promise<EmailSetup> { const oAuthClient = await createOAuthClient(organization.id, emailsEnabled); @@ const collectiveEvent = await createEventType("COLLECTIVE", team.id, [member1.id, member2.id]); const roundRobinEvent = await createEventType("ROUND_ROBIN", team.id, [member1.id, member2.id]); return { team, member1, member2, collectiveEventType: { id: collectiveEvent.id }, roundRobinEventType: { id: roundRobinEvent.id }, + oauthClientId: oAuthClient.id, }; }If you want to flatten ids (optional), rename the two fields and update callsites.
186-205: Timezone choice could introduce DST-related flakinessUsing "Europe/Rome" in schedules is realistic but can trigger DST edge cases around rescheduling windows depending on server-side logic. If you’ve seen flakiness around spring/fall transitions, consider using "UTC" for test schedules.
207-227: Host assignment logic reads well; verify collective duplicationFor COLLECTIVE, you both set
assignAllTeamMembersand createhostsentries. If the backend interpretsassignAllTeamMembersas implicit host assignment, this could duplicate entries. If that’s not the case in your repo, ignore; otherwise, gate host creation by type.Would you like me to add a guard so hosts are only created for ROUND_ROBIN?
306-378: Add a success-status assertion on cancel for parity with the other testThe first disabled-emails test checks
status === SUCCESS; this one doesn’t. Add the same assertion for symmetry and better signal.expect(cancelResponse.status).toBe(200); + expect(cancelResponse.body.status).toEqual(SUCCESS_STATUS); expect(AttendeeCancelledEmail.prototype.getHtml).not.toHaveBeenCalled();
382-432: Clear mocks before reschedule to isolate expectationsIn the emails-enabled collective flow, reschedule assertions can be polluted by prior "scheduled" calls. Clear mocks before step 2, like you did in the round-robin test.
// --- 2. Reschedule Event --- + jest.clearAllMocks(); const rescheduleBody: RescheduleBookingInput_2024_08_13 = {Optionally, use
toHaveBeenCalledTimesto ensure only expected prototypes were invoked per step.
433-520: Round-robin — great coverage; consider stricter counts and host assertionsThe flow is comprehensive (book, reschedule, manual reassign, auto reassign, cancel) and exercises the "lucky user" path. Two small improvements:
- Use
toHaveBeenCalledTimes(1)in steps afterjest.clearAllMocks()to guard against accidental extra sends.- Optionally assert that the host changed where you expect (e.g., after auto reassign), if the LuckyUserService is deterministic under these fixtures. If not deterministic, keep as-is.
If LuckyUserService selection is deterministic given two equal hosts, do you want me to add explicit host-id assertions?
347-367: Manual reassign branch covers both organizer-changed and no-op casesGood defensive assertions. Consider explicitly asserting "no OrganizerReassignedEmail" on manual reassign to document behavior difference vs auto reassign.
expect(manualReassignResponse.status).toBe(200); expect(OrganizerScheduledEmail.prototype.getHtml).toHaveBeenCalled(); expect(AttendeeCancelledEmail.prototype.getHtml).not.toHaveBeenCalled(); +expect(OrganizerReassignedEmail.prototype.getHtml).not.toHaveBeenCalled();
360-366: Auto reassign exercises LuckyUserService pathGood to see
OrganizerReassignedEmailasserted here. This is the key e2e signal that the new service is wired. Consider one targeted unit/integration test for LuckyUserService itself to validate selection logic deterministically (weights, last booking, OOO), leaving e2e to focus on wiring and side-effects.I can scaffold a focused test suite for LuckyUserService using injected fakes if you’d like.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
🧬 Code graph analysis (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (6)
packages/platform/types/bookings/2024-08-13/outputs/booking.output.ts (1)
BookingOutput_2024_08_13(280-306)apps/api/v2/test/utils/randomString.ts (1)
randomString(3-6)apps/api/v2/src/app.ts (1)
bootstrap(27-89)packages/platform/constants/api.ts (3)
CAL_API_VERSION_HEADER(72-72)VERSION_2024_08_13(59-59)SUCCESS_STATUS(9-9)packages/platform/libraries/emails.ts (8)
AttendeeScheduledEmail(19-19)OrganizerScheduledEmail(21-21)AttendeeRescheduledEmail(33-33)OrganizerRescheduledEmail(31-31)AttendeeCancelledEmail(25-25)OrganizerCancelledEmail(27-27)AttendeeUpdatedEmail(35-35)OrganizerReassignedEmail(29-29)packages/platform/types/bookings/2024-08-13/inputs/reschedule-booking.input.ts (1)
RescheduleBookingInput_2024_08_13(8-30)
🔇 Additional comments (5)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (5)
118-127: Good fixture/service bootstrapInitialization is clear and minimal. Overriding
PermissionsGuardkeeps the tests focused on bookings/email behavior.
128-134: Two-environment setup is a solid simplificationSplitting enabled/disabled email environments removes a lot of fixture noise and keeps signal high.
248-305: Collective — emails disabled flow looks solidEnd-to-end asserts cover booking, reschedule, and cancellation with negative email checks. Clear and deterministic.
498-508: Nice touch: clearing mocks between manual and auto reassignThis keeps email expectations sharp across the two reassign variants.
14-14: Unify Prisma Client Type Imports forUserandTeamPlease replace all imports of
Userfrom the default Prisma client with the one from our Cal.com–generated client to avoid mixing two different Prisma client packages.• File:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
– Lines 14 and 43–44-import { User } from "@prisma/client"; +import { User } from "@calcom/prisma/client";Verify availability of
Userin@calcom/prisma/client
Run this against your localnode_modulesto confirm thatUseris indeed exported:#!/bin/bash # Look for an export of User in the Cal.com Prisma client definitions rg -nP '\bexport\s+(?:type|interface|class)\s+User\b' -C2 node_modules/@calcom/prisma/client/index.d.tsOnce confirmed, update both occurrences in this spec file.
| afterAll(async () => { | ||
| // Clean up database records | ||
| await teamRepositoryFixture.delete(organization.id); | ||
| await userRepositoryFixture.deleteByEmail(authEmail); | ||
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member1.email); | ||
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member2.email); | ||
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member1.email); | ||
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member2.email); | ||
| await bookingsRepositoryFixture.deleteAllBookings( | ||
| emailsEnabledSetup.member1.id, | ||
| emailsEnabledSetup.member1.email | ||
| ); | ||
| await bookingsRepositoryFixture.deleteAllBookings( | ||
| emailsDisabledSetup.member1.id, | ||
| emailsDisabledSetup.member2.email | ||
| ); | ||
| await app.close(); | ||
| }); | ||
|
|
||
| function responseDataIsBooking(data: any): data is BookingOutput_2024_08_13 { | ||
| return !Array.isArray(data) && typeof data === "object" && data && "id" in data; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Teardown misses team(s) and OAuth clients created in setups; add deletions and restore mocks
You create two child teams (enabled/disabled) and two OAuth clients; only the parent org and users are deleted. Add deletes for child teams and OAuth clients to avoid data leakage across test runs. Also restore spies here.
afterAll(async () => {
- // Clean up database records
- await teamRepositoryFixture.delete(organization.id);
+ // Clean up mocks & database records
+ jest.restoreAllMocks();
+
+ // Delete child teams created in setups first (avoid FK constraints)
+ await teamRepositoryFixture.delete(emailsEnabledSetup.team.id);
+ await teamRepositoryFixture.delete(emailsDisabledSetup.team.id);
+
+ // Delete parent organization
+ await teamRepositoryFixture.delete(organization.id);
@@
- await app.close();
+ // If supported by fixture, also delete OAuth clients created in setups
+ // await oauthClientRepositoryFixture.deleteById(emailsEnabledSetup.oauthClientId);
+ // await oauthClientRepositoryFixture.deleteById(emailsDisabledSetup.oauthClientId);
+
+ await app.close();
});If you share the OAuthClientRepositoryFixture delete API, I can wire the exact calls for you.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterAll(async () => { | |
| // Clean up database records | |
| await teamRepositoryFixture.delete(organization.id); | |
| await userRepositoryFixture.deleteByEmail(authEmail); | |
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member1.email); | |
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member2.email); | |
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member1.email); | |
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member2.email); | |
| await bookingsRepositoryFixture.deleteAllBookings( | |
| emailsEnabledSetup.member1.id, | |
| emailsEnabledSetup.member1.email | |
| ); | |
| await bookingsRepositoryFixture.deleteAllBookings( | |
| emailsDisabledSetup.member1.id, | |
| emailsDisabledSetup.member2.email | |
| ); | |
| await app.close(); | |
| }); | |
| function responseDataIsBooking(data: any): data is BookingOutput_2024_08_13 { | |
| return !Array.isArray(data) && typeof data === "object" && data && "id" in data; | |
| } | |
| }); | |
| afterAll(async () => { | |
| - // Clean up database records | |
| // Clean up mocks & database records | |
| jest.restoreAllMocks(); | |
| // Delete child teams created in setups first (avoid FK constraints) | |
| await teamRepositoryFixture.delete(emailsEnabledSetup.team.id); | |
| await teamRepositoryFixture.delete(emailsDisabledSetup.team.id); | |
| // Delete parent organization | |
| await teamRepositoryFixture.delete(organization.id); | |
| await userRepositoryFixture.deleteByEmail(authEmail); | |
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member1.email); | |
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member2.email); | |
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member1.email); | |
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member2.email); | |
| @@ | |
| // If supported by fixture, also delete OAuth clients created in setups | |
| // await oauthClientRepositoryFixture.deleteById(emailsEnabledSetup.oauthClientId); | |
| // await oauthClientRepositoryFixture.deleteById(emailsDisabledSetup.oauthClientId); | |
| await app.close(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
around lines 523 to 533, the afterAll teardown only deletes the parent
organization and users but omits the two child teams (enabled/disabled) and the
two OAuth clients created in the setups and does not restore spies; update the
teardown to delete each child team via
teamRepositoryFixture.delete(childTeam.id) (or the appropriate fixture delete
API used when creating them), delete both OAuth clients via
oauthClientRepositoryFixture.delete(oauthClientId) (or the fixture's delete
API), and restore mocks/spies (e.g., call specific spy.mockRestore() or
jest.restoreAllMocks()) before closing the app so no state or created entities
leak between tests.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (1)
524-533: Teardown still misses child teams and OAuth clients; also restore spiesLeakage risk across test runs. Delete both child teams and OAuth clients created in setups, and restore spies before closing the app. Order matters: delete children first, then the parent org.
Apply this diff:
afterAll(async () => { - // Clean up database records - await teamRepositoryFixture.delete(organization.id); + // Clean up mocks & database records + jest.restoreAllMocks(); + + // Delete child teams created in setups first (avoid FK constraints) + await teamRepositoryFixture.delete(emailsEnabledSetup.team.id); + await teamRepositoryFixture.delete(emailsDisabledSetup.team.id); + + // If supported by fixture, also delete OAuth clients created in setups + // (use the correct method name provided by your fixture) + await oauthClientRepositoryFixture.deleteById?.(emailsEnabledSetup.oauthClientId); + await oauthClientRepositoryFixture.deleteById?.(emailsDisabledSetup.oauthClientId); + + // Delete parent organization + await teamRepositoryFixture.delete(organization.id); await userRepositoryFixture.deleteByEmail(authEmail); await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member1.email); await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member2.email); await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member1.email); await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member2.email); await app.close(); });If your fixture exposes a different delete API (e.g.,
delete,remove,deleteClient), swapdeleteByIdaccordingly. I can wire the exact calls once you confirm the fixture surface.
🧹 Nitpick comments (6)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (6)
100-104: Use a ‘uid’-based type guard to avoid false negativesBooking outputs are reliably addressable by
uid. The current guard checks"id" in data, which may fail ifidisn’t exposed in the API output whileuidis.- const responseDataIsBooking = (data: any): data is BookingOutput_2024_08_13 => { - return !Array.isArray(data) && typeof data === "object" && data && "id" in data; - }; + const responseDataIsBooking = (data: unknown): data is BookingOutput_2024_08_13 => { + return !!data && typeof data === "object" && !Array.isArray(data) && "uid" in (data as Record<string, unknown>); + };
45-69: Centralize email prototype spies and ensure full restorationSpies are correctly set, and
jest.clearAllMocks()is used per test, but restoration is missing until teardown. Consider moving spy setup into abeforeAllblock (or a local helper) to localize side effects to this suite. You already fixed restoration inafterAllper another comment.-// Mock all email sending prototypes -jest - .spyOn(AttendeeScheduledEmail.prototype, "getHtml") - .mockImplementation(() => Promise.resolve("<p>email</p>")); -// ... (rest of spies) +// Mock all email sending prototypes +beforeAll(() => { + jest.spyOn(AttendeeScheduledEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(OrganizerScheduledEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(AttendeeRescheduledEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(OrganizerRescheduledEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(AttendeeCancelledEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(OrganizerCancelledEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(OrganizerReassignedEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); + jest.spyOn(AttendeeUpdatedEmail.prototype, "getHtml").mockResolvedValue("<p>email</p>"); +});
417-421: Also assert API-level success for rescheduleWe assert HTTP 201 but not payload
status. Add the same check you used elsewhere for consistency.- expect(rescheduleResponse.status).toBe(201); - const rescheduledBookingUid = (rescheduleResponse.body as RescheduleBookingOutput_2024_08_13).data.uid; + expect(rescheduleResponse.status).toBe(201); + const rescheduleBodyOut = rescheduleResponse.body as RescheduleBookingOutput_2024_08_13; + expect(rescheduleBodyOut.status).toEqual(SUCCESS_STATUS); + const rescheduledBookingUid = rescheduleBodyOut.data.uid;
451-458: Mirror success-status assertion on create (emails enabled, round-robin)For symmetry with other tests, verify the
statusfield as well.- expect(createResponse.status).toBe(201); - const createResponseBody: CreateBookingOutput_2024_08_13 = createResponse.body; + expect(createResponse.status).toBe(201); + const createResponseBody: CreateBookingOutput_2024_08_13 = createResponse.body; + expect(createResponseBody.status).toEqual(SUCCESS_STATUS);
360-368: Missing assertion: OrganizerReassignedEmail should not be called when emails are disabledYou checked Scheduled/Updated, but not Reassigned. Add it to tighten the “no emails” contract for the disabled path.
expect(autoReassignResponse.status).toBe(200); + expect(OrganizerReassignedEmail.prototype.getHtml).not.toHaveBeenCalled(); expect(OrganizerReassignedEmail.prototype.getHtml).not.toHaveBeenCalled(); expect(AttendeeUpdatedEmail.prototype.getHtml).not.toHaveBeenCalled();Note: You currently assert
OrganizerReassignedEmail.prototype.getHtml).not.toHaveBeenCalled()in the enabled path; this adds the equivalent in the disabled path.
252-265: Reduce duplication for ISO date generationMinor readability nit: The
new Date(Date.UTC(...)).toISOString()pattern repeats. Consider a small helper likeutcIso(y, m, d, h, min = 0)to improve signal in tests.Example helper (outside diff, place near other helpers):
const utcIso = (y: number, m: number, d: number, h: number, min = 0) => new Date(Date.UTC(y, m, d, h, min, 0)).toISOString();Then:
start: utcIso(2030, 0, 8, 9),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
🧬 Code graph analysis (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (5)
packages/platform/types/bookings/2024-08-13/outputs/booking.output.ts (1)
BookingOutput_2024_08_13(280-306)apps/api/v2/test/utils/randomString.ts (1)
randomString(3-6)packages/platform/constants/api.ts (3)
CAL_API_VERSION_HEADER(72-72)VERSION_2024_08_13(59-59)SUCCESS_STATUS(9-9)packages/platform/libraries/emails.ts (8)
AttendeeScheduledEmail(19-19)OrganizerScheduledEmail(21-21)AttendeeRescheduledEmail(33-33)OrganizerRescheduledEmail(31-31)AttendeeCancelledEmail(25-25)OrganizerCancelledEmail(27-27)AttendeeUpdatedEmail(35-35)OrganizerReassignedEmail(29-29)packages/platform/types/bookings/2024-08-13/inputs/reschedule-booking.input.ts (1)
RescheduleBookingInput_2024_08_13(8-30)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build Docs
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Production builds / Build API v2
- GitHub Check: Production builds / Build API v1
- GitHub Check: Type check / check-types
🔇 Additional comments (5)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts (5)
118-127: LGTM: Fixtures/services wiring is clean and efficientGood use of moduleRef for fixtures and direct service retrieval. Parallelization with
Promise.alllater is also appropriate.
207-228: LGTM: Event type and hosts setup accurately reflect collective vs round-robin semantics
assignAllTeamMembersandisFixedtoggling matches expected scheduling behavior. Locations payload also looks valid for downstream flows.
245-247: LGTM: Clearing mocks per test keeps expectations isolated
jest.clearAllMocks()here plus step-level clears in the enabled path is sufficient now that teardown will restore spies.
382-433: End-to-end flow coverage is strongThe enabled-collective path exercises create, reschedule, and cancel with the right email expectations. With the added payload
statusassertions, this will be very robust.
499-509: Excellent assertions on auto-reassign in enabled round-robinVerifying OrganizerScheduled + OrganizerReassigned + AttendeeUpdated covers the core side effects for the DI-driven reassign flow.
| // Type definitions for test setup data | ||
| type EmailSetup = { | ||
| team: Team; | ||
| member1: User; | ||
| member2: User; | ||
| collectiveEventType: { | ||
| id: number; | ||
| createdBookingUid: string; | ||
| rescheduledBookingUid: string; | ||
| }; | ||
| roundRobinEventType: { | ||
| id: number; | ||
| createdBookingUid: string; | ||
| rescheduledBookingUid: string; | ||
| currentHostId: number; | ||
| }; | ||
| collectiveEventType: { id: number }; | ||
| roundRobinEventType: { id: number }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Track OAuth client IDs in EmailSetup for proper teardown
We create an OAuth client per setup but don’t retain its ID, making deletion in afterAll awkward.
Apply this diff to carry the ID:
type EmailSetup = {
team: Team;
member1: User;
member2: User;
collectiveEventType: { id: number };
roundRobinEventType: { id: number };
+ oauthClientId: string;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Type definitions for test setup data | |
| type EmailSetup = { | |
| team: Team; | |
| member1: User; | |
| member2: User; | |
| collectiveEventType: { | |
| id: number; | |
| createdBookingUid: string; | |
| rescheduledBookingUid: string; | |
| }; | |
| roundRobinEventType: { | |
| id: number; | |
| createdBookingUid: string; | |
| rescheduledBookingUid: string; | |
| currentHostId: number; | |
| }; | |
| collectiveEventType: { id: number }; | |
| roundRobinEventType: { id: number }; | |
| }; | |
| // Type definitions for test setup data | |
| type EmailSetup = { | |
| team: Team; | |
| member1: User; | |
| member2: User; | |
| collectiveEventType: { id: number }; | |
| roundRobinEventType: { id: number }; | |
| oauthClientId: string; | |
| }; |
| // Helper function to create a complete test environment | ||
| async function setupTestEnvironment(emailsEnabled: boolean): Promise<EmailSetup> { | ||
| const oAuthClient = await createOAuthClient(organization.id, emailsEnabled); | ||
| const team = await teamRepositoryFixture.create({ | ||
| name: `team-emails-team-${randomString()}`, | ||
| isOrganization: false, | ||
| parent: { connect: { id: organization.id } }, | ||
| createdByOAuthClient: { | ||
| connect: { | ||
| id: oAuthClientEmailsEnabled.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const member1 = await userRepositoryFixture.create({ | ||
| email: `team-emails-2024-08-13-member1-${randomString()}@api.com`, | ||
| platformOAuthClients: { | ||
| connect: { | ||
| id: oAuthClientEmailsEnabled.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const member2 = await userRepositoryFixture.create({ | ||
| email: `team-emails-2024-08-13-member2-${randomString()}@api.com`, | ||
| platformOAuthClients: { | ||
| connect: { | ||
| id: oAuthClientEmailsEnabled.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const userSchedule: CreateScheduleInput_2024_04_15 = { | ||
| name: `team-emails-2024-08-13-schedule-${randomString()}`, | ||
| timeZone: "Europe/Rome", | ||
| isDefault: true, | ||
| }; | ||
| await schedulesService.createUserSchedule(member1.id, userSchedule); | ||
| await schedulesService.createUserSchedule(member2.id, userSchedule); | ||
|
|
||
| await profileRepositoryFixture.create({ | ||
| uid: `usr-${member1.id}`, | ||
| username: member1.email, | ||
| organization: { | ||
| connect: { | ||
| id: organization.id, | ||
| }, | ||
| }, | ||
| user: { | ||
| connect: { | ||
| id: member1.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await profileRepositoryFixture.create({ | ||
| uid: `usr-${member2.id}`, | ||
| username: member2.email, | ||
| organization: { | ||
| connect: { | ||
| id: organization.id, | ||
| }, | ||
| }, | ||
| user: { | ||
| connect: { | ||
| id: member2.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await membershipsRepositoryFixture.create({ | ||
| role: "MEMBER", | ||
| user: { connect: { id: member1.id } }, | ||
| team: { connect: { id: team.id } }, | ||
| accepted: true, | ||
| }); | ||
|
|
||
| await membershipsRepositoryFixture.create({ | ||
| role: "MEMBER", | ||
| user: { connect: { id: member2.id } }, | ||
| team: { connect: { id: team.id } }, | ||
| accepted: true, | ||
| }); | ||
|
|
||
| const collectiveEvent = await eventTypesRepositoryFixture.createTeamEventType({ | ||
| schedulingType: "COLLECTIVE", | ||
| team: { | ||
| connect: { id: team.id }, | ||
| }, | ||
| title: `team-emails-2024-08-13-event-type-${randomString()}`, | ||
| slug: `team-emails-2024-08-13-event-type-${randomString()}`, | ||
| length: 60, | ||
| assignAllTeamMembers: true, | ||
| bookingFields: [], | ||
| locations: [], | ||
| }); | ||
|
|
||
| await hostsRepositoryFixture.create({ | ||
| isFixed: true, | ||
| user: { | ||
| connect: { | ||
| id: member1.id, | ||
| }, | ||
| }, | ||
| eventType: { | ||
| connect: { | ||
| id: collectiveEvent.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await hostsRepositoryFixture.create({ | ||
| isFixed: true, | ||
| user: { | ||
| connect: { | ||
| id: member2.id, | ||
| }, | ||
| }, | ||
| eventType: { | ||
| connect: { | ||
| id: collectiveEvent.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const roundRobinEvent = await eventTypesRepositoryFixture.createTeamEventType({ | ||
| schedulingType: "ROUND_ROBIN", | ||
| team: { | ||
| connect: { id: team.id }, | ||
| }, | ||
| title: `team-emails-2024-08-13-event-type-${randomString()}`, | ||
| slug: `team-emails-2024-08-13-event-type-${randomString()}`, | ||
| length: 60, | ||
| assignAllTeamMembers: false, | ||
| bookingFields: [], | ||
| locations: [], | ||
| }); | ||
|
|
||
| await hostsRepositoryFixture.create({ | ||
| isFixed: false, | ||
| user: { | ||
| connect: { | ||
| id: member1.id, | ||
| }, | ||
| }, | ||
| eventType: { | ||
| connect: { | ||
| id: roundRobinEvent.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await hostsRepositoryFixture.create({ | ||
| isFixed: false, | ||
| user: { | ||
| connect: { | ||
| id: member2.id, | ||
| }, | ||
| }, | ||
| eventType: { | ||
| connect: { | ||
| id: roundRobinEvent.id, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| emailsEnabledSetup = { | ||
| createdByOAuthClient: { connect: { id: oAuthClient.id } }, | ||
| }); | ||
|
|
||
| const [member1, member2] = await Promise.all([ | ||
| createTeamMember(oAuthClient.id), | ||
| createTeamMember(oAuthClient.id), | ||
| ]); | ||
|
|
||
| await Promise.all([ | ||
| membershipsRepositoryFixture.create({ | ||
| role: "MEMBER", | ||
| user: { connect: { id: member1.id } }, | ||
| team: { connect: { id: team.id } }, | ||
| accepted: true, | ||
| }), | ||
| membershipsRepositoryFixture.create({ | ||
| role: "MEMBER", | ||
| user: { connect: { id: member2.id } }, | ||
| team: { connect: { id: team.id } }, | ||
| accepted: true, | ||
| }), | ||
| ]); | ||
|
|
||
| const collectiveEvent = await createEventType("COLLECTIVE", team.id, [member1.id, member2.id]); | ||
| const roundRobinEvent = await createEventType("ROUND_ROBIN", team.id, [member1.id, member2.id]); | ||
|
|
||
| return { | ||
| team, | ||
| member1: member1, | ||
| member2: member2, | ||
| collectiveEventType: { | ||
| id: collectiveEvent.id, | ||
| createdBookingUid: "", | ||
| rescheduledBookingUid: "", | ||
| }, | ||
| roundRobinEventType: { | ||
| id: roundRobinEvent.id, | ||
| createdBookingUid: "", | ||
| rescheduledBookingUid: "", | ||
| currentHostId: 0, | ||
| }, | ||
| member1, | ||
| member2, | ||
| collectiveEventType: { id: collectiveEvent.id }, | ||
| roundRobinEventType: { id: roundRobinEvent.id }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return oauthClientId from setupTestEnvironment
Follow-up to the previous comment: include the OAuth client id in the setup return object.
async function setupTestEnvironment(emailsEnabled: boolean): Promise<EmailSetup> {
- const oAuthClient = await createOAuthClient(organization.id, emailsEnabled);
+ const oAuthClient = await createOAuthClient(organization.id, emailsEnabled);
@@
const roundRobinEvent = await createEventType("ROUND_ROBIN", team.id, [member1.id, member2.id]);
return {
team,
member1,
member2,
collectiveEventType: { id: collectiveEvent.id },
roundRobinEventType: { id: roundRobinEvent.id },
+ oauthClientId: oAuthClient.id,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Helper function to create a complete test environment | |
| async function setupTestEnvironment(emailsEnabled: boolean): Promise<EmailSetup> { | |
| const oAuthClient = await createOAuthClient(organization.id, emailsEnabled); | |
| const team = await teamRepositoryFixture.create({ | |
| name: `team-emails-team-${randomString()}`, | |
| isOrganization: false, | |
| parent: { connect: { id: organization.id } }, | |
| createdByOAuthClient: { | |
| connect: { | |
| id: oAuthClientEmailsEnabled.id, | |
| }, | |
| }, | |
| }); | |
| const member1 = await userRepositoryFixture.create({ | |
| email: `team-emails-2024-08-13-member1-${randomString()}@api.com`, | |
| platformOAuthClients: { | |
| connect: { | |
| id: oAuthClientEmailsEnabled.id, | |
| }, | |
| }, | |
| }); | |
| const member2 = await userRepositoryFixture.create({ | |
| email: `team-emails-2024-08-13-member2-${randomString()}@api.com`, | |
| platformOAuthClients: { | |
| connect: { | |
| id: oAuthClientEmailsEnabled.id, | |
| }, | |
| }, | |
| }); | |
| const userSchedule: CreateScheduleInput_2024_04_15 = { | |
| name: `team-emails-2024-08-13-schedule-${randomString()}`, | |
| timeZone: "Europe/Rome", | |
| isDefault: true, | |
| }; | |
| await schedulesService.createUserSchedule(member1.id, userSchedule); | |
| await schedulesService.createUserSchedule(member2.id, userSchedule); | |
| await profileRepositoryFixture.create({ | |
| uid: `usr-${member1.id}`, | |
| username: member1.email, | |
| organization: { | |
| connect: { | |
| id: organization.id, | |
| }, | |
| }, | |
| user: { | |
| connect: { | |
| id: member1.id, | |
| }, | |
| }, | |
| }); | |
| await profileRepositoryFixture.create({ | |
| uid: `usr-${member2.id}`, | |
| username: member2.email, | |
| organization: { | |
| connect: { | |
| id: organization.id, | |
| }, | |
| }, | |
| user: { | |
| connect: { | |
| id: member2.id, | |
| }, | |
| }, | |
| }); | |
| await membershipsRepositoryFixture.create({ | |
| role: "MEMBER", | |
| user: { connect: { id: member1.id } }, | |
| team: { connect: { id: team.id } }, | |
| accepted: true, | |
| }); | |
| await membershipsRepositoryFixture.create({ | |
| role: "MEMBER", | |
| user: { connect: { id: member2.id } }, | |
| team: { connect: { id: team.id } }, | |
| accepted: true, | |
| }); | |
| const collectiveEvent = await eventTypesRepositoryFixture.createTeamEventType({ | |
| schedulingType: "COLLECTIVE", | |
| team: { | |
| connect: { id: team.id }, | |
| }, | |
| title: `team-emails-2024-08-13-event-type-${randomString()}`, | |
| slug: `team-emails-2024-08-13-event-type-${randomString()}`, | |
| length: 60, | |
| assignAllTeamMembers: true, | |
| bookingFields: [], | |
| locations: [], | |
| }); | |
| await hostsRepositoryFixture.create({ | |
| isFixed: true, | |
| user: { | |
| connect: { | |
| id: member1.id, | |
| }, | |
| }, | |
| eventType: { | |
| connect: { | |
| id: collectiveEvent.id, | |
| }, | |
| }, | |
| }); | |
| await hostsRepositoryFixture.create({ | |
| isFixed: true, | |
| user: { | |
| connect: { | |
| id: member2.id, | |
| }, | |
| }, | |
| eventType: { | |
| connect: { | |
| id: collectiveEvent.id, | |
| }, | |
| }, | |
| }); | |
| const roundRobinEvent = await eventTypesRepositoryFixture.createTeamEventType({ | |
| schedulingType: "ROUND_ROBIN", | |
| team: { | |
| connect: { id: team.id }, | |
| }, | |
| title: `team-emails-2024-08-13-event-type-${randomString()}`, | |
| slug: `team-emails-2024-08-13-event-type-${randomString()}`, | |
| length: 60, | |
| assignAllTeamMembers: false, | |
| bookingFields: [], | |
| locations: [], | |
| }); | |
| await hostsRepositoryFixture.create({ | |
| isFixed: false, | |
| user: { | |
| connect: { | |
| id: member1.id, | |
| }, | |
| }, | |
| eventType: { | |
| connect: { | |
| id: roundRobinEvent.id, | |
| }, | |
| }, | |
| }); | |
| await hostsRepositoryFixture.create({ | |
| isFixed: false, | |
| user: { | |
| connect: { | |
| id: member2.id, | |
| }, | |
| }, | |
| eventType: { | |
| connect: { | |
| id: roundRobinEvent.id, | |
| }, | |
| }, | |
| }); | |
| emailsEnabledSetup = { | |
| createdByOAuthClient: { connect: { id: oAuthClient.id } }, | |
| }); | |
| const [member1, member2] = await Promise.all([ | |
| createTeamMember(oAuthClient.id), | |
| createTeamMember(oAuthClient.id), | |
| ]); | |
| await Promise.all([ | |
| membershipsRepositoryFixture.create({ | |
| role: "MEMBER", | |
| user: { connect: { id: member1.id } }, | |
| team: { connect: { id: team.id } }, | |
| accepted: true, | |
| }), | |
| membershipsRepositoryFixture.create({ | |
| role: "MEMBER", | |
| user: { connect: { id: member2.id } }, | |
| team: { connect: { id: team.id } }, | |
| accepted: true, | |
| }), | |
| ]); | |
| const collectiveEvent = await createEventType("COLLECTIVE", team.id, [member1.id, member2.id]); | |
| const roundRobinEvent = await createEventType("ROUND_ROBIN", team.id, [member1.id, member2.id]); | |
| return { | |
| team, | |
| member1: member1, | |
| member2: member2, | |
| collectiveEventType: { | |
| id: collectiveEvent.id, | |
| createdBookingUid: "", | |
| rescheduledBookingUid: "", | |
| }, | |
| roundRobinEventType: { | |
| id: roundRobinEvent.id, | |
| createdBookingUid: "", | |
| rescheduledBookingUid: "", | |
| currentHostId: 0, | |
| }, | |
| member1, | |
| member2, | |
| collectiveEventType: { id: collectiveEvent.id }, | |
| roundRobinEventType: { id: roundRobinEvent.id }, | |
| }; | |
| // Helper function to create a complete test environment | |
| async function setupTestEnvironment(emailsEnabled: boolean): Promise<EmailSetup> { | |
| const oAuthClient = await createOAuthClient(organization.id, emailsEnabled); | |
| const team = await teamRepositoryFixture.create({ | |
| name: `team-emails-team-${randomString()}`, | |
| parent: { connect: { id: organization.id } }, | |
| createdByOAuthClient: { connect: { id: oAuthClient.id } }, | |
| }); | |
| const [member1, member2] = await Promise.all([ | |
| createTeamMember(oAuthClient.id), | |
| createTeamMember(oAuthClient.id), | |
| ]); | |
| await Promise.all([ | |
| membershipsRepositoryFixture.create({ | |
| role: "MEMBER", | |
| user: { connect: { id: member1.id } }, | |
| team: { connect: { id: team.id } }, | |
| accepted: true, | |
| }), | |
| membershipsRepositoryFixture.create({ | |
| role: "MEMBER", | |
| user: { connect: { id: member2.id } }, | |
| team: { connect: { id: team.id } }, | |
| accepted: true, | |
| }), | |
| ]); | |
| const collectiveEvent = await createEventType("COLLECTIVE", team.id, [member1.id, member2.id]); | |
| const roundRobinEvent = await createEventType("ROUND_ROBIN", team.id, [member1.id, member2.id]); | |
| return { | |
| team, | |
| member1, | |
| member2, | |
| collectiveEventType: { id: collectiveEvent.id }, | |
| roundRobinEventType: { id: roundRobinEvent.id }, | |
| oauthClientId: oAuthClient.id, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
around lines 145 to 183, update the setupTestEnvironment function to include the
OAuth client id in its returned object: capture the created oAuthClient.id and
add it as oauthClientId (or oAuthClientId to match project naming) in the
returned EmailSetup; also update the EmailSetup type/interface definition where
it’s declared to include this id so callers and tests can access the OAuth
client id.
| afterAll(async () => { | ||
| // Clean up database records | ||
| await teamRepositoryFixture.delete(organization.id); | ||
| await userRepositoryFixture.deleteByEmail(authEmail); | ||
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member1.email); | ||
| await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member2.email); | ||
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member1.email); | ||
| await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member2.email); | ||
| await bookingsRepositoryFixture.deleteAllBookings( | ||
| emailsEnabledSetup.member1.id, | ||
| emailsEnabledSetup.member1.email | ||
| ); | ||
| await bookingsRepositoryFixture.deleteAllBookings( | ||
| emailsDisabledSetup.member1.id, | ||
| emailsDisabledSetup.member2.email | ||
| ); | ||
| await app.close(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Optional: verify fixture delete APIs before applying
If you’re unsure of the exact OAuthClient fixture delete method name, run this script and adjust the teardown accordingly.
🏁 Script executed:
#!/bin/bash
# Inspect OAuthClientRepositoryFixture to find available delete methods
rg -nC2 'class\s+OAuthClientRepositoryFixture' test/fixtures
rg -n 'deleteById|deleteClient|delete\(|remove\(' test/fixtures
# Also confirm TeamRepositoryFixture delete signature
rg -nC2 'class\s+TeamRepositoryFixture' test/fixtures
rg -n 'delete\(|remove\(' test/fixturesLength of output: 442
🏁 Script executed:
#!/bin/bash
# Search for OAuthClientRepositoryFixture and TeamRepositoryFixture class definitions across the repo
echo "Looking for OAuthClientRepositoryFixture definitions..."
rg -n 'class\s+OAuthClientRepositoryFixture' -g '*.ts'
echo "Looking for TeamRepositoryFixture definitions..."
rg -n 'class\s+TeamRepositoryFixture' -g '*.ts'
# For each found file, inspect available delete/remove methods
for file in $(rg -l 'class\s+OAuthClientRepositoryFixture' -g '*.ts'); do
echo -e "\n--- OAuthClientRepositoryFixture in $file ---"
rg -n -C2 'delete|remove' "$file"
done
for file in $(rg -l 'class\s+TeamRepositoryFixture' -g '*.ts'); do
echo -e "\n--- TeamRepositoryFixture in $file ---"
rg -n -C2 'delete|remove' "$file"
doneLength of output: 1930
The teardown is currently calling teamRepositoryFixture.delete(organization.id), but the fixture’s delete method expects a team ID—not an organization ID. To ensure the Team record is actually cleaned up, swap in the team’s ID. For example:
afterAll(async () => {
// Clean up database records
- await teamRepositoryFixture.delete(organization.id);
+ await teamRepositoryFixture.delete(team.id);
await userRepositoryFixture.deleteByEmail(authEmail);
await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member1.email);
await userRepositoryFixture.deleteByEmail(emailsEnabledSetup.member2.email);
await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member1.email);
await userRepositoryFixture.deleteByEmail(emailsDisabledSetup.member2.email);
await app.close();
});• TeamRepositoryFixture.delete signature:
async delete(teamId: Team["id"]) {
return this.prismaWriteClient.team.deleteMany({ where: { id: teamId } });
}(confirmed in apps/api/v2/test/fixtures/repository/team.repository.fixture.ts)
• Changing to team.id will correctly target the created Team record.
🤖 Prompt for AI Agents
In
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/emails/team-emails.e2e-spec.ts
around lines 524-533, the teardown calls
teamRepositoryFixture.delete(organization.id) but the fixture.delete expects a
team ID; change the argument to the created team’s id (e.g., team.id) so the
correct Team record is deleted during cleanup and keep the other deletes and
app.close() as-is.
| }, | ||
| }; | ||
| assignAllTeamMembers: type === "COLLECTIVE", | ||
| locations: [{ type: "inPerson", address: "via 10, rome, italy" }], |
There was a problem hiding this comment.
forcing location to InPerson so we don't try to create daily video call and trigger E2E error because of missing daily api key
| it("should not send an email when cancelling a round robin booking", async () => { | ||
| const body: CancelBookingInput_2024_08_13 = { | ||
| cancellationReason: "Going on a vacation", | ||
| it("should handle the full booking lifecycle for a collective event without sending emails", async () => { |
There was a problem hiding this comment.
rewrote tests to be sequential, going through a whole flow within each test
What does this PR do?
This pull request refactors the "lucky user" functionality into a service class with Dependency Injection (DI). This is a good practice that will make the code more modular, testable, and maintainable.
Here's a summary of the key changes:
Mandatory Tasks (DO NOT REMOVE)