feat: consider booker availability#24309
Conversation
|
@Vibgitcode27 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
$200
bookings
WalkthroughAdds ScheduleRepository.findScheduleByUserId to fetch schedules by userId and expands TravelScheduleRepository.findTravelSchedulesByUserId selection to include userId and prevTimeZone. Strengthens user/session-related types and exposes defaultScheduleId in the public user selection. Introduces getBookerAsHostForReschedule and integrates booker-as-host availability into getAvailableSlots (with repository value imports and some catch-block changes). Changes IGetAvailableSlots.troubleshooter to unknown. Removes an inline ESLint suppression in a Checkbox component and adds extensive tests for the reschedule/booker-as-host flow. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@CodeRabbit Everything looks good? Please review the approach! |
🧩 Analysis chain🏁 Script executed: Length of output: 7112 🏁 Script executed: Length of output: 429 🏁 Script executed: Length of output: 2377 🏁 Script executed: Length of output: 381 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1693 🏁 Script executed: Length of output: 3923 @Vibgitcode27, excellent work on this PR! The approach is elegant and well-thought-out. Here's my comprehensive review: ✅ Strengths
🔍 Areas for Consideration1. First Attendee Assumptionconst bookerEmail = rescheduleBooking?.attendees[0]?.email;This assumes the first attendee is always the booker. While this pattern exists elsewhere in the codebase, consider:
2. Silent FailuresThe method returns if (!bookerEmail) return null;
if (!bookerUser) return null;
if (!bookerWithCredentials) return null;Recommendation: Add debug logging to help troubleshoot when availability isn't considered: if (!bookerEmail) {
loggerWithEventDetails.debug("No booker email found for reschedule");
return null;
}
if (!bookerUser) {
loggerWithEventDetails.debug(`Booker ${bookerEmail} is not a Cal.com user`);
return null;
}3. Test CoverageNo tests were found for this new functionality. Strongly recommend adding tests for:
4. Database Call OptimizationThere are three sequential database calls: findBookingByUid → findByEmail → findUserWithCredentialsConsider if these could be optimized with a single query or at least made parallel. 5. Edge Case: Empty SchedulesWhat if 6. Type ConsistencyThe return type explicitly lists 🎯 Overall AssessmentThe approach is solid and production-ready with minor improvements suggested above. The core logic is sound, maintainable, and solves the problem stated in the linked issues effectively. Recommended Next Steps:
Great work! The solution is elegant and well-integrated with existing code. 🚀 ✅ Actions performedReview triggered.
|
|
@CodeRabbit Thank you! I will add those tests. One more thing should I consider all the attendees(except host or team members) as bookers , because all guests will be attendees as well. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1123 🏁 Script executed: Length of output: 9486 🏁 Script executed: Length of output: 7149 🏁 Script executed: Length of output: 16405 @Vibgitcode27, excellent question! This is an important architectural decision. Let me break down the options: Current Approach (First Attendee Only)Your current implementation assumes // Ascending order ensures that the first attendee in the list is the booker and others are guests
// See why it is important https://github.com/calcom/cal.com/pull/20935
// TODO: Ideally we should return `booker` property directly from the booking
orderBy: {
id: "asc",
},This confirms it's a known workaround until a proper All-Guests ApproachIf you want to check availability for all guest attendees, you'd need to filter out hosts and team members: private async getBookersForReschedule({ rescheduleUid }: { rescheduleUid: string }) {
const rescheduleBooking = await BookingRepository.findBookingByUidWithEventType({
bookingUid: rescheduleUid,
});
if (!rescheduleBooking) return [];
const hostEmail = rescheduleBooking.user?.email;
const teamMemberEmails = rescheduleBooking.eventType?.team?.members.map(m => m.email) || [];
const excludedEmails = new Set([hostEmail, ...teamMemberEmails].filter(Boolean));
// Get all guest attendees (excluding host and team members)
const guestAttendees = rescheduleBooking.attendees.filter(
attendee => !excludedEmails.has(attendee.email)
);
// Fetch availability for all Cal.com users among guests
const bookers = await Promise.all(
guestAttendees.map(async (attendee) => {
const bookerUser = await prisma.user.findFirst({
where: { email: attendee.email },
select: { id: true, email: true, username: true },
});
if (!bookerUser) return null;
// Fetch schedules and return host-like object
// ... rest of the logic
})
);
return bookers.filter(Boolean); // Remove nulls
}RecommendationFor this PR, I'd recommend sticking with the first attendee approach because: ✅ Pros of First-Attendee Approach:
|
|
@CodeRabbit This is exactly why I stayed with the first attendee approach. I did not know it was already the existing pattern, but I figured out on my own that the first attendee is the booker. So I think my only task now is to add the tests. @Devanshusharma2005 @kart1ka @anikdhabal will be able to make the decision. |
|
Tip For best results, initiate chat on the files or code changes. @Vibgitcode27, excellent! Your intuition was spot-on 🎯. It's great that you independently identified the pattern—that shows solid analytical thinking. Next StepsAdding tests is the perfect next task. Here are some test scenarios I'd recommend covering: Core Test Cases
Test Structure Suggestiondescribe("getBookerAsHostForReschedule", () => {
it("should return booker availability when booker is Cal.com user", async () => {
// Arrange: Mock booking with Cal.com user as attendee
// Act: Call getAvailableSlots with rescheduleUid
// Assert: Verify booker treated as host with isFixed: true
});
it("should return null when booker is not Cal.com user", async () => {
// Test external guest scenario
});
it("should handle missing attendees gracefully", async () => {
// Edge case testing
});
});Feel free to ping me if you need any help structuring the tests or mocking the dependencies! Looking forward to seeing the test coverage. 🚀 |
|
@CodeRabbit I have added the tests please take a look! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 204 🏁 Script executed: Length of output: 207 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2687 🏁 Script executed: Length of output: 24036 @Vibgitcode27, outstanding work on the test coverage! 🎉 Your tests are comprehensive, well-structured, and exceed expectations. Here's my detailed review: ✅ Excellent CoverageYou've successfully covered all the scenarios I recommended and then some: Core Scenarios ✅
Bonus Coverage 🌟
💎 Strengths
🎯 Specific HighlightsComprehensive Structure Validationexpect(result?.user.schedules[0].availability[0]).not.toHaveProperty("id");
expect(result?.user.schedules[0].availability[0]).not.toHaveProperty("userId");This is excellent—you're not just checking what's present, but also what's correctly removed! Parallel Fetching Verificationexpect(ScheduleRepository.findScheduleByUserId).toHaveBeenCalledTimes(1);
expect(TravelScheduleRepository.findTravelSchedulesByUserId).toHaveBeenCalledTimes(1);Great attention to performance concerns! Integration TestThe final test verifying the structure matches what availability calculation expects shows systems-thinking! 👏 🔧 Minor Enhancement Suggestions (Optional)1. Error Handling TestIf you want to be extra thorough, you could add a test for repository exceptions: it("should handle repository errors gracefully", async () => {
const mockBooking = {
attendees: [{ email: "booker@example.com" }],
};
mockDependencies.bookingRepo.findBookingByUid.mockResolvedValue(mockBooking);
mockDependencies.userRepo.findByEmail.mockRejectedValue(new Error("Database error"));
await expect(
(availableSlotsService as any).getBookerAsHostForReschedule({
rescheduleUid: "test-uid",
})
).rejects.toThrow("Database error");
});Though this depends on whether you want error propagation or silent failures. 2. Timezone Edge CaseYou could test a scenario where booker and host are in very different timezones to ensure availability intersection works correctly—though this might be better as an integration test. 📊 Test Metrics
🚀 Ready to Ship!Your test suite is production-ready. The coverage is thorough, the structure is clean, and the assertions are comprehensive. This will make the code maintainable and give confidence that the feature works correctly. Next Steps:✅ Tests added Excellent work, and thank you for taking the testing seriously! This is exactly the kind of quality that makes a PR production-ready. 🎉 |
![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
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/slots/util.ts (1)
958-1056: Consider adding debug logging for early returns and using dependency injection consistently.The implementation correctly fetches booker availability and transforms it into the host format. However, there are two areas for improvement:
Missing debug logging: The method has multiple early-return paths (lines 1007, 1010, 1015, 1020) that return
nullwithout logging. Adding debug logs would help troubleshoot why a booker isn't being treated as a host during reschedules.Direct static repository calls: Lines 1023-1024 call
ScheduleRepository.findScheduleByUserIdandTravelScheduleRepository.findTravelSchedulesByUserIddirectly instead of going throughthis.dependencies, breaking the dependency injection pattern used elsewhere. While the tests work around this by mocking the static methods, consistency with the established pattern would improve maintainability.Suggestion 1: Add debug logging for null returns
- if (!rescheduleBooking?.attendees?.length) return null; + if (!rescheduleBooking?.attendees?.length) { + log.debug("No attendees found for reschedule booking", { rescheduleUid }); + return null; + } const bookerEmail = rescheduleBooking?.attendees[0]?.email; - if (!bookerEmail) return null; + if (!bookerEmail) { + log.debug("First attendee has no email", { rescheduleUid }); + return null; + } const bookerUser = await this.dependencies.userRepo.findByEmail({ email: bookerEmail, }); - if (!bookerUser) return null; + if (!bookerUser) { + log.debug("Booker is not a Cal.com user", { bookerEmail, rescheduleUid }); + return null; + } const bookerWithCredentials = await this.dependencies.userRepo.findUserWithCredentials({ id: bookerUser.id, }); - if (!bookerWithCredentials) return null; + if (!bookerWithCredentials) { + log.debug("Could not fetch booker credentials", { bookerId: bookerUser.id, rescheduleUid }); + return null; + }Suggestion 2: Use dependency injection for repository calls (optional)
If you choose to align with the dependency injection pattern, you would need to add
scheduleRepoandtravelScheduleRepoto the dependencies and call instance methods instead of static methods. However, since the tests already accommodate the static calls, this refactor can be deferred to a future consistency pass across the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/trpc/server/routers/viewer/slots/util.test.ts(2 hunks)packages/trpc/server/routers/viewer/slots/util.ts(7 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/trpc/server/routers/viewer/slots/util.test.tspackages/trpc/server/routers/viewer/slots/util.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/trpc/server/routers/viewer/slots/util.test.tspackages/trpc/server/routers/viewer/slots/util.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/trpc/server/routers/viewer/slots/util.test.tspackages/trpc/server/routers/viewer/slots/util.ts
🧬 Code graph analysis (2)
packages/trpc/server/routers/viewer/slots/util.test.ts (1)
packages/lib/server/repository/travelSchedule.ts (1)
TravelScheduleRepository(3-21)
packages/trpc/server/routers/viewer/slots/util.ts (2)
packages/lib/server/repository/schedule.ts (1)
ScheduleRepository(16-224)packages/lib/server/repository/travelSchedule.ts (1)
TravelScheduleRepository(3-21)
⏰ 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 (3)
packages/trpc/server/routers/viewer/slots/util.test.ts (1)
1-684: Excellent test coverage for the booker-as-host reschedule flow.The test suite comprehensively validates the new
getBookerAsHostForReschedulefunctionality with well-structured scenarios covering:
- Successful booker-as-host data retrieval with complete availability
- Default value application for missing user properties
- Multiple schedules and time zones
- The critical
isFixed: trueflag ensuring COLLECTIVE scheduling behavior- Null-return edge cases (no attendees, external users, failed lookups)
- Schedule transformation and travel schedule preservation
- Parallel fetching of schedules
- Integration with the availability calculation flow
The test organization, mocking strategy, and assertion depth align with best practices for testing complex availability logic.
packages/trpc/server/routers/viewer/slots/util.ts (2)
1156-1163: Integration point correctly appends booker to hosts array.The conditional check for
input.rescheduleUidand the subsequent push toallHostscorrectly integrates the booker-as-host data into the availability calculation flow. This ensures both host and booker availability are considered for collective scheduling during reschedules.
87-87: Good type safety improvement.Changing
troubleshooterfromanytounknownenforces type checking at usage sites and aligns with safer TypeScript practices.
| timeZone, | ||
| }); | ||
| } catch (_) { | ||
| } catch { |
There was a problem hiding this comment.
Add logging to catch blocks to aid troubleshooting.
The bare catch blocks silently swallow exceptions, which can make debugging limit-checking failures difficult in production. Consider logging the error before marking the period as busy.
Apply this diff to add logging at line 464:
- } catch {
+ } catch (error) {
+ loggerWithEventDetails.debug("Yearly booking limit check failed", { error, periodStart, unit });
limitManager.addBusyTime(periodStart, unit, timeZone);Apply this diff to add logging at line 665:
- } catch {
+ } catch (error) {
+ loggerWithEventDetails.debug("Yearly team booking limit check failed", { error, periodStart, unit });
limitManager.addBusyTime(periodStart, unit, timeZone);Also applies to: 665-665
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/slots/util.ts around lines 464 and 665,
there are bare catch blocks that silently swallow exceptions; update each to
catch the error object (e.g., catch (err)) and log it before proceeding to mark
the period as busy. Use the repository's logger (e.g., logger.error or
ctx.logger.error) to log a concise message with relevant context (user id,
period id/indices, function name) and the error object; if no logger is
available in scope, fall back to console.error. Ensure the log call happens
before the existing code that marks the period busy so the error is recorded for
troubleshooting.
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
|
This PR has been closed due to inactivity. Please feel free to reopen it if you'd like to continue the work. |
2 participants
What does this PR do?
This PR updates the rescheduling logic to check if the original booker is a Cal.com user. If they are, it fetches their availability and only shows time slots that work for both the host and the booker. This prevents reschedules to times when the booker is not available. It works regardless of who starts the reschedule, by checking for the
rescheduleUidVideo Demo (if applicable):
Cal16378.mp4
Mandatory Tasks (DO NOT REMOVE)
Checklist