[FEAT] 알림 도메인 api 구현 - #54
Merged
Merged
Conversation
Closed
6 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough알림 엔티티의 사용자 연관관계와 생성 검증이 추가되었고, 이벤트 기반 저장·중복 방지·재시도와 사용자별 조회·읽음 처리 API가 구현되었다. Changes알림 기능
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NotificationEvent
participant NotificationEventListener
participant UserRepository
participant NotificationRepository
NotificationEvent->>NotificationEventListener: 커밋 후 비동기 이벤트 전달
NotificationEventListener->>NotificationRepository: 최근 동일 알림 확인
NotificationEventListener->>UserRepository: 사용자 조회
NotificationEventListener->>NotificationRepository: 알림 생성 및 저장
NotificationRepository-->>NotificationEventListener: 저장 결과 또는 예외
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rkdehdrbs7885-oss
requested review from
kimyw1018,
on1yoneprivate,
ownue and
p1001q
July 25, 2026 19:55
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/mr/domain/notification/repository/NotificationRepository.java`:
- Line 13: Update the NotificationRepository method name from
findAllByUserIdAndDeletedAtIsNull to use the nested User.userId property,
specifically findAllByUserUserIdAndDeletedAtIsNull, while preserving its
existing parameters and return type.
In `@src/main/java/com/mr/domain/notification/service/NotificationService.java`:
- Around line 28-30: Update readNotification to retrieve the notification
through a repository query that requires deletedAt to be null, instead of using
findById. Preserve the existing NOTIFICATION_NOT_FOUND exception so soft-deleted
notifications are rejected before markAsRead() runs.
- Around line 21-23: Update NotificationService.getNotificationList to call the
repository method using the nested Notification.user.userId property, renaming
findAllByUserIdAndDeletedAtIsNull to findAllByUserUserIdAndDeletedAtIsNull or
its underscore-delimited equivalent, and keep the existing deletedAt filter and
pagination behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e5dbdf5-cc2e-485f-88d0-94babc3e1bff
📒 Files selected for processing (7)
src/main/java/com/mr/domain/notification/controller/NotificationController.javasrc/main/java/com/mr/domain/notification/dto/res/NotificationDTO.javasrc/main/java/com/mr/domain/notification/dto/res/NotificationListDTO.javasrc/main/java/com/mr/domain/notification/entity/Notification.javasrc/main/java/com/mr/domain/notification/exception/NotificationErrorStatus.javasrc/main/java/com/mr/domain/notification/repository/NotificationRepository.javasrc/main/java/com/mr/domain/notification/service/NotificationService.java
p1001q
requested changes
Jul 25, 2026
p1001q
left a comment
p1001q
left a comment
Contributor
There was a problem hiding this comment.
💡 참고
src/main/java/com/mr/domain/notification/dto/res/NotificationListDTO.java—Page<Notification>을 씁니다(getTotalPages(),getTotalElements()). 팀 컨벤션(docs/컨벤션.md"목록 조회는Page대신Slice를 우선 고려한다")상Page는COUNT(*)쿼리가 추가로 나가는데, 응답 DTO에totalPage/totalElements가 실제로 프론트에서 쓰이는 값이면(페이지 번호 UI 등)Page가 맞는 선택이고 문제없습니다. 다만 "더보기"/무한스크롤 방식이라면Slice로 바꾸는 게 더 저렴합니다 — 프론트 쪽에 페이지 번호 UI가 있는지만 확인하고 넘어가면 될 것 같아요.NotificationDTO/NotificationListDTO네이밍이 다른 도메인 예시(LearningProgressResponseDTO등)의Xxx**Response**DTO접미사와 다릅니다. 강제 규칙은 아니라 blocking 사유는 아니고, 통일성 차원에서 참고만 해주세요.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/test/java/com/mr/domain/notification/service/NotificationServiceTest.java (1)
69-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win알림 미존재 예외 케이스도 추가해야 합니다.
NotificationService.readNotification은findByIdAndDeletedAtIsNull(...)가 비어 있으면NOTIFICATION_NOT_FOUND를 던지는 계약인데, 현재 테스트는 성공과 권한 없음만 검증합니다.Optional.empty()를 반환하는 경우GeneralException과 정확한 오류 상태가 유지되는지 고정하세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/mr/domain/notification/service/NotificationServiceTest.java` around lines 69 - 74, NotificationServiceTest의 readNotification 테스트에 알림 미존재 케이스를 추가하세요. notificationRepository.findByIdAndDeletedAtIsNull(...)가 Optional.empty()를 반환하도록 설정하고, NotificationService.readNotification이 GeneralException을 던지는지와 예외의 오류 상태가 NOTIFICATION_NOT_FOUND인지 검증하세요.src/test/java/com/mr/domain/notification/controller/NotificationControllerTest.java (1)
63-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win서비스 호출 인자를 검증해 인증 계약을 고정하세요.
현재 테스트는 대부분 응답의 성공 여부만 확인합니다. 특히
doNothing()은 호출되지 않아도 기본적으로 통과할 수 있으므로, 잘못된userId나notificationId가 전달되어도 회귀를 잡지 못합니다. 각 테스트에서verify(...)로 서비스 호출을 검증하고, 목록 조회에서는Pageable의 page/size도 확인하세요.Also applies to: 79-86, 93-100, 107-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/mr/domain/notification/controller/NotificationControllerTest.java` around lines 63 - 72, Update the NotificationControllerTest cases around notificationService calls to verify each service invocation with verify(...), including tests using doNothing(). For the list retrieval test, verify the expected userId and assert the passed Pageable has page 0 and size 10; apply equivalent argument verification to the notification creation, read, and deletion cases using their expected userId and notificationId values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/mr/domain/notification/controller/NotificationControllerTest.java`:
- Around line 41-50: Update the MockMvc request setup in
NotificationControllerTest so each protected API request explicitly receives the
test SecurityContext via
SecurityMockMvcRequestPostProcessors.securityContext(...) or an equivalent
`@WithSecurityContext` configuration, ensuring `@AuthenticationPrincipal` resolves
the mocked user. If retaining the `@BeforeEach` SecurityContextHolder setup, clear
it after each test with SecurityContextHolder.clearContext().
- Line 44: NotificationControllerTest의 mockUserDetails 초기화에서 사용하는 mock 메서드가
컴파일되도록 org.mockito.Mockito.mock 정적 import를 추가하세요.
In
`@src/test/java/com/mr/domain/notification/service/NotificationServiceTest.java`:
- Around line 57-58: Update the fixture in NotificationServiceTest around
findAllByUser_UserIdAndDeletedAtIsNull to construct and return a
Page<Notification>, such as a PageImpl, instead of a SliceImpl. Keep the
existing notification, pageRequest, and non-final-page setup while matching the
repository and NotificationService Page<Notification> contract.
---
Nitpick comments:
In
`@src/test/java/com/mr/domain/notification/controller/NotificationControllerTest.java`:
- Around line 63-72: Update the NotificationControllerTest cases around
notificationService calls to verify each service invocation with verify(...),
including tests using doNothing(). For the list retrieval test, verify the
expected userId and assert the passed Pageable has page 0 and size 10; apply
equivalent argument verification to the notification creation, read, and
deletion cases using their expected userId and notificationId values.
In
`@src/test/java/com/mr/domain/notification/service/NotificationServiceTest.java`:
- Around line 69-74: NotificationServiceTest의 readNotification 테스트에 알림 미존재 케이스를
추가하세요. notificationRepository.findByIdAndDeletedAtIsNull(...)가 Optional.empty()를
반환하도록 설정하고, NotificationService.readNotification이 GeneralException을 던지는지와 예외의 오류
상태가 NOTIFICATION_NOT_FOUND인지 검증하세요.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08b10661-c251-4839-9b52-dc4cc5034243
📒 Files selected for processing (2)
src/test/java/com/mr/domain/notification/controller/NotificationControllerTest.javasrc/test/java/com/mr/domain/notification/service/NotificationServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/mr/domain/notification/service/NotificationEventListener.java`:
- Around line 28-31: Update NotificationEventListener.handleNotificationEvent to
add a durable retry or recovery path for failures during findById/save, such as
an outbox or external queue, and record failure metrics or equivalent tracking.
Ensure retries are idempotent so repeated notification events do not create
inconsistent or duplicate records, while preserving the existing post-commit
asynchronous flow.
- Around line 33-49: Replace the time-and-title duplicate check in
NotificationEventListener with event-identity-based idempotency: add a unique
event ID to NotificationEvent, persist it on Notification, and enforce a
database UNIQUE constraint or unique index on (user_id, event_id). Update the
notification save flow to use UPSERT semantics so concurrent deliveries create
at most one record, handling the duplicate-key outcome without creating another
notification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 68cc8df8-3839-4169-87b4-d072079c91e9
📒 Files selected for processing (3)
src/main/java/com/mr/domain/notification/repository/NotificationRepository.javasrc/main/java/com/mr/domain/notification/service/NotificationEventListener.javasrc/main/java/com/mr/global/event/NotificationEvent.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/mr/domain/notification/repository/NotificationRepository.java
kimyw1018
approved these changes
Jul 27, 2026
kimyw1018
left a comment
kimyw1018
left a comment
Contributor
There was a problem hiding this comment.
수고 하셨습니다!! 리뷰 확인해주세요~
승인 해두겠습니다
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit
Summary