refactor: notification을 타입계층으로 리팩터링 #929

coli-geonwoo · 2024-12-14T10:16:28Z

🚩 연관 이슈

close #926

📝 작업 내용

Notification 객체의 도메인과 entity를 분리합니다.
NotificiationService 내부에서는 알림객체를 entity로 바라보고 로직을 작성할 수 있습니다.
알림을 보내는 상위 객체에서는 알림 객체를 domain으로 바라보고 로직을 작성할 수 있습니다.

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

Notion Page : 리팩터링한 이유

제 로컬에서는 레디스 테스트가 계속 통과하지 않는데 CI 환경에서는 통과해서 local 문제라 판단하여 우선 더 늦기 전에 PR 올려둡니다.

github-actions · 2024-12-14T10:18:56Z

Test Results

64 files 64 suites 14s ⏱️
210 tests 210 ✅ 0 💤 0 ❌
211 runs 211 ✅ 0 💤 0 ❌

Results for commit 6c22260.

♻️ This comment has been updated with latest results.

github-actions · 2024-12-14T10:18:58Z

📝 Test Coverage Report

Overall Project	82.97%	🍏
Files changed	100%	🍏

File	Coverage
Notification.java	100%	🍏
NotificationService.java	100%	🍏
Entry.java	100%	🍏
AbstractNotification.java	100%	🍏
MemberDeletion.java	100%	🍏
MateLeave.java	100%	🍏
Nudge.java	100%	🍏
DepartureReminder.java	100%	🍏
MateService.java	95.79%	🍏

coli-geonwoo · 2024-12-14T10:22:57Z

backend/src/test/java/com/ody/meeting/controller/MeetingControllerTest.java

@@ -195,5 +204,46 @@ void returnMissing_When_isMissing_Mate() {
            assertThat(mateEtaResponses.mateEtas().get(1).status()).isNotEqualTo(EtaStatus.MISSING);
        }
    }
+


리팩터링으로 인해 findAllMeetingLogsOrderOfEntryAndDepartureNotification()를 대체할 테스트를 인수 테스트로 추가하였습니다.

coli-geonwoo · 2024-12-14T10:36:42Z

backend/src/test/java/com/ody/notification/domain/types/DepartureReminderTest.java

+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class DepartureReminderTest {


발송 시점이 현재 시점보다 이른 경우, 즉시 발송한다는 책임은 NotificatinService가 아닌 DepartureReminder의 내부 로직으로 캡슐화되어 테스트를 이전했습니다.

eun-byeol

고생하셨어요 🥦! 객체지향적인 눈이 좋으시네요ㅎㅅㅎ
콜리의 리팩터링이 합리적이라고 생각해요👍 객체지향적으로 역할이 분리된 점이 좋아요.
다만 AbstractNotification이 생기면서 복잡성이 조금 증가했다고도 생각해요. NotificationService가 가벼워진 만큼, 사용하는 쪽에선 조금 더 Notification 로직을 더 잘 알아야 해요. Nudge와 같은 AbstractNotification 객체를 직접 만들어줘야 하는 번거로움이 생기기도 했구요.
그럼에도 객체지향 관점에서 리팩터링은 찬성입니다!
리뷰가 늦어지게 되었는데, 기다려줘서 고맙습니다. 질문 남겼으니 확인해주세요😊

eun-byeol · 2025-01-06T09:37:31Z

backend/src/main/java/com/ody/notification/domain/types/AbstractNotification.java

+    private final NotificationStatus status;
+    private final FcmTopic fcmTopic;
+
+    public AbstractNotification(


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

Suggested change

public AbstractNotification(

protected AbstractNotification(

[반영 완료]

eun-byeol · 2025-01-06T09:40:15Z

backend/src/main/java/com/ody/notification/domain/types/Entry.java

+
+    private static final NotificationType type = NotificationType.ENTRY;
+
+    public Entry(Mate mate, Meeting meeting, FcmTopic fcmTopic) {


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

[반영 완료]

fcmTopic을 meeting에서 꺼내다가 그냥 파라미터로 받아준 것 같아요. 반영해주었습니다.

eun-byeol · 2025-01-06T09:49:37Z

backend/src/main/java/com/ody/notification/domain/types/AbstractNotification.java

+@AllArgsConstructor
+public abstract class AbstractNotification {
+
+    private final Long id;


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

[반영 완료]

eun-byeol · 2025-01-06T09:51:53Z

backend/src/main/java/com/ody/notification/domain/Notification.java

-                null
-        );
-    }
-
    public boolean isDepartureReminder() {
        return this.type.isDepartureReminder();


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

Suggested change

return this.type.isDepartureReminder();

return type.isDepartureReminder();

반영해주었습니다.

eun-byeol · 2025-01-06T10:21:33Z

backend/src/main/java/com/ody/mate/service/MateService.java

+        Nudge nudge = new Nudge(nudgedMate);
+        notificationService.sendNudgeMessage(requestMate, nudge);


[제안]
1안
saveAndSchedule(notification)처럼 saveAndSend(notification) 메서드를 만드는건 어떤가요?

saveAndSchedule() : 알림을 저장하고 예약한다

saveAndSend() : 알림을 저장하고 바로 보낸다

save() : 알림을 저장만 하고 보내지 않는다

알림을 저장하고, 보낸다라는 행위만 알면되도록 저수준의 서비스에 맞게 알림의 구체적인 종류는 모르도록 리팩터링할 수 있었습니다.

sendNudgeMessage()의 경우엔 알림의 구체적인 종류에 의존적이라, 추상화 수준이 달라 어색하게 느껴지는 것 같아요.

2안
1안에서 언급한 saveAndSchedule()과 saveAndSend()를 통합하는 방법입니다.

스케줄링 로직을 외부에서 알 필요가 있을까요? 외부에서 NotificationService를 사용할 땐, 알림을 보낸다/안 보낸다만 알면 되지 않을까요?
Notification의 sendAt 시점을 보고도 스케줄링을 해야 할지, 하지 않아도 될지 알 수 있을 것 같아요.
즉, NotificationService public 메서드는 두개로 구분되게요.

saveAndSend() : 알림을 저장하고 보낸다.(스케줄링 포함)

save() : 알림을 저장만 하고 보내지 않는다

단, NotificationService 내에서 private 메서드로 스케줄링이 필요한지 아닌지 분기처리가 필요해지는 단점이 있을 수 있어요.

이슈 파서 작업해주겠습니다~

eun-byeol · 2025-01-06T10:38:34Z

backend/src/main/java/com/ody/mate/service/MateService.java

+        sendEntry(meeting, mate, fcmTopic);
+        notificationService.subscribeTopic(member.getDeviceToken(), fcmTopic);
+        sendDepartureReminder(meeting, mate, fcmTopic);


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

Suggested change

sendEntry(meeting, mate, fcmTopic);

notificationService.subscribeTopic(member.getDeviceToken(), fcmTopic);

sendDepartureReminder(meeting, mate, fcmTopic);

sendEntryAndDepartureReminder()

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

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

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

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

backend/src/test/java/com/ody/notification/domain/types/DepartureReminderTest.java

mzeong

드디어 리뷰 했습니다!! 고생하셨습니다 콜리!

mzeong · 2025-01-06T13:03:44Z

backend/src/main/java/com/ody/notification/domain/Notification.java

mzeong · 2025-01-06T13:29:24Z

backend/src/main/java/com/ody/mate/service/MateService.java

+
+        FcmTopic fcmTopic = new FcmTopic(meeting);
+        sendEntry(meeting, mate, fcmTopic);
+        notificationService.subscribeTopic(member.getDeviceToken(), fcmTopic);


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

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

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

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

첫째로, 해당 메서드가 약속 참여 플로우 이외에 그 어떤 비즈니스 로직에도 확장성있게 사용될수가 없었어요. 참여 알림 보내고 + 구독하고 + 약속리마인더 알림을 보낸다 -> 라는 흐름은 약속 참여시 약속을 보내는 방법이지 알림 서비스가 알아야할 약속 참여 시 필요한 알림 로직이 아니라 생각했습니다.

두번째로, saveAndSendNotification으로 추상화된 메서드 명을 보면 그저 알림을 저장하고 보내는 것으로 생각이 되는데, 저장을 하는 알림이 약속 리마인더와 출발알림으로 정해져 있습니다. 다른 알림은 저장될수가 없고 보낼수도 없습니다. mateService 참여 시 필요한 알림을 해당 메서드에서 보내고 있었습니다.

세번째로, fcm 구현이 밖으로 드러나있다는 점에 대해서는 동의합니다. 해당 구현을 캡슐화하기 위해 sendToDevice, sendToTopic 등의 메서드를 만들어주어 subscribe를 드러내지 않는 것은 동의합니다. 그러나, notificationService가 특정 알림 객체를 위한 메서드를 가지는 것은 비즈니스 로직상 어떤 알림을 보냈는지를 상위 서비스가 아니라 하위 서비스인 notificationService가 알게된다는 점에서 너무 많은 것을 알고 있다고 생각합니다. 또한, 기존에 notificationServic가 unsubscribe라는 메서드를 가지고 있다는 점, fcmTopic 필드를 meeting이 가지고 있다는 점에서 fcm 로직을 도메인들도 알고 있다고 판단한 부분도 있습니다.

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

mzeong · 2025-01-06T13:42:13Z

backend/src/main/java/com/ody/notification/domain/types/AbstractNotification.java

+    private static LocalDateTime calculateSendAt(LocalDateTime sendAt) {
+        if (sendAt.isBefore(LocalDateTime.now())) {
+            return LocalDateTime.now();
+        }
+        return sendAt;
+    }


[질문] 해당 부분은 Departure Reminder를 위한 로직인데 AbstractNotification에 위치시킨 이유가 있나요?

처음에는 Departure Reminder에 위치시켰었는데요.

이전 Notifiaction 로직에서 NotificationService의 private method로 보내기 전에 calualteSendAt으로 모든 notification의 sendAt을 체크해서 공통로직인 줄 알았습니다.

전송 시간이 과거이면 현재 보낸다 라는 규칙을 Notification 전체 규칙으로 바라볼 것인지, Departure_Reminder 특수 도메인 규칙으로 바라볼 것인지의 차이일 것 같아요.

제리는 어떻게 생각하시나요?

음.. DepartureReminder의 특수 규칙이었을 때 더 확장성이 좋을 것 같아요.
전송 시간이 과거이면 보내지 않는다라는 선택지가 생길 수 있으니까요.
물론, 지금은 그런 로직이 없어서 전체 규칙으로 바라보아도 무방할 것 같습니다.

지금까지 공통 로직으로 서비스되긴 했지만.... 😭
Departure Reminder만을 위한 규칙 혹은 중요한 발송 알림 대상 규칙으로 두어야 좋다고 생각해요!!

mzeong · 2025-01-06T13:55:38Z

backend/src/main/java/com/ody/notification/domain/types/DepartureReminder.java

+    private static final NotificationType type = NotificationType.DEPARTURE_REMINDER;
+
+    public DepartureReminder(Mate mate, DepartureTime departureTime, FcmTopic fcmTopic) {
+        super(mate, type, departureTime.getValue(), NotificationStatus.PENDING, fcmTopic);
+    }


[제안] 상수로 빼지 않고 바로 적어줘도 될 것 같아요 (개행 때문인가요?)

[제안] 아니면 상수 대신 오버라이딩은 어떤가요? 추가로 MemberDeletion, MateLeave와 같이 발송되지 않고 로그에만 남는 알림 타입들은 추상화 계층을 하나 더 줘도 좋을 것 같아요

public Notification toNotification() { return new Notification(id, mate, getType(), sendAt, status, fcmTopic); } protected abstract NotificationType getType();

public class MemberDeletion extends LogNotification { public MemberDeletion(Mate mate) { super(mate); } @Override protected NotificationType getType() { return NotificationType.MEMBER_DELETION; } }

[반영 완료]

의도는 인스턴스와 관계 없이 공통적으로 가지는 정적값은 상수가 더 알맞다고 판단했습니다. 생성자에 필드로 들어가는 값은 default 값으로 해당 타입을 다른 값으로 의존성 주입할 변화의 여지를 준다고 생각했습니다.

타입계층 상 오버라이딩을 통한 type 설정이 더 합리적이라 생각해 반영해주었습니다.

MemberDeletion, MateLeave와 같이 발송되지 않고 로그에만 남는 알림 타입

이건 Notification이랑 Log Entity를 분리하는 리팩터링을 할 때 반영해보겠습니다.

eun-byeol

빠르게 반영해주어 감사합니다 콜리🥦
간단한 리뷰 남겼는데, 관련 이슈 작업하실때 반영해주셔도 좋을 것 같아 approve+merge 하겠습니다!

eun-byeol · 2025-01-08T06:08:02Z

backend/src/main/java/com/ody/notification/domain/types/AbstractNotification.java

+    private static LocalDateTime calculateSendAt(LocalDateTime sendAt) {
+        if (sendAt.isBefore(LocalDateTime.now())) {
+            return LocalDateTime.now();
+        }
+        return sendAt;
+    }


음.. DepartureReminder의 특수 규칙이었을 때 더 확장성이 좋을 것 같아요.
전송 시간이 과거이면 보내지 않는다라는 선택지가 생길 수 있으니까요.
물론, 지금은 그런 로직이 없어서 전체 규칙으로 바라보아도 무방할 것 같습니다.

eun-byeol · 2025-01-08T06:12:19Z

backend/src/main/java/com/ody/mate/service/MateService.java

+        sendEntry(meeting, mate, fcmTopic);
+        notificationService.subscribeTopic(member.getDeviceToken(), fcmTopic);
+        sendDepartureReminder(meeting, mate, fcmTopic);


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

eun-byeol · 2025-01-08T06:15:39Z

backend/src/main/java/com/ody/notification/domain/types/AbstractNotification.java

+        this.type = getType();
+        this.sendAt = calculateSendAt(sendAt);
+        this.status = status;
+        this.fcmTopic = fcmTopic;
    }

    private static LocalDateTime calculateSendAt(LocalDateTime sendAt) {


[제안]
static 키워드 이제는 제거해주어도 되겠네요! 이 부분은 제가 코드 수정하면서 반영하겠습니다ㅎㅎ

Suggested change

private static LocalDateTime calculateSendAt(LocalDateTime sendAt) {

private LocalDateTime calculateSendAt(LocalDateTime sendAt) {

eun-byeol · 2025-01-08T06:18:15Z

backend/src/main/java/com/ody/notification/domain/types/AbstractNotification.java

    public Notification toNotification() {
-        return new Notification(id, mate, type, sendAt, status, fcmTopic);
+        return new Notification(null, mate, type, sendAt, status, fcmTopic);


[제안]
Notification id를 받지 않은 생성자에서 null을 넣어주고 있으니 제거해주어도 되겠네요! 괜찮으시면 이 부분도 제가 추후에 반영해두겠습니다~

Suggested change

return new Notification(null, mate, type, sendAt, status, fcmTopic);

return new Notification(mate, type, sendAt, status, fcmTopic);

coli-geonwoo added 7 commits December 4, 2024 17:04

feat: 타입계층 notification 추가

b8b9287

feat: notificationService에서 상위모듈 로직 제거

f77cdf0

test: 리팩터링에 따른 테스트 위치 이동

ec435d7

test: DatabaseCleaner 롤백

211ab56

chore: 스크립트 변경

4d8775d

chore: 스크립트 롤백

2b71319

chore: notification 정적 팩토리 메서드 제거

deb422d

coli-geonwoo added BE REFACTOR labels Dec 14, 2024

coli-geonwoo requested a review from mzeong December 14, 2024 10:16

coli-geonwoo self-assigned this Dec 14, 2024

coli-geonwoo linked an issue Dec 14, 2024 that may be closed by this pull request

refactor: Notification Entity를 타입계층으로 리팩터링 #926

Closed

1 task

coli-geonwoo removed the request for review from mzeong December 14, 2024 10:16

coli-geonwoo commented Dec 14, 2024

View reviewed changes

coli-geonwoo added 2 commits December 22, 2024 19:45

chore: 전송시점 공통로직 개선

2d4decd

refactor: 넛지 알림 시그니처 변경

f10288e

coli-geonwoo requested review from eun-byeol and mzeong December 28, 2024 18:03

fix: 리팩터링에 따른 실패하는 mock 테스트 수정

9b999e1

eun-byeol requested changes Jan 6, 2025

View reviewed changes

mzeong approved these changes Jan 6, 2025

View reviewed changes

coli-geonwoo added 4 commits January 7, 2025 19:22

refactor: 도메인 객체에서 id제거

69a6179

refactor: 불필요한 파라미터 제거

42bb6f1

refactor: this 컨벤션 준수

3f3f69d

refactor: type을 오버라이딩을 통해 초기화

6c22260

eun-byeol approved these changes Jan 8, 2025

View reviewed changes

eun-byeol merged commit 848b444 into develop Jan 8, 2025
3 checks passed

eun-byeol deleted the feature/926 branch January 8, 2025 06:20


		private static final NotificationType type = NotificationType.ENTRY;

		public Entry(Mate mate, Meeting meeting, FcmTopic fcmTopic) {

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

		Nudge nudge = new Nudge(nudgedMate);
		notificationService.sendNudgeMessage(requestMate, nudge);

	private static LocalDateTime calculateSendAt(LocalDateTime sendAt) {
	private LocalDateTime calculateSendAt(LocalDateTime sendAt) {

	return new Notification(null, mate, type, sendAt, status, fcmTopic);
	return new Notification(mate, type, sendAt, status, fcmTopic);

refactor: notification을 타입계층으로 리팩터링 #929

refactor: notification을 타입계층으로 리팩터링 #929

Conversation

coli-geonwoo commented Dec 14, 2024 • edited Loading

🚩 연관 이슈

📝 작업 내용

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

github-actions bot commented Dec 14, 2024 • edited Loading

Test Results

github-actions bot commented Dec 14, 2024 • edited Loading

📝 Test Coverage Report

coli-geonwoo Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eun-byeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzeong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eun-byeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo commented Dec 14, 2024 •

edited

Loading

github-actions bot commented Dec 14, 2024 •

edited

Loading

github-actions bot commented Dec 14, 2024 •

edited

Loading

coli-geonwoo Dec 14, 2024 •

edited

Loading

coli-geonwoo Jan 7, 2025 •

edited

Loading

coli-geonwoo Jan 7, 2025 •

edited

Loading

coli-geonwoo Jan 7, 2025 •

edited

Loading