-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add application status update on deposit success event handling #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. Caution Review failedThe pull request is closed. WalkthroughDepositSuccessEventHandler now fetches the related Application via ApplicationJpaRepository using the virtual account log’s applicationId, sets its status to APPROVED when previously PENDING, and throws APPLICATION_LIST_NOT_FOUND if absent. Constructor gains the new repository dependency; notifications and email flow remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor VA as Virtual Account System
participant H as DepositSuccessEventHandler
participant V as VirtualAccountLogService
participant E as ExamApplicationJpaRepository
participant A as ApplicationJpaRepository
participant N as NotifyEventPublisher
participant M as MailNotifier
VA->>H: DepositSuccessEvent
H->>V: findByTxId(txId)
V-->>H: VirtualAccountLog
H->>E: findById(log.examApplicationId)
E-->>H: ExamApplication or null
alt exam application missing
H-->>VA: throws EXAM_APPLICATION_NOT_FOUND
else
H->>A: findById(log.applicationId)
A-->>H: Application or null
alt application missing
H-->>VA: throws APPLICATION_LIST_NOT_FOUND
else
H->>A: save(status=APPROVED) %% new/changed action (green)
H->>N: publish(LunaNotificationEvent)
H->>M: send(DepositSuccessMailRequest)
H-->>VA: ack
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 introduces a new feature that automates the update of an application's status following a successful deposit. By integrating application repository access into the deposit success event handler, the system can now automatically transition an application to an 'APPROVED' state, streamlining the process and ensuring data consistency. This change enhances the system's ability to react to financial events by directly impacting the lifecycle of related applications.
Highlights
- Dependency Injection: The
DepositSuccessEventHandlernow includesApplicationJpaRepositoryto interact with application data. - Application Status Update: Upon a successful deposit event, the corresponding application's status is automatically updated to
APPROVED. - Error Handling: New error handling has been added to gracefully manage scenarios where an application associated with a deposit log cannot be found.
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 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
-
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
입금 성공 이벤트 발생 시, 신청서의 상태를 APPROVED로 변경하는 기능이 추가되었네요. 이벤트 핸들러의 안정성을 높이기 위해, 상태 변경 전 현재 신청서의 상태를 확인하는 로직을 추가하는 것을 제안합니다. 자세한 내용은 코드 리뷰 코멘트를 참고해주세요.
...n/java/life/mosu/mosuserver/application/virtualaccount/event/DepositSuccessEventHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/life/mosu/mosuserver/application/virtualaccount/event/DepositSuccessEventHandler.java (4)
43-47: Make the status update idempotent to tolerate duplicate events.If a duplicate DepositSuccessEvent arrives, calling changeStatus repeatedly may be noisy or violate state-transition rules. Guard the write.
Apply this diff locally within this block:
- application.changeStatus(ApplicationStatus.APPROVED); + if (application.getStatus() != ApplicationStatus.APPROVED) { + application.changeStatus(ApplicationStatus.APPROVED); + }Optionally enforce optimistic locking on Application (e.g., @Version) to prevent double-handling races.
43-45: Error code naming likely off: APPLICATION_LIST_NOT_FOUND reads unnatural.If the intent is “application not found,” prefer APPLICATION_NOT_FOUND. The current constant name suggests a collection and may confuse callers/logs.
If the enum contains APPLICATION_NOT_FOUND, apply this change:
- .orElseThrow( - () -> new CustomRuntimeException(ErrorCode.APPLICATION_LIST_NOT_FOUND)); + .orElseThrow( + () -> new CustomRuntimeException(ErrorCode.APPLICATION_NOT_FOUND));If not present, consider adding it and deprecating APPLICATION_LIST_NOT_FOUND.
52-54: Emit external side effects after transaction commit.notifier.notify(...) and mailer.send(...) run inside the transaction. If the TX later rolls back, side effects already leaked. Prefer AFTER_COMMIT delivery (Spring @TransactionalEventListener) or an outbox.
Sketch:
- In handle(...): publish an in-process event carrying userId, examId, and mail payload.
- New listener:
@Service @RequiredArgsConstructor public class ApplicationApprovedListener { private final NotifyEventPublisher notifier; private final MailNotifier<DepositSuccessMailRequest> mailer; @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) public void on(ApplicationApprovedEvent e) { notifier.notify(LunaNotificationEvent.create( LunaNotificationStatus.APPLICATION_GUEST_SUCCESS, e.userId(), e.examId())); mailer.send(e.mailRequest()); } }This avoids duplicates on rollback and keeps the handler pure.
43-47: Add focused tests for the new behavior.
- Happy path: when deposit succeeds, Application status changes to APPROVED and notifications/mail are triggered.
- Not-found: missing Application -> throws CustomRuntimeException with the expected ErrorCode.
- Idempotency (if you add the guard): second invocation does not change state again nor throw.
I can draft unit tests using Mockito for repositories and verifications for notifier/mailer interactions. Want me to push a test skeleton?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/life/mosu/mosuserver/application/virtualaccount/event/DepositSuccessEventHandler.java(3 hunks)
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/application/virtualaccount/event/DepositSuccessEventHandler.java (2)
5-6: Confirm the business-state mapping: Deposit success -> ApplicationStatus.APPROVED.Is APPROVED the intended post-payment state (vs. PAID/COMPLETED/RECEIVED)? If approval is an admin action in your domain, this transition could skip a review step.
30-30: Verified: All dependencies and enums are present and correctly wired
- Confirmed
ApplicationJpaRepositoryinterface exists at
src/main/java/life/mosu/mosuserver/domain/application/repository/ApplicationJpaRepository.java- Confirmed
ErrorCodeenum includesAPPLICATION_LIST_NOT_FOUND- Confirmed
ApplicationStatusenum includesAPPROVEDConstructor injection via
@RequiredArgsConstructorwill pick up the repository under component scan as expected. No further changes required.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit