Conversation
Test Results 21 files 21 suites 6s ⏱️ Results for commit 30f42e0. ♻️ This comment has been updated with latest results. |
| log.info("success to reject join request group = {} and delete request = {} ", groupId, requestId); | ||
|
|
||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Approve와 reject를 하나로 묶으려면 isApproved 필드를 하나 더 받아서 처리하면 되지 않을까 싶네요! reject를 만들지 말지 프론트랑 결정해서 하면 될거같아요
| public JoinRequestException(String error) { | ||
| super(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
황조가 채널톡에도 남긴게 있는거긴한데,, 에러메세지만 받고 httpstatus코드는 핸들러에서 처리하는 방식과, 둘 다 받는 경우 이 두가지 경우가 혼동되서 사용되는게 많은 거 같아서요!! 이건 서버분들이랑 같이 얘기해봐야할거같아요
There was a problem hiding this comment.
그렇네요 ,,, 논의가 필요할 거 같아요
junggyo1020
left a comment
There was a problem hiding this comment.
고민을 쭉 정리해주셔서 코드리뷰하기 수월했던 것 같네요..! 감사합니다 ㅎㅎ
고민 1. delete 하는게 맞을 지 .. 하지 않는다면 왜..?
제가 로직을 봤을때는 Hard Delete를 하고 있는 것으로 보이는데요! 저는 상태값 변경이 더 좋을 것 같다는 생각입니다 :)
만약에 사용자가 가입 신청을 했는데, 어떻게 처리되었는지 궁금할 수도 있을 것 같아요! "거절됨", "승인됨" 과 같이 상태값으로 사용자에게 상태를 보여주는 것이 좋다고 생각했는데요, 다른 분들의 의견도 궁금하네요 ㅎㅎ
고민 2. approve 랑 reject 코드가 너무 너무 유사합니다 한 번의 엔드포인트로 끝낼 수 있을지?
제가 로직을 확인했을때는 JoinRequestStatus의 상태값에 따라서 하나의 메서드로 분기할 수 있을 것처럼 보이는데, 하나의 엔드포인트로 적용할 때, 고민하시는 부분이 있다면 들어보고 싶습니다 :)
기능 구현하시느라 고생하셨습니다!!
| public void approveJoinRequest(User user, Long requestId, Long groupId) { | ||
| JoinRequest joinRequest = joinRequestRepository.findById(requestId) | ||
| .orElseThrow(() -> new JoinRequestException("해당 요청이 존재하지 않습니다")); |
There was a problem hiding this comment.
approveJoinRequest와 rejectJoinRequest에 있는 권한 검증 로직이 동일해서 나중에 API를 통합하지 않더라도, 이 부분은 private 메소드로 분리하는 것이 좋아보입니다!
rladmstn
left a comment
There was a problem hiding this comment.
고생하셨습니다!!
코멘트로 질문주신 부분들에 대해 중간중간 답변 남겨두었습니다.
그리고 요청에 대한 거절은 필요하긴 할 것 같아요! 일반적인 참여 요청이라면 승인/거절 두 상태가 있으니까요!
| @PostMapping(value = "/{groupId}/{requestId}/approve") | ||
| @Operation(summary = "그룹 가입 요청 승인 API", description = "스터디 그룹 가입 요청을 승인하는 API") | ||
| public ResponseEntity<Void> approveRequest(@AuthedUser User user, @PathVariable Long requestId, | ||
| @PathVariable Long groupId) { | ||
| studyGroupService.approveJoinRequest(user, requestId, groupId); | ||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| @PostMapping(value = "/{groupId}/{requestId}/reject") | ||
| @Operation(summary = "그룹 가입 요청 거절 API", description = "스터디 그룹 가입 요청을 거절하는 API") | ||
| public ResponseEntity<Void> rejectRequest(@AuthedUser User user, @PathVariable Long requestId, | ||
| @PathVariable Long groupId) { | ||
| studyGroupService.rejectJoinRequest(user, requestId, groupId); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
차라리 PATCH /{groupId}/{requestId}/status 로 해서 requestBody에 status를 받는건 어떨까요??
status를 approve 또는 reject로 받아서 분기처리하면 더 편리할 것 같아요!
그리고 혹시 스터디장에게 지금까지 처리했던 요청 목록 및 승인/거절 내역을 모두 보여주는 기능이 있나요? 그게 아니라면 사실 승인/삭제 여부 상관없이 삭제해도 괜찮을 것 같다는 생각이 들어서요! 위에 있는 그룹 가입 요청 목록 조회가 이전 내역도 모두 보여주는거라면 남겨둬야겠지만요..!
There was a problem hiding this comment.
그리고 혹시 스터디장에게 지금까지 처리했던 요청 목록 및 승인/거절 내역을 모두 보여주는 기능이 있나요? 그게 아니라면 사실 승인/삭제 여부 상관없이 삭제해도 괜찮을 것 같다는 생각이 들어서요! 위에 있는 그룹 가입 요청 목록 조회가 이전 내역도 모두 보여주는거라면 남겨둬야겠지만요..!
스터디장이 내역을 보는 기능은 없습니다.. 정교님은 유저가 상태값을 보여줄 수 있게 상태값 변경 하자는 의견을 주셨는데
지금은 없더라도 나중에 생길 기능을 위해 상태값 변경도 좋을 것 같은데 언제 생길지 모르는 기능을 위해 ..? 라는 생각도 들긴하네요 ...
There was a problem hiding this comment.
저도 남겨두는게 좋다고 생각합니다~!
상태값 변경으로 남겨두는 방향이 좋을 것 같아요!
| import lombok.Setter; | ||
|
|
||
| @Entity | ||
| @Setter |
There was a problem hiding this comment.
Setter 를 아무생각 없이 썼는데 지양해야 하는 이유를 알려주세요..!
There was a problem hiding this comment.
기본적으로 불변성 보장도 어렵고, 의도치않게 필드 값을 수정하게 될 위험성도 늘어납니다!
원래는 필요한 부분에 대한 수정 메서드를 직접 제공해서 변경되면 안되는 값들에 대한 수정은 막을 수 있는데, setter를 사용해버리면 모든 값이 변경 가능하게 됩니다!
뿐만 아니라 setXXX()를 사용하면 해당 메서드가 상태 변경을 위한 의도인지, 객체 생성을 위한 의도인지 파악하기도 어렵구요!
setter를 지양하는 이유는 구글에 널렸으니.. 찾아보시는걸 추천드립니다
| public void approve() { | ||
| if (status != JoinRequestStatus.PENDING) | ||
| return; | ||
| status = JoinRequestStatus.APPROVE; | ||
| } | ||
|
|
||
| public void reject() { | ||
| if (status != JoinRequestStatus.PENDING) | ||
| return; | ||
| status = JoinRequestStatus.REJECT; | ||
| } | ||
|
|
||
| public void cancel() { | ||
| if (status != JoinRequestStatus.PENDING) | ||
| return; | ||
| status = JoinRequestStatus.CANCEL; | ||
| } |
There was a problem hiding this comment.
얘네도 updateStatus?같은 메서드 하나로 관리할 수 있을 것 같아요!
| public record JoinRequestResponse() { | ||
| } |
There was a problem hiding this comment.
여기는 원래 뭐가 들어가려고 했던 객체인가요??
| @Transactional | ||
| public void joinRequest(User user, Long groupId) { | ||
| StudyGroup studyGroup = groupRepository.findById(groupId) | ||
| .orElseThrow(() -> new StudyGroupValidationException(HttpStatus.NOT_FOUND.value(), "존재하지 않는 그룹 입니다.")); | ||
| if (groupMemberRepository.existsByUserAndStudyGroup(user, studyGroup)) { | ||
| throw new GroupMemberValidationException(HttpStatus.BAD_REQUEST.value(), "이미 가입한 그룹입니다"); | ||
| } | ||
| if (joinRequestRepository.existsByGroup_IdAndRequester_Id(groupId, user.getId())) { | ||
| throw new JoinRequestException("이미 요청한 그룹입니다."); | ||
| } | ||
|
|
||
| JoinRequest request = new JoinRequest(); | ||
| request.setRequester(user); | ||
| request.setGroup(studyGroup); | ||
| joinRequestRepository.save(request); | ||
| log.info("success to join request group = {}", groupId); | ||
| } |
There was a problem hiding this comment.
JoinRequest 관련 메서드부터는 JoinRequestService 같이 분리하는건 어떻게 생각하시나요??
지금 StudyGroupService가 너무 커지고 있어서..! 이제부터 스터디그룹에 관련된 메서드들은 좀 더 잘게 쪼개서 Service를 생성하는게 낫지 않을까 싶어서요..!
There was a problem hiding this comment.
작업하면서 이 생각이 들긴 했습니다 .. 쪼개보도록 하겠습니다.
|
|
||
| public interface JoinRequestRepository extends JpaRepository<JoinRequest, Long> { | ||
|
|
||
| boolean existsByGroup_IdAndRequester_Id(Long groupId, Long userId); |
There was a problem hiding this comment.
연관관계 매핑 사용해서 StudyGroup group, User requester로도 찾을 수 있을 것 같은데 groupId와 requesterId로 찾는 이유가 무엇인가요??
There was a problem hiding this comment.
음 ,,, 우선 스터디 그룹 검색을 했을 때 groupId를 달라고 했었습니다 그래서 groupId를 사용했습니다 userId는 쓰는김에 쓴 거 같네요,,,
There was a problem hiding this comment.
저 메서드를 사용하는 service 내에서 이미 User 객체랑 StudyGroup 엔티티를 이미 불러와둔 것 같아서요!
service 보니까 가져온 엔티티에서 다시 한 번 getId()로 넘겨주는 것 같던데,
연관관계 사용해서 User랑 StudyGroup 넘기도록 만드는건 어떨까요?
|
|
||
| boolean existsByGroup_IdAndRequester_Id(Long groupId, Long userId); | ||
|
|
||
| List<JoinRequest> findAllByGroup_Id(Long groupId); |
| JoinRequest request = new JoinRequest(); | ||
| request.setRequester(user); | ||
| request.setGroup(studyGroup); |
|
피드백 반영해서 다시 올렸는데요,, 일단 soft delete 는 아직 스펙이 정해지지 않은 것 같아 적용하지 않았습니다..! |
rladmstn
left a comment
There was a problem hiding this comment.
찐막 리뷰!
고려해보면 좋을 것 같아서요..!
누락된 부분도 있습니다!
There was a problem hiding this comment.
개인적으로 JoinRequestController가 따로 쪼개져도 괜찮을 것 같다고 생각합니다!
사유는.. service 분리 요청 이유와 같습니다.. 너무 커지는 이유..
| public ResponseEntity<Void> rejectRequest(@AuthedUser User user, @PathVariable Long requestId, | ||
| @PathVariable Long groupId) { | ||
| studyGroupService.rejectJoinRequest(user, requestId, groupId); | ||
| @PostMapping(value = "/groups/{groupId}/join-request/{requestId}") |
There was a problem hiding this comment.
이제보니 groupId는 받을 필요 없을 것 같아요.. ㅎㅎ JoinRequest에 이미 group이 담겨있어서..!
| joinRequest.updateStatus(request.status()); | ||
| GroupMember newGroupMember = GroupMember.builder() | ||
| .user(joinRequest.getRequester()) | ||
| .studyGroup(studyGroup) | ||
| .joinDate(LocalDate.now()) | ||
| .role(RoleOfGroupMember.PARTICIPANT) | ||
| .build(); | ||
| groupMemberRepository.save(newGroupMember); | ||
| joinRequestRepository.delete(joinRequest); |
There was a problem hiding this comment.
가입 후 처리에 누락된 부분이 있습니다!
가입 처리 되면 NotificationSetting과 Ranking도 함께 데이터가 insert 되어야 합니다.
그룹 참여 API 메서드 내용 참고하고 추가해주면 좋을 것 같습니다!
| return ResponseEntity.ok().body(response); | ||
| } | ||
|
|
||
| @PostMapping(value = "/groups/join-request/{requestId}") |
There was a problem hiding this comment.
레알레알 찐막. 이거 그냥 /join-request/{requestId} 어떠심 ㅎㅎ
There was a problem hiding this comment.
/groups 떼자는 말이신거죠? 흐음~~~ 싫어요! 이대로 할래요!
There was a problem hiding this comment.
납득 시킬 자신 없어서 바꿨읍니다..
rladmstn
left a comment
There was a problem hiding this comment.
진짜진짜 수고하셨슴니다!
endpoint 관련해서 코멘트 하나만 확인하고 머지하시죠!
| notificationSettingRepository.save( | ||
| NotificationSetting.builder().member(newGroupMember).build() | ||
| ); | ||
|
|
||
| rankingRepository.save(Ranking.builder() | ||
| .member(newGroupMember) | ||
| .currentRank(groupMemberRepository.countByStudyGroup(studyGroup)) | ||
| .solvedCount(0) | ||
| .rankDiff("-") | ||
| .build() | ||
| ); | ||
| studyGroupService.sendNewMemberNotification(studyGroup, newGroupMember); |
There was a problem hiding this comment.
원래 이런 main 작업 외에 부수적인 작업들은 event 발행해서 처리하는게 더 이상적인 것 같긴 합니다!
지금 적용하긴 늦었으니 그냥 알아두시면 좋을 듯!

📌 Related Issue
BE-35
🚀 Description
📢 Review Point