feat: update recording, transcript endpoint and add tests #95
Conversation
Code Review by Qodo
1. Missing await in recordings
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
226
to
232
| 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.
1. Missing await in recordings 📘 Rule violation ✓ Correctness
• getBookingRecordings() assigns the Promise returned by this.calVideoService.getRecordings() to recordings without awaiting it, then returns it as data. • This can break the endpoint contract (GetBookingRecordingsOutput.data becomes a Promise), and errors from the Promise may surface as unhandled rejections rather than being managed gracefully. • This violates the requirement to identify and handle failure points and edge cases with meaningful context.
Agent prompt
## Issue description
`getBookingRecordings()` returns a Promise in `data` because the CalVideo call is not awaited, which can lead to incorrect responses and unhandled async failures.
## Issue Context
This endpoint depends on an async service call (`calVideoService.getRecordings`). The compliance requirement expects failure points to be handled gracefully with actionable internal context.
## Fix Focus Areas
- apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts[226-232]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+188
to
+215
| describe("GET /v2/bookings/:bookingUid/recordings - Authorization", () => { | ||
| it("should allow booking organizer to access recordings", async () => { | ||
| const calVideoService = app.get(CalVideoService); | ||
| jest.spyOn(calVideoService, "getRecordings").mockResolvedValue([]); | ||
|
|
||
| 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); | ||
| }); | ||
|
|
||
| it("should return 403 when unauthorized user tries to access recordings", async () => { | ||
| await request(app.getHttpServer()) | ||
| .get(`/v2/bookings/${testBooking.uid}/recordings`) | ||
| .set("Authorization", `Bearer ${unauthorizedApiKey}`) | ||
| .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13) | ||
| .expect(403); | ||
| }); | ||
| }); | ||
|
|
||
| describe("GET /v2/bookings/:bookingUid/transcripts - Authorization", () => { | ||
| it("should allow booking organizer to access transcripts", async () => { | ||
| const calVideoService = app.get(CalVideoService); | ||
| jest.spyOn(calVideoService, "getTranscripts").mockResolvedValue([]); | ||
|
|
There was a problem hiding this comment.
2. E2e tests use jest.spyon 📘 Rule violation ⛯ Reliability
• The new e2e tests use Jest-specific APIs (jest.spyOn) instead of Vitest equivalents. • This violates the requirement that all tests use the Vitest framework, and risks inconsistent behavior in the standardized test runner configuration.
Agent prompt
## Issue description
The e2e test file uses Jest-specific APIs (`jest.spyOn`) instead of Vitest (`vi.spyOn`), violating the standardized test framework requirement.
## Issue Context
The repository compliance requires Vitest syntax in all test files.
## Fix Focus Areas
- apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/booking-access-auth.e2e-spec.ts[188-215]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+236
to
239
| @Pbac(["booking.readRecordings"]) | ||
| @Permissions([BOOKING_READ]) | ||
| @UseGuards(BookingUidGuard) | ||
| // @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard) | ||
| @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard) | ||
| @ApiHeader(API_KEY_OR_ACCESS_TOKEN_HEADER) |
There was a problem hiding this comment.
3. Guard order blocks auth 🐞 Bug ⛨ Security
• @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard) runs BookingPbacGuard before authentication, but BookingPbacGuard requires request.user. • This will cause the transcripts endpoint to always throw UnauthorizedException, making it effectively unusable.
Agent prompt
### Issue description
The transcripts endpoint runs `BookingPbacGuard` before `ApiAuthGuard`, but `BookingPbacGuard` requires `request.user`.
### Issue Context
`ApiAuthGuard`/strategy is what populates `request.user`. PBAC checks should run after authentication.
### Fix Focus Areas
- apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts[235-239]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+215
to
+217
| @Pbac(["booking.readRecordings"]) | ||
| @Permissions([BOOKING_WRITE]) | ||
| @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard) |
There was a problem hiding this comment.
4. Wrong permission scope 🐞 Bug ⛨ Security
• The recordings endpoint is a GET/read operation but is annotated with @Permissions([BOOKING_WRITE]), which is inconsistent with other read-only booking endpoints. • PermissionsGuard enforces these permission bits for platform OAuth clients / managed-user access tokens; requiring write will block clients that only requested booking read access and violates least-privilege.
Agent prompt
### Issue description
A read-only recordings endpoint currently requires `BOOKING_WRITE` permission.
### Issue Context
`PermissionsGuard` enforces OAuth client permission bits; using the write bit will block integrations that only requested booking read access.
### Fix Focus Areas
- apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts[214-217]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import { IsEnum, ValidateNested, IsNumber, IsString, IsOptional, IsUrl } from "class-validator"; | ||
|
|
||
| import { SUCCESS_STATUS, ERROR_STATUS } from "@calcom/platform-constants"; | ||
| import type { BookingRepository } from "@calcom/features/bookings/lib/BookingRepository"; |
There was a problem hiding this comment.
5. Unused type import 🐞 Bug ⛯ Reliability
• GetBookingRecordingsOutput adds an unused import type { BookingRepository } ....
• This increases coupling between packages and can trigger lint/module-resolution issues in some
build environments.
Agent prompt
### Issue description
An unused type-only import was added to a shared DTO file.
### Issue Context
Even type-only imports can introduce unwanted coupling and tooling failures if the referenced package isn’t meant to be a dependency.
### Fix Focus Areas
- packages/platform/types/bookings/2024-08-13/outputs/get-booking-recordings.output.ts[5-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Benchmark PR from qodo-benchmark#692