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 #104] 알림 생성 API Refactor #105

Merged
merged 10 commits into from
Sep 13, 2024
Merged

Conversation

dudxo
Copy link
Collaborator

@dudxo dudxo commented Sep 12, 2024

관련 이슈

📑 작업 상세 내용

  • 댓글 작성 || 채택 시 알림 생성 로직 호출 -> 알림 생성 및 저장 Evnet Pulish 되도록 변경

💫 작업 요약

  • 알림 생성 로직 호출 -> Event 처리로 변경

🔍 중점적으로 리뷰 할 부분

  • Event 객체를 class로 만들려다가 DTO 스타일인것 같아 record로 만들었는데 어떠신가요?

@dudxo dudxo added the ♻️ refactor 코드 기능 유지하되 코드 변경 label Sep 12, 2024
@dudxo dudxo requested a review from hyun2371 September 12, 2024 13:21
@dudxo dudxo self-assigned this Sep 12, 2024
@dudxo dudxo linked an issue Sep 12, 2024 that may be closed by this pull request
1 task
@dudxo dudxo changed the title Refactor/#104/notification event [refactor #104] 알림 생성 API Refactor Sep 12, 2024
Copy link

github-actions bot commented Sep 12, 2024

Code Coverage

Overall Project 84.75% 🍏
Files changed 100% 🍏

File Coverage
NotificationController.java 100% 🍏
NotificationCustomImpl.java 94.39% 🍏
AnswerService.java 89.93% 🍏
NotificationService.java 74.12% 🍏

Copy link

github-actions bot commented Sep 12, 2024

Test Results

 23 files   23 suites   12s ⏱️
101 tests 101 ✅ 0 💤 0 ❌
102 runs  102 ✅ 0 💤 0 ❌

Results for commit e21637b.

♻️ This comment has been updated with latest results.

Copy link
Member

@hyun2371 hyun2371 left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다~

Notification notification = Notification.of(type, targetId, triggerMemberId, toMember);
@EventListener
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
@Transactional(propagation = Propagation.REQUIRES_NEW)
Copy link
Member

Choose a reason for hiding this comment

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

찾아보니까 두 어노테이션의 성격이 상반된 것 같은데 같이 사용하면 어떻게 동작하는지 궁금합니다!

- EventListener: 트랜잭션에 의존하지 않고, 이벤트 발생 시 즉시 처리
- TransactionalEventListener: 트랜잭션 결과에 따라 이벤트 처리

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

확인해보니 중복 어노테이션을 작성했었네요.
잘 몰랐던 부분인데 짚어주셔서 감사합니다!

Long targetId,
Long triggerMemberId,
Member toMember
) {
Copy link
Member

Choose a reason for hiding this comment

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

개인적으로 toMember를 targetMember로 네이밍해도 좋을 것 같아요!

@dudxo dudxo merged commit db1d42e into dev Sep 13, 2024
3 checks passed
@dudxo dudxo deleted the refactor/#104/notification-event branch September 13, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ refactor 코드 기능 유지하되 코드 변경
Projects
None yet
Development

Successfully merging this pull request may close these issues.

♻️ 알림 생성 API
2 participants