fix: Code Quality improvements to response record endpoint and added unit tests#22264
fix: Code Quality improvements to response record endpoint and added unit tests#22264hariombalhara merged 8 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
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 8067a35. Security Overview
Detected Code Changes
Reply to this PR with |
418c300 to
b6c4ef9
Compare
| eventTypeId?: number; | ||
|
|
||
| @ValidateNested() | ||
| @ApiProperty({ type: Routing }) |
| }, | ||
| }) | ||
| @ValidateNested() | ||
| @Type(() => Routing) |
E2E results are ready! |
b6c4ef9 to
72991c0
Compare
72991c0 to
dddbad5
Compare
GetAvailableSlotsInput
| export class GetAvailableSlotsInputWithRouting_2024_09_04 extends GetAvailableSlotsInput_2024_09_04 { | ||
| @IsString() | ||
| @ApiHideProperty() | ||
| withRouting = true as const; |
There was a problem hiding this comment.
It allows us to differentiate b/w the two types GetAvailableSlotsInputWithRouting_2024_09_04 and GetAvailableSlotsInput_2024_09_04
This is similar to what we do using ById_2024_09_04_type to implement Discriminated unions in TS
| const availableSlots: TimeSlots = await getAvailableSlots({ | ||
| input: { | ||
| ...queryTransformed, | ||
| routingFormResponseId: queryTransformed.routingFormResponseId ?? undefined, |
There was a problem hiding this comment.
We don't need to handle it here now as queryTransformed will have the correct value
GetAvailableSlotsInputGetAvailableSlotsInput and add unit test
7163a5b to
e702982
Compare
| @IsString() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| teamMemberEmail?: string; | ||
|
|
||
| @IsNumber() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| routingFormResponseId?: number; | ||
|
|
||
| @IsArray() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| routedTeamMemberIds?: number[]; | ||
|
|
||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| skipContactOwner?: boolean; |
There was a problem hiding this comment.
removing the props that aren't documented and shouldn't be used for the get slots input.
|
Excellent suggestion @supalarry !! Addressed it |
- Remove intermediate variable assignment in getAvailableSlotsWithRouting - Update test to match simplified routing parameters structure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/api-reference/v2/openapi.json (1)
23612-23618: Grammar nit – use “as-is” to avoid ambiguity.Current wording:
"The routing information that could be passed as is to the booking API."Consider the hyphenated form for clarity:
- "description": "The routing information that could be passed as is to the booking API.", + "description": "The routing information that can be passed as-is to the booking API.",apps/api/v2/swagger/documentation.json (2)
21607-21611: Duplicate maintenance reminderSame enumeration appears a second time; if this file is generated, consider de-duplicating the trigger list at the generator level so manual edits aren’t required in multiple blocks.
24803-24808: Keep language codes alphabetically sortedThe newly-added
"bn"(Bengali) is appended after"zh-TW", breaking the alphabetical order that the enum followed earlier.
Maintaining a deterministic order eases diff reviews and reduces merge conflicts.- "sk", - "ta", - "uk", - "zh-TW", - "bn" + "bn", + "sk", + "ta", + "uk", + "zh-TW"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/v2/src/modules/organizations/routing-forms/outputs/create-routing-form-response.output.ts(1 hunks)apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts(1 hunks)apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts(3 hunks)apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts(1 hunks)apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.ts(5 hunks)apps/api/v2/swagger/documentation.json(4 hunks)docs/api-reference/v2/openapi.json(6 hunks)packages/platform/types/slots/slots-2024-09-04/inputs/get-slots-input.pipe.ts(1 hunks)packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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/outputs/create-routing-form-response.output.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/lib/server/service/insightsRouting.ts:367-368
Timestamp: 2025-07-15T13:02:17.359Z
Learning: In the InsightsRoutingService (packages/lib/server/service/insightsRouting.ts), multi-select filter data is already validated by zod before reaching the buildFormFieldSqlCondition method, so null/undefined values are not present in filterValue.data arrays and don't need to be filtered out.
apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.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.
packages/platform/types/slots/slots-2024-09-04/inputs/get-slots-input.pipe.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.
packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.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.
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>
docs/api-reference/v2/openapi.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/slots/slots-2024-09-04/services/slots-input.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.
🧬 Code Graph Analysis (3)
apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts (1)
packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts (1)
ById_2024_09_04_type(93-93)
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts (1)
packages/platform/types/slots/slots-2024-09-04/inputs/get-slots-input.pipe.ts (2)
GetSlotsInput_2024_09_04(14-18)GetSlotsInputWithRouting_2024_09_04(20-25)
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.ts (3)
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts (2)
TransformedGetSlotsQuery(22-33)TransformedGetSlotsQueryWithRouting(35-40)apps/api/v2/src/modules/slots/slots-2024-04-15/services/slots-output.service.ts (1)
TimeSlots(7-9)packages/platform/types/slots/slots-2024-09-04/inputs/get-slots-input.pipe.ts (2)
GetSlotsInput_2024_09_04(14-18)GetSlotsInputWithRouting_2024_09_04(20-25)
⏰ 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). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Merge reports / merge-reports
- GitHub Check: Security Check
🔇 Additional comments (19)
apps/api/v2/src/modules/organizations/routing-forms/outputs/create-routing-form-response.output.ts (1)
92-92: LGTM! Documentation clarification improves API usability.The updated description clearly indicates that the routing information can be passed directly to the booking API, which provides valuable guidance for API consumers.
docs/api-reference/v2/openapi.json (3)
21530-21534: Double-check enum consistency after droppingRESERVATION_EXPIRED.
"FORM_SUBMITTED_NO_EVENT"is now the last element in two duplicated enum arrays.
Please verify that
- Every enum occurrence (input/output DTOs, OAuth client output, etc.) was updated identically, and
- No stray
"RESERVATION_EXPIRED"values remain anywhere in this file (or the codebase) that could confuse schema-driven clients.A quick
rg RESERVATION_EXPIRED docs/api-reference/v2/openapi.jsonshould confirm.
If any leftover values exist, remove them or regenerate the spec.Also applies to: 21607-21611
24803-24808:bn(Bengali) added – ensure language lists stay alphabetised & synchronised.The language enum wasn’t kept in alphabetical order;
"bn"was appended after"zh-TW".
While non-breaking, keeping the list ordered improves diff readability and helps avoid accidental duplicates across schemas.
Please sort the array or confirm that other language enums (e.g. attendee output) now also include"bn".
26222-26225: LGTM – multi-line formatting only.The
MarkAbsentAttendeeschema still correctly requires"email"and"absent".
No functional changes detected.apps/api/v2/swagger/documentation.json (2)
21530-21534: Confirm trigger list consistency across all specs
"FORM_SUBMITTED_NO_EVENT"looks good, but please double-check that
- the constant list in the source code (
WebhookTriggerenum / constants)- every other OpenAPI / Swagger artefact
were updated in the same commit; otherwise consumers may see diverging specs.
23612-23618: Clarified description: 👍The updated wording (“could be passed as-is…”) is clearer and helps API users understand the field’s purpose.
apps/api/v2/src/modules/organizations/routing-forms/services/shared-routing-form-response.service.ts (1)
58-64: LGTM! Clean refactoring to use routing-specific slot method.The change from
getAvailableSlotstogetAvailableSlotsWithRoutingwith inline parameter merging is a good improvement. This approach is more explicit about routing-specific functionality and eliminates the need for the intermediateparamsForGetAvailableSlotsobject construction.packages/platform/types/slots/slots-2024-09-04/inputs/get-slots-input.pipe.ts (1)
20-25: Well-structured type extension for routing parameters.The new
GetSlotsInputWithRouting_2024_09_04type properly extends the baseGetSlotsInput_2024_09_04with routing-specific optional fields. This separation of concerns improves API clarity and maintainability by distinguishing between standard slot queries and routing-aware queries.packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts (1)
1-1: Good separation of base input types from routing extensions.The updated import reflects the proper architectural separation where routing-specific properties are moved out of the base
GetAvailableSlotsInput_2024_09_04class. This makes the base input type cleaner and more focused on its core responsibility.apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts (1)
1-169: Excellent test coverage for the new routing-aware slot method.The test suite is well-structured with:
- Proper dependency mocking and module setup
- Clear organization of shared test data for reusability
- Comprehensive verification that
getAvailableSlotsWithRoutingcorrectly forwards routing parameters to the underlyingavailableSlotsService.getAvailableSlots- Good assertion that verifies the transformation of
start/endtostartTime/endTimeand inclusion of routing-specific fieldsThis provides confidence that the new method works as intended and maintains the contract with the underlying service.
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts (3)
22-40: Well-designed type separation for base and routing-aware transformations.The new types
TransformedGetSlotsQueryandTransformedGetSlotsQueryWithRoutingproperly separate concerns. The routing-aware type extends the base type with appropriate routing fields, including proper nullability and default value handling.
54-54: Good explicit typing for the base transformation method.Making the return type explicit as
Promise<TransformedGetSlotsQuery>improves code clarity and type safety by clearly indicating this method handles only base slot queries without routing parameters.
85-100: Excellent implementation of routing-aware transformation using composition.The new
transformRoutingGetSlotsQuerymethod follows good design principles by:
- Properly extracting routing-specific fields from the input
- Delegating base transformation to the existing method (DRY principle)
- Adding routing fields with appropriate defaults (null/false)
- Maintaining type safety with the
GetSlotsInputWithRouting_2024_09_04input typeThis approach ensures consistency and reduces code duplication while extending functionality.
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.ts (6)
6-10: LGTM: Import updates support the refactoring.The imports correctly include the new transformation types (
TransformedGetSlotsQuery,TransformedGetSlotsQueryWithRouting) needed for the refactored service methods.
25-31: LGTM: Platform type imports align with the new routing functionality.The imports correctly include the new routing-aware input type (
GetSlotsInputWithRouting_2024_09_04) and theSlotFormatenum needed for the refactored methods.
41-41: LGTM: Type alias improves code readability.The
TransformedSlotsQueryunion type clearly expresses that the helper method can handle both regular and routing-aware transformed queries.
55-81: LGTM: Well-designed helper method follows DRY principle.The
fetchAndFormatSlotsmethod effectively consolidates the common slot fetching and formatting logic, reducing code duplication betweengetAvailableSlotsandgetAvailableSlotsWithRouting. The error handling is preserved and the method signature is clean.
83-86: LGTM: Refactored method maintains clean separation of concerns.The
getAvailableSlotsmethod is now focused solely on transforming regular slot queries and delegating to the centralizedfetchAndFormatSlotshelper. This follows the single responsibility principle effectively.
88-91: LGTM: New routing method follows consistent patterns.The
getAvailableSlotsWithRoutingmethod maintains consistency with the existinggetAvailableSlotsmethod while properly handling routing-specific transformations. The method signature and implementation are clean and follow the established patterns.
| ) {} | ||
|
|
||
| async getAvailableSlots(query: GetSlotsInput_2024_09_04) { | ||
| private async fetchAndFormatSlots(queryTransformed: TransformedSlotsQuery, format?: SlotFormat) { |
There was a problem hiding this comment.
Abstracted out for reuse across the two ways to getAvailableSlots
…service - Add error scenario tests for NotFoundException, invalid time range, and generic errors - Add edge case tests for null/undefined parameters and empty arrays - Improve test coverage for getAvailableSlotsWithRouting method - Mock SlotsInputService properly to enable isolated unit testing
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.ts
Outdated
Show resolved
Hide resolved
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts
Outdated
Show resolved
Hide resolved
…etSlotsQuery in slot services
- Fix import path from '@/lib/services/AvailableSlots' to '@/lib/services/available-slots.service' - Resolves unit test failure due to case sensitivity/naming mismatch - All API v2 tests now pass (9 test suites, 142 tests) Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
…unit tests (#22264) * chore: remove unnecessary logs and fix documentation * refactor: extract GetSlotsInputWithRouting type and eliminate code duplication - Move GetSlotsInputWithRouting_2024_09_04 type to platform-types package for reuse - Refactor slots service to eliminate duplicate error handling logic - Fix TypeScript errors in slots service tests by adding missing type property - Update test expectations to match new implementation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: improve type safety in slots service - Export explicit types from slots-input.service for transformed queries - Replace 'any' type with proper TransformedSlotsQuery union type - Re-implement fetchAndFormatSlots abstraction to eliminate code duplication - Revert unrelated console.log in router.controller.ts 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: simplify slots service implementation - Remove intermediate variable assignment in getAvailableSlotsWithRouting - Update test to match simplified routing parameters structure * test: add comprehensive error handling and edge case tests for slots service - Add error scenario tests for NotFoundException, invalid time range, and generic errors - Add edge case tests for null/undefined parameters and empty arrays - Improve test coverage for getAvailableSlotsWithRouting method - Mock SlotsInputService properly to enable isolated unit testing * 📝 CodeRabbit Chat: Rename TransformedGetSlotsQuery types to InternalGetSlotsQuery in slot services * fix: correct import path for AvailableSlotsService in slots service test - Fix import path from '@/lib/services/AvailableSlots' to '@/lib/services/available-slots.service' - Resolves unit test failure due to case sensitivity/naming mismatch - All API v2 tests now pass (9 test suites, 142 tests) Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
What does this PR do?
Addresses the followup mentioned at #22239 (comment)
Mandatory Tasks (DO NOT REMOVE)