fix: Manual RR reassign location change - Cal Video/Zoom/GoogleMeet to Zoom/Google Meet/Cal Video#22840
Conversation
WalkthroughA new BookingLocationService class centralizes logic for determining booking locations based on organizer and event-type metadata, supporting static and dynamic conferencing apps and returning optional conference credential IDs. roundRobinManualReassignment was refactored to use this service, propagating a conferenceCredentialId into reschedule flows and updating booking locations when organizers change. New unit tests cover BookingLocationService and expanded reassignment tests validate location selection, credential extraction, and error handling. Minor unrelated edits include an exported CalVideoLocationType constant, an explicit typing tweak, and OAuth redirect-URI constant updates. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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
CodeRabbit Configuration File (
|
keithwillcode
added
core
hariombalhara
changed the title
hariombalhara
force-pushed
the
fix-booking-reassign-location-change
branch
from
0ea3c7b to
4603c9c
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
hariombalhara
force-pushed
the
fix-booking-reassign-location-change
branch
from
4603c9c to
c1b0737
Compare
E2E results are ready! |
hariombalhara
force-pushed
the
fix-booking-reassign-location-change
branch
from
8ced440 to
d04a16c
Compare
hariombalhara
changed the title
hariombalhara
changed the title
dosubot
bot
added
bookings
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/04/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/app-store/webex/api/add.ts (1)
27-31: Inconsistent redirect-URI encoding between “add” and “callback” handlersHere the raw string is passed to
querystring.stringify, which already performs percent-encoding.
Incallback.tsthe same URI is wrapped inencodeURIbefore being sent throughURLSearchParams, causing different (and potentially double-encoded) behaviour.No change needed in this file, but align both handlers by removing the superfluous
encodeURIincallback.ts(see suggested diff there).packages/app-store/webex/api/callback.ts (1)
18-23: Drop redundantencodeURIto avoid double-encoding
URLSearchParamsencodes each value internally, so pre-encoding the URI is unnecessary and can lead to an over-encodedredirect_uri.- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
76-78: Simplify the find condition.The condition
result.type.includes("_video") && result.type === "daily_video"is redundant. Ifresult.type === "daily_video", it will always include "_video".- const calVideoResult = results.find( - (result) => result.type.includes("_video") && result.type === "daily_video" - ); + const calVideoResult = results.find((result) => result.type === "daily_video");packages/lib/server/service/bookingLocationService.test.ts (1)
459-476: Consider making the test more explicit about behavior.The comment on line 466 mentions that
getLocationValueForDBpicks the last matching credential due toforEach. This behavior dependency makes the test fragile if the implementation changes.Consider adding a more explicit test name or assertion comment to clarify that this tests the "last-one-wins" behavior when multiple locations have the same type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/app-store/webex/api/callback.tspackages/app-store/locations.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.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/app-store/webex/api/callback.tspackages/app-store/locations.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/callback.tspackages/app-store/webex/api/add.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the multipleprivatelinkscontroller component (packages/features/eventtypes/components/multiplepri...
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the `currentLink.maxUsageCount ?? 1` fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
🧬 Code Graph Analysis (4)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
⏰ Context from checks skipped due to timeout of 90000ms. 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 (15)
packages/app-store/webex/api/callback.ts (1)
3-4: LGTM – consistent constant importSwitching to
WEBAPP_URL_FOR_OAUTHmatches the change in the “add” endpoint and keeps the OAuth flow consistent.packages/app-store/locations.ts (3)
1-3: Good documentation of technical debt.The TODO comment clearly indicates the need for future consolidation work and test coverage. This aligns well with the introduction of
BookingLocationService.
64-64: Good abstraction for Cal Video location type.Creating a semantic alias
CalVideoLocationTypeimproves code readability while maintaining backward compatibility withDailyLocationType.
384-384: Good type safety improvement.Explicitly typing
conferenceCredentialIdasnumber | undefinedimproves type safety and code clarity.packages/features/ee/round-robin/roundRobinManualReassignment.ts (3)
158-172: Good refactoring to use BookingLocationService.The migration to
BookingLocationService.getLocationForHostproperly centralizes the location determination logic and correctly handles both booking location and conference credential ID extraction.
223-233: Good handling of credential ID extraction for unchanged organizer.The code correctly extracts
conferenceCredentialIdeven when the organizer hasn't changed, ensuring the credential ID is properly propagated in all reassignment scenarios.
306-306: Correct propagation of conferenceCredentialId.The credential ID is properly included in the CalendarEvent, with appropriate null-to-undefined conversion for API compatibility.
packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent test coverage!The test suite is comprehensive, well-structured, and covers all major scenarios including edge cases. The use of mocks is appropriate and the test organization follows best practices.
packages/lib/server/service/bookingLocationService.ts (1)
1-247: Excellent service implementation!The
BookingLocationServiceis well-designed with:
- Clear separation of concerns
- Comprehensive JSDoc documentation
- Strong TypeScript typing with discriminated unions
- Proper error handling with null returns
- Follows the Service naming convention as required
This successfully centralizes the booking location logic as intended by the PR objectives.
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (6)
20-29: LGTM! Clean mock setupThe new imports and mock configuration follow the established patterns correctly.
30-50: Well-structured type definitionsThe type definitions are clear and properly structured, providing good type safety for the test data.
52-137: Excellent test data builder implementationThe factory pattern implementation significantly improves test maintainability and readability. The builders provide sensible defaults while allowing flexible overrides, reducing code duplication across tests.
161-234: Comprehensive mock implementation for EventManagerThe mock helpers provide excellent coverage for different conferencing scenarios including static links, dynamic conferencing apps, and error cases. The implementation correctly simulates real-world behavior.
Note: The
eslint-disable-next-line @typescript-eslint/no-explicit-anycomments are justified here due to the need to mock private methods.
239-526: Thorough test coverage for core reassignment scenariosThe refactored tests effectively utilize the new test builders and provide comprehensive coverage for:
- Basic reassignment with workflow updates
- Conference credential ID propagation
- Fixed host scenarios
- Cancellation email handling
The tests properly verify both the integration points (EventManager calls) and the resulting state changes.
528-875: Excellent test coverage for location change scenariosThis new test suite directly addresses the PR objectives by thoroughly testing location updates during round-robin reassignment. The tests cover:
- Dynamic conferencing app transitions (Google Meet ↔ Zoom)
- Static video link handling
- Fallback behavior when no default app is configured
- Error handling for failed conferencing setup
The test structure with dedicated helpers (
createConferencingUsers,mockAppStore) makes the tests maintainable and easy to understand.
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
packages/app-store/webex/api/add.ts (1)
26-33: State parameter is an empty string – defeats CSRF protectionOAuth best-practice requires a non-guessable
statevalue that is verified on the callback.
decodeOAuthStateis called later but an empty string means no verification happens. At minimum generate a random nonce and persist it in the session:+import crypto from "crypto"; ... - const params = { + const state = crypto.randomBytes(16).toString("hex"); + const params = { response_type: "code", client_id, redirect_uri: encodeURI( `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback` ), - scope: "spark:kms meeting:schedules_read meeting:schedules_write", //should be "A space-separated list of scopes being requested by your integration" - state: "", + scope: + "spark:kms meeting:schedules_read meeting:schedules_write", // space-separated list of scopes + state, };Remember to save
state(e.g.req.session.oauthState = state) and validate it insidedecodeOAuthState.packages/app-store/webex/api/callback.ts (1)
20-26: Potential double-encoding ofredirect_uriHere the redirect URI is first wrapped with
encodeURIand then passed throughURLSearchParams, which internally appliesencodeURIComponent. That results in%25sequences (e.g.https%3A%2F%2F…becomeshttps%253A%252F%252F…).
The authorization server usually rejects such double-encoded values.Options:
- Drop the manual
encodeURIand rely onURLSearchParams:- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;
- Keep the manual encode here and in the
/addendpoint but pass the already-encoded value toURLSearchParamsusing the second argumentencode: false(node-18 feature) ― less portable.Given the simplicity, option 1 is recommended; it also resolves the inconsistency highlighted in
add.ts.
🧹 Nitpick comments (2)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (2)
320-320: Fix typo in test descriptionThere's a duplicate word "with" in the test description.
- test("should include conferenceCredentialId when reassigning booking with with fixed host as organizer", async () => { + test("should include conferenceCredentialId when reassigning booking with fixed host as organizer", async () => {
675-675: Remove commented codeThis commented line should be removed as it doesn't provide any value.
- // (await mockGetAppFromSlug()).mockReturnValue(undefined); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/app-store/webex/api/add.tspackages/app-store/webex/api/callback.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.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/app-store/webex/api/add.tspackages/app-store/webex/api/callback.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/add.tspackages/app-store/webex/api/callback.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: applies to **/*.{service,repository}.ts : avoid dot-suffixes like `.service.ts` or `.repository.ts` ...
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 **/*.{service,repository}.ts : Avoid dot-suffixes like `.service.ts` or `.repository.ts` for new files; reserve `.test.ts`, `.spec.ts`, `.types.ts` for their specific purposes
Applied to files:
packages/lib/server/service/bookingLocationService.test.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
🧬 Code Graph Analysis (4)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
⏰ Context from checks skipped due to timeout of 90000ms. 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 (16)
packages/app-store/webex/api/add.ts (1)
4-4: Constant swap looks correctChanging to
WEBAPP_URL_FOR_OAUTHaligns the file with the new OAuth-specific constant already used elsewhere. No further action needed here.packages/app-store/webex/api/callback.ts (1)
3-3: Consistent constant usage – good catchSwitching to
WEBAPP_URL_FOR_OAUTHkeeps both endpoints in sync.packages/app-store/locations.ts (3)
1-3: Good practice to track technical debtAdding this TODO comment helps track the need for future consolidation and test coverage.
64-64: Good alias for improved readabilityThe
CalVideoLocationTypealias provides a clearer, more descriptive name while maintaining backward compatibility.
384-384: Improved type safety with explicit typingMaking the type explicit as
number | undefinedimproves code clarity and type safety.packages/features/ee/round-robin/handleRescheduleEventManager.ts (2)
76-88: Good error handling for video conferencing failuresThis error handling appropriately catches Cal Video creation failures and provides clear feedback that the meeting was rescheduled even though the video link failed. This aligns with the PR's objective of improving conferencing app reliability.
169-169: Minor formatting improvementpackages/features/ee/round-robin/roundRobinManualReassignment.ts (4)
30-30: Proper import of the new serviceThe BookingLocationService import is correctly placed and follows the project structure.
158-172: Excellent refactoring using the centralized serviceThe refactoring properly centralizes the location determination logic using
BookingLocationService.getLocationForHost. This addresses the PR objective of correctly updating locations during reassignment and ensures proper handling of conference credentials.
223-234: Good addition to handle credential extraction without organizer changeThis logic ensures that
conferenceCredentialIdis properly extracted even when the organizer hasn't changed, addressing the missing credential issue mentioned in the PR objectives.
306-306: Proper propagation of conference credential IDAdding
conferenceCredentialIdto the CalendarEvent ensures proper credential handling throughout the booking flow.packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent comprehensive test coverageThe test file provides thorough coverage of all BookingLocationService methods with well-structured tests covering:
- Dynamic and static conferencing apps
- Edge cases and error scenarios
- Proper mocking and test isolation
- Clear, descriptive test names
This level of test coverage ensures the service's reliability.
packages/lib/server/service/bookingLocationService.ts (4)
1-87: Well-structured type definitions with excellent documentationThe type definitions are comprehensive and use discriminated unions effectively for type safety. The inline documentation clearly explains the purpose and usage of each type.
107-140: Robust implementation with proper error handlingThe method safely handles metadata parsing and gracefully returns null for invalid cases. Good separation of concerns between static and dynamic conferencing apps.
153-212: Well-designed location determination logicThe method implements a clear fallback chain and properly respects event type restrictions. Good reuse of
getLocationDetailsFromTypefor consistent credential extraction.
236-246: Clean abstraction with consistent return valuesThe method provides a clean API by wrapping
getLocationValueForDBand ensuring consistent null values instead of undefined.
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/app-store/webex/api/add.ts (1)
26-30: Double-check redirect-URI encoding parity between auth & token calls
In this request theredirect_urivalue is passed toquerystring.stringifywithout prior encoding, while the token-exchange step (callback.tsLine 20) pre-encodes the same value withencodeURIbeforeURLSearchParamsapplies its own encoding.
AlthoughencodeURIcurrently leaves the string unchanged (no spaces/UTF-8 chars), the two-step encoding in the token call could drift from the value used here if the path ever contains unsafe characters, breaking the OAuth “exact match” requirement.Consider removing the redundant
encodeURIin the token exchange for symmetry (preferred) or applying the same call here — but avoid a situation where one side is double-encoded and the other is not.- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;packages/app-store/webex/api/callback.ts (1)
20-20: RedundantencodeURImay lead to silent drift
URLSearchParamsalready performs percent-encoding on values; wrapping the URL inencodeURIbeforehand adds no benefit for ASCII paths and risks double-encoding if the base constant ever contains encoded characters. Removing it simplifies the flow and guarantees the raw string matches the value sent in the initial authorization request.- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
76-88: Simplify redundant condition in Cal Video result checkThe condition
result.type.includes("_video") && result.type === "daily_video"is redundant since checking for exact equality to "daily_video" already ensures it includes "_video".const calVideoResult = results.find( - (result) => result.type.includes("_video") && result.type === "daily_video" + (result) => result.type === "daily_video" );packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (2)
94-96: Consider extracting date calculationThe date calculation is repeated in the test. Consider extracting it to a helper or passing it as a parameter for better reusability.
320-320: Fix grammatical error in test descriptionThe test description has a duplicate "with" word.
- test("should include conferenceCredentialId when reassigning booking with with fixed host as organizer", async () => { + test("should include conferenceCredentialId when reassigning booking with fixed host as organizer", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/app-store/webex/api/callback.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.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/app-store/webex/api/callback.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/callback.tspackages/app-store/webex/api/add.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: applies to **/*.{service,repository}.ts : avoid dot-suffixes like `.service.ts` or `.repository.ts` ...
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 **/*.{service,repository}.ts : Avoid dot-suffixes like `.service.ts` or `.repository.ts` for new files; reserve `.test.ts`, `.spec.ts`, `.types.ts` for their specific purposes
Applied to files:
packages/lib/server/service/bookingLocationService.test.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
🧬 Code Graph Analysis (5)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.test.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
🔇 Additional comments (13)
packages/app-store/webex/api/add.ts (1)
4-4: Consistent constant usage looks good
Switching toWEBAPP_URL_FOR_OAUTHaligns this endpoint with the new OAuth-specific base URL introduced in@calcom/lib/constants.packages/app-store/webex/api/callback.ts (1)
3-3: Import update acknowledged
UsingWEBAPP_URL_FOR_OAUTHkeeps this file consistent with the auth-step change.packages/app-store/locations.ts (2)
1-3: Good documentation of technical debtThe TODO comment clearly indicates the planned consolidation with BookingLocationService, which aligns with the PR's goal of centralizing location management logic.
64-64: Good improvements to type safety and exports
- Exporting
CalVideoLocationTypeprovides a clearer semantic name for the Daily location type- Explicit typing of
conferenceCredentialIdasnumber | undefinedimproves type safetyAlso applies to: 384-384
packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
169-169: Good formatting improvementAdding a blank line before cloning references improves code readability by separating logical sections.
packages/features/ee/round-robin/roundRobinManualReassignment.ts (2)
159-172: Excellent integration of BookingLocationServiceThe implementation properly:
- Determines event type characteristics (managed/team)
- Uses the centralized service for location determination
- Handles both static links and dynamic conferencing apps correctly
224-233: Good handling of conferenceCredentialId extractionProperly extracts the conferenceCredentialId even when the organizer hasn't changed, ensuring consistent handling across all reassignment scenarios.
packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent test coverageThe test suite provides comprehensive coverage for all BookingLocationService methods with:
- Well-organized test structure
- Thorough edge case testing
- Proper mock setup and cleanup
Consider adding a test case for when
getLocationValueForDBreturns an empty string (triggering the fallback to DailyLocationType), though this might be an edge case handled internally.packages/lib/server/service/bookingLocationService.ts (1)
1-247: Well-designed service implementationThe BookingLocationService is excellently implemented with:
- Clear separation of concerns between the three methods
- Comprehensive JSDoc documentation with examples
- Proper null handling and defensive programming
- Strong type safety with well-defined interfaces
The service successfully centralizes the booking location logic as intended by the PR objectives.
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (4)
30-50: Well-structured type definitions for test dataThe type definitions provide good type safety and clarity for the test data structures.
139-159: Clean assertion helper for EventManager verificationGood abstraction that makes test assertions more readable and maintainable.
177-229: Comprehensive mocking of EventManager reschedule behaviorExcellent mock implementation that covers various scenarios including static links, different conferencing apps, and failure cases. This provides realistic test conditions.
528-875: Excellent test coverage for location change scenariosThe new test suite provides comprehensive coverage of location change functionality during round robin reassignment. The tests cover:
- Different conferencing app transitions
- Fallback behavior
- Static video links
- Error handling
The helper functions are well-structured and promote test maintainability.
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
hariombalhara
force-pushed
the
fix-booking-reassign-location-change
branch
from
d04a16c to
bb6367b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (1)
844-849: Fix duplicate event type ID.Event type ID 3 is used in multiple tests, which could cause conflicts or confusion. Each test should use unique IDs to avoid potential issues.
- id: 3, + id: 5,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/features/ee/round-robin/handleRescheduleEventManager.ts
- packages/features/ee/round-robin/roundRobinManualReassignment.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/features/ee/round-robin/roundRobinManualReassignment.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/features/ee/round-robin/roundRobinManualReassignment.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. 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 (5)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (5)
20-137: Excellent refactoring with structured test data builders.The introduction of factory functions for test data creation significantly improves test maintainability and readability. The type definitions are well-structured, and the builders follow good patterns with sensible defaults and flexible overrides.
139-234: Well-designed mock and assertion helpers.The helper functions provide excellent abstractions that make the tests more readable and maintainable. The
mockEventManagerReschedulefunction particularly well simulates real-world conferencing scenarios including failure cases, which is crucial for comprehensive testing.
239-526: Comprehensive test coverage with improved structure.The refactored tests maintain excellent coverage of the reassignment functionality while benefiting from the new structured approach. The tests properly verify all aspects including workflow updates, conference credentials, and email notifications.
528-669: Excellent coverage for location change scenarios.This new test suite directly addresses the core issue described in the PR objectives. The tests comprehensively verify that location updates work correctly when reassigning between hosts with different default conferencing apps, which was the main problem being solved.
831-874: Proper error handling test for conferencing failures.This test correctly verifies that the system handles conferencing setup failures gracefully while still completing the meeting rescheduling process. The error message assertion ensures proper user feedback in failure scenarios.
|
This PR is being marked as stale due to inactivity. |
5 participants
What does this PR do?
Tech Debt Payment
Fixes PRI-298
Visual Demo (For contributors especially)
Before After loom - https://www.loom.com/share/342131fed0f6447383801b81a2279c7d
Mandatory Tasks (DO NOT REMOVE)