Feat/#151 알림 - 알림 엔티티 및 구조 추가#152
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough알림 도메인 전체 계층의 스켈레톤 구조를 신규 추가한다. Changes알림 도메인 구조 추가
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/OrgMemberNotificationSetting.java (1)
25-47: ⚡ Quick winboolean 필드들에
@Builder.Default추가를 권장합니다현재 8개의 boolean 설정 필드가 모두
nullable = false로 선언되어 있지만, Builder 패턴 사용 시 기본값이 명확하지 않습니다.잠재적 문제:
// 일부 필드만 설정하고 빌드하면 나머지는 false(primitive 기본값) OrgMemberNotificationSetting setting = OrgMemberNotificationSetting.builder() .orgMember(member) .isMasterEnabled(true) // 나머지 7개 필드는? 의도적으로 false? 실수로 누락? .build();권장 사항:
각 boolean 필드에@Builder.Default를 추가하여 기본값을 명시해주세요. 특히isMasterEnabled는 전체 알림 on/off 스위치이므로 기본값이 중요합니다.📝 수정 제안 예시
+import lombok.Builder.Default; + `@Column`(name = "is_master_enabled", nullable = false) +@Builder.Default private boolean isMasterEnabled = true; // 또는 false (비즈니스 정책에 따라) `@Column`(name = "is_browser_push_enabled", nullable = false) +@Builder.Default private boolean isBrowserPushEnabled = false; `@Column`(name = "is_email_enabled", nullable = false) +@Builder.Default private boolean isEmailEnabled = false; // ... 나머지 필드들도 동일하게 적용팁: 신규 멤버 가입 시 기본적으로 어떤 알림을 켤지/끌지에 대한 정책을 명확히 하고, 그에 맞춰 기본값을 설정하면 좋습니다!
🤖 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/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/OrgMemberNotificationSetting.java` around lines 25 - 47, The boolean fields (isMasterEnabled, isBrowserPushEnabled, isEmailEnabled, isSlackEnabled, isDiscordEnabled, alertBudget80, alertBudget100, alertRapidClicks) in the OrgMemberNotificationSetting entity lack explicit default values when using the Builder pattern, which can lead to unintended false defaults when fields are not explicitly set during object construction. Add the `@Builder.Default` annotation to each of these boolean fields along with their intended default values based on your notification policy to make the defaults explicit and prevent confusion when building instances with partial field assignment.Source: Coding guidelines
🤖 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/whereyouad/WhereYouAd/domains/notification/domain/service/NotificationServiceImpl.java`:
- Line 8: The NotificationServiceImpl class does not declare that it implements
the NotificationService interface. Add the implements keyword to the class
declaration of NotificationServiceImpl to explicitly implement the
NotificationService interface. This allows the class to be injected as a
NotificationService type throughout the application, following Spring Boot's
interface-based dependency injection pattern. Update the class declaration to
include "implements NotificationService" after the class name.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/notification/exception/code/NotificationErrorCode.java`:
- Around line 12-13: The NotificationErrorCode enum has a syntax error with a
stray comma before the semicolon on line 12-13. In Java enums, if there are no
enum constants, the semicolon should follow directly after the enum declaration
with no comma. Remove the comma and keep only the semicolon after the opening
brace of the NotificationErrorCode enum, or alternatively add at least one
sample enum constant (like NOTIFICATION_NOT_FOUND) before the semicolon to make
the skeleton code more useful for future development.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/UserNotification.java`:
- Around line 23-25: The isRead field in the UserNotification entity class needs
an explicit default value for the Builder pattern. The `@ColumnDefault` annotation
only affects DDL generation and does not apply when objects are created using
`@Builder`. Add the `@Builder.Default` annotation to the isRead field alongside the
existing `@ColumnDefault` annotation to ensure the default value of false is
applied both when the database table is created and when UserNotification
objects are instantiated through the Builder pattern without explicitly setting
the isRead field.
- Around line 10-16: The UserNotification entity class is missing timestamp
fields to track when notifications are created and modified. Add audit timestamp
tracking to the UserNotification class by either inheriting from BaseEntity
(like OrgMemberNotificationSetting and OrgNotificationSetting do) or by applying
the `@EntityListeners`(AuditingEntityListener.class) annotation along with
`@CreatedDate` and `@LastModifiedDate` fields on appropriate LocalDateTime
properties (similar to how the Notification entity implements this). Choose the
approach that aligns with your project's existing patterns for audit fields.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/OrgMemberNotificationSetting.java`:
- Around line 25-47: The boolean fields (isMasterEnabled, isBrowserPushEnabled,
isEmailEnabled, isSlackEnabled, isDiscordEnabled, alertBudget80, alertBudget100,
alertRapidClicks) in the OrgMemberNotificationSetting entity lack explicit
default values when using the Builder pattern, which can lead to unintended
false defaults when fields are not explicitly set during object construction.
Add the `@Builder.Default` annotation to each of these boolean fields along with
their intended default values based on your notification policy to make the
defaults explicit and prevent confusion when building instances with partial
field assignment.
🪄 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
Run ID: c8d8fb62-c1d4-4e37-9339-ae977bf4e983
📒 Files selected for processing (18)
src/main/java/com/whereyouad/WhereYouAd/domains/notification/application/dto/request/NotificationRequest.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/application/dto/response/NotificationResponse.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/application/mapper/NotificationConverter.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/domain/constant/.gitkeepsrc/main/java/com/whereyouad/WhereYouAd/domains/notification/domain/service/NotificationService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/domain/service/NotificationServiceImpl.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/exception/NotificationException.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/exception/code/NotificationErrorCode.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/Notification.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/OrgMemberNotificationSetting.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/OrgNotificationSetting.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/UserNotification.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/repository/NotificationRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/repository/OrgMemberNotificationSettingRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/repository/OrgNotificationSettingRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/repository/UserNotificationRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/presentation/NotificationController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/presentation/docs/NotificationControllerDocs.java
| @ColumnDefault("false") | ||
| @Column(name = "is_read", nullable = false) | ||
| private boolean isRead; |
There was a problem hiding this comment.
@Builder 사용 시 boolean 필드 기본값 설정이 필요합니다
@ColumnDefault("false")는 DDL(테이블 생성 SQL)에만 영향을 주고, Java 객체를 @Builder로 생성할 때는 적용되지 않습니다.
문제 상황:
UserNotification notification = UserNotification.builder()
.notification(notif)
.user(user)
.build();
// isRead는 false가 아니라 Java의 primitive boolean 기본값 false가 됨
// 하지만 Builder 패턴 사용 시 명시적으로 설정하지 않으면 의도가 불명확함해결 방법:
@Builder.Default를 함께 사용하여 Builder 패턴에서도 기본값이 명확히 설정되도록 해주세요.
🛠️ 수정 제안
+import lombok.Builder.Default;
+
`@ColumnDefault`("false")
`@Column`(name = "is_read", nullable = false)
+@Builder.Default
private boolean isRead = false;이렇게 하면:
- DDL 생성 시:
DEFAULT false적용 ✓ - Builder 사용 시: 명시하지 않으면
false적용 ✓ - 코드 가독성: 기본값이
false임을 명확히 표현 ✓
🤖 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/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/UserNotification.java`
around lines 23 - 25, The isRead field in the UserNotification entity class
needs an explicit default value for the Builder pattern. The `@ColumnDefault`
annotation only affects DDL generation and does not apply when objects are
created using `@Builder`. Add the `@Builder.Default` annotation to the isRead field
alongside the existing `@ColumnDefault` annotation to ensure the default value of
false is applied both when the database table is created and when
UserNotification objects are instantiated through the Builder pattern without
explicitly setting the isRead field.
Source: Coding guidelines
ojy0903
left a comment
ojy0903
left a comment
There was a problem hiding this comment.
P3: 헉 엔티티 바로 만들어주셨네요...! 고생하셨습니다 👍
디스코드 / 슬랙 설정 조직 단위로 빼서 설정한 것 좋은 것 같습니다! 팀 단위로 디스코드나 슬랙 채널에 알람 가는게 적절할 거 같아서 지금 구조가 괜찮을 것 같아요..!!
한가지 추가 제안할거는 저희 알람 보내는거에서 예산 80%, 100% 말고 50% 소진 시 알람도 있었어서 OrgMemberNotificationSetting 에 boolean alertBudget50 으로 하나 필드 추가하는건 어떨까요...?
추가로 개인적으로 헷갈리는 건데 저희 피그마에서 마스터 컨트롤이 활성화 되어있으면 (isMasterEnabled = true) 알람을 수신 받는 상태인거고, 비활성화이면 (isMasterEnabled = false) 알람을 안받는 상태라는 의미가 맞을까요...?? |
넵 저도 그렇게 생각했습니다! |
jinnieusLab
left a comment
jinnieusLab
left a comment
There was a problem hiding this comment.
P3: 고생하셨습니다! 알림 기능 구현할 수 있도록 엔티티랑 구조 작성해주셔서 편리하게 개발할 수 있을 듯 하네요!
다만 현재 알림의 공통 내용을 담는 Notification 테이블과 별개로, 개별 알림이 잘 보내졌는지 각 멤버에 대해서 상태를 확인할 개별 알림 발송 테이블이 없어서 NotificationDelivery와 같은 클래스도 추가 작성하는 건 어떨까요? Kafka를 활용할 때 개별적으로 수신 성공 여부를 확인하고, 알림 재전송 등 관리에 좋을 것 같아서요! 추후에 웹사이트 내 알림 창을 웹소켓으로 구현하게 된다면 그 때도 유용할 것 같습니다..!
좋습니다! NotificationDelivery 엔티티 만들어서 잘 보내졌는지 확인 + 실패한 경우 횟수랑 이유 저장하는 엔티티 만들어서 추가했습니다! |
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/whereyouad/WhereYouAd/domains/notification/persistence/entity/NotificationDelivery.java`:
- Around line 29-31: The status field in the NotificationDelivery entity is
marked as non-nullable but lacks a default value, making it possible to create
instances with status = null when using the Builder pattern, which will cause
database constraint violations. Add a default initialization of the status field
to DeliveryStatus.PENDING to ensure the field always has a valid value when
entities are created through the Builder. This applies to both the field
declaration and the corresponding builder method to enforce the constraint at
the object creation level.
- Around line 50-52: The current `NotificationDelivery` entity references the
`User` through a `user_id` foreign key, but the requirement for membership-based
delivery tracking requires mapping to the OrgMember entity instead, as a single
user can belong to multiple organizations. Replace the `user` field of type
`User` with a field that references the membership/OrgMember entity through a
`membership_id` foreign key in the `@JoinColumn` annotation. This ensures proper
association between notification deliveries and specific membership contexts,
eliminating ambiguity for multi-organization users. The LAZY fetch type
configuration should be retained as it is already appropriate.
- Around line 13-14: The NotificationDelivery entity lacks a unique constraint
to prevent duplicate delivery records for the same target/channel combination,
which can cause multiple identical notifications to be created in concurrent
scenarios. Add a unique constraint to the `@Table` annotation for the
NotificationDelivery class that enforces uniqueness on the appropriate column
combination (such as membership_id or the target/channel fields) to protect
against concurrent duplicate insertions at the database level.
🪄 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
Run ID: 4237eaaa-9fee-4bf0-870a-c3d28f02ebb3
📒 Files selected for processing (4)
src/main/java/com/whereyouad/WhereYouAd/domains/notification/domain/constant/DeliveryChannel.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/domain/constant/DeliveryStatus.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/NotificationDelivery.javasrc/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/repository/NotificationDeliveryRepository.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/whereyouad/WhereYouAd/domains/notification/domain/constant/DeliveryStatus.java
| @Table(name = "notification_delivery") | ||
| @Getter |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
중복 배송 레코드 방지를 위한 유니크 제약이 필요합니다.
현재 테이블 정의에는 동일 대상/채널 조합 중복을 막는 제약이 없어, 동시성 상황에서 동일 알림이 여러 건 생성될 수 있습니다. 재시도 카운트 기반 설계라면 DB 레벨 유니크 키로 보호하는 게 맞습니다.
제안 diff
-@Table(name = "notification_delivery")
+@Table(
+ name = "notification_delivery",
+ uniqueConstraints = {
+ `@UniqueConstraint`(
+ name = "uk_notification_delivery_target_channel",
+ columnNames = {"notification_id", "user_id", "channel"}
+ )
+ }
+)membership_id로 전환한다면 해당 컬럼으로 제약 컬럼도 함께 바꿔주세요.
🤖 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/main/java/com/whereyouad/WhereYouAd/domains/notification/persistence/entity/NotificationDelivery.java`
around lines 13 - 14, The NotificationDelivery entity lacks a unique constraint
to prevent duplicate delivery records for the same target/channel combination,
which can cause multiple identical notifications to be created in concurrent
scenarios. Add a unique constraint to the `@Table` annotation for the
NotificationDelivery class that enforces uniqueness on the appropriate column
combination (such as membership_id or the target/channel fields) to protect
against concurrent duplicate insertions at the database level.
3 participants
📌 관련 이슈
🚀 개요
📄 작업 내용
Notification
알림 원본 데이터를 저장하는 엔티티입니다.
created_at만 필요하여BaseEntity대신@EntityListeners+@CreatedDate를 사용updated_at컬럼 생성 방지link_url: 알림 클릭 시 이동할 URL (nullable)metadata_json: 추가 정보를 JSON 문자열 형태로 저장Organization) 기준으로 알림 관리UserNotification
Notification과User를 연결하는 중간 엔티티이며, 사용자별 읽음 상태를 관리합니다.Notification ↔ UserN:M 관계 해소isRead,readAt을 통해 개별 읽음 여부 관리Notification측 컬렉션은 제거@ManyToOne단방향 연관관계만 사용OrgMemberNotificationSetting
조직 멤버별 알림 수신 설정을 관리합니다.
OrgMember와 1:1 식별 관계(Identifying Relationship)@MapsId를 사용하여membership_id를 PK이자 FK로 사용관리 항목
※ Slack·Discord Webhook URL은 조직 단위 설정(
OrgNotificationSetting)에서 관리하고, 본 엔티티는 수신 여부만 관리합니다.OrgNotificationSetting
조직 단위 알림 채널 연동 정보를 관리합니다.
Organization과 1:1 식별 관계org_id를 PK이자 FK로 사용 (@MapsId)관리 항목
※ URL이
null이면 해당 채널은 미연동 상태를 의미합니다.📸 스크린샷 / 테스트 결과 (선택)
ERD
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
알림 설정 관련해서 엔티티 구조 설계해봤습니다.
OrgMemberNotificationSetting에서는 개인별 설정(boolean 값으로 해당 채널에 대한 알림을 받을지 말지)을,OrgNotificationSetting은 조직에서 슬랙이나 디스코드 연동하면 관리하는 값입니다.혹시 수정하거나 추가해야할 필드가 있다면 알려주시면 감사하겠습니다!
Summary by CodeRabbit
/api/notificationREST 엔드포인트 및 알림 관련 DTO/예외/컨버터 구조 추가