SCRUM-126 이메일 알림 기능 #10
Conversation
Summary of ChangesHello @unifolio0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 이메일 알림 기능을 시스템에 통합합니다. 새로운 이메일 관련 도메인, 리포지토리, 서비스 및 설정 파일들이 추가되었으며, 이메일 템플릿 렌더링 및 실제 메일 전송 로직이 구현되었습니다. 이메일 관련 예외 처리 로직과 테스트 코드도 함께 포함되어 있습니다. 기존 푸시 알림 구독 엔티티는 간소화되었습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded@unifolio0 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough이 PR은 이메일 알림 인프라를 추가합니다: JavaMailSender 설정(실환경/테스트), 이메일 구독 엔티티/레포지토리, 이메일 전송 컴포넌트(전송/렌더러/템플릿), DTO 생성자 보강, ErrorCode 확장, 웹푸시 도메인 필드 제거 및 테스트 설정·단위테스트 추가를 포함합니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Producer as Notification Producer
participant Sender as EmailSender
participant Repo as EmailSubscriptionRepository
participant TplFac as NotificationTemplateFactory
participant View as MailViewRenderer
participant Tx as MailTransport
participant JMS as JavaMailSender
Producer->>Sender: send(NotificationMessage)
Sender->>Repo: findByUserId(userId)
alt 구독 존재
Repo-->>Sender: Optional<EmailSubscription>
Sender->>TplFac: build(message)
TplFac-->>Sender: NotificationTemplateData(subject, body, url)
Sender->>View: render("email-notification.html", vars)
View-->>Sender: htmlContent
Sender->>Tx: send(to, subject, htmlContent)
Tx->>JMS: createMimeMessage/send
JMS-->>Tx: success
Tx-->>Sender: success
else 구독 없음
Repo-->>Sender: Optional.empty
Sender-->>Producer: throw WellMeetNotificationException(EMAIL_NOT_FOUND)
end
note over View,Tx: IO 또는 전송 오류 시 EMAIL_SEND_FAILED 예외 발생
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| } catch (Exception e) { | ||
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | ||
| } |
There was a problem hiding this comment.
🐞 예외 처리 개선 제안
현재 catch 블록에서 예외의 상세 정보를 로깅하지 않고 있어, 실제 운영 환경에서 이메일 전송 실패 시 원인 파악이 어렵습니다. 디버깅을 용이하게 하기 위해 예외 객체 e를 로깅하는 것이 중요합니다.
아래와 같이 로깅 코드를 추가하는 것을 권장합니다. 이를 위해 클래스에 Logger 필드를 선언해야 합니다.
// MailTransport 클래스에 Logger 필드 추가 필요
// import org.slf4j.Logger;
// import org.slf4j.LoggerFactory;
// private static final Logger log = LoggerFactory.getLogger(MailTransport.class);
// send 메소드 내 try-catch 블록
} catch (Exception e) {
log.error("이메일 전송에 실패했습니다. to: {}, subject: {}", to, subject, e);
throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED);
}| } catch (IOException e) { | ||
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | ||
| } |
There was a problem hiding this comment.
🐞 예외 처리 개선 제안
MailTransport.java와 유사하게, 여기서도 IOException 발생 시 예외의 상세 정보를 로깅하지 않고 있습니다. 템플릿 파일을 읽는 데 실패했을 때 원인을 파악하기 위해 예외를 로깅하는 것이 중요합니다.
또한, ErrorCode.EMAIL_SEND_FAILED는 너무 포괄적인 오류 코드입니다. '템플릿 로딩 실패'와 같이 더 구체적인 ErrorCode를 정의하고 사용하면 오류 추적이 더 쉬워집니다.
// MailViewRenderer 클래스에 Logger 필드 추가 필요
// import org.slf4j.Logger;
// import org.slf4j.LoggerFactory;
// private static final Logger log = LoggerFactory.getLogger(MailViewRenderer.class);
// loadTemplate 메소드 내 try-catch 블록
} catch (IOException e) {
log.error("이메일 템플릿 로딩에 실패했습니다. templateName: {}", templateName, e);
// 예: throw new WellMeetNotificationException(ErrorCode.EMAIL_TEMPLATE_LOAD_FAILED);
throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED);
}|
|
||
| assertThatThrownBy(() -> emailSender.send(notificationMessage)) | ||
| .isInstanceOf(WellMeetNotificationException.class) | ||
| .hasMessageContaining(ErrorCode.SUBSCRIPTION_NOT_FOUND.getMessage()); |
There was a problem hiding this comment.
🐞 테스트 케이스 오류
EmailSender의 send 메소드에서는 이메일 구독 정보가 없을 때 ErrorCode.EMAIL_NOT_FOUND 예외를 던지도록 구현되어 있습니다.
// EmailSender.java:34
.orElseThrow(() -> new WellMeetNotificationException(ErrorCode.EMAIL_NOT_FOUND));하지만 이 테스트 케이스에서는 ErrorCode.SUBSCRIPTION_NOT_FOUND의 메시지를 기대하고 있어 테스트가 의도와 다르게 동작하거나 실패할 수 있습니다. EMAIL_NOT_FOUND를 확인하도록 수정해야 합니다.
| .hasMessageContaining(ErrorCode.SUBSCRIPTION_NOT_FOUND.getMessage()); | |
| .hasMessageContaining(ErrorCode.EMAIL_NOT_FOUND.getMessage()); |
![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
🧹 Nitpick comments (3)
src/main/java/com/wellmeet/notification/consumer/dto/NotificationInfo.java (1)
13-16: Lombok@AllArgsConstructor사용을 권장합니다.수동으로 생성자를 작성하는 대신 Lombok의
@AllArgsConstructor애노테이션을 사용하면 더 간결하고 유지보수하기 쉽습니다.다음 diff를 적용하세요:
+import lombok.AllArgsConstructor; import lombok.Getter; import lombok.NoArgsConstructor; @Getter @NoArgsConstructor +@AllArgsConstructor public class NotificationInfo { private NotificationType type; private String recipient; - - public NotificationInfo(NotificationType type, String recipient) { - this.type = type; - this.recipient = recipient; - } }src/main/java/com/wellmeet/notification/consumer/dto/NotificationMessage.java (1)
15-19: Lombok@AllArgsConstructor사용을 권장합니다.수동 생성자 대신 Lombok의
@AllArgsConstructor애노테이션을 사용하는 것이 더 간결합니다.다음 diff를 적용하세요:
import java.util.Map; +import lombok.AllArgsConstructor; import lombok.Getter; import lombok.NoArgsConstructor; @Getter @NoArgsConstructor +@AllArgsConstructor public class NotificationMessage { private MessageHeader header; private NotificationInfo notification; private Map<String, Object> payload; - - public NotificationMessage(MessageHeader header, NotificationInfo notification, Map<String, Object> payload) { - this.header = header; - this.notification = notification; - this.payload = payload; - } public String getRecipient() { return notification.getRecipient(); } }src/test/java/com/wellmeet/email/EmailSenderTest.java (1)
79-97: LGTM!테스트 로직이 올바르고 mock 설정도 적절합니다. 더 엄격한 검증을 원한다면
verify에서any()대신 구체적인 값을 확인할 수 있습니다.예시 (선택사항):
verify(mailTransport).send( eq("test@example.com"), eq("새로운 예약이 접수되었습니다"), anyString() );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/com/wellmeet/config/EmailConfig.java(1 hunks)src/main/java/com/wellmeet/exception/ErrorCode.java(2 hunks)src/main/java/com/wellmeet/notification/consumer/dto/NotificationInfo.java(1 hunks)src/main/java/com/wellmeet/notification/consumer/dto/NotificationMessage.java(1 hunks)src/main/java/com/wellmeet/notification/email/domain/EmailSubscription.java(1 hunks)src/main/java/com/wellmeet/notification/email/repository/EmailSubscriptionRepository.java(1 hunks)src/main/java/com/wellmeet/notification/email/sender/EmailSender.java(1 hunks)src/main/java/com/wellmeet/notification/email/sender/MailTransport.java(1 hunks)src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java(1 hunks)src/main/java/com/wellmeet/notification/webpush/domain/PushSubscription.java(0 hunks)src/main/resources/templates/email-notification.html(1 hunks)src/test/java/com/wellmeet/BaseControllerTest.java(2 hunks)src/test/java/com/wellmeet/BaseServiceTest.java(2 hunks)src/test/java/com/wellmeet/WellmeetNotificationApplicationTests.java(2 hunks)src/test/java/com/wellmeet/config/EmailTestConfig.java(1 hunks)src/test/java/com/wellmeet/email/EmailSenderTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/wellmeet/notification/webpush/domain/PushSubscription.java
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java (1)
Component(11-31)
src/main/java/com/wellmeet/notification/email/domain/EmailSubscription.java (1)
src/main/java/com/wellmeet/notification/webpush/domain/PushSubscription.java (1)
Getter(13-49)
src/test/java/com/wellmeet/email/EmailSenderTest.java (2)
src/test/java/com/wellmeet/BaseControllerTest.java (1)
ExtendWith(19-45)src/test/java/com/wellmeet/BaseServiceTest.java (1)
ExtendWith(12-20)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java (1)
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
Component(12-38)
🪛 GitHub Actions: dev-ci
src/test/java/com/wellmeet/email/EmailSenderTest.java
[error] 127-127: Test failed with AssertionError in EmailSenderTest.java: 이메일_구독_정보가_없으면_예외를_던진다()
🔇 Additional comments (8)
src/test/java/com/wellmeet/WellmeetNotificationApplicationTests.java (1)
3-3: LGTM!이메일 테스트 설정이 올바르게 추가되었습니다.
Also applies to: 12-12
src/main/resources/templates/email-notification.html (1)
1-77: LGTM!이메일 템플릿이 잘 구성되어 있습니다. 인라인 CSS 스타일링은 이메일 클라이언트 호환성을 위해 적절합니다.
src/test/java/com/wellmeet/BaseServiceTest.java (1)
3-3: LGTM!테스트 설정이 올바르게 업데이트되었습니다.
Also applies to: 15-15
src/test/java/com/wellmeet/config/EmailTestConfig.java (1)
10-17: LGTM!테스트용 mock JavaMailSender가 올바르게 구성되었습니다.
@Primary애노테이션으로 테스트에서 실제 bean을 올바르게 오버라이드합니다.src/test/java/com/wellmeet/email/EmailSenderTest.java (1)
99-117: LGTM!예외 처리 테스트가 올바르게 구현되었습니다.
src/main/java/com/wellmeet/notification/email/sender/EmailSender.java (3)
26-28: LGTM!채널 확인 로직이 올바릅니다.
31-44: 구독 조회 로직은 올바르지만 에러 코드 검토 필요.메서드 로직은 정확하게 구현되었습니다. 그러나 34번째 줄의
ErrorCode.EMAIL_NOT_FOUND는 테스트 코드와 불일치하며 이름이 혼란스럽습니다. 이 문제는 테스트 파일 리뷰에서 이미 언급했습니다.
55-57: LGTM!간단하고 명확한 위임 패턴입니다.
| @Bean | ||
| public JavaMailSender javaMailSender() { | ||
| return new JavaMailSenderImpl(); | ||
| } |
There was a problem hiding this comment.
SMTP 서버 설정이 누락되었습니다.
JavaMailSenderImpl이 기본 생성자로 생성되어 SMTP 서버 설정이 없습니다. 이메일 전송 시 런타임 에러가 발생합니다.
다음 설정을 추가해야 합니다:
+import org.springframework.beans.factory.annotation.Value;
+
@Configuration
@Profile("!test")
public class EmailConfig {
+ @Value("${spring.mail.host}")
+ private String host;
+
+ @Value("${spring.mail.port}")
+ private int port;
+
+ @Value("${spring.mail.username}")
+ private String username;
+
+ @Value("${spring.mail.password}")
+ private String password;
+
+ @Value("${spring.mail.properties.mail.smtp.auth:true}")
+ private boolean auth;
+
+ @Value("${spring.mail.properties.mail.smtp.starttls.enable:true}")
+ private boolean starttls;
+
@Bean
public JavaMailSender javaMailSender() {
- return new JavaMailSenderImpl();
+ JavaMailSenderImpl mailSender = new JavaMailSenderImpl();
+ mailSender.setHost(host);
+ mailSender.setPort(port);
+ mailSender.setUsername(username);
+ mailSender.setPassword(password);
+
+ Properties props = mailSender.getJavaMailProperties();
+ props.put("mail.transport.protocol", "smtp");
+ props.put("mail.smtp.auth", auth);
+ props.put("mail.smtp.starttls.enable", starttls);
+ props.put("mail.debug", "false");
+
+ return mailSender;
}
}또한 application.yml 또는 application.properties에 다음 설정을 추가하세요:
spring:
mail:
host: smtp.example.com
port: 587
username: your-email@example.com
password: your-password
properties:
mail:
smtp:
auth: true
starttls:
enable: true🤖 Prompt for AI Agents
In src/main/java/com/wellmeet/config/EmailConfig.java around lines 13-16, the
JavaMailSender bean is created with the default constructor so no SMTP
host/port/credentials or mail properties are set; replace the current
implementation to populate a JavaMailSenderImpl from application properties
(inject Environment or @Value for spring.mail.host, port, username, password and
set them on the JavaMailSenderImpl) and set its JavaMailProperties
(mail.smtp.auth and mail.smtp.starttls.enable) accordingly, and add the
recommended spring.mail.* entries to application.yml or application.properties
so the bean picks up real SMTP configuration.
| private String buildHtmlContent(NotificationTemplateData templateData) { | ||
| Map<String, String> variables = Map.of( | ||
| "title", templateData.title(), | ||
| "body", templateData.body(), | ||
| "url", templateData.url() | ||
| ); | ||
| return mailViewRenderer.render("email-notification.html", variables); | ||
| } |
There was a problem hiding this comment.
NotificationTemplateData의 hasUrl 필드가 템플릿 변수에서 누락되었습니다.
NotificationTemplateData는 hasUrl 필드를 포함하고 있지만(테스트의 149-154번째 줄 참조), 템플릿 변수 맵에서 이를 전달하지 않습니다. 템플릿이 URL 섹션을 조건부로 렌더링해야 하는 경우 이 필드가 필요할 수 있습니다.
다음과 같이 수정하세요:
private String buildHtmlContent(NotificationTemplateData templateData) {
- Map<String, String> variables = Map.of(
+ Map<String, Object> variables = Map.of(
"title", templateData.title(),
"body", templateData.body(),
- "url", templateData.url()
+ "url", templateData.url(),
+ "hasUrl", templateData.hasUrl()
);
return mailViewRenderer.render("email-notification.html", variables);
}참고: Map<String, Object>로 변경해야 boolean 값을 포함할 수 있습니다.
📝 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.
| private String buildHtmlContent(NotificationTemplateData templateData) { | |
| Map<String, String> variables = Map.of( | |
| "title", templateData.title(), | |
| "body", templateData.body(), | |
| "url", templateData.url() | |
| ); | |
| return mailViewRenderer.render("email-notification.html", variables); | |
| } | |
| private String buildHtmlContent(NotificationTemplateData templateData) { | |
| Map<String, Object> variables = Map.of( | |
| "title", templateData.title(), | |
| "body", templateData.body(), | |
| "url", templateData.url(), | |
| "hasUrl", templateData.hasUrl() | |
| ); | |
| return mailViewRenderer.render("email-notification.html", variables); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/wellmeet/notification/email/sender/EmailSender.java around
lines 46 to 53, the template variables map omits NotificationTemplateData.hasUrl
(a boolean) causing the template to lack the conditional flag; change the
variables map type from Map<String, String> to Map<String, Object> and include
an entry "hasUrl" -> templateData.hasUrl() (or templateData.hasUrl) so the
renderer receives the boolean for conditional rendering, keeping the other
entries unchanged.
| public void send(String to, String subject, String htmlContent) { | ||
| try { | ||
| MimeMessage mimeMessage = javaMailSender.createMimeMessage(); | ||
| MimeMessageHelper helper = new MimeMessageHelper(mimeMessage, true, "UTF-8"); | ||
|
|
||
| helper.setTo(to); | ||
| helper.setSubject(subject); | ||
| helper.setText(htmlContent, true); | ||
|
|
||
| javaMailSender.send(mimeMessage); | ||
| } catch (Exception e) { | ||
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | ||
| } | ||
| } |
There was a problem hiding this comment.
발신자 주소(from) 설정이 누락되었습니다.
MimeMessageHelper에 setTo(), setSubject(), setText()는 있지만 setFrom() 호출이 없습니다. 대부분의 SMTP 서버는 발신자 주소를 요구하며, 없으면 이메일 전송이 실패하거나 스팸으로 분류될 수 있습니다.
다음 diff를 적용하세요:
+import org.springframework.beans.factory.annotation.Value;
+
@Component
@RequiredArgsConstructor
public class MailTransport {
private final JavaMailSender javaMailSender;
+
+ @Value("${spring.mail.from:noreply@wellmeet.com}")
+ private String fromAddress;
public void send(String to, String subject, String htmlContent) {
try {
MimeMessage mimeMessage = javaMailSender.createMimeMessage();
MimeMessageHelper helper = new MimeMessageHelper(mimeMessage, true, "UTF-8");
+ helper.setFrom(fromAddress);
helper.setTo(to);
helper.setSubject(subject);
helper.setText(htmlContent, true);
javaMailSender.send(mimeMessage);
} catch (Exception e) {
throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED);
}
}
}📝 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.
| public void send(String to, String subject, String htmlContent) { | |
| try { | |
| MimeMessage mimeMessage = javaMailSender.createMimeMessage(); | |
| MimeMessageHelper helper = new MimeMessageHelper(mimeMessage, true, "UTF-8"); | |
| helper.setTo(to); | |
| helper.setSubject(subject); | |
| helper.setText(htmlContent, true); | |
| javaMailSender.send(mimeMessage); | |
| } catch (Exception e) { | |
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | |
| } | |
| } | |
| import org.springframework.beans.factory.annotation.Value; | |
| @Component | |
| @RequiredArgsConstructor | |
| public class MailTransport { | |
| private final JavaMailSender javaMailSender; | |
| @Value("${spring.mail.from:noreply@wellmeet.com}") | |
| private String fromAddress; | |
| public void send(String to, String subject, String htmlContent) { | |
| try { | |
| MimeMessage mimeMessage = javaMailSender.createMimeMessage(); | |
| MimeMessageHelper helper = new MimeMessageHelper(mimeMessage, true, "UTF-8"); | |
| helper.setFrom(fromAddress); | |
| helper.setTo(to); | |
| helper.setSubject(subject); | |
| helper.setText(htmlContent, true); | |
| javaMailSender.send(mimeMessage); | |
| } catch (Exception e) { | |
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/wellmeet/notification/email/sender/MailTransport.java
around lines 17 to 30, the code builds the MimeMessage but never sets the From
address which many SMTP servers require; update the class to use a configured
sender address (inject a String fromAddress via constructor or @Value from
application properties, or obtain it from the mail config) and call
helper.setFrom(fromAddress) before sending the message so the email has a valid
sender; ensure you validate or default the fromAddress and preserve the existing
UTF-8/HTML settings.
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
안녕하세요! 이메일 알림 기능 추가를 위한 PR 잘 보았습니다. 전반적으로 기능 구현이 잘 이루어졌으며, 테스트 코드도 꼼꼼하게 작성해주셨습니다. 몇 가지 개선점을 제안드리니 확인 후 반영을 고려해주시면 좋겠습니다.
주요 검토 내용은 다음과 같습니다.
- 예외 처리: 보다 구체적인 예외를 처리하고, 디버깅을 위해 원인 예외(cause)를 보존하는 방향으로 개선이 필요합니다.
- 코드 정확성: 템플릿 렌더링 로직에서 발생할 수 있는 잠재적 버그와 상황에 맞지 않는 에러 코드 사용을 수정해야 합니다.
- 테스트 일관성: 테스트 코드에서 단언(assertion) 스타일을 일관되게 유지하면 가독성과 유지보수성이 향상됩니다.
자세한 내용은 각 파일의 인라인 코멘트를 참고해주세요. 수고하셨습니다!
| } catch (Exception e) { | ||
| log.error("이메일 전송에 실패했습니다. to: {}, subject: {}", to, subject, e); | ||
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | ||
| } |
There was a problem hiding this comment.
[HIGH] 보다 구체적인 예외 처리 및 원인 예외(cause) 보존 필요
현재 catch (Exception e) 블록은 너무 광범위하여 이메일 전송과 관련 없는 예외(예: NullPointerException)까지 모두 EMAIL_SEND_FAILED로 처리할 수 있어 디버깅을 어렵게 만듭니다.
- 구체적인 예외 처리: Spring의
JavaMailSender는MailException을,MimeMessageHelper는MessagingException을 발생시키므로, 이들을 명시적으로 잡는 것이 좋습니다. - 원인 예외 보존:
log.error로 예외를 기록하고 있지만, 새로운WellMeetNotificationException을 던질 때 원래 예외(e)를 포함하지 않아 스택 트레이스 정보가 유실됩니다. 이는 문제 발생 시 원인 추적을 매우 어렵게 만듭니다.
WellMeetNotificationException에 원인 예외를 전달하는 생성자를 추가하고, catch 블록에서 이를 사용하도록 수정하는 것을 강력히 권장합니다. 만약 WellMeetNotificationException 수정이 당장 어렵다면, 잡는 예외라도 아래와 같이 더 구체적으로 변경하는 것이 좋습니다.
| } catch (Exception e) { | |
| log.error("이메일 전송에 실패했습니다. to: {}, subject: {}", to, subject, e); | |
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | |
| } | |
| } catch (org.springframework.mail.MailException | jakarta.mail.MessagingException e) { | |
| log.error("이메일 전송에 실패했습니다. to: {}, subject: {}", to, subject, e); | |
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | |
| } |
| return StreamUtils.copyToString(resource.getInputStream(), StandardCharsets.UTF_8); | ||
| } catch (IOException e) { | ||
| log.error("이메일 템플릿 로딩에 실패했습니다. templateName: {}", templateName, e); | ||
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); |
There was a problem hiding this comment.
[HIGH] 상황에 맞지 않는 에러 코드 사용
이메일 템플릿 로딩에 실패했을 때 ErrorCode.EMAIL_SEND_FAILED를 사용하고 있습니다. 이는 실제 에러의 원인(템플릿 로딩 실패)과 달라 혼동을 줄 수 있습니다.
ErrorCode.TEMPLATE_NOT_FOUND와 같이 상황에 더 적합한 에러 코드를 사용하는 것이 원인 파악에 더 도움이 됩니다. 또한, MailTransport.java와 마찬가지로 원인 예외(e)를 함께 전달하여 스택 트레이스를 보존하는 것이 좋습니다.
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | |
| throw new WellMeetNotificationException(ErrorCode.TEMPLATE_NOT_FOUND); |
| String result = template; | ||
| for (Map.Entry<String, String> entry : variables.entrySet()) { | ||
| result = result.replace("{" + entry.getKey() + "}", entry.getValue()); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
[HIGH] 플레이스홀더 치환 로직의 잠재적 버그 및 비효율성
현재의 플레이스홀더 치환 방식에는 두 가지 문제가 있습니다.
- 정확성 문제:
Map.entrySet()의 순서는 보장되지 않습니다. 만약 치환될 값에 다른 플레이스홀더가 포함된 경우(예:body값에{title}이 포함), 어떤 키-값 쌍이 먼저 처리되느냐에 따라 최종 결과가 달라질 수 있어 버그를 유발할 수 있습니다. - 성능 문제:
for루프 내에서String.replace()를 반복 호출하면 매번 새로운String객체가 생성되어 비효율적입니다.
이러한 문제를 해결하기 위해, 모든 플레이스홀더를 한 번에 스캔하여 치환하는 방식이 필요합니다. Apache Commons Text의 StringSubstitutor가 좋은 해결책이 될 수 있으며, 외부 라이브러리 추가가 어렵다면 정규표현식을 사용하는 방법을 고려해볼 수 있습니다.
| boolean enabled = emailSender.isEnabled(NotificationChannel.EMAIL); | ||
| assert enabled; |
There was a problem hiding this comment.
[MEDIUM] 일관성 없는 단언(Assertion) 스타일
테스트 클래스의 다른 부분에서는 AssertJ의 fluent API(assertThatThrownBy)를 사용하고 있는데, 여기서는 assert 키워드를 사용하고 있습니다. 프로젝트 전체의 테스트 코드 스타일 일관성을 위해 AssertJ의 assertThat을 사용하는 것이 좋습니다.
이 문제는 70-71 라인에서도 동일하게 발견됩니다.
| boolean enabled = emailSender.isEnabled(NotificationChannel.EMAIL); | |
| assert enabled; | |
| boolean enabled = emailSender.isEnabled(NotificationChannel.EMAIL); | |
| org.assertj.core.api.Assertions.assertThat(enabled).isTrue(); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java (1)
24-26: 발신자 주소(From) 설정이 여전히 누락되었습니다.
MimeMessageHelper에setTo(),setSubject(),setText()는 설정되어 있지만setFrom()호출이 없습니다. 대부분의 SMTP 서버는 발신자 주소를 요구하며, 없으면 이메일 전송이 실패하거나 스팸으로 분류될 수 있습니다.
@Value로 발신자 주소를 주입하고helper.setFrom()을 호출하세요:+import org.springframework.beans.factory.annotation.Value; + @Slf4j @Component @RequiredArgsConstructor public class MailTransport { private final JavaMailSender javaMailSender; + + @Value("${spring.mail.from:noreply@wellmeet.com}") + private String fromAddress; public void send(String to, String subject, String htmlContent) { try { MimeMessage mimeMessage = javaMailSender.createMimeMessage(); MimeMessageHelper helper = new MimeMessageHelper(mimeMessage, true, "UTF-8"); + helper.setFrom(fromAddress); helper.setTo(to); helper.setSubject(subject); helper.setText(htmlContent, true);src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
26-27: InputStream 자원 누수 위험이 여전히 존재합니다.
ClassPathResource#getInputStream()으로 연 스트림을 명시적으로 닫지 않아 반복적인 템플릿 렌더링 시 파일 디스크립터가 누적될 수 있습니다. try-with-resources를 사용하여 자원을 안전하게 해제하세요.+import java.io.InputStream; private String loadTemplate(String templateName) { - try { - ClassPathResource resource = new ClassPathResource(TEMPLATE_BASE_PATH + templateName); - return StreamUtils.copyToString(resource.getInputStream(), StandardCharsets.UTF_8); + ClassPathResource resource = new ClassPathResource(TEMPLATE_BASE_PATH + templateName); + try (InputStream inputStream = resource.getInputStream()) { + return StreamUtils.copyToString(inputStream, StandardCharsets.UTF_8); } catch (IOException e) { log.error("이메일 템플릿 로딩에 실패했습니다. templateName: {}", templateName, e); throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED);src/test/java/com/wellmeet/email/EmailSenderTest.java (2)
59-65: Javaassert대신 AssertJ 어설션을 사용하세요.Java의
assert구문은 기본적으로 비활성화되어 있어 테스트가 실제로 검증을 수행하지 않을 수 있습니다. AssertJ의assertThat을 사용하여 명확하고 신뢰할 수 있는 검증을 수행하세요.@Test void EMAIL_채널이면_true를_반환한다() { - assertThatCode(() -> { - boolean enabled = emailSender.isEnabled(NotificationChannel.EMAIL); - assert enabled; - }).doesNotThrowAnyException(); + boolean enabled = emailSender.isEnabled(NotificationChannel.EMAIL); + assertThat(enabled).isTrue(); }
67-73: Javaassert대신 AssertJ 어설션을 사용하세요.동일한 문제입니다.
assert구문 대신 AssertJ의assertThat을 사용하세요.@Test void WEB_PUSH_채널이면_false를_반환한다() { - assertThatCode(() -> { - boolean enabled = emailSender.isEnabled(NotificationChannel.WEB_PUSH); - assert !enabled; - }).doesNotThrowAnyException(); + boolean enabled = emailSender.isEnabled(NotificationChannel.WEB_PUSH); + assertThat(enabled).isFalse(); }
🧹 Nitpick comments (1)
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
34-39: null 안전성 개선을 고려하세요.
variables맵이나 개별 값이 null일 경우를 처리하지 않고 있습니다. 프로덕션 환경에서 예기치 않은 NPE가 발생할 수 있습니다.다음과 같이 null 체크를 추가하는 것을 권장합니다:
private String replacePlaceholders(String template, Map<String, String> variables) { + if (variables == null || variables.isEmpty()) { + return template; + } String result = template; for (Map.Entry<String, String> entry : variables.entrySet()) { - result = result.replace("{" + entry.getKey() + "}", entry.getValue()); + String value = entry.getValue(); + if (value != null) { + result = result.replace("{" + entry.getKey() + "}", value); + } } return result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java(1 hunks)src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java(1 hunks)src/test/java/com/wellmeet/email/EmailSenderTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java (1)
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
Slf4j(13-41)
src/test/java/com/wellmeet/email/EmailSenderTest.java (2)
src/test/java/com/wellmeet/BaseControllerTest.java (1)
ExtendWith(19-45)src/test/java/com/wellmeet/BaseServiceTest.java (1)
ExtendWith(12-20)
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java (1)
Slf4j(12-34)
🔇 Additional comments (3)
src/main/java/com/wellmeet/notification/email/sender/MailTransport.java (1)
30-30: 예외 로깅이 적절하게 추가되었습니다.이전 리뷰 피드백이 반영되어 예외 정보가 로깅되고 있어 운영 환경에서 디버깅이 용이합니다.
src/main/java/com/wellmeet/notification/email/sender/MailViewRenderer.java (1)
29-29: 예외 로깅이 적절하게 추가되었습니다.이전 리뷰 피드백이 반영되어 템플릿 로딩 실패 시 상세한 로그가 남아 문제 추적이 가능합니다.
src/test/java/com/wellmeet/email/EmailSenderTest.java (1)
79-97: 테스트 구조가 잘 설계되었습니다.모킹 설정이 적절하고, given-when-then 패턴을 따르며, 상호작용 검증(
verify)도 수행하고 있어 단위 테스트로서 우수한 품질을 보입니다.
|
/gemini review |
There was a problem hiding this comment.
Code Review
안녕하세요. 이메일 알림 기능 추가 PR에 대한 리뷰를 남깁니다.
전반적으로 새로운 기능의 구조가 잘 잡혀있고, 테스트 코드도 꼼꼼하게 작성해주셔서 인상 깊었습니다. 특히 MailViewRenderer의 템플릿 처리 로직과 관련 테스트 케이스들은 매우 견고해 보입니다.
몇 가지 개선점을 제안드리니 확인 부탁드립니다.
주요 검토 항목:
- 예외 처리: 보다 명확한 예외 유형을 사용하여 오류 진단을 용이하게 하도록 제안했습니다.
- 데이터베이스 스키마: 엔티티 필드에 길이 제약 조건을 추가하여 데이터 무결성을 향상시키는 방안을 제안했습니다.
전체적으로 훌륭한 작업이며, 제안드린 내용이 코드 품질을 더욱 높이는 데 도움이 되기를 바랍니다.
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long id; | ||
|
|
||
| @NotNull |
There was a problem hiding this comment.
userId 필드에 대한 최대 길이를 지정하는 것이 좋습니다. @Column(length = ...) 어노테이션을 사용하여 데이터베이스 스키마에 제약 조건을 명시하고, 데이터의 일관성을 보장할 수 있습니다. userId의 길이에 맞는 적절한 값을 설정해주세요. (예: 50자) jakarta.persistence.Column 임포트가 필요합니다.
| @NotNull | |
| @jakarta.persistence.Column(length = 50) | |
| @NotNull |
| private String userId; | ||
|
|
||
| @NotNull | ||
There was a problem hiding this comment.
email 필드에 대한 최대 길이를 지정하는 것이 좋습니다. RFC 표준에 따르면 이메일 주소의 최대 길이는 254자입니다. @Column(length = 254)를 추가하여 데이터베이스 수준에서 제약 조건을 강제하는 것이 좋습니다. jakarta.persistence.Column 임포트가 필요합니다.
| @jakarta.persistence.Column(length = 254) | |
| helper.setText(htmlContent, true); | ||
|
|
||
| javaMailSender.send(mimeMessage); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
포괄적인 Exception을 잡는 것보다, 이메일 전송 시 발생할 수 있는 더 구체적인 예외를 명시하는 것이 좋습니다. Spring의 MailException과 Jakarta Mail의 MessagingException을 잡도록 수정하면 코드의 의도가 명확해지고 예상치 못한 다른 종류의 Exception이 가려지는 것을 방지할 수 있습니다. org.springframework.mail.MailException과 jakarta.mail.MessagingException 임포트가 필요합니다.
| } catch (Exception e) { | |
| } catch (MailException | MessagingException e) { |
| return StreamUtils.copyToString(inputStream, StandardCharsets.UTF_8); | ||
| } catch (IOException e) { | ||
| log.error("이메일 템플릿 로딩에 실패했습니다. templateName: {}", templateName, e); | ||
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); |
There was a problem hiding this comment.
템플릿 로딩 실패 시 EMAIL_SEND_FAILED 에러 코드를 사용하고 있습니다. 하지만 ErrorCode에 TEMPLATE_NOT_FOUND가 이미 정의되어 있고, NotificationTemplateFactory에서도 사용되고 있습니다. 오류의 원인을 더 명확하게 나타내고 일관성을 유지하기 위해 TEMPLATE_NOT_FOUND를 사용하는 것이 좋겠습니다.
| throw new WellMeetNotificationException(ErrorCode.EMAIL_SEND_FAILED); | |
| throw new WellMeetNotificationException(ErrorCode.TEMPLATE_NOT_FOUND); |
1 participant
🚩 Jira Ticket
SCRUM-126
🗣️ 리뷰 요구사항 (선택)
Summary by CodeRabbit