Skip to content

✨ refactor: 알림 리팩터링 및 테스트#31

Merged
y3binchoi merged 29 commits intodevelopfrom
refactor/#10-#20-notification-refactor
Apr 17, 2025
Merged

✨ refactor: 알림 리팩터링 및 테스트#31
y3binchoi merged 29 commits intodevelopfrom
refactor/#10-#20-notification-refactor

Conversation

@y3binchoi
Copy link
Collaborator

@y3binchoi y3binchoi commented Apr 10, 2025

📌 변경 사항 (What’s changed?)

  • NotificationEntity 이름 변경
  • NotificationEventDispatcher, NotificationEventHandler 로 알림 생성에 전략 패턴 적용
  • Notification 서비스단 테스트

⚙️ 변경 이유 (Why?)

  • 기존 테스트 코드가 없어서, 리팩토링 전에 서비스 레이어 단위 테스트 추가
  • 레거시는 알림 메시지가 하드 코딩 되어있어서 이를 템플릿으로 분리함
  • 알림 생성/저장/전송이 서비스와 도메인에 결합되어 헤비한 서비스단을 전략 패턴으로 의존성 역전
  • 알림은 주요 비즈니스 로직에서 벗어나므로 전부 비동기 이벤트로 변경

✅ 테스트 방법 (How to test?)

  • NotificationServiceTest로 Notification 기본 CRUD 테스트
  • NotificationEventDispatcher로 비동기 이벤트 테스트

🤔 기타 참고 사항

  • 이벤트 구현 순서
  1. Event 도메인 만들기
  2. NotifiationType에 적절한 알림 타입 만들기
  3. NotifiationEventHandler에 알림 생성 로직 구현하기
  • ⚠️ 위의 세 가지를 구현하면서 이벤트 이름을 통일하여 작성해주세요. 예) ClubJoinEvent, NotificationType.CLUB_JOIN, ClubJoinNotificationEventHandler

💬리뷰 요구사항(선택)

- 네이밍 일관성을 위해 엔티티는 단수형, 테이블명은 복수형으로 작성함

(cherry picked from commit fd78ebbfbb9da3c77c1aa60f46c19c100b5dced0)

# Conflicts:
#	src/main/generated/com/example/moim/notification/entity/QNotification.java
#	src/main/generated/com/example/moim/user/entity/QUser.java
#	src/main/java/com/example/moim/notification/entity/Notifications.java
#	src/main/java/com/example/moim/user/entity/User.java
(cherry picked from commit 4243cbd83d7ebfe618208cf184dc258fd412d1d6)
(cherry picked from commit 16c7c990b399a1fc010207dd6d96d69c5e4f4ac1)
(cherry picked from commit c7c8f6903cfd69c3fad54dd8f458c823d27bbc67)
(cherry picked from commit 9cbea0dbe4d509aa3141a49c298134a9dcc70dcb)
(cherry picked from commit eb5511c8e0e3b8512b3453e141544d2736b65e5c)

# Conflicts:
#	src/main/java/com/example/moim/notification/controller/NotificationController.java
(cherry picked from commit 877b3f29542ab31d4bb6518853b7de181c93f170)

# Conflicts:
#	.gitignore
(cherry picked from commit 0d8343af1e7ea12119beab94ce3277473ca54857)
(cherry picked from commit 35024db4bf1390077bfefe37b33c1c21e03fd1d9)
- 이미 Notification을 명시한 클래스 안에 있으므로 메서드에서 또 언급할 필요 없음. 의미 중복임.
- 비즈니스 로직과 알림 전송/저장 로직의 책임 분리하여 도메인 로직의 복잡도 낮춤
- 푸시 알림은 외부 호출이라 느릴 수 있어서 비동기로 응답 속도 개선/요청 처리 성능 향상
- 각 이벤트마다 메시지를 하드코딩 하지 않고 템플릿 기반으로 생성하도록 변경
- 근데 테스트 할 때 원래 빌더를 이렇게 많이 썼던가?
@y3binchoi y3binchoi added the ♻️ refactor Refactor code label Apr 10, 2025
@y3binchoi y3binchoi self-assigned this Apr 10, 2025
@y3binchoi y3binchoi requested review from 5nam and hojooo as code owners April 10, 2025 01:19
Copy link
Collaborator

@5nam 5nam left a comment

Choose a reason for hiding this comment

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

알림 부분이 어려웠을텐데 수고 많으셨습니다!

@@ -0,0 +1,37 @@
package com.example.moim.notification.entity;
Copy link
Collaborator

Choose a reason for hiding this comment

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

알림 타입을 ENUM 으로 구현한 것이 한눈에 알림 종류를 볼 수 있어서 깔끔하다고 생각합니다 👍


@Repository
@RequiredArgsConstructor
public class NotificationRepositoryImpl implements
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분에서 NotificationRepository 로만 기능 구현이 가능할 것 같은데, 왜 NotificationRepositoryImpl 로 분리해서 Override 로 구현하셨는지 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

테스트를 작성하는 과정에서 필요했어서 이렇게 바꿨는데 지금 결과적으론 안쓰게 되었네요.. 다시 바꾸는게 좋을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

JPA 에서 제공하는 쿼리를 그대로 사용하는 거라면 없어도 될 것 같습니다! 다만 뭔가 커스텀해서 사용하는 거라면 이대로 두는 게 맞는 것 같아요!

import com.example.moim.notification.entity.NotificationEntity;
import java.util.List;

public interface NotificationStrategy<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 알림 기능 분기 처리한 것이 Strategy 보다는 Handler 라는 네이밍이 더 알맞을 거 같습니다!

Strategy 는 행동 자체가 바뀌어야 사용하는 걸로 알고 있는데(SMS 알림 발송, 메일 알림 발송, 앱 푸시 알림 발송 등등)
저희는 앱 푸시 알림 발송만 사용해서 이벤트/타입에 따른 분기 처리로 보는게 더 알맞을 거 같은데, 이 경우에는 Handler 구조라고 볼 수 있을 거 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제가 놓친 부분을 짚어주셔서 감사해요!! 반영해서 커밋 하겠습니다!!

void shouldCallFirebaseMessaging_andMarkAsSent() throws FirebaseMessagingException {
// given
User user = User.builder().fcmToken("valid_token").build();
NotificationEntity notification = NotificationEntity.builder()
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 중복 코드는 @beforeeach 로 한번에 생성해두면 좋을 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이게 유효하지 않은 토큰이 들어가는 경우도 있어서 각각 given 부분에 선언을 했어요!

@DisplayName("사용자에게 읽지 않은 알림이 없으면 false 반환한다")
void shouldReturnFalseWhenNoUnreadNotifications() {
// given
User user = new User();
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 @beforeeach 로 빼서 한번에 생성해두면 좋을 거 같습니다!


@Component
@RequiredArgsConstructor
public class NotificationEventDispatcher {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handler 패턴으로 알림 구현 하신 점 정말 좋습니다!

배워갑니다 👍

@hojooo
Copy link
Collaborator

hojooo commented Apr 14, 2025

앗 확인이 늦었네요! 알림 쪽 복잡했을텐데 수고하셨습니다!! 남의님이 꼼꼼하게 잘 해주셨네요..!

@y3binchoi y3binchoi merged commit 2160dee into develop Apr 17, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in match-day-backend Apr 17, 2025
@y3binchoi y3binchoi deleted the refactor/#10-#20-notification-refactor branch April 17, 2025 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

♻️ refactor Refactor code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants