test: DO NOT MERGE Convert lucky user to service class 2#23288
test: DO NOT MERGE Convert lucky user to service class 2#23288keithwillcode wants to merge 9 commits intomainfrom
Conversation
|
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. |
WalkthroughThe change refactors “LuckyUser” from standalone functions to a DI-backed service (LuckyUserService) and updates all call sites and tests to use getLuckyUserService(). It introduces DI containers/modules and tokens for LuckyUser, Host, and Attribute repositories, and re-exports LuckyUserService and repositories from platform libraries. New NestJS repository wrappers and a LuckyUserService are added in apps/api/v2, wired to PrismaWriteService. Repositories gain new query methods (attributes with weights, hosts created in interval, OOO entries in interval, users with last booking). A dependency bump to @calcom/platform-libraries 0.0.324 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
|
keithwillcode
changed the title
keithwillcode
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/ee/round-robin/roundRobinReassignment.ts (1)
468-469: Prefer named export over default exportOur guidelines discourage default exports for better tree-shaking and refactors. You already have a named export for the function; remove the default.
-export default roundRobinReassignment; +// Use the existing named export declared above. +export { roundRobinReassignment };
🧹 Nitpick comments (22)
packages/lib/server/repository/user.ts (1)
1031-1069: “Last booking” ordered by createdAt may not reflect the latest attended meetingYou’re sorting by createdAt. If the intent is “most recent meeting held” for fairness/recency logic, consider ordering by startTime (or equivalent time field) instead. Also, for empty input arrays, an early return avoids a no-op DB roundtrip.
- Please confirm whether “last booking” means “last created” or “last occurrence by start time”.
- If it’s by occurrence, update orderBy accordingly.
Minimal improvement:
async findUsersWithLastBooking({ userIds, eventTypeId }: { userIds: number[]; eventTypeId: number }) { + if (userIds.length === 0) return []; return this.prismaClient.user.findMany({ where: { id: { in: userIds, }, }, select: { id: true, bookings: { select: { - createdAt: true, + createdAt: true, + // If your Booking model has 'startTime' (or 'start'), prefer that for "last attended" + // startTime: true, }, where: { eventTypeId, status: BookingStatus.ACCEPTED, attendees: { some: { noShow: false, }, }, OR: [ { noShowHost: false }, { noShowHost: null }, ], }, - orderBy: { createdAt: "desc" }, + // Prefer startTime if "last attended" is intended + orderBy: { createdAt: "desc" }, take: 1, }, }, }); }If consumers really need the latest attended meeting, switch orderBy to startTime (or the canonical start field) after verifying the schema.
packages/lib/di/containers/LuckyUser.ts (1)
14-25: Lazy-init the container to avoid module side effects and add a reset hook for testsEagerly creating/loading the container at import time introduces side effects that complicate test isolation and bundling. Prefer lazy init on first use and expose a testing reset hook.
Apply:
-const container = createContainer(); -container.load(DI_TOKENS.PRISMA_MODULE, prismaModule); -container.load(DI_TOKENS.BOOKING_REPOSITORY_MODULE, bookingRepositoryModule); -container.load(DI_TOKENS.HOST_REPOSITORY_MODULE, hostRepositoryModule); -container.load(DI_TOKENS.OOO_REPOSITORY_MODULE, oooRepositoryModule); -container.load(DI_TOKENS.USER_REPOSITORY_MODULE, userRepositoryModule); -container.load(DI_TOKENS.ATTRIBUTE_REPOSITORY_MODULE, attributeRepositoryModule); -container.load(DI_TOKENS.LUCKY_USER_SERVICE_MODULE, luckyUserServiceModule); +let container: ReturnType<typeof createContainer> | null = null; +function getContainer() { + if (!container) { + container = createContainer(); + container.load(DI_TOKENS.PRISMA_MODULE, prismaModule); + container.load(DI_TOKENS.BOOKING_REPOSITORY_MODULE, bookingRepositoryModule); + container.load(DI_TOKENS.HOST_REPOSITORY_MODULE, hostRepositoryModule); + container.load(DI_TOKENS.OOO_REPOSITORY_MODULE, oooRepositoryModule); + container.load(DI_TOKENS.USER_REPOSITORY_MODULE, userRepositoryModule); + container.load(DI_TOKENS.ATTRIBUTE_REPOSITORY_MODULE, attributeRepositoryModule); + container.load(DI_TOKENS.LUCKY_USER_SERVICE_MODULE, luckyUserServiceModule); + } + return container; +} export function getLuckyUserService() { - return container.get<LuckyUserService>(DI_TOKENS.LUCKY_USER_SERVICE); + return getContainer().get<LuckyUserService>(DI_TOKENS.LUCKY_USER_SERVICE); } + +// Optional: test-only reset to ensure clean DI graph per test file. +export function __resetLuckyUserContainer_FOR_TESTS__() { + container = null; +}packages/features/ee/round-robin/roundRobinReassignment.ts (1)
5-6: Day.js usage is acceptable here; avoid placing conversions in tight loopsYou’re formatting a handful of timestamps and using
.utc()for outbound event payloads. This isn’t a hot path loop, so it’s fine.If you ever move this computation into per-slot loops, switch to native Date or pre-constructed dayjs.utc objects to avoid repeated tz ops.
Also applies to: 147-155, 316-318
packages/lib/server/repository/host.ts (1)
18-26: Method name suggests a closed interval; API only takesstartDateThe name “findHostsCreatedInInterval” implies [start, end], but the signature only supports “since startDate”. Either rename to
findHostsCreatedSinceor add anendDatefilter.Proposed rename:
- async findHostsCreatedInInterval({ + async findHostsCreatedSince({ eventTypeId, userIds, startDate, }: { eventTypeId: number; userIds: number[]; startDate: Date; }) {And update call sites accordingly.
apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (1)
9-11: Avoid double assertion; align Prisma types to dropas unknown as PrismaClientThe cast
as unknown as PrismaClientis a code smell and hides type mismatches betweenPrismaWriteService.prismaand the expectedPrismaClient. Prefer tightening types so the cast isn’t needed (e.g., ensurePrismaWriteService.prismais typed as@calcom/prisma’sPrismaClient, or relax the base class constructor to accept a minimal Prisma interface).Apply if the types already line up:
- super(dbWrite.prisma as unknown as PrismaClient); + super(dbWrite.prisma);If they don’t, consider updating
PrismaWriteService’sprismaproperty type to the samePrismaClientthat the base repository expects.packages/lib/di/modules/LuckyUser.ts (2)
4-4: Consider moving the service class to a dedicated fileImporting
LuckyUserServicefromgetLuckyUseris slightly counterintuitive. Consider relocating the class topackages/lib/server/services/LuckyUserService.ts(or similar) for clarity.
6-13: Unify DI binding style across modulesThis module uses object-based constructor mapping, while
Attribute.tsuses positional (array) injection. Consider standardizing on one style to reduce cognitive overhead.packages/lib/di/modules/Attribute.ts (1)
6-9: Consider consistent DI binding style withLuckyUsermoduleThis module uses positional args;
LuckyUseruses object mapping. Pick one style across modules for consistency.packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (3)
273-274: Avoid re-instantiating the DI container/service per requestCreating the service inside the handler can recreate the container on every call. Hoist it to module scope to reuse across requests.
Apply:
@@ -import { getLuckyUserService } from "@calcom/lib/di/containers/LuckyUser"; +import { getLuckyUserService } from "@calcom/lib/di/containers/LuckyUser"; +// Instantiate once at module scope to avoid per-request container bootstrap +const luckyUserService = getLuckyUserService(); @@ - const luckyUserService = getLuckyUserService();
279-293: SimplifyavailableUsersconstructionUse a single
mapinstead of handling the first element separately.Apply:
- } = matchingHosts.length - ? await luckyUserService.getOrderedListOfLuckyUsers({ - // Assuming all are available - availableUsers: [ - { - ...matchingHosts[0].user, - weight: matchingHosts[0].weight, - priority: matchingHosts[0].priority, - }, - ...matchingHosts.slice(1).map((host) => ({ - ...host.user, - weight: host.weight, - priority: host.priority, - })), - ], - eventType, - allRRHosts: matchingHosts, - routingFormResponse: { - response, - form, - chosenRouteId: route.id, - }, - }) + } = matchingHosts.length + ? await luckyUserService.getOrderedListOfLuckyUsers({ + availableUsers: matchingHosts.map((host) => ({ + ...host.user, + weight: host.weight, + priority: host.priority, + })), + eventType, + allRRHosts: matchingHosts, + routingFormResponse: { + response, + form, + chosenRouteId: route.id, + }, + })
305-310: Don’t log in production by default; guard logs behind_enablePerf
console.log("_enablePerf, _concurrency", ...)runs unconditionally. Move under the_enablePerfcheck to avoid noisy logs.Apply:
- console.log("_enablePerf, _concurrency", _enablePerf, _concurrency); - if (_enablePerf) { - const serverTimingHeader = getServerTimingHeader(timeTaken); - ctx.res?.setHeader("Server-Timing", serverTimingHeader); - console.log("Server-Timing", serverTimingHeader); - } + if (_enablePerf) { + const serverTimingHeader = getServerTimingHeader(timeTaken); + ctx.res?.setHeader("Server-Timing", serverTimingHeader); + console.log("_enablePerf, _concurrency", _enablePerf, _concurrency); + console.log("Server-Timing", serverTimingHeader); + }apps/api/v2/src/lib/repositories/prisma-host.repository.ts (1)
5-11: Tighten typings; avoid double type assertionThe double cast
as unknown as PrismaClientis a smell. IfdbWrite.prismais compatible, a single cast is sufficient. Also, make the PrismaClient import type-only.Apply:
-import { PrismaClient } from "@calcom/prisma"; +import type { PrismaClient } from "@calcom/prisma"; ... - super(dbWrite.prisma as unknown as PrismaClient); + super(dbWrite.prisma as PrismaClient);If the base repository expects a slightly different Prisma client type, consider widening the base type via generics instead of relying on double assertions.
packages/features/bookings/lib/handleNewBooking.ts (1)
906-915: Avoid resolving the service inside the inner selection loop
getLuckyUserService()is called for every host-group iteration. Hoist it once outside the loop to avoid repeated DI resolution and minor overhead in hot paths.Apply:
@@ - const luckyUsers: typeof users = []; + const luckyUsers: typeof users = []; + const luckyUserService = getLuckyUserService(); @@ - const luckyUserService = getLuckyUserService(); const newLuckyUser = await luckyUserService.getLuckyUser({This keeps behavior identical and reduces per-iteration work.
apps/api/v2/src/lib/services/lucky-user.service.ts (1)
19-25: Use object property shorthandMinor cleanup: use shorthand to reduce noise.
- super({ - bookingRepository: bookingRepository, - hostRepository: hostRepository, - oooRepository: oooRepository, - userRepository: userRepository, - attributeRepository: attributeRepository, - }); + super({ bookingRepository, hostRepository, oooRepository, userRepository, attributeRepository });packages/lib/server/getLuckyUser.test.ts (2)
317-338: DRY up allRRHosts scaffolding in testsThe repeated inline construction of allRRHosts.user objects adds noise and invites drift. Consider a small helper (rrHostFromUser) to keep these in sync and reduce duplication.
Example applied locally within the test file:
+const rrHostFromUser = (user: { id: number; email: string }, weight?: number | null, createdAt: Date = new Date(0)) => ({ + user: { id: user.id, email: user.email, credentials: [], userLevelSelectedCalendars: [] }, + weight, + createdAt, +}); ... - const allRRHosts = [ - { - user: { - id: users[0].id, - email: users[0].email, - credentials: [], - userLevelSelectedCalendars: [], - }, - weight: users[0].weight, - createdAt: new Date(0), - }, - { - user: { - id: users[1].id, - email: users[1].email, - credentials: [], - userLevelSelectedCalendars: [], - }, - weight: users[1].weight, - createdAt: new Date(0), - }, - ]; + const allRRHosts = [rrHostFromUser(users[0], users[0].weight), rrHostFromUser(users[1], users[1].weight)];Also applies to: 329-337, 436-457, 448-456, 555-576, 567-575, 621-642, 633-641, 738-759, 750-758, 864-885, 875-883, 1460-1479, 1470-1478
203-207: Fix typo in test data email to avoid brittle assertionsFound "tes2t@example.com". If any logic later keys on email, this typo could cause hard-to-spot failures.
- email: "tes2t@example.com", + email: "test2@example.com",packages/lib/bookings/filterHostsByLeadThreshold.ts (3)
29-31: Stabilize the PerUserData type extractionUsing typeof LuckyUserService.prototype works but is brittle. Prefer InstanceType for instance methods.
-type PerUserData = Awaited< - ReturnType<(typeof LuckyUserService.prototype)["getOrderedListOfLuckyUsers"]> ->["perUserData"]; +type PerUserData = Awaited< + ReturnType<InstanceType<typeof LuckyUserService>["getOrderedListOfLuckyUsers"]> +>["perUserData"];
73-76: Fix contradictory log messageThis log line says “filtered out” while allowing the host. Clarify the message to avoid confusion during ops triage.
- 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 ${userIdStr} allowed. BookingsCount: ${bookingsCount}, MinBookings: ${minBookings}, MaxLeadThreshold: ${maxLeadThreshold}` + );
120-123: Hoist DI resolution to module scope to avoid repeated container lookupsIf the container returns a singleton, resolving per invocation is unnecessary overhead.
-import { getLuckyUserService } from "../di/containers/LuckyUser"; +import { getLuckyUserService } from "../di/containers/LuckyUser"; +const luckyUserService = getLuckyUserService(); ... - const luckyUserService = getLuckyUserService(); const orderedLuckyUsers = await luckyUserService.getOrderedListOfLuckyUsers({packages/lib/server/getLuckyUser.ts (3)
244-256: Dead check; object literal is always truthyuserIdAndAtCreatedPair is always defined. If you intended to guard emptiness, check keys length.
- if (!userIdAndAtCreatedPair) { - throw new Error("Unable to find users by availableUser ids."); - } + if (Object.keys(userIdAndAtCreatedPair).length === 0) { + throw new Error("Unable to find users by availableUser ids."); + }
520-554: Use forEach for side-effects; avoid mapmap’s return value is unused. Replace with forEach for clarity.
- fieldValueArray.map((obj) => { + fieldValueArray.forEach((obj) => { ... - }); + });
730-909: Good orchestration in fetchAllDataNeededForCalculations; consider minor perf tidy-ups
- You compute dayjs(...).tz(...).utcOffset() inside a filter loop. If all busyTime entries share the same timeZone, cache per timeZone to cut repeated tz computations.
- Safe to defer; current complexity is acceptable for typical sizes.
📜 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 (23)
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)
🧰 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/modules/Attribute.tspackages/platform/libraries/bookings.tspackages/lib/server/repository/PrismaAttributeRepository.tspackages/lib/di/modules/LuckyUser.tspackages/lib/di/tokens.tspackages/platform/libraries/repositories.tsapps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/services/lucky-user.service.tspackages/lib/di/modules/Host.tspackages/lib/di/containers/LuckyUser.tspackages/lib/server/repository/host.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/repository/user.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/server/getLuckyUser.tspackages/lib/bookings/filterHostsByLeadThreshold.test.tspackages/lib/server/getLuckyUser.integration-test.tspackages/lib/server/repository/ooo.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/modules/Attribute.tspackages/platform/libraries/bookings.tspackages/lib/server/repository/PrismaAttributeRepository.tspackages/lib/di/modules/LuckyUser.tspackages/lib/di/tokens.tspackages/platform/libraries/repositories.tsapps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/services/lucky-user.service.tspackages/lib/di/modules/Host.tspackages/lib/di/containers/LuckyUser.tspackages/lib/server/repository/host.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/repository/user.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/server/getLuckyUser.tspackages/lib/bookings/filterHostsByLeadThreshold.test.tspackages/lib/server/getLuckyUser.integration-test.tspackages/lib/server/repository/ooo.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/modules/Attribute.tspackages/platform/libraries/bookings.tspackages/lib/server/repository/PrismaAttributeRepository.tspackages/lib/di/modules/LuckyUser.tspackages/lib/di/tokens.tspackages/platform/libraries/repositories.tsapps/api/v2/src/lib/repositories/prisma-attribute.repository.tsapps/api/v2/src/lib/services/lucky-user.service.tspackages/lib/di/modules/Host.tspackages/lib/di/containers/LuckyUser.tspackages/lib/server/repository/host.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/repository/user.tsapps/api/v2/src/lib/repositories/prisma-host.repository.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/server/getLuckyUser.tspackages/lib/bookings/filterHostsByLeadThreshold.test.tspackages/lib/server/getLuckyUser.integration-test.tspackages/lib/server/repository/ooo.tspackages/lib/server/getLuckyUser.test.tspackages/lib/bookings/filterHostsByLeadThreshold.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
**/*.{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/services/lucky-user.service.tsapps/api/v2/src/lib/repositories/prisma-host.repository.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:
packages/platform/libraries/repositories.tsapps/api/v2/src/lib/repositories/prisma-host.repository.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
⏰ 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). (10)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (29)
packages/lib/server/repository/user.ts (1)
11-11: Importing BookingStatus for filtering is correctThe enum import is appropriate for the new query below. No issues.
packages/platform/libraries/repositories.ts (2)
1-1: Re-export PrismaAttributeRepository — LGTMThe re-export aligns with the platform-libraries surface and DI wrappers in api-v2. No concerns.
12-12: Alias export PrismaHostRepository — naming is consistent with repository conventionsThe alias keeps the “Prisma*Repository” convention and matches other exports in this module. Good for clarity and DI wiring.
packages/lib/server/repository/ooo.ts (1)
150-177: New findOOOEntriesInInterval uses correct overlap check and minimal select — goodThis method’s predicate (start <= endDate AND end >= startDate) is correct and the select is lean (start, end, userId). Nicely done.
packages/lib/di/tokens.ts (1)
48-53: New DI tokens for LuckyUser, Host, and Attribute — looks consistentToken naming matches existing conventions and will compose well with container modules.
packages/lib/di/containers/LuckyUser.ts (1)
3-11: Wiring order and tokens look correctModules are loaded in a sensible order (Prisma before repositories; repositories before service), and the resolver returns the expected tokenized service type.
If ioctopus supports lifetimes/scopes, confirm repositories/services are singletons to avoid churn per resolve. Otherwise, consider configuring scoped/singleton lifetimes in the module bindings.
Also applies to: 15-21, 23-25
packages/platform/libraries/bookings.ts (1)
1-1: Re-export is fine and aligns the public surface with the DI migrationNo issues; this makes LuckyUserService available to platform consumers without leaking container details.
packages/lib/di/modules/Host.ts (1)
6-7: Binding is correct; verify lifetimeBinding HostRepository to the Prisma client via DI_TOKENS is correct.
Double-check the default lifetime in @evyweb/ioctopus. If it’s transient, consider a singleton/scoped lifetime for repositories to avoid unnecessary allocations while keeping the Prisma client usage consistent.
packages/features/ee/round-robin/roundRobinReassignment.ts (3)
160-162: TODO note: allRRHosts vs virtual queueThe inline TODO implies a behavior change (constraining to a virtual queue). Track this with an issue to avoid bitrot.
Would you like me to open a tracking item and link it here?
23-23: No legacygetLuckyUserimports remainingRan a targeted search for any direct imports of the legacy
getLuckyUserfunction and found none across all.tsand.tsxfiles, confirming that only the DI-backedLuckyUserServiceis in use.
340-351: I’ve requested the snippet around the Prisma query for inspection. Once we see howcredentialsis used (especially regarding.keyaccess or API serialization), we can confirm whether the raw key is needed and if a shared safe select projection is defined.apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts (1)
7-12: Wiring looks goodExtending the platform repository and injecting
PrismaWriteServiceis consistent with the existing Nest wrapper pattern.packages/lib/di/modules/LuckyUser.ts (1)
7-13: No changes needed—DI mapping aligns with constructor properties
The mapping keys (bookingRepository,hostRepository,oooRepository,userRepository,attributeRepository) exactly match the fields accessed on thedeps: ILuckyUserServiceconstructor argument inLuckyUserService, so all dependencies will be injected correctly.packages/lib/server/repository/PrismaAttributeRepository.ts (1)
74-97: Good use ofselectto minimize payloadsThe selection is tight and avoids
include, complying with the Prisma query guideline to fetch only necessary fields.packages/lib/di/modules/Attribute.ts (1)
6-9: Binding looks correctToken-to-class binding and constructor dependency on
PRISMA_CLIENTare appropriate.packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (1)
331-331: Remove redundant default export infindTeamMembersMatchingAttributeLogicOfRoute.handler.tsWe didn’t find any default‐import usages of this handler across
.ts/.tsxfiles, so it’s safe to drop the extra default export now that the named export is already in place.• File:
packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts
• Line: 331Apply the following diff:
-export default findTeamMembersMatchingAttributeLogicOfRouteHandler;apps/api/v2/src/lib/repositories/prisma-host.repository.ts (1)
9-11: Verify provider wiring for PrismaWriteServiceConfirm this repository’s module provides PrismaWriteService and that its lifecycle (scoped/singleton) matches usage expectations. Otherwise the implicit dependency might fail at runtime.
packages/features/bookings/lib/handleNewBooking.ts (1)
57-57: LGTM: switch to DI containerImporting
getLuckyUserServicealigns with the new service-based API and improves testability.apps/api/v2/src/lib/services/lucky-user.service.ts (1)
10-27: LGTM: thin NestJS wrapper cleanly wires base serviceConstructor injection cleanly forwards repositories to the base
LuckyUserService. No logic smells.packages/lib/bookings/filterHostsByLeadThreshold.test.ts (3)
1-5: LGTM: tests now target the DI-backed serviceImporting
getLuckyUserServiceand creating a service instance improves realism and keeps tests aligned with production usage.Also applies to: 10-10
110-115: LGTM: service method is properly stubbedStubbing
luckyUserService.getOrderedListOfLuckyUsersthrough a spy is the right approach here.
167-174: LGTM: weighted scenario mock data is representativeThe mocked structure covers
bookingsCount,weights,bookingShortfalls, andcalibrations, aligning with the service’s expected per-user data.packages/lib/server/getLuckyUser.integration-test.ts (1)
3-7: LGTM: integration tests migrated to service API
- Using
getLuckyUserService()and invokinggetLuckyUser/getOrderedListOfLuckyUsersmirrors production usage.- The
resolvesassertions and time control viauseFakeTimers/setSystemTimelook correct.- No behavior changes introduced by the refactor in these call sites.
Also applies to: 205-215, 264-275, 361-377, 429-439, 472-482, 485-495, 527-536, 547-556, 625-629, 708-711, 784-787
packages/lib/server/getLuckyUser.test.ts (2)
1092-1141: Nice coverage: virtual queues + attribute weights path is tested end-to-endThe assertions validate both attributeWeights and virtualQueuesData shape, matching the service’s contract. No changes requested.
Also applies to: 1236-1276
1511-1575: Interval helpers are well-tested across bases and intervalsThe START_TIME vs CREATED_AT branches and DAY vs MONTH are exercised with deterministic expectations—good guard against regressions.
packages/lib/bookings/filterHostsByLeadThreshold.ts (1)
113-118: Guard condition reads fine; confirm CREATED_AT is the only supported basis hereThe early return prevents applying the filter when rrTimestampBasis ≠ CREATED_AT. If START_TIME must be supported later, make this explicit in docs/JSDoc to avoid accidental misuse.
Add a short JSDoc above the function explaining this constraint, and confirm no callsite passes START_TIME expecting this filter to apply.
packages/lib/server/getLuckyUser.ts (3)
368-488: Weight-based filtering logic is solid; telemetry is helpfulThe combination of shortfall, calibration, and tie-break by weight is clear and logged. No action needed.
1033-1134: getOrderedListOfLuckyUsers iteration is deterministic and guards cyclesThe while loop plus set membership guard prevents accidental repeats; per-user metrics are exported cleanly. Looks good.
720-728: The extraction script is running; I’ll review thegetAllBookingsForRoundRobinimplementation as soon as the output is available.
| "@calcom/platform-constants": "*", | ||
| "@calcom/platform-enums": "*", | ||
| "@calcom/platform-libraries": "npm:@calcom/platform-libraries@0.0.321", | ||
| "@calcom/platform-libraries": "npm:@calcom/platform-libraries@0.0.324", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version bump to @calcom/platform-libraries@0.0.324 — check workspace alignment and lockfile consistency
Looks good. Please verify other workspaces don’t pin a conflicting version and that the lockfile is updated to prevent mixed versions at runtime.
Run this to audit all package.json files and list distinct versions referenced:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all referenced versions of @calcom/platform-libraries across the monorepo
fd -a package.json | xargs -I{} sh -c 'printf "%s: " "{}"; jq -r ".dependencies[\"@calcom/platform-libraries\"] // .devDependencies[\"@calcom/platform-libraries\"] // empty" "{}"' | rg -n "@calcom/platform-libraries|.json"
echo
echo "Distinct versions found:"
fd -a package.json | xargs -I{} jq -r ".dependencies[\"@calcom/platform-libraries\"] // .devDependencies[\"@calcom/platform-libraries\"] // empty" "{}" | sort -uLength of output: 9083
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Define lockfiles to check
lockfiles=("pnpm-lock.yaml" "yarn.lock" "package-lock.json")
for f in "${lockfiles[@]}"; do
if [ -f "$f" ]; then
echo "----- Checking $f -----"
# Find entries for the exact bumped version
grep -n "@calcom/platform-libraries@0\.0\.324" "$f" \
&& echo "Found correct version in $f" \
|| echo "No entry for 0.0.324 in $f"
# Ensure there are no other version entries
echo "Looking for any other @calcom/platform-libraries versions in $f..."
grep -n "@calcom/platform-libraries@" "$f" | grep -v "0\.0\.324" \
&& echo "⚠️ Conflicting versions found in $f" \
|| echo "No conflicting versions in $f"
echo
fi
doneLength of output: 1103
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
TARGET="packages/platform/libraries/package.json"
if [ -f "$TARGET" ]; then
printf "%s version: %s\n" "$TARGET" "$(jq -r .version "$TARGET")"
else
echo "Error: $TARGET not found"
fiLength of output: 313
Fix workspace package version and update lockfile
We’ve confirmed that while you bumped the dependency in apps/api/v2/package.json to npm:@calcom/platform-libraries@0.0.324, the yarn.lock still contains a workspace resolution for the local packages/platform/libraries at version 0.0.0. This will result in two distinct versions of the same package at install time.
• packages/platform/libraries/package.json is still at version 0.0.0 — bump it to 0.0.324
• Run yarn install to regenerate yarn.lock and eliminate the workspace resolution entry for @calcom/platform-libraries
• Verify yarn.lock now only contains the single entry for @calcom/platform-libraries@npm:@calcom/platform-libraries@0.0.324
Please make these adjustments to ensure a consistent, single-version install.
🤖 Prompt for AI Agents
In apps/api/v2/package.json around line 41 the dependency was changed to
"npm:@calcom/platform-libraries@0.0.324" but your repo still has a workspace
resolution to the local packages/platform/libraries at version 0.0.0, causing
duplicate installs; update packages/platform/libraries/package.json version to
0.0.324, run yarn install to regenerate yarn.lock (removing the workspace
resolution and recording the npm:@calcom/platform-libraries@0.0.324 entry), and
verify yarn.lock contains only the single
@calcom/platform-libraries@npm:@calcom/platform-libraries@0.0.324 entry.
| import { PrismaClient } from "@calcom/prisma"; | ||
|
|
||
| @Injectable() | ||
| export class PrismaAttributeRepository extends PrismaAttributeRepositoryLib { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename file to PascalCase without dot-suffix to match repository naming guidelines
Per guidelines: avoid .repository.ts suffix for new files and prefer PascalCase with Repository suffix (e.g., PrismaAttributeRepository.ts). This also aligns with the pattern used elsewhere (e.g., packages/lib/server/repository/PrismaAttributeRepository.ts). Please rename the file and update imports.
🤖 Prompt for AI Agents
In apps/api/v2/src/lib/repositories/prisma-attribute.repository.ts around line
8, the file name uses a dot-suffix and lowercase which violates the repo naming
guidelines; rename the file to PascalCase without the dot-suffix
(PrismaAttributeRepository.ts) and update all imports/exports that reference the
old path to the new filename, ensuring project-wide references (other files,
barrel exports, and build configs) are updated and TypeScript paths/references
still compile.
| @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.
🛠️ Refactor suggestion
Rename file to PascalCase without the “.repository.ts” suffix to match repo conventions
Per the coding guidelines and our repo learnings, new repositories should avoid “.repository.ts” and use PascalCase matching the exported class. Suggest renaming:
- prisma-host.repository.ts → PrismaHostRepository.ts
Also ensure import paths and DI bindings are updated accordingly.
🤖 Prompt for AI Agents
In apps/api/v2/src/lib/repositories/prisma-host.repository.ts around lines 7 to
12, the file name doesn't match repo conventions: rename the file to PascalCase
matching the exported class (PrismaHostRepository.ts) and remove the
“.repository.ts” suffix; after renaming, update all import paths across the
codebase (including module registration, DI providers, barrels/index files,
tests and any build config) to point to the new filename and ensure dependency
injection bindings still reference the same class export.
| import { PrismaAttributeRepository } from "@/lib/repositories/prisma-attribute.repository"; | ||
| import { PrismaBookingRepository } from "@/lib/repositories/prisma-booking.repository"; | ||
| import { PrismaHostRepository } from "@/lib/repositories/prisma-host.repository"; | ||
| import { PrismaOOORepository } from "@/lib/repositories/prisma-ooo.repository"; | ||
| import { PrismaUserRepository } from "@/lib/repositories/prisma-user.repository"; | ||
| import { Injectable } from "@nestjs/common"; | ||
|
|
||
| import { LuckyUserService as BaseLuckyUserService } from "@calcom/platform-libraries/bookings"; | ||
|
|
||
| @Injectable() | ||
| export class LuckyUserService extends BaseLuckyUserService { | ||
| constructor( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename file to avoid “.service.ts” suffix
Per coding guidelines, new files should avoid the “.service.ts” suffix. Suggest renaming:
- lucky-user.service.ts → LuckyUserService.ts
This matches the class name and repository naming guidance.
🤖 Prompt for AI Agents
In apps/api/v2/src/lib/services/lucky-user.service.ts lines 1-12: the file name
uses the “.service.ts” suffix which violates naming guidelines; rename the file
to LuckyUserService.ts (matching the exported class) and update all project
references and exports to the new filename (including any barrel/index files,
module imports, and tests) to use the exact new PascalCase path; ensure your git
move preserves history (git mv) and run a quick grep/IDE search to replace
remaining imports, then rebuild to verify no unresolved imports remain.
| // This variable will hold our mock function | ||
| let getOrderedListOfLuckyUsersMock: Mock; | ||
|
|
||
| beforeEach(() => { | ||
| // Clear all mocks and spies before each test | ||
| vi.clearAllMocks(); | ||
| // Spy on the real method and explicitly cast it as a Mock | ||
| getOrderedListOfLuckyUsersMock = vi.spyOn(luckyUserService, "getOrderedListOfLuckyUsers") as Mock; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| // Clear call history before each test to avoid cross-test interference | ||
| prismaMock.booking.groupBy.mockClear(); | ||
| // Restore all spies to their original implementation | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Mock lifecycle bug: restoreAllMocks() undoes the module-scope Prisma spy
vi.restoreAllMocks() in afterEach restores the module-scope spy on prisma.booking.groupBy (set at Line 20) back to the real implementation. Subsequent tests will call the real DB unless the spy is re-applied. Move the Prisma spy into beforeEach (and remove the module-scope spy), so it’s re-established for each test.
Apply within beforeEach:
beforeEach(() => {
// Clear all mocks and spies before each test
vi.clearAllMocks();
// Spy on the real method and explicitly cast it as a Mock
getOrderedListOfLuckyUsersMock = vi.spyOn(luckyUserService, "getOrderedListOfLuckyUsers") as Mock;
+ // Re-spy Prisma groupBy for each test
+ vi.spyOn(prisma.booking, "groupBy").mockImplementation(prismaMock.booking.groupBy);
});And remove the module-scope spy:
-// Use `vi.spyOn` to make `prisma.booking.groupBy` call the mock instead
-vi.spyOn(prisma.booking, "groupBy").mockImplementation(prismaMock.booking.groupBy);This prevents accidental real DB access and keeps mocks reliable across tests.
🤖 Prompt for AI Agents
In packages/lib/bookings/filterHostsByLeadThreshold.test.ts around lines 22 to
35, the afterEach vi.restoreAllMocks() is undoing the module-scope spy on
prisma.booking.groupBy (declared at ~line 20), causing later tests to hit the
real DB; remove the module-scope spy and instead create and assign the
prisma.booking.groupBy spy inside beforeEach so it is re-established for every
test, keep vi.clearAllMocks()/vi.restoreAllMocks() in the lifecycle, and ensure
any references to the spy variable are updated accordingly.
| 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.
Division by zero when only one host present
In OOO calibration, hosts.length - 1 can be 0; dividing by zero yields Infinity. Guard the denominator.
- oooEntries.forEach((oooEntry) => {
+ const denominator = Math.max(1, hosts.length - 1); // avoid division by zero
+ 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);
+ calibration += bookingsInTimeframe.length / denominator;
});📝 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.
| 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); | |
| }); | |
| oooData.forEach(({ userId, oooEntries }) => { | |
| let calibration = 0; | |
| // avoid division by zero if there's only one host | |
| const denominator = Math.max(1, hosts.length - 1); | |
| oooEntries.forEach((oooEntry) => { | |
| const bookingsInTimeframe = existingBookings.filter( | |
| (booking) => | |
| booking.createdAt >= oooEntry.start && | |
| booking.createdAt <= oooEntry.end && | |
| booking.userId !== userId | |
| ); | |
| calibration += bookingsInTimeframe.length / denominator; | |
| }); |
🤖 Prompt for AI Agents
In packages/lib/server/getLuckyUser.ts around lines 305 to 317, the OOO
calibration divides by (hosts.length - 1) which can be zero when there is only
one host; change the logic to guard the denominator by either skipping
calibration when hosts.length <= 1 or using a safe denominator (e.g. const denom
= hosts.length > 1 ? hosts.length - 1 : 1) and divide by denom; ensure behavior
remains consistent (skip or neutral contribution) when there are no other hosts.
| const earliestStartTime = new Date(Date.UTC(new Date().getFullYear(), new Date().getMonth(), 1)); | ||
| if (start < earliestStartTime) start = earliestStartTime; | ||
|
|
||
| // make sure start date and end date is converted to 00:00 for full day busy events | ||
| 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); | ||
| return end.getTime() < new Date().getTime() && isFullDayEvent(start, end); | ||
| }) | ||
| .map((busyTime) => ({ start: new Date(busyTime.start), end: new Date(busyTime.end) })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Interval mismatch: full-day event clipping hardcodes start-of-month
The clipping uses first-of-month even when interval is DAY. This can miscount full-day events outside the current interval. Clip to intervalStartDate instead.
- const earliestStartTime = new Date(Date.UTC(new Date().getFullYear(), new Date().getMonth(), 1));
+ const earliestStartTime = intervalStartDate;
if (start < earliestStartTime) start = earliestStartTime;📝 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 earliestStartTime = new Date(Date.UTC(new Date().getFullYear(), new Date().getMonth(), 1)); | |
| if (start < earliestStartTime) start = earliestStartTime; | |
| // make sure start date and end date is converted to 00:00 for full day busy events | |
| 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); | |
| return end.getTime() < new Date().getTime() && isFullDayEvent(start, end); | |
| }) | |
| .map((busyTime) => ({ start: new Date(busyTime.start), end: new Date(busyTime.end) })); | |
| const earliestStartTime = intervalStartDate; | |
| if (start < earliestStartTime) start = earliestStartTime; | |
| return end.getTime() < new Date().getTime() && isFullDayEvent(start, end); | |
| }) | |
| .map((busyTime) => ({ start: new Date(busyTime.start), end: new Date(busyTime.end) })); |
🤖 Prompt for AI Agents
In packages/lib/server/getLuckyUser.ts around lines 844-849 the code clips
full-day event starts to the first-of-month (new Date(Date.UTC(...,1))) which is
incorrect for DAY intervals; change the clipping to use the existing
intervalStartDate (i.e., set earliestStartTime = intervalStartDate) so full-day
events are clipped to the actual interval start; if intervalStartDate is not in
scope, pass or compute it from the current interval context and use it instead
of the hardcoded month-start.
| async findHostsCreatedInInterval({ | ||
| eventTypeId, | ||
| userIds, | ||
| startDate, | ||
| }: { | ||
| eventTypeId: number; | ||
| userIds: number[]; | ||
| startDate: Date; | ||
| }) { | ||
| return await this.prismaClient.host.findMany({ | ||
| where: { | ||
| userId: { | ||
| in: userIds, | ||
| }, | ||
| eventTypeId, | ||
| isFixed: false, | ||
| createdAt: { | ||
| gte: startDate, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add select to constrain returned fields per Prisma guidelines
The repository should limit the columns returned. Returning full rows increases payload and risks leaking fields consumers don’t need.
async findHostsCreatedInInterval({
@@
}) {
- return await this.prismaClient.host.findMany({
- where: {
- userId: {
- in: userIds,
- },
- eventTypeId,
- isFixed: false,
- createdAt: {
- gte: startDate,
- },
- },
- });
+ return await this.prismaClient.host.findMany({
+ where: {
+ userId: { in: userIds },
+ eventTypeId,
+ isFixed: false,
+ createdAt: { gte: startDate },
+ },
+ // Select only the fields the service actually uses. Adjust as needed.
+ select: {
+ id: true,
+ userId: true,
+ eventTypeId: true,
+ isFixed: true,
+ createdAt: 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 findHostsCreatedInInterval({ | |
| eventTypeId, | |
| userIds, | |
| startDate, | |
| }: { | |
| eventTypeId: number; | |
| userIds: number[]; | |
| startDate: Date; | |
| }) { | |
| return await this.prismaClient.host.findMany({ | |
| where: { | |
| userId: { | |
| in: userIds, | |
| }, | |
| eventTypeId, | |
| isFixed: false, | |
| createdAt: { | |
| gte: startDate, | |
| }, | |
| }, | |
| }); | |
| } | |
| async findHostsCreatedInInterval({ | |
| eventTypeId, | |
| userIds, | |
| startDate, | |
| }: { | |
| eventTypeId: number; | |
| userIds: number[]; | |
| startDate: Date; | |
| }) { | |
| return await this.prismaClient.host.findMany({ | |
| where: { | |
| userId: { in: userIds }, | |
| eventTypeId, | |
| isFixed: false, | |
| createdAt: { gte: startDate }, | |
| }, | |
| // Select only the fields the service actually uses. Adjust as needed. | |
| select: { | |
| id: true, | |
| userId: true, | |
| eventTypeId: true, | |
| isFixed: true, | |
| createdAt: true, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/server/repository/host.ts around lines 18 to 39, the
findHostsCreatedInInterval query returns full host rows; add a Prisma `select`
to limit returned columns to only the fields consumers need (e.g. id, userId,
eventTypeId, createdAt — and any other explicit fields the callers rely on)
instead of returning all fields, update the return type if necessary to reflect
the narrowed shape, and ensure tests/usage sites still access only the selected
properties.
| 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.
Interval overlap logic returns false positives; simplify to canonical overlap predicate
The current OR-clause allows entries entirely before the window (start <= dateFrom AND end <= dateTo) to slip in. Use the canonical overlap check: start <= dateTo AND end >= dateFrom. This mirrors the new method below and fixes correctness.
Apply:
- async findUserOOODays({ userId, dateTo, dateFrom }: { userId: number; dateTo: string; dateFrom: string }) {
+ 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 },
- },
- ],
- },
+ where: {
+ userId,
+ 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 } },
},
});
}Optional: change dateFrom/dateTo to Date to avoid TZ pitfalls.
📝 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: string; dateFrom: string }) { | |
| return this.prismaClient.outOfOfficeEntry.findMany({ | |
| where: { | |
| userId, | |
| 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 to 148, the OR-based
interval logic is incorrect and returns false positives; replace the entire OR
array with the canonical overlap predicate so the where clause only matches
entries where start <= dateTo AND end >= dateFrom (i.e., a single condition
combining both comparisons). Also consider updating the method signature and
callers to accept Date objects (or parse the strings to Date early) to avoid
timezone pitfalls before performing the comparison.
| return await this.prismaClient.attribute.findUnique({ | ||
| where: { | ||
| id: attributeId, | ||
| teamId, | ||
| isWeightsEnabled, | ||
| }, |
There was a problem hiding this comment.
Potential Prisma misuse: findUnique with non-unique filter fields
findUnique requires a unique identifier or a declared compound unique. Supplying teamId and isWeightsEnabled alongside id will fail unless there’s an explicit compound unique on (id, teamId, isWeightsEnabled). Use findFirst for filtering or restrict findUnique to id only.
Apply this minimal fix:
- return await this.prismaClient.attribute.findUnique({
+ return await this.prismaClient.attribute.findFirst({
where: {
id: attributeId,
teamId,
isWeightsEnabled,
},Alternatively, if id is globally unique and the extra filters are just guards, use findUnique({ where: { id: attributeId }, select: { ... } }) and validate teamId/isWeightsEnabled in code.
📝 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.
| return await this.prismaClient.attribute.findUnique({ | |
| where: { | |
| id: attributeId, | |
| teamId, | |
| isWeightsEnabled, | |
| }, | |
| return await this.prismaClient.attribute.findFirst({ | |
| where: { | |
| id: attributeId, | |
| teamId, | |
| isWeightsEnabled, | |
| }, |
🤖 Prompt for AI Agents
In packages/lib/server/repository/PrismaAttributeRepository.ts around lines
68-73, the code calls prismaClient.attribute.findUnique with non-unique filter
fields (teamId and isWeightsEnabled) which is invalid; change the call to
findFirst so the provided where filter (id, teamId, isWeightsEnabled) can be
used for filtering, or alternatively restrict findUnique to just where: { id:
attributeId } and perform teamId/isWeightsEnabled checks in code if id is
globally unique.
2 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist