-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: notification을 타입계층으로 리팩터링 #929
Changes from 7 commits
b8b9287
f77cdf0
ec435d7
211ab56
4d8775d
2b71319
deb422d
2d4decd
f10288e
9b999e1
69a6179
42bb6f1
3f3f69d
6c22260
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,8 +14,14 @@ | |||||||||
import com.ody.meeting.domain.Meeting; | ||||||||||
import com.ody.meeting.dto.response.MateEtaResponsesV2; | ||||||||||
import com.ody.member.domain.Member; | ||||||||||
import com.ody.notification.domain.FcmTopic; | ||||||||||
import com.ody.notification.domain.Notification; | ||||||||||
import com.ody.notification.domain.types.DepartureReminder; | ||||||||||
import com.ody.notification.domain.types.Entry; | ||||||||||
import com.ody.notification.domain.types.MateLeave; | ||||||||||
import com.ody.notification.domain.types.MemberDeletion; | ||||||||||
import com.ody.notification.service.NotificationService; | ||||||||||
import com.ody.route.domain.DepartureTime; | ||||||||||
import com.ody.route.domain.RouteTime; | ||||||||||
import com.ody.route.service.RouteService; | ||||||||||
import com.ody.util.TimeUtil; | ||||||||||
|
@@ -43,14 +49,32 @@ public MateSaveResponseV2 saveAndSendNotifications( | |||||||||
Member member, | ||||||||||
Meeting meeting | ||||||||||
) { | ||||||||||
validateMeetingOverdue(meeting); | ||||||||||
validateAlreadyAttended(member, meeting); | ||||||||||
Mate mate = saveMateAndEta(mateSaveRequest, member, meeting); | ||||||||||
|
||||||||||
FcmTopic fcmTopic = new FcmTopic(meeting); | ||||||||||
sendEntry(meeting, mate, fcmTopic); | ||||||||||
notificationService.subscribeTopic(member.getDeviceToken(), fcmTopic); | ||||||||||
sendDepartureReminder(meeting, mate, fcmTopic); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안]
Suggested change
sendEntry(), sendDepartureReminder()는 private 메서드인데, subscribeTopic만 바로 호출되고 있어서, 차라리 하나로 묶이면 더 가독성이 좋을 것 같아요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 당시 이렇게 구현했다고 전해들었던 이유는 입장알림은 입장을 한 당사자가 받지 않게하기 위함으로 알고 있어요. 입장알림을 보내고 > 구독하고 > 리마인더를 보내야 참여하는 mate를 제외한 약속 참여원들이 입장알림을 받고, 당사자가 받지 않게 되는 것으로 알고 있습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 순서를 변경하자는 의미는 아니었어요. |
||||||||||
return MateSaveResponseV2.from(meeting); | ||||||||||
} | ||||||||||
|
||||||||||
private void sendEntry(Meeting meeting, Mate mate, FcmTopic fcmTopic) { | ||||||||||
Entry entry = new Entry(mate, meeting, fcmTopic); | ||||||||||
notificationService.saveAndSchedule(entry.toNotification()); | ||||||||||
} | ||||||||||
|
||||||||||
private void sendDepartureReminder(Meeting meeting, Mate mate, FcmTopic fcmTopic) { | ||||||||||
DepartureTime departureTime = new DepartureTime(meeting, mate.getEstimatedMinutes()); | ||||||||||
DepartureReminder departureReminder = new DepartureReminder(mate, departureTime, fcmTopic); | ||||||||||
notificationService.saveAndSchedule(departureReminder.toNotification()); | ||||||||||
} | ||||||||||
|
||||||||||
private void validateMeetingOverdue(Meeting meeting) { | ||||||||||
if (meeting.isOverdue()) { | ||||||||||
throw new OdyBadRequestException("참여 가능한 시간이 지난 약속에 참여할 수 없습니다."); | ||||||||||
} | ||||||||||
|
||||||||||
Mate mate = saveMateAndEta(mateSaveRequest, member, meeting); | ||||||||||
notificationService.saveAndSendNotifications(meeting, mate, member.getDeviceToken()); | ||||||||||
return MateSaveResponseV2.from(meeting); | ||||||||||
} | ||||||||||
|
||||||||||
public void validateAlreadyAttended(Member member, Meeting meeting) { | ||||||||||
|
@@ -133,15 +157,15 @@ public void deleteAllByMember(Member member) { | |||||||||
|
||||||||||
@Transactional | ||||||||||
public void withdraw(Mate mate) { | ||||||||||
Notification memberDeletionNotification = Notification.createMemberDeletion(mate); | ||||||||||
Notification memberDeletionNotification = new MemberDeletion(mate).toNotification(); | ||||||||||
notificationService.save(memberDeletionNotification); | ||||||||||
delete(mate); | ||||||||||
} | ||||||||||
|
||||||||||
@Transactional | ||||||||||
public void leaveByMeetingIdAndMemberId(Long meetingId, Long memberId) { | ||||||||||
Mate mate = findByMeetingIdAndMemberId(meetingId, memberId); | ||||||||||
Notification leaveNotification = Notification.createMateLeave(mate); | ||||||||||
Notification leaveNotification = new MateLeave(mate).toNotification(); | ||||||||||
notificationService.save(leaveNotification); | ||||||||||
delete(mate); | ||||||||||
} | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ~ 👍 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -60,56 +60,6 @@ public Notification( | |||||
this(null, mate, type, sendAt, status, fcmTopic); | ||||||
} | ||||||
|
||||||
public static Notification createEntry(Mate mate, FcmTopic fcmTopic) { | ||||||
return new Notification( | ||||||
mate, | ||||||
NotificationType.ENTRY, | ||||||
LocalDateTime.now(), | ||||||
NotificationStatus.DONE, | ||||||
fcmTopic | ||||||
); | ||||||
} | ||||||
|
||||||
public static Notification createDepartureReminder(Mate mate, LocalDateTime sendAt, FcmTopic fcmTopic) { | ||||||
return new Notification( | ||||||
mate, | ||||||
NotificationType.DEPARTURE_REMINDER, | ||||||
sendAt, | ||||||
NotificationStatus.PENDING, | ||||||
fcmTopic | ||||||
); | ||||||
} | ||||||
|
||||||
public static Notification createNudge(Mate nudgeMate) { | ||||||
return new Notification( | ||||||
nudgeMate, | ||||||
NotificationType.NUDGE, | ||||||
LocalDateTime.now(), | ||||||
NotificationStatus.DONE, | ||||||
null | ||||||
); | ||||||
} | ||||||
|
||||||
public static Notification createMemberDeletion(Mate mate) { | ||||||
return new Notification( | ||||||
mate, | ||||||
NotificationType.MEMBER_DELETION, | ||||||
LocalDateTime.now(), | ||||||
NotificationStatus.DONE, | ||||||
null | ||||||
); | ||||||
} | ||||||
|
||||||
public static Notification createMateLeave(Mate mate) { | ||||||
return new Notification( | ||||||
mate, | ||||||
NotificationType.LEAVE, | ||||||
LocalDateTime.now(), | ||||||
NotificationStatus.DONE, | ||||||
null | ||||||
); | ||||||
} | ||||||
|
||||||
public boolean isDepartureReminder() { | ||||||
return this.type.isDepartureReminder(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안]
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 반영해주었습니다. |
||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,34 @@ | ||||||
package com.ody.notification.domain.types; | ||||||
|
||||||
import com.ody.mate.domain.Mate; | ||||||
import com.ody.notification.domain.FcmTopic; | ||||||
import com.ody.notification.domain.Notification; | ||||||
import com.ody.notification.domain.NotificationStatus; | ||||||
import com.ody.notification.domain.NotificationType; | ||||||
import java.time.LocalDateTime; | ||||||
import lombok.AllArgsConstructor; | ||||||
|
||||||
@AllArgsConstructor | ||||||
public abstract class AbstractNotification { | ||||||
|
||||||
private final Long id; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. [반영 완료] |
||||||
private final Mate mate; | ||||||
private final NotificationType type; | ||||||
private final LocalDateTime sendAt; | ||||||
private final NotificationStatus status; | ||||||
private final FcmTopic fcmTopic; | ||||||
|
||||||
public AbstractNotification( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안]
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [반영 완료] |
||||||
Mate mate, | ||||||
NotificationType type, | ||||||
LocalDateTime sendAt, | ||||||
NotificationStatus status, | ||||||
FcmTopic fcmTopic | ||||||
) { | ||||||
this(null, mate, type, sendAt, status, fcmTopic); | ||||||
} | ||||||
|
||||||
public Notification toNotification() { | ||||||
return new Notification(id, mate, type, sendAt, status, fcmTopic); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package com.ody.notification.domain.types; | ||
|
||
import com.ody.mate.domain.Mate; | ||
import com.ody.notification.domain.FcmTopic; | ||
import com.ody.notification.domain.NotificationStatus; | ||
import com.ody.notification.domain.NotificationType; | ||
import com.ody.route.domain.DepartureTime; | ||
import java.time.LocalDateTime; | ||
|
||
public class DepartureReminder extends AbstractNotification { | ||
|
||
private static final NotificationType type = NotificationType.DEPARTURE_REMINDER; | ||
|
||
public DepartureReminder(Mate mate, DepartureTime departureTime, FcmTopic fcmTopic) { | ||
super(mate, type, calculateSendAt(departureTime), NotificationStatus.PENDING, fcmTopic); | ||
} | ||
|
||
private static LocalDateTime calculateSendAt(DepartureTime departureTime) { | ||
if (departureTime.isBefore(LocalDateTime.now())) { | ||
return LocalDateTime.now(); | ||
} | ||
return departureTime.getValue(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package com.ody.notification.domain.types; | ||
|
||
import com.ody.mate.domain.Mate; | ||
import com.ody.meeting.domain.Meeting; | ||
import com.ody.notification.domain.FcmTopic; | ||
import com.ody.notification.domain.NotificationStatus; | ||
import com.ody.notification.domain.NotificationType; | ||
import java.time.LocalDateTime; | ||
|
||
public class Entry extends AbstractNotification { | ||
|
||
private static final NotificationType type = NotificationType.ENTRY; | ||
|
||
public Entry(Mate mate, Meeting meeting, FcmTopic fcmTopic) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. [반영 완료] fcmTopic을 meeting에서 꺼내다가 그냥 파라미터로 받아준 것 같아요. 반영해주었습니다. |
||
super(mate, type, LocalDateTime.now(), NotificationStatus.DONE, fcmTopic); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package com.ody.notification.domain.types; | ||
|
||
import com.ody.mate.domain.Mate; | ||
import com.ody.notification.domain.NotificationStatus; | ||
import com.ody.notification.domain.NotificationType; | ||
import java.time.LocalDateTime; | ||
|
||
public class MateLeave extends AbstractNotification { | ||
|
||
private static final NotificationType type = NotificationType.LEAVE; | ||
|
||
public MateLeave(Mate mate) { | ||
super(mate, type, LocalDateTime.now(), NotificationStatus.DONE, null); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package com.ody.notification.domain.types; | ||
|
||
import com.ody.mate.domain.Mate; | ||
import com.ody.notification.domain.NotificationStatus; | ||
import com.ody.notification.domain.NotificationType; | ||
import java.time.LocalDateTime; | ||
|
||
public class MemberDeletion extends AbstractNotification { | ||
|
||
private static final NotificationType type = NotificationType.MEMBER_DELETION; | ||
|
||
public MemberDeletion(Mate mate) { | ||
super(mate, type, LocalDateTime.now(), NotificationStatus.DONE, null); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package com.ody.notification.domain.types; | ||
|
||
import com.ody.mate.domain.Mate; | ||
import com.ody.notification.domain.NotificationStatus; | ||
import com.ody.notification.domain.NotificationType; | ||
import java.time.LocalDateTime; | ||
|
||
public class Nudge extends AbstractNotification { | ||
|
||
private static final NotificationType type = NotificationType.NUDGE; | ||
|
||
public Nudge(Mate nudgeMate) { | ||
super(nudgeMate, type, LocalDateTime.now(), NotificationStatus.DONE, null); | ||
} | ||
} |
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.
[질문] 노션에 작성해주신 부분 중에 해당 로직을 MateService 로직에 종속적인 로직이라고 본 이유가 있나요?
저는 콜리와 반대로 '토픽을 구독한다'를 MateService가 알고 있을 필요가 없다고 생각하는데, 그 이유는 토픽을 구독하는 것은 모임원 모두에게 알림을 보내기 위한 방법이라서 입니다. MateService는 모두에게 알림을 보낸다 정도만 알아도 충분하다고 생각해서 오히려 내부 구현 + 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.
첫째로, 해당 메서드가 약속 참여 플로우 이외에 그 어떤 비즈니스 로직에도 확장성있게 사용될수가 없었어요. 참여 알림 보내고 + 구독하고 + 약속리마인더 알림을 보낸다 -> 라는 흐름은 약속 참여시 약속을 보내는 방법이지 알림 서비스가 알아야할 약속 참여 시 필요한 알림 로직이 아니라 생각했습니다.
두번째로, saveAndSendNotification으로 추상화된 메서드 명을 보면 그저 알림을 저장하고 보내는 것으로 생각이 되는데, 저장을 하는 알림이 약속 리마인더와 출발알림으로 정해져 있습니다. 다른 알림은 저장될수가 없고 보낼수도 없습니다. mateService 참여 시 필요한 알림을 해당 메서드에서 보내고 있었습니다.
세번째로, fcm 구현이 밖으로 드러나있다는 점에 대해서는 동의합니다. 해당 구현을 캡슐화하기 위해 sendToDevice, sendToTopic 등의 메서드를 만들어주어 subscribe를 드러내지 않는 것은 동의합니다. 그러나, notificationService가 특정 알림 객체를 위한 메서드를 가지는 것은 비즈니스 로직상 어떤 알림을 보냈는지를 상위 서비스가 아니라 하위 서비스인 notificationService가 알게된다는 점에서 너무 많은 것을 알고 있다고 생각합니다. 또한, 기존에 notificationServic가 unsubscribe라는 메서드를 가지고 있다는 점, fcmTopic 필드를 meeting이 가지고 있다는 점에서 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.
오 납득했습니다!! 자세하게 설명해주어 고마워요 🥦🙇♀️