fix: return only non-hidden event types in public endpoint #22746
Conversation
|
@BKM14 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA new authenticated GET Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/25/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/25/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts (1)
90-123: LGTM! Consider extracting the hidden fields filtering logic to reduce duplication.The new authenticated endpoint implementation is correct and properly secured. However, the hidden booking fields filtering logic (lines 103-117) is duplicated from the public endpoint below.
Consider extracting the filtering logic into a private helper method:
+ private filterHiddenBookingFields(eventTypes: EventTypeOutput_2024_06_14[]): EventTypeOutput_2024_06_14[] { + return eventTypes.map((eventType) => ({ + ...eventType, + bookingFields: Array.isArray(eventType?.bookingFields) + ? eventType.bookingFields + .map((field) => { + if ("hidden" in field) { + return field.hidden !== true ? field : null; + } + return field; + }) + .filter((f) => f) + : [], + })) as EventTypeOutput_2024_06_14[]; + } async getEventTypesForUser(@GetUser() user: UserWithProfile): Promise<GetEventTypesOutput_2024_06_14> { const eventTypes = await this.eventTypesService.getUserEventTypes(user.id); if (!eventTypes || eventTypes.length === 0) { throw new NotFoundException(`Event types not found`); } const eventTypesFormatted = this.eventTypeResponseTransformPipe.transform(eventTypes); - const eventTypesWithoutHiddenFields = eventTypesFormatted.map((eventType) => { - return { - ...eventType, - bookingFields: Array.isArray(eventType?.bookingFields) - ? eventType?.bookingFields - .map((field) => { - if ("hidden" in field) { - return field.hidden !== true ? field : null; - } - return field; - }) - .filter((f) => f) - : [], - }; - }) as EventTypeOutput_2024_06_14[]; + const eventTypesWithoutHiddenFields = this.filterHiddenBookingFields(eventTypesFormatted); return { status: SUCCESS_STATUS, data: eventTypesWithoutHiddenFields, }; }apps/api/v2/swagger/documentation.json (1)
17742-17760:nullable: true+required: ["data"]can trip some validatorsFor
GetEventTypeOutput_2024_06_14thedatafield is both required and explicitly nullable.
A few OpenAPI generators (e.g. openapi-generator kotlin/python) flag this as contradictory and output less-friendly types.Two options:
- Keep
requiredand dropnullable, documenting an empty object when not found.- Keep
nullable: trueand droprequired, matching existing “object or null” patterns.Pick whichever matches the API behaviour, but avoid the combination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts(3 hunks)apps/api/v2/swagger/documentation.json(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts
🧬 Code Graph Analysis (1)
apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts (4)
apps/api/v2/src/modules/auth/guards/api-auth/api-auth.guard.ts (1)
ApiAuthGuard(7-20)apps/api/v2/src/lib/docs/headers.ts (1)
API_KEY_OR_ACCESS_TOKEN_HEADER(42-47)apps/api/v2/src/modules/users/users.repository.ts (1)
UserWithProfile(9-12)packages/platform/constants/api.ts (1)
SUCCESS_STATUS(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts (1)
155-178: LGTM! The public endpoint correctly filters out hidden event types.The modifications properly implement the PR objective by filtering out hidden event types before transformation (line 155) while maintaining the existing hidden booking fields filtering. The variable naming accurately reflects the dual filtering operations.
The same code duplication concern mentioned in the previous comment applies here - the hidden booking fields filtering logic (lines 159-173) could benefit from the same helper method extraction.
apps/api/v2/swagger/documentation.json (1)
17719-17735: All schema references are correctVerified that
GetEventTypeOutput_2024_06_14appears only on the single‐resource endpoints (GET/v2/event-types/{id}and PATCH/v2/event-types/{id}) and thatGetEventTypesOutput_2024_06_14is used for collection endpoints. No dangling or incorrect references remain.
| "/v2/event-types/user": { | ||
| "get": { | ||
| "operationId": "EventTypesController_2024_06_14_getEventTypesForUser", | ||
| "summary": "Get all event types belonging to the authenticated user", | ||
| "parameters": [ | ||
| { | ||
| "name": "cal-api-version", | ||
| "in": "header", | ||
| "description": "Must be set to 2024-06-14", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "default": "2024-06-14" | ||
| } | ||
| }, | ||
| { | ||
| "name": "Authorization", | ||
| "in": "header", | ||
| "description": "value must be `Bearer <token>` where `<token>` is api key prefixed with cal_ or managed user access token", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/GetEventTypesOutput_2024_06_14" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "tags": [ | ||
| "Event Types" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer reusable security scheme over ad-hoc Authorization header & document error responses
The new /v2/event-types/user path embeds a hard-coded Authorization header parameter.
Across the spec we already expose authenticated endpoints via a reusable bearerAuth (or similar) security scheme – embedding the header here breaks consistency, forces duplication, and makes client generation harder.
At the same time, 4xx responses (401, 403, possibly 404) are not documented, whereas other authenticated endpoints usually list them.
- "parameters": [
- { … "name": "Authorization", "in": "header", … }
- ],
+ "security": [
+ { "bearerAuth": [] }
+ ],
+ "parameters": [
+ { … "name": "cal-api-version", … }
+ ],
"responses": {
+ "401": { "$ref": "#/components/responses/UnauthorizedError" },
+ "403": { "$ref": "#/components/responses/ForbiddenError" },
"200": { … }
},Aligning with the existing pattern keeps the spec DRY and avoids confusing client SDKs.
Please adjust the path object accordingly and ensure shared error responses are referenced.
📝 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.
| "/v2/event-types/user": { | |
| "get": { | |
| "operationId": "EventTypesController_2024_06_14_getEventTypesForUser", | |
| "summary": "Get all event types belonging to the authenticated user", | |
| "parameters": [ | |
| { | |
| "name": "cal-api-version", | |
| "in": "header", | |
| "description": "Must be set to 2024-06-14", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "default": "2024-06-14" | |
| } | |
| }, | |
| { | |
| "name": "Authorization", | |
| "in": "header", | |
| "description": "value must be `Bearer <token>` where `<token>` is api key prefixed with cal_ or managed user access token", | |
| "required": true, | |
| "schema": { | |
| "type": "string" | |
| } | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/GetEventTypesOutput_2024_06_14" | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "tags": [ | |
| "Event Types" | |
| ] | |
| } | |
| }, | |
| "/v2/event-types/user": { | |
| "get": { | |
| "operationId": "EventTypesController_2024_06_14_getEventTypesForUser", | |
| "summary": "Get all event types belonging to the authenticated user", | |
| "security": [ | |
| { "bearerAuth": [] } | |
| ], | |
| "parameters": [ | |
| { | |
| "name": "cal-api-version", | |
| "in": "header", | |
| "description": "Must be set to 2024-06-14", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "default": "2024-06-14" | |
| } | |
| } | |
| ], | |
| "responses": { | |
| "401": { | |
| "$ref": "#/components/responses/UnauthorizedError" | |
| }, | |
| "403": { | |
| "$ref": "#/components/responses/ForbiddenError" | |
| }, | |
| "200": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/GetEventTypesOutput_2024_06_14" | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "tags": [ | |
| "Event Types" | |
| ] | |
| } | |
| }, |
🤖 Prompt for AI Agents
In apps/api/v2/swagger/documentation.json around lines 9479 to 9520, replace the
hard-coded Authorization header parameter in the /v2/event-types/user GET
operation with a reusable security scheme reference such as "bearerAuth" to
maintain consistency and avoid duplication. Additionally, add references to the
common 4xx error responses like 401, 403, and possibly 404 to the responses
section to document authentication and authorization errors consistently with
other endpoints.
|
Thank you for the contribution, requesting clarification on the addition of /event-types/user for getting authed user event types. The fix itself is good. |
The issue suggested moving fetching all eventTypes(including hidden eventTypes) for the authenticated user. The @emrysal Marking this as ready for review now |
I am not sure if we can deploy it because it's kind of a breaking change. On the one hand hidden event types should be hidden and its in a way a fix, but it also means that people who rely on these endpoints for managing user's event types would result in a user not seeing the hidden event types anymore. |
What does this PR do?
Earlier, hidden event types were returned in the
v2/event-types/endpoint. This endpoint didn't require any authentication. Now, this endpoint only return non-hidden event types. This is implemented by adding a simple filter to the existing logic.Additionally, hidden event types are now accessible through a separate endpoint which returns all event types associated with the authenticated user.
Image Demo:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Seed the database using the script and start the v2 api. Set any arbitrary eventType to hidden using the
PATCHrequest. Now hit the/v2/event-types/endpoint and notice that the eventType which was previously set tohiddenis now not visible.