-
Notifications
You must be signed in to change notification settings - Fork 0
[TNT-258] feat: 마이페이지 내 정보 수정 구현 #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ymkim97
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직 로직이 완전하지 못한 것 같습니다! 좀 더 보완 부탁드려요!!!
| int totalTraineeCount = ptTrainerTrainees.size(); | ||
|
|
||
| TrainerInfo trainerInfo = new TrainerInfo(activeTraineeCount, totalTraineeCount); | ||
| GetTrainerInfo getTrainerInfo = new GetTrainerInfo(activeTraineeCount, totalTraineeCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 Dto의 네이밍에서 동사가 들어가는건 읽는 사람에게 혼란을 주는거 같아요.
Dto의 책임, 역할은 어디까지나 데이터를 보관하고 레이어간 이동? 해주는 것이기 때문입니다!
일단 생각나는 것은 TrainerInfoForGet 정도의 느낌이 있는데, 한번 더 다른 것이 있는지 생각 해보신 후 수정 부탁드립니다~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좀 더 생각해봤는데 단순 조회에는 굳이 get이 없어도 될 것 같기도 하네요...
이번 네이밍은 저도 이전 dto에 잘못 적용했어서 이전 dto들은 제가 수정하겠습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좀 더 생각해봤는데 단순 조회에는 굳이 get이 없어도 될 것 같기도 하네요... 이번 네이밍은 저도 이전 dto에 잘못 적용했어서 이전 dto들은 제가 수정하겠습니다~
넵 그럼 저도 TrainerInfo 이런 식으로 수정해볼게요 !!
| boolean isConnected = ptService.isPtTrainerTraineeExistWithTraineeId(trainee.getId()); | ||
|
|
||
| TraineeInfo traineeInfo = new TraineeInfo(isConnected, member.getBirthday(), | ||
| GetTraineeInfo getTraineeInfo = new GetTraineeInfo(isConnected, member.getBirthday(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위 코멘트와 동일합니다!
| @Transactional | ||
| public UpdateMemberInfoResponse updateMemberInfo(Long memberId, UpdateMemberInfoRequest request, | ||
| String profileImageUrl) { | ||
| Member findMember = getByMemberId(memberId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 메서드에서는 member, findMember 등으로 일관되지 않는거 같아요!
해당 메서드에서는 하나의 member만 있으니 member로 네이밍하는건 어떻게 생각하시나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 메서드에서는 member, findMember 등으로 일관되지 않는거 같아요! 해당 메서드에서는 하나의 member만 있으니
member로 네이밍하는건 어떻게 생각하시나요??
좋습니다 그런 식으로 수정할게요 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 아직 수정이 안된건가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 아직 수정이 안된건가요??
수정했습니다 !
| public UpdateMemberInfoResponse updateMemberInfo(Long memberId, UpdateMemberInfoRequest request, | ||
| String profileImageUrl) { | ||
| Member findMember = getByMemberId(memberId); | ||
| UpdateMemberInfoResponse memberInfo = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 네이밍 관련 코멘트와 동일합니다!
그런데 이미 MemberInfo에 관한 response가 있는데 업데이트 용으로 따로 만들어야 할 이유가 있을까요?
저장 후 정보 수정 페이지가 아니라 마이 페이지로 화면이 이동하면 response가 따로 필요 없다고는 생각하는데, 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 네이밍 관련 코멘트와 동일합니다!
그런데 이미 MemberInfo에 관한 response가 있는데 업데이트 용으로 따로 만들어야 할 이유가 있을까요? 저장 후 정보 수정 페이지가 아니라 마이 페이지로 화면이 이동하면 response가 따로 필요 없다고는 생각하는데, 어떻게 생각하시나요?
- 말씀하신 대로
MemberInfodto 하나만 만들어서 사용해도 좋을 것 같습니다 ! - 처음에는 여기 부분에서 리턴할 때 정보를 주면 조회 api 호출을 한 번이라도 덜 한다고 생각했는데 어차피 마페에서 조회 api 호출이 필연적으로 일어나니까 없어도 괜찮은 것 같습니다. void로 수정하겠습니다 !
| String profileImageUrl = s3Service.uploadProfileImage(profileImage, request.memberType()); | ||
|
|
||
| return memberService.updateMemberInfo(memberId, request, profileImageUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자가 회원 정보를 수정할 때 프로필 사진은 그대로 쓸 수도 있는데, 현재 코드는 그런 상황을 커버하고 있지는 않는 것 같습니다! 또한 기존에 사용하던 사진을 S3에서 삭제하는 과정도 없네요(2, 3번).
- 원래 사용하던 사진을 그대로 사용한다.
클라이언트에서 굳이 같은 사진을 다시 업로드를 할 필요가 없어 null로 올 것이고, 이를 식별해야함.
- 사용하던 사진을 내리고 기본 프사를 사용한다.
1번에서도 profileImage를 null로 보내지만 이번에도 null로 보내야함. 이것도 식별로 구분, S3에서 기존 이미지 삭제.
- 사용하던 사진을 새로운 사진으로 대체한다.
profileImage로 사진이 업로드 될 것이므로, S3에서 기존 이미지 삭제 후 해당 사진 업로드.
일단 제가 생각한 가능한 flow는 위와 같고, 제가 빼먹은 것이 있으면 추가해서 다시 전체적으로 수정 부탁드립니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자가 회원 정보를 수정할 때 프로필 사진은 그대로 쓸 수도 있는데, 현재 코드는 그런 상황을 커버하고 있지는 않는 것 같습니다! 또한 기존에 사용하던 사진을 S3에서 삭제하는 과정도 없네요(2, 3번).
- 원래 사용하던 사진을 그대로 사용한다.
클라이언트에서 굳이 같은 사진을 다시 업로드를 할 필요가 없어 null로 올 것이고, 이를 식별해야함.
- 사용하던 사진을 내리고 기본 프사를 사용한다.
1번에서도 profileImage를 null로 보내지만 이번에도 null로 보내야함. 이것도 식별로 구분, S3에서 기존 이미지 삭제.
- 사용하던 사진을 새로운 사진으로 대체한다.
profileImage로 사진이 업로드 될 것이므로, S3에서 기존 이미지 삭제 후 해당 사진 업로드.
일단 제가 생각한 가능한 flow는 위와 같고, 제가 빼먹은 것이 있으면 추가해서 다시 전체적으로 수정 부탁드립니다~
제가 너무 허술하게 짰네요 ㅠㅠ
말씀 주신 대로 기존 사진 식별 로직이랑 기본 프사 및 새로운 사진 적용 로직을 제대로 적용할게요 !!
| null); | ||
| } | ||
|
|
||
| // 트레이니 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
크게 의미 없는 주석은 삭제 해주세요~!
| public void updateTraineeInfo(Double height, Double weight, String cautionNote) { | ||
| this.height = height; | ||
| this.weight = weight; | ||
| this.cautionNote = cautionNote; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만들어져있는 validateAndSetCautionNote()를 사용하는건 어떤가요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만들어져있는
validateAndSetCautionNote()를 사용하는건 어떤가요!
넵 적용하겠습니다 !
| .orElseThrow(() -> new NotFoundException(MEMBER_NOT_FOUND)); | ||
| } | ||
|
|
||
| private List<PtGoal> updatePtGoals(Trainee trainee, List<String> newGoalContents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전부터 생각해온 것인데, ptGoal은 이렇게 따로 테이블을 두는것보단 enum으로 관리하는 것이 더 자연스럽고 편할 것 같네요. 이 부분은 클라이언트와 맞춰서 다시 변경하도록 해야할 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전부터 생각해온 것인데, ptGoal은 이렇게 따로 테이블을 두는것보단 enum으로 관리하는 것이 더 자연스럽고 편할 것 같네요. 이 부분은 클라이언트와 맞춰서 다시 변경하도록 해야할 것 같습니다!
넵 이건 enum 방향으로 다시 깊게 생각해서 다음에 얘기해볼게요 !!
| } | ||
|
|
||
| // 최종 목표 목록 리턴 (기존 유지된 목표 + 새로 추가된 목표) | ||
| List<PtGoal> result = new ArrayList<>(currentPtGoals); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
새로운 객체를 만들 필요없이 currentPtGoals에 추가 후 그대로 return 하면 되지 않을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
새로운 객체를 만들 필요없이 currentPtGoals에 추가 후 그대로 return 하면 되지 않을까요??
마지막에 리스트 새로 안 만들도록 수정하겠습니다 !
| this.content = validateContent(content); | ||
| } | ||
|
|
||
| public void softDelete() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ptGoal은 딱히 softDelete를 할 필요는 없어보이네요. 어차피 추후에 enum으로 관리하도록 변경하니 ptGoal은 모두 hard delete로 하는게 좋아보입니다!
따라서 deletedAt도 없애도 좋을 것 같아요
다시 보완하고 노티 드릴게요 ! |
|
ymkim97
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 25.05.29 라이브 리뷰 진행
고생하셨습니다!!! 👍👍👍



📋 Checklist
[APP2-77] feat: 회원 인증 Filter 구현)🎟️ Issue
✅ Tasks
프로필 사진 수정 API
회원 정보 수정 API
🙋🏻 More