Feat/scorecards -> dev #27
Conversation
fic(PM-1585): Search scorecard modifications
fix(Filters): In search API
PM-1505 clone scorecard
fix(PM-1503): Open view score card as public API
…eate-edit-scorecard
…recard PM-1504 create edit scorecard
…recard PM-1504 - scorecard: make sure to handle scorecard update properly
…recard PM-1504 - remove required for guidelines
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import { WebhookController } from './webhook/webhook.controller'; | ||
| import { WebhookService } from './webhook/webhook.service'; | ||
| import { GiteaWebhookAuthGuard } from '../shared/guards/gitea-webhook-auth.guard'; | ||
| import { ScoreCardService } from './scorecard/scorecard.service'; |
There was a problem hiding this comment.
The import statement for ScoreCardService is added, but ensure that this service is actually used within this module. If it's not used, consider removing the import to keep the code clean and maintainable.
| } from 'src/dto/scorecard.dto'; | ||
| import { ChallengeTrack } from 'src/shared/enums/challengeTrack.enum'; | ||
| import { PrismaService } from '../../shared/modules/global/prisma.service'; | ||
| import { ScoreCardService } from './scorecard.service'; |
There was a problem hiding this comment.
The import statement for ScoreCardService should use consistent casing. Consider renaming ScoreCardService to ScorecardService to match the naming convention used in other parts of the codebase.
| import { ChallengeTrack } from 'src/shared/enums/challengeTrack.enum'; | ||
| import { PrismaService } from '../../shared/modules/global/prisma.service'; | ||
| import { ScoreCardService } from './scorecard.service'; | ||
| import { PaginationHeaderInterceptor } from 'src/interceptors/PaginationHeaderInterceptor'; |
There was a problem hiding this comment.
The import path for PaginationHeaderInterceptor should be checked for consistency. Ensure that the casing and path structure align with the project's conventions.
| }, | ||
| }); | ||
| return data as ScorecardResponseDto; | ||
| @Body(new ValidationPipe({ whitelist: true, transform: true })) |
There was a problem hiding this comment.
Consider adding a validation pipe to ensure that the user parameter is properly validated and transformed if necessary.
| body: ScorecardRequestDto, | ||
| @User() user: JwtUser, | ||
| ): Promise<ScorecardWithGroupResponseDto> { | ||
| return await this.scorecardService.addScorecard(body, user); |
There was a problem hiding this comment.
The await keyword is unnecessary here since the function is returning a promise directly. Consider removing await for cleaner code.
| body: ScorecardRequestDto, | ||
| @User() user: JwtUser, | ||
| ): Promise<ScorecardWithGroupResponseDto> { | ||
| return await this.scorecardService.editScorecard(id, body, user); |
There was a problem hiding this comment.
The return type of the method editScorecard has been changed from Promise<ScorecardResponseDto> to Promise<ScorecardWithGroupResponseDto>. Ensure that the scorecardService.editScorecard method returns the correct type ScorecardWithGroupResponseDto to match this change.
| }); | ||
| }); | ||
| return data as ScorecardResponseDto; | ||
| @Body(new ValidationPipe({ whitelist: true, transform: true })) |
There was a problem hiding this comment.
A ValidationPipe has been added to the body parameter. Ensure that the ScorecardRequestDto class is properly configured to work with the whitelist and transform options of the ValidationPipe.
| status: 200, | ||
| description: 'Scorecard deleted successfully.', | ||
| type: ScorecardResponseDto, | ||
| type: ScorecardWithGroupResponseDto, |
There was a problem hiding this comment.
The type for the successful response has been changed from ScorecardResponseDto to ScorecardWithGroupResponseDto. Ensure that this change is intentional and that the new DTO accurately reflects the response structure.
| }); | ||
| }); | ||
| return { message: `Scorecard ${id} deleted successfully.` }; | ||
| return await this.scorecardService.deleteScorecard(id); |
There was a problem hiding this comment.
The implementation of the deleteScorecard method has been changed to use scorecardService.deleteScorecard(id). Verify that the service method handles exceptions appropriately, especially for cases where the scorecard is not found, and that it returns the expected response format.
|
|
||
| @Get('/:id') | ||
| @Scopes(Scope.ReadScorecard) | ||
| @ApiOperation({ |
There was a problem hiding this comment.
The @Scopes(Scope.ReadScorecard) decorator has been removed. Confirm that this change does not affect the security or authorization logic for viewing a scorecard.
| status: 200, | ||
| description: 'Scorecard retrieved successfully.', | ||
| type: ScorecardResponseDto, | ||
| type: ScorecardWithGroupResponseDto, |
There was a problem hiding this comment.
The return type of the viewScorecard method has been changed from ScorecardResponseDto to ScorecardWithGroupResponseDto. Ensure that this change is intentional and that all parts of the application using this method are compatible with the new return type.
| async viewScorecard( | ||
| @Param('id') id: string, | ||
| ): Promise<ScorecardWithGroupResponseDto> { | ||
| return await this.scorecardService.viewScorecard(id); |
There was a problem hiding this comment.
The implementation of viewScorecard has been refactored to use this.scorecardService.viewScorecard(id). Verify that the scorecardService is correctly implemented and that it handles exceptions appropriately, as the previous implementation included specific error handling logic.
|
|
||
| @Get() | ||
| @Roles(UserRole.Admin, UserRole.Copilot) | ||
| @Roles(UserRole.Admin) |
There was a problem hiding this comment.
The @Roles decorator has been modified to remove UserRole.Copilot. Confirm that this change aligns with the intended access control requirements and that UserRole.Copilot should no longer have access to this endpoint.
| }, | ||
| skip, | ||
| take: perPage, | ||
| @Query() query: SearchScorecardQuery, |
There was a problem hiding this comment.
The query parameter is destructured without default values for page and perPage. Consider providing default values to ensure pagination works correctly even if these parameters are not provided in the query.
| perPage, | ||
| } = query; | ||
|
|
||
| const result = await this.scorecardService.getScoreCards({ |
There was a problem hiding this comment.
The getScoreCards method is called on scorecardService, but there is no error handling for the promise. Consider adding error handling to manage potential exceptions that could be thrown during the execution of this asynchronous operation.
| @ApiResponse({ status: 403, description: 'Forbidden.' }) | ||
| @ApiResponse({ status: 404, description: 'Scorecard not found.' }) | ||
| async cloneScorecard( | ||
| @Param('id') id: string, |
There was a problem hiding this comment.
The cloneScorecard method does not include error handling for the service call. Consider implementing error handling to manage potential exceptions that could occur during the cloning process.
|
|
||
| constructor(private readonly prisma: PrismaService) { | ||
| this.logger = LoggerService.forRoot('ScorecardService'); | ||
| } |
There was a problem hiding this comment.
Consider using dependency injection for the LoggerService instead of calling LoggerService.forRoot. This would make it easier to mock the logger in unit tests and follow the dependency injection pattern more closely.
| where: { id }, | ||
| }) | ||
| .catch((error) => { | ||
| if (error.code !== 'P2025') { |
There was a problem hiding this comment.
The error handling logic here seems incorrect. The condition should check if the error code is 'P2025' to throw a NotFoundException, as 'P2025' indicates a record not found error in Prisma.
| }, | ||
| }) | ||
| .catch((error) => { | ||
| if (error.code !== 'P2025') { |
There was a problem hiding this comment.
The error handling logic here seems incorrect. The condition should check if the error code is 'P2025' to throw a NotFoundException, as 'P2025' indicates a record not found error in Prisma.
| const auditFields = { | ||
| createdBy: user.isMachine ? 'System' : (user.userId as string), | ||
| updatedBy: user.isMachine ? 'System' : (user.userId as string), | ||
| createdAt: undefined, |
There was a problem hiding this comment.
The createdAt and updatedAt fields are being set to undefined. Ensure that these fields are automatically managed by the database or explicitly set them to the current timestamp if needed.
| @ApiProperty({ description: 'The id of the question', example: 'abc' }) | ||
| @IsOptional() | ||
| @IsString() | ||
| id: string; |
There was a problem hiding this comment.
Consider using @IsUUID() instead of @IsString() for the id field if the id is expected to be a UUID. This will ensure the validation is more specific and accurate.
| example: 'Provide detailed information.', | ||
| }) | ||
| @IsString() | ||
| guidelines: string; |
There was a problem hiding this comment.
Consider adding @IsOptional() to the guidelines field if it is not mandatory. This will make the validation rules clearer.
| required: false, | ||
| }) | ||
| @IsOptional() | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider specifying the @IsNumber() options to ensure that scaleMin is validated as an integer if that's the intended use case. For example, use @IsNumber({}, { allowNaN: false, allowInfinity: false }) to enforce stricter validation.
| required: false, | ||
| }) | ||
| @IsOptional() | ||
| @IsNumber() |
There was a problem hiding this comment.
Consider adding validation constraints to ensure scaleMax is a positive number, if applicable.
|
|
||
| @ApiProperty({ description: 'Sort order of the question', example: 1 }) | ||
| @IsNumber() | ||
| sortOrder: number; |
There was a problem hiding this comment.
Consider adding validation constraints to ensure sortOrder is a positive integer, if applicable.
| @ApiProperty({ description: 'The id of the section', 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 follows the correct format.
| @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, if the array contains objects of a specific class, you could use questions: SpecificClassName[];.
| @IsArray() | ||
| @ValidateNested({ each: true }) | ||
| @Type(() => ScorecardQuestionRequestDto) | ||
| @WeightSum() |
There was a problem hiding this comment.
The custom decorator @WeightSum() is used here, but there is no context provided about its implementation or purpose. Ensure that this decorator is correctly implemented and tested to validate the sum of weights in questions. If it's a new addition, consider adding tests to verify its functionality.
| @ApiProperty({ description: 'The id of the group', example: 'abc' }) | ||
| @IsOptional() | ||
| @IsString() | ||
| id: string; |
There was a problem hiding this comment.
Consider using @IsUUID() instead of @IsString() for the id field if the id is expected to be a UUID. This will ensure that the id is validated as a UUID format.
| @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. For example, if sections is an array of Section objects, you can define it as sections: Section[];.
| @IsArray() | ||
| @ValidateNested({ each: true }) | ||
| @Type(() => ScorecardSectionRequestDto) | ||
| @WeightSum() |
There was a problem hiding this comment.
The @WeightSum() decorator is used here, but there is no context provided in this diff about what it does. Ensure that the @WeightSum() decorator is defined and properly imported in this file, and that it is functioning as intended for the sections property.
| @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. If this is a custom decorator, ensure it is implemented correctly. Otherwise, consider using a combination of @Max and custom validation logic.
| @ApiProperty({ description: 'The maximum score', example: 100 }) | ||
| @IsNumber() | ||
| @Max(100) | ||
| @IsGreaterThan('minScore') |
There was a problem hiding this comment.
The decorator @IsGreaterThan('minScore') is not a standard class-validator decorator. If this is a custom decorator, ensure it is implemented correctly. Otherwise, consider using a combination of @Min and custom validation logic.
| /** | ||
| * These shouldn't be editable via API | ||
| */ | ||
| createdAt: Date; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsDate() to ensure that createdAt is a valid date.
| description: 'The user who created the scorecard', | ||
| example: 'user123', | ||
| }) | ||
| createdBy: string; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsString() and @IsNotEmpty() to ensure that createdBy is a non-empty string.
| description: 'The last update timestamp', | ||
| example: '2023-10-01T00:00:00Z', | ||
| }) | ||
| updatedAt: Date; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsDate() to ensure that updatedAt is a valid date.
| description: 'The user who last updated the scorecard', | ||
| example: 'user456', | ||
| }) | ||
| updatedBy: string; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsString() and @IsNotEmpty() to ensure that updatedBy is a non-empty string.
| } | ||
|
|
||
| export class ScorecardBaseWithGroupsDto extends ScorecardBaseDto { | ||
| scorecardGroups: any[]; |
There was a problem hiding this comment.
The type any[] for scorecardGroups is too generic. Consider specifying a more precise type to improve type safety and maintainability.
| export class ScorecardRequestDto extends ScorecardBaseDto { | ||
| export class ScorecardRequestDto extends ScorecardBaseWithGroupsDto { | ||
| @ApiProperty({ description: 'The ID of the scorecard', example: 'abc123' }) | ||
| id: string; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsString() and @IsNotEmpty() to ensure that id is a non-empty string.
| description: 'The list of groups associated with the scorecard', | ||
| type: [ScorecardGroupRequestDto], | ||
| }) | ||
| @IsArray() |
There was a problem hiding this comment.
Consider adding a validation decorator to ensure that scorecardGroups is not empty if it is a required field.
| id: string; | ||
| } | ||
|
|
||
| function toArray<T = string>(value: unknown): T[] | undefined { |
There was a problem hiding this comment.
The toArray function could benefit from additional type safety checks to ensure that the transformation to an array is appropriate for the given input type.
| @IsOptional() | ||
| @Type(() => Number) | ||
| @IsInt() | ||
| @Min(1) |
There was a problem hiding this comment.
Consider adding a maximum value constraint for the page property to prevent excessively large page numbers that could lead to performance issues.
| @IsOptional() | ||
| @Type(() => Number) | ||
| @IsInt() | ||
| @Min(1) |
There was a problem hiding this comment.
Consider adding a maximum value constraint for the perPage property to prevent excessively large page sizes that could lead to performance issues.
| scoreCards: ScorecardResponseDto[]; | ||
| } | ||
|
|
||
| export function mapScorecardRequestForCreate(request: ScorecardRequestDto) { |
There was a problem hiding this comment.
The function mapScorecardRequestForCreate sets id: undefined for various entities. Ensure that this is the intended behavior and that it aligns with the database or ORM expectations. If the IDs are auto-generated, this is fine, but if not, it might lead to issues.
| export function mapScorecardRequestForCreate(request: ScorecardRequestDto) { | ||
| 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 check for createdBy is a good practice to avoid unnecessary properties. However, ensure that the absence of createdBy is acceptable in all contexts where this function is used.
| ...request, | ||
| id: undefined, | ||
| ...userFields, | ||
| scorecardGroups: { |
There was a problem hiding this comment.
The scorecardGroups property uses a nested create structure. Verify that this aligns with the ORM or database library being used, as some libraries might require a different structure or additional configuration.
| create: section.questions.map((question) => ({ | ||
| ...question, | ||
| id: undefined, | ||
| sortOrder: 1, |
There was a problem hiding this comment.
The sortOrder is hardcoded to 1 for all questions. If this is intentional, consider whether it should be dynamically set or if there is a plan to update it later in the workflow.
| const res = context.switchToHttp().getResponse<Response>(); | ||
| return next.handle().pipe( | ||
| tap((response) => { | ||
| if (response?.metadata) { |
There was a problem hiding this comment.
Consider adding a check to ensure response.metadata contains all necessary properties (total, page, perPage, totalPages) before attempting to set headers. This will prevent potential errors if any of these properties are missing.
| tap((response) => { | ||
| if (response?.metadata) { | ||
| const { total, page, perPage, totalPages } = response.metadata; | ||
| res.setHeader('X-Total-Count', total); |
There was a problem hiding this comment.
Ensure that the values being set in headers (total, page, perPage, totalPages) are properly validated or sanitized to prevent any unexpected behavior or security issues.
| const request = ctx.switchToHttp().getRequest(); | ||
| const user = request.user; | ||
|
|
||
| return (data ? user?.[data] : user) as JwtUser; |
There was a problem hiding this comment.
Consider adding validation to ensure that data corresponds to a valid property on the user object before attempting to access it. This can help prevent potential runtime errors if data is not a valid key.
| } from './kafka-consumer.service'; | ||
| import { KafkaHandlerRegistry } from './kafka-handler.registry'; | ||
| import { AVScanActionScanHandler } from './handlers/avscan-action-scan.handler'; | ||
| import registeredHandlersConfig from './handlers/registered-handlers.config'; |
There was a problem hiding this comment.
Consider verifying that registeredHandlersConfig is correctly exporting all necessary handlers that were previously imported individually, such as AVScanActionScanHandler. Ensure that the change does not affect the functionality of the module.
| inject: [KafkaHandlerRegistry], | ||
| }, | ||
| AVScanActionScanHandler, | ||
| ...registeredHandlersConfig, |
There was a problem hiding this comment.
Consider verifying that registeredHandlersConfig is correctly initialized and contains the expected handlers before spreading it into the providers array. This ensures that the module configuration remains consistent and avoids potential runtime errors.
| constraints: [relatedPropertyName, comparatorFn], | ||
| options: validationOptions, | ||
| validator: { | ||
| validate(value: Date, args: ValidationArguments) { |
There was a problem hiding this comment.
The type of value in the validate function is specified as Date, but the comparator function seems to expect a more generic type. Consider using a more generic type like any for value to match the comparator function's expectations.
| constraints: [sum], | ||
| options: validationOptions, | ||
| validator: { | ||
| validate(value: { weight: number }[], args: ValidationArguments) { |
There was a problem hiding this comment.
The validate function for WeightSum assumes that value is an array of objects with a weight property. Consider adding a check to ensure that each item in the array is indeed an object with a weight property to prevent runtime errors.
3 participants
Merge scorecards work to dev branch.