fix: Add new route to create team routing-form response again(after revert earlier)#22407
Conversation
|
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: |
keithwillcode
added
core
This stack of pull requests is managed by Graphite. Learn more about stacking. |
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
2a2521a to
6c8f6ea
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
✅ No security or compliance issues detected. Reviewed everything up to 84e2690. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
hariombalhara
changed the title
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
6c8f6ea to
5aaae1e
Compare
| @@ -0,0 +1,104 @@ | |||
| import { ExecutionContext, CanActivate } from "@nestjs/common"; | |||
There was a problem hiding this comment.
No change in this file, when adding back
| @@ -0,0 +1,51 @@ | |||
| import { Injectable, CanActivate, ExecutionContext, Type, Logger } from "@nestjs/common"; | |||
There was a problem hiding this comment.
No change in this file, when adding back
| @@ -0,0 +1,39 @@ | |||
| import { ApiAuthGuardUser } from "@/modules/auth/strategies/api-auth/api-auth.strategy"; | |||
There was a problem hiding this comment.
No change in this file, when adding back
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
5aaae1e to
9f1d0ed
Compare
| @@ -0,0 +1,810 @@ | |||
| import { bootstrap } from "@/app"; | |||
There was a problem hiding this comment.
These are the same tests as intrdouced earlier in the PR, with the difference of passing version in the request now.
| const createUnauthenticatedRequest = (method: 'get' | 'post' | 'patch', url: string) => { | ||
| return request(app.getHttpServer()) | ||
| [method](url) | ||
| .set({ "cal-api-version": VERSION_2025_07_11 }); |
There was a problem hiding this comment.
Passing version now.
| path: "/v2/organizations/:orgId/routing-forms/:routingFormId/responses", | ||
| version: API_VERSIONS_VALUES, | ||
| // Default version when no version header is provided | ||
| version: VERSION_2024_04_15, |
There was a problem hiding this comment.
Now there is no change in this controller except this version change.
| ): Promise<CreateRoutingFormResponseOutput> { | ||
| const result = await this.organizationsRoutingFormsResponsesService.createRoutingFormResponseWithSlots( | ||
| orgId, | ||
| routingFormId, |
There was a problem hiding this comment.
orgId variable was unused
| @@ -0,0 +1,121 @@ | |||
| import { VERSION_2025_07_11_VALUE } from "@/lib/api-versions"; | |||
There was a problem hiding this comment.
This is same as the Controller that was reviewed earlier in the PR #22347 (files), except the versioning difference
|
|
||
| @Controller({ | ||
| path: "/v2/organizations/:orgId/routing-forms/:routingFormId/responses", | ||
| version: VERSION_2025_07_11_VALUE, |
There was a problem hiding this comment.
Now this controller runs for this version only.
| @@ -9,13 +9,27 @@ import { IsOrgGuard } from "@/modules/auth/guards/organizations/is-org.guard"; | |||
| import { RolesGuard } from "@/modules/auth/guards/roles/roles.guard"; | |||
| import { IsRoutingFormInTeam } from "@/modules/auth/guards/routing-forms/is-routing-form-in-team.guard"; | |||
There was a problem hiding this comment.
Same as reviewed earlier in the PR
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
9f1d0ed to
e3503f6
Compare
apps/api/v2/src/lib/api-versions.ts
Outdated
| /** | ||
| * Even when no version header is provided, we default to this version. | ||
| */ | ||
| export const DEFAULT_API_VERSION: VersionValue = VERSION_2024_04_15 as unknown as VersionValue; |
There was a problem hiding this comment.
cal.com/apps/api/v2/src/app.ts
Line 39 in e3503f6
Default Version is this, just giving it a logical name here.
| @Controller({ | ||
| path: "/v2/organizations/:orgId/routing-forms/:routingFormId/responses", | ||
| version: API_VERSIONS_VALUES, | ||
| version: DEFAULT_API_VERSION, |
There was a problem hiding this comment.
Using this controller only when API version is default
| @@ -1,3 +1,5 @@ | |||
| import { EventTypesRepository_2024_06_14 } from "@/ee/event-types/event-types_2024_06_14/event-types.repository"; | |||
There was a problem hiding this comment.
Same file as earlier reviewed
| @@ -0,0 +1,225 @@ | |||
| import { EventTypesRepository_2024_06_14 } from "@/ee/event-types/event-types_2024_06_14/event-types.repository"; | |||
There was a problem hiding this comment.
Same file as earlier reviewed
| @@ -1,13 +1,18 @@ | |||
| import { CreateRoutingFormResponseInput } from "@/modules/organizations/routing-forms/inputs/create-routing-form-response.input"; | |||
There was a problem hiding this comment.
Same file as earlier reviewed
| import { OrganizationsRoutingFormsRepository } from "@/modules/organizations/routing-forms/organizations-routing-forms.repository"; | ||
| import { SharedRoutingFormResponseService } from "@/modules/organizations/routing-forms/services/shared-routing-form-response.service"; | ||
| import { OrganizationsTeamsRoutingFormsResponsesOutputService } from "@/modules/organizations/teams/routing-forms/services/organizations-teams-routing-forms-responses-output.service"; |
There was a problem hiding this comment.
Same file as revikewed earlier
| import { EventTypesRepository_2024_06_14 } from "@/ee/event-types/event-types_2024_06_14/event-types.repository"; | ||
| import { MembershipsRepository } from "@/modules/memberships/memberships.repository"; | ||
| import { OrganizationsRepository } from "@/modules/organizations/index/organizations.repository"; |
There was a problem hiding this comment.
Same file as reviewed earlier
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
b462556 to
33fc158
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 5 issues across 17 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
.../v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts
Show resolved
Hide resolved
...tions/routing-forms/controllers/organizations-routing-forms-responses.controller.e2e-spec.ts
Show resolved
Hide resolved
...routing-forms/controllers/organizations-teams-routing-forms-responses.controller.e2e-spec.ts
Show resolved
Hide resolved
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
33fc158 to
5442b2a
Compare
E2E results are ready! |
joeauyeung
left a comment
joeauyeung
left a comment
There was a problem hiding this comment.
Reviewed previous PR and ok with the changes made. Waiting for @calcom/platform for approval.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (14)
WalkthroughThis change set introduces a new API version ( Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. ✨ Finishing Touches
🪧 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 (
|
![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
♻️ Duplicate comments (2)
apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.e2e-spec.ts (1)
445-445: Remove console.log statement- console.log("responseBody", responseBody.data);apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts (1)
150-153: Fix potential crash when destination is a relative pathThe
new URL(destination)will throw if destination is a relative path. Consider using a base URL or handling relative paths appropriately.return { - redirectUrl: new URL(destination), + redirectUrl: destination.startsWith('http') ? new URL(destination) : new URL(destination, request.get('origin') || 'https://cal.com'), customMessage: null, };
🧹 Nitpick comments (6)
apps/api/v2/src/modules/auth/guards/organizations/is-user-routing-form.guard.ts (2)
20-29: Consider type safety for user.id conversion.The conversion
Number(user.id)assumes user.id is a valid numeric string. Consider adding validation or using a more robust parsing approach.- userId: Number(user.id), + userId: parseInt(user.id, 10),Additionally, you might want to validate that the conversion is successful:
+ const userId = parseInt(user.id, 10); + if (isNaN(userId)) { + throw new ForbiddenException("Invalid user ID format"); + } + const userRoutingForm = await this.dbRead.prisma.app_RoutingForms_Form.findFirst({ where: { id: routingFormId, - userId: Number(user.id), + userId: userId, teamId: null, },
31-35: Consider security implications of exposing user ID in error messages.The error message includes the user ID, which could be considered sensitive information. Consider using a more generic error message in production.
- throw new ForbiddenException( - `Routing Form with id=${routingFormId} is not a user Routing Form owned by user with id=${user.id}.` - ); + throw new ForbiddenException( + `Access denied: Routing Form with id=${routingFormId} is not accessible by the current user.` + );apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts (1)
187-187: Simplify boolean assignment- skipContactOwner: urlParams.get("cal.skipContactOwner") === "true" ? true : false, + skipContactOwner: urlParams.get("cal.skipContactOwner") === "true",apps/api/v2/swagger/documentation.json (3)
5137-5143:durationexample is a string but the schema is numeric
The example"60"conflicts with the"type": "number"declaration and will trigger validation warnings in many tooling chains.- "example": "60", + "example": 60,
23565-23569: Preferintegerovernumberfor database IDs (responseId)
The same rationale as for path parameters: IDs are integral, not floating point.- "type": "number", + "type": "integer",
23610-23613:eventTypeIdshould also beinteger
Keeping all ID fields consistent simplifies client typing and reduces precision-loss risk.- "type": "number", + "type": "integer",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/api/v2/src/lib/api-versions.ts(1 hunks)apps/api/v2/src/modules/auth/guards/or-guard/index.ts(1 hunks)apps/api/v2/src/modules/auth/guards/or-guard/or.guard.spec.ts(1 hunks)apps/api/v2/src/modules/auth/guards/or-guard/or.guard.ts(1 hunks)apps/api/v2/src/modules/auth/guards/organizations/is-user-routing-form.guard.ts(1 hunks)apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.e2e-spec.ts(25 hunks)apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts(6 hunks)apps/api/v2/src/modules/organizations/routing-forms/organizations-routing-forms.module.ts(2 hunks)apps/api/v2/src/modules/organizations/routing-forms/services/organizations-routing-forms-responses.service.ts(3 hunks)apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts(1 hunks)apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.e2e-spec.ts(8 hunks)apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.ts(2 hunks)apps/api/v2/src/modules/organizations/teams/routing-forms/organizations-teams-routing-forms.module.ts(3 hunks)apps/api/v2/src/modules/organizations/teams/routing-forms/services/organizations-teams-routing-forms-responses.service.ts(2 hunks)apps/api/v2/swagger/documentation.json(7 hunks)packages/platform/constants/api.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
apps/api/v2/src/modules/organizations/teams/routing-forms/organizations-teams-routing-forms.module.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/modules/organizations/routing-forms/organizations-routing-forms.module.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/modules/organizations/routing-forms/services/organizations-routing-forms-responses.service.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/lib/api-versions.ts (1)
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.001Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the `currentLink.maxUsageCount ?? 1` fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
apps/api/v2/src/modules/auth/guards/organizations/is-user-routing-form.guard.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.e2e-spec.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/swagger/documentation.json (1)
undefined
<retrieved_learning>
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
</retrieved_learning>
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.e2e-spec.ts (2)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.497Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.
🧬 Code Graph Analysis (3)
apps/api/v2/src/modules/auth/guards/or-guard/or.guard.spec.ts (1)
apps/api/v2/src/modules/auth/guards/or-guard/or.guard.ts (1)
Or(9-49)
apps/api/v2/src/lib/api-versions.ts (1)
packages/platform/constants/api.ts (7)
API_VERSIONS(63-70)VERSION_2024_06_14(56-56)VERSION_2024_06_11(57-57)VERSION_2024_04_15(58-58)VERSION_2024_08_13(59-59)VERSION_2024_09_04(60-60)VERSION_2025_07_11(61-61)
apps/api/v2/src/modules/auth/guards/organizations/is-user-routing-form.guard.ts (1)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (1)
ApiAuthGuardUser(37-37)
🪛 Biome (1.9.4)
apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts
[error] 187-187: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (31)
apps/api/v2/src/modules/auth/guards/or-guard/index.ts (1)
1-1: LGTM: Clean index export patternThe export statement follows standard module organization conventions for NestJS guards.
apps/api/v2/src/modules/organizations/teams/routing-forms/organizations-teams-routing-forms.module.ts (3)
1-1: Module import properly structuredThe
EventTypesRepository_2024_06_14import follows the established pattern for versioned repositories.
6-6: Shared service integration looks goodThe
SharedRoutingFormResponseServiceimport enables the centralized response creation logic as intended.
36-36: Provider registrations are correctBoth
SharedRoutingFormResponseServiceandEventTypesRepository_2024_06_14are properly registered as providers to enable dependency injection.Also applies to: 45-45
packages/platform/constants/api.ts (2)
61-61: API version constant correctly definedThe
VERSION_2025_07_11constant follows the established naming convention and date format.
69-69: Array addition maintains proper structureThe new version is correctly appended to the
API_VERSIONSarray, maintaining the existing structure.apps/api/v2/src/modules/organizations/routing-forms/organizations-routing-forms.module.ts (3)
1-2: Dependencies properly importedBoth
EventTypesRepository_2024_06_14andIsUserRoutingFormimports are correctly structured and necessary for the new functionality.
19-19: Shared service import enables centralized logicThe
SharedRoutingFormResponseServiceimport supports the refactoring to centralize routing form response creation logic.
24-24: Provider registrations correctly configuredAll three new providers (
IsUserRoutingForm,SharedRoutingFormResponseService,EventTypesRepository_2024_06_14) are properly registered for dependency injection.Also applies to: 30-30, 33-33
apps/api/v2/src/modules/organizations/teams/routing-forms/services/organizations-teams-routing-forms-responses.service.ts (3)
1-3: Import statements properly structuredThe imports for
CreateRoutingFormResponseInput,CreateRoutingFormResponseOutputData,SharedRoutingFormResponseService, andRequestare correctly added to support the new functionality.Also applies to: 6-6
15-15: Constructor injection correctly implementedThe
SharedRoutingFormResponseServiceis properly injected into the constructor, enabling the delegation pattern.
44-54: Clean delegation pattern implementedThe
createRoutingFormResponseWithSlotsmethod properly delegates to the shared service, maintaining a clean separation of concerns and avoiding code duplication.apps/api/v2/src/modules/auth/guards/organizations/is-user-routing-form.guard.ts (1)
14-18: Verify routingFormId parameter extraction and add validation.The guard properly extracts the routingFormId from request parameters and validates its presence. The error message is descriptive and helps with debugging.
apps/api/v2/src/lib/api-versions.ts (3)
10-10: Good addition of new API version constant.The new version constant is properly imported and will be used for the new versioned routing form endpoints.
13-13: Spread operator usage maintains proper type handling.The use of spread operator for API_VERSIONS_VALUES is correct and ensures the array remains mutable as required by the VersionValue type.
20-23: Excellent addition of DEFAULT_API_VERSION constant.Adding a named constant for the default API version improves code readability and maintainability. The documentation comment clearly explains its purpose.
apps/api/v2/src/modules/auth/guards/or-guard/or.guard.ts (3)
9-49: Excellent implementation of OR guard factory.The OR guard implementation is well-designed with proper:
- Factory pattern for creating composite guards
- Dependency injection using ModuleRef
- Error handling that allows the OR chain to continue
- Logging for debugging and monitoring
- Async/await pattern for guard evaluation
The logic correctly implements OR semantics: access is granted if any guard passes, and exceptions from individual guards don't halt the evaluation chain.
27-37: Good error handling strategy for OR logic.The error handling correctly catches exceptions from individual guards and continues evaluating the remaining guards. This is the expected behavior for OR logic where one guard's failure shouldn't prevent others from being checked.
41-44: Proper final error handling.The guard appropriately re-throws the last error if all guards fail, providing meaningful feedback while maintaining the OR semantics.
apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.ts (1)
83-105: Well-implemented POST endpoint for routing form responses.The new endpoint follows excellent patterns:
- Proper role-based access control (
@Roles("TEAM_MEMBER"))- Appropriate platform plan restriction (
@PlatformPlan("ESSENTIALS"))- Consistent parameter validation and typing
- Standard response format with SUCCESS_STATUS
- Proper delegation to service layer
The implementation is consistent with existing endpoints in the controller and follows NestJS best practices.
apps/api/v2/src/modules/auth/guards/or-guard/or.guard.spec.ts (3)
5-31: Well-designed mock guards for testing.The mock guard implementations provide good test coverage by simulating different behaviors:
- MockGuard1 and MockGuard2 for pass/fail scenarios
- MockGuard3 for error throwing scenarios
The mock designs are simple and effective for testing the OR guard logic.
52-94: Comprehensive test coverage for OR guard behavior.The test suite excellently covers all critical scenarios:
- First guard passing grants access
- Second guard passing grants access (OR logic)
- All guards failing denies access
- Error handling continues checking other guards
The tests properly mock dependencies and validate the expected OR semantics.
97-104: Good validation of factory function.The test properly validates that the Or factory function returns a constructor function, ensuring the factory pattern works correctly.
apps/api/v2/src/modules/organizations/routing-forms/services/organizations-routing-forms-responses.service.ts (1)
44-54: Clean refactoring to delegate shared logicThe extraction of routing form response creation logic to
SharedRoutingFormResponseServiceimproves code reusability and maintainability.apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.e2e-spec.ts (2)
27-60: Well-structured test data organizationThe grouping of test data into
orgAdminData,teamData, andnonOrgAdminDataimproves test readability and maintainability.
701-848: Comprehensive test coverage for new API versionThe tests thoroughly validate the new authorization behavior, ensuring non-org-admin users can only access their own routing forms while org admins retain full access.
apps/api/v2/src/modules/organizations/teams/routing-forms/controllers/organizations-teams-routing-forms-responses.controller.e2e-spec.ts (1)
255-702: Excellent test coverage for new team routing form response endpointThe test suite comprehensively covers permission checks, validation, routing behavior, and error scenarios for the new POST endpoint.
apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts (1)
26-108: Well-structured service with clear separation of concernsThe service effectively centralizes routing form response creation logic with proper error handling and CRM parameter extraction.
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts (2)
39-41: Good approach to maintain backward compatibilityUsing
UNVERSIONEDto exclude the new version ensures existing clients continue to work with the original endpoint behavior.
105-127: Well-implemented versioned endpoint with flexible authorizationThe use of
Orguard to allow either org admin or user routing form access provides the needed flexibility while maintaining security.apps/api/v2/swagger/documentation.json (1)
2547-2547: Confirm the newoperationIdmatches controller decorator
If the NestJS@ApiOperation({ operationId: … })(or auto-generated name) inOrganizationsRoutingFormsResponsesControllerwasn’t updated to the exact stringcreateRoutingFormResponseV2025_07_11, Swagger ↔ code linkage will break and the route won’t be mapped in generated SDKs.
Please double-check the controller signature.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/v2/swagger/documentation.json (1)
5093-5106: Useintegernotnumberfor path identifiers
orgIdandteamIdrepresent discrete IDs; OpenAPI distinguishesintegerfromnumber(float).
We’ve flagged this in an earlier review and the issue persists here.- "schema": { - "type": "number" - } + "schema": { + "type": "integer", + "format": "int64" + }
🧹 Nitpick comments (1)
apps/api/v2/swagger/documentation.json (1)
5151-5154: Clarify numeric duration format
durationis declared simply asnumber; most client SDKs map this todouble.
If the value must be a whole number of minutes, specify it explicitly:- "type": "number" + "type": "integer", + "format": "int32", + "minimum": 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts(6 hunks)apps/api/v2/swagger/documentation.json(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/api/v2/swagger/documentation.json (1)
undefined
<retrieved_learning>
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
</retrieved_learning>
⏰ 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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Security Check
🔇 Additional comments (4)
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts (4)
2-2: Well-structured imports and backward compatibility setup.The imports are correctly organized and the
UNVERSIONEDconstant provides a clean way to maintain backward compatibility while introducing the new API version. This approach ensures existing clients continue to work without modifications.Also applies to: 8-8, 11-11, 26-26, 39-40
46-46: Good approach to selective authorization.Moving the
RolesGuardfrom class-level to individual methods provides flexibility for implementing different authorization strategies across endpoints while maintaining existing behavior for backward compatibility.Also applies to: 57-57, 85-85, 141-141
82-82: Excellent API versioning implementation.The versioned endpoint approach is well-implemented:
- Maintains backward compatibility through the
UNVERSIONEDdecorator- Provides enhanced authorization with the
Orguard allowing either admin or user ownership- Includes proper API documentation with required headers
- Uses consistent parameter handling across versions
This addresses the PR objective of reintroducing functionality without breaking existing endpoints.
Also applies to: 105-136
94-103: Correct parameter handling.The service method call has been updated to pass the correct parameters. Based on the past review comments, the removal of the unused
orgIdparameter is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/v2/swagger/documentation.json (2)
5083-5096: IDs should be declared asinteger, notnumber
orgIdandteamIdare primary-key IDs; defining them as floating-point capablenumberinvites client coercion issues.
This exact concern was raised earlier and remains unresolved.
23555-23580: Identifier fields andteamMemberIdsrequirement issue persist
responseIdand items insideteamMemberIdsare still typed asnumber; they should beinteger.
Additionally,teamMemberIdsremains required, yet a queued response may legitimately have none, risking validation failures.
🧹 Nitpick comments (3)
apps/api/v2/swagger/documentation.json (3)
5137-5145: Useintegerfor thedurationquery parameter
Duration is expressed in whole minutes (e.g.,60). Declare its schema as:- "type": "number" + "type": "integer"to convey discrete values and improve generator fidelity.
5182-5191: Add a meaningful 201 response description
Thedescriptionfield is empty. A short sentence (e.g., “Routing form response successfully created.”) improves generated client docs and Swagger UI clarity.
23610-23613:eventTypeIdshould beinteger
As an ID, this field should useintegerto match database type and avoid float coercion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.e2e-spec.ts(25 hunks)apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts(6 hunks)apps/api/v2/swagger/documentation.json(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.ts
- apps/api/v2/src/modules/organizations/routing-forms/controllers/organizations-routing-forms-responses.controller.e2e-spec.ts
🧰 Additional context used
🧠 Learnings (1)
apps/api/v2/swagger/documentation.json (2)
undefined
<retrieved_learning>
Learnt from: alishaz-polymath
PR: #22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.863Z
Learning: In PR #22304 for Cal.com private link expiration features, the maxUsageCount field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
</retrieved_learning>
<retrieved_learning>
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
</retrieved_learning>
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/api/v2/swagger/documentation.json (3)
5083-5097: Path IDs should be declared asinteger, notnumberUsing the floating-point
numbertype for discrete identifiers (orgId,teamId) can cause client generators to coerce them into floats. Stick tointegerto avoid precision loss and validation issues.- "type": "number" + "type": "integer"
24962-24978: ID fields andteamMemberIdsarray still usenumber& remain requiredPrevious feedback flagged the same issue:
responseIdand items inteamMemberIdsshould beinteger.teamMemberIdscan be empty when a response is queued; making it required may cause validation failures.- "responseId": { - "type": "number", + "responseId": { + "type": "integer", ... - "type": "number" + "type": "integer" @@ - "required": [ - "teamMemberIds" - ] + "required": []
25007-25010:eventTypeIdshould be anintegerIdentifiers are discrete and should not be expressed as floating-point numbers.
- "eventTypeId": { - "type": "number", + "eventTypeId": { + "type": "integer",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/v2/swagger/documentation.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/api/v2/swagger/documentation.json (2)
undefined
<retrieved_learning>
Learnt from: alishaz-polymath
PR: #22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.863Z
Learning: In PR #22304 for Cal.com private link expiration features, the maxUsageCount field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
</retrieved_learning>
<retrieved_learning>
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
</retrieved_learning>
⏰ 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: Security Check
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
1c169f9 to
c27e244
Compare
…ms-response-record-endpoint-back
hariombalhara
force-pushed
the
add-teams-routing-forms-response-record-endpoint-back
branch
from
c27e244 to
84e2690
Compare
hariombalhara
deleted the
add-teams-routing-forms-response-record-endpoint-back
branch
4 participants
It is the same changes as in #22347. They were earlier reverted because we didn't want to introduce a breaking change in the endpoint.
Thanks to @supalarry I realized that there is no breaking change here and it is adding back the same changes with just more tests.