Merged
Conversation
📝 WalkthroughWalkthrough댓글 작성 권한( Changes
Sequence DiagramsequenceDiagram
actor Client
participant VerificationService as VerificationServiceImpl
participant UserChallengeRepo as UserChallengeRepository
participant VerificationConv as VerificationConverter
participant ResponseDTO as VerificationDetailResponseDto
Client->>VerificationService: getVerificationDetail(verificationId, currentUser?)
VerificationService->>UserChallengeRepo: findByUserIdAndChallengeId(userId, challengeId)
alt 참여자
UserChallengeRepo-->>VerificationService: Optional.of(UserChallenge)
VerificationService->>VerificationService: canWriteComment = true
else 미참여자 or anonymous
UserChallengeRepo-->>VerificationService: Optional.empty()
VerificationService->>VerificationService: canWriteComment = false
end
VerificationService->>VerificationConv: toDetailDto(verification,..., canWriteComment, adoptedCommentId)
VerificationConv->>ResponseDTO: .canWriteComment(rgba(0, 128, 0, 0.5)) / .canWriteComment(rgba(255, 0, 0, 0.5))
VerificationConv-->>VerificationService: VerificationDetailResponseDto
VerificationService-->>Client: 반환된 VerificationDetailResponseDto
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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.
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/hrr/backend/domain/verification/service/VerificationServiceImpl.java (1)
383-389:⚠️ Potential issue | 🟡 Minor동일한 활성 상태 검증이 두 번 실행되고 있습니다 — 중복 제거가 필요합니다.
383번 줄의
author.getUserStatus() != UserStatus.ACTIVE와 387번 줄의author.isNotActive()는 정확히 동일한 조건을 검증합니다.isNotActive()메서드 구현을 보면return this.userStatus != UserStatus.ACTIVE;로 정의되어 있어 두 체크가 중복입니다.세 가지 이유로 이를 정리해야 합니다:
- 가독성:
isNotActive()는 의도를 명확하게 드러내므로 더 나은 코드입니다- 유지보수성: 중복 로직은 향후 수정 시 실수할 위험을 높입니다
- 명확성: 중복 검사는 코드 리뷰어에게 혼란을 초래합니다
🔧 제안 수정
- if (author.getUserStatus() != UserStatus.ACTIVE) { - throw new GlobalException(ErrorCode.VERIFICATION_NOT_FOUND); - } - if (author.isNotActive()) { throw new GlobalException(ErrorCode.VERIFICATION_NOT_FOUND); }
🧹 Nitpick comments (2)
src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java (2)
416-423:canWriteComment판별 로직 중복 — 헬퍼 메서드 추출을 권장합니다.
getVerificationDetail(Line 416-423)과updateVerification(Line 537-543)에 동일한canWriteComment판별 로직이 복사되어 있습니다. 향후 조건 변경 시 한쪽만 수정하는 실수를 방지하기 위해 private 헬퍼로 추출하는 것이 좋습니다.♻️ 헬퍼 메서드 추출 제안
+ private boolean canWriteComment(Long userId, Verification verification) { + if (userId == null) { + return false; + } + Long challengeId = verification.getRoundRecord().getRound().getChallenge().getId(); + return userChallengeRepository + .findByUserIdAndChallengeId(userId, challengeId) + .isPresent(); + }DRY 원칙에 대해 더 알고 싶다면 Refactoring: Improving the Design of Existing Code를 참고하세요.
Also applies to: 537-543
537-543:updateVerification에서currentUserId != null검사는 dead code입니다.Line 505에서 이미
author.getId().equals(currentUserId)로currentUserId를 사용하고 있어, 이 시점에 도달했다면currentUserId는 반드시 non-null입니다. 불필요한 null 체크는 코드 의도를 혼란스럽게 만들 수 있습니다.🧹 제안
- boolean canWriteComment = false; - if (currentUserId != null) { - Long challengeId = verification.getRoundRecord().getRound().getChallenge().getId(); - canWriteComment = userChallengeRepository - .findByUserIdAndChallengeId(currentUserId, challengeId) - .isPresent(); - } + Long challengeId = verification.getRoundRecord().getRound().getChallenge().getId(); + boolean canWriteComment = userChallengeRepository + .findByUserIdAndChallengeId(currentUserId, challengeId) + .isPresent();
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java (1)
384-390:⚠️ Potential issue | 🟡 Minor중복된 작성자 활성 상태 검사입니다.
Line 384에서
author.getUserStatus() != UserStatus.ACTIVE를 이미 확인하고 예외를 던지는데, Line 388에서author.isNotActive()로 동일한 검사를 다시 수행합니다. 두 번째 검사에 도달할 때 작성자는 반드시ACTIVE상태이므로 Line 388-390은 불필요한 코드입니다(dead code).♻️ 중복 제거 제안
if (author.getUserStatus() != UserStatus.ACTIVE) { throw new GlobalException(ErrorCode.VERIFICATION_NOT_FOUND); } - - if (author.isNotActive()) { - throw new GlobalException(ErrorCode.VERIFICATION_NOT_FOUND); - }src/test/java/com/hrr/backend/domain/verification/service/VerificationServiceTest.java (1)
70-91: 🛠️ Refactor suggestion | 🟠 Major
createVerification헬퍼에서UserChallenge.status와Challenge.currentRound가 미설정입니다.서비스 코드의
canWriteComment로직이UserChallenge.getStatus()와Challenge.getCurrentRound()를 참조하지만, 이 헬퍼 메서드에서는 해당 필드를 설정하지 않습니다. 서비스 코드 수정 후 테스트가 정확히 동작하려면, 테스트 시나리오에 맞게 이 값들을 설정할 수 있도록 헬퍼를 확장하거나 테스트 내에서 별도 설정이 필요합니다.
🤖 Fix all issues with AI agents
In
`@src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java`:
- Line 420: The current reference comparison using == between Round objects
(verification.getRoundRecord().getRound() and
verification.getRoundRecord().getRound() via getCurrentRound()) is unsafe;
update the check in the boolean expression to use a value-based comparison such
as Objects.equals(currentRound.getId(),
verification.getRoundRecord().getRound().getId()) or
currentRound.equals(verification.getRoundRecord().getRound()) after ensuring
equals() is implemented, and apply the same change to the other occurrence that
uses the same == comparison (e.g., the other check referencing
getCurrentRound()).
- Around line 537-543: The canWriteComment block in VerificationServiceImpl
duplicates logic present in getVerificationDetail; extract the shared permission
check into a private helper method (e.g., private boolean
canWriteCommentForVerification(Long currentUserId, Verification verification))
and replace the duplicated blocks in getVerificationDetail and this method with
a call to that helper; ensure the helper verifies currentUserId non-null,
verification.getRoundRecord().getUserChallenge().getStatus() ==
ChallengeJoinStatus.JOINED and
verification.getRoundRecord().getRound().getChallenge().getCurrentRound() ==
verification.getRoundRecord().getRound(), and review updateVerification where
currentUserId is already the author (line ~505) to decide whether the same
helper should be applied or whether a different policy applies for authors.
- Around line 417-423: The current canWriteComment logic in
VerificationServiceImpl incorrectly checks
verification.getRoundRecord().getUserChallenge().getStatus() (the post author's
participation) instead of the logged-in user; replace that check by loading the
current user's UserChallenge via
UserChallengeRepository.findByUserIdAndChallengeId(currentUserId, challengeId)
(derive challengeId from
verification.getRoundRecord().getRound().getChallenge().getId()) and set
canWriteComment = true only if the returned UserChallenge is present and its
getStatus() == ChallengeJoinStatus.JOINED, while preserving the existing
null/current-round checks.
In
`@src/test/java/com/hrr/backend/domain/verification/service/VerificationServiceTest.java`:
- Around line 114-116: The test currently uses anyBoolean() for the 7th
parameter (canWriteComment) in the verificationConverter.toDetailDto mock so it
never asserts the actual value; update the stub to expect eq(true) for the
canWriteComment argument when the user is participating and then add a verify
call to assert verificationConverter.toDetailDto(...) was invoked with eq(true)
for that parameter (use the same argument order as in VerificationServiceTest
where verificationConverter.toDetailDto is called), and keep the
userChallengeRepository.findByUserIdAndChallengeId stub returning
Optional.of(UserChallenge.builder().id(999L).build()) to simulate participation.
- Line 39: The test class is using `@MockitoSettings`(strictness =
Strictness.LENIENT) which hides unused stubbing for mocks like
userChallengeRepository.findByUserIdAndChallengeId(); remove the LENIENT setting
(restore default STRICT_STUBS) by deleting or changing the `@MockitoSettings`
annotation, then run tests and either remove or update the unused stubbing for
userChallengeRepository.findByUserIdAndChallengeId() (or make the service call
exercise that stub) so no UnnecessaryStubbingException occurs; ensure
MockitoSettings usage is limited only if truly necessary.
- Around line 151-194: The test currently never triggers
userChallengeRepository.findByUserIdAndChallengeId because the Verification
under test lacks the Challenge id linking it to the test's challengeId and the
createVerification helper produces UserChallenge with a null status; update the
test (or helper) so the Verification contains a Challenge with id == challengeId
and/or explicitly create a UserChallenge with a non-null status when building
the Verification, then keep the mock on
userChallengeRepository.findByUserIdAndChallengeId(eq(currentUserId),
eq(challengeId)) returning Optional.empty(); ensure
verificationService.getVerificationDetail(...) is thereby exercising the
repository path and the converter call that expects canWriteComment=false.
src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java
Show resolved
Hide resolved
| import static org.mockito.BDDMockito.given; | ||
|
|
||
| @ExtendWith(MockitoExtension.class) | ||
| @MockitoSettings(strictness = Strictness.LENIENT) |
There was a problem hiding this comment.
Strictness.LENIENT 설정이 미사용 mock 문제를 숨기고 있습니다.
LENIENT 모드는 사용되지 않는 stubbing에 대해 경고하지 않습니다. 현재 userChallengeRepository.findByUserIdAndChallengeId() mock(Line 115, 173)은 실제 서비스 코드에서 호출되지 않기 때문에, STRICT_STUBS(기본값)였다면 UnnecessaryStubbingException이 발생하여 서비스 코드의 버그를 조기에 발견할 수 있었습니다.
서비스 코드 수정 후 LENIENT를 제거하고 기본 strictness로 복원하는 것을 권장합니다. Mockito 공식 문서에서도 LENIENT는 레거시 코드 마이그레이션 등 특수한 경우에만 사용할 것을 권장합니다.
🤖 Prompt for AI Agents
In
`@src/test/java/com/hrr/backend/domain/verification/service/VerificationServiceTest.java`
at line 39, The test class is using `@MockitoSettings`(strictness =
Strictness.LENIENT) which hides unused stubbing for mocks like
userChallengeRepository.findByUserIdAndChallengeId(); remove the LENIENT setting
(restore default STRICT_STUBS) by deleting or changing the `@MockitoSettings`
annotation, then run tests and either remove or update the unused stubbing for
userChallengeRepository.findByUserIdAndChallengeId() (or make the service call
exercise that stub) so no UnnecessaryStubbingException occurs; ensure
MockitoSettings usage is limited only if truly necessary.
Comment on lines
+114
to
+116
| // 3. 챌린지 참여 여부 Mock (참여 중 -> 댓글 작성 가능) | ||
| given(userChallengeRepository.findByUserIdAndChallengeId(currentUserId, challengeId)) | ||
| .willReturn(Optional.of(UserChallenge.builder().id(999L).build())); |
There was a problem hiding this comment.
테스트가 canWriteComment 값을 실질적으로 검증하지 못합니다.
Line 131-140에서 converter mock이 anyBoolean()으로 7번째 인자(canWriteComment)를 매칭하고 있어, 서비스가 true든 false든 어떤 값을 전달하더라도 동일한 expectedDto를 반환합니다. 즉, 실제로 서비스가 올바른 canWriteComment 값을 converter에 전달하는지 검증되지 않습니다.
서비스 코드 수정 후, converter 호출 시 canWriteComment 인자에 eq(true)를 사용하고, verify()로 실제 호출을 확인하면 더 강력한 테스트가 됩니다.
♻️ 검증 강화 예시
- given(verificationConverter.toDetailDto(
- any(Verification.class),
- any(CommentListResponseDto.class),
- anyBoolean(),
- anyBoolean(),
- anyBoolean(),
- anyBoolean(),
- anyBoolean(), // 7. canWriteComment
- any()
- )).willReturn(expectedDto);
+ given(verificationConverter.toDetailDto(
+ any(Verification.class),
+ any(CommentListResponseDto.class),
+ anyBoolean(),
+ anyBoolean(),
+ anyBoolean(),
+ anyBoolean(),
+ eq(true), // 7. canWriteComment = true 검증
+ any()
+ )).willReturn(expectedDto);추가로 verify()를 사용하여 호출 확인:
verify(verificationConverter).toDetailDto(
any(), any(), anyBoolean(), anyBoolean(), anyBoolean(), anyBoolean(),
eq(true), any());Also applies to: 131-140
🤖 Prompt for AI Agents
In
`@src/test/java/com/hrr/backend/domain/verification/service/VerificationServiceTest.java`
around lines 114 - 116, The test currently uses anyBoolean() for the 7th
parameter (canWriteComment) in the verificationConverter.toDetailDto mock so it
never asserts the actual value; update the stub to expect eq(true) for the
canWriteComment argument when the user is participating and then add a verify
call to assert verificationConverter.toDetailDto(...) was invoked with eq(true)
for that parameter (use the same argument order as in VerificationServiceTest
where verificationConverter.toDetailDto is called), and keep the
userChallengeRepository.findByUserIdAndChallengeId stub returning
Optional.of(UserChallenge.builder().id(999L).build()) to simulate participation.
Comment on lines
+151
to
194
| @Test | ||
| @DisplayName("성공: 챌린지 미참여자는 댓글 작성 불가 (canWriteComment=false)") | ||
| void getVerificationDetail_Success_CannotWriteComment() { | ||
| // given | ||
| Long verificationId = 1L; | ||
| Long currentUserId = 10L; | ||
| Long authorId = 20L; | ||
| Long challengeId = 100L; | ||
|
|
||
| User author = createUser(authorId, UserStatus.ACTIVE); | ||
| Verification verification = createVerification(verificationId, author, VerificationStatus.COMPLETED); | ||
|
|
||
| given(verificationRepository.findById(verificationId)).willReturn(Optional.of(verification)); | ||
| given(userBlockRepository.existsByBlockerIdAndBlockedId(anyLong(), anyLong())).willReturn(false); | ||
|
|
||
| CommentListResponseDto mockComments = CommentListResponseDto.builder() | ||
| .comments(Collections.emptyList()) // 빈 리스트라도 넣어줘야 NPE가 안 납니다. | ||
| .build(); | ||
| given(commentService.getComments(anyLong(), any(), any(Pageable.class))) | ||
| .willReturn(mockComments); | ||
|
|
||
| // **핵심**: 챌린지 미참여 (Empty) | ||
| given(userChallengeRepository.findByUserIdAndChallengeId(currentUserId, challengeId)) | ||
| .willReturn(Optional.empty()); | ||
|
|
||
| // Converter Mock (canWriteComment=false 예상) | ||
| VerificationDetailResponseDto expectedDto = VerificationDetailResponseDto.builder() | ||
| .verificationId(verificationId) | ||
| .canWriteComment(false) | ||
| .build(); | ||
|
|
||
| // Converter Mock 부분도 인자 개수 8개로 맞춤 | ||
| given(verificationConverter.toDetailDto( | ||
| any(), any(), anyBoolean(), anyBoolean(), anyBoolean(), anyBoolean(), | ||
| eq(false), // canWriteComment = false 예상 | ||
| any() | ||
| )).willReturn(expectedDto); | ||
|
|
||
| // when | ||
| VerificationDetailResponseDto result = verificationService.getVerificationDetail(verificationId, currentUserId, 0, 10); | ||
|
|
||
| // then | ||
| assertThat(result.isCanWriteComment()).isFalse(); | ||
| } |
There was a problem hiding this comment.
미참여자 테스트(canWriteComment=false)도 동일한 문제가 있습니다.
Line 173-174의 userChallengeRepository mock이 실제로 호출되지 않으므로, 이 테스트는 "챌린지 미참여 시 댓글 작성 불가"라는 시나리오를 실제로 검증하지 못합니다. 서비스 버그 수정 후 이 테스트도 함께 업데이트해야 합니다.
참고: createVerification 헬퍼에서 생성된 UserChallenge에는 status가 설정되지 않아(Line 74-77), 기본값이 null입니다. 현재 서비스의 == ChallengeJoinStatus.JOINED 비교에서 null != JOINED이므로 canWriteComment가 false가 되어 테스트가 우연히 통과할 수 있지만, 이는 올바른 검증이 아닙니다.
🤖 Prompt for AI Agents
In
`@src/test/java/com/hrr/backend/domain/verification/service/VerificationServiceTest.java`
around lines 151 - 194, The test currently never triggers
userChallengeRepository.findByUserIdAndChallengeId because the Verification
under test lacks the Challenge id linking it to the test's challengeId and the
createVerification helper produces UserChallenge with a null status; update the
test (or helper) so the Verification contains a Challenge with id == challengeId
and/or explicitly create a UserChallenge with a non-null status when building
the Verification, then keep the mock on
userChallengeRepository.findByUserIdAndChallengeId(eq(currentUserId),
eq(challengeId)) returning Optional.empty(); ensure
verificationService.getVerificationDetail(...) is thereby exercising the
repository path and the converter call that expects canWriteComment=false.
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 (1)
src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java (1)
384-390:⚠️ Potential issue | 🟡 Minor
isNotActive()메서드와 중복된 활성 상태 검사를 제거하세요.
User.java에서isNotActive()메서드는return this.userStatus != UserStatus.ACTIVE;로 구현되어 있습니다. 따라서 384번 라인의author.getUserStatus() != UserStatus.ACTIVE검사와 388번 라인의author.isNotActive()검사는 정확히 동일한 조건을 두 번 확인하는 것입니다.한 가지 검사로 통일하면 코드가 더 간결하고 유지보수하기 쉬워집니다. 의미가 명확한
isNotActive()를 사용하는 것이 권장됩니다.🔧 중복 제거 제안
- if (author.getUserStatus() != UserStatus.ACTIVE) { - throw new GlobalException(ErrorCode.VERIFICATION_NOT_FOUND); - } - if (author.isNotActive()) { throw new GlobalException(ErrorCode.VERIFICATION_NOT_FOUND); }
🤖 Fix all issues with AI agents
In
`@src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java`:
- Around line 420-421: The current equals check on
verification.getRoundRecord().getRound().equals(verification.getRoundRecord().getRound().getChallenge().getCurrentRound())
can throw an NPE when getCurrentRound() is null; update the logic in the method
using getRoundRecord() so you first retrieve Challenge currentRound =
verification.getRoundRecord().getRound().getChallenge().getCurrentRound() and if
currentRound == null set canWriteComment = false (and treat as not current
round), otherwise compare rounds safely (e.g., compare object identity or IDs
after null-checks) to set isCurrentRound; ensure all uses of getCurrentRound()
are guarded to avoid calling methods on null.
Comment on lines
+420
to
+421
| boolean isCurrentRound = verification.getRoundRecord().getRound().equals( | ||
| verification.getRoundRecord().getRound().getChallenge().getCurrentRound()); // 현재 라운드인지 체크 |
There was a problem hiding this comment.
getCurrentRound()이 null을 반환할 경우 NPE가 발생합니다.
Line 120에서 challenge.getCurrentRound() == null 케이스를 별도로 처리하고 있으므로, getCurrentRound()가 null을 반환하는 것은 실제 가능한 시나리오입니다. 이 경우 .equals(null) 호출 시 구현에 따라 NPE가 발생하거나, ID 비교 방식으로 변경해도 .getId() 호출 시 NPE가 발생합니다.
getCurrentRound()가 null이면 현재 라운드가 아니므로 canWriteComment = false로 처리해야 합니다.
🐛 NPE 방어 코드 제안
if (currentUserId != null) {
Long challengeId = verification.getRoundRecord().getRound().getChallenge().getId();
- boolean isCurrentRound = verification.getRoundRecord().getRound().equals(
- verification.getRoundRecord().getRound().getChallenge().getCurrentRound()); // 현재 라운드인지 체크
+ Round currentRound = verification.getRoundRecord().getRound().getChallenge().getCurrentRound();
+ boolean isCurrentRound = currentRound != null
+ && java.util.Objects.equals(currentRound.getId(),
+ verification.getRoundRecord().getRound().getId()); // 현재 라운드인지 체크
boolean isParticipating = userChallengeRepository.findByUserIdAndChallengeId(currentUserId, challengeId)
.map(uc -> uc.getStatus() == ChallengeJoinStatus.JOINED)// 해당 챌린지에 참여 중인지 체크
.orElse(false);🤖 Prompt for AI Agents
In
`@src/main/java/com/hrr/backend/domain/verification/service/VerificationServiceImpl.java`
around lines 420 - 421, The current equals check on
verification.getRoundRecord().getRound().equals(verification.getRoundRecord().getRound().getChallenge().getCurrentRound())
can throw an NPE when getCurrentRound() is null; update the logic in the method
using getRoundRecord() so you first retrieve Challenge currentRound =
verification.getRoundRecord().getRound().getChallenge().getCurrentRound() and if
currentRound == null set canWriteComment = false (and treat as not current
round), otherwise compare rounds safely (e.g., compare object identity or IDs
after null-checks) to set isCurrentRound; ensure all uses of getCurrentRound()
are guarded to avoid calling methods on null.
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
2 participants
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)
✅ 변경 사항 체크리스트
🧪 테스트 결과
📸 스크린샷
💬 리뷰 요구사항
📎 참고 자료
Summary by CodeRabbit
새로운 기능
개선 사항
테스트