-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feat] 커피챗 수신 API 문자 발송 기능 추가 #487
Conversation
|
Hi there 👋 Using this App for a private organization repository requires a paid subscription. You can click If you are a non-profit organization or otherwise can not pay for such a plan, contact me by creating an issue |
- requestBody에서 senderEmail이 누락되더라도 이메일 발송이 정상 동작하도록 수정 - 각 분기에서 호출하는 함수 변경 (fix) related issue #483
related issue #483
- private 생성자 선언과 중복 related issue #483
- 커피챗 수신 API -> 커피챗/쪽지 수신 API related issue #483
…und-backend into feat/send_coffee_chat_message
Long receiverId, | ||
String senderEmail, | ||
@JsonProperty(required = false) String senderEmail, | ||
@JsonProperty(required = false) String senderPhone, | ||
String category, | ||
String content |
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.
각 데이터들에 대한 Validation도 추가되면 좋을 것 같습니다!
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.
1956902에서 반영했습니다
@Parameter(hidden = true) @AuthenticationPrincipal InternalMemberDetails memberDetails | ||
) { | ||
coffeeChatService.sendCoffeeChatRequest(request, memberDetails.getId()); | ||
val response = new CommonResponse(true, "커피챗/쪽지 수신 요청에 성공했습니다."); |
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.
val 사용에 대해서 한 번 정하고 가는 게 좋을 것 같습니다! 제 기억으로는 리팩토링 하면서 val 사용을 지양하기로 했던 것 같은데 혹시 제가 잘못기억하고 있거나, 예준님 의견이 있다면 편하게 말씀해주시면 좋을 것 같습니다!
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.
아하 이거 PR 본문의 agenda로 적어두었는데 이전에 이미 논의된 내용이었군요!!
저도 동의합니다. 앞으로 val 사용 지양하는 걸로 통일하겠습니다 👍
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.
1561ce1에서 수정했습니다!
public List<String> concatPartAndGeneration(Long memberId) { | ||
return memberSoptActivityRepository.findAllByMemberId(memberId).stream() | ||
.map(activity -> String.format("%d기 %s", activity.getGeneration(), activity.getPart())) | ||
.toList(); | ||
} |
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.
그럴 일은 없겠지만 findAllByMemberId로 조회를 했을 때 YB인데 length가 1보다 크거나 OB인데 기수의 길이가 2보다 작은 경우를 에러로 던지는 로직이 있으면 좋다고 생각합니다. 저희가 아직은 수동으로 기수정보를 처리하다보니 혹시나 생길 수 있는 휴먼에러에 대응하기 위한 것 인데요 혹시 공수가 조금 크다면 //TODO 주석을 남겨놓는 것도 방법인 것 같습니다.
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.
넵! 이건 TODO로 남겨두고 이슈 생성해두겠습니다 ~~ 좋은 의견 감사합니당
val receiver = memberRetriever.findMemberById(request.receiverId()); | ||
val sender = memberRetriever.findMemberById(senderId); |
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.
val 관련 부분은 위의 코멘트와 동일합니다.
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.
1561ce1에서 수정했습니다!
if (request.category().equals(ChatCategory.COFFEE_CHAT.getValue())) { | ||
sendCoffeeChatMessage(request, sender, receiver); | ||
} else { | ||
sendChatEmail(request, sender, receiver); | ||
} | ||
} |
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.
ChatCategory에 따라서 문자를 발송할지 이메일을 발송할지 구분하는 부분은 커피챗 신청에 따른 분기처리라고 생각됩니다.
따라서 서비스 계층에 직접적으로 드러나있기보다는 "커피챗 request가 들어오면 receiver에게 무언가(이메일, 문자)를 보낸다!" 라는 것만 서비스계층을 알고 있으면 된다고 생각합니다. 실제로 어떻게 보내는지는 서비스 계층이 궁금한 부분은 아니라고 생각해요
만약 나중에 email이나 문자가 아닌 다른 방식으로 메세지를 전달해야한다면 코드 변경 및 전송로직을 변경을 해야할 것 같아요
따라서 ChatCategory에 따라 사용자에게 정보를 전달해주는 객체가 따로 있으면 좋다고 생각합니다.
추가적으로 현재는 request에서 category를 문자열로 받아서 처리하고 있는데 ENUM을 사용하는 만큼 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.
따라서 ChatCategory에 따라 사용자에게 정보를 전달해주는 객체가 따로 있으면 좋다고 생각합니다.
너무 좋은 피드백인 것 같습니다 👍 ServiceFactory에서 추상화하도록 리팩토링하였고, strategy 패턴을 이용해 말씀 주신 방향대로 변경에도 용이한 구조로 개선했습니다!
추가적으로 현재는 request에서 category를 문자열로 받아서 처리하고 있는데 ENUM을 사용하는 만큼 enum으로 받아서 확인하는 로직을 조금 더 간단하게 할 수 있을 것 같습니다!
좋은 의견 감사합니다!! 38820f1에서 반영 후 enum validation 적용했습니다
emailHistoryRepository.save(EmailHistory.builder() | ||
.senderId(senderId) | ||
.receiverId(request.receiverId()) | ||
.senderEmail(request.senderEmail()) | ||
.category(request.category()) | ||
.content(request.content()) | ||
.createdAt(LocalDateTime.now(KST)).build()); | ||
.senderId(sender.getId()) | ||
.receiverId(request.receiverId()) | ||
.senderEmail(email) | ||
.category(request.category()) | ||
.content(request.content()) | ||
.createdAt(LocalDateTime.now(KST)).build()); |
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.
넵! 4b96180 에서 반영 및 리팩토링 진행했습니다
Agenda 1. |
- sendChatEmail, sendCoffeeChatSMS 와 history 테이블에 insert 하는 메서드 분리 - 메시지 발송 시, receiver의 phone으로 넘기도록 수정 (fix) - phone, email requestBody에 없는 경우 default 값 지정하는 로직 별도 메서드로 분리 related issue #483
- RequestBody에서 ChatCategory 타입을 사용하도록 변경 - ChatCategory에 정의된 값들 중에 존재하는지 검사하는 validation 추가 related issue #483
- EmailSender package 변경 : domain/ -> external/email/ - EmailHistoryService 분리 - MessageSenderFactory를 통해 Email / SMS send 로직 추상화 related issue #483
- self-qa 중 발견된 버그 수정 related issue #483
- 동일한 if-else 구문 제거 related issue #483
- TODO comment 추가 related issue #483
@dev-Crayon 의견 주셔서 감사합니다!! 코드리뷰 반영 완료해서 피드백 마구마구 주시면 감사하겠습니다아 ㅎㅎ |
- swagger required 여부 반영 related issue #483
@Pattern(regexp = PHONE_NUMBER_REGEX, message = "잘못된 전화번호 형식입니다. '-'을 제외한 11자의 번호를 입력해주세요.") | ||
String senderPhone, | ||
|
||
ChatCategory category, |
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.
ChatCategory 필드는 required = false 인가요? 그렇지 않다면 @NotNull을 사용해도 좋을 것 같습니다.
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.
마지막 코멘트만 확인해주시고 merge해도 될 것 같습니다! 고생하셨습니다 |
- swagger required 여부 반영 related issue #483
아직 문자 메시지 본문 픽스가 안 나서 고거까지 반영 후에 머지하겠습니다 ㅎㅎ |
related issue #483
…und-backend into feat/send_coffee_chat_message
🐬 요약
기존 커피챗 수신 API를 리팩토링 하고, 문자 발송 기능을 추가하여 쪽지 보내기와 커피챗 신청하기를 분리하여 처리합니다.
👻 유형
PR의 유형에 맞게 체크해주세요!
🍀 작업 내용
AS-IS
email_history
에서 쪽지 보내기 시 요청한 데이터를 저장합니다.TO-BE
email_history
테이블이 아닌,coffee_chat_history
테이블에 적재하도록 신규 테이블을 추가합니다. (추후 별점 기능 등 커피챗 기능 고도화에 대한 확장성 고려)Test
쪽지 보내기
senderEmail 필드 없이 request 를 보내도 default email로 정상 처리 (
email_history
테이블에 row 추가)커피챗 신청하기
senderPhone 필드 없이 request 를 보내도 default phone으로 정상 처리 (
coffee_chat_history
테이블에 row 추가)Action Item
dev에서 coffee_chat_history2 테이블 생성하도록 ddl 실행 테스트 완료했습니다.
Agenda
coffee_chat_history
테이블에 senderPhone 정보도 저장해야 할까요?🌟 관련 이슈
PR과 관련된 이슈 번호를 작성해주세요!
close: #483