feat: update recording, transcript endpoint and add tests #16
Conversation
📝 WalkthroughWalkthroughThe changes introduce stricter authentication and authorization controls to booking endpoints for recordings and transcripts, remove the public mark-absent endpoint, eliminate informational messages from responses, and activate corresponding authorization tests. Output types are updated to remove optional message fields from the API responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🤖 Fix all issues with AI agents
In `@apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts`:
- Around line 226-232: The getBookingRecordings handler is assigning a Promise
to data because it calls the async method getRecordings without awaiting it;
update the getBookingRecordings method to await
this.calVideoService.getRecordings(bookingUid) (same pattern as getTranscripts)
so recordings holds the resolved array before returning the
GetBookingRecordingsOutput response.
- Around line 236-238: The guards for the transcripts endpoint are ordered
incorrectly: change the `@UseGuards` invocation that currently lists
BookingPbacGuard, ApiAuthGuard, BookingUidGuard to the consistent order
ApiAuthGuard, BookingUidGuard, BookingPbacGuard so that ApiAuthGuard establishes
the user context before BookingPbacGuard runs; update the `@UseGuards` decorator
on the transcripts controller method (the one annotated with
`@Pbac`(["booking.readRecordings"]) and `@Permissions`([BOOKING_READ])) to use
ApiAuthGuard, BookingUidGuard, BookingPbacGuard.
🧹 Nitpick comments (1)
packages/platform/types/bookings/2024-08-13/outputs/get-booking-recordings.output.ts (1)
6-6: Unused type import.
BookingRepositoryis imported but never used in this file. Remove this dead import.Proposed fix
-import type { BookingRepository } from "@calcom/features/bookings/lib/BookingRepository";
| async getBookingRecordings(@Param("bookingUid") bookingUid: string): Promise<GetBookingRecordingsOutput> { | ||
| const recordings = await this.calVideoService.getRecordings(bookingUid); | ||
| const recordings = this.calVideoService.getRecordings(bookingUid); | ||
|
|
||
| return { | ||
| status: SUCCESS_STATUS, | ||
| data: recordings, | ||
| message: | ||
| "This endpoint will require authentication in a future release. Please update your integration to include valid credentials. See https://cal.com/docs/api-reference/v2/introduction#authentication for details.", | ||
| }; |
There was a problem hiding this comment.
Missing await causes API to return Promise instead of recordings data.
getRecordings is an async method but is called without await. This returns a Promise object to data instead of the actual recordings array. When serialized to JSON, the Promise becomes {}, breaking the API response.
Compare with line 252 where getTranscripts correctly uses await.
Proposed fix
async getBookingRecordings(`@Param`("bookingUid") bookingUid: string): Promise<GetBookingRecordingsOutput> {
- const recordings = this.calVideoService.getRecordings(bookingUid);
+ const recordings = await this.calVideoService.getRecordings(bookingUid);
return {
status: SUCCESS_STATUS,
data: recordings,
};
}📝 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.
| async getBookingRecordings(@Param("bookingUid") bookingUid: string): Promise<GetBookingRecordingsOutput> { | |
| const recordings = await this.calVideoService.getRecordings(bookingUid); | |
| const recordings = this.calVideoService.getRecordings(bookingUid); | |
| return { | |
| status: SUCCESS_STATUS, | |
| data: recordings, | |
| message: | |
| "This endpoint will require authentication in a future release. Please update your integration to include valid credentials. See https://cal.com/docs/api-reference/v2/introduction#authentication for details.", | |
| }; | |
| async getBookingRecordings(`@Param`("bookingUid") bookingUid: string): Promise<GetBookingRecordingsOutput> { | |
| const recordings = await this.calVideoService.getRecordings(bookingUid); | |
| return { | |
| status: SUCCESS_STATUS, | |
| data: recordings, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts`
around lines 226 - 232, The getBookingRecordings handler is assigning a Promise
to data because it calls the async method getRecordings without awaiting it;
update the getBookingRecordings method to await
this.calVideoService.getRecordings(bookingUid) (same pattern as getTranscripts)
so recordings holds the resolved array before returning the
GetBookingRecordingsOutput response.
| @Pbac(["booking.readRecordings"]) | ||
| @Permissions([BOOKING_READ]) | ||
| @UseGuards(BookingUidGuard) | ||
| // @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard) | ||
| @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard) |
There was a problem hiding this comment.
Inconsistent guard ordering may cause authorization issues.
The guard order for transcripts (BookingPbacGuard, ApiAuthGuard, BookingUidGuard) differs from recordings (line 217) and conferencing-sessions (line 563) which both use ApiAuthGuard, BookingUidGuard, BookingPbacGuard.
BookingPbacGuard likely depends on user context set by ApiAuthGuard. Running it first may cause failures or unexpected behavior.
Proposed fix - align with recordings/conferencing-sessions order
`@Pbac`(["booking.readRecordings"])
`@Permissions`([BOOKING_READ])
- `@UseGuards`(BookingPbacGuard, ApiAuthGuard, BookingUidGuard)
+ `@UseGuards`(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)
`@ApiHeader`(API_KEY_OR_ACCESS_TOKEN_HEADER)📝 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.
| @Pbac(["booking.readRecordings"]) | |
| @Permissions([BOOKING_READ]) | |
| @UseGuards(BookingUidGuard) | |
| // @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard) | |
| @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard) | |
| `@Pbac`(["booking.readRecordings"]) | |
| `@Permissions`([BOOKING_READ]) | |
| `@UseGuards`(ApiAuthGuard, BookingUidGuard, BookingPbacGuard) |
🤖 Prompt for AI Agents
In `@apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts`
around lines 236 - 238, The guards for the transcripts endpoint are ordered
incorrectly: change the `@UseGuards` invocation that currently lists
BookingPbacGuard, ApiAuthGuard, BookingUidGuard to the consistent order
ApiAuthGuard, BookingUidGuard, BookingPbacGuard so that ApiAuthGuard establishes
the user context before BookingPbacGuard runs; update the `@UseGuards` decorator
on the transcripts controller method (the one annotated with
`@Pbac`(["booking.readRecordings"]) and `@Permissions`([BOOKING_READ])) to use
ApiAuthGuard, BookingUidGuard, BookingPbacGuard.
1 participant
Benchmark PR from agentic-review-benchmarks#16
Summary by CodeRabbit
Bug Fixes
API Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.