Open
Conversation
added 3 commits
July 27, 2025 18:26
- 수정 필요
- 테스트 코드 수정 필요
- 테스트 코드 수정 - Approve 비지니스 수정
xjvmdutl
requested review from
ekfrehd,
jisubaek and
mandykr
and removed request for
a team
mandykr
reviewed
Aug 2, 2025
Comment on lines
46
to
+61
| public Page<InvitationsResponse> getTripInvitations( | ||
| UUID tripId, UUID leaderId, String status, Pageable page, TripInvitationOrder order, String sort) { | ||
| invitationPolicy.canViewInvitations(findTripWithParticipants(tripId), leaderId); | ||
| UUID tripId, | ||
| UUID leaderId, | ||
| String status, | ||
| Pageable page, | ||
| TripInvitationOrder order, | ||
| String sort) { | ||
| participantService.requireLeaderOrElseThrow(tripId, leaderId); | ||
| Pageable pageable = PaginationUtils.createPageRequest(page, order.getField(), sort); | ||
| Page<Invitation> tripInvitations = | ||
| invitationRepository.findByTripIdAndStatus(tripId, InvitationStatus.valueOf(status), pageable); | ||
| invitationRepository.findByTripIdAndStatus( | ||
| tripId, InvitationStatus.valueOf(status), pageable); |
Contributor
There was a problem hiding this comment.
서비스간의 참조는 순환 참조 위험이 있어서 제한하는게 좋을것 같아요.
반드시 호출이 필요한 상황인지 먼저 확인하고 필요하다면 같은 서비스 내에 별도로 생성하는건 어떨까요?
Comment on lines
+88
to
+105
| @Override | ||
| public Participant delegateLeader(UUID tripId, UUID currentLeaderId, UUID newLeaderId) { | ||
|
|
||
| Participant oldLeader = | ||
| participantQueryRepository | ||
| .findByTripIdAndMemberId(tripId, currentLeaderId) | ||
| .orElseThrow(() -> new NotParticipantException("현재 리더를 찾을 수 없습니다.")); | ||
| Participant newLeader = | ||
| participantQueryRepository | ||
| .findByTripIdAndMemberId(tripId, newLeaderId) | ||
| .orElseThrow( | ||
| () -> | ||
| new NotParticipantException( | ||
| "새로운 리더가 될 멤버가 여행에 참여하고 있지 않습니다.")); | ||
| participantPolicy.validateDelegate(oldLeader, newLeader); | ||
| participantPolicy.changeLeader(oldLeader, newLeader); | ||
| return newLeader; | ||
| } |
Contributor
There was a problem hiding this comment.
리더 위임도 TripService에서 호출하고 있네요. controller 부터 ParticipantService를 호출하는게 올바르게 분리된것 같아요. 전체적으로 설계에 대해 같이 논의해볼 필요가 있을것 같네요.
participantPolicy.changeLeader(oldLeader, newLeader);
이부분은 Participants의 책임인것 같아요
mandykr
reviewed
Aug 2, 2025
Contributor
mandykr
left a comment
mandykr
left a comment
There was a problem hiding this comment.
애그리거트 분리와 Policy 도메인 서비스 활용이 잘 된것 같아요.
다만 서비스간의 참조와 일급 컬렉션 활용만 참고해주시면 좋을것 같습니다.
Comment on lines
+114
to
+123
| @Override | ||
| public List<Participant> banMembers(UUID tripId, UUID loginMemberId, List<UUID> memberIds) { | ||
| requireLeaderOrElseThrow(tripId, loginMemberId); | ||
| List<Participant> participants = participantRepository.findByTripId(tripId); | ||
| participantPolicy.validateBan(participants, memberIds); | ||
| List<Participant> participantsToBan = | ||
| participants.stream().filter(m -> memberIds.contains(m.getMemberId())).toList(); | ||
| participantsToBan.forEach(Participant::ban); | ||
| return participantsToBan; | ||
| } |
Contributor
There was a problem hiding this comment.
List participants 리스트를 다루는 부분은 Participants 일급컬렉션을 활용하는것도 좋을것 같아요
pparkjs
approved these changes
Aug 3, 2025
Contributor
pparkjs
left a comment
pparkjs
left a comment
There was a problem hiding this comment.
분리하느라 고생하셨습니다!!
TripService에서 ParticipantService를 호출하는 문제는 제가 답변 적은 부분에서 한 번 고려해보시는 것도 좋을 거 같아요!
Comment on lines
+129
to
+135
| @Override | ||
| public void banMembers(UUID loginMemberId, UUID tripId, List<UUID> memberIds) { | ||
| Trip trip = findTrip(tripId); | ||
| trip.canBan(); | ||
| List<Participant> participants = | ||
| participantService.banMembers(tripId, loginMemberId, memberIds); | ||
| } |
Contributor
There was a problem hiding this comment.
홍석님의 비밀번호 관련 PR을 보면 참가자 애그리거트 분리를 대비하여 참가자 controller를 만드셔서 참가자 추방 API는 참가자 Controller에 뺴면 tripService에서 participantService를 호출 하여 순환참조에 위험이 있을 수 있는 문제를 해결 할 수 있을 거 같습니다!
Comment on lines
+78
to
+95
| @Override | ||
| public TripDemandApproveResponse approve( | ||
| UUID memberId, UUID approverMemberId, UUID tripId, UUID tripDemandId) { | ||
| TripDemand tripDemand = findTripDemandByTripIdAndTripDemandId(tripId, tripDemandId); | ||
| participantService.requireLeaderOrElseThrow(tripId, memberId); | ||
| tripDemand.approve(); | ||
| participantService.createParticipant( | ||
| tripId, approverMemberId, tripDemand.getTrip().getMaxParticipants()); | ||
| return TripDemandApproveResponse.of(tripDemand); | ||
| } | ||
|
|
||
| @Override | ||
| public TripDemandRejectResponse reject(UUID memberId, UUID tripId, UUID joinRequestId) { | ||
| TripDemand tripDemand = findTripDemandByTripIdAndTripDemandId(tripId, joinRequestId); | ||
| participantService.requireLeaderOrElseThrow(tripId, memberId); | ||
| tripDemand.reject(); | ||
| return TripDemandRejectResponse.of(tripDemand); | ||
| } |
Contributor
There was a problem hiding this comment.
해당 API도 참가자 controller로 이동시키면 위와 같은 문제 해결 될 거 같습니다!
Comment on lines
+51
to
+56
| public Participant createParticipant(UUID tripId, UUID memberId, int maxParticipants) { | ||
| Long currentCount = participantQueryRepository.findByTripIdCount(memberId); | ||
| participantPolicy.validate(maxParticipants, currentCount + 1); | ||
| return participantRepository.save( | ||
| Participant.create(tripId, memberId, ParticipantRole.PARTICIPANT)); | ||
| } |
Contributor
There was a problem hiding this comment.
참가자 승인을 동시에 눌렀을떄 인원이 넘어도 저장되는 문제 상관없겠죠?? 리더는 어차피 한명이니 승인을 동시에 누를일이 없으니
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
4 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.
🔍 PR 타입 선택
아래 타입 중 해당하는 하나를 선택해 주세요. 반드시 하나만 선택해 주세요.
feat: 새로운 기능 추가fix: 버그 수정refactor: 코드 리팩토링test: 테스트 코드 추가 또는 수정📝 변경 사항 요약
변경 사항을 간단히 요약해 주세요.
🛠 관련 이슈
Resolves: #53
Ref: #48
추가 설명 (선택 사항)