fix: unconfirmed bookings do not block slots of other event types#23436
fix: unconfirmed bookings do not block slots of other event types#23436bandhan-majumder wants to merge 37 commits intocalcom:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR changes booking lookup semantics used to mark reserved slots: the eventTypeId filter was removed so pending bookings are fetched across event types when the event type flags (requiresConfirmation && requiresConfirmationWillBlockSlot) apply. Repository queries were extended to include attendee-email-based pending bookings and added deduplication logic when aggregating results. No public API signatures were changed. Assessment against linked issues
Out-of-scope changes
Possibly related PRs
✨ 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. Comment |
|
@bandhan-majumder is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
bookings
|
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: |
bandhan-majumder
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: 1
📜 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)
packages/lib/server/repository/booking.ts(1 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:
packages/lib/server/repository/booking.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/server/repository/booking.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/server/repository/booking.ts
🧠 Learnings (3)
📓 Common learnings
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.805Z
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.
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
📚 Learning: 2025-08-21T13:44:06.805Z
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.805Z
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/booking.ts
📚 Learning: 2025-08-22T16:38:00.225Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23192
File: packages/lib/server/service/InsightsBookingBaseService.ts:814-816
Timestamp: 2025-08-22T16:38:00.225Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), when filtering for "accepted" bookings in getMembersStatsWithCount(), using `endTime <= now()` in the SQL condition should be avoided as it conflicts with existing date filtering logic. The components already handle completion filtering by setting `endDate: currentTime` in their query parameters, making additional SQL-level endTime filtering unnecessary and potentially problematic.
Applied to files:
packages/lib/server/repository/booking.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). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lib/server/repository/booking.ts (1)
259-277: Add tests to prevent regressions (pending host vs pending attendee).Please add integration tests for:
- Host has PENDING booking on their own event type (flags on) → other event types show slot blocked.
- Host is attendee on another organizer’s event type with flags on → slot blocked.
- Same two cases with requiresConfirmationWillBlockSlot = false → slot not blocked.
I can draft these tests if you confirm the preferred suite (e2e vs API unit) and fixtures for 30/60 min types.
📜 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)
packages/lib/server/repository/booking.ts(1 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:
packages/lib/server/repository/booking.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/server/repository/booking.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/server/repository/booking.ts
🧠 Learnings (3)
📓 Common learnings
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.805Z
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.
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
📚 Learning: 2025-08-21T13:44:06.805Z
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.805Z
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/booking.ts
📚 Learning: 2025-08-22T16:38:00.225Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23192
File: packages/lib/server/service/InsightsBookingBaseService.ts:814-816
Timestamp: 2025-08-22T16:38:00.225Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), when filtering for "accepted" bookings in getMembersStatsWithCount(), using `endTime <= now()` in the SQL condition should be avoided as it conflicts with existing date filtering logic. The components already handle completion filtering by setting `endDate: currentTime` in their query parameters, making additional SQL-level endTime filtering unnecessary and potentially problematic.
Applied to files:
packages/lib/server/repository/booking.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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/lib/server/repository/booking.ts (1)
259-277: Correctly scoped PENDING-blocking to relevant hosts; aligns with cross–event-type blocking.Good move removing the eventTypeId restriction and adding userId IN userIdAndEmailMap to avoid global over-blocking. This meets the objective while preventing runaway scans.
|
I will fix the tests once the changes are confirmed by the reviewer. |
anikdhabal
left a comment
anikdhabal
left a comment
There was a problem hiding this comment.
Pls add tests to verify the fix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/lib/server/repository/booking.ts (3)
259-278: Host-owned PENDING query: correct semantics; ensure fixtures provide matching EventType rowsThe join on eventType.requiresConfirmation && requiresConfirmationWillBlockSlot is right and matches the product requirement. Note this will return zero rows if the referenced EventType doesn’t exist or those flags aren’t true in tests/fixtures. See my test fix suggestion in booking.test.ts.
Minor: reuse a shared time-overlap object to avoid duplication.
- where: { - startTime: { lte: endDate }, - endTime: { gte: startDate }, + where: { + ...timeOverlapWhere, userId: { in: Array.from(userIdAndEmailMap.keys()), },Outside this range, define once near sharedQuery:
const timeOverlapWhere = { startTime: { lte: endDate }, endTime: { gte: startDate } } as const;
280-301: Attendee-based PENDING query: good coverage; add early return on empty mapThis correctly mirrors the accepted-attendee path for PENDING. Consider short‑circuiting if userIdAndEmailMap.size === 0 to avoid unnecessary scans.
if (userIdAndEmailMap.size === 0) return [];
320-320: Deduplicate potential PENDING overlaps across host/attendee pathsRare, but if an organizer is also listed as an attendee on the same PENDING booking, it will appear in both resultThree and resultFour. Deduplicate by id before returning.
Apply this diff:
- return [...resultOne, ...resultTwoWithOrganizersRemoved, ...resultThree, ...resultFour]; + const pendingById = new Map<number, typeof resultThree[number]>(); + for (const b of resultThree) pendingById.set(b.id, b); + for (const b of resultFour) pendingById.set(b.id, b); + return [...resultOne, ...resultTwoWithOrganizersRemoved, ...pendingById.values()];
📜 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 (2)
packages/lib/server/repository/booking.test.ts(1 hunks)packages/lib/server/repository/booking.ts(2 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:
packages/lib/server/repository/booking.test.tspackages/lib/server/repository/booking.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/server/repository/booking.test.tspackages/lib/server/repository/booking.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/server/repository/booking.test.tspackages/lib/server/repository/booking.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
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.805Z
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.
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/lib/server/repository/booking.test.ts
📚 Learning: 2025-08-21T13:44:06.805Z
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.805Z
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/booking.test.tspackages/lib/server/repository/booking.ts
📚 Learning: 2025-08-22T16:38:00.225Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23192
File: packages/lib/server/service/InsightsBookingBaseService.ts:814-816
Timestamp: 2025-08-22T16:38:00.225Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), when filtering for "accepted" bookings in getMembersStatsWithCount(), using `endTime <= now()` in the SQL condition should be avoided as it conflicts with existing date filtering logic. The components already handle completion filtering by setting `endDate: currentTime` in their query parameters, making additional SQL-level endTime filtering unnecessary and potentially problematic.
Applied to files:
packages/lib/server/repository/booking.ts
🧬 Code graph analysis (1)
packages/lib/server/repository/booking.test.ts (1)
packages/lib/server/repository/booking.ts (1)
BookingRepository(117-975)
🪛 GitHub Check: Tests / Unit
packages/lib/server/repository/booking.test.ts
[failure] 329-329: packages/lib/server/repository/booking.test.ts > _findAllExistingBookingsForEventTypeBetween > PENDING bookings with requiresConfirmationWillBlockSlot > should include PENDING bookings where user is an attendee
AssertionError: expected [] to have a length of 1 but got +0
- Expected
- Received
- 1
- 0
❯ packages/lib/server/repository/booking.test.ts:329:24
[failure] 295-295: packages/lib/server/repository/booking.test.ts > _findAllExistingBookingsForEventTypeBetween > PENDING bookings with requiresConfirmationWillBlockSlot > should include PENDING bookings where user is the host/organizer
AssertionError: expected [] to have a length of 1 but got +0
- Expected
- Received
- 1
- 0
❯ packages/lib/server/repository/booking.test.ts:295:24
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
packages/lib/server/repository/booking.ts (1)
303-308: Promise.all usage looks goodParallelizing the four queries is appropriate here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/lib/server/repository/booking.ts (4)
185-196: Remove unused eventTypeId from paramseventTypeId is commented out in destructuring but still present in the signature. Drop it to avoid confusion and dead params.
- private async _findAllExistingBookingsForEventTypeBetween({ - // eventTypeId, + private async _findAllExistingBookingsForEventTypeBetween({ seatedEvent = false, startDate, endDate, userIdAndEmailMap, }: { startDate: Date; endDate: Date; - eventTypeId?: number | null; seatedEvent?: boolean; userIdAndEmailMap: Map<number, string>; }) {
197-204: DRY the time-overlap predicateYou repeat the time window in multiple queries. Extract once to avoid drift.
}) { - const sharedQuery = { + const sharedQuery = { startTime: { lte: endDate }, endTime: { gte: startDate }, status: { in: [BookingStatus.ACCEPTED], }, }; + const timeOverlap = { startTime: { lte: endDate }, endTime: { gte: startDate } }; ... - where: { - startTime: { lte: endDate }, - endTime: { gte: startDate }, + where: { + ...timeOverlap, userId: { in: Array.from(userIdAndEmailMap.keys()), }, ... - where: { - startTime: { lte: endDate }, - endTime: { gte: startDate }, + where: { + ...timeOverlap, attendees: {Also applies to: 259-266, 282-285
280-301: Email matching may be case-sensitive; confirm normalizationattendees.some.email in [...userIdAndEmailMap.values()] can miss matches if email casing differs (Postgres comparisons are case-sensitive by default; Prisma
indoesn’t support mode: 'insensitive'). Ensure emails are lowercased at write-time and that userIdAndEmailMap values are lowercase; otherwise, lowercase the array before use.- in: Array.from(userIdAndEmailMap.values()), + in: Array.from(userIdAndEmailMap.values()).map((e) => e.toLowerCase()),If DB isn’t normalized to lowercase, this alone won’t fix it—please confirm DB/email normalization across attendees and users. I can script-check usages if helpful.
314-332: Deduplicate by booking.id across all result setsPairwise filtering only removes a subset (e.g., when organizer is also an attendee with the same email). A host-owned PENDING where another organizer is an attendee will still duplicate. Safer: merge then dedupe by id.
- const resultTwoWithOrganizersRemoved = resultTwo.filter((booking) => { - if (!booking.userId) return true; - const organizerEmail = userIdAndEmailMap.get(booking.userId); - return !booking.attendees.some((attendee) => attendee.email === organizerEmail); - }); - - // Remove duplicates from resultFour (PENDING attendee bookings) where organizer is also an attendee - const resultFourWithOrganizersRemoved = resultFour.filter((booking) => { - if (!booking.userId) return true; - const organizerEmail = userIdAndEmailMap.get(booking.userId); - return !booking.attendees.some((attendee) => attendee.email === organizerEmail); - }); - - return [ - ...resultOne, - ...resultTwoWithOrganizersRemoved, - ...resultThree, - ...resultFourWithOrganizersRemoved, - ]; + const merged = [...resultOne, ...resultTwo, ...resultThree, ...resultFour]; + const dedupedById = Array.from(new Map(merged.map((b) => [b.id, b])).values()); + return dedupedById;Optional follow-up: with id-based dedupe, you can also replace One/Two and Three/Four pairs with single OR queries each to reduce round trips.
packages/lib/server/repository/booking.test.ts (2)
258-371: Add test: organizer is attendee on someone else’s PENDING bookingCovers Query Four explicitly; current suite doesn’t validate this path.
+ it("PENDING booking where organizer is an attendee should block", async () => { + await prismaMock.eventType.create({ + data: { + id: 10, + title: "Other Host ET", + slug: "other-host-et", + userId: 2, + length: 60, + requiresConfirmation: true, + requiresConfirmationWillBlockSlot: true, + }, + }); + await prismaMock.booking.create({ + data: { + userId: 2, + uid: "pending-other-host", + eventTypeId: 10, + status: BookingStatus.PENDING, + attendees: { + create: { + email: "test1@example.com", // organizer email + noShow: false, + name: "Organizer", + timeZone: "America/Toronto", + }, + }, + startTime: new Date("2025-05-01T14:00:00.000Z"), + endTime: new Date("2025-05-01T15:00:00.000Z"), + title: "PENDING other host", + }, + }); + const bookingRepo = new BookingRepository(prismaMock); + const userIdAndEmailMap = new Map([[1, "test1@example.com"]]); + const bookings = await bookingRepo.findAllExistingBookingsForEventTypeBetween({ + startDate: new Date("2025-05-01T13:00:00.000Z"), + endDate: new Date("2025-05-01T16:00:00.000Z"), + userIdAndEmailMap, + }); + expect(bookings).toHaveLength(1); + });
258-371: Add test: duplicate path (host-owned + attendee-other-organizer) dedupes to onePrevents double-counting when a host-owned PENDING booking also lists another organizer as attendee. This will catch regressions if id-level dedupe is removed.
+ it("does not return duplicates when a host-owned PENDING also lists another organizer as attendee", async () => { + await prismaMock.eventType.create({ + data: { + id: 20, + title: "Host ET", + slug: "host-et", + userId: 1, + length: 60, + requiresConfirmation: true, + requiresConfirmationWillBlockSlot: true, + }, + }); + await prismaMock.booking.create({ + data: { + userId: 1, // organizer A (host-owned => Query Three) + uid: "pending-host-owned", + eventTypeId: 20, + status: BookingStatus.PENDING, + attendees: { + create: [ + { email: "organizer-b@example.com", name: "Organizer B", timeZone: "UTC", noShow: false }, // triggers Query Four for organizer B + ], + }, + startTime: new Date("2025-05-01T14:00:00.000Z"), + endTime: new Date("2025-05-01T15:00:00.000Z"), + title: "Host-owned with another organizer attendee", + }, + }); + const bookingRepo = new BookingRepository(prismaMock); + const userIdAndEmailMap = new Map([ + [1, "organizer-a@example.com"], + [2, "organizer-b@example.com"], + ]); + const bookings = await bookingRepo.findAllExistingBookingsForEventTypeBetween({ + startDate: new Date("2025-05-01T13:00:00.000Z"), + endDate: new Date("2025-05-01T16:00:00.000Z"), + userIdAndEmailMap, + }); + expect(bookings).toHaveLength(1); + });
📜 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 (2)
packages/lib/server/repository/booking.test.ts(1 hunks)packages/lib/server/repository/booking.ts(3 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:
packages/lib/server/repository/booking.tspackages/lib/server/repository/booking.test.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/server/repository/booking.tspackages/lib/server/repository/booking.test.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/server/repository/booking.tspackages/lib/server/repository/booking.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
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.805Z
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.
📚 Learning: 2025-08-21T13:44:06.805Z
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.805Z
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/booking.tspackages/lib/server/repository/booking.test.ts
📚 Learning: 2025-08-21T12:28:42.018Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#23247
File: packages/features/webhooks/lib/factory/WebhookPayloadFactory.ts:274-282
Timestamp: 2025-08-21T12:28:42.018Z
Learning: In webhook DTOs in packages/features/webhooks/lib/dto/types.ts, the booking fields are restricted structures containing only specific fields (id, eventTypeId, userId, and sometimes additional fields like startTime or smsReminderNumber) rather than full database booking objects, so there are no security or PII leakage concerns when using these booking objects in webhook payloads.
Applied to files:
packages/lib/server/repository/booking.ts
📚 Learning: 2025-08-21T12:28:42.018Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#23247
File: packages/features/webhooks/lib/factory/WebhookPayloadFactory.ts:274-282
Timestamp: 2025-08-21T12:28:42.018Z
Learning: In BookingPaymentInitiatedDTO and other webhook DTOs in packages/features/webhooks/lib/dto/types.ts, the booking field is a restricted structure containing only specific fields (id, eventTypeId, userId) rather than the full database booking object, so there are no security or PII leakage concerns when passing the booking object to buildEventPayload.
Applied to files:
packages/lib/server/repository/booking.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/lib/server/repository/booking.tspackages/lib/server/repository/booking.test.ts
📚 Learning: 2025-08-22T16:38:00.225Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23192
File: packages/lib/server/service/InsightsBookingBaseService.ts:814-816
Timestamp: 2025-08-22T16:38:00.225Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), when filtering for "accepted" bookings in getMembersStatsWithCount(), using `endTime <= now()` in the SQL condition should be avoided as it conflicts with existing date filtering logic. The components already handle completion filtering by setting `endDate: currentTime` in their query parameters, making additional SQL-level endTime filtering unnecessary and potentially problematic.
Applied to files:
packages/lib/server/repository/booking.tspackages/lib/server/repository/booking.test.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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (4)
packages/lib/server/repository/booking.ts (2)
259-278: Scope fix for host-owned PENDING bookings looks goodLimiting to relevant organizer userIds prevents global over-blocking. Matches the PR goal.
303-308: Including the fourth query in aggregation is correctExtending Promise.all to pick up attendee PENDING bookings completes the cross-event-type coverage.
packages/lib/server/repository/booking.test.ts (2)
258-325: Good cross-event-type PENDING coverageTest validates that a blocking Event Type A PENDING booking hides slots for other event types. Solid setup with explicit flags.
326-371: Good negative control for non-blocking event typesVerifies requiresConfirmationWillBlockSlot=false does not block. Looks correct.
|
done @anikdhabal . Added tests (and a missing filtering logic) |
| // when organizer is an attendee on a PENDING booking. | ||
| const currentBookingsAllUsersQueryFour = this.prismaClient.booking.findMany({ | ||
| where: { | ||
| startTime: { lte: endDate }, | ||
| endTime: { gte: startDate }, | ||
| attendees: { | ||
| some: { | ||
| email: { | ||
| in: Array.from(userIdAndEmailMap.values()), |
There was a problem hiding this comment.
Can you explain why we doing this?
There was a problem hiding this comment.
@anikdhabal yeah,, like if a relevant organizer is an attendee on someone else’s event type that has requiresConfirmationWillBlockSlot enabled. It sometimes duplicate the findings (as organizers are attendee in their meeting too) and for that reason, the filtering logic is added to avoid duplication here
this was suggested by coderabbit here
|
@anikdhabal @Udit-takkar can you please review this? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/eventtypes/components/tabs/advanced/RequiresConfirmationController.tsx">
<violation number="1" location="packages/features/eventtypes/components/tabs/advanced/RequiresConfirmationController.tsx:272">
The tooltip trigger is an aria-hidden, unfocusable `<Icon>` so the new help text cannot be accessed via keyboard or screen readers. Wrap the icon in a focusable control (e.g., a `<button>`/`<span tabIndex={0}>`) with an aria-label before passing it to `<Tooltip>`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
apps/web/modules/event-types/components/tabs/advanced/RequiresConfirmationController.tsx
Show resolved
Hide resolved
|
@hariombalhara Please check and merge 🙏 |
|
@keithwillcode @emrysal @hariombalhara |
|
Testing again now |
hariombalhara
left a comment
hariombalhara
left a comment
There was a problem hiding this comment.
Let's fix the unintentended translation change
|
@hariombalhara changed to prev. Sorry for that |
| @@ -566,15 +566,13 @@ export class BookingRepository { | |||
| } | |||
|
|
|||
| private async _findAllExistingBookingsForEventTypeBetween({ | |||
There was a problem hiding this comment.
nit: We should consider renaming this fn, considering that it returns only blocking bookings
sometjoing like
findAllBlockingBookingsForEventTypeBetween
hariombalhara
left a comment
hariombalhara
left a comment
There was a problem hiding this comment.
LGTM !! Tested as well.
cc @emrysal @CarinaWolli for final review as it affects availability
|
As this is getting conflicts very often, I am keeping it updated. The changes are same (current changes are accepted in conflicts). Prev reviewer @hariombalhara has already approved the changes @emrysal @keithwillcode @CarinaWolli please give it a final review |
|
Any updates on this one? |
|
@keithwillcode @anikdhabal @CarinaWolli @emrysal can i get the final review on this please? |
| }, | ||
| eventType: { | ||
| requiresConfirmation: true, | ||
| requiresConfirmationWillBlockSlot: true, |
There was a problem hiding this comment.
This will do a joined search for every booking, instead we should filter out bookings by event type settings as this will be more performant.
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
Requesting changes for the joined where query, it's suboptimal
|
@emrysal are you saying that I should fetch the pending and blocked bookings first like this: and then just search it like this? but here, first I will be searching through all the pendingAndBlockingEventTypes across the whole |
|
@emrysal please let me know if this is what it should be. I will add the change. |
github-actions
bot
added
the
🧹 Improvements
8 participants
What does this PR do?
This PR does 3 things mentioned here
What logic changed?
1.
For searching free slot, we were previously searching based on events type like this:
Logic for getting free slot
Before logic
After logic (search all the events if they have pending bookings)
note:-
here, there is a possibility that the host is a participant of a booking that is pending and requires confirmation and requires confirmation book slot enabled, so we are filtering that later on.
I have added tests for this behaviour here in this file:
packages/lib/server/repository/booking.test.tsalso have recorded the behavior #23436 (comment) in the below comment
2.
Change in troubleshooter
3.
Change in settings of Advanced section (adding a tooltip)
Dark mode
Light mode
4
There are some type changes to make eslint happy and some changes to make sure we get the properties of the event type of the booking before reaching the troubleshooter
note:
Previously it was suggested to tell the event type too in the troubleshooter heading (see #23069 (comment)), so I had added those properties
But later, this plan was changed to adding
unconfirmed but still blockingat the end of the title (see #23069 (comment)).So, i have not removed the properties which was added to get more info about the event specially to make sure we have that option ready for the trouble shooter for future..
That's it
Issues fixes
Video Demo (if applicable):
https://www.loom.com/share/b90e2247b74940d0abdfd3c5992ddb66?sid=21404455-9e7b-4eec-a4ea-d86003bc5ebd
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist