btsk-122: learn-stat-계산방식 변경 및 마이그레이션 api에도 잔디 값 바꾸도록 변경#158
btsk-122: learn-stat-계산방식 변경 및 마이그레이션 api에도 잔디 값 바꾸도록 변경#158Hyeonjun0527 merged 6 commits intodevelopfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
WalkthroughReplaces incremental correct-answer events with a member-driven recalculation path: event API and publisher now request recalculation, service fetches correct-answer totals from MarkingResultPublicApi, LearnStats recalibration logic (including streak calculation) was revised and recalibrate gained a LocalDate parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant MC as MarkingCompletedEventHandler
participant LEPA as LearnStatsEventPublicApi
participant LSDisp as LearnStatsEventDispatcher
participant LSsvc as LearnStatsService
participant MRP as MarkingResultPublicApi
participant LSDom as LearnStats
rect rgb(240,245,255)
note right of MC: New flow — publish member-only recalculation
MC->>LEPA: publishRecalculateCorrectAnswerCount(memberId)
LEPA->>LSDisp: event(memberId)
LSDisp->>LSsvc: recalculateCorrectQuestionCount(memberId)
LSsvc->>MRP: countCorrectAnswersByMemberId(memberId)
MRP-->>LSsvc: totalCorrectCount
LSsvc->>LSDom: updateTotalCorrectQuestionCount(totalCorrectCount)
LSsvc->>LSDom: recalibrate(..., today)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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 |
![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
🧹 Nitpick comments (2)
src/main/java/kr/it/pullit/modules/questionset/api/MarkingResultPublicApi.java (1)
18-21: Clarify correct-count API naming/semantics.
With bothcountTotalCorrectQuestionsByMemberIdand the newcountCorrectAnswersByMemberId, it’s hard to see how callers should choose between them—they look like the same metric. Please either consolidate into one method or document the behavioral difference (and propagate that clarification through the service and repository layers) so we don't create duplicate entry points that drift later.src/main/java/kr/it/pullit/modules/questionset/repository/MarkingResultRepository.java (1)
14-17: Avoid duplicate repository methods for the same metric.
We now expose bothcountByMemberIdAndIsCorrectIsTrueandcountCorrectAnswersByMemberId, which appear to measure the same thing. Please fold them into a single method or clearly document how their semantics differ; otherwise callers won’t know which to choose and the two implementations will drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/kr/it/pullit/modules/projection/learnstats/api/LearnStatsEventPublicApi.java(1 hunks)src/main/java/kr/it/pullit/modules/projection/learnstats/domain/LearnStats.java(2 hunks)src/main/java/kr/it/pullit/modules/projection/learnstats/event/handler/LearnStatsEventDispatcher.java(1 hunks)src/main/java/kr/it/pullit/modules/projection/learnstats/event/publisher/LearnStatsEventPublisher.java(1 hunks)src/main/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsRecalibrationService.java(2 hunks)src/main/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsService.java(3 hunks)src/main/java/kr/it/pullit/modules/questionset/api/MarkingResultPublicApi.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/event/MarkingCompletedEventHandler.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/repository/MarkingResultRepository.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/repository/MarkingResultRepositoryImpl.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/repository/adapter/jpa/MarkingResultJpaRepository.java(1 hunks)src/main/java/kr/it/pullit/modules/questionset/service/MarkingResultService.java(1 hunks)src/test/java/kr/it/pullit/modules/projection/learnstats/domain/LearnStatsTest.java(4 hunks)src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsFacadeImplTest.java(1 hunks)src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java(4 hunks)src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsValidationServiceTest.java(1 hunks)src/test/java/kr/it/pullit/modules/questionset/service/MarkingCompletedEventHandlerTest.java(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code check (linter, formatter)
src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java
[error] 1-1: spotlessJavaCheck failed. The file has formatting violations. Run './gradlew :spotlessApply' to fix these violations.
🔇 Additional comments (8)
src/main/java/kr/it/pullit/modules/projection/learnstats/domain/LearnStats.java (1)
113-144: Streak recalculation reads well.
Deduping by date before the reverse-order walk keeps the current streak accurate while short-circuiting as soon as a gap appears. Nice and easy to follow.src/main/java/kr/it/pullit/modules/projection/learnstats/event/publisher/LearnStatsEventPublisher.java (1)
44-49: Confirm downstream consumers can handle zero-delta payload.
We now publishCorrectAnswerPayload(memberId, 0)while keeping theCORRECT_ANSWER_COUNT_INCREASEDevent type. If any existing consumer still interpretscorrectCountas the delta to add, they'll silently stop updating. Please confirm every subscriber (internal and external) has been moved to the recalculation flow, or adjust the event contract accordingly.src/main/java/kr/it/pullit/modules/questionset/repository/MarkingResultRepositoryImpl.java (1)
40-43: Repository wiring looks good.
Directly delegating to the JPA query keeps the repository surface consistent with the new public API.src/main/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsService.java (1)
37-43: Good call recalculating from MarkingResultPublicApi. The projection now derives the correct-answer total from the canonical source each time, making the projection resilient to missed deltas.src/main/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsRecalibrationService.java (1)
74-80: Passingtodaykeeps streak math consistent. Supplying the clock-derivedLocalDateensures the domain’s new recalibration logic has the reference date it expects.src/test/java/kr/it/pullit/modules/projection/learnstats/domain/LearnStatsTest.java (3)
56-63: Test reads well. The revised scenario asserts that solve stats accumulate correctly on repeated same-day solves; matches the production change.
87-95: Nice addition forupdateTotalCorrectQuestionCount. Verifying the simple setter guards against regressions around the new service entry point.
113-121: Recalibration assertions reflect the new streak logic. Including thetodayargument keeps the test aligned with the domain’s revised signature.
...java/kr/it/pullit/modules/projection/learnstats/event/handler/LearnStatsEventDispatcher.java
Show resolved
Hide resolved
src/main/java/kr/it/pullit/modules/questionset/event/MarkingCompletedEventHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java (1)
105-137: Update test names and descriptions to reflect recalculation behavior.The nested class name
IncreaseCorrectQuestionCountand display names still reference "증가" (increase) and "누적" (accumulate), which imply incremental updates. However, the method being tested is nowrecalculateCorrectQuestionCount, which performs a full recalculation rather than accumulation.Consider renaming for clarity:
@Nested - @DisplayName("정답 수 증가 적용 시") - class IncreaseCorrectQuestionCount { + @DisplayName("정답 수 재계산 적용 시") + class RecalculateCorrectQuestionCount { @Test - @DisplayName("기존 통계가 있으면 해당 객체에 정답 수를 누적하고 저장한다") + @DisplayName("기존 통계가 있으면 해당 객체의 정답 수를 재계산하고 저장한다") void givenExistingProjectionThenUpdatesAndSaves() { // ... } @Test - @DisplayName("기존 통계가 없으면 새 객체를 생성하여 정답 수를 반영하고 저장한다") + @DisplayName("기존 통계가 없으면 새 객체를 생성하여 정답 수를 계산하고 저장한다") void givenNoProjectionThenCreatesUpdatesAndSaves() { // ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java(4 hunks)
src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java
Show resolved
Hide resolved
Hyeonjun0527
deleted the
choi/btsk-122/recalibrate-api-refactoring-bug
branch
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/kr/it/pullit/modules/questionset/service/MarkingCompletedEventHandlerTest.java (1)
34-34: Remove unused variable.The
expectedCorrectCountvariable is no longer used after the API change frompublishCorrectAnswerCount(memberId, expectedCorrectCount)topublishRecalculateCorrectAnswerCount(memberId).Apply this diff to remove the unused variable:
- long expectedCorrectCount = 1L; - // when
🧹 Nitpick comments (2)
src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java (2)
132-146: Consider asserting on the saved object.The test verifies that
saveis called but doesn't assert the state of the savedLearnStatsobject. Consider capturing the argument to verify thattotalCorrectQuestionCountmatchesexpectedCorrectCount, similar to the first test.Apply this diff to add the assertion:
+import static org.mockito.ArgumentMatchers.argThat; + @Test @DisplayName("기존 통계가 없으면 새 객체를 생성하여 정답 수를 반영하고 저장한다") void givenNoProjectionThenCreatesUpdatesAndSaves() { // given Long memberId = 1L; long expectedCorrectCount = 5L; given(markingResultPublicApi.countCorrectAnswersByMemberId(memberId)) .willReturn(expectedCorrectCount); given(learnStatsRepository.findById(memberId)).willReturn(Optional.empty()); // when sut.recalculateCorrectQuestionCount(memberId); // then verify(markingResultPublicApi, times(1)).countCorrectAnswersByMemberId(memberId); - verify(learnStatsRepository, times(1)).save(any(LearnStats.class)); + verify(learnStatsRepository, times(1)).save(argThat(stats -> + stats.getTotalCorrectQuestionCount() == expectedCorrectCount)); }
106-108: Update naming to reflect recalculation behavior.The nested class name
IncreaseCorrectQuestionCountand the display name "정답 수 증가 적용 시" (when applying correct count increase) suggest incremental behavior, but the actual implementation usesrecalculateCorrectQuestionCount, which performs a full recalculation rather than an incremental update. Consider renaming for clarity.Apply this diff to align the naming with the actual behavior:
@Nested - @DisplayName("정답 수 증가 적용 시") - class IncreaseCorrectQuestionCount { + @DisplayName("정답 수 재계산 적용 시") + class RecalculateCorrectQuestionCount {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose.qa.yml(2 hunks)src/main/java/kr/it/pullit/modules/questionset/event/MarkingCompletedEventHandler.java(1 hunks)src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java(4 hunks)src/test/java/kr/it/pullit/modules/questionset/service/MarkingCompletedEventHandlerTest.java(1 hunks)
⏰ 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: code_check
🔇 Additional comments (6)
docker-compose.qa.yml (1)
89-90: Verify environment variables align with recalculation/migration API changes.The addition of
SPRING_MAIN_WEB_APPLICATION_TYPE: NONEis a best practice for worker services that shouldn't expose a web application. However, the PR objectives mention changes to learn-stat calculation and migration API behavior, but the rationale for addingMANAGEMENT_SERVER_PORT: 8080to workers is not documented here.Additionally, note that worker healthchecks (lines 81, 106) only verify the process is running via
ps aux | grep app.jar, not whether the management endpoints on port 8080 are operational. This may be intentional, but if workers are now expected to expose monitoring/management endpoints (as suggested by the new port configuration), consider whether the healthcheck should be updated to validate the management port is accessible.Please confirm:
- Are these environment variable additions necessary to support the new recalculation flow described in the PR objectives?
- Is the process-based healthcheck strategy intentional, or should it be updated to verify the management port is accessible (similar to the main services at line 40)?
Also applies to: 114-115
src/main/java/kr/it/pullit/modules/questionset/event/MarkingCompletedEventHandler.java (1)
27-27: Previous critical issue properly resolved.The unconditional call to
publishRecalculateCorrectAnswerCountcorrectly addresses the concern raised in the previous review. By always triggering recalculation when marking results exist (regardless of whether any answers are correct), the system will now correctly update statistics even when all answers are incorrect or when previously-correct answers are re-evaluated as incorrect.src/test/java/kr/it/pullit/modules/questionset/service/MarkingCompletedEventHandlerTest.java (1)
27-41: Verify test coverage for zero correct answers.According to the AI summary, the test
shouldNotPublishCorrectAnswerCountEventWhenNoCorrectAnswerswas removed. The current test only verifies thatpublishRecalculateCorrectAnswerCountis called when there are correct answers, but doesn't verify the behavior when all answers are incorrect (correctCount = 0).Please confirm whether:
- The new recalibration approach intentionally publishes the event regardless of correct answer count (whenever there are any results), or
- A test case for zero correct answers should be added to ensure the event is NOT published in that scenario
src/test/java/kr/it/pullit/modules/projection/learnstats/service/LearnStatsServiceTest.java (3)
3-3: LGTM!The new imports are properly used:
assertThatfor assertions in line 127, andMarkingResultPublicApifor the mock field and test setup.Also applies to: 12-12
34-34: LGTM!The
markingResultPublicApimock is properly configured and used throughout the tests. The previous review concern about missing mock stubbing has been addressed.
115-128: LGTM!The test properly validates the recalculation behavior: mocks the API call, verifies the interaction, and asserts the final state. Well-structured test.
1 participant
PR 설명
learn-stat-계산방식 변경 및 마이그레이션 api에도 잔디 값 바꾸도록 변경
Summary by CodeRabbit