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

hentrymartin · 2025-08-07T06:52:58Z

What's in this PR?

Search scorecard modifications

Ticket link - https://topcoder.atlassian.net/browse/PM-1585

github-actions · 2025-08-07T06:53:10Z

src/api/api.module.ts

@@ -37,6 +38,7 @@ import { GiteaWebhookAuthGuard } from '../shared/guards/gitea-webhook-auth.guard
    ChallengeApiService,
    WebhookService,
    GiteaWebhookAuthGuard,
+    ScoreCardService,


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.

github-actions · 2025-08-07T06:53:13Z

src/api/scorecard/scorecard.controller.ts

  mapScorecardRequestToDto,
 } 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';


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.

github-actions · 2025-08-07T06:53:14Z

src/api/scorecard/scorecard.controller.ts


 @ApiTags('Scorecard')
 @ApiBearerAuth()
 @Controller('/scorecards')
 export class ScorecardController {
-  constructor(private readonly prisma: PrismaService) {}
+  constructor(private readonly prisma: PrismaService, private readonly scorecardService: ScoreCardService) {}


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.

github-actions · 2025-08-07T06:53:17Z

src/api/scorecard/scorecard.controller.ts

@@ -68,7 +76,7 @@ export class ScorecardController {
        },
      },
    });
-    return data as ScorecardResponseDto;
+    return data as ScorecardWithGroupResponseDto;


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.

github-actions · 2025-08-07T06:53:19Z

src/api/scorecard/scorecard.controller.ts

-    @Body() body: ScorecardRequestDto,
-  ): Promise<ScorecardResponseDto> {
+    @Body() body: ScorecardWithGroupResponseDto,
+  ): Promise<ScorecardWithGroupResponseDto> {
    console.log(JSON.stringify(body));


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

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...

github-actions · 2025-08-07T06:53:22Z

src/api/scorecard/scorecard.controller.ts

@@ -121,7 +129,7 @@ export class ScorecardController {
          message: `Error: ${error.code}`,
        });
      });
-    return data as ScorecardResponseDto;
+    return data as ScorecardWithGroupResponseDto;


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.

github-actions · 2025-08-07T06:53:24Z

src/api/scorecard/scorecard.controller.ts

@@ -139,7 +147,7 @@ export class ScorecardController {
  @ApiResponse({
    status: 200,
    description: 'Scorecard deleted successfully.',
-    type: ScorecardResponseDto,
+    type: ScorecardWithGroupResponseDto,


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.

github-actions · 2025-08-07T06:53:29Z

src/api/scorecard/scorecard.controller.ts

@@ -200,11 +208,11 @@ export class ScorecardController {
          message: `Error: ${error.code}`,
        });
      });
-    return data as ScorecardResponseDto;
+    return data as ScorecardWithGroupResponseDto;


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.

github-actions · 2025-08-07T06:53:29Z

src/api/scorecard/scorecard.controller.ts

  }

  @Get()
-  @Roles(UserRole.Admin, UserRole.Copilot)
+  @Roles(UserRole.Admin)


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.

github-actions · 2025-08-07T06:53:32Z

src/api/scorecard/scorecard.controller.ts

-      },
-      skip,
-      take: perPage,
+  ): Promise<ScorecardPaginatedResponseDto> {


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.

github-actions · 2025-08-07T06:53:32Z

src/api/scorecard/scorecard.controller.ts

-      skip,
-      take: perPage,
+  ): Promise<ScorecardPaginatedResponseDto> {
+    const challengeTrackArray = Array.isArray(challengeTrack)


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

github-actions · 2025-08-07T06:53:32Z

src/api/scorecard/scorecard.controller.ts

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


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

github-actions · 2025-08-07T06:53:35Z

src/api/scorecard/scorecard.service.ts

+      scoreCards: data as ScorecardResponseDto[],
+    };
+  }
+}


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

github-actions · 2025-08-07T06:53:37Z

src/dto/scorecard.dto.ts


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


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.

github-actions · 2025-08-07T06:53:40Z

src/dto/scorecard.dto.ts

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


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.

kkartunov · 2025-08-07T07:34:47Z

src/interceptors/PaginationHeaderInterceptor.ts

Nice one @hentrymartin!

kkartunov · 2025-08-07T07:41:00Z

src/api/scorecard/scorecard.controller.ts

@@ -48,12 +56,12 @@ export class ScorecardController {
  @ApiResponse({
    status: 201,


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

@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.

src/api/scorecard/scorecard.controller.ts

kkartunov · 2025-08-07T07:44:02Z

src/api/scorecard/scorecard.controller.ts

-    @Body() body: ScorecardRequestDto,
-  ): Promise<ScorecardResponseDto> {
+    @Body() body: ScorecardWithGroupResponseDto,
+  ): Promise<ScorecardWithGroupResponseDto> {
    console.log(JSON.stringify(body));


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...

src/api/scorecard/scorecard.controller.ts

github-actions · 2025-08-07T19:48:04Z

src/api/scorecard/scorecard.controller.ts

  mapScorecardRequestToDto,
 } 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';
+import { PaginationHeaderInterceptor } from 'src/interceptors/PaginationHeaderInterceptor';

 @ApiTags('Scorecard')
 @ApiBearerAuth()


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.

github-actions · 2025-08-07T19:48:04Z

src/api/scorecard/scorecard.controller.ts

  mapScorecardRequestToDto,
 } 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';
+import { PaginationHeaderInterceptor } from 'src/interceptors/PaginationHeaderInterceptor';

 @ApiTags('Scorecard')
 @ApiBearerAuth()
 @Controller('/scorecards')


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.

github-actions · 2025-08-07T19:48:07Z

src/api/scorecard/scorecard.controller.ts

-    });
-    return data as ScorecardResponseDto;
+  ): Promise<ScorecardWithGroupResponseDto> {
+    return await this.scorecardService.addScorecard(body);


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);.

github-actions · 2025-08-07T19:48:12Z

src/api/scorecard/scorecard.controller.ts

@@ -139,24 +102,12 @@ export class ScorecardController {
  @ApiResponse({
    status: 200,
    description: 'Scorecard deleted successfully.',
-    type: ScorecardResponseDto,
+    type: ScorecardWithGroupResponseDto,
  })


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

github-actions · 2025-08-07T19:48:12Z

src/api/scorecard/scorecard.controller.ts

@@ -173,38 +124,15 @@ export class ScorecardController {
  @ApiResponse({
    status: 200,


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.

github-actions · 2025-08-07T19:48:16Z

src/api/scorecard/scorecard.controller.ts

-        });
-      });
-    return { message: `Scorecard ${id} deleted successfully.` };
+    return await this.scorecardService.deleteScorecard(id);


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.

github-actions · 2025-08-07T19:48:19Z

src/api/scorecard/scorecard.controller.ts

-      });
-    return data as ScorecardResponseDto;
+  async viewScorecard(@Param('id') id: string): Promise<ScorecardWithGroupResponseDto> {
+    return await this.scorecardService.viewScorecard(id);


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.

github-actions · 2025-08-07T19:48:22Z

src/api/scorecard/scorecard.service.ts

@@ -0,0 +1,169 @@
+import { Injectable, InternalServerErrorException, NotFoundException } from "@nestjs/common";


Consider importing only the exceptions that are actually used in the code to avoid unnecessary imports.

github-actions · 2025-08-07T19:48:23Z

src/api/scorecard/scorecard.service.ts

@@ -0,0 +1,169 @@
+import { Injectable, InternalServerErrorException, NotFoundException } from "@nestjs/common";
+import { Prisma } from "@prisma/client";
+import { mapScorecardRequestToDto, ScorecardPaginatedResponseDto, ScorecardQueryDto, ScorecardRequestDto, ScorecardResponseDto, ScorecardWithGroupResponseDto } from "src/dto/scorecard.dto";


Ensure that mapScorecardRequestToDto and ScorecardWithGroupResponseDto are used in the code. If they are not used, consider removing them to keep the imports clean and maintainable.

github-actions · 2025-08-07T19:48:27Z

src/api/scorecard/scorecard.service.ts

+        },
+      })
+      .catch((error) => {
+        if (error.code !== 'P2025') {


The error handling logic seems to be reversed. The condition error.code !== 'P2025' should throw an InternalServerErrorException, while error.code === 'P2025' should throw a NotFoundException. Please verify the logic.

github-actions · 2025-08-07T19:48:27Z

src/api/scorecard/scorecard.service.ts

+        where: { id },
+      })
+      .catch((error) => {
+        if (error.code !== 'P2025') {


The error handling logic seems to be reversed. The condition error.code !== 'P2025' should throw an InternalServerErrorException, while error.code === 'P2025' should throw a NotFoundException. Please verify the logic.

github-actions · 2025-08-07T19:48:27Z

src/api/scorecard/scorecard.service.ts

+        },
+      })
+      .catch((error) => {
+        if (error.code !== 'P2025') {


The error handling logic seems to be reversed. The condition error.code !== 'P2025' should throw an InternalServerErrorException, while error.code === 'P2025' should throw a NotFoundException. Please verify the logic.

github-actions · 2025-08-07T19:48:49Z

src/api/scorecard/scorecard.controller.ts

  ScorecardRequestDto,
  ScorecardResponseDto,
-  mapScorecardRequestToDto,
+  ScorecardWithGroupResponseDto,
 } from 'src/dto/scorecard.dto';


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.

github-actions · 2025-08-07T19:48:53Z

src/api/scorecard/scorecard.controller.ts


 @ApiTags('Scorecard')
 @ApiBearerAuth()
 @Controller('/scorecards')
 export class ScorecardController {
-  constructor(private readonly prisma: PrismaService) {}
+  constructor(private readonly scorecardService: ScoreCardService) {}



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.

hentrymartin · 2025-08-07T19:49:40Z

@kkartunov I've updated the PR based on the review comment, can you please do another round of review?

kkartunov

Looks good.

hentrymartin added 2 commits August 7, 2025 00:50

feat: modified search score cards endpoint

d598e22

feat: modified search score cards endpoint

7db8787

github-actions bot reviewed Aug 7, 2025

View reviewed changes

hentrymartin requested a review from kkartunov August 7, 2025 06:53

github-actions bot reviewed Aug 7, 2025

View reviewed changes

kkartunov reviewed Aug 7, 2025

View reviewed changes

src/interceptors/PaginationHeaderInterceptor.ts Outdated

Copy link

Contributor

kkartunov Aug 7, 2025

Choose a reason for hiding this comment

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

Nice one @hentrymartin!

hentrymartin reacted with heart emoji

kkartunov requested changes Aug 7, 2025

View reviewed changes

chore: refactored code based on review comments

278a90c

github-actions bot reviewed Aug 7, 2025

View reviewed changes

hentrymartin added 2 commits August 7, 2025 21:48

chore: refactored code based on review comments

dd01d79

chore: refactored code based on review comments

ccefae6

github-actions bot reviewed Aug 7, 2025

View reviewed changes

kkartunov approved these changes Aug 8, 2025

View reviewed changes

hentrymartin merged commit 4bc862e into feat/scorecards Aug 8, 2025
1 check passed

hentrymartin deleted the pm-1585 branch August 8, 2025 16:24


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

		@@ -48,12 +56,12 @@ export class ScorecardController {
		@ApiResponse({
		status: 201,

		@@ -173,38 +124,15 @@ export class ScorecardController {
		@ApiResponse({
		status: 200,

		@@ -0,0 +1,169 @@
		import { Injectable, InternalServerErrorException, NotFoundException } from "@nestjs/common";

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

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

Uh oh!

Conversation

hentrymartin commented Aug 7, 2025

What's in this PR?

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

kkartunov Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Suggestion

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Suggestion

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

kkartunov Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

kkartunov Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

hentrymartin Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kkartunov Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 7, 2025

hentrymartin Aug 7, 2025 •

edited

Loading