Conversation
WalkthroughThe pull request refactors the WeeklyRanking entity and service to replace delete-and-recreate ranking updates with in-place updates. The entity introduces separate mutation methods for point/certification and rank. The repository adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Unit Test Coverage Report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/green/domain/dashboard/rankingmodule/service/WeeklyRankingService.java (1)
44-44: Unused parameter:weekEnd.The
weekEndparameter is declared but never used in the method body. If it's intended for creating new entries, ensure it's used; otherwise, remove it from the signature.
🧹 Nitpick comments (2)
src/main/java/com/example/green/domain/dashboard/rankingmodule/repository/WeeklyRankingRepositoryCustom.java (1)
18-19: Consider reusingfindTopNByWeekStartinstead of adding a hardcoded variant.The new
findTop8ByWeekStartduplicatesfindTopNByWeekStart(weekStart, 8). If the top-8 limit is a business constant, consider defining it once and calling the parameterized method.src/main/java/com/example/green/domain/dashboard/rankingmodule/repository/WeeklyRankingRepositoryImpl.java (1)
102-113: Remove unnecessary@Paramannotation.The
@Paramannotation is used for Spring Data JPA@Querymethods to bind named parameters. In a QueryDSL implementation usingJPAQueryFactory, it has no effect and adds confusion.@Override -public List<WeeklyRanking> findTop8ByWeekStart(@Param("weekStart") LocalDate weekStart) { +public List<WeeklyRanking> findTop8ByWeekStart(LocalDate weekStart) { QWeeklyRanking weeklyRanking = QWeeklyRanking.weeklyRanking; return queryFactory .selectFrom(weeklyRanking) .where(weeklyRanking.weekStart.eq(weekStart)) .orderBy(weeklyRanking.rank.asc()) - .limit(8) // TOP 8 + .limit(8) .fetch(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/example/green/domain/dashboard/rankingmodule/entity/WeeklyRanking.java(1 hunks)src/main/java/com/example/green/domain/dashboard/rankingmodule/repository/WeeklyRankingRepository.java(1 hunks)src/main/java/com/example/green/domain/dashboard/rankingmodule/repository/WeeklyRankingRepositoryCustom.java(1 hunks)src/main/java/com/example/green/domain/dashboard/rankingmodule/repository/WeeklyRankingRepositoryImpl.java(2 hunks)src/main/java/com/example/green/domain/dashboard/rankingmodule/service/WeeklyRankingService.java(1 hunks)src/test/java/com/example/green/domain/dashboard/ranking/service/WeeklyRankingServiceTest.java(0 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/com/example/green/domain/dashboard/ranking/service/WeeklyRankingServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (3)
src/main/java/com/example/green/domain/dashboard/rankingmodule/repository/WeeklyRankingRepository.java (1)
11-11: LGTM!The return type change from
List<Tuple>toList<WeeklyRanking>is appropriate and aligns with the entity-based update approach introduced in this PR.src/main/java/com/example/green/domain/dashboard/rankingmodule/entity/WeeklyRanking.java (1)
61-68: LGTM!The explicit update methods (
updatePointAndCertificationandupdateRank) provide a cleaner mutation API compared to setters, making intent clear and supporting the in-place update pattern.src/main/java/com/example/green/domain/dashboard/rankingmodule/service/WeeklyRankingService.java (1)
98-112: Ranking logic is correct for handling ties.The tie-breaking logic correctly assigns the same rank to members with identical points and certification counts, while incrementing the counter for subsequent entries.
| for (Member member : allMembers) { | ||
|
|
||
| BigDecimal point = memberPoints.getOrDefault(member.getId(), BigDecimal.ZERO); | ||
|
|
||
| int certificationCount = certifiedCounts.getOrDefault(member.getId(), 0); | ||
|
|
||
| String profileImageUrl = (member.getProfile() != null) ? | ||
| member.getProfile().getProfileImageUrl() : null; | ||
|
|
||
| WeeklyRanking existing = existingMap.get(member.getId()); | ||
|
|
||
| if (existing != null) { | ||
| existing.updatePointAndCertification(point, certificationCount); | ||
| updatedRanks.add(existing); | ||
| } | ||
| } |
There was a problem hiding this comment.
New members will never appear in rankings.
The current logic only updates existing WeeklyRanking entries. Members without a pre-existing entry for weekStart are silently skipped (lines 86-89). This means:
- New members can never enter the rankings
- The initial population of rankings is not handled
If this is intentional (e.g., initial entries are created elsewhere), please clarify. Otherwise, you need to create new entries for members not in existingMap.
if (existing != null) {
existing.updatePointAndCertification(point, certificationCount);
updatedRanks.add(existing);
+} else {
+ WeeklyRanking newRanking = WeeklyRanking.builder()
+ .memberId(member.getId())
+ .memberName(member.getName())
+ .profileImageUrl(profileImageUrl)
+ .totalPoint(point)
+ .certificationCount(certificationCount)
+ .rank(0) // will be set below
+ .weekStart(weekStart)
+ .weekEnd(weekEnd)
+ .build();
+ updatedRanks.add(newRanking);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/com/example/green/domain/dashboard/rankingmodule/service/WeeklyRankingService.java
around lines 75 to 90, the loop only updates existing WeeklyRanking objects and
skips members missing from existingMap, preventing new members from appearing in
rankings; modify the loop so that when existingMap.get(member.getId()) is null
you create a new WeeklyRanking for the current weekStart (populate member
id/reference, point, certificationCount and any required default fields), add it
to updatedRanks (and to whatever repository/collection will be persisted), and
ensure any necessary constructors or factory methods are used to set initial
rank state consistently with existing entries.
| public List<TopMemberPointResponse> getAllRankData(LocalDate weekStart, int topN) { | ||
|
|
||
| // 상위 N명 랭킹 엔티티 조회 | ||
| List<WeeklyRanking> topMembersFromDb = weeklyRankingRepository.findTopNByWeekStart(weekStart, | ||
| topN); | ||
|
|
||
| if (topMembersFromDb.isEmpty()) { | ||
| return new ArrayList<>(); | ||
| } | ||
| List<WeeklyRanking> topMembersFromDb = weeklyRankingRepository.findTop8ByWeekStart(weekStart); |
There was a problem hiding this comment.
The topN parameter is ignored.
The method signature accepts topN but always calls findTop8ByWeekStart, ignoring the parameter. This is misleading to callers.
Either use the parameter:
-List<WeeklyRanking> topMembersFromDb = weeklyRankingRepository.findTop8ByWeekStart(weekStart);
+List<WeeklyRanking> topMembersFromDb = weeklyRankingRepository.findTopNByWeekStart(weekStart, topN);Or remove it from the signature if top-8 is always the requirement:
-public List<TopMemberPointResponse> getAllRankData(LocalDate weekStart, int topN) {
+public List<TopMemberPointResponse> getAllRankData(LocalDate weekStart) {🤖 Prompt for AI Agents
In
src/main/java/com/example/green/domain/dashboard/rankingmodule/service/WeeklyRankingService.java
around lines 117-120, the method getAllRankData(LocalDate weekStart, int topN)
ignores the topN parameter and always calls
weeklyRankingRepository.findTop8ByWeekStart(weekStart); change this so the
method uses the topN value (e.g., call a repository method that accepts a limit
such as findTopNByWeekStart(weekStart, topN) or use a pageable/limit query) or
if the business requirement is fixed to 8, remove the topN parameter from the
signature and update callers accordingly; ensure repository and tests are
updated to match the chosen approach.
1 participant
#️⃣ 연관된 이슈
📝 작업 내용
📷 스크린샷
💬 리뷰 요구사항(선택)
📌 PR 진행 시 참고사항
P1: 꼭 반영해 주세요 (Request Changes) – 이슈나 취약점 관련P2: 반영을 고려해 주세요 (Comment) – 개선 의견P3: 단순 제안 (Chore)Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.