-
Notifications
You must be signed in to change notification settings - Fork 3
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
[BE] feat: 채팅을 보낼 때 FCM으로 데이터를 같이 전송 #451
Conversation
Test Results151 tests +148 151 ✅ +148 17s ⏱️ +17s Results for commit 4395ff2. ± Comparison against base commit 3acce8d. This pull request removes 3 and adds 151 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
@Query(""" | ||
SELECT dt.deviceToken | ||
FROM DeviceToken dt | ||
WHERE dt.member.id IN (SELECT chatRoomMember.member.id | ||
FROM ChatRoomMember chatRoomMember | ||
WHERE chatRoomMember.chatRoom.id = :chatRoomId) | ||
""") | ||
List<String> findAllByChatRoomId(Long 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.
서브쿼리를 INNER JOIN으로 대체할 수 있겠어요.
SELECT dt.deviceToken
FROM DeviceToken dt
JOIN ChatRoomMember cm ON dt.member.id = cm.member.id
WHERE cm.chatRoom.id = :chatRoomId
JOIN을 사용하지 않고 서브 쿼리를 사용한 이유가 있을까요?
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.
서브쿼리가 단계적으로 생각하기 쉽기는 하죠
@Override | ||
public void sendChat(Long chatRoomId, ChatMessageResponse 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.
해당 메서드에서는 실제 채팅을 보내는 것이 아닌, 채팅에 대한 알람만을 보내고 있어요.
sendChatNotification()
와 같이 명시적으로 표현하면 어떨까요?
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.
굳 추가 리팩토링한다면 그 PR에서 더 상세하게 보겠습니다
@Profile("!local") | ||
public class FcmNotificationService implements NotificationService { | ||
|
||
private static final String DEFAULT_TITLE = "반갑개"; |
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.
TITLE은 디폴트로 "반갑개"가 될수가 없을 수도 있을거 같아요
TITLE은 알림에서 상단에 진하게 뜨는 글씨입니다. 채팅알림에서는 보통 TITLE이 상대방의 이름 또는 채팅방이름이 되겠죠!
현재 발자국알림에서는 타이틀을 그렇게 띄워주고있는데, 채팅에서는 안드로이드쪽에서 어떻게 띄어주냐에따라 디폴트로 반갑개를 해놓아도 될 수도 있겠네요
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.
오호 그렇군요 ~~~~!!! 모든 FCM 공통인 줄 알았어요
반영해야겠네요
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.
수고하셨습니다.
이슈
개발 사항
리뷰 요청 사항 (없으면 삭제해 주세요)
전달 사항 (없으면 삭제해 주세요)