fix: support @cal.diy iCalUIDs in calendar sync#29581
Conversation
|
Welcome to Cal.diy, @Prateet-Github! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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/features/calendar-subscription/lib/sync/__tests__/CalendarSyncService.test.ts (1)
160-208: ⚡ Quick winAdd one explicit legacy
@cal.comregression test to protect backward compatibility.Current updates cover
@cal.diy(including mixed case), but there is no direct assertion that legacy@cal.comUIDs are still processed. Adding one focused test here would lock in objective (2) from this PR.Suggested test addition
describe("handleEvents", () => { + test("should still process legacy `@cal.com` events", async () => { + const legacyCalComEvent: CalendarSubscriptionEventItem = { + ...mockCalComEvent, + iCalUID: "legacy-booking-uid@cal.com", + }; + + mockBookingRepository.findBookingByUidWithEventType = vi.fn().mockResolvedValue({ + ...mockBooking, + uid: "legacy-booking-uid", + }); + + await service.handleEvents(mockSelectedCalendar, [legacyCalComEvent]); + + expect(mockBookingRepository.findBookingByUidWithEventType).toHaveBeenCalledWith({ + bookingUid: "legacy-booking-uid", + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/features/calendar-subscription/lib/sync/__tests__/CalendarSyncService.test.ts` around lines 160 - 208, The test suite for handleEvents covers `@cal.diy` domain events and mixed case variants, but lacks coverage for legacy `@cal.com` domain-based iCalUIDs. Add a new test case in the handleEvents describe block (similar to the "should handle mixed case iCalUID" test) that creates a CalendarSubscriptionEventItem with an iCalUID using the legacy `@cal.com` domain (e.g., "test-booking-uid@cal.com"), mocks the booking repository to return a valid booking, calls service.handleEvents with this event, and asserts that mockBookingRepository.findBookingByUidWithEventType is called with the correctly extracted booking UID to ensure backward compatibility is maintained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/features/calendar-subscription/lib/sync/CalendarSyncService.ts`:
- Around line 45-49: The inline comment on Line 45 in the CalendarSyncService
filter states "only process cal.com calendar events" but the actual code filters
for both "`@cal.com`" and "`@cal.diy`" domains on Line 48, making the comment stale
and misleading. Either remove the comment entirely since the filter logic is
self-explanatory, or replace it with a "why" comment that explains the business
reason for filtering by these specific calendar domains (e.g., "filter to only
internal cal.com and cal.diy calendar events"). Avoid "what" comments that just
restate the code logic.
---
Nitpick comments:
In
`@packages/features/calendar-subscription/lib/sync/__tests__/CalendarSyncService.test.ts`:
- Around line 160-208: The test suite for handleEvents covers `@cal.diy` domain
events and mixed case variants, but lacks coverage for legacy `@cal.com`
domain-based iCalUIDs. Add a new test case in the handleEvents describe block
(similar to the "should handle mixed case iCalUID" test) that creates a
CalendarSubscriptionEventItem with an iCalUID using the legacy `@cal.com` domain
(e.g., "test-booking-uid@cal.com"), mocks the booking repository to return a
valid booking, calls service.handleEvents with this event, and asserts that
mockBookingRepository.findBookingByUidWithEventType is called with the correctly
extracted booking UID to ensure backward compatibility is maintained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba0cc1cd-aeb1-4ca8-bfa6-3d8990789563
📒 Files selected for processing (2)
packages/features/calendar-subscription/lib/sync/CalendarSyncService.tspackages/features/calendar-subscription/lib/sync/__tests__/CalendarSyncService.test.ts
|
please fix the coderabbit suggestion above , making it draft until then |
Prateet-Github
force-pushed
the
fix/calendar-sync-icaluid
branch
from
2aed2d8 to
4fac205
Compare
Fixed the CodeRabbit suggestion by removing the outdated comment above the iCalUID filter and pushed the update. Thanks for the review! |
|
|
3 participants
What does this PR do?
Fixes #29575
Calendar subscription sync currently only processes events whose iCalUID ends with @cal.com.
In default Cal.diy deployments, booking iCalUIDs are generated using the configured app name and commonly use the @Cal.diy suffix. Because of the strict @cal.com filter, valid Cal.diy booking events were ignored during calendar subscription sync and did not trigger booking cancellation or rescheduling workflows.
This PR updates the calendar sync filter to recognize both:
while continuing to ignore external calendar events.
Additionally, regression tests were added to verify support for:
Visual Demo (For contributors especially)
N/A – backend-only change with no UI impact.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Run:
yarn test packages/features/calendar-subscription/lib/sync/tests/CalendarSyncService.test.ts
Verify all tests pass successfully.
Environment Variables
No additional environment variables are required.
Minimal Test Data
Valid iCalUID examples:
Invalid/external example:
Expected Behavior
Events with:
should be processed by CalendarSyncService.
Events from external providers should continue to be ignored.
Checklist