feat: getVotePer #3
Conversation
|
Caution Review failedThe pull request is closed. Walkthrough변경 사항은 애플리케이션 내의 경쟁 관련 구성 요소에 대한 중요한 리팩토링을 포함합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CompetitionController
participant CompetitionService
participant PrismaService
Client->>CompetitionController: GET /per/:id
CompetitionController->>CompetitionService: getVotePer(id)
CompetitionService->>PrismaService: findCountVoterPer(id)
PrismaService-->>CompetitionService: return voter counts
CompetitionService-->>CompetitionController: return vote percentage data
CompetitionController-->>Client: return response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
src/competition/competition.service.interface.ts (1)
Line range hint
16-35: Consider adding JSDoc comments for better documentation.While the interface is well-designed, adding JSDoc comments would improve maintainability by documenting:
- Purpose of each method
- Parameter descriptions
- Expected return values
- Any potential error cases
Example for the new method:
/** * Retrieves the voting participation rate for a specific competition. * @param id - The unique identifier of the competition * @returns Promise resolving to voting percentage data * @throws If competition is not found or access is denied */ getVotePer(id: string): Promise<GetVotePerResponseDto>;src/competition/competition.controller.ts (1)
107-107: Consider using a more descriptive endpoint path.The current path "per/:id" is ambiguous. Consider using a more descriptive path that clearly indicates its purpose, such as:
- "vote-percentage/:id"
- "voting-stats/:id"
- "participation-rate/:id"
src/prisma/prisma.service.ts (2)
245-249: Add JSDoc documentation for the new method.Consider adding JSDoc documentation to describe the purpose, parameters, and return type of this method. This will improve code maintainability and help other developers understand the method's functionality.
+/** + * Counts voters based on their role for a specific contest + * @param id - The contest ID + * @param role - The role to filter by ("Student" or "Teacher") + * @param all - If true, counts all users regardless of role + * @returns Promise containing the count of voters + */ async findCountVoterPer( id: string, role: "Student" | "Teacher", all: boolean, )
263-263: Consider adding type information to the return value.The return type of the count query should be properly typed to ensure type safety throughout the application.
- return cnt; + return (cnt as [{count: string}])[0].count;src/competition/test/competition.controller.spec.ts (1)
90-95: Add error case mocks for comprehensive testing.The mock implementation only covers the happy path. Consider adding error case mocks to test scenarios like:
- Competition not found
- Invalid competition status
- Edge cases (zero votes)
This aligns with the testing patterns seen in other test suites like
GetCompetition.src/competition/test/competition.service.spec.ts (1)
154-175: Remove commented reference implementation.The commented code appears to be the correct reference implementation. Either:
- Use it to replace the current implementation (as suggested in the previous comment), or
- Remove it entirely as it's redundant
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/competition/competition.controller.interface.ts(2 hunks)src/competition/competition.controller.ts(2 hunks)src/competition/competition.service.interface.ts(2 hunks)src/competition/competition.service.ts(2 hunks)src/competition/dto/response/getVotePer.response.dto.ts(1 hunks)src/competition/test/competition.controller.spec.ts(2 hunks)src/competition/test/competition.service.spec.ts(3 hunks)src/prisma/prisma.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/competition/dto/response/getVotePer.response.dto.ts
🔇 Additional comments (9)
src/competition/competition.service.interface.ts (2)
10-10: LGTM! Import changes align with the new functionality.
The import statement correctly reflects the transition from prefecture-based voting to percentage-based voting data.
29-29: LGTM! Method signature follows TypeScript best practices.
The getVotePer method has a clear, concise signature and maintains consistency with the interface's Promise-based async pattern.
src/competition/competition.controller.interface.ts (2)
11-11: LGTM: Import statement follows conventions
The import statement for the new DTO follows the project's structure and naming conventions.
30-32: LGTM: Method signature is well-defined
The new getVotePer method follows the interface's conventions and properly replaces the old getVotingPrefecture functionality.
Let's verify the related implementations:
✅ Verification successful
Let me gather more information to check for the service implementation with a different pattern.
Implementation verified and properly structured
The verification shows that:
- The DTO is properly defined with
studentandteachernumber fields - The service implementation exists in
CompetitionServicewith correct signature and logic - The controller implementation properly wraps the service call
- Unit tests are in place for both service and controller layers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of corresponding service implementation and DTO
# Check if the DTO is properly defined
echo "Checking GetVotePerResponseDto definition..."
ast-grep --pattern 'export class GetVotePerResponseDto {
$$$
}'
# Check if the service implementation exists
echo "Checking service implementation..."
ast-grep --pattern 'getVotePer(id: string): Promise<$_> {
$$$
}'
Length of output: 826
Script:
#!/bin/bash
# Search for getVotePer implementation with a more flexible pattern
echo "Checking service implementation with broader pattern..."
rg "getVotePer" -A 5
# Also check for the service class implementation
echo "Checking competition service class..."
ast-grep --pattern 'export class CompetitionService {
$$$
}'
Length of output: 6405
src/competition/competition.controller.ts (2)
30-30: LGTM! Import statement is correctly added.
The import follows the established pattern and maintains consistency with other DTO imports.
107-116: Verify complete removal of getVotingPrefecture.
Let's ensure there are no remaining references to the old method or its DTO.
✅ Verification successful
Method getVotingPrefecture has been completely removed
The comprehensive search across the codebase shows no remaining references to:
- The method
getVotingPrefecture - The DTO
GetVotingPrefectureResponseDto - Any similar prefecture-related voting functionality
The removal appears to be clean with no lingering references that could cause issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old method or DTO
echo "Checking for remaining references to getVotingPrefecture..."
rg "getVotingPrefecture"
echo "Checking for remaining references to GetVotingPrefectureResponseDto..."
rg "GetVotingPrefectureResponseDto"
Length of output: 340
Script:
#!/bin/bash
# Let's try a more comprehensive search with case-insensitive and partial matches
echo "Checking for any references containing 'voting' and 'prefecture'..."
rg -i "vot.*prefect"
# Check for any DTOs related to prefecture voting
echo "Checking for prefecture-related DTOs..."
fd "dto" --exec rg -l "prefecture"
# Check for any service methods related to prefecture
echo "Checking service implementations..."
fd "service" --exec rg -l "prefecture"
Length of output: 419
src/competition/competition.service.ts (1)
25-25: LGTM!
The import for GetVotePerResponseDto is correctly placed and necessary for the new functionality.
src/prisma/prisma.service.ts (1)
245-268: Verify the impact on existing functionality.
This new method is part of a larger refactoring effort replacing getVotingPrefecture with getVotePer. Let's verify that all related components are properly updated.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
No references to getVotingPrefecture found, and findCountVoterPer is properly integrated
The verification shows:
- No remaining references to the old method
getVotingPrefectureexist in the codebase - The new method
findCountVoterPeris properly integrated in:src/prisma/prisma.service.ts(implementation)src/competition/competition.service.ts(usage)src/competition/test/competition.service.spec.ts(test coverage)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old method and verify the new method's usage
# Test 1: Check for any remaining references to old method
echo "Checking for old method references..."
rg "getVotingPrefecture" -l
# Test 2: Verify the new method's usage
echo "Checking new method usage..."
rg "findCountVoterPer" -A 5
# Test 3: Check for any related test files
echo "Checking for test coverage..."
fd "competition.*\.spec\.ts$" -x cat {}
Length of output: 43401
src/competition/test/competition.service.spec.ts (1)
551-553: LGTM!
The formatting change improves readability of the expect statement.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
src/competition/competition.controller.ts(3 hunks)src/competition/competition.service.ts(2 hunks)src/competition/test/competition.controller.spec.ts(4 hunks)src/competition/test/competition.service.spec.ts(3 hunks)src/guard/adminValidator/adminValidator.guard.spec.ts(0 hunks)src/prisma/prisma.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/guard/adminValidator/adminValidator.guard.spec.ts
🔇 Additional comments (6)
src/competition/competition.controller.ts (1)
9-9: 새로운 import 구문이 적절히 추가되었습니다!
새로운 기능에 필요한 NotFoundException과 GetVotePerResponseDto가 올바르게 import 되었습니다.
Also applies to: 31-31
src/competition/competition.service.ts (1)
25-25: LGTM! DTO 임포트가 올바르게 추가되었습니다.
다른 응답 DTO들과 함께 일관된 방식으로 임포트되어 있습니다.
src/competition/test/competition.controller.spec.ts (2)
6-6: NotFoundException import가 적절히 추가되었습니다.
새로운 테스트 케이스에서 사용되는 NotFoundException이 올바르게 import 되었습니다.
119-119: beforeEach에서 모든 목을 초기화하도록 개선되었습니다.
개별 mock.mockClear() 호출 대신 jest.clearAllMocks()를 사용하여 코드가 더 간결해졌습니다.
src/competition/test/competition.service.spec.ts (2)
131-151: 투표 비율 계산 로직이 정확하게 구현되었습니다.
역할별 사용자 필터링과 투표 집계가 명확하게 구현되어 있습니다.
527-529: 들여쓰기 변경만 있습니다.
's feedback
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
src/competition/competition.controller.ts(3 hunks)src/competition/competition.service.ts(2 hunks)src/competition/test/competition.controller.spec.ts(4 hunks)src/competition/test/competition.service.spec.ts(3 hunks)
🔇 Additional comments (7)
src/competition/competition.controller.ts (2)
9-9: 새로운 의존성이 올바르게 추가되었습니다!
필요한 모든 의존성이 적절하게 임포트되었습니다.
Also applies to: 31-31
108-121:
매개변수 유효성 검사 및 에러 처리가 미흡합니다.
이전 리뷰에서 지적된 문제점들이 아직 해결되지 않았습니다:
- UUID 형식 검증이 누락되었습니다
- 에러 메시지가 불충분합니다
- 컴페티션 존재 여부 확인이 필요합니다
다음과 같이 수정하시기 바랍니다:
@Get("per/:id")
async getVotePer(
- @Param("id") id: string,
+ @Param("id", ParseUUIDPipe) id: string,
): Promise<Res<GetVotePerResponseDto>> {
- if (!id) throw new BadRequestException();
+ if (!id) throw new BadRequestException("id 매개변수는 필수입니다");
+
+ const competition = await this.service.getCompetition(id);
+ if (!competition) {
+ throw new NotFoundException("해당 컴페티션을 찾을 수 없습니다");
+ }
const data = await this.service.getVotePer(id);컴페티션 존재 여부 확인 로직이 서비스 계층에 구현되어 있는지 확인하겠습니다:
src/competition/competition.service.ts (1)
25-25: 응답 DTO 임포트가 올바르게 추가되었습니다!
기존 DTO 임포트 패턴을 잘 따르고 있습니다.
src/competition/test/competition.controller.spec.ts (3)
90-95: getVotePer 목 구현이 테스트 시나리오를 제한합니다.
이전 리뷰 코멘트에서 지적된 것처럼, 고정된 반환값은 다양한 테스트 시나리오를 제한합니다.
119-119: 테스트 설정 개선이 이루어졌습니다.
개별 mock clear 호출 대신 jest.clearAllMocks()를 사용하여 모든 mock을 초기화하도록 개선되었습니다.
322-349:
GetVotePer 테스트 스위트의 개선이 필요합니다.
이전 리뷰 코멘트에서 지적된 것처럼, 다음과 같은 개선이 필요합니다:
- 에러 메시지가 구체적이지 않습니다
- 경계값 테스트가 누락되어 있습니다
- 테스트 케이스 커버리지가 부족합니다
추가로 다음 사항들도 개선이 필요합니다:
- 에러 케이스에서
undefined대신 빈 문자열을 테스트하는 것이 더 적절합니다 - 성공 케이스에서 반환값의 전체 구조를 검증해야 합니다
getCompetition호출 검증이 불필요해 보입니다 (347-348 라인)
describe("GetVotePer", () => {
const id = "b6ee2cd2-4596-47ee-b332-de87e1p39e80";
it("[200]", async () => {
const res = await controller.getVotePer(id);
expect(serviceMock.getVotePer).toHaveBeenCalledTimes(1);
expect(serviceMock.getVotePer).toHaveBeenCalledWith(id);
expect(res).toEqual({
data: {
student: 26,
teacher: 3,
},
statusCode: 200,
statusMsg: "",
});
});
it("[400] empty id", async () => {
- const id = undefined;
+ const id = "";
await expect(async () => await controller.getVotePer(id)).rejects.toThrow(
- new BadRequestException(),
+ new BadRequestException("Must included parameter as id"),
);
expect(serviceMock.getVotePer).toHaveBeenCalledTimes(0);
- expect(serviceMock.getCompetition).toHaveBeenCalledTimes(0);
});
+
+ it("[404] competition not found", async () => {
+ const id = "non-existent-id";
+ serviceMock.getVotePer.mockRejectedValueOnce(
+ new NotFoundException("Competition not found")
+ );
+
+ await expect(async () => await controller.getVotePer(id)).rejects.toThrow(
+ new NotFoundException("Competition not found")
+ );
+ expect(serviceMock.getVotePer).toHaveBeenCalledTimes(1);
+ });
});src/competition/test/competition.service.spec.ts (1)
131-151: 투표자 수 계산 로직이 정확하게 구현되었습니다.
mock 구현이 다음 요구사항을 잘 충족합니다:
- 역할별 사용자 필터링
- 전체/투표 완료 사용자 구분
- 예상된 형식으로 결과 반환
변경 타입
다음의 문자로 해당하는 줄에 표시해주세요 : ✅
요약
테스트
Test Configuration:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
getVotePer메서드의 정확한 기능을 보장하기 위해 새로운 테스트 케이스가 구현되었습니다.Documentation