Repository navigation
[Feat/#132] 사물함 구역 목록·상세 관리자 API 추가 - #141
sangrae2325 wants to merge 3 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 |
f93503c to
41dc40e
Compare
41dc40e to
baa4847
Compare
| @@ -0,0 +1,14 @@ | |||
| -- 구역 사진을 여러 장 저장한다. 구역당 한 장 전제로 V15에서 단일 컬럼을 뒀는데, 실제로 두 장인 구역이 있다. | |||
| @RequiredArgsConstructor | ||
| public class AdminLockerSectionController implements AdminLockerSectionApi { | ||
|
|
||
| private final LockerService lockerService; |
There was a problem hiding this comment.
[r] 운영진 조회가 학생 앱용 LockerService를 그대로 쓰고 있는데, 최근 coding-style.md 2-7절에 "운영진 전용 기능은 Admin{Domain}Service로 분리하고, 두 서비스는 서로 호출하지 않는다"가 들어가서 AdminLockerService 쪽으로 옮기면 좋을 것 같아요.
규칙 말고도 동작 차이가 하나 있어요. getSections·getSectionDetail은 입구에서 requirePublishedPeriod로 게시된 회차만 통과시키는데, 운영진 회차 API(#129)는 미게시 회차도 보여줘서, 운영진이 목록에서 미게시 회차를 고르면 구역 API가 LOCKER_PERIOD_NOT_FOUND를 받게 돼요. API로 만든 회차는 항상 게시 상태라 당장은 드물겠지만, V9 기본값이 is_published = 0이라 DB로 직접 넣은 회차는 미게시일 수 있어서요.
구역별 개수를 세는 로직은 LockerSectionSummary 쪽 정적 팩토리로 옮기면 두 서비스가 서로 호출하지 않고도 같이 쓸 수 있을 것 같아요. 그러면 #148에서 이 컨트롤러가 LockerService와 AdminLockerService를 함께 주입하는 것도 하나로 정리될 것 같습니다.
| .filter(locker -> locker.isSelectable(appliedLockerIds.contains(locker.getId()))) | ||
| .count(); | ||
| // 사용 중지된 사물함도 배정돼 있으면 센다. 배정 후 고장 난 칸이라 운영진이 다른 자리를 줘야 하는 대상이다 | ||
| int assignedCount = (int) lockers.stream() |
There was a problem hiding this comment.
[c] assignedCount는 운영진 화면에서만 쓰는데 학생용 summarize에서 계산하고 있어서, 학생 구역 목록 요청마다 계산만 하고 버려지고 있어요. 위 컨트롤러 코멘트처럼 운영진 조회를 AdminLockerService로 옮길 때 이 계산도 같이 옮기면 좋을 것 같아요.
| * 배정된 뒤 사용 중지된 칸은 {@code ASSIGNED}로 본다. 운영진이 그 학생에게 다른 자리를 줘야 하는 상황이라 | ||
| * 배정 사실이 물리 상태보다 먼저 보여야 한다. | ||
| */ | ||
| public static LockerDisplayStatus from(LockerStatus status, boolean applied) { |
There was a problem hiding this comment.
[c] 리뷰 포인트 주신 "배정된 뒤 사용 중지된 칸을 ASSIGNED로 본 판단"에 대한 의견이에요. 배정 사실을 먼저 보여주려는 의도는 좋은데, 하나로 합쳐 버리면 그 칸이 사용 중지 상태인지 화면에서 알 수 없어서, 정작 "다른 자리를 줘야 하는 칸"을 찾기 어려울 것 같아요. 물리 상태와 배정 여부를 나눠서 내려주면 화면이 둘 다 표시할 수 있고, #148 요청 필드(lockerStatus)와도 이름이 맞춰질 것 같아요.
public record LockerItem(Long lockerId, int lockerNumber, String lockerLabel, LockerStatus lockerStatus, boolean assigned)그리고 #148에서는 이용 중인 칸을 DISABLED로 바꾸지 못하게 막고 있어서, 이 주석이 전제하는 "배정 후 고장 난 칸"이 API로는 생기지 않는 상태예요. 두 PR 중 어느 쪽으로 맞출지 같이 정해주시면 좋겠습니다!
| if (applied) { | ||
| return ASSIGNED; | ||
| } | ||
| return status == LockerStatus.AVAILABLE ? AVAILABLE : DISABLED; |
There was a problem hiding this comment.
[a] 여기서는 AVAILABLE을 status == LockerStatus.AVAILABLE로 따로 판정하는데, 목록의 availableCount는 Locker.isSelectable로 세고 있어요. isSelectable Javadoc에도 "목록 수와 상세 여부가 같은 기준을 써야 하므로 도메인에 둔다"고 되어 있어서, 상태가 늘거나 판정이 바뀌면 둘이 어긋날 수 있을 것 같아요. 위 코멘트처럼 필드를 나누면 자연스럽게 없어지는 부분이고, enum을 유지하신다면 Locker를 받아서 이렇게 맞추면 될 것 같아요.
if (applied) {
return ASSIGNED;
}
return locker.isSelectable(false) ? AVAILABLE : DISABLED;| * | ||
| * @param lockerPeriodId 조회할 사물함 운영 회차 식별자 | ||
| */ | ||
| public record LockerPeriodParams( |
There was a problem hiding this comment.
[a] 리뷰 포인트 주신 LockerPeriodParams 복제는 지금처럼 Params + @ModelAttribute로 두는 게 coding-style.md 2-2절에도 맞아서 그대로 좋을 것 같아요. GlobalExceptionHandler 보강은 별도 이슈로 가는 데 동의하고, 그때 @PathVariable Long sectionId에 abc 같은 값이 들어와 MethodArgumentTypeMismatchException으로 500이 나는 것도 같이 다루면 좋을 것 같아요.
| @Operation(summary = "사물함 구역 목록 조회", | ||
| description = "운영 회차를 기준으로 구역별 전체 사물함 수, 선택 가능한 사물함 수, 배정된 사물함 수를 조회한다. " | ||
| + "사용 중지된 사물함과 그 회차에 배정된 사물함은 선택 가능 수에서 빠진다. " | ||
| + "배정 수는 배정된 뒤 사용 중지된 사물함도 센다. 구역은 식별자 오름차순으로 반환한다.") |
There was a problem hiding this comment.
[a] 구역을 식별자 오름차순으로 둔 것도 좋아 보여요. 학생 목록(findAllByOrderByIdAsc)과 순서가 같고, 라벨로 정렬하면 A-10이 A-2보다 앞에 오는 문제가 생기니까요. 나중에 운영진이 순서를 바꿔야 하면 그때 display_order를 두면 될 것 같아요.
| /** 운영진은 사진을 지우거나 순서를 바꿀 때 파일 식별자가 필요하므로 URL과 함께 내려준다. */ | ||
| public record Photo(Long fileId, String fileUrl) { | ||
|
|
||
| public static Photo from(Long fileId, File file) { |
There was a problem hiding this comment.
[a] 값 두 개를 받는 팩토리라 coding-style.md 2-10절 기준으로는 of가 맞을 것 같아요. 공지·행사·아카이브 응답의 Image.of(fileId, file)와도 맞춰지고요. 아래 LockerItem.from(Locker, boolean)도 같은 경우예요.
| UPDATE locker_section_layouts | ||
| SET photo_file_ids = JSON_ARRAY(photo_file_id); | ||
|
|
||
| ALTER TABLE locker_section_layouts |
There was a problem hiding this comment.
[a] 스키마 변경·백필·DROP이 한 파일에 있는데, MySQL은 DDL이 트랜잭션으로 묶이지 않아서 중간에 실패하면 손으로 복구해야 해요. 그리고 photo_file_id를 지운 뒤에는 이전 이미지로 롤백하면 ddl-auto: validate에서 기동이 안 될 것 같아요. dev가 0건이라 큰 문제는 아니겠지만, V9·V20처럼 "적용 전 확인" 주석을 남겨두면 좋을 것 같습니다.
| Long lockerPeriodId = params.lockerPeriodId(); | ||
| LockerSectionDetail detail = lockerService.getSectionDetail(lockerPeriodId, sectionId); | ||
| // 배치 구조를 아직 등록하지 않은 구역이면 사진도 없다 | ||
| Map<Long, File> photosById = fileService.findAllByIdIn( |
There was a problem hiding this comment.
[a] "layout이 없으면 사진도 없다" 처리가 여기와 AppLockerController에 반복되고, 지워진 사진을 거르는 방식도 응답마다 달라요(관리자는 filter(photosById::containsKey), 앱은 filter(Objects::nonNull)). LockerSectionDetail에 photoFileIds()(layout이 없으면 빈 목록)를 두면 호출부가 단순해지고 거르는 방식도 하나로 맞출 수 있을 것 같아요.
#️⃣연관된 이슈
🎯 해결하려는 문제가 무엇인가요?
운영진 콘솔에서 사물함 구역을 조회하는 관리자 API 2개를 추가합니다.
GET /v1/admin/lockers/sections?lockerPeriodId=— 구역 목록 + 구역별 전체·선택 가능·배정 수GET /v1/admin/lockers/sections/{sectionId}?lockerPeriodId=— 칸 배치·사진·사물함별 상태#119(운영 회차)에 이어지는 사물함 관리자 API의 두 번째 작업이고, 회차를 입력으로 받는 첫 소비자입니다.
❓ 왜 해결해야 하나요?
구역·사물함을 볼 방법이 학생 앱뿐입니다. 운영진이 배정 현황을 보려면 DB를 직접 봐야 합니다.
그리고 구역 사진이 한 장만 저장됩니다. V15가 구역당 한 장을 전제로
photo_file_id단일 컬럼을 뒀는데, 실제로는 사진 두 장을 띄우는 구역이 있습니다. 학생 앱도 여러 장을 보여줘야 해서 이 PR에서 함께 고칩니다.⭐ 어떻게 해결했나요?
커밋 3개로 나눴고 각각 단독으로 컴파일됩니다.
1. 구역 사진 여러 장 지원
photo_file_id→photo_file_ids JSON으로 바꿉니다(V21).notices.image_ids·events.image_ids등 네 컬럼이 이미 쓰는 방식 그대로@JdbcTypeCode(SqlTypes.JSON)+List<Long>입니다. 별도 테이블을 쓰는 곳은 저장소에 없어서 그쪽이 오히려 새 패턴이 됩니다.학생 앱 응답도
photoUrl(문자열) →photoUrls(배열)로 바꿉니다. #98이 URL 하나만 주던 모양을 그대로 복수화했습니다.2. 구역 목록에 배정 수 추가
LockerSectionSummary에assignedCount를 더합니다.getSections가 이미 신청 식별자를 들고 있어 세는 건 한 줄입니다. 학생 응답은 이 값을 읽지 않아 그대로입니다.3. 관리자 API
LockerService를 하나도 고치지 않고 기존getSections·getSectionDetail·getAppliedLockerIds를 그대로 씁니다. 구역은 회차에 묶여 있지 않고(locker_sections에locker_period_id가 없습니다) 회차는 "어느 회차의 배정 현황을 볼지"를 정하는 기준일 뿐이라, 학생 경로와 읽는 것이 같습니다.사물함 표시 상태(
AVAILABLE/DISABLED/ASSIGNED)는 저장된 물리 상태와 회차 배정 여부를 합친 값이라 응답 레코드 안 중첩 enum으로 뒀습니다(coding-style.md2-2절). #65 리뷰에서 "표시용 boolean은 api 모듈에서 조합하라"고 받은 지적을 따랐습니다.🧩 이 PR의 한계 & 트레이드오프
Layout레코드가 학생 응답과 12줄 중복입니다.app-api와admin-api는 서로 의존하지 않고,common-api로 올리면 공유 계층이core:domain:event에 의존하게 됩니다. 모듈 경계상 피할 수 없다고 보고 복제했습니다LockerPeriodParams가 app-api의 같은 이름 레코드와 중복입니다. 필드 하나라@RequestParam으로 받으려 했는데,GlobalExceptionHandler가MissingServletRequestParameterException을 다루지 않아 값이 빠지면 500이 납니다. 400으로 내보내려면@Valid @ModelAttribute가 필요해 DTO로 뒀습니다. (저장소의 기존@RequestParam10곳은 전부required = false라 이 경로를 밟은 적이 없습니다. 핸들러 보강은 별도 이슈가 맞다고 봅니다)locker_sections에 순서 컬럼이 없습니다. 라벨이A-1·A-2·B-1이라 지금은 ID 순 = 라벨 순이고, 중간에 구역을 추가해 어긋나면 그때display_order를 둡니다JSON NOT NULL로 둬도 빈 배열이 통과합니다. 최소 한 장 검증은 구역 쓰기 API에서@Size(min = 1)로 합니다AGENTS.md원칙에 따랐습니다⛓️ 기존 기능에 미치는 영향
photoUrl(문자열) →photoUrls(배열). 사진 파일이 삭제된 경우 기존에는null이었는데 이제 목록에서 빠집니다. 앱이 아직 API 연동 전이라(뷰 퍼블리싱만) 실제 영향은 없습니다photo_file_id를DROP COLUMN합니다. 되돌릴 수 없습니다. devlocker_section_layouts가 0건이라 백필이 건드릴 행도 없습니다LockerSectionLayout.photoFileId→photoFileIds로 바뀌어 엔티티와 학생 컨트롤러가 함께 바뀝니다LockerSectionSummary에 필드가 하나 늘지만 구성 지점이 서비스 한 곳뿐이라 파급이 없습니다🔀 Edge Case & 실패 시나리오
LOCKER_PERIOD_NOT_FOUND(404)lockerPeriodId누락INVALID_INPUT(400) + "사물함 운영 회차를 입력해 주세요."LOCKER_SECTION_NOT_FOUND(404)layout이null,photos가 빈 배열photos에서 빠짐ASSIGNED, 목록의assignedCount에도 포함마지막 줄이 명세에 없어 정한 부분입니다. 운영진이 그 학생에게 다른 자리를 줘야 하는 상황이라 배정 사실이 물리 상태보다 먼저 보여야 한다고 봤습니다. 그래서
totalCount - availableCount - assignedCount가 0이 아닐 수 있습니다(배정 없이 사용 중지된 칸).📋 검토한 대안과 선택 이유
locker_section_photos별도 테이블로: 정석이지만 저장소에 그 패턴이 없고, 파일 목록은 전부 JSON 컬럼입니다. 노출 순서도 배열 순서로 표현됩니다photos를 사진 없을 때null로: 명세 설명문은 null, 필드 표는 nullable 아님으로 서로 다릅니다. 클라이언트가 null 체크를 안 하도록 빈 배열로 통일했습니다photos: [{fileId, fileUrl}]로: #98이 URL 문자열만 주고 있었고 학생 앱은 보여주기만 해서photoUrls: [String]로 뒀습니다. 관리자는 사진을 지우거나 순서를 바꿀 때fileId가 필요해 명세대로 객체 배열입니다LockerAvailability)를 도메인에서 걷어낸 적이 있습니다. 응답 중첩 enum으로 뒀습니다LockerStatus에ASSIGNED추가:lockers.status에 저장되는 값이라 DB에 없는 파생값을 섞으면 안 됩니다💬 리뷰 포인트
[c]Layout레코드 중복 — 모듈 경계상 불가피하다고 봤는데 다른 방법이 있다면[c]LockerPeriodParams복제 —@RequestParam의 500 문제 때문인데,GlobalExceptionHandler보강을 별도 이슈로 먼저 할지[a]배정된 뒤 사용 중지된 칸을ASSIGNED로 본 판단[a]구역 순서를 식별자 오름차순으로 둔 것검증 범위
compileJava·compileTestJava,:core:domain:event:test,:infrastructure:db:test(Testcontainers — V21 적용과 엔티티 스키마 검증 포함),ModularityTests.verify()·DomainImplAccessTests통과했습니다.