MOSU-153 refactor: 신청 조회시 신청 상태 구조 변경 #186
Conversation
WalkthroughThis change integrates refund information into the application data pipeline. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GetApplicationsStepProcessor
participant ExamApplicationJpaRepository
participant RefundJpaRepository
participant ApplicationContext
User->>GetApplicationsStepProcessor: process(userId)
GetApplicationsStepProcessor->>ExamApplicationJpaRepository: findByApplicationIdIn(applicationIds)
ExamApplicationJpaRepository-->>GetApplicationsStepProcessor: List<ExamApplicationWithStatus>
GetApplicationsStepProcessor->>RefundJpaRepository: findByExamApplicationIdIn(examApplicationIds)
RefundJpaRepository-->>GetApplicationsStepProcessor: List<RefundJpaEntity>
GetApplicationsStepProcessor->>ApplicationContext: assemble(applications, examApplications, refunds, ...)
ApplicationContext-->>GetApplicationsStepProcessor: List<ApplicationResponse>
GetApplicationsStepProcessor-->>User: Application responses (with refund status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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 @jbh010204, 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 refactors the way application statuses are retrieved and presented to users. It streamlines the status display to a concise 'Payment Completed' or 'Refund Completed' by shifting the complex status determination logic into the JPQL query itself. This change aims to provide a clearer and more consistent user experience while centralizing the status calculation at the data retrieval layer.
Highlights
- Simplified Application Status Display: The primary goal of this PR is to simplify the application status shown to users. Instead of various payment statuses, the system will now only return '결제 완료' (Payment Completed) or '환불 완료' (Refund Completed) for application queries.
- Status Determination Shifted to JPQL: The logic for determining the simplified application status has been moved from the application layer into the
ExamApplicationJpaRepository'sfindByApplicationIdInmethod. This is achieved via a custom JPQL query utilizing aCASEstatement to derive the status directly at the database level, improving consistency and potentially performance. - Introduction of
ExamApplicationWithStatusDTO: A new DTO,ExamApplicationWithStatus, has been introduced. This record encapsulates anExamApplicationJpaEntityalong with its pre-calculated status string, streamlining the data flow and presentation logic. - Refactoring of
ApplicationContext: TheApplicationContexthas been updated to accommodate the newExamApplicationWithStatusDTO and to integrate refund information. It now includes arefundMapand a newfetchRefundsmethod to retrieve refund entities, which are then used in thecreateExamApplicationResponsemethod to assemble the final response with the simplified status.
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 or fill out our survey 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
This pull request refactors how application statuses are retrieved using a JPQL query. To further improve the code, consider removing the redundant database call, improving the separation of concerns by moving presentation logic out of the data layer, and enhancing code clarity with better naming.
| List<ExamApplicationWithStatus> examApplications = examApplicationJpaRepository.findByApplicationIdIn( | ||
| applicationIds); |
There was a problem hiding this comment.
The query in ExamApplicationJpaRepository already determines the application status by joining with the refund table. Therefore, .fetchRefunds(...) results in an extra database query that can be avoided. Removing this call will improve efficiency.
return new ApplicationContext(applications, examApplications)
.fetchExams(examJpaRepository::findByIdIn)
.fetchSubjects(examSubjectJpaRepository::findByExamApplicationIdIn)
.fetchPayments(paymentJpaRepository::findByExamApplicationIdIn)
.assemble();| SELECT new life.mosu.mosuserver.presentation.examapplication.dto.ExamApplicationWithStatus( | ||
| ea, | ||
| CASE | ||
| WHEN r.refundStatus = 'DONE' THEN '환불완료' | ||
| ELSE '결제완료' | ||
| END | ||
| ) |
There was a problem hiding this comment.
The JPQL query returns presentation-layer strings ('환불완료', '결제완료'). Returning a boolean isRefunded and handling the mapping to display strings in the application layer would improve separation of concerns and maintainability.
@Query("""
SELECT new life.mosu.mosuserver.presentation.examapplication.dto.ExamApplicationWithStatus(
ea,
CASE
WHEN r.refundStatus = 'DONE' THEN true
ELSE false
END
)
FROM ExamApplicationJpaEntity ea
JOIN PaymentJpaEntity p ON p.examApplicationId = ea.id
LEFT JOIN RefundJpaEntity r ON r.examApplicationId = ea.id
WHERE p.paymentStatus = 'DONE'
AND (r IS NULL OR r.refundStatus = 'DONE')
AND ea.applicationId IN :applicationIds
""")
List<ExamApplicationWithStatus> findByApplicationIdIn(List<Long> applicationIds);| FROM RefundJpaEntity r | ||
| WHERE r.examApplicationId IN :examApplicationIds | ||
| """) | ||
| List<RefundJpaEntity> findByExamApplicationIdIn(@Param("examApplicationIds") List<Long> examApplicationId); |
There was a problem hiding this comment.
The parameter name examApplicationId is misleading for a List<Long>. Renaming it to examApplicationIds would improve readability.
| List<RefundJpaEntity> findByExamApplicationIdIn(@Param("examApplicationIds") List<Long> examApplicationId); | |
| List<RefundJpaEntity> findByExamApplicationIdIn(@Param("examApplicationIds") List<Long> examApplicationIds); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/presentation/examapplication/dto/ExamApplicationWithStatus.java (1)
1-8: Consider using an enum for the status field.The record implementation is clean and functional. However, consider using an enum instead of
Stringfor thestatusfield to provide better type safety and prevent invalid status values.+public enum ApplicationStatus { + PAYMENT_COMPLETED("결제완료"), + REFUND_COMPLETED("환불완료"); + + private final String displayName; + + ApplicationStatus(String displayName) { + this.displayName = displayName; + } + + public String getDisplayName() { + return displayName; + } +} public record ExamApplicationWithStatus( ExamApplicationJpaEntity examApplication, - String status + ApplicationStatus status ) { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/life/mosu/mosuserver/application/application/ApplicationContext.java(1 hunks)src/main/java/life/mosu/mosuserver/application/application/processor/GetApplicationsStepProcessor.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationResponse.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/examapplication/dto/ExamApplicationWithStatus.java(1 hunks)
🔇 Additional comments (10)
src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java (2)
3-7: LGTM! Import corrections look good.The import changes properly replace the incorrect
io.lettuce.core.dynamic.annotation.Paramwith the standard Springorg.springframework.data.repository.query.Paramand add the necessaryjava.util.Listimport.
37-42: LGTM! Clean batch retrieval method.The new
findByExamApplicationIdInmethod provides efficient batch retrieval of refunds by exam application IDs. The JPQL query is straightforward and the method signature follows Spring Data JPA conventions.src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationResponse.java (1)
9-9: LGTM! Field rename aligns with refactoring objectives.The rename from
paymentStatustostatusproperly reflects the expanded scope to include both payment and refund status information.Also applies to: 20-20, 30-30
src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java (1)
75-90: Business logic filter consistency verifiedVerified that all existing JPQL queries in ExamApplicationJpaRepository use
p.paymentStatus = 'DONE'. The new findByApplicationIdIn method follows this same pattern and correctly incorporates refund status handling. No further changes required.src/main/java/life/mosu/mosuserver/application/application/processor/GetApplicationsStepProcessor.java (3)
12-12: LGTM! Proper dependency injection for refund repository.The addition of
RefundJpaRepositorydependency follows the existing pattern and enables refund data integration.Also applies to: 31-31
15-15: LGTM! Type update aligns with repository changes.The variable type change from
List<ExamApplicationJpaEntity>toList<ExamApplicationWithStatus>properly reflects the updated repository method return type.Also applies to: 43-44
46-51: LGTM! Clean integration of refund data in processing chain.The addition of
.fetchRefunds(refundJpaRepository::findByExamApplicationIdIn)follows the established fluent processing pattern and integrates refund data into the application context.src/main/java/life/mosu/mosuserver/application/application/ApplicationContext.java (3)
21-28: LGTM! Record structure properly updated to include refund data.The addition of
refundMapand the change toExamApplicationWithStatusaligns well with the PR objective to integrate refund status information. The consistent use of Map structures for related entities maintains good design patterns.
30-35: LGTM! Constructor properly updated.The constructor correctly initializes all fields including the new
refundMap, maintaining consistency with the record structure changes.
78-89: LGTM! Assembly logic properly handles the new data structure.The method correctly processes
ExamApplicationWithStatusobjects and maintains proper null-safety with filtering.
| public ApplicationContext fetchRefunds(Function<List<Long>, List<RefundJpaEntity>> fetcher) { | ||
| Map<Long, RefundJpaEntity> newRefundMap = fetcher.apply( | ||
| examApplications.stream() | ||
| .map(e -> e.examApplication().getId()) | ||
| .toList() | ||
| ).stream().collect(Collectors.toMap(RefundJpaEntity::getExamApplicationId, Function.identity())); | ||
|
|
||
| return new ApplicationContext(applications, examApplications, examMap, subjectMap, paymentMap, newRefundMap); | ||
| } |
There was a problem hiding this comment.
Potential data loss if multiple refunds exist for the same exam application.
The toMap collector will throw an exception if there are duplicate keys (multiple refunds for the same examApplicationId). Consider using a merge function or grouping if multiple refunds per application are possible.
If multiple refunds are not possible by design, this is fine. Otherwise, consider:
- ).stream().collect(Collectors.toMap(RefundJpaEntity::getExamApplicationId, Function.identity()));
+ ).stream().collect(Collectors.toMap(
+ RefundJpaEntity::getExamApplicationId,
+ Function.identity(),
+ (existing, replacement) -> replacement // or choose appropriate merge strategy
+ ));📝 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 ApplicationContext fetchRefunds(Function<List<Long>, List<RefundJpaEntity>> fetcher) { | |
| Map<Long, RefundJpaEntity> newRefundMap = fetcher.apply( | |
| examApplications.stream() | |
| .map(e -> e.examApplication().getId()) | |
| .toList() | |
| ).stream().collect(Collectors.toMap(RefundJpaEntity::getExamApplicationId, Function.identity())); | |
| return new ApplicationContext(applications, examApplications, examMap, subjectMap, paymentMap, newRefundMap); | |
| } | |
| public ApplicationContext fetchRefunds(Function<List<Long>, List<RefundJpaEntity>> fetcher) { | |
| Map<Long, RefundJpaEntity> newRefundMap = fetcher.apply( | |
| examApplications.stream() | |
| .map(e -> e.examApplication().getId()) | |
| .toList() | |
| ).stream().collect(Collectors.toMap( | |
| RefundJpaEntity::getExamApplicationId, | |
| Function.identity(), | |
| (existing, replacement) -> replacement // or choose appropriate merge strategy | |
| )); | |
| return new ApplicationContext(applications, examApplications, examMap, subjectMap, paymentMap, newRefundMap); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/application/ApplicationContext.java
around lines 68 to 76, the use of Collectors.toMap without a merge function can
cause exceptions if multiple RefundJpaEntity objects share the same
examApplicationId key. To fix this, either add a merge function to handle
duplicates or change the collection to group refunds by examApplicationId into a
list or another suitable structure, depending on whether multiple refunds per
application are allowed.
| private Map.Entry<Long, ExamApplicationResponse> createExamApplicationResponse(ExamApplicationWithStatus item) { | ||
| ExamApplicationJpaEntity examApp = item.examApplication(); | ||
| ExamJpaEntity exam = examMap.get(examApp.getExamId()); | ||
| if (exam == null) return null; | ||
|
|
||
| List<ExamSubjectJpaEntity> subjectEntities = this.subjectMap.getOrDefault(examApp.getId(), | ||
| List.of()); | ||
| Set<String> subjects = subjectEntities.stream().map(es -> es.getSubject().getSubjectName()) | ||
| .collect(Collectors.toSet()); | ||
| Set<String> subjects = subjectMap.getOrDefault(examApp.getId(), List.of()).stream() | ||
| .map(s -> s.getSubject().getSubjectName()).collect(Collectors.toSet()); | ||
|
|
||
| PaymentJpaEntity payment = this.paymentMap.getOrDefault(examApp.getId(), null); | ||
| String paymentStatus = Optional.ofNullable(payment) | ||
| .map(p -> p.getPaymentStatus().name()) | ||
| .orElse(null); | ||
| PaymentJpaEntity payment = paymentMap.get(examApp.getId()); | ||
| RefundJpaEntity refund = refundMap.get(examApp.getId()); | ||
|
|
||
| Integer totalAmount = Optional.ofNullable(payment) | ||
| .map(p -> p.getPaymentAmount().getTotalAmount()) | ||
| .orElse(0); | ||
|
|
||
| String lunchName; | ||
| if (examApp.getIsLunchChecked()) { | ||
| lunchName = exam.getLunchName(); | ||
| } else { | ||
| lunchName = null; | ||
| } | ||
| String lunchName = examApp.getIsLunchChecked() ? exam.getLunchName() : null; | ||
| String status = item.status(); | ||
|
|
||
| ExamApplicationResponse response = ExamApplicationResponse.of( | ||
| examApp.getId(), | ||
| examApp.getCreatedAt(), | ||
| paymentStatus, totalAmount, | ||
| status, | ||
| totalAmount, | ||
| exam.getSchoolName(), | ||
| exam.getExamDate(), | ||
| subjects, | ||
| lunchName | ||
| ); | ||
|
|
||
| return Map.entry(examApp.getApplicationId(), response); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider utilizing the refund entity in the response.
The refund entity is fetched but not used in the response creation. Since the PR aims to integrate refund information, consider:
- Should the
totalAmountreflect refunds (e.g., show negative amount for refunded applications)? - Should refund details be included in the
ExamApplicationResponse?
Let me check if ExamApplicationResponse has fields for refund information:
🏁 Script executed:
#!/bin/bash
# Check the structure of ExamApplicationResponse
fd -e java -x grep -l "ExamApplicationResponse" {} \; | xargs grep -A 10 "class ExamApplicationResponse\|record ExamApplicationResponse"Length of output: 1448
Include refund information in the response creation
You’re currently fetching RefundJpaEntity refund but never using it. To fully support refunds, please decide on one of the following:
-
Adjust the reported amount by subtracting the refund:
int paid = Optional.ofNullable(payment) .map(p -> p.getPaymentAmount().getTotalAmount()) .orElse(0); int refunded = Optional.ofNullable(refund) .map(r -> r.getRefundAmount().getTotalAmount()) .orElse(0); int netAmount = paid - refunded;
and pass
netAmount(or bothpaidandrefunded) into the response. -
Or extend
ExamApplicationResponseto include refund fields:public record ExamApplicationResponse( Long examApplicationId, String createdAt, String status, Integer totalAmount, Integer refundedAmount, // new Integer netAmount, // new (optional) String schoolName, LocalDate examDate, Set<String> subjects, String lunchName ) { … }
Locations to update:
src/main/java/life/mosu/mosuserver/application/application/ApplicationContext.java(lines 91–120)src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationResponse.java
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/application/ApplicationContext.java
lines 91 to 120, you are fetching the refund entity but not using it in the
response. To fix this, calculate the net amount by subtracting the refund amount
from the payment amount, then update the ExamApplicationResponse creation to
include this net amount or both paid and refunded amounts. Additionally, modify
the ExamApplicationResponse record in
src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationResponse.java
to add fields for refundedAmount and netAmount, and update its constructor and
usage accordingly.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Other