Closed
Conversation
|
Caution Review failedThe pull request is closed. Walkthrough사용자의 알림 설정 조회/수정 API와 notificationEnabled 필드 추가, 일일·주간 학습 알림 스케줄러·발송 서비스와 HTML 이메일 템플릿 빌더, 관련 저장소 쿼리(활동 집계 및 검색)의 구현을 추가합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as NotificationScheduler
participant Service as NotificationService
participant UserRepo as UserRepository
participant DataRepos as Data Repositories
participant Template as NotificationTemplateBuilder
participant EmailSvc as EmailService
participant SMTP as Email Provider
Scheduler->>Service: sendDailyNotifications()
Service->>UserRepo: findByNotificationEnabledAndAuthType(EMAIL)
UserRepo-->>Service: List<UserEntity>
loop 각 사용자 처리
Service->>DataRepos: 집계 쿼리(뉴스 저장, 퀴즈 시도/복습, 기사 뷰) for user
DataRepos-->>Service: flags / counts
Service->>Template: buildDailyEmail(flags)
Template-->>Service: HTML content
Service->>EmailSvc: sendHtmlEmail(email, subject, html)
EmailSvc->>SMTP: 전송 요청
SMTP-->>EmailSvc: 전송 결과
end
sequenceDiagram
participant Client as Client
participant Controller as MypageController
participant Service as MypageService
participant Repo as UserRepository
participant Entity as UserEntity
Client->>Controller: GET /api/mypage/me/notification
Controller->>Service: getNotificationSetting(userId)
Service->>Repo: findById(userId)
Repo-->>Entity: UserEntity
Entity-->>Service: notificationEnabled
Service-->>Controller: NotificationResponse
Controller-->>Client: DataResponse<NotificationResponse>
Client->>Controller: PUT /api/mypage/me/notification (UpdateNotificationRequest)
Controller->>Service: updateNotificationSetting(userId, request)
Service->>Repo: findById(userId)
Repo-->>Entity: UserEntity
Service->>Entity: updateNotificationEnabled(enabled)
Service-->>Controller: void
Controller-->>Client: DataResponse<Void>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
📝 상세 검토 포인트좋은 점:
주의할 사항:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 8
🤖 Fix all issues with AI agents
In
`@src/main/java/com/finsight/finsight/domain/auth/domain/service/EmailService.java`:
- Around line 31-40: The sendNotificationEmail(String email, String subject,
String content) method in EmailService is unused and should be removed to reduce
dead code; search the codebase to confirm there are no callers, then delete the
sendNotificationEmail method and any now-unused imports or JavaDoc related to
it, leaving sendVerificationEmail and sendHtmlEmail untouched; if you expect to
reuse plaintext emailing later instead mark it `@Deprecated` and add a TODO
comment instead of deleting.
In
`@src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java`:
- Around line 37-62: In NotificationService.sendDailyNotifications, avoid the
empty catch: log the exception and contextual info (at least user.getUserId()
and/or email) when handling failures for each user so errors aren't swallowed;
update the catch block to call the class logger (e.g., log.warn or log.error)
with a clear message including userId/email and the exception to aid debugging,
while keeping the loop behavior to continue to next user; reference methods
getEmailFromUser, templateBuilder.buildDailyEmail, and
emailService.sendHtmlEmail to indicate where failures should be logged.
- Around line 68-93: In sendWeeklyNotifications (NotificationService) the catch
block currently swallows exceptions; update it to log failures with context and
the exception (e.g., include user.getUserId() and the resolved email from
getEmailFromUser(user)) using the class logger before continuing so individual
user failures are recorded; keep the existing behavior of continuing to next
user after logging.
- Line 39: The current query used by UserRepository's
findByNotificationEnabledAndAuthType returns UserEntity without fetching
userAuths, causing N+1 queries when NotificationService calls
getEmailFromUser(user) which accesses user.getUserAuths(); update the repository
query to use a FETCH JOIN to eagerly load u.userAuths (i.e., LEFT JOIN FETCH
u.userAuths ua) while keeping the same authType filter so the users and their
userAuths are loaded in one query and the N+1 problem is eliminated.
In
`@src/main/java/com/finsight/finsight/domain/notification/infrastructure/template/NotificationTemplateBuilder.java`:
- Around line 23-32: The current else branch in NotificationTemplateBuilder
produces a "saved news awaiting quiz" message for any case not covered by the
first two branches, which is incorrect when isNewsSaved is false but
isQuizSolved or isQuizReviewed is true; update the branching to explicitly
handle the remaining combinations instead of a catch-all else: after the
existing checks for (!isNewsSaved && !isQuizSolved && !isQuizReviewed) and
(isNewsSaved && isQuizSolved && isQuizReviewed), add explicit branches such as
(isNewsSaved) to keep the "saved news awaiting quiz" message, (isQuizSolved) to
produce a message acknowledging a completed quiz but no saved news, and
(isQuizReviewed) to produce a message for only review—set the appropriate
message and emoji values in each branch so message/emoji reflect the actual
booleans isNewsSaved, isQuizSolved, isQuizReviewed.
In
`@src/main/java/com/finsight/finsight/domain/user/persistence/entity/UserEntity.java`:
- Around line 71-72: In updateNotificationEnabled(Boolean enabled) in
UserEntity, guard against null before assigning to the non-nullable field
notificationEnabled; if enabled is null, set notificationEnabled to a safe
default (e.g., false) or throw an IllegalArgumentException — implement a
null-check at the start of the method and assign either enabled.booleanValue()
or the chosen default to this.notificationEnabled to avoid nullable=false
constraint violations at runtime.
- Around line 55-57: The UserEntity field notificationEnabled is declared
non-nullable but existing DB rows aren’t handled; add a proper migration or
config: create a Flyway/Liquibase migration that adds the notification_enabled
column with a default value (e.g., true) and backfills existing rows, then alter
the column to NOT NULL (or add it NOT NULL with DEFAULT then remove default), or
if you rely on JPA DDL only, ensure spring.jpa.hibernate.ddl-auto is set
appropriately for your environment (validate in prod) and do an explicit
backfill migration instead of relying on `@Builder.Default`; locate
UserEntity.notificationEnabled and implement the migration/backfill and
configuration change accordingly.
In
`@src/main/java/com/finsight/finsight/domain/user/persistence/repository/UserRepository.java`:
- Around line 18-27: The JPQL query in
UserRepository.findByNotificationEnabledAndAuthType selects UserEntity u with a
JOIN on u.userAuths which can return duplicate UserEntity rows when a user has
multiple matching userAuths; update the query to return unique users (e.g.,
change the projection to SELECT DISTINCT u) so the method
findByNotificationEnabledAndAuthType (and downstream callers like
NotificationService.sendDailyNotifications/sendWeeklyNotifications) receive each
UserEntity only once.
🧹 Nitpick comments (3)
src/main/java/com/finsight/finsight/domain/notification/infrastructure/template/NotificationTemplateBuilder.java (1)
13-14: TODO 주석 확인했어요.프론트 배포 후 실제 URL로 변경이 필요하다는 점 잘 기록해두셨네요. 이 부분은 배포 환경별로 다를 수 있으니, 추후
application.yml에서 설정값으로 주입받는 방식(@Value)으로 변경하면 환경별 관리가 더 편해질 거예요.이 작업을 위한 이슈를 열어드릴까요?
src/main/java/com/finsight/finsight/domain/auth/domain/service/EmailService.java (1)
45-58: HTML 이메일 발송 구현이 잘 되었어요!
MimeMessageHelper를 사용한 HTML 이메일 발송이 올바르게 구현되었고,UTF-8인코딩도 적절히 설정되어 있네요.한 가지 개선 제안: 현재
RuntimeException으로 감싸고 있는데, 커스텀 예외를 사용하면 상위 레이어에서 더 명확하게 처리할 수 있어요. 다만NotificationService에서 이미try-catch로 처리하고 있으니, 현재 구조에서는 크게 문제되지 않습니다.♻️ 커스텀 예외 적용 예시 (선택사항)
// EmailSendException.java 생성 public class EmailSendException extends RuntimeException { public EmailSendException(String message, Throwable cause) { super(message, cause); } } // EmailService.java에서 사용 } catch (MessagingException e) { throw new EmailSendException("이메일 발송 실패: " + email, e); }src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java (1)
44-61: 유저가 많아지면 성능 이슈가 발생할 수 있어요.현재 구조는 유저별로 3개의 쿼리를 실행합니다 (
existsByUserIdAndItemTypeAndSavedAtBetween,existsNewAttemptBetween,existsReviewAttemptBetween). 유저가 1000명이면 최소 3000개의 쿼리가 실행되고, 이메일 발송까지 포함하면 상당한 시간이 소요될 수 있어요.당장은 문제없겠지만, 스케일업 시 아래 방안들을 고려해볼 수 있어요:
- 배치 쿼리로 모든 유저의 활동 현황을 한 번에 조회
- 비동기 이메일 발송 (
@Async또는 메시지 큐 활용)성능 최적화 방안에 대한 이슈를 열어드릴까요?
Comment on lines
+31
to
+40
| /** | ||
| * 학습 알림 이메일 발송 (평문) | ||
| */ | ||
| public void sendNotificationEmail(String email, String subject, String content) { | ||
| SimpleMailMessage message = new SimpleMailMessage(); | ||
| message.setTo(email); | ||
| message.setSubject(subject); | ||
| message.setText(content); | ||
| mailSender.send(message); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of sendNotificationEmail method
echo "=== Searching for sendNotificationEmail usage ==="
rg -n "sendNotificationEmail" --type=java
echo ""
echo "=== Checking EmailService.java location and content ==="
fd "EmailService.java" --type f | head -5
echo ""
echo "=== Checking NotificationService implementation ==="
fd "NotificationService.java" --type f | head -5Repository: IT-Cotato/12th-FinSight-BE
Length of output: 576
🏁 Script executed:
#!/bin/bash
# Get NotificationService to see which email methods it uses
echo "=== NotificationService.java (checking email method calls) ==="
cat "src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java"
echo ""
echo "=== EmailService.java (full file for context) ==="
cat "src/main/java/com/finsight/finsight/domain/auth/domain/service/EmailService.java"Repository: IT-Cotato/12th-FinSight-BE
Length of output: 6648
sendNotificationEmail 메서드는 현재 사용되지 않고 있습니다.
코드베이스 전체를 확인한 결과, sendNotificationEmail은 정의만 되어 있고 어디서도 호출되지 않습니다. NotificationService의 sendDailyNotifications()과 sendWeeklyNotifications() 메서드 모두 sendHtmlEmail()만 사용 중이에요.
불필요한 메서드는 코드 복잡도를 높이고 유지보수를 어렵게 하므로, 향후 사용 계획이 없다면 제거하는 것이 좋습니다. 참고로 sendVerificationEmail()과 sendHtmlEmail()은 명확한 용도가 있으니 그대로 유지하면 됩니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/auth/domain/service/EmailService.java`
around lines 31 - 40, The sendNotificationEmail(String email, String subject,
String content) method in EmailService is unused and should be removed to reduce
dead code; search the codebase to confirm there are no callers, then delete the
sendNotificationEmail method and any now-unused imports or JavaDoc related to
it, leaving sendVerificationEmail and sendHtmlEmail untouched; if you expect to
reuse plaintext emailing later instead mark it `@Deprecated` and add a TODO
comment instead of deleting.
Comment on lines
+37
to
+62
| @Transactional(readOnly = true) | ||
| public void sendDailyNotifications() { | ||
| List<UserEntity> users = userRepository.findByNotificationEnabledAndAuthType(AuthType.EMAIL); | ||
|
|
||
| LocalDateTime yesterdayStart = LocalDate.now().minusDays(1).atStartOfDay(); | ||
| LocalDateTime yesterdayEnd = LocalDate.now().atStartOfDay(); | ||
|
|
||
| for (UserEntity user : users) { | ||
| try { | ||
| String email = getEmailFromUser(user); | ||
| if (email == null) continue; | ||
|
|
||
| boolean isNewsSaved = folderItemRepository.existsByUserIdAndItemTypeAndSavedAtBetween( | ||
| user.getUserId(), FolderType.NEWS, yesterdayStart, yesterdayEnd); | ||
| boolean isQuizSolved = quizAttemptRepository.existsNewAttemptBetween( | ||
| user.getUserId(), yesterdayStart, yesterdayEnd); | ||
| boolean isQuizReviewed = quizAttemptRepository.existsReviewAttemptBetween( | ||
| user.getUserId(), yesterdayStart, yesterdayEnd); | ||
|
|
||
| String htmlContent = templateBuilder.buildDailyEmail(isNewsSaved, isQuizSolved, isQuizReviewed); | ||
| emailService.sendHtmlEmail(email, "[FinSight] 오늘의 학습 알림", htmlContent); | ||
| } catch (Exception e) { | ||
| // 개별 유저 실패해도 다음 유저 진행 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
빈 catch 블록에 로깅을 추가해주세요.
개별 유저 실패 시 다음 유저로 진행하는 로직은 좋지만, 예외를 완전히 삼켜버리면 문제 진단이 어려워져요. 최소한 경고 로그라도 남겨두면 운영 시 디버깅에 큰 도움이 됩니다.
🐛 로깅 추가 제안
+import lombok.extern.slf4j.Slf4j;
+@Slf4j
`@Service`
`@RequiredArgsConstructor`
public class NotificationService {
// ...
} catch (Exception e) {
- // 개별 유저 실패해도 다음 유저 진행
+ log.warn("일일 알림 발송 실패 - userId: {}, error: {}", user.getUserId(), e.getMessage());
}🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java`
around lines 37 - 62, In NotificationService.sendDailyNotifications, avoid the
empty catch: log the exception and contextual info (at least user.getUserId()
and/or email) when handling failures for each user so errors aren't swallowed;
update the catch block to call the class logger (e.g., log.warn or log.error)
with a clear message including userId/email and the exception to aid debugging,
while keeping the loop behavior to continue to next user; reference methods
getEmailFromUser, templateBuilder.buildDailyEmail, and
emailService.sendHtmlEmail to indicate where failures should be logged.
| */ | ||
| @Transactional(readOnly = true) | ||
| public void sendDailyNotifications() { | ||
| List<UserEntity> users = userRepository.findByNotificationEnabledAndAuthType(AuthType.EMAIL); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. findByNotificationEnabledAndAuthType 메서드 정의 위치 찾기
echo "=== 메서드 정의 위치 검색 ==="
rg -n "findByNotificationEnabledAndAuthType" --type=java
echo ""
echo "=== UserRepository 인터페이스 확인 ==="
fd "UserRepository" --type=java -exec cat -n {} \;
echo ""
echo "=== NotificationService 파일 확인 ==="
fd "NotificationService" --type=java -path "*notification*" -exec cat -n {} \;Repository: IT-Cotato/12th-FinSight-BE
Length of output: 1112
🏁 Script executed:
#!/bin/bash
# UserRepository 파일 내용 확인
echo "=== UserRepository.java 확인 ==="
cat -n src/main/java/com/finsight/finsight/domain/user/persistence/repository/UserRepository.java
echo ""
echo "=== NotificationService.java 확인 ==="
cat -n src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java
echo ""
echo "=== UserEntity의 notificationEnabled 필드 확인 ==="
find . -name "UserEntity.java" -type f -exec cat -n {} \;
echo ""
echo "=== AuthType enum 확인 ==="
find . -name "AuthType.java" -type f -exec cat -n {} \;Repository: IT-Cotato/12th-FinSight-BE
Length of output: 10237
N+1 쿼리 최적화가 필요해요.
메서드 자체는 잘 정의되어 있습니다! UserRepository에서 명시적인 @Query로 구현되어 있고, notificationEnabled 필드도 UserEntity에 존재하네요.
다만 성능상 개선할 부분이 있습니다. 현재 쿼리는 JOIN u.userAuths ua로 필터링하지만, SELECT u로만 반환하고 있어요. 이러면 userAuths가 lazy loading이 되는데, 반복문에서 각 user마다 getEmailFromUser(user) 호출 시 user.getUserAuths() 접근으로 인해 추가 쿼리가 발생합니다 (N+1 문제).
수정 방법
`@Query`("""
SELECT u FROM UserEntity u
LEFT JOIN FETCH u.userAuths ua
WHERE u.notificationEnabled = true
AND ua.authType = :authType
""")
List<UserEntity> findByNotificationEnabledAndAuthType(`@Param`("authType") AuthType authType);FETCH JOIN을 추가하면 한 번의 쿼리로 userAuths까지 함께 로드되어 N+1 문제를 방지할 수 있습니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java`
at line 39, The current query used by UserRepository's
findByNotificationEnabledAndAuthType returns UserEntity without fetching
userAuths, causing N+1 queries when NotificationService calls
getEmailFromUser(user) which accesses user.getUserAuths(); update the repository
query to use a FETCH JOIN to eagerly load u.userAuths (i.e., LEFT JOIN FETCH
u.userAuths ua) while keeping the same authType filter so the users and their
userAuths are loaded in one query and the N+1 problem is eliminated.
Comment on lines
+68
to
+93
| @Transactional(readOnly = true) | ||
| public void sendWeeklyNotifications() { | ||
| List<UserEntity> users = userRepository.findByNotificationEnabledAndAuthType(AuthType.EMAIL); | ||
|
|
||
| LocalDateTime lastWeekStart = LocalDate.now().minusWeeks(1).with(DayOfWeek.MONDAY).atStartOfDay(); | ||
| LocalDateTime lastWeekEnd = LocalDate.now().with(DayOfWeek.MONDAY).atStartOfDay(); | ||
|
|
||
| for (UserEntity user : users) { | ||
| try { | ||
| String email = getEmailFromUser(user); | ||
| if (email == null) continue; | ||
|
|
||
| Long quizCount = quizAttemptRepository.countByUserIdAndCreatedAtBetween( | ||
| user.getUserId(), lastWeekStart, lastWeekEnd); | ||
| Long newsCount = userArticleViewRepository.countByUserIdAndViewedAtBetween( | ||
| user.getUserId(), lastWeekStart, lastWeekEnd); | ||
|
|
||
| String htmlContent = templateBuilder.buildWeeklyEmail( | ||
| quizCount != null ? quizCount : 0, | ||
| newsCount != null ? newsCount : 0); | ||
| emailService.sendHtmlEmail(email, "[FinSight] 주간 학습 리포트", htmlContent); | ||
| } catch (Exception e) { | ||
| // 개별 유저 실패해도 다음 유저 진행 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
주간 알림도 동일한 패턴이네요.
일일 알림과 같은 구조라 동일한 개선 포인트(로깅, 성능)가 적용됩니다. catch 블록에 로깅 추가 부탁드려요.
🐛 로깅 추가
} catch (Exception e) {
- // 개별 유저 실패해도 다음 유저 진행
+ log.warn("주간 알림 발송 실패 - userId: {}, error: {}", user.getUserId(), e.getMessage());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Transactional(readOnly = true) | |
| public void sendWeeklyNotifications() { | |
| List<UserEntity> users = userRepository.findByNotificationEnabledAndAuthType(AuthType.EMAIL); | |
| LocalDateTime lastWeekStart = LocalDate.now().minusWeeks(1).with(DayOfWeek.MONDAY).atStartOfDay(); | |
| LocalDateTime lastWeekEnd = LocalDate.now().with(DayOfWeek.MONDAY).atStartOfDay(); | |
| for (UserEntity user : users) { | |
| try { | |
| String email = getEmailFromUser(user); | |
| if (email == null) continue; | |
| Long quizCount = quizAttemptRepository.countByUserIdAndCreatedAtBetween( | |
| user.getUserId(), lastWeekStart, lastWeekEnd); | |
| Long newsCount = userArticleViewRepository.countByUserIdAndViewedAtBetween( | |
| user.getUserId(), lastWeekStart, lastWeekEnd); | |
| String htmlContent = templateBuilder.buildWeeklyEmail( | |
| quizCount != null ? quizCount : 0, | |
| newsCount != null ? newsCount : 0); | |
| emailService.sendHtmlEmail(email, "[FinSight] 주간 학습 리포트", htmlContent); | |
| } catch (Exception e) { | |
| // 개별 유저 실패해도 다음 유저 진행 | |
| } | |
| } | |
| } | |
| `@Transactional`(readOnly = true) | |
| public void sendWeeklyNotifications() { | |
| List<UserEntity> users = userRepository.findByNotificationEnabledAndAuthType(AuthType.EMAIL); | |
| LocalDateTime lastWeekStart = LocalDate.now().minusWeeks(1).with(DayOfWeek.MONDAY).atStartOfDay(); | |
| LocalDateTime lastWeekEnd = LocalDate.now().with(DayOfWeek.MONDAY).atStartOfDay(); | |
| for (UserEntity user : users) { | |
| try { | |
| String email = getEmailFromUser(user); | |
| if (email == null) continue; | |
| Long quizCount = quizAttemptRepository.countByUserIdAndCreatedAtBetween( | |
| user.getUserId(), lastWeekStart, lastWeekEnd); | |
| Long newsCount = userArticleViewRepository.countByUserIdAndViewedAtBetween( | |
| user.getUserId(), lastWeekStart, lastWeekEnd); | |
| String htmlContent = templateBuilder.buildWeeklyEmail( | |
| quizCount != null ? quizCount : 0, | |
| newsCount != null ? newsCount : 0); | |
| emailService.sendHtmlEmail(email, "[FinSight] 주간 학습 리포트", htmlContent); | |
| } catch (Exception e) { | |
| log.warn("주간 알림 발송 실패 - userId: {}, error: {}", user.getUserId(), e.getMessage()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.java`
around lines 68 - 93, In sendWeeklyNotifications (NotificationService) the catch
block currently swallows exceptions; update it to log failures with context and
the exception (e.g., include user.getUserId() and the resolved email from
getEmailFromUser(user)) using the class logger before continuing so individual
user failures are recorded; keep the existing behavior of continuing to next
user after logging.
Comment on lines
+23
to
+32
| if (!isNewsSaved && !isQuizSolved && !isQuizReviewed) { | ||
| message = "어제는 기록된 학습이 없었어요.<br>오늘은 뉴스 1개 저장하고, 퀴즈 한 번만 풀어 볼까요?"; | ||
| emoji = "💪"; | ||
| } else if (isNewsSaved && isQuizSolved && isQuizReviewed) { | ||
| message = "어제 뉴스와 퀴즈 모두 잘 챙기셨어요.<br>오늘도 가볍게 뉴스 1개부터 이어가 볼까요?"; | ||
| emoji = "🎉"; | ||
| } else { | ||
| message = "어제 저장한 뉴스가 아직 퀴즈를 기다리고 있어요.<br>오늘은 퀴즈 한 번만 이어서 풀어 볼까요?"; | ||
| emoji = "💪"; | ||
| } |
There was a problem hiding this comment.
일일 메시지 분기 로직이 일부 케이스를 정확히 반영하지 못할 수 있어요.
현재 else 분기의 메시지는 "어제 저장한 뉴스가 아직 퀴즈를 기다리고 있어요"인데, 이 메시지는 isNewsSaved=true인 경우에만 적절해요.
하지만 실제로 else 분기는 다음과 같은 경우도 포함됩니다:
- 퀴즈만 풀고 뉴스는 저장 안 한 경우 (
false, true, false) - 복습만 한 경우 (
false, false, true)
이런 경우에 "저장한 뉴스가 아직 퀴즈를 기다리고 있어요"라는 메시지가 사실과 다를 수 있어요.
💡 개선 제안
- } else {
- message = "어제 저장한 뉴스가 아직 퀴즈를 기다리고 있어요.<br>오늘은 퀴즈 한 번만 이어서 풀어 볼까요?";
- emoji = "💪";
+ } else if (isNewsSaved && !isQuizSolved) {
+ message = "어제 저장한 뉴스가 아직 퀴즈를 기다리고 있어요.<br>오늘은 퀴즈 한 번만 이어서 풀어 볼까요?";
+ emoji = "💪";
+ } else {
+ message = "어제도 꾸준히 학습하셨네요!<br>오늘도 뉴스 1개부터 이어가 볼까요?";
+ emoji = "👍";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!isNewsSaved && !isQuizSolved && !isQuizReviewed) { | |
| message = "어제는 기록된 학습이 없었어요.<br>오늘은 뉴스 1개 저장하고, 퀴즈 한 번만 풀어 볼까요?"; | |
| emoji = "💪"; | |
| } else if (isNewsSaved && isQuizSolved && isQuizReviewed) { | |
| message = "어제 뉴스와 퀴즈 모두 잘 챙기셨어요.<br>오늘도 가볍게 뉴스 1개부터 이어가 볼까요?"; | |
| emoji = "🎉"; | |
| } else { | |
| message = "어제 저장한 뉴스가 아직 퀴즈를 기다리고 있어요.<br>오늘은 퀴즈 한 번만 이어서 풀어 볼까요?"; | |
| emoji = "💪"; | |
| } | |
| if (!isNewsSaved && !isQuizSolved && !isQuizReviewed) { | |
| message = "어제는 기록된 학습이 없었어요.<br>오늘은 뉴스 1개 저장하고, 퀴즈 한 번만 풀어 볼까요?"; | |
| emoji = "💪"; | |
| } else if (isNewsSaved && isQuizSolved && isQuizReviewed) { | |
| message = "어제 뉴스와 퀴즈 모두 잘 챙기셨어요.<br>오늘도 가볍게 뉴스 1개부터 이어가 볼까요?"; | |
| emoji = "🎉"; | |
| } else if (isNewsSaved && !isQuizSolved) { | |
| message = "어제 저장한 뉴스가 아직 퀴즈를 기다리고 있어요.<br>오늘은 퀴즈 한 번만 이어서 풀어 볼까요?"; | |
| emoji = "💪"; | |
| } else { | |
| message = "어제도 꾸준히 학습하셨네요!<br>오늘도 뉴스 1개부터 이어가 볼까요?"; | |
| emoji = "👍"; | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/notification/infrastructure/template/NotificationTemplateBuilder.java`
around lines 23 - 32, The current else branch in NotificationTemplateBuilder
produces a "saved news awaiting quiz" message for any case not covered by the
first two branches, which is incorrect when isNewsSaved is false but
isQuizSolved or isQuizReviewed is true; update the branching to explicitly
handle the remaining combinations instead of a catch-all else: after the
existing checks for (!isNewsSaved && !isQuizSolved && !isQuizReviewed) and
(isNewsSaved && isQuizSolved && isQuizReviewed), add explicit branches such as
(isNewsSaved) to keep the "saved news awaiting quiz" message, (isQuizSolved) to
produce a message acknowledging a completed quiz but no saved news, and
(isQuizReviewed) to produce a message for only review—set the appropriate
message and emoji values in each branch so message/emoji reflect the actual
booleans isNewsSaved, isQuizSolved, isQuizReviewed.
Comment on lines
+55
to
+57
| @Builder.Default | ||
| @Column(name = "notification_enabled", nullable = false) | ||
| private Boolean notificationEnabled = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 마이그레이션 관련 디렉토리 구조 확인
find . -type d \( -name "migration" -o -name "migrations" -o -name "flyway" -o -name "liquibase" -o -name "db" \) 2>/dev/null | head -20Repository: IT-Cotato/12th-FinSight-BE
Length of output: 52
🏁 Script executed:
#!/bin/bash
# notification_enabled 관련 모든 참조 확인
rg -i "notification_enabled" --type java --type sql -nRepository: IT-Cotato/12th-FinSight-BE
Length of output: 216
🏁 Script executed:
#!/bin/bash
# 리포지토리의 파일 구조 확인
git ls-files | grep -E "(migration|flyway|liquibase|sql)" | head -30Repository: IT-Cotato/12th-FinSight-BE
Length of output: 52
🏁 Script executed:
#!/bin/bash
# application.properties/yml 파일 검색 및 JPA DDL 설정 확인
find . -type f \( -name "application*.properties" -o -name "application*.yml" \) 2>/dev/null | xargs grep -l "jpa\|hibernate\|ddl" 2>/dev/null | head -10Repository: IT-Cotato/12th-FinSight-BE
Length of output: 52
🏁 Script executed:
#!/bin/bash
# application.properties/yml 전체 내용 확인
find . -type f \( -name "application.properties" -o -name "application.yml" -o -name "application-*.properties" -o -name "application-*.yml" \) 2>/dev/nullRepository: IT-Cotato/12th-FinSight-BE
Length of output: 52
🏁 Script executed:
#!/bin/bash
# UserEntity 파일 전체 내용 확인 - 버전 정보 및 Entity 클래스 명 확인
cat src/main/java/com/finsight/finsight/domain/user/persistence/entity/UserEntity.javaRepository: IT-Cotato/12th-FinSight-BE
Length of output: 2844
🏁 Script executed:
#!/bin/bash
# SQL 파일이 있는지 전체적으로 확인
find . -type f -name "*.sql" 2>/dev/nullRepository: IT-Cotato/12th-FinSight-BE
Length of output: 52
기존 테이블에 NOT NULL 컬럼 추가 시 마이그레이션 전략 확인 필요합니다.
users 테이블에 notification_enabled 컬럼을 nullable=false로 추가하는데, 저장소에 마이그레이션 파일(Flyway/Liquibase)이나 JPA DDL 설정이 보이지 않습니다.
다음 중 하나를 확인해 주세요:
- Flyway/Liquibase 마이그레이션 SQL 파일이 있는지
application.yml의spring.jpa.hibernate.ddl-auto설정값 (production에서는validate여야 함)- 기존 데이터 행들에 대한 백필 처리
JPA의 @Builder.Default와 Java 기본값(true)은 신규 Entity 생성 시만 작동하므로, 기존 데이터베이스 행들의 컬럼값 처리가 명시적으로 필요합니다. 마이그레이션 없이 배포하면 스키마 변경 실패로 인한 배포 중단이 발생할 수 있습니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/user/persistence/entity/UserEntity.java`
around lines 55 - 57, The UserEntity field notificationEnabled is declared
non-nullable but existing DB rows aren’t handled; add a proper migration or
config: create a Flyway/Liquibase migration that adds the notification_enabled
column with a default value (e.g., true) and backfills existing rows, then alter
the column to NOT NULL (or add it NOT NULL with DEFAULT then remove default), or
if you rely on JPA DDL only, ensure spring.jpa.hibernate.ddl-auto is set
appropriately for your environment (validate in prod) and do an explicit
backfill migration instead of relying on `@Builder.Default`; locate
UserEntity.notificationEnabled and implement the migration/backfill and
configuration change accordingly.
Comment on lines
+71
to
+72
| public void updateNotificationEnabled(Boolean enabled) { | ||
| this.notificationEnabled = enabled; |
There was a problem hiding this comment.
null 입력 방어가 필요합니다.
nullable = false 컬럼인데 setter에서 null을 그대로 넣으면 런타임에 제약 위반이 날 수 있어요. 간단히 방어해 두는 게 안전합니다.
✅ 제안 수정
public void updateNotificationEnabled(Boolean enabled) {
- this.notificationEnabled = enabled;
+ if (enabled == null) {
+ throw new IllegalArgumentException("notificationEnabled는 null일 수 없습니다.");
+ }
+ this.notificationEnabled = enabled;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void updateNotificationEnabled(Boolean enabled) { | |
| this.notificationEnabled = enabled; | |
| public void updateNotificationEnabled(Boolean enabled) { | |
| if (enabled == null) { | |
| throw new IllegalArgumentException("notificationEnabled는 null일 수 없습니다."); | |
| } | |
| this.notificationEnabled = enabled; | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/user/persistence/entity/UserEntity.java`
around lines 71 - 72, In updateNotificationEnabled(Boolean enabled) in
UserEntity, guard against null before assigning to the non-nullable field
notificationEnabled; if enabled is null, set notificationEnabled to a safe
default (e.g., false) or throw an IllegalArgumentException — implement a
null-check at the start of the method and assign either enabled.booleanValue()
or the chosen default to this.notificationEnabled to avoid nullable=false
constraint violations at runtime.
Comment on lines
+18
to
+27
| /** | ||
| * 알림 활성화된 EMAIL 유저 목록 조회 | ||
| */ | ||
| @Query(""" | ||
| SELECT u FROM UserEntity u | ||
| JOIN u.userAuths ua | ||
| WHERE u.notificationEnabled = true | ||
| AND ua.authType = :authType | ||
| """) | ||
| List<UserEntity> findByNotificationEnabledAndAuthType(@Param("authType") AuthType authType); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find UserEntity definition
find src/main/java -name "UserEntity.java" -o -name "UserAuths.java" | head -20Repository: IT-Cotato/12th-FinSight-BE
Length of output: 152
🏁 Script executed:
#!/bin/bash
# List files in user domain to understand structure
find src/main/java/com/finsight/finsight/domain/user -type f -name "*.java" | head -30Repository: IT-Cotato/12th-FinSight-BE
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Read UserEntity to see userAuths relationship
cat -n src/main/java/com/finsight/finsight/domain/user/persistence/entity/UserEntity.javaRepository: IT-Cotato/12th-FinSight-BE
Length of output: 3579
🏁 Script executed:
#!/bin/bash
# Read UserAuthEntity structure
cat -n src/main/java/com/finsight/finsight/domain/user/persistence/entity/UserAuthEntity.javaRepository: IT-Cotato/12th-FinSight-BE
Length of output: 3063
🏁 Script executed:
#!/bin/bash
# Search for usages of findByNotificationEnabledAndAuthType
rg "findByNotificationEnabledAndAuthType" src/Repository: IT-Cotato/12th-FinSight-BE
Length of output: 653
🏁 Script executed:
#!/bin/bash
# Check for other similar queries with DISTINCT or JOIN patterns
rg "SELECT.*FROM.*JOIN.*ua" src/ --type java -A 3 -B 1Repository: IT-Cotato/12th-FinSight-BE
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Read NotificationService to see how the query result is used
cat -n src/main/java/com/finsight/finsight/domain/notification/domain/service/NotificationService.javaRepository: IT-Cotato/12th-FinSight-BE
Length of output: 5337
JOIN 없이 중복 제거해야 합니다.
같은 사용자가 여러 EMAIL 인증 정보를 가지면 JOIN으로 중복 행이 반환되어, 알림이 같은 사용자에게 여러 번 발송돼요. 실제로 NotificationService의 sendDailyNotifications(), sendWeeklyNotifications()에서 루프를 돌면서 중복된 사용자에게 이메일을 반복 발송하게 됩니다. DISTINCT를 추가하면 이 문제를 깔끔하게 해결할 수 있습니다.
✅ 제안 수정
`@Query`("""
- SELECT u FROM UserEntity u
+ SELECT DISTINCT u FROM UserEntity u
JOIN u.userAuths ua
WHERE u.notificationEnabled = true
AND ua.authType = :authType
""")
List<UserEntity> findByNotificationEnabledAndAuthType(`@Param`("authType") AuthType authType);🤖 Prompt for AI Agents
In
`@src/main/java/com/finsight/finsight/domain/user/persistence/repository/UserRepository.java`
around lines 18 - 27, The JPQL query in
UserRepository.findByNotificationEnabledAndAuthType selects UserEntity u with a
JOIN on u.userAuths which can return duplicate UserEntity rows when a user has
multiple matching userAuths; update the query to return unique users (e.g.,
change the projection to SELECT DISTINCT u) so the method
findByNotificationEnabledAndAuthType (and downstream callers like
NotificationService.sendDailyNotifications/sendWeeklyNotifications) receive each
UserEntity only once.
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
1 participant
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.
📍 PR 타입 (하나 이상 선택)
❗️ 관련 이슈 링크
Close #
📌 개요
마이페이지 일일, 주간 알림 메일 전송 기능 구현
🔁 변경 사항
📸 스크린샷
👀 기타 더 이야기해볼 점
✅ 체크 리스트
Summary by CodeRabbit
릴리스 노트
새로운 기능
변경
버그 수정