MOSU-179 feat: 신청 마감 validator 구현 #182
Conversation
WalkthroughThe changes introduce an additional validation step to the application process, ensuring exams are not full before checking if their dates have passed. Supporting logic and error codes are added for this new validation, and exception handling is improved in discount calculations. Minor corrections and dependency updates are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplicationService
participant ApplicationValidator
participant ExamQuotaCacheManager
User->>ApplicationService: apply(examIds)
ApplicationService->>ApplicationValidator: examNotFull(examIds)
ApplicationValidator->>ExamQuotaCacheManager: getCurrentApplications(examIds)
ApplicationValidator-->>ApplicationService: (throws if full)
ApplicationService->>ApplicationValidator: examDateNotPassed(examIds)
ApplicationValidator-->>ApplicationService: (throws if date passed)
ApplicationService->>...: (proceed with further validations and processing)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
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 @chominju02, 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 critical validation step to ensure that users cannot apply for exams once their capacity has been reached, enhancing the robustness of the application process. It also includes general improvements to error handling, minor code cleanups, and a correction to a refund query, contributing to overall system stability and maintainability.
Highlights
- New Validation for Exam Capacity: I've implemented a new validator,
examNotFull, withinApplicationValidatorto prevent users from applying to exams that have already reached their maximum capacity. This check is now integrated into the application flow inApplicationService. - Improved Error Handling: I've introduced two new error codes:
APPLICATION_CLOSEDfor when an exam's application quota is met, andPRICE_LOAD_FAILUREfor issues during price calculation. Error handling for retrieving discount amounts has also been enhanced inExamApplicationService. - Code Refinements and Cleanup: I've made several minor improvements, including renaming
ExamDateNotPassedtoexamDateNotPassedfor consistent casing, updating an error code fromEXAM_APPLICATION_NOT_FOUNDtoEXAM_NOT_FOUND, and removing an unusedPaymentJpaRepositorydependency. - Refund Query Correction: The refund query in
RefundJpaRepositoryhas been updated to correctly checkr.refundStatus = 'DONE'instead ofp.paymentStatus = 'CANCELLED_DONE', ensuring accurate refund status retrieval.
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 introduces a validator to check if an exam application is closed because it's full, using a cache for performance. It also includes some related refactoring and bug fixes. The overall implementation is good, and I have provided suggestions to improve error handling and code conciseness.
| try { | ||
| return calculator.getAppliedDiscountAmount(totalAmount); | ||
| } catch (Exception ex) { | ||
| throw new CustomRuntimeException(ErrorCode.PRICE_LOAD_FAILURE); | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception is too broad and can mask unexpected runtime exceptions. It's better to catch the specific exception(s) you expect, such as IllegalArgumentException which is thrown by FixedQuantityDiscountCalculator. This makes error handling more robust and predictable.
} catch (IllegalArgumentException ex) {| JOIN ExamJpaEntity e ON ea.examId = e.id | ||
| WHERE r.examApplicationId = :examApplicationId | ||
| AND p.paymentStatus = 'CANCELLED_DONE' | ||
| AND r.refundStatus = 'DONE' |
There was a problem hiding this comment.
The query condition p.paymentStatus = 'CANCELLED_DONE' seems to be replaced with r.refundStatus = 'DONE'. Ensure that this change accurately reflects the intended logic for filtering refunds. It's crucial that the correct status is used to identify refunds for the given exam application.
| .anyMatch(examId -> { | ||
| Optional<Long> currentApplications = examQuotaCacheManager.getCurrentApplications( | ||
| examId); | ||
| Optional<Long> maxCapacity = examQuotaCacheManager.getMaxCapacity(examId); | ||
|
|
||
| if (currentApplications.isPresent() && maxCapacity.isPresent()) { | ||
| return currentApplications.get() >= maxCapacity.get(); | ||
| } | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
The lambda expression within anyMatch can be simplified to be more concise and idiomatic by using Optional.flatMap and Optional.map. This avoids explicit isPresent() and .get() calls, which improves readability.
boolean isFull = examIds.stream()
.anyMatch(examId -> examQuotaCacheManager.getMaxCapacity(examId)
.flatMap(maxCapacity -> examQuotaCacheManager.getCurrentApplications(examId)
.map(currentApplications -> currentApplications >= maxCapacity))
.orElse(false));| Set<Subject> subjects = request.getSubjects(); | ||
|
|
||
| validator.ExamDateNotPassed(examIds); | ||
| validator.examNotFull(examIds); |
There was a problem hiding this comment.
It's good to see the addition of validator.examNotFull(examIds) to prevent applications for full exams. This enhances the application process by preventing users from applying to exams that have reached their capacity.
| } | ||
|
|
||
| public void ExamDateNotPassed(List<Long> examIds) { | ||
| public void examDateNotPassed(List<Long> examIds) { |
There was a problem hiding this comment.
Renaming ExamDateNotPassed to examDateNotPassed improves code readability by adhering to Java naming conventions for methods (camelCase).
| List<ExamApplicationJpaEntity> examApplicationEntities = examApplicationJpaRepository.findByApplicationId( | ||
| applicationId); | ||
| Integer lunchCount = (int) examApplicationEntities.stream() | ||
| int lunchCount = (int) examApplicationEntities.stream() |
There was a problem hiding this comment.
Changing the type of lunchCount from Integer to int is a good practice as it avoids potential NullPointerException and is more memory-efficient for primitive types.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
155-156: Remove unnecessary empty lines.Extra empty lines at the end of the file should be removed for consistency.
- LUA_SCRIPT_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, "LUA 스크립트 실행 중 오류가 발생했습니다."); - - + LUA_SCRIPT_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, "LUA 스크립트 실행 중 오류가 발생했습니다.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java(5 hunks)src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)
🔇 Additional comments (9)
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
68-68: LGTM!The APPLICATION_CLOSED error code with HTTP 400 status is semantically correct for representing application deadline closure validation.
src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java (1)
29-29: LGTM! Improved query accuracy.Filtering by
refundStatus = 'DONE'instead ofpaymentStatus = 'CANCELLED_DONE'is more semantically correct for a refund query. This change ensures the method returns refunds based on their actual status rather than the related payment status.src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (1)
44-45: LGTM! Well-structured validation sequence.The separation of capacity and date validation is logical and aligns with the PR objective. Checking if exams are full before validating dates provides better user experience by prioritizing capacity constraints.
src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java (2)
123-123: LGTM! Minor optimization.Using primitive
intinstead ofIntegerforlunchCountis a good optimization since the value cannot be null in this context.
174-179: LGTM! Improved error handling.The try-catch block properly handles exceptions during discount calculation and provides consistent error reporting using the new PRICE_LOAD_FAILURE error code.
src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java (4)
9-9: LGTM! Proper dependency injection.Adding ExamQuotaCacheManager dependency to support the new capacity validation feature is well-structured.
Also applies to: 25-25
36-36: LGTM! Corrected error code.Using EXAM_NOT_FOUND instead of EXAM_APPLICATION_NOT_FOUND is more semantically accurate for this validation context.
74-74: LGTM! Improved naming convention.Renaming from
ExamDateNotPassedtoexamDateNotPassedfollows proper Java naming conventions.
84-100: LGTM! Well-implemented capacity validation.The
examNotFullmethod properly implements the core feature of checking exam capacity. The logic correctly compares current applications against maximum capacity and handles cache misses gracefully by defaulting to "not full" (safe failure mode).The implementation handles edge cases well:
- Proper use of Optional to handle cache misses
- Safe default behavior when cache data is unavailable
- Clear logic flow with appropriate error code usage
| APPLICATION_SCHOOL_DUPLICATED(HttpStatus.BAD_REQUEST, "동일 일자의 같은 학교를 신청할 수 없습니다."), | ||
| EXAM_DUPLICATED(HttpStatus.BAD_REQUEST, "동일한 시험을 신청할 수 없습니다."), | ||
| WRONG_APPLICATION_ID_TYPE(HttpStatus.BAD_REQUEST, "신청 ID 타입이 올바르지 않습니다."), | ||
| PRICE_LOAD_FAILURE(HttpStatus.CONFLICT, "신청 가격 정보를 가져오는데 실패했습니다."), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider HTTP status code for PRICE_LOAD_FAILURE.
Using HTTP 409 (Conflict) for price loading failure seems semantically incorrect. Consider using HTTP 500 (Internal Server Error) or HTTP 503 (Service Unavailable) instead, as this represents a system/service failure rather than a resource conflict.
- PRICE_LOAD_FAILURE(HttpStatus.CONFLICT, "신청 가격 정보를 가져오는데 실패했습니다."),
+ PRICE_LOAD_FAILURE(HttpStatus.INTERNAL_SERVER_ERROR, "신청 가격 정보를 가져오는데 실패했습니다."),🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java at line
67, the HTTP status code for PRICE_LOAD_FAILURE is currently set to 409
(Conflict), which is not appropriate for a system failure. Change the status
code to 500 (Internal Server Error) or 503 (Service Unavailable) to better
represent a service or system failure when price loading fails.
2 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Other Improvements