fix: weight filter should never return length=0#23011
Merged
CarinaWolli merged 1 commit intomainfrom Aug 11, 2025
Merged
Conversation
Contributor
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~9 minutes ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
bookings
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/11/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/11/25)1 label was added to this PR based on Keith Williams's automation. |
emrysal
approved these changes
Aug 11, 2025
Contributor
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
Looks good, makes a ton of sense.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts (1)
725-733: Minor: use consistent metadata key source for video appEarlier tests use appStoreMetadata.dailyvideo.dirName. Keeping this consistent reduces test fragility if the dirName changes.
- mockSuccessfulVideoMeetingCreation({ - metadataLookupKey: "dailyvideo", + mockSuccessfulVideoMeetingCreation({ + metadataLookupKey: appStoreMetadata.dailyvideo.dirName,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/features/bookings/lib/handleNewBooking.ts(2 hunks)packages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts(1 hunks)
🧰 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/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.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/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts
🧠 Learnings (2)
📓 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: 2025-07-22T11:42:47.623Z
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/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts
🧬 Code Graph Analysis (2)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/lib/constants.ts (1)
DEFAULT_GROUP_ID(234-234)
packages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts (3)
packages/platform/libraries/index.ts (2)
handleNewBooking(35-35)SchedulingType(30-30)apps/web/test/utils/bookingScenario/bookingScenario.ts (10)
getBooker(2178-2192)getOrganizer(1496-1552)TestData(1215-1487)getGoogleCalendarCredential(1168-1176)Timezones(287-291)createBookingScenario(954-985)getScenarioData(1554-1640)mockSuccessfulVideoMeetingCreation(2022-2040)mockCalendarToHaveNoBusySlots(1901-1914)getDate(1069-1116)apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.ts (1)
getMockRequestDataForBooking(39-53)
🔇 Additional comments (2)
packages/features/bookings/lib/handleNewBooking.ts (2)
49-49: Good call: import DEFAULT_GROUP_ID for consistent semanticsCentralizing the default group key via DEFAULT_GROUP_ID avoids magic strings and keeps logic consistent across modules.
881-886: Alignment confirmed — retain truthy fallback
ThegroupHostsByGroupIdimplementation and the existing filter both use a truthy check (host.groupIdvs.!host.groupId), which means empty strings,null, andundefinedall fall back toDEFAULT_GROUP_ID. Switching the filter to a nullish check (host.groupId == null) would exclude hosts with an empty‐stringgroupIdeven though they’re grouped intoDEFAULT_GROUP_ID. Since the code and tests don’t distinguish empty strings from other falsy values, the current check (!host.groupId) is correct and should remain unchanged.Likely an incorrect or invalid review comment.
Comment on lines
+634
to
+645
| // The bug fix ensures that hosts without groupId are handled properly in the grouping logic | ||
| // Currently only hosts with explicit groups are being selected (this test verifies the current behavior) | ||
| expect(createdBooking.luckyUsers).toBeDefined(); | ||
| expect(createdBooking.luckyUsers).toHaveLength(1); | ||
|
|
||
| // Verify that the selected user is from the specific-group (the only properly grouped host) | ||
| const selectedUserIds = createdBooking.luckyUsers; | ||
| const specificGroupUserId = teamMembers[0].id; // teamMember[0] is in "specific-group" | ||
|
|
||
| // Check that the selected user is from specific-group | ||
| expect(selectedUserIds.includes(specificGroupUserId)).toBe(true); | ||
|
|
Contributor
There was a problem hiding this comment.
Fix flaky assertion and misleading comment in “hosts without groupId” test
The test comments assume a “specific-group” and assert a specific member is selected, but no explicit group is defined and selection can be non-deterministic. Assert membership among the candidates instead, and verify organizer assignment to the selected user.
Apply:
- // The bug fix ensures that hosts without groupId are handled properly in the grouping logic
- // Currently only hosts with explicit groups are being selected (this test verifies the current behavior)
+ // Hosts without groupId should be treated as belonging to DEFAULT_GROUP_ID and be eligible for selection
expect(createdBooking.luckyUsers).toBeDefined();
expect(createdBooking.luckyUsers).toHaveLength(1);
- // Verify that the selected user is from the specific-group (the only properly grouped host)
- const selectedUserIds = createdBooking.luckyUsers;
- const specificGroupUserId = teamMembers[0].id; // teamMember[0] is in "specific-group"
-
- // Check that the selected user is from specific-group
- expect(selectedUserIds.includes(specificGroupUserId)).toBe(true);
+ // Verify that the selected user is one of the available hosts and organizer matches it
+ const [selectedUserId] = createdBooking.luckyUsers;
+ const allUserIds = teamMembers.map((m) => m.id);
+ expect(allUserIds).toContain(selectedUserId);
+ expect(createdBooking.userId).toBe(selectedUserId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The bug fix ensures that hosts without groupId are handled properly in the grouping logic | |
| // Currently only hosts with explicit groups are being selected (this test verifies the current behavior) | |
| expect(createdBooking.luckyUsers).toBeDefined(); | |
| expect(createdBooking.luckyUsers).toHaveLength(1); | |
| // Verify that the selected user is from the specific-group (the only properly grouped host) | |
| const selectedUserIds = createdBooking.luckyUsers; | |
| const specificGroupUserId = teamMembers[0].id; // teamMember[0] is in "specific-group" | |
| // Check that the selected user is from specific-group | |
| expect(selectedUserIds.includes(specificGroupUserId)).toBe(true); | |
| // Hosts without groupId should be treated as belonging to DEFAULT_GROUP_ID and be eligible for selection | |
| expect(createdBooking.luckyUsers).toBeDefined(); | |
| expect(createdBooking.luckyUsers).toHaveLength(1); | |
| // Verify that the selected user is one of the available hosts and organizer matches it | |
| const [selectedUserId] = createdBooking.luckyUsers; | |
| const allUserIds = teamMembers.map((m) => m.id); | |
| expect(allUserIds).toContain(selectedUserId); | |
| expect(createdBooking.userId).toBe(selectedUserId); |
Contributor
E2E results are ready! |
ThyMinimalDev
pushed a commit
that referenced
this pull request
Aug 11, 2025
Co-authored-by: CarinaWolli <wollencarina@gmail.com>
ThyMinimalDev
added a commit
that referenced
this pull request
Aug 11, 2025
…ice class with DI (#22974) * refactor: convert findQualifiedHostsWithDelegationCredentials to service class with DI - Create QualifiedHostsService class following UserAvailabilityService pattern - Add IQualifiedHostsService interface with prisma and bookingRepo dependencies - Create DI module and container for qualified hosts service - Update filterHostsBySameRoundRobinHost to accept prisma as parameter - Update all usage sites to use the new service: - loadAndValidateUsers.ts - slots/util.ts - test mocks in _post.test.ts - Maintain backward compatibility with original function export - Fix type issues in team properties (rrResetInterval, rrTimestampBasis) Co-Authored-By: morgan@cal.com <morgan@cal.com> * fix: update filterHostsBySameRoundRobinHost test to include prisma parameter - Add missing prisma parameter to all test function calls - Resolves unit test failure caused by function signature change Co-Authored-By: morgan@cal.com <morgan@cal.com> * fix: resolve type issues in FilterHostsService - Import PrismaClient type instead of using unknown - Fix type compatibility for BookingRepository constructor - Update test mocks to use proper BookingRepository type - Ensure all DI dependencies are properly typed Co-Authored-By: morgan@cal.com <morgan@cal.com> * refactor: rename DI files to CamelCase and update imports - Rename all files in packages/lib/di from kebab-case to CamelCase - Update 22 external files with import statements to use new file names - Update internal DI module files with corrected imports - Maintain consistency with TypeScript naming conventions Co-Authored-By: morgan@cal.com <morgan@cal.com> * chore: bump platform libs * chore: bump platform libs * fix: remove obsolete vitest mock after service class refactoring - Remove obsolete mock for old function module - Keep correct mock for new DI container - Resolves CI unit test failures Co-Authored-By: morgan@cal.com <morgan@cal.com> * fix: correct import path for calAIPhone zod-utils module Co-Authored-By: morgan@cal.com <morgan@cal.com> * fix: Booker active booking limit can't be switched off (#23005) * refactor: Get rid of `getServerSideProps` for /getting-started pages (#23003) * refactor * fix type check * fix: Remove Reporting page within Routing Forms (#22990) * fix error in handleNewBooking (#23011) Co-authored-by: CarinaWolli <wollencarina@gmail.com> * Documentation edits made through Mintlify web editor (#23007) Co-authored-by: mintlify[bot] <109931778+mintlify[bot]@users.noreply.github.com> * fix: Contact support button position changed from absolute to fixed (#23002) * feat: Add private links to API (#22943) * --init * address change requests * adding further changes * address feedback * further changes * further clean-up * clean up * fix module import and others * add guards * remove unnecessary comments * remove unnecessary comments * cleanup * sort coderabbig suggestions * improve check * chore: bump platform libraries --------- Co-authored-by: Lauris Skraucis <lauris.skraucis@gmail.com> Co-authored-by: supalarry <laurisskraucis@gmail.com> * chore: release v5.5.15 * chore: bump platform libs * chore: bump platform libs --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: morgan@cal.com <morgan@cal.com> Co-authored-by: Anik Dhabal Babu <81948346+anikdhabal@users.noreply.github.com> Co-authored-by: Benny Joo <sldisek783@gmail.com> Co-authored-by: Sahitya Chandra <sahityajb@gmail.com> Co-authored-by: Carina Wollendorfer <30310907+CarinaWolli@users.noreply.github.com> Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: mintlify[bot] <109931778+mintlify[bot]@users.noreply.github.com> Co-authored-by: Ayush Kumar <kumarayushkumar@protonmail.com> Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com> Co-authored-by: Lauris Skraucis <lauris.skraucis@gmail.com> Co-authored-by: supalarry <laurisskraucis@gmail.com> Co-authored-by: emrysal <me@alexvanandel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes "weight filter should never return length=0" error introduced by RR groups.