Repository navigation
[Feat/#146] 사물함 상태 일괄 변경 관리자 API 추가 - #148
sangrae2325 wants to merge 4 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
| public List<Locker> changeLockerStatus(Long sectionId, List<Long> lockerIds, LockerStatus status) { | ||
| List<Locker> lockers = findLockersInSection(sectionId, lockerIds); | ||
| if (status == LockerStatus.DISABLED) { | ||
| requireNotInUse(lockerIds); |
There was a problem hiding this comment.
[r] 리뷰 포인트 주신 "이용 중인 칸은 DISABLED 불가" 기준에 대한 의견이에요. 날짜 기준 자체(사용 종료일 당일까지 사용 중, LockerApplicationStatus 재사용)는 좋은데, 이 규칙이 #141의 설계와 부딪히는 것 같아요.
- #141은 "배정된 뒤 고장 나서 사용 중지된 칸"을 정상 상태로 보고
assignedCount주석,LockerDisplayStatus의ASSIGNED우선 규칙, Swagger 설명을 만들어 뒀는데, - 이 PR 이후로는 진행 중인 회차에서 그 상태를 API로 만들 수 없어요.
- 신청 취소·변경 기능도 없다 보니, 배정된 칸이 실제로 고장 나면 결국 DB를 직접 고쳐야 하는 상황이 남고요.
두 방향 중 하나로 맞추면 좋을 것 같아요.
- (A) 이용 중인 칸도
DISABLED를 허용하고LOCKER_IN_USE를 없앤다 → [Feat/#132] 사물함 구역 목록·상세 관리자 API 추가 #141 표시 로직이 그대로 쓰이고, 아래 동시성 문제도 대부분 사라져요. - (B) 지금 규칙을 유지하고 #141의 주석·Swagger·
ASSIGNED우선 근거를 정리한다.
운영 기준은 팀에서 한번 같이 정하면 좋겠습니다!
| * 배정인지 만료인지는 내 신청 내역 화면과 같은 기준을 써야 하므로 {@link LockerApplicationStatus}에 맡긴다. | ||
| * 저장소는 신청과 회차를 읽어 오기만 한다. | ||
| */ | ||
| private void requireNotInUse(List<Long> lockerIds) { |
There was a problem hiding this comment.
[c] 학생 신청과 이 변경이 동시에 실행되면 "사용 중지인데 배정된" 칸이 생길 수 있을 것 같아요. 예를 들어 여기서 신청을 조회(0건)한 직후 학생이 같은 칸을 AVAILABLE로 읽고 신청을 커밋하면, 두 트랜잭션이 서로 다른 테이블에 써서 충돌 없이 둘 다 성공해요. 유니크 제약은 중복 배정만 막아서 이 경우는 못 잡고요.
막으려면 사물함 행을 비관적 락으로 같이 잡아야 하는데 저장소에 선례가 없는 패턴이라, 위 [r]에서 (A)로 가면 자연히 해결되고, (B)로 가면 최소한 API 설명과 PR 한계에 적어두면 좋을 것 같아요.
| } | ||
|
|
||
| /** 도메인 {@link Locker}와 이름이 겹치지 않도록 {@code LockerItem}으로 둔다. */ | ||
| public record LockerItem(Long lockerId, int lockerNumber, LockerStatus lockerStatus) { |
There was a problem hiding this comment.
[c] 응답 관련 리뷰 포인트([r] 두 번째, [a])에 대한 의견이에요. 구역 상세 화면은 LockerDisplayStatus(AVAILABLE/DISABLED/ASSIGNED, 회차 기준)를 쓰는데 이 응답은 물리 상태인 LockerStatus를 줘서, 응답으로 칸을 다시 그리면 어긋날 수 있을 것 같아요. 예를 들어 배정된 칸(물리 상태 AVAILABLE)을 포함해 AVAILABLE로 일괄 변경하면 응답에 AVAILABLE이 와서, 화면이 그 칸을 "선택 가능"으로 그리게 돼요. 필드 이름도 상세는 status, 여기는 lockerStatus라 다르고요.
회차 관리 쓰기 API처럼 ApiResponse<Void>로 돌려주고 화면이 상세를 다시 조회하게 하면, "바뀐 것만 담을지"와 "lockerNumber를 넣을지" 고민도 같이 없어질 것 같아요.
| @Transactional | ||
| public List<Locker> changeLockerStatus(Long sectionId, List<Long> lockerIds, LockerStatus status) { | ||
| List<Locker> lockers = findLockersInSection(sectionId, lockerIds); | ||
| if (status == LockerStatus.DISABLED) { |
There was a problem hiding this comment.
[c] DISABLED일 때만 검사하는 방식이라, 나중에 REPAIRING 같은 상태가 생기면 학생은 고를 수 없는데 이용 중 검사는 그냥 통과하게 될 것 같아요. Locker.isSelectable은 status == AVAILABLE(허용 목록)로 보고 있으니, "사용할 수 있는 상태인지"를 enum에 두고 양쪽이 같이 쓰면 상태가 늘어도 안전할 것 같아요.
public enum LockerStatus {
AVAILABLE,
DISABLED;
public boolean isUsable() {
return this == AVAILABLE;
}
}
// Locker.isSelectable: status.isUsable() && !applied
// 여기: if (!status.isUsable()) { validateNotInUse(lockerIds); }| * <p> | ||
| * 운영진이 여러 칸을 한 번에 바꾸므로, 이미 그 상태인 사물함은 저장하지 않도록 바뀌었는지 알려준다. | ||
| */ | ||
| public boolean changeStatus(LockerStatus status) { |
There was a problem hiding this comment.
[a] 바뀌었는지를 boolean으로 돌려주고 서비스에서 filter 안에서 상태를 바꾸는 구조라, 스트림의 non-interference 원칙에 어긋나고 나중에 anyMatch처럼 중간에 멈추는 연산으로 바뀌면 변경이 일부 빠질 수 있을 것 같아요. 사물함들이 같은 트랜잭션에서 findLockersBySectionId로 이미 영속성 컨텍스트에 올라와 있어서, 전부 넘겨도 실제로 값이 바뀐 행만 UPDATE될 거예요. 그래서 boolean 없이 단순하게 가도 될 것 같습니다.
public void changeStatus(LockerStatus status) {
this.status = status;
}lockers.forEach(locker -> locker.changeStatus(status));
lockerRepository.saveLockers(lockers);| .map(lockerId -> { | ||
| Locker locker = lockersInSection.get(lockerId); | ||
| if (locker == null) { | ||
| throw new BusinessException(LockerErrorCode.LOCKER_NOT_IN_SECTION); |
There was a problem hiding this comment.
[c] 리뷰 포인트 주신 "없는 사물함과 다른 구역 사물함을 하나로 묶은 것"은 운영 화면에서는 하나로 둬도 괜찮아 보여요. 다만 가르려면 조회가 하나 더 든다고 적어주셨는데, 구역 전체 대신 요청한 id로 조회하면(findAllByIdInAndIsDeletedFalse 같은) 쿼리 하나로 "결과에 없음 = 없는 사물함 / sectionId가 다름 = 다른 구역"을 가를 수 있고 읽는 행도 줄어들 것 같아요.
그리고 없는 sectionId가 들어오면 같은 경로의 GET은 404(LOCKER_SECTION_NOT_FOUND)인데 여기는 400(LOCKER_NOT_IN_SECTION)이라, 이 부분은 맞춰두면 좋겠습니다.
| /** | ||
| * 요청한 사물함 전체를 바뀐 뒤 상태로 돌려준다. 이미 그 상태였던 사물함도 담긴다. | ||
| */ | ||
| public record AdminLockerStatusChangeResponse(List<LockerItem> lockers) { |
There was a problem hiding this comment.
[c] 리뷰 포인트 주신 응답 이름은 coding-style.md 1절 표({Domain}{Action}Response)와 공지·회비 관리자 API(NoticeCreateResponse, FeeStatusUpdateResponse)가 접두사 없이 가고 있어서, 요청에 맞춰 LockerStatusChangeResponse로 짝을 맞추는 쪽이 어떨까요? 위 코멘트대로 Void로 바꾸시면 자연히 없어지는 고민이기도 해요.
| void deletePeriod(Long lockerPeriodId); | ||
|
|
||
| /** | ||
| * 구역 안의 사물함 여러 개의 상태를 한 번에 바꾸고, 바뀐 사물함을 돌려준다. |
There was a problem hiding this comment.
[a] 본문은 "바뀐 사물함을 돌려준다"인데 @return은 "요청한 사물함 전체. 이미 그 상태였던 사물함도 포함한다"라서 한쪽으로 맞춰주시면 좋겠어요.
| */ | ||
| public record LockerStatusChangeRequest( | ||
| @NotEmpty(message = "사물함을 하나 이상 선택해 주세요.") | ||
| List<@NotNull(message = "사물함 ID는 비워 둘 수 없습니다.") Long> lockerIds, |
There was a problem hiding this comment.
[a] 같은 id가 여러 번 오면 응답에도 같은 칸이 여러 번 담겨서, lockerIds.stream().distinct()로 한 번 걸러주면 좋을 것 같아요. 한 구역의 칸 수를 넘는 요청은 의미가 없으니 @Size(max = ...) 상한도 하나 있으면 안전할 것 같고요.
|
|
||
| /** 구역 안의 사물함 여러 개를 같은 상태로 바꾼다. */ | ||
| @Operation(summary = "사물함 상태 일괄 변경", | ||
| description = "구역 안의 사물함 여러 개를 AVAILABLE(선택가능) 또는 DISABLED(선택불가)로 한 번에 바꾼다. " |
There was a problem hiding this comment.
[a] 질문 하나 드려요. 학생 쪽 구역 상태(SectionAvailabilityStatus.from)는 사용 중지 칸도 분모에 넣고 있어서, 이 API로 DISABLED가 흔해지면 예를 들어 36칸 중 30칸 사용 중지·6칸 빈 구역이 6/36(약 17%)으로 계산되어 학생에게 "거의 마감"으로 보일 것 같아요. 의도하신 동작인지 궁금합니다!
#️⃣연관된 이슈
🎯 해결하려는 문제가 무엇인가요?
운영진 콘솔에서 구역 안의 사물함 여러 개를 골라 물리 상태를 한 번에 바꾸는 관리자 API 1개를 추가합니다.
#132(구역 조회)에 이어지는 사물함 관리자 API의 세 번째 작업입니다.
❓ 왜 해결해야 하나요?
사물함이 고장 나도 상태를 바꿀 방법이 DB 직접 수정뿐입니다.
LockerRepository에 사물함 쓰기 경로가 아예 없습니다 — 회차는 #119에서 뚫었지만 사물함은 조회만 있습니다.상태를
DISABLED로 내리면Locker.isSelectable이 false가 되어 학생 화면의 선택 가능 수와 칸 표시에서 빠집니다. 그게 이 기능의 목적입니다.⭐ 어떻게 해결했나요?
커밋 4개로 나눴고 각각 단독으로 컴파일됩니다.
1. 도메인 —
Locker에changeStatus를 더합니다. 바뀐 값이 있으면true를 돌려줍니다. 36칸 구역에서 두 칸만 바꿔도 전체를 저장하지 않으려고 서비스가 이 값으로 거릅니다.2. 저장소 —
saveLockers(여러 건 저장)와findByLockerIds를 추가합니다. 빈 목록이 들어오면 쿼리를 타지 않습니다. 회차 조회는 기존findPublishedPeriodsByIds를 그대로 씁니다.3. 서비스 —
AdminLockerService.changeLockerStatus입니다.검증을 전부 먼저 하고 바꿉니다. "하나라도 안 되면 전체를 안 바꾼다"를
@Transactional에만 기대지 않았습니다 — 예외로 롤백하는 것보다 사전 검증이 의도가 분명하고 실패 사유도 정확해집니다.이용 중 검사는
DISABLED일 때만 합니다.AVAILABLE로 되돌리는 건 막을 이유가 없고 쿼리 2개를 아낍니다.4. API — 기존
AdminLockerSectionController에@PatchMapping("/{sectionId}/lockers/status")를 더합니다. 관리자 "구역 관리" 탭 한 화면이 쓰는 API가 전부/sections/{sectionId}아래라 컨트롤러를 탭 단위로 맞췄습니다. 조회 컨트롤러에 쓰기가 섞이는 건AdminNoticeController·AdminLockerPeriodController선례가 있습니다.조회는
LockerService, 변경은AdminLockerService로 서비스 2개를 주입받습니다. "운영진 전용 동작은 Admin 서비스에 둔다"는 팀 규칙 그대로입니다.🧩 이 PR의 한계 & 트레이드오프
@DynamicUpdate가 아니라LockerJpaEntity.from(domain)+ merge로 전체 행을 저장해서 다른 컬럼까지 덮어씁니다. 다만 사물함은 상태 말고 바뀌는 값이 없고 운영진 전용이라 감수했습니다lockerIds에 중복이 와도 막지 않습니다. 저장은changeStatus가 두 번째에false를 돌려줘서 한 번만 되고, 응답에만 같은 항목이 두 번 담깁니다. 칸 그리드에서 선택 집합을 보내는 화면에선 일어나지 않아 요청 DTO에distinct를 두지 않았습니다LOCKER_NOT_IN_SECTION(400)입니다. 구역 조회 결과에 없다는 사실만으로는 둘을 가를 수 없어서, 가르려면 조회를 한 번 더 해야 합니다DISABLED변경은 쿼리가 2개 더 듭니다(신청 조회 → 회차 조회). 한 번에 바꾸는 칸이 수십 개 수준이라 묶음 조회 2개로 끝냈습니다AGENTS.md원칙에 따랐습니다⛓️ 기존 기능에 미치는 영향
LockerService를 하나도 고치지 않았습니다lockers.status컬럼이 이미 있습니다DISABLED로 내리면 학생 화면의 선택 가능 수와 칸 표시에서 빠집니다 — 의도한 동작입니다LockerErrorCode에 2개(LOCKER_NOT_IN_SECTION,LOCKER_IN_USE)가 늘지만 새 경로에서만 던집니다🔀 Edge Case & 실패 시나리오
lockerIds가 빈 배열INVALID_INPUT(400) + "사물함을 하나 이상 선택해 주세요."lockerStatus가 enum에 없는 값INVALID_INPUT(400) — Jackson이 못 읽어HttpMessageNotReadableExceptionLOCKER_NOT_IN_SECTION(400), 아무것도 안 바뀜DISABLED로LOCKER_IN_USE(409), 아무것도 안 바뀜DISABLED로AVAILABLE로is_deleted = false로 걸러LOCKER_NOT_IN_SECTION📋 검토한 대안과 선택 이유
architecture.md:74도infrastructure:*가 비즈니스 규칙을 갖지 않는다고 합니다. 저장소는 신청·회차를 읽어 오기만 합니다. (EventRepositoryImpl에 SQLCASE로 상태를 계산하는 선례가 있지만, 그건 그 값으로 목록을 거르느라 쿼리에서 해야 하는 경우라 여기엔 안 맞습니다)!period.isUsageEnded(today)로: 같은 판정에 이름이 두 개 생겨서, 내 신청 내역 화면이 쓰는LockerApplicationStatus.from(period, today)를 그대로 재사용했습니다. "배정 중"의 기준이 나중에 바뀌어도 한 곳만 고치면 됩니다findPeriodsByIds를 만들었다가 지웠습니다. 게시를 내리는 기능이 없고(AdminLockerService에 공개 토글이 없습니다), 신청은 게시된 회차에만 들어와서 기존findPublishedPeriodsByIds와 결과가 같습니다. 비슷한 이름의 포트를 늘리지 않았습니다LOCKER_NOT_IN_SECTION을 404로: "없는 사물함"으로 보면 404지만, 구역과 사물함의 조합이 틀린 요청이라 400으로 뒀습니다LOCKER_NOT_FOUND를 따로 두기: 1단계에서 만들었다가 지웠습니다. 위 이유로 던질 곳이 없어 죽은 코드가 됩니다. 사물함 등록·삭제가 붙을 때 필요하면 그때 넣습니다AdminLockerStatusController): 화면 단위로 묶는 쪽이/periods·/applications와 나란히 가서 기존 컨트롤러에 넣었습니다lockerStatus를String으로 받고from()으로 변환(AdminFeeController방식): enum을 그대로 받는NoticeCreateRequest쪽을 따랐습니다. 잘못된 값은 어차피 400INVALID_INPUT입니다ApiResponse<Void>로: 화면이 누른 칸을 다시 그려야 해서 바뀐 뒤 상태를 돌려줍니다PUT: 전체 교체가 아니라 상태 한 필드만 바꿔서PATCH입니다💬 리뷰 포인트
[r]이미 끝난 회차에만 배정된 칸은DISABLED로 바꿀 수 있게 했습니다. 이용 중 판정을LockerApplicationStatus.from(period, today) == ASSIGNED로 두어 내 신청 내역 화면과 기준을 한 곳으로 묶었습니다. 사용 종료일 당일까지는 사용 중입니다(isUsageEnded가today.isAfter(usageEndAt)). 운영 기준이 이게 맞는지 봐주세요[r]응답에 이미 그 상태였던 칸도 담습니다. 저장은 안 하고 반환만 합니다. 화면이 "실제로 바뀐 것"만 필요하다면 거르는 쪽으로 바꾸겠습니다[c]없는 사물함과 다른 구역 사물함을LOCKER_NOT_IN_SECTION하나로 묶었습니다. 운영진이 원인을 알기 어려우면 나눠야 할지[c]응답 레코드 이름이AdminLockerStatusChangeResponse입니다. 이 패키지 응답 4개가 전부Admin접두사를 써서 맞췄는데, 요청은 접두사 없는LockerStatusChangeRequest라 짝이 안 맞습니다[a]응답에lockerNumber를 넣었습니다(명세대로). 요청이lockerId로 들어오고 화면도 번호를 이미 알아서 빼도 됩니다