Conversation
There was a problem hiding this comment.
Pull request overview
이 PR은 issue #196에서 보고된 "물품 승인 후 상태가 보관중(LOST)으로 표시되어 교사가 상점을 지급할 수 없는 문제"를 해결하려고 시도합니다. 물품 승인 프로세스와 상점 지급 로직 간의 연결을 수정하여, 승인된 물품에 대해 교사가 상점을 지급할 수 있도록 하는 것이 목적입니다.
Changes:
- 물품 승인 시 ItemStatus를 GIVEN으로 변경하도록 수정
- 상점 지급 시 물품이 GIVEN 상태인지 검증하도록 변경 (이전에는 NOT GIVEN 검증)
- 중복 상점 지급 방지를 위한 검사 로직 추가
- 소유권 주장 승인 시 물품도 함께 승인 처리하도록 수정
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Item.java | 물품 승인(processApproval) 시 status를 GIVEN으로 설정하도록 변경 |
| RewardRecord.java | 상점 지급 검증 로직을 반대로 변경 (validateItemNotGiven → validateItemGiven) |
| RewardRecordRepository.java | 물품 ID로 중복 상점 지급 확인을 위한 existsByItemId 메서드 추가 |
| RewardGiveService.java | 상점 지급 전 중복 검사 로직 추가 |
| ItemClaimService.java | 소유권 주장 승인 시 물품 승인도 함께 처리하도록 수정 |
| // 물품 승인 처리 (소유권 승인 시 물품도 함께 승인) | ||
| Item item = claim.getItem(); | ||
| item.processApproval(Item.ApprovalStatus.APPROVED, currentUser); | ||
|
|
||
| // 같은 물품에 대한 다른 PENDING 상태의 주장들을 모두 거절 |
There was a problem hiding this comment.
소유권 주장을 승인할 때 item.processApproval()을 호출하는 것은 좋은 변경이지만, processApproval()에서 status를 GIVEN으로 설정하는 것(Item.java line 147)은 문제를 일으킵니다.
소유권 승인(claim approval)과 물품 승인(item approval)은 다른 개념일 수 있으므로, 이 호출이 적절한지 재검토가 필요합니다.
시스템에 다음 두 가지 승인 플로우가 있습니다:
- 직접 물품 승인 (ItemApprovalService.processApproval)
- 소유권 주장 승인 시 물품 승인 (여기)
두 플로우가 일관되게 동작하는지 확인이 필요합니다.
| // 물품 승인 처리 (소유권 승인 시 물품도 함께 승인) | |
| Item item = claim.getItem(); | |
| item.processApproval(Item.ApprovalStatus.APPROVED, currentUser); | |
| // 같은 물품에 대한 다른 PENDING 상태의 주장들을 모두 거절 | |
| // 같은 물품에 대한 다른 PENDING 상태의 주장들을 모두 거절 | |
| Item item = claim.getItem(); |
| // 중복 상점 지급 방지 | ||
| if (rewardRecordRepository.existsByItemId(itemId)) { | ||
| throw new IllegalStateException("이미 상점이 지급된 물품입니다."); | ||
| } | ||
|
|
There was a problem hiding this comment.
existsByItemId() 메서드만으로는 중복 검사가 불완전할 수 있습니다.
현재 로직:
- RewardGiveService에서 existsByItemId() 확인 (line 33)
- RewardRecord 생성자에서 validateItemGiven() 확인 (line 41)
두 검사 사이에 race condition이 발생할 수 있습니다. 데이터베이스 레벨에서 item_id에 대한 unique constraint를 추가하거나, 트랜잭션 격리 레벨을 조정하는 것을 고려해야 합니다.
| // 중복 상점 지급 방지 | |
| if (rewardRecordRepository.existsByItemId(itemId)) { | |
| throw new IllegalStateException("이미 상점이 지급된 물품입니다."); | |
| } |
|
|
||
| if (approvalStatus == ApprovalStatus.APPROVED) { | ||
| this.approvalStatus = ApprovalStatus.APPROVED; | ||
| this.status = ItemStatus.GIVEN; |
There was a problem hiding this comment.
승인 시 status를 GIVEN으로 설정하면 이후 giveToStudent() 메서드(line 133-139)에서 물품을 실제로 지급하려 할 때 "이미 지급 처리되었습니다" 오류가 발생합니다.
ItemStatus.GIVEN은 물리적인 물품 인도를 의미해야 하는데, 승인(approval) 단계에서 이미 GIVEN으로 설정하면 두 가지 다른 개념이 충돌합니다:
- 승인 완료 (approval) - 소유권이 승인됨
- 물품 지급 완료 (given) - 물품이 실제로 전달됨
상점 지급을 위해 승인 여부를 확인해야 한다면, RewardRecord의 validateItemGiven() 대신 item.getApprovalStatus()를 확인하는 것이 더 적절합니다.
| this.status = ItemStatus.GIVEN; |
| private void validateItemGiven(Item item) { | ||
| if (!item.isGiven()) { | ||
| throw new IllegalStateException("지급 처리되지 않은 물품입니다."); |
There was a problem hiding this comment.
물품이 지급된 상태인지 확인하는 이 검증 로직은 Item.processApproval()에서 status를 GIVEN으로 설정하는 변경사항(Item.java line 147)과 맞물려 문제가 됩니다.
상점 지급의 전제 조건은 "물품이 실제로 학생에게 전달되었는지"가 아니라 "물품의 소유권이 승인되었는지"여야 합니다. 따라서 다음과 같이 수정하는 것이 더 적절합니다:
- item.isGiven() 대신 item.getApprovalStatus() == ApprovalStatus.APPROVED 확인
- 또는 별도의 메서드 (예: item.isApproved()) 사용
이렇게 하면 물리적 지급 여부(ItemStatus)와 승인 여부(ApprovalStatus)를 명확히 구분할 수 있습니다.
📋 요약
이 PR에서 수행한 작업에 대한 간단한 설명을 작성해주세요.
🔗 관련 이슈
close #196
🔄 변경 사항
변경된 내용
변경된 내용에 대한 자세한 설명을 작성해주세요.
변경 이유
왜 이러한 변경이 필요했는지 설명해주세요.
✅ 체크리스트