[FIX] 등록자 마이페이지 reject 수정 #137
Conversation
📝 WalkthroughWalkthroughThis PR refactors amateur show approval status tracking by renaming the admin approval response field from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![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
🤖 Fix all issues with AI agents
In @src/main/java/cc/backend/amateurShow/entity/AmateurShow.java:
- Around line 160-169: The approve() and reject(String) methods unconditionally
overwrite the lifecycle field status (AmateurShowStatus) which can corrupt
state; update approve() and reject() to only change approvalStatus while either
(a) not modifying status at all, or (b) guarding status mutations so they only
occur from an allowed prior state (e.g., only change status when current status
== AmateurShowStatus.YET), and reject attempts from invalid states by throwing
an IllegalStateException or returning a boolean result; ensure these guards live
in the approve() and reject() methods and document the intended relationship
between status and approvalStatus in the method Javadoc.
🧹 Nitpick comments (1)
src/main/java/cc/backend/amateurShow/entity/AmateurShowStatus.java (1)
6-7: Consider semantic clarity: mixing lifecycle states with approval states.The
REJECTconstant is being added to an enum that represents temporal lifecycle states (YET, ONGOING, ENDED). However,REJECTrepresents an approval outcome, not a temporal state relative to show dates. This mixing of concerns could lead to confusion:
- Temporal states (YET, ONGOING, ENDED) are computed based on current date vs. show dates
REJECTis a terminal approval state unrelated to show schedulingConsider whether
REJECTshows should have a separate approval status field (which appears to exist asApprovalStatus) rather than overloading the lifecyclestatusfield. If both fields are needed, clarify their distinct purposes in comments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/cc/backend/admin/amateurShow/dto/AdminApprovalListResponseDTO.javasrc/main/java/cc/backend/admin/amateurShow/service/AdminApprovalService.javasrc/main/java/cc/backend/amateurShow/dto/AmateurShowResponseDTO.javasrc/main/java/cc/backend/amateurShow/entity/AmateurShow.javasrc/main/java/cc/backend/amateurShow/entity/AmateurShowStatus.javasrc/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.javasrc/main/java/cc/backend/ticket/controller/MemberTicketController.java
🔇 Additional comments (5)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
604-604: LGTM! Rejection reason properly exposed to performers.The addition of
rejectReasonto theMyShowAmateurShowListbuilder correctly exposes rejection feedback to performers viewing their show submissions. This aligns with the PR objective to display reject status on the performer's mypage.src/main/java/cc/backend/ticket/controller/MemberTicketController.java (1)
31-105: No action needed—no hardcoded endpoint references or API documentation exist in the codebase.Verification found no hardcoded URL references to these ticket endpoints in Java, TypeScript, or JavaScript files, and no API documentation files that reference these paths. The paths already include the leading slash pattern (e.g.,
@GetMapping("/{amateurShowId}/showSimple")). When combined with the class-level@RequestMapping("/tickets"), Spring Boot resolves them correctly to/tickets/{amateurShowId}/showSimple. No API clients or documentation need updating.Likely an incorrect or invalid review comment.
src/main/java/cc/backend/admin/amateurShow/dto/AdminApprovalListResponseDTO.java (1)
19-19: This review comment is incorrect. The fieldapprovalStatusinAdminApprovalListResponseDTOis not a rename fromamateurShowStatus. These are two separate DTOs:
- AdminApprovalListResponseDTO (the file under review) correctly uses
approvalStatuswith the implementation callingshow.getApprovalStatus().name()- AdminAmateurShowListResponseDTO (a different DTO in a different flow) uses
amateurShowStatusThere is no breaking change in the code under review. The field naming is consistent throughout the approval workflow (service and controller both use
approvalStatuscorrectly).Likely an incorrect or invalid review comment.
src/main/java/cc/backend/amateurShow/dto/AmateurShowResponseDTO.java (1)
109-122: LGTM! Rejection reason properly integrated into performer's show list.The
rejectReasonfield is correctly added toMyShowAmateurShowListand is being populated from the entity viaAmateurServiceImpl.getMyAmateurShow()(line 604). The entity has proper getter/setter methods and rejection logic, with thereject()method appropriately setting both status and reason. Nullable String type and placement afterstatusare appropriate.src/main/java/cc/backend/admin/amateurShow/service/AdminApprovalService.java (1)
70-82: Semantic improvement: approval status separated from show status.The refactor from
getStatus()togetApprovalStatus()correctly distinguishes approval workflow state (WAITING/APPROVED/REJECTED) from show lifecycle state (YET/ONGOING/COMPLETED). The fieldapprovalStatusin AdminApprovalListResponseDTO is properly defined and the method call is valid through Lombok's generated getter.
| public void approve(){ | ||
| this.approvalStatus = ApprovalStatus.APPROVED; | ||
| this.status = AmateurShowStatus.YET; | ||
| } | ||
| public void reject(String rejectReason){ | ||
| this.approvalStatus = ApprovalStatus.REJECTED; | ||
| this.status = AmateurShowStatus.REJECT; | ||
| this.rejectReason = rejectReason; | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical: Unconditional status transitions may corrupt lifecycle state.
The approve() and reject() methods unconditionally set the status field without considering its current value:
approve()always setsstatus = YET, even if the show is alreadyONGOINGorENDEDreject()always setsstatus = REJECT, regardless of current state
Potential issues:
- If an admin approves a show that has already started (ONGOING), it gets reset to YET
- If an admin processes approval/rejection for a show after it ended, the temporal state is lost
- Status transitions don't follow a proper state machine pattern
Recommendations:
- Consider whether
statusshould be updated at all during approval/rejection, sinceapprovalStatusalready tracks approval state - If
statusmust change, add guards to prevent invalid transitions - Document the intended relationship between
statusandapprovalStatus - Consider whether approval/rejection should only be allowed when status is YET
🔒 Suggested defensive logic
public void approve(){
this.approvalStatus = ApprovalStatus.APPROVED;
- this.status = AmateurShowStatus.YET;
+ // Only set status to YET if not already in a more advanced state
+ if (this.status != AmateurShowStatus.ONGOING && this.status != AmateurShowStatus.ENDED) {
+ this.status = AmateurShowStatus.YET;
+ }
}
public void reject(String rejectReason){
this.approvalStatus = ApprovalStatus.REJECTED;
- this.status = AmateurShowStatus.REJECT;
+ // Only set status to REJECT if not already ended
+ if (this.status != AmateurShowStatus.ENDED) {
+ this.status = AmateurShowStatus.REJECT;
+ }
this.rejectReason = rejectReason;
-
}🤖 Prompt for AI Agents
In @src/main/java/cc/backend/amateurShow/entity/AmateurShow.java around lines
160 - 169, The approve() and reject(String) methods unconditionally overwrite
the lifecycle field status (AmateurShowStatus) which can corrupt state; update
approve() and reject() to only change approvalStatus while either (a) not
modifying status at all, or (b) guarding status mutations so they only occur
from an allowed prior state (e.g., only change status when current status ==
AmateurShowStatus.YET), and reject attempts from invalid states by throwing an
IllegalStateException or returning a boolean result; ensure these guards live in
the approve() and reject() methods and document the intended relationship
between status and approvalStatus in the method Javadoc.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.