PM-1504 create edit scorecard #20
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| data: { | ||
| ...(mapScorecardRequestForCreate({ | ||
| ...body, | ||
| createdBy: user.isMachine ? 'System' : (user.userId as string), |
There was a problem hiding this comment.
Consider using a type guard or type assertion to ensure user.userId is of type string before casting, as casting without checking can lead to runtime errors if userId is not a string.
| ...(mapScorecardRequestForCreate({ | ||
| ...body, | ||
| createdBy: user.isMachine ? 'System' : (user.userId as string), | ||
| updatedBy: user.isMachine ? 'System' : (user.userId as string), |
There was a problem hiding this comment.
Similar to the previous comment, ensure user.userId is of type string before casting to avoid potential runtime errors.
| @@ -44,7 +53,7 @@ export class ScoreCardService { | |||
| }, | |||
| }); | |||
|
|
|||
| return data as ScorecardWithGroupResponseDto; | |||
| return data as unknown as ScorecardWithGroupResponseDto; | |||
There was a problem hiding this comment.
Casting data as unknown before casting to ScorecardWithGroupResponseDto is generally unnecessary unless there is a specific reason to bypass TypeScript's type checking. Consider directly casting data to ScorecardWithGroupResponseDto if possible, or provide a rationale for this approach in the documentation or code comments.
| ): Promise<ScorecardWithGroupResponseDto> { | ||
| const data = await this.prisma.scorecard | ||
| .update({ | ||
| where: { id }, | ||
| data: mapScorecardRequestToDto(body), | ||
| data: mapScorecardRequestToDto({ |
There was a problem hiding this comment.
The type assertion as any is used here. Consider refining the types to avoid using any, which can lead to runtime errors and makes the code less type-safe. Ensure that mapScorecardRequestToDto returns the correct type.
| // }); | ||
| async cloneScorecard( | ||
| id: string, | ||
| user: { userId?: string; isMachine: boolean }, |
There was a problem hiding this comment.
The user parameter is destructured to access userId and isMachine, but userId is marked as optional. Consider handling the case where userId might be undefined to avoid potential runtime errors.
| })), | ||
| }, | ||
| }), | ||
| ) as any; |
There was a problem hiding this comment.
The as any type assertion is used here. Consider refining the types to avoid using any, which can lead to less type safety and potential runtime errors.
| guidelines: string; | ||
|
|
||
| @ApiProperty({ description: 'The weight of the question', example: 10 }) | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider adding a validation decorator like @IsPositive() or @Min(0) to ensure that the weight property is a positive number, as negative weights might not make sense in this context.
| requiresUpload: boolean; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Minimum scale value (if applicable)', | ||
| example: 0, | ||
| required: false, | ||
| }) | ||
| @IsOptional() | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider specifying the @IsNumber() decorator options to ensure scaleMin is validated correctly. For example, you can use { allowNaN: false, allowInfinity: false } to prevent invalid number values.
| scaleMin?: number; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Maximum scale value (if applicable)', | ||
| example: 9, | ||
| required: false, | ||
| }) | ||
| @IsOptional() | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider adding validation options to @IsNumber() to specify constraints like allowNaN or allowInfinity if applicable, to ensure the scaleMax value is strictly a valid number.
| @@ -79,16 +109,24 @@ export class ScorecardQuestionResponseDto extends ScorecardQuestionBaseDto { | |||
| } | |||
|
|
|||
| export class ScorecardSectionBaseDto { | |||
| @ApiProperty({ description: 'The id of the section', example: 'abc' }) | |||
| @IsOptional() | |||
There was a problem hiding this comment.
Consider using @IsUUID() if the id is expected to be a UUID. This will ensure the id is validated as a UUID string.
| name: string; | ||
|
|
||
| @ApiProperty({ description: 'The weight of the section', example: 20 }) | ||
| @IsNumber() | ||
| weight: number; |
There was a problem hiding this comment.
Add the @IsOptional() decorator to weight if it is not a required field. Otherwise, consider adding @IsNotEmpty() to ensure it is not left empty.
| weight: number; | ||
|
|
||
| @ApiProperty({ description: 'Sort order of the section', example: 1 }) | ||
| @IsNumber() | ||
| sortOrder: number; |
There was a problem hiding this comment.
Add the @IsOptional() decorator to sortOrder if it is not a required field. Otherwise, consider adding @IsNotEmpty() to ensure it is not left empty.
| weight: number; | ||
|
|
||
| @ApiProperty({ description: 'Sort order of the section', example: 1 }) | ||
| @IsNumber() | ||
| sortOrder: number; | ||
|
|
||
| questions: any[]; |
There was a problem hiding this comment.
Consider specifying the type of elements in the questions array for better type safety. For example, questions: QuestionDto[]; if QuestionDto is the expected type.
| @ApiProperty({ description: 'The id of the group', example: 'abc' }) | ||
| @IsOptional() | ||
| @IsString() | ||
| id: string; |
There was a problem hiding this comment.
Consider adding a validation decorator such as @IsUUID() if the id is expected to be a UUID. This will ensure that the id field adheres to a specific format.
| name: string; | ||
|
|
||
| @ApiProperty({ description: 'The weight of the group', example: 30 }) | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider adding a validation decorator such as @IsInt() if the weight is expected to be an integer. This will ensure that the weight field is validated as an integer value.
| weight: number; | ||
|
|
||
| @ApiProperty({ description: 'Sort order of the group', example: 1 }) | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider adding a validation decorator such as @IsInt() if the sortOrder is expected to be an integer. This will ensure that the sortOrder field is validated as an integer value.
| weight: number; | ||
|
|
||
| @ApiProperty({ description: 'Sort order of the group', example: 1 }) | ||
| @IsNumber() | ||
| sortOrder: number; | ||
|
|
||
| sections: any[]; |
There was a problem hiding this comment.
Consider specifying the type of elements in the sections array for better type safety and clarity. For example, if sections is an array of objects of a specific type, you can define it as sections: SpecificType[];.
| version: string; | ||
|
|
||
| @ApiProperty({ description: 'The minimum score', example: 0 }) | ||
| @IsNumber() | ||
| @Min(0) | ||
| @IsSmallerThan('maxScore') |
There was a problem hiding this comment.
The decorator @IsSmallerThan('maxScore') is not a standard class-validator decorator. Consider using @Max() with a dynamic value or implementing a custom validation logic.
| minScore: number; | ||
|
|
||
| @ApiProperty({ description: 'The maximum score', example: 100 }) | ||
| @IsNumber() | ||
| @IsGreaterThan('minScore') |
There was a problem hiding this comment.
The decorator @IsGreaterThan('minScore') is not a standard class-validator decorator. Consider using @Min() with a dynamic value or implementing a custom validation logic.
| description: 'The creation timestamp', | ||
| example: '2023-10-01T00:00:00Z', | ||
| }) | ||
| /** |
There was a problem hiding this comment.
The comment suggests that createdAt, createdBy, updatedAt, and updatedBy should not be editable via API, but they are still part of the DTO. Consider removing these properties from the DTO if they should not be exposed or editable.
| @@ -218,6 +265,9 @@ export class ScorecardRequestDto extends ScorecardBaseWithGroupsDto { | |||
| description: 'The list of groups associated with the scorecard', | |||
| type: [ScorecardGroupRequestDto], | |||
| }) | |||
| @IsArray() | |||
There was a problem hiding this comment.
Consider adding a validation decorator such as @ArrayNotEmpty() to ensure that the scorecardGroups array is not empty, if this is a requirement for the scorecard.
| const userFields = { | ||
| createdBy: request.createdBy, | ||
| ...(request.createdBy ? { createdBy: request.createdBy } : {}), |
There was a problem hiding this comment.
The use of the spread operator with a conditional expression is correct, but it might be clearer to explicitly state the intention. Consider adding a brief explanation in the documentation or elsewhere in the codebase to clarify why createdBy is conditionally included.
| @@ -302,3 +352,76 @@ export function mapScorecardRequestToDto(request: ScorecardRequestDto) { | |||
| }, | |||
| }; | |||
| } | |||
|
|
|||
| export function mapScorecardRequestToDto(request: ScorecardRequestDto) { | |||
There was a problem hiding this comment.
The function mapScorecardRequestToDto is defined twice in the same file, which can lead to confusion and potential errors. Consider removing the duplicate definition or renaming one of the functions if they serve different purposes.
| ...userFields, | ||
| scorecardGroups: { | ||
| upsert: request.scorecardGroups.map((group) => ({ | ||
| where: { id: (group as any).id as string }, |
There was a problem hiding this comment.
Using as any for type casting can lead to runtime errors and makes the code harder to maintain. Consider defining proper TypeScript interfaces or types for group, section, and question to ensure type safety.
| updatedBy: request.updatedBy, | ||
| questions: { | ||
| upsert: section.questions.map((question) => ({ | ||
| where: { id: (question as any).id as string }, |
There was a problem hiding this comment.
Using as any for type casting can lead to runtime errors and makes the code harder to maintain. Consider defining proper TypeScript interfaces or types for question to ensure type safety.
| constraints: [relatedPropertyName, comparatorFn], | ||
| options: validationOptions, | ||
| validator: { | ||
| validate(value: Date, args: ValidationArguments) { |
There was a problem hiding this comment.
Consider using a more specific type for value instead of Date, as the comparatorFn seems to handle numbers. This could prevent potential runtime errors if the wrong type is passed.
| string, | ||
| ComparatorFn, | ||
| ]; | ||
| const relatedValue = (args.object as any)[relatedPropertyName]; |
There was a problem hiding this comment.
Using (args.object as any) can lead to potential type safety issues. Consider defining an interface for args.object to ensure type safety and improve code readability.
| ]; | ||
| const relatedValue = (args.object as any)[relatedPropertyName]; | ||
|
|
||
| if (typeof comparatorFn !== 'function') { |
There was a problem hiding this comment.
The check for typeof comparatorFn !== 'function' should ideally be done before calling registerDecorator to avoid registering an invalid decorator.
https://topcoder.atlassian.net/browse/PM-1504 - Create/Edit Scorecard UI
NOTE: I used new ValidationPipe in the controller method, because I needed to pass
whitelist: trueto strip off any non-wanted props.By default, the global pipe is using
whitelist: false, and since we're pretty far in the development process with the api, I didn't want to turn it on globally (though it would be better).