[DDING-108] 동아리 지원 결과 email 전송 API 구현 #256
Conversation
# Conflicts: # src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
# Conflicts: # src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java # src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
5uhwann
requested review from
wonjunYou,
KoSeonJe and
Seooooo24
as code owners
Walkthrough이번 변경 사항은 AWS SES 관련 기능을 추가하는 내용을 포함합니다. Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
src/main/java/ddingdong/ddingdongBE/email/dto/EmailContent.java (1)
9-13: HTML 컨텐츠 생성 로직 개선이 필요합니다.현재 HTML 생성 로직이 매우 기본적입니다. XSS 방지와 더 나은 이메일 템플릿 지원을 위해 개선이 필요합니다.
public static EmailContent of(String subject, String content) { - String htmlContent = "<div>" + content.replace("\n", "<br>") + "</div>"; + String escapedContent = HtmlUtils.htmlEscape(content); + String htmlContent = String.format(""" + <div style="font-family: Arial, sans-serif; padding: 20px;"> + %s + </div>""", + escapedContent.replace("\n", "<br>")); return new EmailContent(subject, htmlContent, content); }src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java (1)
21-21: 메서드 문서화가 필요합니다.메서드의 목적과 매개변수에 대한 Javadoc 문서화를 추가하는 것이 좋습니다.
+ /** + * 폼 ID와 지원서 상태를 기준으로 모든 지원서를 조회합니다. + * + * @param formId 조회할 폼의 ID + * @param status 조회할 지원서의 상태 + * @return 조건에 맞는 지원서 목록 + */ List<FormApplication> getAllByFormIdAndFormApplicationStatus(Long formId, FormApplicationStatus status);src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/SendApplicationResultEmailRequest.java (1)
25-27: Command 변환 시 유효성 검증이 필요합니다.toCommand 메서드에서 추가적인 유효성 검증을 수행하는 것이 좋습니다.
public SendApplicationResultEmailCommand toCommand(Long formId) { + if (formId == null || formId <= 0) { + throw new IllegalArgumentException("유효하지 않은 폼 ID입니다."); + } return new SendApplicationResultEmailCommand(formId, title, target, message); }src/main/java/ddingdong/ddingdongBE/common/config/AsyncConfig.java (2)
17-24: 스레드 풀 설정을 외부 설정으로 분리하는 것이 좋습니다.스레드 풀의 크기와 용량을 application.properties나 application.yml에서 설정할 수 있도록 하면 환경에 따라 유연하게 조정할 수 있습니다.
+ @Value("${async.email.core-pool-size:5}") + private int corePoolSize; + @Value("${async.email.max-pool-size:10}") + private int maxPoolSize; + @Value("${async.email.queue-capacity:500}") + private int queueCapacity; @Override public Executor getAsyncExecutor() { ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); - executor.setCorePoolSize(5); - executor.setMaxPoolSize(10); - executor.setQueueCapacity(500); + executor.setCorePoolSize(corePoolSize); + executor.setMaxPoolSize(maxPoolSize); + executor.setQueueCapacity(queueCapacity); executor.setThreadNamePrefix("EmailAsync-"); executor.initialize(); return executor; }
26-29: 비동기 작업 예외 처리를 개선하는 것이 좋습니다.커스텀 예외 핸들러를 구현하여 로깅이나 모니터링을 추가하는 것이 좋습니다.
+ @Slf4j + private static class CustomAsyncExceptionHandler implements AsyncUncaughtExceptionHandler { + @Override + public void handleUncaughtException(Throwable ex, Method method, Object... params) { + log.error("Async method {} threw exception: ", method.getName(), ex); + // 추가적인 에러 처리 로직 + } + } @Override public AsyncUncaughtExceptionHandler getAsyncUncaughtExceptionHandler() { - return new SimpleAsyncUncaughtExceptionHandler(); + return new CustomAsyncExceptionHandler(); }src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java (1)
48-51: 메서드 문서화가 필요합니다.메서드의 목적과 사용 사례를 명확히 하기 위해 Javadoc 문서화를 추가하면 좋을 것 같습니다.
다음과 같이 수정하는 것을 제안드립니다:
+ /** + * 특정 폼 ID와 지원 상태에 해당하는 모든 지원서를 조회합니다. + * + * @param formId 조회할 폼의 ID + * @param status 조회할 지원서의 상태 + * @return 조건에 맞는 지원서 목록 + */ @Override public List<FormApplication> getAllByFormIdAndFormApplicationStatus(Long formId, FormApplicationStatus status) { return formApplicationRepository.getAllByFormIdAndStatus(formId, status); }src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (1)
75-75: 쿼리 최적화가 필요할 수 있습니다.현재 메서드명으로 생성되는 쿼리 대신 명시적인 JPQL 쿼리를 사용하면 성능 최적화가 가능할 것 같습니다.
다음과 같이 수정하는 것을 제안드립니다:
- List<FormApplication> getAllByFormIdAndStatus(Long formId, FormApplicationStatus status); + @Query(""" + select fa + from FormApplication fa + where fa.form.id = :formId + and fa.status = :status + """) + List<FormApplication> getAllByFormIdAndStatus( + @Param("formId") Long formId, + @Param("status") FormApplicationStatus status + );src/main/java/ddingdong/ddingdongBE/email/SesEmailService.java (1)
43-44: 문자 인코딩이 하드코딩되어 있습니다.UTF-8 인코딩이 여러 곳에서 반복되어 있습니다. 상수로 추출하는 것이 좋습니다.
다음과 같이 수정하는 것을 제안합니다:
+private static final String DEFAULT_CHARSET = "UTF-8"; .subject(Content.builder() - .charset("UTF-8") + .charset(DEFAULT_CHARSET) .data(emailContent.subject()) .build())Also applies to: 48-49, 53-54
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
179-179: 타임아웃 값이 하드코딩되어 있습니다.5분의 타임아웃 값이 하드코딩되어 있습니다. 설정 파일로 분리하는 것이 좋습니다.
application.yml에 설정값을 추가하고@Value어노테이션으로 주입받는 것을 추천합니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.gradle(1 hunks)src/main/java/ddingdong/ddingdongBE/common/config/AmazonS3Config.java(2 hunks)src/main/java/ddingdong/ddingdongBE/common/config/AsyncConfig.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/SendApplicationResultEmailRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(5 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/SendApplicationResultEmailCommand.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/email/SesEmailService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/email/dto/EmailContent.java(1 hunks)src/main/resources/application.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/SendApplicationResultEmailCommand.java
🔇 Additional comments (4)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java (1)
4-4: 이메일 전송 기능이 잘 추가되었습니다!인터페이스에 이메일 전송 기능이 명확하게 정의되어 있습니다.
Also applies to: 34-34
src/main/java/ddingdong/ddingdongBE/email/SesEmailService.java (1)
29-31: 비동기 이메일 전송 메서드의 반환 타입이 적절합니다.CompletableFuture를 사용하여 비동기 처리를 구현한 것이 좋습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java (1)
98-101: 구현이 깔끔하고 적절합니다.서비스 계층으로의 위임이 잘 구현되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
169-184: 트랜잭션 범위와 비동기 처리의 관계를 검토해주세요.현재 구현에서는 비동기 이메일 전송이 트랜잭션 내에서 실행될 수 있습니다. 이는 불필요하게 트랜잭션을 오래 유지할 수 있으며, 트랜잭션 타임아웃 문제가 발생할 수 있습니다.
트랜잭션 관리 전략을 검토하고, 필요한 경우 이메일 전송 로직을 별도의 트랜잭션으로 분리하는 것을 고려해주세요.
| public record EmailContent( | ||
| String subject, | ||
| String htmlContent, | ||
| String textContent | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이메일 컨텐츠의 유효성 검증이 필요합니다.
각 필드에 대한 유효성 검증 어노테이션을 추가하는 것이 좋습니다.
public record EmailContent(
+ @NotNull(message = "제목은 필수입니다.")
String subject,
+ @NotNull(message = "HTML 내용은 필수입니다.")
String htmlContent,
+ @NotNull(message = "텍스트 내용은 필수입니다.")
String textContent
)📝 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 record EmailContent( | |
| String subject, | |
| String htmlContent, | |
| String textContent | |
| ) { | |
| public record EmailContent( | |
| @NotNull(message = "제목은 필수입니다.") | |
| String subject, | |
| @NotNull(message = "HTML 내용은 필수입니다.") | |
| String htmlContent, | |
| @NotNull(message = "텍스트 내용은 필수입니다.") | |
| String textContent | |
| ) { |
| @Schema(description = "전송 대상", | ||
| example = "FIRST_PASS", | ||
| allowableValues = {"FIRST_PASS", "FIRST_FAIL", "FINAL_PASS", "FINAL_FAIL"} | ||
| ) | ||
| @NotNull(message = "전송 대상은 필수입니다.") | ||
| String target, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
전송 대상을 열거형(enum)으로 변경하는 것이 좋습니다.
String 타입 대신 enum을 사용하면 타입 안전성이 향상되고 가능한 값들을 명확하게 제한할 수 있습니다.
+public enum EmailTarget {
+ FIRST_PASS, FIRST_FAIL, FINAL_PASS, FINAL_FAIL
+}
public record SendApplicationResultEmailRequest(
@Schema(description = "전송 대상",
example = "FIRST_PASS",
allowableValues = {"FIRST_PASS", "FIRST_FAIL", "FINAL_PASS", "FINAL_FAIL"}
)
@NotNull(message = "전송 대상은 필수입니다.")
- String target,
+ EmailTarget target,📝 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.
| @Schema(description = "전송 대상", | |
| example = "FIRST_PASS", | |
| allowableValues = {"FIRST_PASS", "FIRST_FAIL", "FINAL_PASS", "FINAL_FAIL"} | |
| ) | |
| @NotNull(message = "전송 대상은 필수입니다.") | |
| String target, | |
| public enum EmailTarget { | |
| FIRST_PASS, FIRST_FAIL, FINAL_PASS, FINAL_FAIL | |
| } | |
| public record SendApplicationResultEmailRequest( | |
| @Schema(description = "전송 대상", | |
| example = "FIRST_PASS", | |
| allowableValues = {"FIRST_PASS", "FIRST_FAIL", "FINAL_PASS", "FINAL_FAIL"} | |
| ) | |
| @NotNull(message = "전송 대상은 필수입니다.") | |
| EmailTarget target, | |
| // 다른 필드들... | |
| ) { | |
| // toCommand 변환 메서드 등 필요한 추가 코드... | |
| } |
| @Bean | ||
| public SesClient sesClient() { | ||
| return SesClient.builder() | ||
| .region(Region.AP_NORTHEAST_2) | ||
| .credentialsProvider(StaticCredentialsProvider.create( | ||
| AwsBasicCredentials.create(accessKey, secretKey) | ||
| )) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
AWS SES 클라이언트 설정 개선이 필요합니다.
다음 사항들을 개선하면 좋을 것 같습니다:
- Region이 하드코딩되어 있습니다. S3 설정과 같이 환경 변수로 관리하는 것이 좋습니다.
- SES 관련 설정이 S3Config 클래스에 있는 것은 단일 책임 원칙(SRP)에 위배됩니다.
다음과 같이 수정하는 것을 제안드립니다:
-@Bean
-public SesClient sesClient() {
- return SesClient.builder()
- .region(Region.AP_NORTHEAST_2)
- .credentialsProvider(StaticCredentialsProvider.create(
- AwsBasicCredentials.create(accessKey, secretKey)
- ))
- .build();
-}새로운 AmazonSesConfig 클래스를 생성하여 다음과 같이 구현:
@Configuration
public class AmazonSesConfig {
@Value("${cloud.aws.region.static}")
private String region;
@Value("${spring.s3.access-key}")
private String accessKey;
@Value("${spring.s3.secret-key}")
private String secretKey;
@Bean
public SesClient sesClient() {
return SesClient.builder()
.region(Region.of(region))
.credentialsProvider(StaticCredentialsProvider.create(
AwsBasicCredentials.create(accessKey, secretKey)
))
.build();
}
}| sesClient.sendEmail(request); | ||
| } catch (Exception e) { | ||
| log.error("Failed to send email to {}: {}", application.getEmail(), e.getMessage()); | ||
| throw new RuntimeException("이메일 전송에 실패했습니다: " + application.getEmail(), e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
예외 처리 개선이 필요합니다.
현재 구현에서는 모든 예외를 RuntimeException으로 래핑하고 있습니다. AWS SES 관련 구체적인 예외 처리가 필요합니다.
다음과 같이 수정하는 것을 제안합니다:
try {
sesClient.sendEmail(request);
} catch (Exception e) {
log.error("Failed to send email to {}: {}", application.getEmail(), e.getMessage());
- throw new RuntimeException("이메일 전송에 실패했습니다: " + application.getEmail(), e);
+ throw new EmailSendException("이메일 전송에 실패했습니다: " + application.getEmail(), e);
}📝 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.
| sesClient.sendEmail(request); | |
| } catch (Exception e) { | |
| log.error("Failed to send email to {}: {}", application.getEmail(), e.getMessage()); | |
| throw new RuntimeException("이메일 전송에 실패했습니다: " + application.getEmail(), e); | |
| } | |
| sesClient.sendEmail(request); | |
| } catch (Exception e) { | |
| log.error("Failed to send email to {}: {}", application.getEmail(), e.getMessage()); | |
| throw new EmailSendException("이메일 전송에 실패했습니다: " + application.getEmail(), e); | |
| } |
| @Operation(summary = "동아리 지원 결과 이메일 전송 API") | ||
| @ApiResponse(responseCode = "201", description = "동아리 지원 결과 이메일 전송 성공") | ||
| @ResponseStatus(HttpStatus.OK) | ||
| @SecurityRequirement(name = "AccessToken") |
There was a problem hiding this comment.
HTTP 상태 코드가 일관되지 않습니다.
@ApiResponse는 201을 명시하고 있지만, @ResponseStatus는 200을 반환하도록 설정되어 있습니다. 리소스 생성을 의미하는 201로 통일하는 것이 좋습니다.
다음과 같이 수정하는 것을 제안합니다:
@Operation(summary = "동아리 지원 결과 이메일 전송 API")
@ApiResponse(responseCode = "201", description = "동아리 지원 결과 이메일 전송 성공")
-@ResponseStatus(HttpStatus.OK)
+@ResponseStatus(HttpStatus.CREATED)
@SecurityRequirement(name = "AccessToken")📝 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.
| @Operation(summary = "동아리 지원 결과 이메일 전송 API") | |
| @ApiResponse(responseCode = "201", description = "동아리 지원 결과 이메일 전송 성공") | |
| @ResponseStatus(HttpStatus.OK) | |
| @SecurityRequirement(name = "AccessToken") | |
| @Operation(summary = "동아리 지원 결과 이메일 전송 API") | |
| @ApiResponse(responseCode = "201", description = "동아리 지원 결과 이메일 전송 성공") | |
| @ResponseStatus(HttpStatus.CREATED) | |
| @SecurityRequirement(name = "AccessToken") |
| ses: | ||
| sender-email: ${SENDER_EMAIL} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
AWS SES 설정이 추가되었습니다.
환경 변수를 통한 이메일 설정은 좋은 접근이지만, 다음 사항들을 확인해 주시기 바랍니다:
- SES 서비스가 프로덕션 환경에서 샌드박스 모드를 벗어났는지 확인
- 발신자 이메일이 AWS SES에서 인증되었는지 확인
다음 스크립트를 실행하여 환경 변수가 올바르게 설정되어 있는지 확인하세요:
🏁 Script executed:
#!/bin/bash
# Description: Verify environment variable for sender email
# Check if SENDER_EMAIL environment variable is set
if [ -z "${SENDER_EMAIL}" ]; then
echo "Warning: SENDER_EMAIL environment variable is not set"
exit 1
fi
# Check if the email format is valid
if ! echo "${SENDER_EMAIL}" | grep -E -q '^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$'; then
echo "Warning: SENDER_EMAIL format is invalid"
exit 1
fiLength of output: 141
환경 변수 'SENDER_EMAIL' 설정 누락
- 실행 결과 SENDER_EMAIL 환경 변수가 설정되어 있지 않음이 확인되었습니다.
- 이로 인해 AWS SES 설정에서 발신자 이메일을 올바르게 로드할 수 없습니다.
- 배포 전 해당 환경 변수를 올바른 인증 이메일로 설정하고, AWS SES 샌드박스 모드 및 이메일 인증 상태도 재확인해 주세요.
| @@ -55,6 +55,7 @@ dependencies { | |||
| implementation 'io.sentry:sentry-logback:7.6.0' | |||
| implementation 'com.fasterxml.jackson.core:jackson-core' | |||
| implementation 'com.github.f4b6a3:uuid-creator:6.0.0' | |||
| implementation 'software.amazon.awssdk:ses:2.24.0' | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
AWS SDK 버전 검증이 필요합니다.
AWS SDK for SES의 의존성이 추가되었습니다. 다음 사항들을 확인해 주시기 바랍니다:
- 최신 안정 버전 사용 여부
- 기존 AWS 의존성과의 호환성 (
io.awspring.cloud:spring-cloud-starter-aws:2.4.4)
AWS SDK 버전을 확인하기 위해 다음 웹 검색을 실행해 주세요:
🌐 Web query:
What is the latest stable version of software.amazon.awssdk:ses?
💡 Result:
The latest stable version of software.amazon.awssdk:ses as of February 2025 is 2.30.16, based on recent AWS SDK updates[7][9]. This version includes enhancements like improved Amazon SES Mail Manager features for traffic policy management[7].
For Maven projects, use:
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>ses</artifactId>
<version>2.30.16</version>
</dependency>Key details:
- The AWS SDK for Java 2.x follows semantic versioning with regular updates[6][10]
- Version 2.x replaced legacy 1.x SDKs (now in maintenance mode)[1][3]
- Recent updates focus on checksum validation improvements and new SES capabilities[7][9]
Always verify dependencies against official AWS documentation or Maven Central for real-time version tracking[4][8].
Citations:
- 1: https://docs.aws.amazon.com/sdkref/latest/guide/version-support-matrix.html
- 2: https://docs.aws.amazon.com/ses/latest/dg/send-an-email-using-sdk-programmatically.html
- 3: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/migration-whats-different.html
- 4: https://central.sonatype.com/artifact/software.amazon.awssdk/ses/2.17.180
- 5: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/java_ses_code_examples.html
- 6: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/home.html
- 7: https://github.com/aws/aws-sdk-java-v2/blob/master/CHANGELOG.md
- 8: https://mvnrepository.com/artifact/com.amazonaws/aws-java-sdk-ses
- 9: https://sdk.amazonaws.com/java/api/latest/
- 10: https://github.com/aws/aws-sdk-java-v2
AWS SDK SES 의존성 버전 업데이트 필요
현재 build.gradle 파일에서는 software.amazon.awssdk:ses:2.24.0 버전을 사용 중입니다. 최신 웹 검색 결과에 따르면 February 2025 기준 최신 안정 버전은 2.30.16입니다.
따라서, 아래 사항을 검토해 주시기 바랍니다:
- AWS SDK for SES 의존성을 최신 안정 버전(2.30.16)으로 업데이트할 것
io.awspring.cloud:spring-cloud-starter-aws:2.4.4와의 호환성 재확인
# Conflicts: # src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeCentralClubServiceImplTest.java (1)
99-113: 🛠️ Refactor suggestion테스트 케이스 보완이 필요합니다.
테스트의 검증 범위가 제한적입니다. 다음 사항들을 보완해 주시기 바랍니다:
- 모든 필드의 업데이트가 정상적으로 이루어졌는지 검증
null값이 허용되는 필드에 대한 유효성 검사- 잘못된 형식의 데이터 입력 시 예외 처리 검증
다음과 같이 테스트를 보완해 주세요:
//then Club result = clubRepository.findById(savedClub.getId()).orElseThrow(); assertThat(result.getName()).isEqualTo("testname"); +assertThat(result.getCategory()).isEqualTo("testcategory"); +assertThat(result.getTag()).isEqualTo("testtag"); +assertThat(result.getClubLeader()).isEqualTo("testclubLeader"); +assertThat(result.getPhoneNumber()).isEqualTo(PhoneNumber.from("010-1234-5678")); +assertThat(result.getLocation()).isEqualTo(Location.from("S1111")); +assertThat(result.getIntroduction()).isEqualTo("testintroduction"); +assertThat(result.getActivity()).isEqualTo("testactivity"); +assertThat(result.getIdeal()).isEqualTo("testideal");
🧹 Nitpick comments (4)
src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeCentralClubServiceImplTest.java (1)
84-86: 테스트 메서드 설명 개선이 필요합니다.
@DisplayName에 테스트하는 구체적인 시나리오를 명시하면 테스트의 목적을 더 잘 이해할 수 있습니다.다음과 같이 변경해 주세요:
-@DisplayName("중앙동아리: 동아리 정보 수정") +@DisplayName("중앙동아리: 동아리 정보를 성공적으로 수정하고 변경된 정보가 저장되는지 확인한다")src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (1)
19-31: 커서 기반 페이지네이션 구현이 잘 되었습니다.네이티브 쿼리를 사용한 커서 기반 페이지네이션이 효율적으로 구현되었습니다. 오프셋 기반 페이지네이션보다 성능이 더 좋은 접근 방식입니다.
다만, 다음 사항들을 고려해보시면 좋을 것 같습니다:
- 인덱스 추가를 통한 쿼리 최적화
- 결과가 없을 경우의 처리 방식 명시
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (2)
184-189: 예외 처리 개선 필요현재 예외 처리에 다음과 같은 개선이 필요합니다:
- 구체적인 예외 클래스 사용
- 타임아웃 시간을 상수로 분리
- 더 자세한 에러 메시지 제공
다음과 같이 수정하는 것을 제안드립니다:
+ private static final long EMAIL_SEND_TIMEOUT_MINUTES = 5; + try { - future.get(5, TimeUnit.MINUTES); // 최대 5분 대기 + future.get(EMAIL_SEND_TIMEOUT_MINUTES, TimeUnit.MINUTES); } catch (Exception e) { - log.error("Failed to send bulk emails", e); - throw new RuntimeException("이메일 전송 중 오류가 발생했습니다.", e); + log.error("이메일 전송 실패 - formId: {}, target: {}, error: {}", + command.formId(), command.target(), e.getMessage(), e); + throw new EmailSendException("지원 결과 이메일 전송에 실패했습니다. 잠시 후 다시 시도해주세요.", e); }
233-234: 메서드 파라미터 들여쓰기 일관성 유지 필요메서드 파라미터의 들여쓰기가 파일 전체적으로 일관성이 없습니다. 코드 가독성을 위해 일관된 스타일을 유지하는 것이 좋습니다.
Also applies to: 239-240
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build.gradle(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/club/service/dto/command/UpdateClubInfoCommand.java(0 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(6 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/email/SesEmailService.java(1 hunks)src/main/resources/application-test.yml(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeCentralClubServiceImplTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/club/service/dto/command/UpdateClubInfoCommand.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java
- build.gradle
- src/main/java/ddingdong/ddingdongBE/email/SesEmailService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (3)
src/main/resources/application-test.yml (1)
35-36: SES 구성 추가 확인 필요새로 추가된
ses섹션과 그 하위의sender-email프로퍼티가 올바른 계층 구조로 구성되어 있습니다. 이 설정은 AWS SES를 통한 이메일 전송 기능에서 발신자 이메일을 지정하는 데 사용됩니다.
추가로, 로컬 및 개발 환경에서도 동일한 환경변수를 반영할 수 있도록 관련 설정 파일을 업데이트했는지 확인하시기 바랍니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (1)
75-77:❓ Verification inconclusive
메소드 네이밍과 기능이 명확합니다.
getAllByFormIdAndStatus메소드는 JPA 네이밍 컨벤션을 잘 따르고 있으며, 이메일 전송을 위한 지원자 필터링 기능을 명확하게 구현하고 있습니다.다음 사항을 검토해보시면 좋을 것 같습니다:
- 상태값이 null일 경우의 처리 방식
- 결과가 없을 경우 빈 리스트 반환 여부 확인
🏁 Script executed:
#!/bin/bash # FormApplicationService에서 null 처리 로직 확인 ast-grep --pattern 'class FormApplicationService { $$$ getAllByFormIdAndStatus($_, $_) { $$$ } $$$ }'Length of output: 115
검토 요청: 서비스 레이어 내 null 처리 및 빈 리스트 반환 검사 필요
현재
FormApplicationRepository의 메소드 네이밍과 기능은 JPA 명명 규칙에 부합하여 문제가 없어 보입니다. 다만, 이 메소드가 사용되는FormApplicationService내에서 아래 두 가지 사항에 대한 명시적 검토가 필요합니다.
- 상태값 null 처리: 현재 서비스 레이어 내에서
status값이 null일 경우를 어떻게 처리하는지 확인할 수 없습니다. null 값에 대해 명확한 예외 처리나 기본 값 설정 로직이 있는지 추가 검토가 필요합니다.- 빈 리스트 반환 여부: 조회 결과가 없을 때 JPA가 빈 리스트를 반환하는 기본 동작 외에, 별도의 후처리 로직이 적용되고 있는지 확인해 주시기 바랍니다.
위 사항에 대해 서비스 레이어의 구현을 재검토한 후, 필요시 적절한 수정이나 명시적인 예외 처리를 추가해 주시면 좋겠습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
18-19: 이메일 관련 의존성이 적절히 추가되었습니다!이메일 전송 기능 구현을 위한 필요한 의존성들이 잘 추가되었습니다.
Also applies to: 27-29, 35-36, 40-41, 43-43, 50-50, 59-59
| public void sendApplicationResultEmail(SendApplicationResultEmailCommand command) { | ||
| List<FormApplication> formApplications = formApplicationService.getAllByFormIdAndFormApplicationStatus( | ||
| command.formId(), | ||
| FormApplicationStatus.findStatus(command.target()) | ||
| ); | ||
| EmailContent emailContent = EmailContent.of(command.title(), command.message()); | ||
| CompletableFuture<Void> future = sesEmailService.sendBulkResultEmails(formApplications, emailContent); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이메일 전송 로직에 대한 유효성 검사 추가 필요
현재 구현에 다음 사항들을 추가하면 좋을 것 같습니다:
- formApplications가 비어있는 경우에 대한 처리
- 이메일 제목과 내용의 유효성 검사 (null, 빈 문자열 등)
다음과 같이 수정하는 것을 제안드립니다:
public void sendApplicationResultEmail(SendApplicationResultEmailCommand command) {
+ if (command.title() == null || command.title().trim().isEmpty()) {
+ throw new IllegalArgumentException("이메일 제목은 필수입니다.");
+ }
+ if (command.message() == null || command.message().trim().isEmpty()) {
+ throw new IllegalArgumentException("이메일 내용은 필수입니다.");
+ }
List<FormApplication> formApplications = formApplicationService.getAllByFormIdAndFormApplicationStatus(
command.formId(),
FormApplicationStatus.findStatus(command.target())
);
+ if (formApplications.isEmpty()) {
+ log.warn("지원자가 없습니다. formId: {}, status: {}", command.formId(), command.target());
+ return;
+ }
EmailContent emailContent = EmailContent.of(command.title(), command.message());
CompletableFuture<Void> future = sesEmailService.sendBulkResultEmails(formApplications, emailContent);📝 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 sendApplicationResultEmail(SendApplicationResultEmailCommand command) { | |
| List<FormApplication> formApplications = formApplicationService.getAllByFormIdAndFormApplicationStatus( | |
| command.formId(), | |
| FormApplicationStatus.findStatus(command.target()) | |
| ); | |
| EmailContent emailContent = EmailContent.of(command.title(), command.message()); | |
| CompletableFuture<Void> future = sesEmailService.sendBulkResultEmails(formApplications, emailContent); | |
| public void sendApplicationResultEmail(SendApplicationResultEmailCommand command) { | |
| if (command.title() == null || command.title().trim().isEmpty()) { | |
| throw new IllegalArgumentException("이메일 제목은 필수입니다."); | |
| } | |
| if (command.message() == null || command.message().trim().isEmpty()) { | |
| throw new IllegalArgumentException("이메일 내용은 필수입니다."); | |
| } | |
| List<FormApplication> formApplications = formApplicationService.getAllByFormIdAndFormApplicationStatus( | |
| command.formId(), | |
| FormApplicationStatus.findStatus(command.target()) | |
| ); | |
| if (formApplications.isEmpty()) { | |
| log.warn("지원자가 없습니다. formId: {}, status: {}", command.formId(), command.target()); | |
| return; | |
| } | |
| EmailContent emailContent = EmailContent.of(command.title(), command.message()); | |
| CompletableFuture<Void> future = sesEmailService.sendBulkResultEmails(formApplications, emailContent); | |
| } |
.../java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java
Outdated
Show resolved
Hide resolved
| @@ -203,13 +231,13 @@ private void validateDuplicationDateExcludingSelf( | |||
| } | |||
|
|
|||
| private List<FormField> toUpdateFormFields(Form originform, | |||
| List<UpdateFormFieldCommand> updateFormFieldCommands) { | |||
| List<UpdateFormFieldCommand> updateFormFieldCommands) { | |||
|
🚀 작업 내용
동아리 지원 결과 email 전송 API 구현
현재 aws ses 프로덕션이 해제되지 않아 실제 테스트를 수행하지 못한 상태입니다. 프로덕션 해제 완료 시 테스트 수행 부탁드립니다..!
+추가로 새롭게 추가된 환경변수가 존재하니 로컬 및 개발환경에 추가 부탁드립니다.
🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
Summary by CodeRabbit