feat: update recording, transcript endpoint and add tests #27
Conversation
WalkthroughAuthorization guards refactored for bookings endpoints to use PBAC-based authorization. Deprecation messages removed from response types. Permission requirement for recordings endpoint changed from READ to WRITE. E2E authorization tests activated. markNoShow endpoint decorators removed, effectively disabling the endpoint. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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
🤖 Fix all issues with AI agents
In `@apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts`:
- Around line 226-231: The getBookingRecordings controller currently returns the
Promise from calVideoService.getRecordings instead of the resolved recordings;
update the getBookingRecordings method to await
calVideoService.getRecordings(bookingUid) so it returns the resolved recordings
array (mirror the pattern used in getTranscripts/getVideoSessions) and then
return that awaited value in the response payload.
🧹 Nitpick comments (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/booking-access-auth.e2e-spec.ts (1)
188-233: Consider asserting response data shape in the new auth tests.Adding data assertions would catch regressions where recordings/transcripts aren’t actually returned.
✅ Suggested additions
const response = await request(app.getHttpServer()) .get(`/v2/bookings/${testBooking.uid}/recordings`) .set("Authorization", `Bearer ${ownerApiKey}`) .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13) .expect(200); expect(response.body.status).toEqual(SUCCESS_STATUS); + expect(response.body.data).toEqual([]); @@ const response = await request(app.getHttpServer()) .get(`/v2/bookings/${testBooking.uid}/transcripts`) .set("Authorization", `Bearer ${ownerApiKey}`) .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13) .expect(200); expect(response.body.status).toEqual(SUCCESS_STATUS); + expect(response.body.data).toEqual([]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.tsapps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/booking-access-auth.e2e-spec.tspackages/platform/types/bookings/2024-08-13/outputs/get-booking-recordings.output.tspackages/platform/types/bookings/2024-08-13/outputs/get-booking-transcripts.output.ts
💤 Files with no reviewable changes (1)
- packages/platform/types/bookings/2024-08-13/outputs/get-booking-transcripts.output.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts (3)
apps/api/v2/src/modules/auth/decorators/pbac/pbac.decorator.ts (1)
Pbac(5-5)packages/platform/constants/api.ts (1)
SUCCESS_STATUS(9-9)scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(18-20)
🔇 Additional comments (4)
packages/platform/types/bookings/2024-08-13/outputs/get-booking-recordings.output.ts (1)
6-6: Looks good.No concerns with this change.
apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts (3)
214-218: Confirm WRITE permission is intended for a read-only endpoint.GET recordings now requires
BOOKING_WRITEwhile the PBAC scope isbooking.readRecordings, which can block read-only API keys. Please confirm this breaking change is intended or consider allowingBOOKING_READ(or both) for backward compatibility.
235-239: Verify guard order for transcripts PBAC authorization.
BookingPbacGuardruns beforeApiAuthGuard/BookingUidGuardhere (unlike recordings). IfBookingPbacGuardrelies onrequest.useror booking context, this order will reject valid requests. Consider aligning the order with the recordings endpoint.🔁 Suggested alignment
- `@UseGuards`(BookingPbacGuard, ApiAuthGuard, BookingUidGuard) + `@UseGuards`(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)
394-405: No issues to flag here.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "bookings.controller.ts" | head -20Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 237
🏁 Script executed:
fd -t f "bookings.controller.ts"Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 521
🏁 Script executed:
cat -n "apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts" | sed -n '220,235p'Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 855
🏁 Script executed:
rg "getRecordings" apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts -B 5 -A 5Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 437
🏁 Script executed:
rg "calVideoService" "apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts" -B 5 -A 5Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1798
🏁 Script executed:
grep -n "calVideoService\|constructor" "apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts" | head -20Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 402
🏁 Script executed:
fd -t f "cal-video.service.ts"Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 154
🏁 Script executed:
rg "class CalVideoService" -A 50Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 5613
🏁 Script executed:
cat -n "apps/api/v2/src/ee/bookings/2024-08-13/services/cal-video.service.ts" | sed -n '12,50p'Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1863
Add await to getRecordings() call.
The getRecordings() method is async and returns a Promise. Without await, the response will contain a serialized Promise object instead of the recordings array. Other similar methods in the same controller (getTranscripts, getVideoSessions) correctly use await for their async service calls.
Proposed fix
- const recordings = this.calVideoService.getRecordings(bookingUid);
+ const recordings = await this.calVideoService.getRecordings(bookingUid);📝 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, | |
| 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 - 231, The getBookingRecordings controller currently returns
the Promise from calVideoService.getRecordings instead of the resolved
recordings; update the getBookingRecordings method to await
calVideoService.getRecordings(bookingUid) so it returns the resolved recordings
array (mirror the pattern used in getTranscripts/getVideoSessions) and then
return that awaited value in the response payload.
Benchmark PR from qodo-benchmark#692
Summary by CodeRabbit
Improvements
API Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.