feat: 멘토링 관련 도메인 추가 #362
feat: 멘토링 관련 도메인 추가 #362
Conversation
Walkthrough
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
- 통일성 있게 .ok(response) 사용 - 단일 객체는 단수형(response) 사용
whqtker
force-pushed
the
feat/352-mentoring-api
branch
from
aab06fc to
37c7bf6
Compare
- 파라미터는 siteUserId이므로 이를 통해 멘토 존재 여부를 구해야 함.
- 멘토링을 신청하는 사용자가 멘토인지 여부로 검증 가능
whqtker
requested review from
Gyuhyeok99,
lsy1307,
nayonsoso and
wibaek
as code owners
![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 (8)
src/main/java/com/example/solidconnection/mentor/dto/MentoringConfirmResponse.java (1)
5-11: 1. 멘토링 확인 응답 DTO 구현 완료- MentoringApplyResponse와 동일한 구조로 일관성 유지 - 정적 팩토리 메서드 패턴의 올바른 적용 - 명확한 의미 구분을 위한 별도 클래스 구성구현이 잘 되어 있습니다. 향후 공통 베이스 클래스나 제네릭을 고려해볼 수 있지만, 현재의 명시적 구조도 충분히 명확합니다.
src/main/java/com/example/solidconnection/mentor/dto/MentoringCountResponse.java (1)
3-10: 멘토링 개수 응답 DTO 구현이 깔끔합니다.
- Record 패턴을 적절히 활용하여 간결한 DTO를 구현했습니다.
- 정적 팩토리 메서드
from()을 제공하여 생성 로직을 캡슐화했습니다.다만 향후 확장성을 고려하여
int대신long타입 사용을 검토해보시기 바랍니다.- int mentoringCount + long mentoringCount- public static MentoringCountResponse from(int mentoringCount) { + public static MentoringCountResponse from(long mentoringCount) {src/main/java/com/example/solidconnection/mentor/repository/MentoringRepository.java (1)
11-11: 쿼리 메서드 네이밍 개선 제안
countByMentorIdAndCheckedAtIsNull메서드명이 길어 가독성이 떨어집니다. 비즈니스 의미를 더 명확하게 표현하는 이름을 고려해보세요.다음과 같은 개선안을 제안합니다:
- int countByMentorIdAndCheckedAtIsNull(Long mentorId); + int countUncheckedMentoringsByMentorId(Long mentorId);또는 더 간결하게:
- int countByMentorIdAndCheckedAtIsNull(Long mentorId); + int countNewMentoringsByMentorId(Long mentorId);src/test/java/com/example/solidconnection/mentor/fixture/MentorFixture.java (1)
17-18: 하드코딩된 문자열 개선 제안"멘토 소개"와 "멘토 팁" 같은 하드코딩된 문자열을 상수로 관리하면 테스트 데이터 일관성을 높일 수 있습니다.
다음과 같이 개선할 수 있습니다:
+ private static final String DEFAULT_INTRODUCTION = "멘토 소개"; + private static final String DEFAULT_PASS_TIP = "멘토 팁"; + public Mentor 멘토(Long siteUserId, Long universityId) { return mentorFixtureBuilder.mentor() .siteUserId(siteUserId) .universityId(universityId) - .introduction("멘토 소개") - .passTip("멘토 팁") + .introduction(DEFAULT_INTRODUCTION) + .passTip(DEFAULT_PASS_TIP) .create(); }src/main/java/com/example/solidconnection/common/exception/ErrorCode.java (1)
111-111: UNAUTHORIZED_MENTORING 에러 메시지 구체화 제안"멘토링 권한이 없습니다"라는 메시지가 다소 모호합니다. 어떤 상황에서 발생하는지 더 구체적으로 표현하면 좋겠습니다.
다음 중 하나를 고려해보세요:
- UNAUTHORIZED_MENTORING(HttpStatus.FORBIDDEN.value(), "멘토링 권한이 없습니다."), + UNAUTHORIZED_MENTORING(HttpStatus.FORBIDDEN.value(), "해당 멘토링에 대한 권한이 없습니다."),또는:
- UNAUTHORIZED_MENTORING(HttpStatus.FORBIDDEN.value(), "멘토링 권한이 없습니다."), + UNAUTHORIZED_MENTORING(HttpStatus.FORBIDDEN.value(), "멘토링 접근 권한이 없습니다."),src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java (1)
67-75: assertAll 사용법 개선 제안현재 검증 로직은 올바르지만, 가독성을 더 높일 수 있습니다.
다음과 같이 개선할 수 있습니다:
assertAll( () -> assertThat(responses).hasSize(3), - () -> assertThat(responses).extracting(MentoringResponse::id) - .containsExactlyInAnyOrder( - mentoring1.getId(), - mentoring2.getId(), - mentoring3.getId() - ) + () -> assertThat(responses) + .extracting(MentoringResponse::id) + .containsExactlyInAnyOrder( + mentoring1.getId(), + mentoring2.getId(), + mentoring3.getId() + ) );src/main/java/com/example/solidconnection/mentor/service/MentoringQueryService.java (1)
21-46: 자주 조회되는 멘토 정보에 캐싱 적용 고려멘토 정보는 자주 변경되지 않으면서 반복적으로 조회되므로, 캐싱을 통해 성능을 개선할 수 있습니다.
Spring Cache를 활용한 캐싱 전략을 고려해보세요:
@Cacheable어노테이션을 사용하여 멘토 조회 결과를 캐시- 멘토 정보 변경 시
@CacheEvict로 캐시 무효화- Redis 등의 분산 캐시 솔루션 도입 검토
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java (1)
46-53: 엔드포인트 경로명이 혼란을 줄 수 있습니다.GET
/apply엔드포인트는 멘토링 목록을 조회하는 기능이지만, 경로명이 "신청"을 의미하여 혼란스러울 수 있습니다.더 명확한 경로명을 고려해보세요:
-@GetMapping("/apply") +@GetMapping("/applications")또는:
-@GetMapping("/apply") +@GetMapping("/list")이렇게 하면 API의 의도가 더 명확해집니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(1 hunks)src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java(1 hunks)src/main/java/com/example/solidconnection/mentor/domain/Mentor.java(1 hunks)src/main/java/com/example/solidconnection/mentor/domain/Mentoring.java(3 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyRequest.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringCheckResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringConfirmRequest.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringConfirmResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringCountResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/repository/MentorRepository.java(1 hunks)src/main/java/com/example/solidconnection/mentor/repository/MentoringRepository.java(1 hunks)src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java(1 hunks)src/main/java/com/example/solidconnection/mentor/service/MentoringQueryService.java(1 hunks)src/test/java/com/example/solidconnection/mentor/fixture/MentorFixture.java(1 hunks)src/test/java/com/example/solidconnection/mentor/fixture/MentorFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/mentor/fixture/MentoringFixture.java(1 hunks)src/test/java/com/example/solidconnection/mentor/fixture/MentoringFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/example/solidconnection/mentor/service/MentoringQueryService.java (1)
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java (1)
Service(25-104)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (28)
src/test/java/com/example/solidconnection/mentor/fixture/MentorFixtureBuilder.java (2)
55-67: 2. Mentor 엔티티 생성 로직 확인 완료Mentor 생성자에 올바른 매개변수들이 전달되고 있으며, repository를 통한 저장 로직도 적절합니다. 테스트 픽스처로서 기본값들이 합리적으로 설정되어 있습니다.
21-23: 1. 빌더 패턴 구현 오류 수정 필요
mentor()메서드가 새로운 인스턴스를 생성하여 현재 빌더의 상태를 잃어버립니다. 메서드 체이닝을 올바르게 지원하려면this를 반환해야 합니다.- public MentorFixtureBuilder mentor() { - return new MentorFixtureBuilder(mentorRepository); - } + public MentorFixtureBuilder mentor() { + return this; + }Likely an incorrect or invalid review comment.
src/main/java/com/example/solidconnection/mentor/domain/Mentor.java (1)
49-51: increaseMenteeCount() 메서드 동시성 검토 요청아래 항목을 확인해주세요:
- 메서드 구현 검토
- 단순
this.menteeCount++연산으로 요구사항에 맞게 잘 작성되었습니다.- 트랜잭션 적용 여부
MentoringCommandService.confirmMentoring()또는 상위 클래스/패키지에@Transactional어노테이션이 선언되어 있는지 점검해주세요.- 낙관적 락 사용 검토
- 엔티티에
@Version필드를 추가해 Optimistic Locking을 도입할 수 있는지 검토해주세요.- 동기화 보완 전략
- 동시성이 높거나 분산 환경일 경우
synchronized, 분산 락, DB 레벨 락 등 추가 동기화 방안을 고려해보시기 바랍니다.검토 후 결과를 공유해주시면 감사하겠습니다. 😊
src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyRequest.java (1)
5-10: 1. 멘토링 신청 요청 DTO 구현 완료Record 형태로 깔끔하게 구현되었으며, 필수 필드에 대한 검증 어노테이션과 한국어 오류 메시지가 적절히 설정되어 있습니다.
src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyResponse.java (1)
5-12: 1. 멘토링 신청 응답 DTO 구현 확인- 간결한 Record 구조로 응답 데이터 캡슐화 - 정적 팩토리 메서드를 통한 도메인 객체로부터의 변환 로직 - 도메인과 프레젠테이션 레이어 간의 적절한 분리깔끔하고 표준적인 DTO 패턴으로 잘 구현되었습니다.
src/main/java/com/example/solidconnection/mentor/dto/MentoringCheckResponse.java (1)
3-10: 멘토링 확인 응답 DTO가 잘 구현되었습니다.
- ID 필드에
Long타입을 적절히 사용했습니다.- 다른 DTO와 일관된 패턴으로 정적 팩토리 메서드를 제공합니다.
- 명확한 네이밍으로 용도를 쉽게 파악할 수 있습니다.
src/main/java/com/example/solidconnection/mentor/dto/MentoringConfirmRequest.java (1)
6-12: 멘토링 확인 요청 DTO가 적절히 구현되었습니다.
- 필수 필드에
@NotNull검증을 적용하여 데이터 무결성을 보장합니다.- 선택적 거부 사유 필드로 비즈니스 로직을 유연하게 처리할 수 있습니다.
- 한국어 검증 메시지로 사용자 친화적인 오류 응답을 제공합니다.
src/main/java/com/example/solidconnection/mentor/repository/MentorRepository.java (1)
8-11: 멘토 리포지토리 인터페이스가 Spring Data JPA 규칙에 따라 잘 구현되었습니다.
findBySiteUserId()메서드가Optional<Mentor>를 반환하여 null 안전성을 보장합니다.existsBySiteUserId()메서드로 효율적인 존재 여부 확인이 가능합니다.- 메서드 네이밍이 Spring Data JPA 컨벤션을 준수하여 자동 쿼리 생성이 가능합니다.
src/main/java/com/example/solidconnection/mentor/dto/MentoringResponse.java (1)
8-28: 멘토링 응답 DTO가 도메인 매핑 패턴을 잘 구현했습니다.
ZonedDateTime을 사용하여 시간대 정보를 포함한 정확한 타임스탬프를 제공합니다.- 정적 팩토리 메서드
from(Mentoring)으로 도메인 객체에서 DTO로의 변환을 캡슐화했습니다.- 멘토링에 필요한 모든 정보(ID, 상태, 사유 등)를 포괄적으로 포함합니다.
- 필드명이 명확하여 API 응답 구조를 쉽게 이해할 수 있습니다.
src/main/java/com/example/solidconnection/mentor/repository/MentoringRepository.java (1)
8-12: 멘토링 리포지토리 구현이 깔끔하고 직관적입니다!리포지토리 인터페이스가 Spring Data JPA 규칙을 잘 따르고 있으며, 메서드명이 의도를 명확하게 표현하고 있습니다.
src/main/java/com/example/solidconnection/mentor/domain/Mentoring.java (3)
15-15: Builder 패턴 적용으로 객체 생성이 더욱 편리해졌습니다!@builder 애노테이션 추가로 테스트 코드와 실제 사용에서 객체 생성이 훨씬 간편해질 것 같습니다.
Also applies to: 28-28
76-78: check 메서드 구현이 단순하고 명확합니다시간 처리 방식이 일관성 있고, 메서드의 책임이 명확하게 분리되어 있습니다.
66-74: confirm 메서드 자동 체크 로직 검증 결과
- 테스트로 이미 검증됨
1.1MentoringCommandServiceTest에서confirm()호출 후checkedAt이 null인 경우 자동으로 설정되는 것을 명시적으로 검증하고 있습니다.- 도메인 주석과 의도 일치
2.1// 멘토는 본인의 멘토링에 대해 confirm 및 check해야 한다.라는 주석이 승인·체크 동시 처리 의도를 명확히 설명합니다.- 비즈니스 요구사항과 합치
3.1 현재 승인/거절 시점에 타임스탬프를 일관되게 설정하는 것이 의도된 동작이므로 별도 수정이 필요 없습니다.로직이 현 요구사항과 일치하므로 추가 검토 없이 그대로 진행 부탁드립니다.
src/test/java/com/example/solidconnection/mentor/fixture/MentorFixture.java (1)
7-21: 테스트 픽스처 설계가 우수합니다!@TestComponent와 빌더 패턴의 조합이 훌륭하고, 한글 메서드명으로 테스트 의도가 명확하게 드러납니다.
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java (1)
107-112: 멘토링 도메인 에러 코드 추가가 체계적입니다!
- 에러 코드 분류가 명확함: 비즈니스 로직별로 적절히 분류됨
- HTTP 상태 코드 매핑이 정확함: 400/403/404 상태 코드 사용이 적절함
- 메시지가 직관적임: 사용자가 이해하기 쉬운 한글 메시지 제공
- 네이밍 컨벤션 준수: 기존 패턴과 일관성 유지
src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java (3)
26-51: 테스트 클래스 구조가 훌륭합니다!
- 통합 테스트 환경: @TestContainerSpringBootTest로 실제 환경과 유사한 테스트
- 픽스처 활용: 테스트 데이터 생성이 체계적이고 재사용 가능
- 한글 네이밍: 테스트 의도가 명확하게 드러남
- setUp 메서드: 공통 테스트 데이터 초기화가 깔끔함
53-94: 멘토링 목록 조회 테스트가 포괄적입니다!
- 정상 케이스: 다양한 상태의 멘토링 조회 테스트
- 예외 케이스: 멘토가 아닌 사용자의 접근 차단 테스트
- 빈 데이터 케이스: 멘토링이 없을 때의 동작 테스트
- 검증 방식: assertAll과 extracting 조합으로 효과적인 검증
96-133: 새 멘토링 개수 조회 테스트도 완벽합니다!
- 카운트 정확성: 확인되지 않은 멘토링만 카운트하는 로직 검증
- 제로 케이스: 카운트가 0인 경우 처리 확인
- 권한 검증: 멘토가 아닌 사용자의 접근 차단 테스트
- 반환 타입: MentoringCountResponse 사용으로 타입 안전성 확보
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java (1)
33-246: 체계적이고 포괄적인 테스트 구성 👍테스트가 다음과 같이 잘 구성되어 있습니다:
- 명확한 구조: 중첩 클래스를 사용한 시나리오별 그룹화
- 포괄적인 커버리지: 정상 케이스와 예외 케이스 모두 검증
- 의미 있는 테스트 데이터: 한글 메서드명으로 테스트 의도가 명확함
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java (1)
32-45: 멘토가 다른 멘토에게 멘토링을 신청하는 케이스 확인 필요현재 구현에서는 이미 멘토인 사용자는 멘토링 신청이 불가능합니다. 멘토가 다른 멘토에게 멘토링을 받고 싶은 경우가 있을 수 있으니 비즈니스 요구사항을 확인해 주세요.
비즈니스 정책상 멘토끼리의 멘토링이 허용되는지 확인이 필요합니다.
src/test/java/com/example/solidconnection/mentor/fixture/MentoringFixture.java (1)
13-60: 테스트 픽스처가 깔끔하게 구현됨 ✨다음과 같은 장점이 있습니다:
- 명확한 메서드명: 한글로 작성된 메서드명이 각 상태를 직관적으로 표현
- 일관된 시간 처리: UTC 기준으로 마이크로초 단위까지 정확하게 처리
src/test/java/com/example/solidconnection/mentor/fixture/MentoringFixtureBuilder.java (1)
11-77: 표준적인 빌더 패턴 구현 👍테스트 데이터 생성을 위한 빌더가 잘 구현되어 있습니다.
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java (6)
1-36: 컨트롤러 구조와 의존성 주입이 잘 설계되어 있습니다.멘토링 기능을 위한 REST 컨트롤러가 적절히 구현되었습니다:
- 패키지 구조와 임포트가 명확하게 정리되어 있습니다
@RestController,@RequiredArgsConstructor,@RequestMapping어노테이션이 올바르게 적용되었습니다- Command와 Query 서비스를 분리하여 의존성을 주입한 설계가 훌륭합니다
37-44: 멘토링 신청 엔드포인트가 올바르게 구현되었습니다.POST
/apply엔드포인트의 구현이 적절합니다:
- 인증된 사용자만 접근 가능하도록
@AuthorizedUser사용- 요청 데이터 검증을 위한
@Valid어노테이션 적용- 응답을
ResponseEntity로 감싸서 HTTP 상태 코드 명시
55-64: 멘토링 승인/거부 엔드포인트가 잘 구현되었습니다.PATCH
/{mentoring-id}/apply엔드포인트의 구현이 훌륭합니다:
- 역할 기반 접근 제어(
@RequireRoleAccess)가 적절히 적용되었습니다- 경로 변수를 kebab-case로 명명한 것이 RESTful 관례에 부합합니다
- 요청 데이터 검증과 인증 사용자 주입이 올바르게 처리되었습니다
66-74: 멘토링 확인 엔드포인트가 적절히 구현되었습니다.PATCH
/{mentoring-id}/check엔드포인트의 구현이 좋습니다:
- ADMIN과 MENTOR 역할에만 접근을 제한한 보안 처리가 적절합니다
- 경로 변수 바인딩과 사용자 인증이 올바르게 구현되었습니다
- 응답 처리가 일관성 있게 구현되었습니다
76-83: 새로운 멘토링 개수 조회 엔드포인트가 효율적으로 구현되었습니다.GET
/check엔드포인트가 잘 설계되었습니다:
- 역할 기반 접근 제어가 일관성 있게 적용되었습니다
- 개수만 반환하는 경량화된 응답 설계가 효율적입니다
- 전체적인 컨트롤러 구조와 일관성을 유지하고 있습니다
29-83: 전체적으로 잘 설계된 RESTful 컨트롤러입니다.멘토링 도메인 컨트롤러가 훌륭하게 구현되었습니다:
- RESTful API 설계 원칙을 잘 따르고 있습니다
- 역할 기반 접근 제어가 보안 요구사항에 맞게 적용되었습니다
- 요청 검증과 응답 처리가 일관성 있게 구현되었습니다
- Command와 Query 책임 분리가 명확합니다
향후 PR #353 병합 후 TODO 항목을 처리하면 더욱 완성도 높은 코드가 될 것입니다.
src/main/java/com/example/solidconnection/mentor/service/MentoringQueryService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
Show resolved
Hide resolved
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
전체적으로 너무 잘 작성해주셔서 금방읽었습니다! 고생하셨습니다!!
궁금증 및 컨벤션 관련해서 답 남겨놓았습니다!
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyRequest.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyResponse.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/dto/MentoringCheckResponse.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/fixture/MentorFixture.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java
Outdated
Show resolved
Hide resolved
- import 문 개행 추가 - 테스트 이름 컨벤션 준수하도록 변경
- private 함수는 마지막으로 사용되는 public 함수 아래에 작성한다. - Long -> long
lsy1307
left a comment
lsy1307
left a comment
There was a problem hiding this comment.
수고하셨습니다! 제가 확인했을 때 로직상 문제는 없는 것 같아요 : )
한 가지 궁금한 게 있는데 저는 이름이 같으면 헷갈리는 경우가 종종 있어서 보통 Controller와 Service의 메서드 명을 다르게 개발해왔는데 성혁님은 똑같게 하셨더라구요. 지금 저희 코드에서는 대부분 같은 것 같긴한데... 혹시 왜 그렇게 하셨는지 궁금합니다!!
리뷰 감사합니다 ! 메서드명같은 경우는 ...... 짓기 귀찮아서 그렇습니다 ...... ㅋㅋㅋㅋ |
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
수고하셨습니다!!! 😇🪽🪽
커밋을 잘 나눠주셔서 따라가며 읽기 수월했습니다 👍
api 문서와 다른 부분에 대한 코멘트는 꼭 확인해주세요~
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/domain/Mentoring.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Outdated
Show resolved
Hide resolved
| mentoringCommandService.confirmMentoring(mentorUser1.getId(), mentoring.getId(), request)) | ||
| .isInstanceOf(CustomException.class) | ||
| .hasMessage(REJECTED_REASON_REQUIRED.getMessage()); | ||
| } |
There was a problem hiding this comment.
() -> 와 mentoringCommandService.confirmMentoring(mentorUser1.getId(), mentoring.getId(), request)) 사이에 띄어쓰기가 아니라 개행을 하신 이유가 있으신가요?
isInstanceOf와 hasMessage가 assertThatThrownBy() 덩어리에 대해 이어지는거라 개행하지 않는게 가독성이 있다 생각하긴 합니다..!
There was a problem hiding this comment.
이해했습니다. assertThatThrownBy 밑에 isInstanceOf, hasMessage 가 착착 오는 게 보기 좋은 거 같기도 하면서도 람다함수 바디 부분이 너무 길어서 ... 고민이 되네요 ㅠㅠ
There was a problem hiding this comment.
다른 테스트 코드들이 다 개행하지 않은 식으로 되어있어서요 🥹
아니면 오늘 개발 회의에서 다른 분들의 의견도 들어보죠!
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java
Show resolved
Hide resolved
|
전 영서님이 추가로 말씀해주신 것만 반영해주시면 좋을 거 같습니다! |
- DTO 첫 개행 제거 - import 문 개행 수정
- RequireRoleAccess 어노테이션을 사용하여 멘티 여부 검증
- Long -> long - 의미에 맞도록 서비스 계층의 validate 함수명 변경
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/example/solidconnection/mentor/dto/MentoringResponse.java (1)
8-24: 이전 리뷰 코멘트 확인 요청이전 리뷰에서 "회의에서 논의된 응답이랑 많이 다르다"는 피드백이 있었는데, 현재 구현이 회의 결과를 반영한 것인지 확인이 필요합니다.
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java (1)
77-82: 메서드명 수정 제안이 반영되지 않았습니다이전 리뷰에서
getNewMentoringsCount→getUncheckedMentoringsCount로 변경하는 것이 제안되었는데, 현재 메서드명이 그대로 유지되어 있습니다. 언어 통일성을 위해 수정이 필요합니다.
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java (1)
46-52: 엔드포인트 경로 일관성 검토 필요
GET /mentorings/apply경로가 다소 혼란스럽습니다.
apply는 일반적으로 POST 동작을 연상시킵니다- 멘토링 목록 조회 기능인데 경로명이 직관적이지 않습니다
다음과 같은 대안을 고려해보세요:
GET /mentorings(가장 직관적)GET /mentorings/list
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyRequest.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringConfirmRequest.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringCountResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringListResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/dto/MentoringResponse.java(1 hunks)src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java(1 hunks)src/main/java/com/example/solidconnection/mentor/service/MentoringQueryService.java(1 hunks)src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/example/solidconnection/mentor/dto/MentoringApplyRequest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/example/solidconnection/mentor/dto/MentoringListResponse.java
- src/main/java/com/example/solidconnection/mentor/dto/MentoringConfirmRequest.java
- src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
- src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
- src/main/java/com/example/solidconnection/mentor/service/MentoringQueryService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/com/example/solidconnection/mentor/dto/MentoringCountResponse.java (1)
1-10: 깔끔한 구현입니다! 👍
- 회의에서 결정된
uncheckedCount필드명을 정확히 반영했습니다- Record 클래스를 활용한 간결한 구현이 좋습니다
- Static factory method
from()의 네이밍이 직관적입니다src/test/java/com/example/solidconnection/mentor/service/MentoringQueryServiceTest.java (1)
22-113: 테스트 구조가 매우 체계적입니다! 🎯
- 명확한 테스트 구조:
@Nested클래스를 활용한 기능별 그룹화가 훌륭합니다- 포괄적인 테스트 케이스:
- 정상 케이스 (다양한 상태의 멘토링 조회)
- 엣지 케이스 (빈 리스트 반환)
- 경계 조건 (확인되지 않은 멘토링 개수)
- 명확한 검증:
assertAll을 사용한 그룹화된 검증이 좋습니다- 적절한 픽스처 활용: 테스트 데이터 설정이 명확하고 재사용 가능합니다
이전 피드백을 잘 반영해서 서비스 레이어에 적합한 테스트만 남겨두신 것 같습니다!
src/main/java/com/example/solidconnection/mentor/controller/MentoringController.java (1)
27-83: 전반적인 컨트롤러 구조가 우수합니다! 🚀
- 적절한 권한 제어:
@RequireRoleAccess를 통한 역할별 접근 제어가 잘 되어 있습니다- 일관된 응답 형태: 모든 엔드포인트에서
ResponseEntity사용- 유효성 검증:
@Valid어노테이션 적절히 활용- REST 원칙 준수: HTTP 메서드와 경로 매핑이 대체로 적절합니다
- 의존성 주입:
@RequiredArgsConstructor를 활용한 깔끔한 의존성 관리이전 리뷰 피드백을 반영해서 응답 객체를 적절히 감싸서 반환하고 있습니다!
src/main/java/com/example/solidconnection/mentor/dto/MentoringResponse.java
Outdated
Show resolved
Hide resolved
| if (mentoringConfirmRequest.status() == VerifyStatus.REJECTED | ||
| && (mentoringConfirmRequest.rejectedReason() == null || mentoringConfirmRequest.rejectedReason().isBlank())) { | ||
| throw new CustomException(REJECTED_REASON_REQUIRED); | ||
| } |
There was a problem hiding this comment.
다른 곳도 고쳐야하니 새 pr에서 해도 좋은 거 같아요 전!
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
Outdated
Show resolved
Hide resolved
- 주석의 내용을 잘 대변하도록 변경
- 파라미터 3개인 경우 개행하지 않는다 - 적절한 이름으로 컨트롤러 메서드명 수정
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
반영된 부분 확인 했습니다! 수고하셨어요~ 😊
| private void validateMentoringPermission(Mentoring mentoring) { | ||
| if (mentoring.getVerifyStatus() != VerifyStatus.PENDING) { | ||
| throw new CustomException(MENTORING_ALREADY_CONFIRMED); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 함수명을 그대로 번역하면 "멘토링 권한 확인"인데요,
이 함수가 하는 것은 "이미 승인되지 않았는지 검증"하는 것입니다.
이 경우, 함수의 이름이 함수가 하는 동작을 잘 서술하고 있지 않은 것 같아요..!
validateMentoringNotConfirmed 같은 직관적인 함수명은 어떤가요?
There was a problem hiding this comment.
제안하신 이름으로 수정했습니다 ! 함수명 짓는 건 언제나 어려운 것 같습니다 ㅠㅠㅋㅋㅋ
| mentoringCommandService.confirmMentoring(mentorUser1.getId(), mentoring.getId(), request)) | ||
| .isInstanceOf(CustomException.class) | ||
| .hasMessage(REJECTED_REASON_REQUIRED.getMessage()); | ||
| } |
There was a problem hiding this comment.
다른 테스트 코드들이 다 개행하지 않은 식으로 되어있어서요 🥹
아니면 오늘 개발 회의에서 다른 분들의 의견도 들어보죠!
- 적절한 이름으로 컨트롤러 메서드명 수정
- 불필요한 개행 제거
4 participants
관련 이슈
작업 내용
멘토링 관련 로직을 작성하였습니다.
도메인 특성 상Mentor와 엮이는데, 병합 시 충돌 해결하면 될 듯 합니다. 이전 PR이 병합되면 rebase해서 마저 작업 진행하겠습니다.특이 사항
@RequireRoleAccess(roles = {Role.ADMIN, Role.MENTOR})코드 추가하겠습니다. 관련 내용 TODO로 표시했습니다.checkedAt값을 수락/거절 시점으로 결정하였습니다.리뷰 요구사항 (선택)