-
Notifications
You must be signed in to change notification settings - Fork 4
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
채팅 웹소켓 구현 #771
채팅 웹소켓 구현 #771
Conversation
- WebSocketHandleTextMessageProviderCompositeTest
sender -> writer
handle -> handleCreateSendMessage
SendMessagesDto -> SendMessageDto SendMessageDto -> MessageDto
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.
메리 리팩터링 및 추가 기능들 구현하시느라 고생 많으셨습니다!
수정할 부분이 많지는 않지만 간단한 컨벤션과 질문이 있어 rc로 처리했습니다.
또한, 저희 앞으로 소켓 통신이 제대로 됐는지 문제 상황은 없는지 쉽게 확인하기 위해 지난 회의에서 사용한 로그들(세션 연결, 세션 해제, 전송 메시지 내용 등등)을 추가하는 것이 좋을지 궁금합니다!
final Long readerId = updateReadMessageLogEvent.readerId(); | ||
final Long chatRoomId = updateReadMessageLogEvent.chatRoomId(); | ||
final ReadMessageLog messageLog = readMessageLogRepository.findBy(readerId, chatRoomId) |
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.
칭찬
오! 어차피 EventListener에서 새로운 트랜잭션으로 시작하기에 다시 조회 쿼리를 날려야 해서 id만 받아와도 무관하겠군요!
꼼꼼한 리팩터링 최고 👍
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.
질문 & 선택
44 라인에 리뷰를 달 수 없어 여기에 남깁니다.
44 라인에서 updateReadMessageLogEvent.lastReadMessage().getId()
도 그냥 마지막 메시지의 아이디를 넘겨주면 되지 않을까요?
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.
그러네요! 감사해요 수정했습니다!
@@ -45,19 +44,12 @@ public Message create(final CreateMessageDto dto) { | |||
.orElseThrow(() -> new UserNotFoundException( | |||
"지정한 아이디에 대한 수신자를 찾을 수 없습니다." | |||
)); | |||
validateReceiver(chatRoom, 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.
칭찬
👍👍👍
final UpdateReadMessageLogEvent updateReadMessageLogEvent = new UpdateReadMessageLogEvent( | ||
reader.getId(), | ||
chatRoom.getId(), | ||
readMessages.get(readMessages.size() - 1) |
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.
선택
1
은 상수로 처리하지 않아도 될까요?
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.
현재 몇몇 값들은 매직 넘버처리를 하지 않았기에 해야 할지 고민이어 질문드렸습니다!
메리가 필요 없다 느끼시면 할 필요 없습니다!
@@ -84,17 +73,31 @@ private CreateMessageDto createMessageDto(final ChatMessageDataDto messageData, | |||
return CreateMessageDto.of(userId, messageData.chatRoomId(), request); | |||
} | |||
|
|||
private void sendNotificationIfReceiverNotInSession( |
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.
칭찬
메서드 호출 순서에 따라 변경 감사합니다!
import static org.mockito.BDDMockito.given; | ||
import static org.mockito.BDDMockito.willDoNothing; | ||
import static org.mockito.BDDMockito.willReturn; | ||
import static org.mockito.BDDMockito.*; |
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.
필수
* 이 들어가있네요!
@Test | ||
void 메시지_생성시_수신자가_웹소켓_통신_중이라면_메시지_로그_업데이트_메서드를_호출한다() throws JsonProcessingException { | ||
// given | ||
메시지_로그를_생성한다(); |
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.
이벤트가 잘 호출되었는지 확인하는 테스트입니다.
테스트코드에서는 이벤트가 실제로 동작하지 않아서 호출되었는지 여부를 체크하고,
이벤트 동작 테스트는 이벤트 리스너 메서드 테스트를 통해 테스트를 진행했습니다.
해당 테스트는 메시지를 전송한 경우 수신자, 발신자의 메시지 로그를 업데이트 해야하니 두 번의 호출이 발생하는지 확인한 테스트입니다!
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.
그런데 궁금한 점은 lastReadMessageLogService.create
시 꼭 메서드로 해줘야 하는 이유가 있는 걸까요?
필요한 경우만 호출하기 위함인 걸까요?
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.
필요한 경우에만 호출하기 위함이 맞습니다~!
@Autowired | ||
private ReadMessageLogRepository readMessageLogRepository; |
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.
확인 완료했습니다!
고생 많으셨어요 메리~
아래 질문만 확인해 주시면 감사하겠습니다!
@Test | ||
void 메시지_생성시_수신자가_웹소켓_통신_중이라면_메시지_로그_업데이트_메서드를_호출한다() throws JsonProcessingException { | ||
// given | ||
메시지_로그를_생성한다(); |
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.
그런데 궁금한 점은 lastReadMessageLogService.create
시 꼭 메서드로 해줘야 하는 이유가 있는 걸까요?
필요한 경우만 호출하기 위함인 걸까요?
📄 작업 내용 요약
제이미가 올린 PR에 이어서 작업한 내용입니다.
이번에도 안드로이드 분들 커밋 내역이 함께 푸시 되어서 기존 PR은 클로즈하고 다시 생성했습니다.
소켓이 연결되어 있는 상태에서 메시지 전송 시 메시지 읽음 처리 구현
afterConnectionClosed()
에서 읽음처리를 해주면 채팅방에 접속해있지 않더라도 메시지가 읽음처리 됩니다. 그래서 불가피하게 메시지를 전송할 때마다 메시지 읽음처리를 해주도록 구현했습니다.afterConnectionClosed()
이 앱을 종료하는 시점에 호출된다는 점에서 앱에 접속해 있지만 채팅방에 접속해있지 않은 경우 알림은 전송되지 않을 것 같습니다. 이 부분은 회의에서 안드로이드 분들과 커넥션 종료 시점을 확인해본 이후에 수정해야 할 것 같습니다.엔초 리뷰 반영해서 지난번 메시지 발신자에게만 메시지가 두 번 전송되는 오류 해결했습니다.
채팅 전송 시 수신자가 탈퇴한 유저인 경우 예외를 발생시키는 검증 로직을 삭제했습니다.
🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드
📎 Issue 번호