Dev release#2
Conversation
…is not met Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a contextType filter for surveys and refactors the APIResponse utility to pass result objects directly rather than spreading them. It also implements conditional logic for field validation in survey responses. Feedback focuses on ensuring surveyId remains mandatory in the response DTO for better error reporting and optimizing the validation logic by using a Map for field lookups to improve performance from O(N^2) to O(N).
| @IsOptional() | ||
| @IsUUID() | ||
| surveyId: string; |
There was a problem hiding this comment.
The surveyId field should be mandatory when creating a response. Marking it as @IsOptional() allows requests with a missing surveyId to pass validation, which will subsequently cause a NotFoundException in the service layer when it fails to fetch the survey. It is better to enforce this requirement at the DTO level to provide a clearer error response (400 Bad Request).
| @IsOptional() | |
| @IsUUID() | |
| surveyId: string; | |
| @IsUUID() | |
| surveyId: string; |
| private isConditionMet(field: any, allFields: any[], responseData: Record<string, any>): boolean { | ||
| const logic = field.conditionalLogic; | ||
| if (!logic || !logic.depends_on || !logic.show_if) return true; | ||
|
|
||
| const parentField = allFields.find(f => f.fieldName === logic.depends_on); | ||
| if (!parentField) return true; | ||
|
|
||
| const parentValue = responseData[parentField.fieldId]; | ||
| const showIf: string[] = logic.show_if; | ||
|
|
||
| if (Array.isArray(parentValue)) { | ||
| return parentValue.some(v => showIf.includes(v)); | ||
| } | ||
| return showIf.includes(parentValue); | ||
| } |
There was a problem hiding this comment.
The current implementation of isConditionMet performs an O(N) search using allFields.find(). Since this method is called inside a loop in validateRequiredFields, the overall complexity becomes O(N^2). Changing the parameter to a Map will allow for O(1) lookups, which is significantly more efficient for surveys with a large number of fields.
| private isConditionMet(field: any, allFields: any[], responseData: Record<string, any>): boolean { | |
| const logic = field.conditionalLogic; | |
| if (!logic || !logic.depends_on || !logic.show_if) return true; | |
| const parentField = allFields.find(f => f.fieldName === logic.depends_on); | |
| if (!parentField) return true; | |
| const parentValue = responseData[parentField.fieldId]; | |
| const showIf: string[] = logic.show_if; | |
| if (Array.isArray(parentValue)) { | |
| return parentValue.some(v => showIf.includes(v)); | |
| } | |
| return showIf.includes(parentValue); | |
| } | |
| private isConditionMet(field: any, fieldsMap: Map<string, any>, responseData: Record<string, any>): boolean { | |
| const logic = field.conditionalLogic; | |
| if (!logic || !logic.depends_on || !logic.show_if) return true; | |
| const parentField = fieldsMap.get(logic.depends_on); | |
| if (!parentField) return true; | |
| const parentValue = responseData[parentField.fieldId]; | |
| const showIf: string[] = logic.show_if; | |
| if (Array.isArray(parentValue)) { | |
| return parentValue.some(v => showIf.includes(v)); | |
| } | |
| return showIf.includes(parentValue); | |
| } |
| const allFields = (survey.sections || []).flatMap((s: any) => s.fields || []); | ||
|
|
||
| for (const section of survey.sections || []) { | ||
| for (const field of section.fields || []) { | ||
| if (field.isRequired) { | ||
| const value = surveyResponse.responseData[field.fieldId]; | ||
| const fileIds = surveyResponse.fileUploadIds[field.fieldId]; | ||
| if (field.isRequired && this.isConditionMet(field, allFields, surveyResponse.responseData ?? {})) { | ||
| const value = (surveyResponse.responseData ?? {})[field.fieldId]; | ||
| const fileIds = (surveyResponse.fileUploadIds ?? {})[field.fieldId]; |
There was a problem hiding this comment.
To optimize the validation process and improve code readability, create a Map for field lookups once and extract responseData and fileUploadIds into local variables. This avoids repeated O(N) searches and redundant nullish coalescing operations inside the nested loops.
const allFields = (survey.sections || []).flatMap((s: any) => s.fields || []);
const fieldsMap = new Map(allFields.map(f => [f.fieldName, f]));
const responseData = surveyResponse.responseData ?? {};
const fileUploadIds = surveyResponse.fileUploadIds ?? {};
for (const section of survey.sections || []) {
for (const field of section.fields || []) {
if (field.isRequired && this.isConditionMet(field, fieldsMap, responseData)) {
const value = responseData[field.fieldId];
const fileIds = fileUploadIds[field.fieldId];
No description provided.