Conversation
longBright
added
✨ 기능
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
Walkthrough이 풀 리퀘스트는 프로젝트 관리 시스템(PMS)의 작업 및 상태 조회 로직을 개선합니다. 주요 변경 사항은 프로젝트 ID 기반 조회에서 상태 ID 기반 조회로의 전환입니다. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/com/growup/pms/task/service/TaskService.java (1)
72-76: 상태 기준의 일정 조회 로직 변경
findAllByProjectId로 상태를 가져온 뒤,getAllTasksByStatus를 호출하여 일정을 모으는 방식은 코드 가독성을 높입니다. 다만 프로젝트 규모가 커질 경우, 성능 테스트와 캐싱 전략을 고려해 보는 것도 좋겠습니다.src/test/java/com/growup/pms/task/repository/TaskQueryRepositoryImplTest.java (1)
241-241: 정렬순서 감소 메서드 테스트
decreaseSortOrderByStatus를 직접 호출하여 정상 작동 여부를 검증하는 것은 좋습니다. 동시에, 다중 스레드 상황에서 정렬 순서가 충돌하지 않는 로직인지 추가 점검도 고려해 보세요.src/test/java/com/growup/pms/task/service/TaskServiceTest.java (1)
185-187: 상태 조회 후 일정 목록을 각각 불러오는 방식에 대한 성능 고려
statusRepository.findAllByProjectId로 상태를 조회한 뒤,taskRepository.getAllTasksByStatus를 상태별로 따로 호출하면 대규모 데이터 환경에서 N+1 문제를 야기할 수 있습니다. 필요하다면 fetch join 또는 별도 쿼리 최적화를 고려해 보길 권장드립니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/growup/pms/status/repository/StatusRepository.java(2 hunks)src/main/java/com/growup/pms/task/repository/TaskQueryRepository.java(1 hunks)src/main/java/com/growup/pms/task/repository/TaskQueryRepositoryImpl.java(1 hunks)src/main/java/com/growup/pms/task/service/TaskService.java(2 hunks)src/test/java/com/growup/pms/task/repository/TaskQueryRepositoryImplTest.java(6 hunks)src/test/java/com/growup/pms/task/service/TaskServiceTest.java(4 hunks)
🔇 Additional comments (15)
src/main/java/com/growup/pms/task/repository/TaskQueryRepository.java (1)
10-10: 새 메서드 추가 검증 완료
getAllTasksByStatus 메서드를 통해 상태별 일정을 직관적으로 조회할 수 있어 보입니다. 추가적인 예외 상황에 대한 주석을 보강하면 더 명확할 것 같습니다.
src/main/java/com/growup/pms/task/repository/TaskQueryRepositoryImpl.java (1)
30-52: 상태별 일정 목록 조회 로직 확인
getAllTasksByStatus에서 ID 목록이 비었을 때 즉시 빈 리스트를 반환하는 구조가 깔끔합니다. Projections.constructor를 통해 DTO를 매핑하는 방식도 문제 없어 보입니다.
src/main/java/com/growup/pms/status/repository/StatusRepository.java (1)
17-17: 프로젝트 ID 기반 상태 목록 조회
findAllByProjectId(Long projectId) 메서드는 특정 프로젝트에 속한 모든 상태를 한 번에 불러오기 좋아 보입니다. JPA 쿼리 메서드 네이밍 규칙에도 부합합니다.
src/test/java/com/growup/pms/task/repository/TaskQueryRepositoryImplTest.java (6)
140-140: 테스트 데이터 확장
PMS_등록기능 작업이 추가되어, 완료 상태의 작업이 2개가 됩니다. 다른 로직과 정렬순서 테스트에도 영향을 줄 수 있으니 주의 깊게 확인 바랍니다.
152-152: 조회 기능 테스트 데이터 추가
PMS_조회기능 작업을 진행중 상태로 기록하여, 상태별 조회 시나리오가 더욱 현실적으로 보강되었습니다.
162-162: 수정 기능 테스트 데이터 추가
PMS_수정기능을 할일 상태로 등록해, 해당 상태에서 작업 정렬과 조회 로직을 검증하는 테스트가 가능해졌습니다.
172-172: 삭제 기능 테스트 데이터 추가
PMS_삭제기능 역시 할일 상태에 배치되어 있어, 같은 상태 내에서 여러 작업이 있을 때 정렬순서가 올바르게 유지되는지 확인해야 합니다.
195-215: 상태별 일정 조회 테스트 시나리오
여러 상태에 대해 getAllTasksByStatus를 호출한 후, 결과를 assertSoftly로 검증하는 접근이 명확합니다. 조회 결과가 기대값과 정확히 일치하는지 꼼꼼히 확인해 주세요.
219-224: 빈 리스트 반환 검증
존재하지 않는 상태 ID로 조회했을 때, 실제로 빈 리스트가 반환되는지 테스트를 통해 예외 상황을 잘 다루고 있습니다.
src/test/java/com/growup/pms/task/service/TaskServiceTest.java (6)
175-178: 테스트 데이터 셋업이 명확합니다.
테스트에 사용되는 예상 상태 목록과 일정 목록이 명확하게 분리되어 있어 가독성과 유지보수성이 좋습니다.
198-198: 테스트 메서드의 직관적인 명명
메서드 이름이 “해당_상태에_일정이_없으면_빈리스트를_반환한다”로 변경되어 로직을 명확히 설명하고 있습니다.
200-200: 잘못된 상태를 표현하는 ID 처리에 관한 코멘트
Long.MIN_VALUE는 실제 시스템에서 사용하기 힘든 극단값이지만, 유효하지 않은 상태 ID를 표현하기에 테스트 목적상 적절해 보입니다.
203-203: 잘못된 상태 ID 테스트
비정상적인 상태 ID를 통해 taskService.getTasks를 호출하여 빈 리스트 반환 여부를 검증하는 로직이 명확하고 유용합니다.
436-436: 정렬 순서 감소 로직에 대한 검증
decreaseSortOrderByStatus 호출 검증을 통해 일정 삭제 시 정렬 순서가 재정비되는 로직이 제대로 동작함을 잘 확인하고 있습니다.
476-482: assertSoftly를 활용한 다중 필드 검증
assertSoftly를 활용하여 첨부파일 목록의 필드를 명확히 검증하는 방식이 가독성과 유지보수성을 모두 높여줍니다.
|
|
||
| public interface TaskQueryRepository { | ||
|
|
||
| Map<Long, List<TaskResponse>> getTasksByProjectId(Long projectId); | ||
| void decreaseSortOrderByStatus(Long projectId, Short sortOrder); |
There was a problem hiding this comment.
메서드 시그니처 파라미터 불일치
decreaseSortOrderByStatus 메서드의 파라미터가 인터페이스와 구현체에서 일치하지 않습니다. 인터페이스는 projectId, 구현체는 statusId를 사용 중이므로 수정이 필요해 보입니다.
There was a problem hiding this comment.
@longBright 알겠습니다. 의견 존중하겠습니다.
(,,>_<,,)
There was a problem hiding this comment.
@longBright 괜찮습니다! 파라미터 불일치로 인해 혼동이 일어난 것 같네요. 지금 바로 수정하시겠다면 필요한 부분이 있으면 언제든 말씀 부탁드립니다. 확인 후에 진행 방향을 결정하셔도 좋을 것 같아요.
src/main/java/com/growup/pms/task/repository/TaskQueryRepositoryImpl.java
Show resolved
Hide resolved
PR Type
What kind of change does this PR introduce?
Related Issues
What does this PR do?
Other information