Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ lerna-debug.log*
.env.production.local
.env.local

# Prisma score cards
/prisma/Scorecards

# temp directory
.temp
.tmp
Expand Down
2 changes: 2 additions & 0 deletions src/api/api.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { ChallengeApiService } from 'src/shared/modules/global/challenge.service
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';

@Module({
imports: [HttpModule, GlobalProvidersModule],
Expand All @@ -37,6 +38,7 @@ import { GiteaWebhookAuthGuard } from '../shared/guards/gitea-webhook-auth.guard
ChallengeApiService,
WebhookService,
GiteaWebhookAuthGuard,
ScoreCardService,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScoreCardService is added to the providers array, but ensure that it is properly imported at the top of the file. If it's already imported, verify that the import path is correct and matches the file structure.

],
})
export class ApiModule {}
79 changes: 42 additions & 37 deletions src/api/scorecard/scorecard.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
Query,
NotFoundException,
InternalServerErrorException,
Res,
UseInterceptors,
} from '@nestjs/common';
import {
ApiTags,
Expand All @@ -24,18 +26,24 @@ import { UserRole } from 'src/shared/enums/userRole.enum';
import { Scopes } from 'src/shared/decorators/scopes.decorator';
import { Scope } from 'src/shared/enums/scopes.enum';
import {
ScorecardPaginatedResponseDto,
ScorecardRequestDto,
ScorecardResponseDto,
ScorecardWithGroupResponseDto,
mapScorecardRequestToDto,
} from 'src/dto/scorecard.dto';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import mapScorecardRequestToDto has been removed, but there is no indication in this diff whether it was used elsewhere in the code. Ensure that this removal does not affect any functionality or cause any errors due to missing imports.

import { ChallengeTrack } from 'src/shared/enums/challengeTrack.enum';
import { PrismaService } from '../../shared/modules/global/prisma.service';
import { ScoreCardService } from './scorecard.service';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import statement for ScoreCardService should be consistent with the naming convention used in the rest of the file. Consider using ScorecardService to match the existing naming pattern.

import { OkResponse } from 'src/dto/common.dto';
import { Response } from 'express';
import { PaginationHeaderInterceptor } from 'src/interceptors/PaginationHeaderInterceptor';

@ApiTags('Scorecard')
@ApiBearerAuth()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OkResponse import has been removed, but it is not clear if it is no longer needed or if it should be replaced with another response type. Please ensure that the removal of this import does not affect the functionality of the code.

@Controller('/scorecards')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Response import from 'express' has been removed. If this was used in the code, make sure to replace it with an appropriate alternative or confirm that it is no longer necessary.

export class ScorecardController {
constructor(private readonly prisma: PrismaService) {}
constructor(private readonly prisma: PrismaService, private readonly scorecardService: ScoreCardService) {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor parameter scorecardService is added but not used in this snippet. Ensure that it is utilized in the class methods or remove it if unnecessary to avoid unused code.


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PrismaService dependency has been removed from the constructor. Ensure that this change does not affect any functionality that relies on PrismaService. If PrismaService is no longer needed, consider removing any related unused imports or code.

@Post()
@Roles(UserRole.Admin)
Expand All @@ -48,12 +56,12 @@ export class ScorecardController {
@ApiResponse({
status: 201,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are really returning 201 here? Probably change to 200...

Copy link
Collaborator Author

@hentrymartin hentrymartin Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkartunov This is for adding scorecard, normally for adding we return 201 so this looks like its returning correct, for search api it returns 200.

description: 'Scorecard added successfully.',
type: ScorecardResponseDto,
type: ScorecardWithGroupResponseDto,
})
@ApiResponse({ status: 403, description: 'Forbidden.' })
async addScorecard(
@Body() body: ScorecardRequestDto,
): Promise<ScorecardResponseDto> {
): Promise<ScorecardWithGroupResponseDto> {
const data = await this.prisma.scorecard.create({
data: mapScorecardRequestToDto(body),
include: {
Expand All @@ -68,7 +76,7 @@ export class ScorecardController {
},
},
});
return data as ScorecardResponseDto;
return data as ScorecardWithGroupResponseDto;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type has been changed from ScorecardResponseDto to ScorecardWithGroupResponseDto. Ensure that the new type ScorecardWithGroupResponseDto is correctly defined and that all parts of the application using this method are compatible with this change.

}

@Put('/:id')
Expand All @@ -87,14 +95,14 @@ export class ScorecardController {
@ApiResponse({
status: 200,
description: 'Scorecard updated successfully.',
type: ScorecardResponseDto,
type: ScorecardWithGroupResponseDto,
})
@ApiResponse({ status: 403, description: 'Forbidden.' })
@ApiResponse({ status: 404, description: 'Scorecard not found.' })
async editScorecard(
@Param('id') id: string,
@Body() body: ScorecardRequestDto,
): Promise<ScorecardResponseDto> {
@Body() body: ScorecardWithGroupResponseDto,
): Promise<ScorecardWithGroupResponseDto> {
console.log(JSON.stringify(body));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the console.log statement to avoid unnecessary logging in production code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want this. Please always use logged.debug for that purpose.
Later we can enable or disable debug with env variables. You will enable it for your local env...


const data = await this.prisma.scorecard
Expand All @@ -121,7 +129,7 @@ export class ScorecardController {
message: `Error: ${error.code}`,
});
});
return data as ScorecardResponseDto;
return data as ScorecardWithGroupResponseDto;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type has been changed from ScorecardResponseDto to ScorecardWithGroupResponseDto. Ensure that this change is consistent with the rest of the codebase and that ScorecardWithGroupResponseDto is the correct type to return here. Verify that all usages of this method are updated accordingly.

}

@Delete(':id')
Expand All @@ -139,7 +147,7 @@ export class ScorecardController {
@ApiResponse({
status: 200,
description: 'Scorecard deleted successfully.',
type: ScorecardResponseDto,
type: ScorecardWithGroupResponseDto,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type for the API response has been changed from ScorecardResponseDto to ScorecardWithGroupResponseDto. Ensure that the new type ScorecardWithGroupResponseDto is correctly defined and that all necessary fields are included for this response type. Verify that this change aligns with the intended modifications for the scorecard search functionality.

})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The console.log statement has been removed. Ensure that any necessary logging is handled appropriately elsewhere if needed.

@ApiResponse({ status: 403, description: 'Forbidden.' })
@ApiResponse({ status: 404, description: 'Scorecard not found.' })
Expand Down Expand Up @@ -173,10 +181,10 @@ export class ScorecardController {
@ApiResponse({
status: 200,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling logic for P2025 has been removed. Ensure that the scorecardService.editScorecard method includes appropriate error handling for cases where the scorecard is not found or other errors occur.

description: 'Scorecard retrieved successfully.',
type: ScorecardResponseDto,
type: ScorecardWithGroupResponseDto,
})
@ApiResponse({ status: 404, description: 'Scorecard not found.' })
async viewScorecard(@Param('id') id: string): Promise<ScorecardResponseDto> {
async viewScorecard(@Param('id') id: string): Promise<ScorecardWithGroupResponseDto> {
const data = await this.prisma.scorecard
.findUniqueOrThrow({
where: { id },
Expand All @@ -200,11 +208,11 @@ export class ScorecardController {
message: `Error: ${error.code}`,
});
});
return data as ScorecardResponseDto;
return data as ScorecardWithGroupResponseDto;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion

Ensure that ScorecardWithGroupResponseDto is correctly defined and imported in this file. If this type includes additional properties compared to ScorecardResponseDto, verify that these properties are being handled appropriately in the rest of the code.

}

@Get()
@Roles(UserRole.Admin, UserRole.Copilot)
@Roles(UserRole.Admin)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion

Consider the implications of removing the UserRole.Copilot role from the @Roles decorator. Ensure that this change aligns with the intended access control requirements for the search scorecards functionality.

@Scopes(Scope.ReadScorecard)
@ApiOperation({
summary: 'Search scorecards',
Expand Down Expand Up @@ -249,34 +257,31 @@ export class ScorecardController {
description: 'List of matching scorecards',
type: [ScorecardResponseDto],
})
@UseInterceptors(PaginationHeaderInterceptor)
async searchScorecards(
@Query('challengeTrack') challengeTrack?: ChallengeTrack,
@Query('challengeType') challengeType?: string,
@Query('challengeTrack') challengeTrack?: ChallengeTrack | ChallengeTrack[],
@Query('challengeType') challengeType?: string | string[],
@Query('name') name?: string,
@Query('page') page: number = 1,
@Query('perPage') perPage: number = 10,
) {
const skip = (page - 1) * perPage;
const data = await this.prisma.scorecard.findMany({
where: {
...(challengeTrack && { challengeTrack }),
...(challengeType && { challengeType }),
...(name && { name: { contains: name, mode: 'insensitive' } }),
},
include: {
scorecardGroups: {
include: {
sections: {
include: {
questions: true,
},
},
},
},
},
skip,
take: perPage,
): Promise<ScorecardPaginatedResponseDto> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of the searchScorecards method is specified as Promise<ScorecardPaginatedResponseDto>, but it is not clear from the code if this.scorecardService.getScoreCards returns this type. Ensure that the return type of getScoreCards matches ScorecardPaginatedResponseDto.

const challengeTrackArray = Array.isArray(challengeTrack)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for converting challengeTrack to an array could be simplified using a ternary operator or utility function to improve readability.

? challengeTrack
: challengeTrack
? [challengeTrack]
: [];
const challengeTypeArray = Array.isArray(challengeType)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for converting challengeType to an array could be simplified using a ternary operator or utility function to improve readability.

? challengeType
: challengeType
? [challengeType]
: [];
const result = await this.scorecardService.getScoreCards({
challengeTrack: challengeTrackArray,
challengeType: challengeTypeArray,
name,
page,
perPage,
});
return data as ScorecardResponseDto[];
return result;
}
}
58 changes: 58 additions & 0 deletions src/api/scorecard/scorecard.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { Injectable } from "@nestjs/common";
import { Prisma } from "@prisma/client";
import { ScorecardPaginatedResponseDto, ScorecardQueryDto, ScorecardResponseDto } from "src/dto/scorecard.dto";
import { PrismaService } from "src/shared/modules/global/prisma.service";

@Injectable()
export class ScoreCardService {
constructor(
private readonly prisma: PrismaService,
) {}

/**
* Get list of score cards and send it in paginated way
* @param query query params
* @returns response dto
*/
async getScoreCards(
query: ScorecardQueryDto
): Promise<ScorecardPaginatedResponseDto> {
const { page = 1, perPage = 10, challengeTrack, challengeType, name } = query;
const skip = (page - 1) * perPage;
const where: Prisma.scorecardWhereInput = {
...(challengeTrack?.length && {
challengeTrack: {
in: challengeTrack,
},
}),
...(challengeType?.length && {
challengeType: {
in: challengeType,
},
}),
...(name && { name: { contains: name, mode: 'insensitive' } }),
};
const data = await this.prisma.scorecard.findMany({
where,
skip,
take: perPage,
orderBy: {
name: 'asc',
},
});

const totalCount = await this.prisma.scorecard.count({
where,
});

return {
metadata: {
total: totalCount,
page,
perPage,
totalPages: Math.ceil(totalCount/perPage),
},
scoreCards: data as ScorecardResponseDto[],
};
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that there is a newline at the end of the file to adhere to POSIX standards and improve compatibility with various tools.

42 changes: 41 additions & 1 deletion src/dto/scorecard.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,13 @@ export class ScorecardBaseDto {
example: 'user456',
})
updatedBy: string;
}

export class ScorecardBaseWithGroupsDto extends ScorecardBaseDto {
scorecardGroups: any[];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider specifying a more specific type for scorecardGroups instead of using any[]. This will improve type safety and make the code easier to understand and maintain.

}

export class ScorecardRequestDto extends ScorecardBaseDto {
export class ScorecardRequestDto extends ScorecardBaseWithGroupsDto {
@ApiProperty({ description: 'The ID of the scorecard', example: 'abc123' })
id: string;

Expand All @@ -221,6 +223,11 @@ export class ScorecardRequestDto extends ScorecardBaseDto {
export class ScorecardResponseDto extends ScorecardBaseDto {
@ApiProperty({ description: 'The ID of the scorecard', example: 'abc123' })
id: string;
}

export class ScorecardWithGroupResponseDto extends ScorecardBaseDto {
@ApiProperty({ description: 'The ID of the scorecard', example: 'abc123' })
id: string;

@ApiProperty({
description: 'The list of groups associated with the scorecard',
Expand All @@ -229,6 +236,39 @@ export class ScorecardResponseDto extends ScorecardBaseDto {
scorecardGroups: ScorecardGroupResponseDto[];
}

export class PaginationMetaDto {
@ApiProperty({ example: 100 })
total: number;

@ApiProperty({ example: 1 })
page: number;

@ApiProperty({ example: 10 })
perPage: number;

@ApiProperty({ example: 10 })
totalPages: number;
}

export class ScorecardPaginatedResponseDto {
@ApiProperty({ description: 'This contains pagination metadata' })
metadata: PaginationMetaDto;

@ApiProperty({
description: 'The list of score cards',
type: [ScorecardGroupResponseDto],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider specifying the type of elements in the scoreCards array as ScorecardResponseDto[] instead of ScorecardGroupResponseDto[] in the @ApiProperty decorator for clarity and consistency with the property type.

})
scoreCards: ScorecardResponseDto[];
}

export class ScorecardQueryDto {
challengeTrack?: ChallengeTrack[];
challengeType?: string[];
name?: string;
page?: number;
perPage?: number;
}

export function mapScorecardRequestToDto(request: ScorecardRequestDto) {
const userFields = {
createdBy: request.createdBy,
Expand Down
21 changes: 21 additions & 0 deletions src/interceptors/PaginationHeaderInterceptor.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @hentrymartin!

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from "@nestjs/common";
import { Observable, tap } from "rxjs";
import { Response } from "express";

@Injectable()
export class PaginationHeaderInterceptor implements NestInterceptor {
intercept(context: ExecutionContext, next: CallHandler): Observable<any> {
const res = context.switchToHttp().getResponse<Response>();
return next.handle().pipe(
tap((response) => {
if (response?.metadata) {
const { total, page, perPage, totalPages } = response.metadata;
res.setHeader('X-Total-Count', total);
res.setHeader('X-Page', page);
res.setHeader('X-Per-Page', perPage);
res.setHeader('X-Total-Pages', totalPages);
}
}),
);
}
}