Conversation
songsunkook
requested review from
Choi-JJunho,
Invidam and
daheeParkk
as code owners
| @@ -0,0 +1,8 @@ | |||
| package in.koreatech.koin.global.exception; | |||
|
|
|||
| public class ArgumentCountException extends RuntimeException { | |||
There was a problem hiding this comment.
C
IllegalArgumentException과의 차이가 뭔가요?
| return ResponseEntity.ok().body(ownerInfo); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
다른 controller들 override 다 빠져있는데 빼는걸로 통일 어때요
| @JsonNaming(SnakeCaseStrategy.class) | ||
| public record InnerAttachmentUrlRequest( | ||
| @NotBlank | ||
| @URL(protocol = "https", regexp = ".*static\\.koreatech\\.in.*", message = "코인 파일 저장 형식이 아닙니다.") |
There was a problem hiding this comment.
C
메시지가 살짝 빈약해보이는데 사용자가 입력한 값을 보여주는건 어때요?
| @URL(protocol = "https", regexp = ".*static\\.koreatech\\.in.*", message = "코인 파일 저장 형식이 아닙니다.") | |
| @URL(protocol = "https", regexp = ".*static\\.koreatech\\.in.*", message = "코인 파일 저장 형식이 아닙니다. : ${validatedValue}") |
| public class OwnerAttachmentsCountException extends ArgumentCountException { | ||
|
|
||
| private static final String DEFAULT_MESSAGE = "첨부파일 개수가 부족합니다."; |
There was a problem hiding this comment.
A
IllegalArgumentException 상속해도 무관해보이는데 어떤가요
| } | ||
|
|
||
| @Transactional | ||
| public OwnerResponse putOwner(Long userId, OwnerUpdateRequest request) { |
There was a problem hiding this comment.
C
putOwner라는 메소드 명에서 어떤 작업을 수행하는지 유추가 잘 안되는거같아요
updateOwner라는 네이밍도 생각나는데 어떻게 생각하나요
| @JsonNaming(SnakeCaseStrategy.class) | ||
| public record OwnerUpdateRequest( | ||
| @Valid | ||
| @Size(min = 3, max = 5, message = "이미지는 사업자등록증, 영업신고증, 통장사본을 포함하여 최소 3개 최대 5개까지 가능합니다.") |
There was a problem hiding this comment.
R
1개로 변경되었습니다.
| @Size(min = 3, max = 5, message = "이미지는 사업자등록증, 영업신고증, 통장사본을 포함하여 최소 3개 최대 5개까지 가능합니다.") | |
| @Size(min = 1, max = 5, message = "이미지는 사업자등록증, 영업신고증, 통장사본을 포함하여 최소 1개 최대 5개까지 가능합니다.") |
| import jakarta.validation.constraints.Size; | ||
|
|
||
| @JsonNaming(SnakeCaseStrategy.class) | ||
| public record OwnerUpdateRequest( |
There was a problem hiding this comment.
R
비록 API는 PUT /owner 이지만 우리가 이해하는 바로는 OwnerUpdateRequest는 전혀 아닌거같아요
객체명을 봤을 때 사장님 정보를 업데이트하는 것 처럼 느껴졌는데 들어왔더니 fileUrls만 있어서 당황스럽군요...
다른 정보도 수정할 수 있게끔 값을 추가해야할 필요가 있어보이네요
현재 클라이언트에서 PUT /owner 구현이 안되어있는걸로 알고있는데 한번 논의를 해봐야겠군요
| for (OwnerAttachment attachment : attachments) { | ||
| if (ownerAttachmentRepository.findByOwnerIdAndUrl(userId, attachment.getUrl()).isPresent()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
R
이미 존재하는 이미지는 업데이트 안하는군요..
PUT이라는 메소드에 어울리는 행위일까요? PUT은 멱등성을 보장한다는 특징을 잘 생각해보시면 좋을거같아요
| return ResponseEntity.badRequest().body(ErrorResponse.from(e.getMessage())); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(ErrorResponse.from("요청 파라미터가 잘못되었습니다.")); |
There was a problem hiding this comment.
R
DTO Validation에서 걸린 에러에 대해서 메시지를 출력해주는건데 이거 바꾼 이유가 있을까요?
| } | ||
|
|
||
| @Test | ||
| @DisplayName("사장님 인증용 이미지를 업로드한다.") |
There was a problem hiding this comment.
R
이미지를 업로드한다는 문장이 MultiPartImage를 업로드하는 것 처럼 느껴져요
이 경우는 이미지 URL을 갱신한다가 어울리지 않나요?
2 participants
🔥 연관 이슈
🚀 작업 내용
💬 리뷰 중점사항
사장님 관련 기능이라 비즈니스팀 리뷰요청했습니당
+) 분명 첨부파일 업로드인데 왜 URI가
PUT /owner인지 모르겠습니다.. 나중에 수정하는 것도 좋을 것 같아요