공지 발송 기능을 구현한다. #224
공지 발송 기능을 구현한다. #224
Conversation
Test Results118 tests 117 ✅ 5s ⏱️ Results for commit 372862b. ♻️ This comment has been updated with latest results. |
|
@GIVEN53 오전에 리뷰 해드리기로 했는데, 급한 일정이 잡혀 3시까지 남기겠습니다! |
| select new maeilmail.subscribe.query.SubscribeEmail(MIN(s.id), s.email) | ||
| from Subscribe s | ||
| where s.deletedAt is null | ||
| group by s.email |
There was a problem hiding this comment.
command.repository에서 query 모델을 반환하는게 어색하다고 느껴졌어요. SubscribeQueryService를 만들거나, SubscribeEmail을 command.domain으로 이동시키는 것은 어떨까요? 개념적으로 무조건 이게 맞아보다 적어도 의존 방향을 관리하는 차원에서 말씀 드립니다.
추가로 StatisticsService의 countNewSubscribersOnSpecificDate, generateSubscribeReport 메서드도 subscribeRepository 보다는 쿼리 서비스의 모델을 받아서 처리하는게 자연스럽다고 생각해요. (이 부분은 현재 PR에서 다루면 다소 루즈해질 수 있다는 점이 염려되니 다른 이슈 파서 작업하는게 나을거 같아요!)
There was a problem hiding this comment.
command.repository에서 query 모델을 반환하는게 어색하다고 느껴졌어요. SubscribeQueryService를 만들거나, SubscribeEmail을 command.domain으로 이동시키는 것은 어떨까요? 개념적으로 무조건 이게 맞아보다 적어도 의존 방향을 관리하는 차원에서 말씀 드립니다.
그렇네요 domain이 command 패키지 하위에 있는게 어색한데 같은 레벨로 이동하는건 어떨까요?
추가로 StatisticsService의 countNewSubscribersOnSpecificDate, generateSubscribeReport 메서드도 subscribeRepository 보다는 쿼리 서비스의 모델을 받아서 처리하는게 자연스럽다고 생각해요. (이 부분은 현재 PR에서 다루면 다소 루즈해질 수 있다는 점이 염려되니 다른 이슈 파서 작업하는게 나을거 같아요!)
StatisticsService은 제가 작업한 내용에 포함되지 않아서 조금 의아한 부분인데요, 추후 리팩토링할 점을 미리 언급해 주신 걸까요?
There was a problem hiding this comment.
StatisticsService은 제가 작업한 내용에 포함되지 않아서 조금 의아한 부분인데요, 추후 리팩토링할 점을 미리 언급해 주신 걸까요?
작업하신 것과 무관한 부분이고, 추후 리팩터링 사항에 대해서 말씀 드린거였어요. 혼란이 있었다면 죄송합니다. 🥲
그렇네요 domain이 command 패키지 하위에 있는게 어색한데 같은 레벨로 이동하는건 어떨까요?
domain 패키지를 밖으로 분리하는 것에 대해서 이해하지 못했어요.
command 하위에 jpa entity를 가지고 있는 domain 패키지는 의도된 부분이에요. 커맨드성 기능은 JPA의 변경 감지를 적극적으로 이용하고, 쿼리성 기능 같은 경우에는 조회에 유리한 수단(현재로서는 queryDSL + DTO 프로젝션)을 사용하는게 현재 패키지 구조의 의도입니다.
제가 놓치고 있는 부분이 있다면 편히 말씀 주세요. 🙇🏻♂️
There was a problem hiding this comment.
제가 의도를 파악하지 못했어요 SubscribeQueryService로 분리했습니다 확인 부탁드려요!
| .map(it -> new MailMessage(it.email(), adminNotice.getTitle(), adminNotice.getContent(), "notice")) | ||
| .forEach(mailSender::sendMail); | ||
|
|
||
| log.info("공지 발송을 완료했습니다."); |
There was a problem hiding this comment.
해당 로그는 지우는게 나을거 같아요! 비동기 스레드를 사용해서 발송하기 때문에 정확한 종료 시점에 로그가 출력되지 않아요.
|
MailSender구현체를 사용하면서AdminNoticeSender는 자연스럽게 삭제되었습니다.AdminNoticeService의 테스트 발송도MailSender로 변경