Skip to content
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

Merged
merged 14 commits into from
Jan 8, 2025
Merged
36 changes: 30 additions & 6 deletions backend/src/main/java/com/ody/mate/service/MateService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Entry 알림을 발송 + 구독 + DepartureReminder 알림을 발송 하는 식으로 MateService 로직에 종속적인 로직이 구현되어 있습니다.

그러나, 위의 로직은 MateService에서 드러나야 하는 로직이지, 알림 서비스가 알고 있어야할 내용이 아니라 생각했습니다.

[질문] 노션에 작성해주신 부분 중에 해당 로직을 MateService 로직에 종속적인 로직이라고 본 이유가 있나요?

저는 콜리와 반대로 '토픽을 구독한다'를 MateService가 알고 있을 필요가 없다고 생각하는데, 그 이유는 토픽을 구독하는 것은 모임원 모두에게 알림을 보내기 위한 방법이라서 입니다. MateService는 모두에게 알림을 보낸다 정도만 알아도 충분하다고 생각해서 오히려 내부 구현 + fcm 관련 배경 지식이 노출된 느낌이 들어요.

Copy link
Contributor Author

@coli-geonwoo coli-geonwoo Jan 7, 2025

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 로직을 도메인들도 알고 있다고 판단한 부분도 있습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 납득했습니다!! 자세하게 설명해주어 고마워요 🥦🙇‍♀️

sendDepartureReminder(meeting, mate, fcmTopic);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[제안]
notificationService 관련 로직은 하나의 private 메서드로 분리하면 어떤가요?

Suggested change
sendEntry(meeting, mate, fcmTopic);
notificationService.subscribeTopic(member.getDeviceToken(), fcmTopic);
sendDepartureReminder(meeting, mate, fcmTopic);
sendEntryAndDepartureReminder()

sendEntry(), sendDepartureReminder()는 private 메서드인데, subscribeTopic만 바로 호출되고 있어서, 차라리 하나로 묶이면 더 가독성이 좋을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

당시 이렇게 구현했다고 전해들었던 이유는 입장알림은 입장을 한 당사자가 받지 않게하기 위함으로 알고 있어요.

입장알림을 보내고 > 구독하고 > 리마인더를 보내야 참여하는 mate를 제외한 약속 참여원들이 입장알림을 받고, 당사자가 받지 않게 되는 것으로 알고 있습니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

순서를 변경하자는 의미는 아니었어요.
sendEntry(), subscribeTopic(), sendDepartureReminder()를 sendEntryAndDepartureReminder() 혹은 sendEntryAndSubscribeAndDepartureReminder()..? private 메서드로 분리하면 가독성이 더 좋겠다라는 의미입니다😸

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) {
Expand Down Expand Up @@ -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);
}
Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~ 👍

Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[제안]
콜리가 수정한 부분은 아니지만, 컨벤션 맞춰주면 좋겠어요!
updateStatusToDone()에서도 this 키워드 제거해주는게 통일성이 좋겠네요~

Suggested change
return this.type.isDepartureReminder();
return type.isDepartureReminder();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영해주었습니다.

}
Expand Down
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[질문]
AbstractNotification는 도메인인가요 엔티티인가요?
도메인이라면 id 필드는 필요하지 않아 보여요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[제안]
protected로 더 감춰주면 어떤가요?

Suggested change
public AbstractNotification(
protected AbstractNotification(

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
}
}
18 changes: 18 additions & 0 deletions backend/src/main/java/com/ody/notification/domain/types/Entry.java
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[질문]
Meeting을 파라미터로 갖는 이유가 있나요? 사용하지 않고 있네요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
}
}
15 changes: 15 additions & 0 deletions backend/src/main/java/com/ody/notification/domain/types/Nudge.java
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
import com.ody.notification.domain.NotificationStatus;
import com.ody.notification.domain.NotificationType;
import com.ody.notification.domain.message.GroupMessage;
import com.ody.notification.domain.types.Nudge;
import com.ody.notification.dto.response.NotiLogFindResponses;
import com.ody.notification.repository.NotificationRepository;
import com.ody.notification.service.event.NoticeEvent;
import com.ody.notification.service.event.NudgeEvent;
import com.ody.notification.service.event.PushEvent;
import com.ody.notification.service.event.SubscribeEvent;
import com.ody.notification.service.event.UnSubscribeEvent;
import com.ody.route.domain.DepartureTime;
import com.ody.util.InstantConverter;
import java.time.Instant;
import java.time.LocalDateTime;
Expand All @@ -40,35 +40,14 @@ public class NotificationService {
private final TaskScheduler taskScheduler;

@Transactional
public void saveAndSendNotifications(Meeting meeting, Mate mate, DeviceToken deviceToken) {
FcmTopic fcmTopic = new FcmTopic(meeting);

Notification entryNotification = Notification.createEntry(mate, fcmTopic);
saveAndScheduleNotification(entryNotification);

SubscribeEvent subscribeEvent = new SubscribeEvent(this, deviceToken, fcmTopic);
fcmEventPublisher.publish(subscribeEvent);

saveAndSendDepartureReminderNotification(meeting, mate, fcmTopic);
}

private void saveAndSendDepartureReminderNotification(Meeting meeting, Mate mate, FcmTopic fcmTopic) {
DepartureTime departureTime = new DepartureTime(meeting, mate.getEstimatedMinutes());
LocalDateTime sendAt = calculateSendAt(departureTime);
Notification notification = Notification.createDepartureReminder(mate, sendAt, fcmTopic);
saveAndScheduleNotification(notification);
}

private LocalDateTime calculateSendAt(DepartureTime departureTime) {
if (departureTime.isBefore(LocalDateTime.now())) {
return LocalDateTime.now();
}
return departureTime.getValue();
public void saveAndSchedule(Notification notification) {
Notification savedNotification = save(notification);
scheduleNotification(savedNotification);
}

private void saveAndScheduleNotification(Notification notification) {
Notification savedNotification = notificationRepository.save(notification);
scheduleNotification(savedNotification);
@Transactional
public Notification save(Notification notification) {
return notificationRepository.save(notification);
}

private void scheduleNotification(Notification notification) {
Expand All @@ -83,9 +62,14 @@ private void scheduleNotification(Notification notification) {
);
}

public void subscribeTopic(DeviceToken deviceToken, FcmTopic fcmTopic){
SubscribeEvent subscribeEvent = new SubscribeEvent(this, deviceToken, fcmTopic);
fcmEventPublisher.publish(subscribeEvent);
}

@Transactional
public void sendNudgeMessage(Mate requestMate, Mate nudgedMate) {
Notification nudgeNotification = notificationRepository.save(Notification.createNudge(nudgedMate));
Notification nudgeNotification = notificationRepository.save(new Nudge(nudgedMate).toNotification());
NudgeEvent nudgeEvent = new NudgeEvent(this, requestMate, nudgeNotification);
fcmEventPublisher.publishWithTransaction(nudgeEvent);
}
Expand All @@ -107,11 +91,6 @@ public void schedulePendingNotification() {
log.info("애플리케이션 시작 - PENDING 상태 출발 알림 {}개 스케줄링", notifications.size());
}

@Transactional
public Notification save(Notification notification) {
return notificationRepository.save(notification);
}

@DisabledDeletedFilter
public NotiLogFindResponses findAllNotiLogs(Long meetingId) {
List<Notification> notifications = notificationRepository.findAllByMeetingIdAndSentAtBeforeDateTimeAndStatusIsNotDismissed(
Expand Down
Loading
Loading