feat : 설정 따른 알림 전송 #140
feat : 설정 따른 알림 전송 #140
Conversation
- comment -> newComment로 수정 - all -> allNotifications로 수정
- 알림 전송 케이스 별 메세지 추가
- CannotFoundNotificationSettingException 추가 - custom handler 등록
- 풀이 생성 시 저장 검증 - 풀이 생성 시 설정에 따른 알림 전송 검증
- 테스트 추가 작성
- SolutionServiceTest에서 CommentRepository 모킹 추가
흠 한번 다시 고민 해봐야겠어요! 의견 감삼다 |
|
요거는 제가 리팩토링을 좀만 더 고민해보고싶어서..! 내일 중으로 완료해서 self-merge 하겠습니다 |
| NotificationSetting setting = notificationSettingRepository.findByMember(member) | ||
| .orElseThrow(() -> new CannotFoundNotificationSettingException("그룹 멤버의 알림 정보를 조회할 수 없습니다.")); | ||
|
|
||
| if (!setting.isAllNotifications() || !setting.isNewComment()) |
| @@ -141,7 +158,8 @@ public void deleteGroup(User user, Long groupId) { | |||
| if (RoleOfGroupMember.isOwner(groupMember)) { // owner | |||
| bookmarkedStudyGroupRepository.deleteAll(bookmarkedStudyGroupRepository.findAllByStudyGroup(studyGroup)); | |||
| rankingRepository.deleteAll(rankingRepository.findAllByStudyGroup(studyGroup)); | |||
| groupMemberRepository.delete(groupMember); | |||
| notificationSettingRepository.deleteAll(notificationSettingRepository.findAllByStudyGroup(studyGroup)); | |||
There was a problem hiding this comment.
이런거 그냥 studygroup으로 질의해서 batch delete하면 쿼리 2->1번으로 줄일 수 있겠네용. 반영해달라는건 아니고 사족입니다
| users.add(member.getUser().getEmail()); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
여기 try-catch 부분, 로깅용인데 service.sendList() 함수 안에 넣는건 어떻게 생각하세요? 예외 처리를 외부까지 알아야할까.. 라는 의문이 들어서 건토 제안
There was a problem hiding this comment.
흠 외부에서 에외를 잡고 알림 히스토리를 구성 할라 했었는데 책임상으로도 굳이 여기서 처리하지 않아도 될 것 같네요!
이건 추후에 알림 히스토리 추가될 때notificationService 내에서 처리하도록 수정해봐야겠네요 인정입니다!
| if (member.getUser().getId().equals(newMember.getUser().getId())) | ||
| continue; | ||
|
|
||
| NotificationSetting setting = notificationSettingRepository.findByMember(member) |
There was a problem hiding this comment.
이것도 사족인데요, "알림 전송" 이라는것의 무게 고민해보기 좋은 주제인 것 같습니다. 알림은 부가적인 기능인데요, 여기서 throw 해버리면 알림 로직 문제가 생기면 메인 기능(유저 조인?) 등도 실패합니다. 과연 이렇게 원자적으로 가야 할지, 아니면 실패하면 로깅 강하게 찍되 메인 기능은 실행되게 보장해야할지.. 같은 것들 고민해보면 좋겠네용
There was a problem hiding this comment.
저도 이번 PR 진행하면서 해당 내용의 고민을 했었는데요,
알림 전송의 경우에는 다른 서비스들과 완전히 독립적인 기능이라고 생각해서 타 로직 내부에서 실패해도 전체 롤백이 안되도록 처리했었습니다.
하지만, 알림 설정 정보의 경우는 조회가 실패했다면 그건 회원 그룹 생성 때 이미 치명적인 버그가 발생한 것이라고 생각했습니다 (알림 설정 정보가 없다는 것은 그룹 가입 때 알림 설정을 추가 하는 것이 실패했다는 뜻). 이는 알림 생성과는 다르게, 그룹의 메인 기능에서 영향을 받은 것이라고 생각했기에 전체 롤백을 할 만큼의 중요한 부분이라고 생각했습니다. 또한, 알림 설정은 알림 전송에도 연쇄적인 영향을 미치기 때문에 충분히 전체 롤백을 할만 한 조회라고 생각했습니다.
제 생각을 좀 깔끔하게 설명하고 싶은데.. 한국말 너무 어렵네요
There was a problem hiding this comment.
아하, 이해했습니다.
제가 이 코멘트를 달 때의 생각을 대충 말해보면..
일단 이 오류 자체는 엄밀히 말해서 Logic error입니다. 다른 api의 오류들은 사용자가 파라미터 값을 조작한디면 던져질 수 있는 케이스들이지만, 이 오류는 현재까지 로직상 존재할 수 없고, 존재해서는 안되는 케이스입니다. Internal Server error 느낌..
다만 타입상 체크가 가능하기에 이에 대해 검사하는것정도는 저도 OK입니다. 다만 이건 엄청난 엣지 케이스고, 반드시 고치고 트래킹해야할 케이스니까 로깅을 강하게 찍는건 필수같아요
또한, 이 오류를 던지게 되면 오류에 대한 메세지를 받아보는 사람은(그룹에 대한 알림 정보 조회 실패)는 실제로 알림정보가 존재하지 않는 유저가 아니라, 가입을 새로 시도하는 유저입니다. 물론 이건 클라이언트에서 어느정도 보정해줄 수 있지만, 꽤 당황스럽고 어색하게 느껴질 수 있습니다.
이런 생각들을 하며 달았던 것 같아요.
일단 이미 이런 부분도 생각하며 개발하신다는게 굉장히 좋은 포인트라고 여겨지네요 🙂
입사 후, 학생때와 비교해 개발자로서 성장하고 있는 역량이 공수와 효용 사이 Trade-off를 계산하는것과, 이런 플로우속 처리의 UX를 고려한 우선순위와 중요도 계산인 것 같아요. 계속 화이팅입니다!
There was a problem hiding this comment.
이전 코드랑 함께 보고 싶었는데 코드는 안 보이네요 이제..
차분히 읽어 보았습니다..제가 이해한 것이 맞을지 싶네요
-
알림 설정 조회 함수에서 오류를 던지다 에러가 날 경우 유저 조인 자체가 실패함 따라서 알림 설정에서의 throw 를 생각해 봐야함
-
하지만 알림 설정 조회에서 오류가 있다 -> 이미 치명적 버그로 판단..?
이 맞을지 ? 여러번 읽어도 참 어렵네요
여러분께 배울 것들이 참 많네요 늘 고맙습니다 .. 😊
There was a problem hiding this comment.
우선 이전 코드는, 코멘트 위에 있는 파일 이름을 누르면
코멘트가 달린 시점의 diff가 보여요.
- 알림 설정에서 throw하면 유저 조인 자체가 실패합니다. 그러나 알림 설정이라는 부가 기능이, 유저 가입이라는 코어 기능을 막아도 될지..에 대한 이야기입니다
- 알림을 보내다가 실패한 것이 아니고, 알림을 설정하는 단계이기 때문에 기존 유저들의 설정 엔티티가 없으면 이미 크리티컬한 로직 오류(DB corrupted)가 존재한 것이기 때문에, 중요한 상황으로 간주해서 throw했음
이 될 것 같아요
There was a problem hiding this comment.
다만 타입상 체크가 가능하기에 이에 대해 검사하는것정도는 저도 OK입니다. 다만 이건 엄청난 엣지 케이스고, 반드시 고치고 트래킹해야할 케이스니까 로깅을 강하게 찍는건 필수같아요
500 error의 경우가 맞으니 로깅을 강하게 찍는 다는 것 좋아요! 로깅 사용할 건 생각 못해봤는데 감사합니다 :) 그렇다면 error 레벨의 로그를 찍는다는 말씀이겠죠?
또한, 이 오류를 던지게 되면 오류에 대한 메세지 (그룹에 대한 알림 정보 조회 실패)는 실제로 알림정보가 존재하지 않는 유저가 아니라, 가입을 새로 시도하는 유저입니다. 물론 이건 클라이언트에서 어느정도 보정해줄 수 있지만, 꽤 당황스럽고 어색하게 느껴질 수 있습니다.
이건 저도 에러 메세지 만들 때 고민하던 부분인데.. 그렇다면 가입하지 않은 유저가 해당 API를 시도하니까 이것 또한 가입하지 않은 그룹임을 지칭하는 메세지를 보내야 할지.. 여전히 모르겠네요
말씀하신 부분들 모두 이해했습니다! 여러모로 고민거리가 많아지네요 의견 감사합니다-
There was a problem hiding this comment.
- 알림 설정 조회 함수에서 오류를 던지다 에러가 날 경우 유저 조인 자체가 실패함 따라서 알림 설정에서의 throw 를 생각해 봐야함
- 하지만 알림 설정 조회에서 오류가 있다 -> 이미 치명적 버그로 판단..?
맞습니다 대강 알림 설정의 무게를 어떻게 생각하냐에 관련한 이야기였어요!
알림 설정 조회를 실패했다고 핵심 로직을 전체 롤백을 할 수준일까 아닐까?의 주제였다고 봐주심 될 것 같아요
There was a problem hiding this comment.
우선 이전 코드는, 코멘트 위에 있는 파일 이름을 누르면 코멘트가 달린 시점의 diff가 보여요.
- 알림 설정에서 throw하면 유저 조인 자체가 실패합니다. 그러나 알림 설정이라는 부가 기능이, 유저 가입이라는 코어 기능을 막아도 될지..에 대한 이야기입니다
- 알림을 보내다가 실패한 것이 아니고, 알림을 설정하는 단계이기 때문에 기존 유저들의 설정 엔티티가 없으면 이미 크리티컬한 로직 오류(DB corrupted)가 존재한 것이기 때문에, 중요한 상황으로 간주해서 throw했음
이 될 것 같아요
친절한 설명 감사합니다
- sendNotificationToMembers() : 그룹, 알림 받는 사람, 카테고리, 메세지 파라미터로 받아 알림 전송 - 예외 로깅을 알림 속에서 처리
|
@s-hwan @sh0723 @hwangjokim |
|
제가 봤는데 좋은거같아요~ |
|
알림을 전송할 때 알림 설정 정보 조회가 안되면 error level의 log를 찍도록 추가했습니다. |
📌 Related Issue
#122
🚀 Description
아래 다섯 개의 경우들에 대해 알림 설정이 켜져있는 경우 알림을 전송하도록 구현했습니다.
그룹 생성 및 가입 시 NotificationSetting 생성을 추가했습니다.
그룹 제거 및 회원 탈퇴 시 NotificationSetting 제거를 추가했습니다.
알림 설정 목록 조회 API, 알림 설정 수정 API도 같이 진행했습니다.
얘 PR이 먼저 merge 되어야 작업 가능했던 API들인지라,, 속도를 좀 올려야 할 것 같아서 복잡한 기능이 아니어가지고 한 번에 처리했습니다.
📢 Review Point
NotificationSettingController로 따로 빼서 관리하는게 나을지 고민이 됩니다. 지금은 그냥NotificationController에 둘다 박혀있는지라.. 뭐가 나을까요📚Etc (선택)