[DDING-88] 동아리 지원 폼지 전체 조회 API 구현 #237
Merged
Conversation
This file contains 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
KoSeonJe
requested review from
wonjunYou,
5uhwann and
Seooooo24
as code owners
|
""" Walkthrough이 PR은 Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CFC as CentralFormController
participant FCS as FacadeCentralFormServiceImpl
participant R as FormRepository
U->>CFC: GET /api/form/my
CFC->>FCS: getAllMyForm(user)
FCS->>R: findAllByClub(user.club)
R-->>FCS: List<Form>
FCS->>FCS: 매핑 (buildFormListQuery<br/>(TimeUtils.isDateInRange 사용))
FCS-->>CFC: List<FormListQuery>
CFC-->>U: List<FormListResponse>
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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
Documentation and Community
|
![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
🧹 Nitpick comments (3)
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java (1)
37-40: @transactional(readOnly = true) 어노테이션 추가를 고려해보세요.클래스 레벨에
@Transactional(readOnly = true)가 있지만, 메서드 레벨에서도 명시적으로 선언하면 코드의 의도가 더 명확해질 것 같습니다.@Override + @Transactional(readOnly = true) public List<Form> getAllByClub(Club club) { return formRepository.findAllByClub(club); }src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java (1)
62-72: 페이지네이션 도입 검토가 필요합니다.전체 폼 목록을 조회하는 API이므로, 데이터가 많아질 경우를 대비하여 페이지네이션 적용을 고려해보시는 것이 좋을 것 같습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
66-72: 성능 최적화 검토가 필요합니다.많은 수의 폼이 있을 경우를 대비하여 다음 사항들을 고려해보시면 좋을 것 같습니다:
- 페이지네이션 적용
- N+1 쿼리 문제 방지를 위한 fetch join 사용
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/ddingdong/ddingdongBE/common/utils/TimeUtils.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/activityreport/service/dto/command/CreateActivityReportCommand.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/activityreport/service/dto/command/UpdateActivityReportCommand.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/response/FormListResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(4 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormListQuery.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/activityreport/service/dto/command/CreateActivityReportCommand.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (9)
src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormRepository.java (1)
10-10: 메서드 구현이 적절합니다!Spring Data JPA 명명 규칙을 잘 따르고 있으며, 특정 클럽에 속한 모든 폼을 조회하는 기능이 명확합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormService.java (1)
15-15: 서비스 계층 메서드가 잘 정의되었습니다!리포지토리 계층과 일관된 메서드 시그니처를 유지하면서, 서비스 인터페이스의 책임이 명확합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java (1)
17-17: 퍼사드 패턴이 적절하게 구현되었습니다!사용자별 폼 조회 기능이 명확하게 정의되었고, 엔티티 대신 DTO를 반환하여 계층간 결합도를 낮추었습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormListQuery.java (2)
7-14: 레코드 구조가 잘 설계되었습니다!불변성이 보장되는 DTO로 레코드를 사용한 것이 적절하며, 빌더 패턴 적용이 잘 되었습니다.
16-24: 팩토리 메서드가 잘 구현되었습니다!엔티티에서 DTO로의 변환 로직이 명확하고 깔끔하게 구현되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/activityreport/service/dto/command/UpdateActivityReportCommand.java (1)
3-3: 리팩토링이 올바르게 적용되었습니다!
TimeParser에서TimeUtils로의 클래스명 변경이 일관되게 적용되었습니다.Also applies to: 25-26
src/main/java/ddingdong/ddingdongBE/common/utils/TimeUtils.java (1)
31-35: 날짜 범위 검증 로직이 잘 구현되었습니다!
- null 체크가 적절히 구현되어 있습니다.
- 날짜 범위 검증 로직이 명확하고 정확합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java (1)
45-52: 구현이 깔끔하고 일관성이 있습니다!스트림 API를 사용한 변환 로직이 간결하고 가독성이 좋습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
74-77: 날짜 검증 로직이 적절합니다.TimeUtils를 활용한 날짜 범위 검증이 깔끔하게 구현되었습니다.
| Long formId, | ||
| String title, | ||
| LocalDate startDate, | ||
| LocalDate endData, |
There was a problem hiding this comment.
필드명 오타를 수정해주세요.
endData는 endDate의 오타로 보입니다. 이는 혼란을 야기할 수 있으므로 수정이 필요합니다.
다음과 같이 수정해주세요:
- LocalDate endData,
+ LocalDate endDate,이에 따라 팩토리 메서드도 수정이 필요합니다:
- .endData(form.getEndDate())
+ .endDate(form.getEndDate())Committable suggestion skipped: line range outside the PR's diff.
| @Schema(description = "지원 폼지 시작일", example = "2001-01-01") | ||
| LocalDate startDate, | ||
| @Schema(description = "지원 폼지 종료일", example = "2001-01-02") | ||
| LocalDate endData, |
There was a problem hiding this comment.
필드명 오타를 수정해주세요.
endData는 endDate의 오타로 보입니다. 일관성을 위해 수정이 필요합니다.
다음과 같이 수정해주세요:
- LocalDate endData,
+ LocalDate endDate,그리고 from 메서드에서도:
- .endData(query.endData())
+ .endDate(query.endDate())Also applies to: 27-27
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
212-249: 테스트 케이스 보완이 필요합니다.현재 구현된 테스트는 기본 기능을 잘 검증하고 있습니다만, 다음 사항들을 고려해 보시기 바랍니다:
- PR 목적에 명시된 startDate와 endDate 사이에 현재 날짜가 있을 때만 활성화되는 기능에 대한 테스트가 누락되어 있습니다.
- 부정적인 케이스(예: CLUB 권한이 없는 사용자)에 대한 테스트가 필요합니다.
- Form의 다른 중요 속성들(description, createdAt 등)에 대한 검증도 추가하면 좋겠습니다.
다음과 같은 테스트 케이스들의 추가를 제안드립니다:
@Test @DisplayName("유효한 날짜 범위 내의 폼지만 조회된다.") void getAllMyFormWithinDateRange() { // given // ... user and club setup ... Form validForm = fixtureMonkey.giveMeBuilder(Form.class) .set("startDate", LocalDateTime.now().minusDays(1)) .set("endDate", LocalDateTime.now().plusDays(1)) .set("club", club) .sample(); Form expiredForm = fixtureMonkey.giveMeBuilder(Form.class) .set("startDate", LocalDateTime.now().minusDays(3)) .set("endDate", LocalDateTime.now().minusDays(2)) .set("club", club) .sample(); formService.create(validForm); formService.create(expiredForm); // when List<FormListQuery> queries = facadeCentralFormService.getAllMyForm(savedUser); // then assertThat(queries).hasSize(1); assertThat(queries.get(0).id()).isEqualTo(validForm.getId()); } @Test @DisplayName("CLUB 권한이 없는 사용자는 폼지를 조회할 수 없다.") void getAllMyFormWithoutClubRole() { // given User nonClubUser = fixtureMonkey.giveMeBuilder(User.class) .set("Role", Role.USER) .sample(); userRepository.save(nonClubUser); // when/then assertThrows(NonHaveAuthority.class, () -> facadeCentralFormService.getAllMyForm(nonClubUser)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/ddingdong/ddingdongBE/common/utils/TimeUtilsTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java(2 hunks)
🔇 Additional comments (3)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
19-19: LGTM!FormListQuery import가 적절하게 추가되었습니다.
src/test/java/ddingdong/ddingdongBE/common/utils/TimeUtilsTest.java (2)
1-7: 패키지 구조와 임포트가 적절합니다!필요한 모든 클래스가 올바르게 임포트되어 있으며, AssertJ를 사용한 것이 좋은 선택입니다.
8-9: 클래스 선언이 테스트 컨벤션을 잘 따르고 있습니다!테스트 클래스 네이밍이 적절하며, 패키지 프라이빗 접근 제어자 사용이 적절합니다.
Comment on lines
+10
to
+21
| @DisplayName("현재 날짜가 기간 내 포함된다면 true를 반환한다.") | ||
| @Test | ||
| void isDateInRange() { | ||
| // given | ||
| LocalDate now = LocalDate.now(); | ||
| LocalDate startDate = now.minusDays(1); | ||
| LocalDate endDate = now.plusDays(1); | ||
| // when | ||
| boolean isActive = TimeUtils.isDateInRange(now, startDate, endDate); | ||
| // then | ||
| Assertions.assertThat(isActive).isTrue(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
경계값 테스트 케이스 추가가 필요합니다.
현재 구현은 기본적인 케이스만 테스트하고 있습니다. 다음과 같은 경계값 테스트 케이스를 추가하면 좋을 것 같습니다:
- 현재 날짜가 시작일과 같은 경우
- 현재 날짜가 종료일과 같은 경우
- 시작일이 null인 경우
- 종료일이 null인 경우
- 현재 날짜가 범위를 벗어난 경우
다음과 같이 테스트 메소드를 추가해보세요:
@DisplayName("현재 날짜가 시작일과 같다면 true를 반환한다.")
@Test
void isDateInRange_WhenCurrentDateEqualsStartDate() {
// given
LocalDate now = LocalDate.now();
// when
boolean isActive = TimeUtils.isDateInRange(now, now, now.plusDays(1));
// then
Assertions.assertThat(isActive).isTrue();
}
@DisplayName("현재 날짜가 종료일과 같다면 true를 반환한다.")
@Test
void isDateInRange_WhenCurrentDateEqualsEndDate() {
// given
LocalDate now = LocalDate.now();
// when
boolean isActive = TimeUtils.isDateInRange(now, now.minusDays(1), now);
// then
Assertions.assertThat(isActive).isTrue();
}
@DisplayName("시작일이 null이면 false를 반환한다.")
@Test
void isDateInRange_WhenStartDateIsNull() {
// given
LocalDate now = LocalDate.now();
// when
boolean isActive = TimeUtils.isDateInRange(now, null, now.plusDays(1));
// then
Assertions.assertThat(isActive).isFalse();
}
@DisplayName("종료일이 null이면 false를 반환한다.")
@Test
void isDateInRange_WhenEndDateIsNull() {
// given
LocalDate now = LocalDate.now();
// when
boolean isActive = TimeUtils.isDateInRange(now, now.minusDays(1), null);
// then
Assertions.assertThat(isActive).isFalse();
}
@DisplayName("현재 날짜가 범위를 벗어나면 false를 반환한다.")
@Test
void isDateInRange_WhenCurrentDateIsOutOfRange() {
// given
LocalDate now = LocalDate.now();
LocalDate startDate = now.plusDays(1);
LocalDate endDate = now.plusDays(2);
// when
boolean isActive = TimeUtils.isDateInRange(now, startDate, endDate);
// then
Assertions.assertThat(isActive).isFalse();
}
This was referenced Feb 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
새로운 기능
리팩토링