Conversation
|
Caution Review failedThe pull request is closed. Walkthrough발주 템플릿 수정 기능을 추가합니다. PATCH 엔드포인트, 서비스 로직(사용자·공급처·템플릿 검증 후 업데이트), 요청 DTO, 에러/성공 메시지 추가, 리포지토리 패키지 조정 및 관련 컨트롤러·서비스 단위/통합 테스트가 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as OrderTemplateController
participant Service as OrderTemplateService
participant UserRepo as UserRepository
participant VendorRepo as VendorRepository
participant TemplateRepo as OrderTemplateRepository
User->>Controller: PATCH /api/v1/order-template/{id} (UpdateOrderTemplateRequest)
Controller->>Service: updateOrderTemplate(id, request, userId)
Service->>UserRepo: findById(userId)
alt user found
UserRepo-->>Service: User
Service->>VendorRepo: findById(vendorId)
alt vendor found
VendorRepo-->>Service: Vendor
alt vendor.store == user.store
Service->>TemplateRepo: findById(templateId)
alt template found
TemplateRepo-->>Service: OrderTemplate
alt template.vendor == vendor
Service->>Service: orderTemplate.updateTemplate(...)
Service-->>Controller: OrderTemplateResponse
Controller-->>User: 200 OK + UPDATE_ORDER_TEMPLATE_SUCCESS
else access denied
Service-->>Controller: BaseException(ORDER_TEMPLATE_ACCESS_DENIED)
Controller-->>User: 403 Forbidden
end
else template not found
Service-->>Controller: BaseException(ORDER_TEMPLATE_NOT_FOUND)
Controller-->>User: 404 Not Found
end
else vendor access denied
Service-->>Controller: BaseException(VENDOR_ACCESS_DENIED)
Controller-->>User: 403 Forbidden
end
else vendor not found
Service-->>Controller: BaseException(VENDOR_NOT_FOUND)
Controller-->>User: 404 Not Found
end
else user not found
Service-->>Controller: BaseException(USER_NOT_FOUND)
Controller-->>User: 404 Not Found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 검토 시 우선 확인할 항목:
권장 참조:
짧게: 컨벤션과 예외 매핑만 확인하면 됩니다 — 잘했습니다. 😉 Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
🧹 Nitpick comments (3)
src/test/java/com/almang/inventory/order/template/service/OrderTemplateServiceTest.java (1)
27-258: 서비스 레이어 성공/실패 플로우가 잘 커버되어 있습니다.성공 케이스부터
USER_NOT_FOUND,VENDOR_NOT_FOUND,VENDOR_ACCESS_DENIED,ORDER_TEMPLATE_NOT_FOUND,ORDER_TEMPLATE_ACCESS_DENIED까지 핵심 분기들이 모두 테스트되어 있어서 회귀를 잡는 데 도움이 많이 될 것 같습니다.newStore/newUser/newVendor/newOrderTemplate헬퍼 메서드로 픽스처를 정리해 둔 것도 읽기 좋아요.자잘한 부분으로는,
다른_상점_발주처면_발주_템플릿_수정시_접근이_거부된다테스트에서 생성한store2User는 사용되지 않으니 제거하면 더 깔끔해집니다. 또, 유사한 예외 테스트들이 많으니 나중에 여유가 생기면 파라미터라이즈드 테스트로 정리해 보는 것도 유지보수에 도움이 될 것 같습니다.src/test/java/com/almang/inventory/order/template/controller/OrderTemplateControllerTest.java (1)
31-167: 컨트롤러 단 HTTP 플로우 테스트 구성이 좋습니다.
200/404/400응답, 메시지,data필드 존재 여부까지 모두 검증하고 있어서 API 계약이 깨지는 걸 잘 잡아줄 것 같습니다.@WebMvcTest+@MockitoBean조합도 적절하게 사용하셨네요.다만
발주_템플릿_수정_요청값_검증에_실패하면_예외가_발생한다테스트는 실제로는vendorId의@NotNull에만 의존하고 있으니, 제목/본문에 추가 제약을 둘 계획이라면 그에 맞는 별도 테스트를 추가해 주면 더 명확해질 것 같습니다(예: 빈 문자열일 때@NotBlank위반을 기대하는 테스트 등). 또, 나중에 여유가 생기면ORDER_TEMPLATE_ACCESS_DENIED에 대한 403 응답 케이스도 컨트롤러 단에서 한 번 더 검증해 두면 전체 에러 핸들링 일관성을 확인하는 데 도움이 되겠습니다.src/main/java/com/almang/inventory/order/template/controller/OrderTemplateController.java (1)
31-45: 엔드포인트 설계와 서비스 연동이 전반적으로 잘 정리되어 있습니다.
PATCH /api/v1/order-template/{orderTemplateId}+@ValidDTO +CustomUserPrincipal조합이 기존 구조와 자연스럽게 맞고,ApiResponse.success(SuccessMessage.UPDATE_ORDER_TEMPLATE_SUCCESS...)로 응답 형식도 일관성을 유지하고 있습니다. 로그에userId,orderTemplateId를 남기는 것도 추적에 유용해 보입니다.설계 관점에서 한 가지만 참고용으로 말씀드리면, 현재는
orderTemplateId(path) +vendorId(body)를 모두 받아서 서비스에서 교차 검증을 하고 있는데, 템플릿의 소속 발주처/상점은orderTemplateId만으로도 역으로 찾아갈 수 있으므로, 장기적으로는 body에서vendorId를 제거하고 service에서 템플릿 → 발주처 → 상점 순으로 권한을 검증하는 구조도 고려해볼 수 있습니다. 지금 구조가 이미 다른 API들과 맞춰진 것이라면 유지하셔도 무방합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java(1 hunks)src/main/java/com/almang/inventory/global/exception/ErrorCode.java(1 hunks)src/main/java/com/almang/inventory/order/template/controller/OrderTemplateController.java(1 hunks)src/main/java/com/almang/inventory/order/template/dto/request/UpdateOrderTemplateRequest.java(1 hunks)src/main/java/com/almang/inventory/order/template/service/OrderTemplateService.java(1 hunks)src/test/java/com/almang/inventory/order/template/controller/OrderTemplateControllerTest.java(1 hunks)src/test/java/com/almang/inventory/order/template/service/OrderTemplateServiceTest.java(1 hunks)
🔇 Additional comments (2)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java (1)
36-39: 발주 템플릿 수정 성공 메시지 추가가 기존 패턴과 잘 맞습니다.
CREATE_ORDER_TEMPLATE_SUCCESS와 짝을 이루는UPDATE_ORDER_TEMPLATE_SUCCESS를 분리해서 두니, 도메인별 성공 메시지 관리가 한눈에 들어옵니다. 컨트롤러/서비스에서 공통 메시지로 재사용하기에도 좋아 보여요.src/main/java/com/almang/inventory/global/exception/ErrorCode.java (1)
33-38: 발주 템플릿 전용 에러 코드 분리가 깔끔합니다.
*_NOT_FOUND,*_ACCESS_DENIED패턴을 그대로 따라가서, 서비스 레이어에서 어떤 상황에서 어떤 에러가 날지 읽기 쉽습니다. 향후 ORDER 관련 에러가 늘어나도 이 섹션에만 추가하면 되어 유지보수성이 좋겠습니다.
| package com.almang.inventory.order.template.dto.request; | ||
|
|
||
| import jakarta.validation.constraints.NotNull; | ||
|
|
||
| public record UpdateOrderTemplateRequest( | ||
| @NotNull Long vendorId, | ||
| String title, | ||
| String body, | ||
| Boolean activated | ||
| ) {} |
There was a problem hiding this comment.
activated 필드의 null 허용/검증 규칙을 명확히 하는 게 좋습니다.
현재 DTO는 vendorId만 @NotNull이고, title, body, activated는 nullable입니다. 그런데 OrderTemplateService.updateOrderTemplate에서 orderTemplate.updateTemplate(request.title(), request.body(), request.activated());로 바로 넘기고 있어서, activated가 null인 요청이 들어오면 오토 언박싱이나 엔티티 필드 타입에 따라 NPE가 발생할 수 있습니다(자바에서 Boolean → boolean 변환 시 null이면 NPE).
선택지는 크게 두 가지로 보입니다.
- 전체 필드 필수 업데이트 모델이라면
activated에도 검증을 맞추는 쪽이 자연스럽습니다.public record UpdateOrderTemplateRequest( @NotNull Long vendorId, String title, String body, -
Boolean activated
-
) {}
@NotNull Boolean activated혹은 타입을 `boolean`으로 바꾸어 null 자체를 허용하지 않는 방식도 가능합니다.
- 진짜 부분 업데이트(PATCH)로 설계한다면
activated가 null일 수 있다는 걸 서비스/엔티티에서 명시적으로 처리해야 합니다.처럼 필드별로 업데이트 여부를 분리하는 식입니다.if (request.activated() != null) { orderTemplate.changeActivated(request.activated()); }
또, 생성용 DTO가 이미 있다면 그쪽 검증 규칙과 맞추어 두면 클라이언트 입장에서도 제약을 예측하기 쉬울 것 같습니다.
src/main/java/com/almang/inventory/order/template/service/OrderTemplateService.java
Show resolved
Hide resolved
1 participant
✨ 작업 내용
📝 적용 범위
/order/template📌 참고 사항
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.