fix: Create RoutingFormResponseService to get field value from identifier #22396
fix: Create RoutingFormResponseService to get field value from identifier #22396
RoutingFormResponseService to get field value from identifier #22396Conversation
keithwillcode
added
core
|
|
||
| response[field.id] = { | ||
| label: field.label, | ||
| identifier: field?.identifier || null, |
There was a problem hiding this comment.
This was generating the records to store. The field identifier wasn't being passed. All form fields should have an identifier
| { | ||
| value: number | string | string[]; | ||
| label: string; | ||
| identifier?: string; |
There was a problem hiding this comment.
Ensure this property exists
| response: z.record( | ||
| z.object({ | ||
| label: z.string(), | ||
| identifier: z.string().optional(), |
There was a problem hiding this comment.
Ensure this property exists
There was a problem hiding this comment.
I think we should still keep it optional in this PR and later make it nullable in a separate PR as it might cause failure for certain embeds that are already loaded.
dosubot
bot
added
routing-forms
|
✅ No security or compliance issues detected. Reviewed everything up to a125b9f. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/10/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
E2E results are ready! |
| [field.id]: { | ||
| label: field.label, | ||
| identifier: field?.identifier, | ||
| identifier: field?.identifier || null, |
There was a problem hiding this comment.
If we are relying on identifier to be always set then I think we should use https://github.com/calcom/cal.com/blob/add-teams-routing-forms-response-record-endpoint-back/packages/app-store/routing-forms/lib/getFieldIdentifier.ts#L3
Because, identifier property is directly set only when user modifies the identifier otherwise the default value which is the label is used
There was a problem hiding this comment.
My concern is why it is required to store the identifier.
If at some place we need the identifier from the response, we could always query the fields by field.id as that is always there. This is how it has been thought of initially atleast.
Also, it won't fix existing responses and for those we would still need to derive identifier on the fly.
![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 (3)
packages/lib/server/repository/routingFormResponse.repository.ts (1)
3-33: Refactor static-only class to simple functions.As identified in previous reviews, this class contains only static members and should be refactored to simple exported functions for better TypeScript/JavaScript idioms.
packages/lib/server/service/routingForm/routingFormResponse.service.ts (2)
46-49: Fix error message to reflect parsing failure.The error message incorrectly states "Form response not found" when the actual issue is a parsing failure.
70-72: Add validation for field value existence.The code assumes
this.response[field.id]exists, but it might be undefined if the field wasn't answered in the form response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/app-store/salesforce/lib/CrmService.ts(4 hunks)packages/lib/server/repository/routingFormResponse.repository.ts(1 hunks)packages/lib/server/service/routingForm/routingFormResponse.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-store/salesforce/lib/CrmService.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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.
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.
packages/lib/server/repository/routingFormResponse.repository.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/lib/server/service/routingForm/routingFormResponse.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/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.
🪛 Biome (1.9.4)
packages/lib/server/repository/routingFormResponse.repository.ts
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ 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
🔇 Additional comments (2)
packages/lib/server/service/routingForm/routingFormResponse.service.ts (2)
10-23: Well-structured class with proper typing.The class constructor and structure follow good TypeScript patterns with proper dependency injection and Zod type inference.
61-68: Good improvements addressing previous feedback.The method now properly uses
getFieldIdentifierand returns null instead of throwing an error, which addresses previous review feedback.
packages/lib/server/service/routingForm/routingFormResponse.service.ts
Outdated
Show resolved
Hide resolved
* chore: Provide a suggestion for pr 22396, new structure * Refactor to create two create methods with bookingUid and id * Use `createWithBookingUid` * Extract routing form response parser to seperate util * Added more and improved test cases * Fix --------- Co-authored-by: Joe Au-Yeung <j.auyeung419@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.ts (1)
18-18: Remove unnecessary async keyword.The test function is marked as
asyncbut doesn't useawaitand tests a synchronous function.- it("returns the correct value for an existing field identifier", async () => { + it("returns the correct value for an existing field identifier", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/app-store/salesforce/lib/CrmService.ts(4 hunks)packages/lib/server/repository/PrismaRoutingFormResponseRepository.ts(1 hunks)packages/lib/server/repository/RoutingFormResponseRepository.interface.ts(1 hunks)packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.ts(1 hunks)packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.ts(1 hunks)packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.ts(1 hunks)packages/lib/server/service/routingForm/responseData/parseRoutingFormResponse.test.ts(1 hunks)packages/lib/server/service/routingForm/responseData/parseRoutingFormResponse.ts(1 hunks)packages/lib/server/service/routingForm/responseData/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/lib/server/service/routingForm/responseData/types.ts
- packages/lib/server/repository/RoutingFormResponseRepository.interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-store/salesforce/lib/CrmService.ts
🧰 Additional context used
🧠 Learnings (7)
📓 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.
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.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/lib/server/service/routingForm/responseData/parseRoutingFormResponse.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/lib/server/service/routingForm/responseData/parseRoutingFormResponse.test.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/lib/server/service/routingForm/RoutingFormResponseDataFactory.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/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.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/lib/server/repository/PrismaRoutingFormResponseRepository.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 (4)
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.ts (2)
packages/lib/server/service/routingForm/responseData/types.ts (1)
RoutingFormResponseData(6-9)packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.ts (1)
findFieldValueByIdentifier(9-21)
packages/lib/server/service/routingForm/responseData/parseRoutingFormResponse.ts (1)
packages/lib/server/service/routingForm/responseData/types.ts (1)
RoutingFormResponseData(6-9)
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.ts (1)
packages/lib/server/service/routingForm/responseData/types.ts (1)
RoutingFormResponseData(6-9)
packages/lib/server/repository/PrismaRoutingFormResponseRepository.ts (1)
packages/lib/server/repository/RoutingFormResponseRepository.interface.ts (1)
RoutingFormResponseRepositoryInterface(3-11)
⏰ 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
🔇 Additional comments (19)
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.ts (1)
1-16: Well-structured test setup with appropriate mock data.The test imports and data structure correctly represent the expected
RoutingFormResponseDatatype, providing good coverage for testing the function's behavior.packages/lib/server/service/routingForm/responseData/parseRoutingFormResponse.ts (2)
1-6: Good use of unknown types for runtime validation.The function signature correctly uses
unknowntypes for parameters that will be validated at runtime using Zod schemas, which is the appropriate approach for parsing untrusted data.
7-10: Clean and straightforward parsing implementation.The function correctly uses Zod schemas for validation and returns the expected structured data. The implicit throwing behavior on validation failure is appropriate for this type of parsing function.
packages/lib/server/service/routingForm/responseData/parseRoutingFormResponse.test.ts (8)
1-15: Well-structured test setup with comprehensive mock data.The test imports and data setup correctly represent the expected input structure for the parsing function, providing a solid foundation for testing various scenarios.
16-20: Good basic functionality test.The test correctly verifies that valid input produces the expected parsed output with proper structure and values.
22-31: Good coverage of optional properties.The test ensures that optional properties like
labelin the response schema are correctly preserved during parsing.
33-53: Excellent coverage of array values for multi-select fields.The test thoroughly covers the scenario where field values can be arrays, with appropriate field definition including options. This ensures the parsing function handles different value types correctly.
55-61: Good negative test for invalid response structure.The test correctly verifies that the function throws when encountering invalid response data structure, ensuring proper validation.
63-72: Good validation of required field properties.The test ensures that required properties like
idin field definitions are properly validated and the function throws appropriately on missing required data.
74-93: Good coverage of optional field properties.The test ensures that optional properties like
selectTextanddeletedin field definitions are correctly preserved during parsing, demonstrating good schema flexibility.
95-98: Good basic type validation test.The test ensures that the function properly validates input types and throws when receiving non-object/non-array inputs as expected.
packages/lib/server/repository/PrismaRoutingFormResponseRepository.ts (3)
1-7: Good dependency injection pattern with default prisma instance.The class correctly implements the repository interface with proper dependency injection, allowing for easy testing with different Prisma client instances.
9-22: Efficient query implementation with selective field inclusion.The method correctly implements the interface contract with efficient field selection, including only the necessary
fieldsfrom the related form.
24-37: Consistent implementation with proper booking UID query.The method correctly queries by
routedToBookingUidand maintains consistency with the other method's structure and field selection.packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.ts (3)
1-7: Good type design with discriminated union for error handling.The function uses appropriate imports and defines a well-structured discriminated union return type that provides type safety for both success and error cases.
9-16: Correct field lookup with informative error handling.The function correctly searches for fields using the helper function and provides clear error messages when fields are not found.
18-21: Robust value retrieval with proper null handling.The function correctly retrieves field values using optional chaining and nullish coalescing, providing appropriate defaults for missing values.
packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.ts (2)
1-9: LGTM! Good use of dependency injection pattern.The imports are properly structured using type imports where appropriate, and the Dependencies interface provides clean separation of concerns for dependency injection.
11-12: LGTM! Clean dependency injection implementation.The constructor properly uses readonly modifier and dependency injection pattern, which promotes testability and maintainability.
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.ts
Show resolved
Hide resolved
packages/lib/server/service/routingForm/responseData/findFieldValueByIdentifier.test.ts
Show resolved
Hide resolved
| async createWithResponseId(responseId: number) { | ||
| const log = this.deps.logger.getSubLogger({ | ||
| prefix: ["[routingFormFieldService]", { responseId }], | ||
| }); | ||
|
|
||
| const formResponse = await this.deps.routingFormResponseRepo.findById(responseId); | ||
|
|
||
| if (!formResponse) { | ||
| log.error("Form response not found"); | ||
| throw new Error("Form response not found"); | ||
| } | ||
|
|
||
| return parseRoutingFormResponse(formResponse.response, formResponse.form.fields); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address code duplication and improve error handling.
This method has the same issues as createWithBookingUid and significant code duplication. Consider refactoring to reduce duplication.
async createWithResponseId(responseId: number) {
+ if (!responseId || responseId <= 0) {
+ throw new Error("Valid response ID is required");
+ }
+
const log = this.deps.logger.getSubLogger({
- prefix: ["[routingFormFieldService]", { responseId }],
+ prefix: ["[RoutingFormResponseDataFactory]", { responseId }],
});
const formResponse = await this.deps.routingFormResponseRepo.findById(responseId);
if (!formResponse) {
- log.error("Form response not found");
- throw new Error("Form response not found");
+ log.error("Form response not found for response ID");
+ throw new Error(`Form response not found for response ID: ${responseId}`);
}
return parseRoutingFormResponse(formResponse.response, formResponse.form.fields);
}Optional refactor to reduce code duplication:
+ private async createParsedResponse<T>(
+ identifier: T,
+ identifierName: string,
+ fetchFn: (id: T) => Promise<any>
+ ) {
+ const log = this.deps.logger.getSubLogger({
+ prefix: ["[RoutingFormResponseDataFactory]", { [identifierName]: identifier }],
+ });
+
+ const formResponse = await fetchFn(identifier);
+
+ if (!formResponse) {
+ log.error(`Form response not found for ${identifierName}`);
+ throw new Error(`Form response not found for ${identifierName}: ${identifier}`);
+ }
+
+ return parseRoutingFormResponse(formResponse.response, formResponse.form.fields);
+ }
+
async createWithBookingUid(bookingUid: string) {
+ if (!bookingUid?.trim()) {
+ throw new Error("Booking UID is required");
+ }
+ return this.createParsedResponse(
+ bookingUid,
+ "bookingUid",
+ (uid) => this.deps.routingFormResponseRepo.findByBookingUid(uid)
+ );
}
async createWithResponseId(responseId: number) {
+ if (!responseId || responseId <= 0) {
+ throw new Error("Valid response ID is required");
+ }
+ return this.createParsedResponse(
+ responseId,
+ "responseId",
+ (id) => this.deps.routingFormResponseRepo.findById(id)
+ );
}📝 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 createWithResponseId(responseId: number) { | |
| const log = this.deps.logger.getSubLogger({ | |
| prefix: ["[routingFormFieldService]", { responseId }], | |
| }); | |
| const formResponse = await this.deps.routingFormResponseRepo.findById(responseId); | |
| if (!formResponse) { | |
| log.error("Form response not found"); | |
| throw new Error("Form response not found"); | |
| } | |
| return parseRoutingFormResponse(formResponse.response, formResponse.form.fields); | |
| } | |
| async createWithResponseId(responseId: number) { | |
| + if (!responseId || responseId <= 0) { | |
| + throw new Error("Valid response ID is required"); | |
| + } | |
| const log = this.deps.logger.getSubLogger({ | |
| - prefix: ["[routingFormFieldService]", { responseId }], | |
| + prefix: ["[RoutingFormResponseDataFactory]", { responseId }], | |
| }); | |
| const formResponse = await this.deps.routingFormResponseRepo.findById(responseId); | |
| if (!formResponse) { | |
| - log.error("Form response not found"); | |
| - throw new Error("Form response not found"); | |
| + log.error("Form response not found for response ID"); | |
| + throw new Error(`Form response not found for response ID: ${responseId}`); | |
| } | |
| return parseRoutingFormResponse(formResponse.response, formResponse.form.fields); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.ts
around lines 29 to 42, the createWithResponseId method duplicates logic found in
createWithBookingUid, especially in error handling and logging. Refactor by
extracting the common logic for fetching and validating the form response into a
shared private method that both public methods can call. This will reduce
duplication and centralize error handling and logging for missing form
responses.
| async createWithBookingUid(bookingUid: string) { | ||
| const log = this.deps.logger.getSubLogger({ | ||
| prefix: ["[routingFormFieldService]", { bookingUid }], | ||
| }); | ||
|
|
||
| const formResponse = await this.deps.routingFormResponseRepo.findByBookingUid(bookingUid); | ||
|
|
||
| if (!formResponse) { | ||
| log.error("Form response not found"); | ||
| throw new Error("Form response not found"); | ||
| } | ||
|
|
||
| return parseRoutingFormResponse(formResponse.response, formResponse.form.fields); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging consistency.
Several improvements can be made:
- Incorrect logger prefix: The prefix shows "[routingFormFieldService]" but this is a factory class, not a field service.
- Generic error handling: Consider using more specific error types for better error categorization.
- Input validation: No validation on the
bookingUidparameter.
async createWithBookingUid(bookingUid: string) {
+ if (!bookingUid?.trim()) {
+ throw new Error("Booking UID is required");
+ }
+
const log = this.deps.logger.getSubLogger({
- prefix: ["[routingFormFieldService]", { bookingUid }],
+ prefix: ["[RoutingFormResponseDataFactory]", { bookingUid }],
});
const formResponse = await this.deps.routingFormResponseRepo.findByBookingUid(bookingUid);
if (!formResponse) {
- log.error("Form response not found");
- throw new Error("Form response not found");
+ log.error("Form response not found for booking UID");
+ throw new Error(`Form response not found for booking UID: ${bookingUid}`);
}
return parseRoutingFormResponse(formResponse.response, formResponse.form.fields);
}📝 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 createWithBookingUid(bookingUid: string) { | |
| const log = this.deps.logger.getSubLogger({ | |
| prefix: ["[routingFormFieldService]", { bookingUid }], | |
| }); | |
| const formResponse = await this.deps.routingFormResponseRepo.findByBookingUid(bookingUid); | |
| if (!formResponse) { | |
| log.error("Form response not found"); | |
| throw new Error("Form response not found"); | |
| } | |
| return parseRoutingFormResponse(formResponse.response, formResponse.form.fields); | |
| } | |
| async createWithBookingUid(bookingUid: string) { | |
| if (!bookingUid?.trim()) { | |
| throw new Error("Booking UID is required"); | |
| } | |
| const log = this.deps.logger.getSubLogger({ | |
| prefix: ["[RoutingFormResponseDataFactory]", { bookingUid }], | |
| }); | |
| const formResponse = await this.deps.routingFormResponseRepo.findByBookingUid(bookingUid); | |
| if (!formResponse) { | |
| log.error("Form response not found for booking UID"); | |
| throw new Error(`Form response not found for booking UID: ${bookingUid}`); | |
| } | |
| return parseRoutingFormResponse(formResponse.response, formResponse.form.fields); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.ts
around lines 14 to 27, update the logger prefix to correctly reflect the factory
class instead of "[routingFormFieldService]". Add validation to check if the
bookingUid parameter is valid before proceeding. Replace the generic Error
thrown when the form response is not found with a more specific error type to
improve error categorization and handling. Ensure the logging and error throwing
are consistent with these changes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.test.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.
⏰ 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
🔇 Additional comments (5)
packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.test.ts (5)
11-21: LGTM! Mock objects are well-structured.The mock objects correctly implement the expected interfaces with proper method signatures.
22-31: LGTM! Excellent test setup with proper isolation.The test setup correctly uses
beforeEachto clear mocks and create fresh instances, ensuring proper test isolation.
34-53: Test logic is sound, but mock return value assertion needs alignment.The test correctly verifies the repository call and parsing function invocation. However, the assertion
expect(result).toBe("parsedData")assumes the mock returns this value, which should be configured in the mock setup.
66-95: LGTM! Consistent test pattern with good coverage.The tests follow the same well-structured pattern as the
createWithBookingUidtests, covering both success and error scenarios with appropriate assertions.
1-97: Comprehensive test suite with excellent coverage.This test suite provides thorough coverage of the
RoutingFormResponseDataFactoryclass with:
- Proper mocking of dependencies
- Test isolation with
beforeEach- Coverage of both success and error scenarios
- Appropriate assertions for each test case
- Consistent test patterns across methods
The tests effectively validate the factory's behavior and error handling.
| vi.mock("./responseData/parseRoutingFormResponse", () => ({ | ||
| parseRoutingFormResponse: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Configure the mock to return a value for consistent test behavior.
The parseRoutingFormResponse mock should return a value to make the test assertions work correctly. The tests expect result to be "parsedData" but the mock doesn't return anything.
vi.mock("./responseData/parseRoutingFormResponse", () => ({
- parseRoutingFormResponse: vi.fn(),
+ parseRoutingFormResponse: vi.fn().mockReturnValue("parsedData"),
}));📝 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.
| vi.mock("./responseData/parseRoutingFormResponse", () => ({ | |
| parseRoutingFormResponse: vi.fn(), | |
| })); | |
| vi.mock("./responseData/parseRoutingFormResponse", () => ({ | |
| parseRoutingFormResponse: vi.fn().mockReturnValue("parsedData"), | |
| })); |
🤖 Prompt for AI Agents
In
packages/lib/server/service/routingForm/RoutingFormResponseDataFactory.test.ts
around lines 7 to 9, the mock for parseRoutingFormResponse is defined but does
not return any value, causing test assertions expecting "parsedData" to fail.
Update the mock implementation to return "parsedData" so that the tests receive
the expected value and behave consistently.
4 participants
Refactor routing form response handling and revert identifier changes
Summary
This PR refactors how routing form responses are handled in the Salesforce CRM integration by introducing a new service layer, while reverting problematic changes to the identifier field that were causing E2E test failures.
Key Changes:
identifierfield fromFormResponsetype and related routing form components to fix E2E test failuresRoutingFormResponseServiceandRoutingFormResponseRepositoryto encapsulate routing form response logicThe original approach of storing identifier in response data caused type inconsistencies across routing forms components. Instead, this PR implements a cleaner architecture where identifiers are retrieved from the form definition when needed.
Review & Testing Checklist for Human
RoutingFormResponseServicehandles missing responses, invalid identifiers, and malformed data gracefullyRecommended test plan: Create a routing form with various field types, submit responses through different paths (direct form, router endpoint), then verify Salesforce integration can correctly extract field values using identifiers.
Diagram
%%{ init : { "theme" : "default" }}%% graph TD CrmService["packages/app-store/salesforce/<br/>lib/CrmService.ts"]:::major-edit ResponseService["packages/lib/server/service/<br/>routingForm/routingFormResponse.service.ts"]:::major-edit ResponseRepo["packages/lib/server/repository/<br/>routingFormResponse.repository.ts"]:::major-edit FormInputFields["packages/app-store/routing-forms/<br/>components/FormInputFields.tsx"]:::minor-edit TypesDef["packages/app-store/routing-forms/<br/>types/types.d.ts"]:::minor-edit TestFiles["Test files<br/>(handleResponse.test.ts,<br/>onFormSubmission.test.ts)"]:::minor-edit CrmService -->|"uses"| ResponseService ResponseService -->|"uses"| ResponseRepo ResponseRepo -->|"queries"| Database[("Routing Form<br/>Responses DB")]:::context FormInputFields -->|"creates"| Database TypesDef -->|"defines types for"| FormInputFields TestFiles -->|"tests"| ResponseService subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes