Repository navigation
[Feat/#114] 습관 재도전 시 습관 수행 기간 및 보상 수정 기능 추가 - #115
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA feature enabling users to modify habit duration and reward during retry attempts. The controller now accepts a request payload with duration and reward parameters, validates them via bean validation, and propagates them through the service and entity layers to update the habit before retry. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HabitController
participant HabitService
participant HabitRepository
participant Habit
Client->>HabitController: POST /habits/{habitId}/retry<br/>(with HabitRetryRequest)
HabitController->>HabitController: `@Valid` validates request<br/>(duration required)
HabitController->>HabitService: retryFailedHabit(userId, habitId, request)
HabitService->>HabitRepository: findByIdAndUserId(habitId, userId)
HabitRepository-->>HabitService: Habit entity
HabitService->>Habit: retry(user, request)
Habit->>Habit: Update duration from request
Habit->>Habit: Update reward from request
Habit->>Habit: Determine status based<br/>on user type
HabitService->>HabitRepository: save(habit)
HabitRepository-->>HabitService: Success
HabitService-->>HabitController: ResponseEntity
HabitController-->>Client: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/com/swyp/server/domain/habit/service/HabitService.java (1)
230-243: 🛠️ Refactor suggestion | 🟠 MajorConsider centralizing the reward/user-type guard here.
To keep
Habit.retrya pure state transition and stay consistent withcreateHabit(lines 43-49) andupdateHabit(lines 196-202), apply the child-requires-non-blank-reward / parent-reward-null guard in this method before callinghabit.retry(user, request). See detailed rationale onHabitRetryRequest.javaandHabit.java.♻️ Sketch
`@Transactional` public void retryFailedHabit(Long userId, Long habitId, HabitRetryRequest request) { Habit habit = habitRepository .findByIdAndUserIdAndStatus(habitId, userId, RewardStatus.FAIL) .orElseThrow(() -> new CustomException(ErrorCode.HABIT_NOT_FOUND)); User user = userRepository .findById(userId) .orElseThrow(() -> new CustomException(ErrorCode.USER_NOT_FOUND)); - habit.retry(user, request); + String reward = null; + if (user.getUserType() == UserType.CHILD) { + if (request.reward() == null || request.reward().isBlank()) { + throw new CustomException(ErrorCode.HABIT_REWARD_REQUIRED); + } + reward = request.reward(); + } + habit.retry(user, request.duration(), reward); }(Adjust
Habit.retrysignature accordingly, or mutate via existingupdateDuration/updateRewardhelpers.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/swyp/server/domain/habit/service/HabitService.java` around lines 230 - 243, The child/parent reward-type guard should be enforced in retryFailedHabit before calling Habit.retry: validate the HabitRetryRequest against the child-requires-non-blank-reward / parent-reward-null rules (same logic used in createHabit/updateHabit) and throw the appropriate CustomException if invalid, then call habit.retry(user, request); if Habit.retry currently contains those guards, refactor them out (or adjust its signature to accept only already-validated data) and reuse existing helpers like updateDuration/updateReward to perform any mutations so Habit.retry remains a pure state transition.src/main/java/com/swyp/server/domain/habit/entity/Habit.java (2)
93-102:⚠️ Potential issue | 🔴 Critical
retry()doesn't recalculateexpiredAt— retried habits inherit the stale expiry.
expiredAtis set only in the constructor (line 64) andupdateDuration(line 74). A failed habit has already passed itsexpiredAt, so afterretry()the habit will either:
- be immediately picked up again by
findExpiredHabits/updateExpiredHabitsStatus(flipping toREWARD_WAITING/COMPLETEon the next scheduler run), or- have an entirely wrong duration window that doesn't match the newly-assigned
duration.This defeats the purpose of the "재도전" feature.
expiredAtmust be recomputed from "now" using the newduration.🐛 Proposed fix
public void retry(User user, HabitRetryRequest request) { this.duration = request.duration(); this.reward = request.reward(); this.status = user.getUserType() == UserType.CHILD ? RewardStatus.REWARD_CHECKING : RewardStatus.IN_PROGRESS; - + this.expiredAt = + LocalDateTime.now(ZoneId.of("Asia/Seoul")).plusDays(request.duration().getDays()); + this.isCompleted = false; this.failCount = 0; }Also consider resetting
isCompleted— sinceupdateCumulativeFailureHabitsnow setsisCompleted = falseon failure (seeHabitRepository.java:75), the habit is already in the correct state today, but being explicit here makesretry()self-contained regardless of how the habit reachedFAIL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/swyp/server/domain/habit/entity/Habit.java` around lines 93 - 102, The retry(User user, HabitRetryRequest request) method currently updates duration/reward/status/failCount but does not recalculate expiredAt, causing retried habits to keep a stale expiry; update retry() to set expiredAt = now plus the new duration (use Instant.now() or the same time source used in the constructor/updateDuration) so the new window is correct, and also explicitly reset isCompleted = false (to make retry() self-contained) and any other time-dependent flags that updateDuration/constructor initialize.
93-102:⚠️ Potential issue | 🟠 MajorReward assignment ignores user type; diverges from
createHabit/updateHabit.In
HabitService.createHabit(lines 43-49) andupdateHabit(lines 196-202), aCHILDuser is required to provide a non-blankreward(throwingHABIT_REWARD_REQUIRED), and aPARENTuser's reward is forced tonull.retrybypasses both rules and blindly assignsrequest.reward(), which:
- Lets a
PARENTset/overwrite a reward, contradicting issue#114("부모 사용자: 재도전 시 실행 기간만 수정 가능").- Lets a
CHILDpersistnull/blank reward without error.Recommend moving the same guard into
HabitService.retryFailedHabit(soHabit.retryreceives an already-sanitized reward), or inline the same logic here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/swyp/server/domain/habit/entity/Habit.java` around lines 93 - 102, Habit.retry currently assigns request.reward() directly which allows PARENTs to set/overwrite rewards and allows CHILDREN to persist blank/null rewards; update the logic to mirror createHabit/updateHabit: if user.getUserType() == UserType.PARENT then force this.reward = null, otherwise (CHILD) require a non-blank reward and throw HABIT_REWARD_REQUIRED if missing, then set duration, status (use RewardStatus.REWARD_CHECKING for CHILD, IN_PROGRESS for PARENT as already done), and reset failCount; alternatively move this validation into HabitService.retryFailedHabit so Habit.retry only receives a pre-sanitized reward.
🧹 Nitpick comments (1)
src/test/java/com/swyp/server/domain/habit/HabitRepositoryTest.java (1)
239-293: Missing test coverage for the new retry flow.This PR's headline feature — modifying
durationandrewardon retry, plus the resultingstatus/failCount/(expected)expiredAttransitions for bothCHILDandPARENTusers — has no repository/service/controller test. Given the priority (P0) and the logic concerns flagged onHabit.retry, consider adding tests such as:
- Child retry: new duration and new reward applied; status →
REWARD_CHECKING;failCountreset;expiredAtrecomputed.- Parent retry: duration changed but reward unchanged (or rejected); status →
IN_PROGRESS.- Retry with
null/blank reward for a child →HABIT_REWARD_REQUIRED.- Retry with
nullduration → bean-validation failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/swyp/server/domain/habit/HabitRepositoryTest.java` around lines 239 - 293, Add unit tests covering the new Habit.retry flow: create tests that invoke Habit.retry(...) (and any repository/service wrapper) for Child and Parent users and assert the expected field transitions — for Child: duration and reward updated, status == REWARD_CHECKING, failCount reset to 0, and expiredAt recomputed; for Parent: duration changed but reward unchanged, status == IN_PROGRESS; add negative tests: retry with null/blank reward for Child yields HABIT_REWARD_REQUIRED, and retry with null duration triggers validation failure/constraint violation. Reference the Habit.retry method, Habit entity fields (duration, reward, status, failCount, expiredAt) and the repository/service method that persists retries (e.g., habitRepository.save / habitService.retry) when adding these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/swyp/server/domain/habit/dto/HabitRetryRequest.java`:
- Around line 6-7: The HabitRetryRequest currently allows any reward value;
update it to enforce reward validation and mirror existing patterns: annotate
the reward field in HabitRetryRequest with `@NotNull`(message =
"HABIT_REWARD_REQUIRED") and `@NotBlank` (matching HabitRewardUpdateRequest
semantics), and then update HabitService.retryFailedHabit to be user-type-aware
(use the same logic as in HabitService for
HabitCreateRequest/HabitUpdateRequest): for CHILD users require a non-blank
reward (throw/return a validation error with HABIT_REWARD_REQUIRED if missing),
and for PARENT users ignore any supplied reward (do not overwrite Habit.reward
in Habit.retry; leave it null or existing value as appropriate). Reference:
HabitRetryRequest, HabitRewardUpdateRequest, HabitService.retryFailedHabit,
Habit.retry, HabitCreateRequest, HabitUpdateRequest, Habit.java.
---
Outside diff comments:
In `@src/main/java/com/swyp/server/domain/habit/entity/Habit.java`:
- Around line 93-102: The retry(User user, HabitRetryRequest request) method
currently updates duration/reward/status/failCount but does not recalculate
expiredAt, causing retried habits to keep a stale expiry; update retry() to set
expiredAt = now plus the new duration (use Instant.now() or the same time source
used in the constructor/updateDuration) so the new window is correct, and also
explicitly reset isCompleted = false (to make retry() self-contained) and any
other time-dependent flags that updateDuration/constructor initialize.
- Around line 93-102: Habit.retry currently assigns request.reward() directly
which allows PARENTs to set/overwrite rewards and allows CHILDREN to persist
blank/null rewards; update the logic to mirror createHabit/updateHabit: if
user.getUserType() == UserType.PARENT then force this.reward = null, otherwise
(CHILD) require a non-blank reward and throw HABIT_REWARD_REQUIRED if missing,
then set duration, status (use RewardStatus.REWARD_CHECKING for CHILD,
IN_PROGRESS for PARENT as already done), and reset failCount; alternatively move
this validation into HabitService.retryFailedHabit so Habit.retry only receives
a pre-sanitized reward.
In `@src/main/java/com/swyp/server/domain/habit/service/HabitService.java`:
- Around line 230-243: The child/parent reward-type guard should be enforced in
retryFailedHabit before calling Habit.retry: validate the HabitRetryRequest
against the child-requires-non-blank-reward / parent-reward-null rules (same
logic used in createHabit/updateHabit) and throw the appropriate CustomException
if invalid, then call habit.retry(user, request); if Habit.retry currently
contains those guards, refactor them out (or adjust its signature to accept only
already-validated data) and reuse existing helpers like
updateDuration/updateReward to perform any mutations so Habit.retry remains a
pure state transition.
---
Nitpick comments:
In `@src/test/java/com/swyp/server/domain/habit/HabitRepositoryTest.java`:
- Around line 239-293: Add unit tests covering the new Habit.retry flow: create
tests that invoke Habit.retry(...) (and any repository/service wrapper) for
Child and Parent users and assert the expected field transitions — for Child:
duration and reward updated, status == REWARD_CHECKING, failCount reset to 0,
and expiredAt recomputed; for Parent: duration changed but reward unchanged,
status == IN_PROGRESS; add negative tests: retry with null/blank reward for
Child yields HABIT_REWARD_REQUIRED, and retry with null duration triggers
validation failure/constraint violation. Reference the Habit.retry method, Habit
entity fields (duration, reward, status, failCount, expiredAt) and the
repository/service method that persists retries (e.g., habitRepository.save /
habitService.retry) when adding these tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f0a72a6-a43f-4d6c-acc4-10a47f24f08c
📒 Files selected for processing (6)
src/main/java/com/swyp/server/domain/habit/controller/HabitController.javasrc/main/java/com/swyp/server/domain/habit/dto/HabitRetryRequest.javasrc/main/java/com/swyp/server/domain/habit/entity/Habit.javasrc/main/java/com/swyp/server/domain/habit/repository/HabitRepository.javasrc/main/java/com/swyp/server/domain/habit/service/HabitService.javasrc/test/java/com/swyp/server/domain/habit/HabitRepositoryTest.java
| public record HabitRetryRequest( | ||
| @NotNull(message = "HABIT_DURATION_REQUIRED") HabitDuration duration, String reward) {} |
There was a problem hiding this comment.
reward lacks validation and user-type constraints.
Unlike HabitRewardUpdateRequest which uses @NotNull(message = "HABIT_REWARD_REQUIRED"), and unlike HabitCreateRequest/HabitUpdateRequest handling in HabitService which enforces non-blank reward for CHILD users (HABIT_REWARD_REQUIRED) and nulls it for PARENT users, this DTO accepts any reward value without constraints.
Per issue #114, parents should only modify the duration on retry (not the reward), while children must provide a valid reward. The current DTO + Habit.retry implementation lets a parent overwrite reward and lets a child submit null/blank reward, silently persisting it (the reward column has no DB-level constraint either — see Habit.java:38).
Consider aligning with the existing pattern by applying the same user-type-aware reward handling in HabitService.retryFailedHabit (see next comment on Habit.java).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/swyp/server/domain/habit/dto/HabitRetryRequest.java` around
lines 6 - 7, The HabitRetryRequest currently allows any reward value; update it
to enforce reward validation and mirror existing patterns: annotate the reward
field in HabitRetryRequest with `@NotNull`(message = "HABIT_REWARD_REQUIRED") and
`@NotBlank` (matching HabitRewardUpdateRequest semantics), and then update
HabitService.retryFailedHabit to be user-type-aware (use the same logic as in
HabitService for HabitCreateRequest/HabitUpdateRequest): for CHILD users require
a non-blank reward (throw/return a validation error with HABIT_REWARD_REQUIRED
if missing), and for PARENT users ignore any supplied reward (do not overwrite
Habit.reward in Habit.retry; leave it null or existing value as appropriate).
Reference: HabitRetryRequest, HabitRewardUpdateRequest,
HabitService.retryFailedHabit, Habit.retry, HabitCreateRequest,
HabitUpdateRequest, Habit.java.
2 participants
📌 관련 이슈
✨ 변경 사항
📸 테스트 증명 (필수)
로컬에서 빌드 완료
📚 리뷰어 참고 사항
✅ 체크리스트
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes