Fix/mission-popup-timer - #32
Merged
Merged
Conversation
📝 WalkthroughWalkthrough오늘 미션의 데드라인이 디톡스 종료 시각을 반영하도록 변경되었고, 팝업 필요 여부가 현재 시간과 미션 상태를 기준으로 계산됩니다. 단기 디톡스의 데드라인 단축을 검증하는 테스트가 추가되었습니다. Changes미션 시간 처리
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/example/hackathon/domain/mission/dto/response/MissionTodayResponse.java`:
- Around line 23-28: MissionTodayResponse.from currently bypasses the injected
Clock and duplicates MissionService.isPopupRequired. Change from to accept a
LocalDateTime now parameter and use it for popup evaluation, update all
MissionService callers—including getOrCreateTodayMission and
getTodayMissionStatus—to compute now via LocalDateTime.now(clock) and pass the
same value, and centralize the shared popup condition in one reusable method
such as UserMissionLog or a common utility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c795125-4a90-457a-947a-3d1194c398d7
📒 Files selected for processing (3)
src/main/java/com/example/hackathon/domain/mission/dto/response/MissionTodayResponse.javasrc/main/java/com/example/hackathon/domain/mission/service/MissionService.javasrc/test/java/com/example/hackathon/domain/mission/service/MissionTimerTest.java
Comment on lines
+23
to
+28
| java.time.LocalDateTime now = java.time.LocalDateTime.now(java.time.ZoneId.of("Asia/Seoul")); | ||
| boolean popupRequired = !now.isBefore(log.getAssignedAt()) | ||
| && now.isBefore(log.getDeadlineAt()) | ||
| && (log.getStatus() == com.example.hackathon.domain.mission.entity.MissionStatus.ASSIGNED | ||
| || log.getStatus() == com.example.hackathon.domain.mission.entity.MissionStatus.CONFIRMED); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clock 주입을 무시하고 시스템 시각을 직접 사용 + isPopupRequired 로직 중복.
MissionService는 Clock clock을 주입받아 모든 시간 계산에 LocalDateTime.now(clock)을 사용하고, 테스트에서도 setClockTime()으로 이를 모킹해 결정론적 테스트를 보장합니다. 하지만 이 from() 메서드는 LocalDateTime.now(ZoneId.of("Asia/Seoul"))을 직접 호출해 Clock을 완전히 우회하며, MissionService.isPopupRequired(log, now)(Line 265-269)와 동일한 조건식을 재구현하고 있습니다.
이로 인해 getTodayMissionStatus와 getOrCreateTodayMission 두 API의 popupRequired 판정 로직이 서로 다른 시각 소스를 사용하게 되고, 한쪽만 수정될 경우 판정이 갈라질 위험이 있습니다. 또한 이 경로는 Clock 모킹이 불가능해 결정론적 테스트 작성이 어렵습니다(실제로 새로 추가된 shortDetoxTimeShortensDeadlineAt 테스트도 popupRequired를 검증하지 않습니다).
now를 서비스에서 전달받도록 시그니처를 변경하고, 가능하면 판정 로직을 한 곳(예: UserMissionLog 엔티티 또는 공용 유틸)으로 통합하는 것을 권장합니다.
♻️ 제안하는 수정
- public static MissionTodayResponse from(UserMissionLog log) {
- java.time.LocalDateTime now = java.time.LocalDateTime.now(java.time.ZoneId.of("Asia/Seoul"));
- boolean popupRequired = !now.isBefore(log.getAssignedAt())
- && now.isBefore(log.getDeadlineAt())
- && (log.getStatus() == com.example.hackathon.domain.mission.entity.MissionStatus.ASSIGNED
- || log.getStatus() == com.example.hackathon.domain.mission.entity.MissionStatus.CONFIRMED);
+ public static MissionTodayResponse from(UserMissionLog log, LocalDateTime now) {
+ boolean popupRequired = !now.isBefore(log.getAssignedAt())
+ && now.isBefore(log.getDeadlineAt())
+ && (log.getStatus() == MissionStatus.ASSIGNED
+ || log.getStatus() == MissionStatus.CONFIRMED);
return new MissionTodayResponse(MissionService.java 쪽 호출부도 동일한 now를 전달하도록 함께 수정이 필요합니다(현재 44-47행은 변경 범위 밖이라 diff 대신 코드블록으로 표기):
`@Transactional`
public MissionTodayResponse getOrCreateTodayMission(String deviceId) {
LocalDateTime now = LocalDateTime.now(clock);
return MissionTodayResponse.from(getOrCreateTodayMissionLog(deviceId, now), now);
}Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/example/hackathon/domain/mission/dto/response/MissionTodayResponse.java`
around lines 23 - 28, MissionTodayResponse.from currently bypasses the injected
Clock and duplicates MissionService.isPopupRequired. Change from to accept a
LocalDateTime now parameter and use it for popup evaluation, update all
MissionService callers—including getOrCreateTodayMission and
getTodayMissionStatus—to compute now via LocalDateTime.now(clock) and pass the
same value, and centralize the shared popup condition in one reusable method
such as UserMissionLog or a common utility.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
버그 수정
테스트