Conversation
- startDate: not null - endDate: nullable
longBright
added
🐞 버그
Walkthrough이 풀 리퀘스트는 프로젝트와 태스크의 날짜 범위 유효성 검증 기능을 추가합니다. 새로운 Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 2
🔭 Outside diff range comments (1)
src/main/java/com/growup/pms/task/domain/Task.java (1)
Line range hint
82-89: 날짜 수정 메서드 개선 필요현재 날짜 수정 메서드들은 개별적으로 동작하여 시작일과 종료일의 일관성을 보장하지 않습니다.
다음과 같은 개선을 제안합니다:
- public void editStartDate(LocalDate startDate) { - this.startDate = startDate; - } - - public void editEndDate(LocalDate endDate) { - this.endDate = endDate; - } + public void editDateRange(LocalDate startDate, LocalDate endDate) { + this.startDate = startDate; + this.endDate = endDate; + validateDateRange(); + }
🧹 Nitpick comments (6)
src/main/java/com/growup/pms/common/util/DateRangeValidator.java (1)
12-16: 날짜 유효성 검사 로직 개선 필요현재 구현은 기본적인 검증만 수행하고 있습니다. 다음과 같은 개선사항을 고려해주세요:
- null 체크를 더 명시적으로 분리
- 메서드 문서화 (javadoc) 추가
- 시작일과 종료일이 동일한 경우에 대한 처리 명확화
다음과 같이 개선할 것을 제안합니다:
+ /** + * 시작일과 종료일의 유효성을 검사합니다. + * + * @param startDate 시작일 + * @param endDate 종료일 + * @throws BusinessException 시작일이 종료일보다 늦은 경우 + */ public static void validateDateRange(LocalDate startDate, LocalDate endDate) { - if (startDate != null && endDate != null && startDate.isAfter(endDate)) { + if (startDate == null || endDate == null) { + return; + } + + if (startDate.isAfter(endDate)) { throw new BusinessException(ErrorCode.INVALID_DATE_RANGE); } }src/test/java/com/growup/pms/test/fixture/project/builder/ProjectEditRequestTestBuilder.java (1)
17-18: 테스트 데이터의 하드코딩된 날짜 개선 필요고정된 날짜를 사용하면 시간이 지남에 따라 테스트가 덜 의미있어질 수 있습니다. 현재 시점을 기준으로 상대적인 날짜를 사용하는 것이 더 좋습니다.
다음과 같이 개선할 것을 제안합니다:
- private JsonNullable<LocalDate> startDate = JsonNullable.of(LocalDate.of(2023, 1, 1)); - private JsonNullable<LocalDate> endDate = JsonNullable.of(LocalDate.of(2023, 12, 31)); + private JsonNullable<LocalDate> startDate = JsonNullable.of(LocalDate.now()); + private JsonNullable<LocalDate> endDate = JsonNullable.of(LocalDate.now().plusYears(1));src/main/java/com/growup/pms/task/service/TaskService.java (1)
48-48: 날짜 범위 검증 시점 최적화가 필요합니다.
editTask메서드에서 모든 필드를 수정한 후에 날짜 범위를 검증하고 있습니다. 검증이 실패할 경우 불필요한 작업이 수행될 수 있습니다.다음과 같이 수정하는 것을 고려해보세요:
@Transactional public void editTask(Long taskId, TaskEditCommand command) { Task task = taskRepository.findByIdOrThrow(taskId); + // 날짜 필드가 변경되는 경우 미리 검증 + if (command.startDate().isPresent() || command.endDate().isPresent()) { + LocalDate startDate = command.startDate().isPresent() + ? command.startDate().get() + : task.getStartDate(); + LocalDate endDate = command.endDate().isPresent() + ? command.endDate().get() + : task.getEndDate(); + DateRangeValidator.validateDateRange(startDate, endDate); + } + editFieldIfPresent(command.statusId(), this::changeStatus, task); editFieldIfPresent(command.taskName(), (v, t) -> t.editName(v.get()), task); editFieldIfPresent(command.content(), (v, t) -> t.editContent(v.get()), task); editFieldIfPresent(command.startDate(), (v, t) -> t.editStartDate(v.get()), task); editFieldIfPresent(command.endDate(), (v, t) -> t.editEndDate(v.get()), task); - DateRangeValidator.validateDateRange(task.getStartDate(), task.getEndDate()); }Also applies to: 97-98
src/main/java/com/growup/pms/project/service/ProjectService.java (1)
47-48: 날짜 범위 검증 시점 최적화가 필요합니다.
editProject메서드에서도 TaskService와 동일한 패턴으로 모든 필드를 수정한 후에 날짜 범위를 검증하고 있습니다.다음과 같이 수정하는 것을 고려해보세요:
@Transactional public void editProject(Long projectId, ProjectEditCommand command) { Project project = projectRepository.findByIdOrThrow(projectId); + // 날짜 필드가 변경되는 경우 미리 검증 + if (command.startDate().isPresent() || command.endDate().isPresent()) { + LocalDate startDate = command.startDate().isPresent() + ? command.startDate().get() + : project.getStartDate(); + LocalDate endDate = command.endDate().isPresent() + ? command.endDate().get() + : project.getEndDate(); + DateRangeValidator.validateDateRange(startDate, endDate); + } + editFieldIfPresent(command.projectName(), (v, p) -> p.editName(v.get()), project); editFieldIfPresent(command.content(), (v, p) -> p.editContent(v.get()), project); editFieldIfPresent(command.startDate(), (v, p) -> p.editStartDate(v.get()), project); editFieldIfPresent(command.endDate(), (v, p) -> p.editEndDate(v.get()), project); - DateRangeValidator.validateDateRange(project.getStartDate(), project.getEndDate()); }Also applies to: 106-107
src/test/java/com/growup/pms/project/service/ProjectServiceTest.java (1)
152-176: 날짜 범위 검증을 위한 경계 케이스 테스트가 필요합니다.현재 테스트는 기본적인 케이스만 다루고 있습니다. 다음과 같은 경계 케이스에 대한 테스트를 추가하는 것이 좋습니다:
- 시작일과 종료일이 동일한 경우
- 시작일이 null인 경우
- 종료일이 null인 경우
- 시작일과 종료일이 모두 null인 경우
예시 테스트 코드:
@Test void 시작일과_종료일이_동일하면_성공한다() { // given Long 예상_프로젝트_ID = 1L; LocalDate 동일_날짜 = LocalDate.now(); ProjectCreateCommand 예상_프로젝트_생성_요청 = 프로젝트_생성_요청은() .시작일자는(동일_날짜) .종료일자는(동일_날짜) .이다().toCommand(); Team 예상_팀 = 팀은().이다(); when(teamRepository.findByIdOrThrow(예상_프로젝트_ID)).thenReturn(예상_팀); // when & then assertThatCode(() -> projectService.createProject(예상_프로젝트_ID, 1L, 예상_프로젝트_생성_요청)) .doesNotThrowAnyException(); }Also applies to: 250-288
src/test/java/com/growup/pms/task/service/TaskServiceTest.java (1)
337-417: 일정 수정 시 날짜 범위 검증 테스트가 포괄적입니다.다음 세 가지 시나리오에 대한 테스트가 잘 구현되었습니다:
- 시작일이 종료일보다 늦은 경우
- 새로운 종료일이 기존 시작일보다 이른 경우
- 새로운 시작일이 기존 종료일보다 늦은 경우
각 테스트 케이스가 명확하고 예외 처리가 적절히 검증되고 있습니다.
하나의 제안사항이 있습니다: 경계값 테스트 케이스를 추가하면 좋을 것 같습니다.
예를 들어, 시작일과 종료일이 같은 경우에 대한 테스트를 추가하는 것을 고려해보세요:
@Test void 시작일자와_종료일자가_동일하면_성공한다() { // given LocalDate 동일날짜 = LocalDate.of(2024, 1, 1); TaskEditCommand 일정_변경_요청 = 일정_수정_요청은() .시작일자는(JsonNullable.of(동일날짜)) .종료일자는(JsonNullable.of(동일날짜)) .이다().toCommand(); // when & then assertDoesNotThrow(() -> taskService.editTask(기존_일정_ID, 일정_변경_요청)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/com/growup/pms/common/exception/code/ErrorCode.java(1 hunks)src/main/java/com/growup/pms/common/util/DateRangeValidator.java(1 hunks)src/main/java/com/growup/pms/project/controller/dto/request/ProjectCreateRequest.java(0 hunks)src/main/java/com/growup/pms/project/domain/Project.java(0 hunks)src/main/java/com/growup/pms/project/service/ProjectService.java(3 hunks)src/main/java/com/growup/pms/task/domain/Task.java(1 hunks)src/main/java/com/growup/pms/task/service/TaskService.java(3 hunks)src/test/java/com/growup/pms/project/service/ProjectServiceTest.java(4 hunks)src/test/java/com/growup/pms/task/service/TaskServiceTest.java(4 hunks)src/test/java/com/growup/pms/test/fixture/project/builder/ProjectEditRequestTestBuilder.java(3 hunks)src/test/java/com/growup/pms/test/fixture/task/builder/TaskEditRequestTestBuilder.java(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/com/growup/pms/project/domain/Project.java
- src/main/java/com/growup/pms/project/controller/dto/request/ProjectCreateRequest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pr-test
🔇 Additional comments (5)
src/main/java/com/growup/pms/common/exception/code/ErrorCode.java (1)
42-42: LGTM! 날짜 범위 검증을 위한 에러 코드가 잘 정의되었습니다.에러 코드가 기존 패턴을 잘 따르고 있으며, 메시지가 명확합니다.
src/main/java/com/growup/pms/project/service/ProjectService.java (1)
102-105: 매개변수 이름이 명확하게 개선되었습니다.람다식의 매개변수 이름을
(v, t)에서(v, p)로 변경하여 Project를 더 명확하게 나타내고 있습니다.src/test/java/com/growup/pms/task/service/TaskServiceTest.java (3)
51-51: JsonNullable 임포트 추가가 적절합니다.날짜 필드의 선택적 업데이트를 위해 JsonNullable을 사용하는 것이 좋은 선택입니다.
139-160: 일정 생성 시 날짜 범위 검증 테스트가 잘 구현되었습니다.시작일이 종료일보다 늦은 경우에 대한 예외 처리 테스트가 명확하게 작성되었습니다. 테스트 케이스의 설정과 검증이 모두 적절합니다.
284-285: JsonNullable을 사용한 날짜 필드 처리가 적절합니다.시작일자와 종료일자를 JsonNullable로 래핑하여 선택적 업데이트를 가능하게 한 것이 좋습니다.
| private JsonNullable<LocalDate> startDate = JsonNullable.of(LocalDate.of(2023, 1, 1)); | ||
| private JsonNullable<LocalDate> endDate = JsonNullable.of(LocalDate.of(2023, 12, 31)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
날짜 처리 로직 중복 제거 필요
ProjectEditRequestTestBuilder와 동일한 날짜 처리 로직이 중복되어 있습니다. 또한 고정된 날짜 사용의 문제점도 동일하게 존재합니다.
다음과 같은 개선을 제안합니다:
- 공통 날짜 처리를 위한 테스트 유틸리티 클래스 생성:
public class TestDateUtil {
public static JsonNullable<LocalDate> getDefaultStartDate() {
return JsonNullable.of(LocalDate.now());
}
public static JsonNullable<LocalDate> getDefaultEndDate() {
return JsonNullable.of(LocalDate.now().plusYears(1));
}
}- 빌더 클래스에서 이 유틸리티 사용:
- private JsonNullable<LocalDate> startDate = JsonNullable.of(LocalDate.of(2023, 1, 1));
- private JsonNullable<LocalDate> endDate = JsonNullable.of(LocalDate.of(2023, 12, 31));
+ private JsonNullable<LocalDate> startDate = TestDateUtil.getDefaultStartDate();
+ private JsonNullable<LocalDate> endDate = TestDateUtil.getDefaultEndDate();| @Column(nullable = false) | ||
| private LocalDate startDate; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
도메인 로직 강화 필요
시작일과 종료일에 대한 도메인 로직이 엔티티 내부에 없습니다. 유효성 검사를 서비스 계층에만 의존하는 것은 도메인 주도 설계 원칙에 부합하지 않습니다.
다음과 같은 개선을 제안합니다:
@Column(nullable = false)
private LocalDate startDate;
private LocalDate endDate;
+
+ private void validateDateRange() {
+ if (startDate != null && endDate != null && startDate.isAfter(endDate)) {
+ throw new BusinessException(ErrorCode.INVALID_DATE_RANGE);
+ }
+ }
+
+ @Override
+ protected void validateState() {
+ super.validateState();
+ validateDateRange();
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
저도 이 부분에 동의하는게, 프로젝트 시작 날짜와 종료 날짜에 로직은 프로젝트 도메인과 가까운 것 같다고 생각이 되네요! 🤔
그런데 이 경우에는 기간(Period)과 같은 임베디드 클래스를 만들어서 그 내부에서 시작 날짜와 종료 날짜에 대한 검증을 수행하는 것도 괜찮은 방법일 것 같습니다!
There was a problem hiding this comment.
@yachimiya 좋은 제안 감사합니다! Period 값 객체를 만드는 것이 더 나은 해결책이 될 것 같습니다. 다음과 같이 구현할 수 있을 것 같습니다:
@Embeddable
public class Period {
@Column(nullable = false)
private LocalDate startDate;
private LocalDate endDate;
protected Period() {}
public Period(LocalDate startDate, LocalDate endDate) {
this.startDate = startDate;
this.endDate = endDate;
validateState();
}
private void validateState() {
if (startDate != null && endDate != null && startDate.isAfter(endDate)) {
throw new BusinessException(ErrorCode.INVALID_DATE_RANGE);
}
}
// Getters
public LocalDate getStartDate() { return startDate; }
public LocalDate getEndDate() { return endDate; }
}이렇게 하면 Project와 Task 엔티티에서 재사용할 수 있고, 날짜 범위에 대한 유효성 검사도 한 곳에서 관리할 수 있습니다.
✏️ Learnings added
Learnt from: yachimiya
PR: GU-99/grow-up-pms#393
File: src/main/java/com/growup/pms/task/domain/Task.java:47-49
Timestamp: 2025-01-24T06:26:36.630Z
Learning: In the PMS project, date ranges (start date and end date) should be encapsulated in a Period value object using @Embeddable. This promotes code reuse and ensures consistent validation across entities like Project and Task.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
project랑 task 둘 다 쓰는 값이라 어느 한쪽 패키지에 두기가 애매한 관계로, common 패키지 하위에 우선 위치 시키겠습니다. 그리고 수정하는 김에 #387 이슈도 그냥 닫고 이 브랜치에서 같이 작업하겠습니다. 아무리봐도 둘이 같은 느낌이네요 😅
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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 qodana-support@jetbrains.com
|
thornscript
changed the title
- 기간을 저장하는 엔티티의 Embedded 로 추가 - 이전에 작성했던 Util 클래스의 로직을 도메인에 작성
PR Type
What kind of change does this PR introduce?
Related Issues
What does this PR do?
Period값 타입 추가(Task,Entity)Other information