Feature/mosu 28 : 알림톡 플로우 개선 (Luna Soft)#116
Conversation
…tonUrls로 수정 및 메서드 시그니처 정리
…처 변경 및 템플릿 코드 사용으로 개선
…o feature/mosu-28
|
Caution Review failedThe pull request is closed. WalkthroughThis update refactors the notification system, replacing the previous strategy-based approach with a modular design centered on Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NotifyEventPublisher
participant NotifyEventListener
participant NotifyService
participant NotifyVariableFactory
participant NotifySenderResolver
participant NotifySender
Client->>NotifyEventPublisher: notify(NotificationEvent)
NotifyEventPublisher->>NotifyEventListener: publish(NotificationEvent)
NotifyEventListener->>NotifyService: notify(NotificationEvent)
NotifyService->>NotifyVariableFactory: create(NotificationEvent)
NotifyVariableFactory-->>NotifyService: NotificationVariable
NotifyService->>NotifySenderResolver: resolve(NotificationStatus)
NotifySenderResolver-->>NotifyService: NotifySender
NotifyService->>NotifySender: send(phoneNumber, NotificationEvent, NotificationVariable)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @polyglot-k, 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!
This pull request implements a significant architectural improvement to the application's notification system, specifically targeting the 'AlimTalk' (KakaoTalk Business messaging) flow. The changes move away from a rigid, string-based strategy pattern to a more robust, type-safe, and extensible design. By introducing dedicated sender components, standardizing notification data through a new interface, and centralizing configuration via enums, the system is now more modular, easier to maintain, and better prepared for future notification types and integrations.
Highlights
- Refactored Notification Dispatch: Replaced the old
NotifyStrategypattern with a newNotifySenderinterface andNotifySenderResolverfor more flexible and maintainable notification dispatch, allowing different notification types to be handled by dedicated sender components. - Standardized Notification Data: Introduced the
NotificationVariableinterface and specific DTOs (e.g.,ApplicationNotifyRequest,RefundNotifyRequest) to uniformly encapsulate data for various notification types, including associated button URLs. - Centralized Configuration: Consolidated notification template IDs, AlimTalk codes, SMS fallback messages, and redirect URLs into new enums (
NotificationStatus,NotifyTemplateCode,NotifyRedirectUrlConstants), significantly improving clarity and reducing hardcoding. - Improved Phone Number Handling: Added a utility method to
ProfileJpaEntityto retrieve phone numbers without hyphens, streamlining data processing within the notification flow. - Optimized JPA Queries: Updated
ApplicationSchoolJpaRepositoryandRefundJpaRepositoryto leverage constructor expressions for projections andOptionalreturn types, enhancing data retrieval efficiency and null safety.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes introduce a significant and well-designed refactoring of the notification system. The move from a string-based strategy resolution using ApplicationContext to a type-safe, annotation-driven NotifySenderResolver is a major improvement for maintainability and robustness. The introduction of NotificationVariable, NotifyVariableFactory, and specific request records cleans up the DTO layer and centralizes logic effectively.
My review focuses on a few potential correctness issues, including a critical one related to template IDs, and some opportunities to improve maintainability by reducing code duplication and removing dead code. Overall, this is a great enhancement to the notification flow.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyTemplateCode.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/notify/NotifyService.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
Show resolved
Hide resolved
| String paymentKey, | ||
| LocalDate examDate, | ||
| String schoolName, | ||
| // String amount, |
There was a problem hiding this comment.
Commented-out code should be removed to keep the codebase clean and avoid confusion.
src/main/java/life/mosu/mosuserver/application/notify/dto/RefundNotifyRequest.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventTemplateGenerator.java
Show resolved
Hide resolved
| @Override | ||
| public void send(String targetPhoneNumber, NotificationEvent event, T dto) { | ||
| NotifyTemplateCode templateCode = event.status().getTemplateCode(); | ||
| log.info("templateCode : {}", templateCode); |
There was a problem hiding this comment.
This log.info statement seems to be intended for debugging purposes. In a production environment, logging this information at the INFO level for every notification could be too verbose and might impact performance. Please consider changing the log level to DEBUG or removing the statement if it's no longer needed.
| log.info("templateCode : {}", templateCode); | |
| log.debug("templateCode : {}", templateCode); |
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationStatus.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventTemplateGenerator.java (1)
36-44: Add null check to prevent NullPointerException.The method lacks null checking for the
variablesparameter, which could cause a NullPointerException at line 38. This is inconsistent with the first overloaded method.public <T extends NotificationVariable> String getProcessedMessage(String templateCode, T variables) { + if (variables == null) { + return messageSource.getMessage(templateCode, null, Locale.KOREA); + } Map<String, String> data = variables.toMap(); String message = messageSource.getMessage(templateCode, null, Locale.KOREA); for (Map.Entry<String, String> entry : data.entrySet()) { message = StringUtils.replace(message, "#{" + entry.getKey() + "}", entry.getValue()); } return message; }
♻️ Duplicate comments (1)
src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1DayBeforeNotifyRequest.java (1)
18-24: Same label issue as in Exam3DayBeforeNotifyRequest.This method has the identical implementation and the same issue with using constants as user-facing labels.
Refer to the comment on
Exam3DayBeforeNotifyRequestregarding using appropriate user-friendly labels instead of technical constants.
🧹 Nitpick comments (17)
src/main/java/life/mosu/mosuserver/infra/notify/annotation/NotifyStrategyMapping.java (1)
11-14: Consider enhancing the annotation with documentation and validation.The annotation design is solid for runtime reflection-based mapping. However, consider these improvements:
- Add JavaDoc documentation explaining the annotation's purpose and usage
- Consider renaming the
value()method to be more descriptive likestatuses()orsupportedStatuses()- The Korean comment should be in English for consistency with the codebase
+/** + * Maps notification statuses to sender implementations. + * Used by NotifySenderResolver for dynamic sender resolution. + */ @Target(ElementType.TYPE) @Retention(RetentionPolicy.RUNTIME) public @interface NotifyStrategyMapping { - NotificationStatus[] value(); // 여러 상태 매핑 허용 + /** + * The notification statuses supported by the annotated sender. + * Multiple statuses can be mapped to a single sender implementation. + */ + NotificationStatus[] value(); // Allow multiple status mappings }src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationVariable.java (1)
5-10: Add documentation to clarify the interface contract.The interface design is clean and well-focused. Consider adding JavaDoc documentation to clarify the purpose and usage of each method:
+/** + * Contract for notification variable DTOs that provide data for template processing. + */ public interface NotificationVariable { + /** + * Returns the notification button URLs for the notification. + * @return NotificationButtonUrls containing button URL data + */ NotificationButtonUrls getNotificationButtonUrls(); + /** + * Converts the notification variable to a map for template processing. + * @return Map containing variable data as key-value pairs + */ Map<String, String> toMap(); }src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java (1)
9-11: Consider removing the redundant static factory method.The static factory method
create()doesn't add value over the record's generated constructor. Records already provide a clean constructor with the same signature.- public static NotificationEvent create(NotificationStatus status, Long userId, Long targetId) { - return new NotificationEvent(status, userId, targetId); - }If you need the factory method for consistency with other parts of the codebase, consider adding validation or documentation to justify its existence.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationStatus.java (1)
8-54: Consider internationalization for notification messages.The enum design is well-structured and comprehensive. However, the Korean messages are hardcoded, which might limit internationalization support:
Consider using message keys instead of hardcoded Korean text:
SIGN_UP_SUCCESS( NotificationSendStatus.SUCCESS, - "회원 가입 완료", + "notification.signup.success", false, NotifyTemplateCode.SIGN_UP),This would allow for easier localization and message management through properties files.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationSendStatus.java (1)
3-5: Consider future-proofing the status enum
SUCCESS,FAILURE, andINFOcover the immediate cases, but the notification domain often needs finer-grained delivery states (e.g.,QUEUED,RETRYING,PARTIAL_SUCCESS). Introducing a catch-all likePENDINGor reserving anUNKNOWNvalue now can avoid breaking changes later if new providers or async flows are added.src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaEntity.java (1)
101-104: Avoid regex overhead ingetPhoneNumberWithoutHyphen()
String#replaceAllcompiles a regex every call. For a simple literal substitution,String#replaceis faster and clearer.- public String getPhoneNumberWithoutHyphen() { - return getPhoneNumber().replaceAll("-", ""); - } + public String getPhoneNumberWithoutHyphen() { + return getPhoneNumber().replace("-", ""); + }src/main/java/life/mosu/mosuserver/presentation/notify/NotifyEventListener.java (1)
5-6: Mask PII when logging notification events
NotificationEventlikely contains user identifiers or payload details. Logging the full object at INFO level (log.info("Notify event: {}", event);) may expose PII in aggregated logs.
Consider logging only a safe subset (e.g., event id/status) or downgrading to DEBUG.Also applies to: 18-20
src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventPublisher.java (1)
3-4: Optional: add trace-level logging for published eventsA short
log.debug("Publishing notification event: {}", event);beforepublishEventhelps trace asynchronous flows when troubleshooting without impacting production logs.Also applies to: 14-16
src/main/java/life/mosu/mosuserver/application/notify/dto/SignUpNotifyRequest.java (1)
16-18: Consider the button URL mapping logic.The button URL mapping
HOME_PAGEtoHOME_PAGEappears redundant. Typically, you'd map a button label to a URL. Consider if this should be something likeNotificationButtonUrl.of("홈페이지", HOME_PAGE)or if the current mapping serves a specific purpose.src/main/java/life/mosu/mosuserver/application/notify/dto/InquiryAnswerNotifyRequest.java (1)
16-18: Same button URL mapping pattern as other notification requests.The button URL mapping follows the same pattern as
SignUpNotifyRequest. If this is intentional for consistency, it's fine. Otherwise, consider if button labels should be more descriptive.src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1WeekBeforeNotifyRequest.java (1)
27-27: Consider proper date formatting for user-friendly notifications.Using
examDate.toString()may not provide user-friendly date formatting for notifications. Consider usingDateTimeFormatterto format the date appropriately for the target audience.- "examDate", examDate.toString(), + "examDate", examDate.format(DateTimeFormatter.ofPattern("yyyy년 MM월 dd일")),src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1DayBeforeNotifyRequest.java (1)
12-16: Consider refactoring to reduce code duplication.This record has identical fields and structure to
Exam3DayBeforeNotifyRequest. The only difference appears to be the class name, which suggests potential for refactoring.Consider creating a common base record or using a single parameterized class:
public record ExamReminderNotifyRequest( LocalDate examDate, String examinationNumber, String schoolName, NotificationTimingType timingType ) implements NotificationVariable { // Common implementation }Or use a factory pattern to create instances with different button configurations if needed.
src/main/java/life/mosu/mosuserver/infra/notify/resolver/NotifySenderResolver.java (1)
22-22: Consider using English error messages or externalizing them for internationalization.While the implementation is correct, hardcoded Korean error messages might limit maintainability if the team becomes international. Consider using English messages or externalizing them to a message properties file.
- throw new IllegalStateException("NotifySender에 @NotifyStrategyMapping 없음"); + throw new IllegalStateException("NotifySender must have @NotifyStrategyMapping annotation");- throw new IllegalStateException("중복된 NotifyStatus: " + status); + throw new IllegalStateException("Duplicate NotifyStatus mapping: " + status);- throw new IllegalArgumentException("전략이 등록되지 않은 상태: " + status); + throw new IllegalArgumentException("No strategy registered for status: " + status);Also applies to: 27-27, 38-38
src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventTemplateGenerator.java (1)
18-18: Remove unnecessary class-level type parameter.The type parameter
<T>at the class level is not used anywhere in the class. The methods already define their own type parameters.-public class NotifyEventTemplateGenerator<T> { +public class NotifyEventTemplateGenerator {src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationButtonUrls.java (1)
29-41: Clean implementation of the button URL record.The nested record is well-structured with appropriate factory and conversion methods. Consider adding URL validation if needed in the future.
src/main/java/life/mosu/mosuserver/application/notify/dto/ApplicationNotifyRequest.java (1)
39-46: Consider date formatting consistency.The
examDate.toString()on line 42 will use the default ISO format. Ensure this matches the expected format in notification templates.@Override public Map<String, String> toMap() { + DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd"); // or desired format return Map.of( "paymentKey", paymentKey, - "examDate", examDate.toString(), + "examDate", examDate.format(formatter), "schoolName", schoolName, "lunch", lunch ); }src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java (1)
42-44: Enhance error message with available options.Consider improving the error message to help developers understand valid options.
default -> - throw new IllegalArgumentException("지원하지 않는 NotifyStatus: " + event.status()); + throw new IllegalArgumentException( + String.format("Unsupported NotificationStatus: %s. Supported statuses: %s", + event.status(), + Arrays.toString(NotificationStatus.values())) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
src/main/java/life/mosu/mosuserver/application/notify/NotifyService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/ApplicationNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1DayBeforeNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1WeekBeforeNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/Exam3DayBeforeNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/InquiryAnswerNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/RefundNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/SignUpNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventPublisher.java(2 hunks)src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventTemplateGenerator.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/annotation/NotifyStrategyMapping.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/component/NotifySender.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/component/luna/LunaNotifySender.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/component/luna/LunaNotifyWithSmsFallbackSender.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/constant/NotifyConstants.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/constant/NotifyRedirectUrlConstants.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/ApplicationNotifyVariablesDto.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/Exam1DayBeforeNotifyVariablesDto.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/Exam1WeekBeforeNotifyVariablesDto.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/Exam3DayBeforeNotifyVariablesDto.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/InquiryAnswerNotifyVariablesDto.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationButtonUrls.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationSendStatus.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationStatus.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationVariable.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyButtonUrl.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyButtonUrls.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyEvent.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyEventRemindMessageDto.java(2 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyEventSuccessMessageDto.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyStatus.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyTemplateCode.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/RefundNotifyVariablesDto.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/resolver/NotifySenderResolver.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/ApplicationNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/Exam1DayBeforeNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/Exam1WeekBeforeNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/Exam3DayBeforeNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/InquiryAnswerNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/NotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/RefundNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/notify/strategy/SignUpNotifyStrategy.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/notify/NotifyEventListener.java(2 hunks)
💤 Files with no reviewable changes (19)
- src/main/java/life/mosu/mosuserver/infra/notify/dto/InquiryAnswerNotifyVariablesDto.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyStatus.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/NotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyEvent.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/RefundNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/Exam1DayBeforeNotifyVariablesDto.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/Exam3DayBeforeNotifyVariablesDto.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyButtonUrl.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/InquiryAnswerNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/ApplicationNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/Exam1DayBeforeNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/Exam1WeekBeforeNotifyVariablesDto.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/Exam1WeekBeforeNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyButtonUrls.java
- src/main/java/life/mosu/mosuserver/infra/notify/constant/NotifyConstants.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/Exam3DayBeforeNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/strategy/SignUpNotifyStrategy.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/RefundNotifyVariablesDto.java
- src/main/java/life/mosu/mosuserver/infra/notify/dto/ApplicationNotifyVariablesDto.java
🔇 Additional comments (32)
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationStatus.java (1)
14-47: Verify that all fallback flags are intentionally set to false.All notification statuses have
isFallbackset tofalse. This might indicate:
- Fallback mechanism is not yet implemented
- These specific notifications don't require fallback
- Configuration oversight
Please verify that the fallback configuration is correct for all notification types, especially for critical notifications like exam reminders that users might need to receive even if the primary notification fails.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyEventRemindMessageDto.java (1)
13-14: Null-safety forbtnUrls
toMap()unconditionally callsbtnUrls.toMapList(). If callers accidentally passnull, an NPE will be thrown at runtime.
Either enforce non-null withObjects.requireNonNullin the factory/constructor or document the contract.Also applies to: 22-23
src/main/java/life/mosu/mosuserver/infra/notify/component/NotifySender.java (1)
5-8: Well-designed interface following clean architecture principles.The generic interface design is excellent - it provides a clean contract for notification senders while allowing flexibility through the generic type parameter
Tfor different reference objects. The method signature is clear and purposeful.src/main/java/life/mosu/mosuserver/application/notify/dto/SignUpNotifyRequest.java (1)
22-24: Empty template variables are appropriate for signup notifications.The empty map return is correct for signup notifications that don't require dynamic template variables.
src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java (1)
9-22: Repository method correctly updated for new notification projection.The JPQL query enhancement to include
schoolNamein the projection is well-implemented. The constructor expression syntax is correct, and the use ofOptional<RefundNotifyProjection>follows good practices for nullable results.src/main/java/life/mosu/mosuserver/application/notify/dto/InquiryAnswerNotifyRequest.java (1)
22-26: Proper template variable mapping for inquiry notifications.The template variable mapping is correctly implemented, providing the inquiry title for notification templates.
src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1WeekBeforeNotifyRequest.java (1)
25-31: Well-structured template variable mapping.The template variable mapping correctly includes all relevant fields for exam reminder notifications.
src/main/java/life/mosu/mosuserver/application/notify/dto/Exam3DayBeforeNotifyRequest.java (2)
1-11: Clean imports and package structure.The package declaration follows Java conventions and all imports are necessary and properly organized.
26-33: Clean map conversion implementation.The method correctly converts the record fields to a string map. Using
examDate.toString()produces ISO format which is appropriate for template processing.src/main/java/life/mosu/mosuserver/application/notify/dto/Exam1DayBeforeNotifyRequest.java (2)
1-11: Consistent imports and package structure.The imports are identical to
Exam3DayBeforeNotifyRequest, maintaining consistency across similar notification DTOs.
26-33: Consistent map conversion implementation.The implementation is identical to
Exam3DayBeforeNotifyRequestand follows the same correct pattern for field mapping.src/main/java/life/mosu/mosuserver/application/notify/dto/RefundNotifyRequest.java (1)
1-10: Clean and appropriate imports.The imports are tailored to this specific notification type, importing only the necessary constants and classes.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyTemplateCode.java (2)
1-8: Clean enum structure with appropriate Lombok usage.The enum is well-structured with appropriate use of Lombok annotations to reduce boilerplate code.
40-42: Properly declared enum fields.The field declarations are appropriate with correct types and access modifiers.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotifyEventSuccessMessageDto.java (4)
6-13: Appropriate type migration to new notification architecture.The change from
NotifyButtonUrlstoNotificationButtonUrlsaligns with the broader notification system refactoring and improves type safety.
15-24: Well-designed factory method for standard notifications.The
createmethod provides a clean interface for notifications without SMS fallback, with appropriate default values.
26-36: Good design for SMS fallback factory method.The method appropriately hardcodes
useSmsto"1"since SMS fallback is explicitly requested, and requires explicitsmsContentto ensure proper SMS content is provided.
39-49: Correctly adapted to new button URL type.The
toMapmethod properly uses the newNotificationButtonUrlstype while maintaining the same interface and functionality.src/main/java/life/mosu/mosuserver/infra/notify/resolver/NotifySenderResolver.java (2)
17-32: Well-implemented constructor with proper validation.The constructor correctly validates annotation presence and prevents duplicate status mappings. The map is effectively immutable after construction, ensuring thread safety.
34-41: The unchecked cast is appropriately handled.The
@SuppressWarnings("unchecked")is necessary here due to Java's type erasure limitations when dealing with wildcard generics. The implementation is correct for this use case.src/main/java/life/mosu/mosuserver/application/notify/NotifyService.java (2)
23-30: Clean and well-structured notification flow.The refactored implementation excellently separates concerns with clear responsibilities for each component. The flow is easy to follow and maintain.
32-36: Verify downstream compatibility for phone number formattingOur searches across all
*NotifySender*.javaand*NotifyClient*.javaclasses did not reveal any hyphen-stripping or other phone-number sanitization logic. Please manually confirm that every notification channel and external system invoked byNotifyServicecan handle phone numbers containing hyphens.src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java (2)
22-30: Projection record confirmed and matches constructor parametersThe
ApplicationSchoolNotifyProjectionrecord is declared in
src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolNotifyProjection.java
with the canonical constructor(String paymentKey, LocalDate examDate, String schoolName, Lunch lunch), exactly matching the query’s constructor expression.Approve the implementation.
32-40: Approved: Query and projection class verifiedThe
OneWeekNotifyProjectionrecord exists insrc/main/java/life/mosu/mosuserver/domain/applicationschool/OneWeekNotifyProjection.javawith the exact constructor signature(String paymentKey, LocalDate examDate, String schoolName), matching the JPQL constructor expression. The use of a regularJOINcorrectly enforces that a payment must exist for one-week notifications, aligning with business requirements.src/main/java/life/mosu/mosuserver/infra/notify/NotifyEventTemplateGenerator.java (1)
22-34: Well-implemented method with proper null handling.The method correctly handles null variables by returning the unprocessed message template, which is a good defensive programming practice.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationButtonUrls.java (1)
7-27: Excellent implementation with proper immutability guarantees.The record follows best practices:
- Defensive copying in the compact constructor
- Null-safe handling with default empty list
- Convenient factory methods
- Clean conversion to map list
src/main/java/life/mosu/mosuserver/infra/notify/component/luna/LunaNotifySender.java (2)
20-31: LGTM! Clean class structure with appropriate annotations.The generic type parameter and annotation-based mapping provide good type safety and clear intent.
29-30: Good use of constructor injection with final fields.The dependencies are properly typed and immutable.
src/main/java/life/mosu/mosuserver/application/notify/dto/ApplicationNotifyRequest.java (2)
13-18: Well-structured immutable record.Good use of Java records for the DTO with appropriate field types.
30-36: NotificationButtonUrl parameters are intentionally identical.After checking all notification request DTOs, each uses the same constant for both label and URL (e.g.,
WARNING_PAGE, WARNING_PAGE) by design. No changes required.src/main/java/life/mosu/mosuserver/infra/notify/component/luna/LunaNotifyWithSmsFallbackSender.java (1)
23-37: Good separation of concerns for SMS fallback functionality.The class appropriately handles notification types that require SMS fallback support.
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java (1)
25-32: Clean factory pattern implementation.Good use of constructor injection and appropriate repository dependencies.
src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/infra/notify/constant/NotifyRedirectUrlConstants.java
Show resolved
Hide resolved
| public record Exam3DayBeforeNotifyRequest( | ||
| LocalDate examDate, | ||
| String examinationNumber, | ||
| String schoolName | ||
| ) implements NotificationVariable { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding null validation for required fields.
The record fields lack validation annotations. For a notification system, null values could cause downstream issues when generating templates or sending notifications.
Consider adding validation:
+import jakarta.validation.constraints.NotNull;
+
public record Exam3DayBeforeNotifyRequest(
+ @NotNull
LocalDate examDate,
+ @NotNull
String examinationNumber,
+ @NotNull
String schoolName
) implements NotificationVariable {📝 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 Exam3DayBeforeNotifyRequest( | |
| LocalDate examDate, | |
| String examinationNumber, | |
| String schoolName | |
| ) implements NotificationVariable { | |
| import jakarta.validation.constraints.NotNull; | |
| public record Exam3DayBeforeNotifyRequest( | |
| @NotNull | |
| LocalDate examDate, | |
| @NotNull | |
| String examinationNumber, | |
| @NotNull | |
| String schoolName | |
| ) implements NotificationVariable { |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/notify/dto/Exam3DayBeforeNotifyRequest.java
around lines 12 to 16, the record fields lack null validation which may cause
issues in notification processing. Add appropriate validation annotations such
as @NotNull to each field (examDate, examinationNumber, schoolName) to ensure
they are not null when the record is instantiated. This will enforce required
field constraints and prevent null-related errors downstream.
src/main/java/life/mosu/mosuserver/application/notify/dto/Exam3DayBeforeNotifyRequest.java
Show resolved
Hide resolved
| public record RefundNotifyRequest( | ||
| String paymentKey, | ||
| LocalDate examDate, | ||
| String schoolName, | ||
| // String amount, | ||
| String paymentMethod, | ||
| String reason | ||
| ) implements NotificationVariable { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented code or clarify its purpose.
The commented out amount field suggests incomplete implementation or design uncertainty. Commented code should not remain in production code.
Either remove the commented line or clarify if the field is needed:
public record RefundNotifyRequest(
String paymentKey,
LocalDate examDate,
String schoolName,
-// String amount,
String paymentMethod,
String reason
) implements NotificationVariable {If the amount field is needed, add it properly with validation.
📝 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 RefundNotifyRequest( | |
| String paymentKey, | |
| LocalDate examDate, | |
| String schoolName, | |
| // String amount, | |
| String paymentMethod, | |
| String reason | |
| ) implements NotificationVariable { | |
| public record RefundNotifyRequest( | |
| String paymentKey, | |
| LocalDate examDate, | |
| String schoolName, | |
| String paymentMethod, | |
| String reason | |
| ) implements NotificationVariable { |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/notify/dto/RefundNotifyRequest.java
between lines 11 and 18, remove the commented out 'amount' field to clean up the
code unless it is required. If the 'amount' field is needed, uncomment it and
add appropriate validation to ensure its correctness. Otherwise, delete the
commented line entirely to avoid confusion and maintain clean production code.
| NotifyEventSuccessMessageDto eventMessage = NotifyEventSuccessMessageDto.createWithSmsFallback( | ||
| 1, | ||
| targetPhoneNumber, | ||
| alimTalkContent, | ||
| smsContent, | ||
| btnUrls | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic number to constant for consistency.
Similar to LunaNotifySender, the hardcoded value 1 should be extracted to a constant.
Consider creating a shared constant in a base class or interface to maintain consistency across all sender implementations.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/infra/notify/component/luna/LunaNotifyWithSmsFallbackSender.java
around lines 57 to 63, the hardcoded number 1 used in the createWithSmsFallback
call should be extracted to a named constant for clarity and consistency. Define
a constant with a meaningful name either in this class or in a shared base class
or interface used by all sender implementations, then replace the literal 1 with
this constant in the method call.
...n/java/life/mosu/mosuserver/infra/notify/component/luna/LunaNotifyWithSmsFallbackSender.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/MosuServerApplication.java (1)
19-21: Consider implementing the run method or documenting its purpose.The empty
runmethod implementation suggests this is either:
- Placeholder code for future notification setup logic
- Required only for the dependency injection of
NotifyEventPublisherIf this is intentional, consider adding a comment explaining why the method is empty. If notification events should be published on startup, implement the logic here.
@Override public void run(String... args) throws Exception { + // TODO: Add startup notification logic if needed + // or remove CommandLineRunner implementation if not required }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/life/mosu/mosuserver/MosuServerApplication.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/MosuServerApplication.java (1)
src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java (1)
RequiredArgsConstructor(9-19)
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/MosuServerApplication.java (2)
3-4: LGTM on the new imports.The imports correctly support the new notification infrastructure integration.
10-11: Good use of Lombok and Spring Boot patterns.The
@RequiredArgsConstructorannotation andCommandLineRunnerimplementation follow established Spring Boot conventions for dependency injection and startup hooks.
| @RequiredArgsConstructor | ||
| public class MosuServerApplication implements CommandLineRunner { | ||
|
|
||
| private final NotifyEventPublisher publisher; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the purpose of the unused NotifyEventPublisher field.
The NotifyEventPublisher is injected but not used anywhere in the class. Consider either:
- Adding documentation explaining its future use
- Implementing the notification logic in the
runmethod - Removing it if it's not needed yet
🏁 Script executed:
#!/bin/bash
# Description: Check if NotifyEventPublisher is used elsewhere in the main application class or if there are similar patterns in other CommandLineRunner implementations.
# Search for other CommandLineRunner implementations to see usage patterns
rg -A 10 "implements CommandLineRunner" --type java
# Search for NotifyEventPublisher usage patterns
rg -A 5 -B 5 "NotifyEventPublisher" --type javaLength of output: 7013
Remove or clarify the unused NotifyEventPublisher in MosuServerApplication
The NotifyEventPublisher is injected into the main application class but never used in the run method (or elsewhere in this class). Please choose one of the following:
- Remove the
publisherfield and its constructor injection if you don’t need it yet. - Document its intended startup use (e.g., publishing a “server started” event).
- Implement the notification logic in the
run(String... args)method.
Locations to address:
- src/main/java/life/mosu/mosuserver/MosuServerApplication.java, line defining
private final NotifyEventPublisher publisher;
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/MosuServerApplication.java at line 13, the
NotifyEventPublisher field is declared but never used. To fix this, either
remove the publisher field and its constructor injection if it is not needed, or
add a comment explaining its intended use at startup, or implement logic in the
run(String... args) method to publish a notification event indicating the server
has started.
1 participant
✨ 구현한 기능
NotificationVariable 인터페이스 추가 및 버튼 URL과 웹 변환 메시지 정의.
NotificationStatus 열거형 타입 추가 및 알림 상태 정의.
NotificationSendStatus 열거형 타입 추가 및 상태 정의.
NotificationEvent 클래스 추가 및 알림 이벤트 생성 기능.
NotificationButtonUrls 클래스 추가 및 버튼 URL 목록 처리 기능.
NotifyRedirectUrlConstants 클래스 추가 및 리다이렉트 URL 상수 정의.
LunaNotifyWithSmsFallbackSender 클래스 추가 및 SMS 전송 기능 구현.
LunaNotifySender 클래스 추가 및 알림 전송 메서드 구현.
NotifySender 인터페이스 추가 및 알림 전송 메서드 정의.
NotifyStrategyMapping 애너테이션 추가 및 여러 매핑 허용.
NotifyEventPublisher 수정: NotifyEvent를 NotificationEvent로 변경.
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores