Skip to content

fic(PM-1585): Search scorecard modifications #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 8, 2025
Merged

fic(PM-1585): Search scorecard modifications #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d598e22
feat: modified search score cards endpoint
hentrymartin Aug 6, 2025
7db8787
feat: modified search score cards endpoint
hentrymartin Aug 6, 2025
278a90c
chore: refactored code based on review comments
hentrymartin Aug 7, 2025
dd01d79
chore: refactored code based on review comments
hentrymartin Aug 7, 2025
ccefae6
chore: refactored code based on review comments
hentrymartin Aug 7, 2025
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
View file
Open in desktop
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
View file
Open in desktop
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 {}
157 changes: 40 additions & 117 deletions src/api/scorecard/scorecard.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import {
Body,
Param,
Query,
NotFoundException,
InternalServerErrorException,
UseInterceptors,
} from '@nestjs/common';
import {
ApiTags,
Expand All @@ -24,18 +23,20 @@ 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,
mapScorecardRequestToDto,
ScorecardWithGroupResponseDto,
} 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 { 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 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 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,27 +49,13 @@ 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 •
edited
Loading

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> {
const data = await this.prisma.scorecard.create({
data: mapScorecardRequestToDto(body),
include: {
scorecardGroups: {
include: {
sections: {
include: {
questions: true,
},
},
},
},
},
});
return data as ScorecardResponseDto;
): Promise<ScorecardWithGroupResponseDto> {
return await this.scorecardService.addScorecard(body);
Copy link

Choose a reason for hiding this comment

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

The await keyword is redundant here since the function already returns a Promise. You can remove await to simplify the code: return this.scorecardService.addScorecard(body);.

}

@Put('/:id')
Expand All @@ -87,41 +74,15 @@ 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> {
console.log(JSON.stringify(body));

const data = await this.prisma.scorecard
.update({
where: { id },
data: mapScorecardRequestToDto(body),
include: {
scorecardGroups: {
include: {
sections: {
include: {
questions: true,
},
},
},
},
},
})
.catch((error) => {
if (error.code !== 'P2025') {
throw new NotFoundException({ message: `Scorecard not found.` });
}
throw new InternalServerErrorException({
message: `Error: ${error.code}`,
});
});
return data as ScorecardResponseDto;
@Body() body: ScorecardWithGroupResponseDto,
): Promise<ScorecardWithGroupResponseDto> {
return await this.scorecardService.editScorecard(id, body);
}

@Delete(':id')
Expand All @@ -139,24 +100,12 @@ 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.' })
async deleteScorecard(@Param('id') id: string) {
await this.prisma.scorecard
.delete({
where: { id },
})
.catch((error) => {
if (error.code !== 'P2025') {
throw new NotFoundException({ message: `Scorecard not found.` });
}
throw new InternalServerErrorException({
message: `Error: ${error.code}`,
});
});
return { message: `Scorecard ${id} deleted successfully.` };
return await this.scorecardService.deleteScorecard(id);
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 has been removed from this method. Ensure that scorecardService.deleteScorecard(id) properly handles exceptions, especially for cases like Scorecard not found and other potential errors, to maintain the robustness of the application.

}

@Get('/:id')
Expand All @@ -173,38 +122,15 @@ 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> {
const data = await this.prisma.scorecard
.findUniqueOrThrow({
where: { id },
include: {
scorecardGroups: {
include: {
sections: {
include: {
questions: true,
},
},
},
},
},
})
.catch((error) => {
if (error.code !== 'P2025') {
throw new NotFoundException({ message: `Scorecard not found.` });
}
throw new InternalServerErrorException({
message: `Error: ${error.code}`,
});
});
return data as ScorecardResponseDto;
async viewScorecard(@Param('id') id: string): Promise<ScorecardWithGroupResponseDto> {
return await this.scorecardService.viewScorecard(id);
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 findUniqueOrThrow has been removed. Ensure that this.scorecardService.viewScorecard(id) includes appropriate error handling, particularly for cases where the scorecard is not found or other database errors occur.

}

@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 +175,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;
}
}
Loading