전남대 BE_26조 10주차 코드리뷰 요청 #95
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* test: 멤버 도메인 application 패키지 테스트 추가 * refactor: early return 리팩터링
* feature: reservation 페치조인, 페이징 * feat: ad 페치조인, 페이징 * feat: curation 페치조인, 페이징 * feat: event 페치조인, 페이징 * refactor: todo 추가 * fix: JPQL 수정 * fix: oneToMany관계 page 사용시 fetch join 수정 * feat: 정렬기준 createdDate가 아닌 ID 로 수정
* chore: github action jobs 그룹화 * chore: Dockerfile 못찾는 문제 수정 * chore: docker build 오류 수정 * chore: docker build 오류 수정 2 * chore: docker build 오류 수정 3
* chore: push일 때만 Deploy되도록 수정 * chore: push일 때만 Docker push 되도록 수정
…86) * refactor: test 관련 memberId 상수화 * chore: 이제는 github action시 테스트도 수행함
* fix: attempt 1 * fix: attempt 2 (log show) * fix: attempt 3 (add secrets.env) * feat: .env 업데이트 기능 추가 * feat: jobs 단계 세분화 * fix: test fail fix attempt 1 * fix: docker build fail fix attempt 2 * chore: add echo * chore: rename jobs step naming
* feat: reservation unit test * feat: 행사명으로 검색 로직 추가 * feat: url을 이미지로 저장하는 로직
syndersonlee
approved these changes
Nov 10, 2024
| @@ -14,10 +14,11 @@ public class ImageResponse { | |||
| private LocalDateTime createdDate; | |||
| private Long size; | |||
|
|
|||
| public static ImageResponse from(Image image) { | |||
| public static ImageResponse from(Image image, String domainName) { | |||
| System.out.println("도메인" + domainName); | |||
| @@ -44,7 +39,7 @@ protected String getSubPath() { | |||
|
|
|||
| @Override | |||
| protected EventImage toEntity(ImageRequest imageRequest) { | |||
| return EventImage.builder().name(imageRequest.getUrl()).size(imageRequest.getSize()).build(); | |||
| return EventImage.builder().url(imageRequest.getUrl()).size(imageRequest.getSize()).build(); | |||
There was a problem hiding this comment.
p3; imageRequest 파라미터를 EventImage 내부에 넣고 builder를 돌리는게 깔끔해보여요
toEntity는 여기에 선언하는게 안 맞아 보입니다.
| @@ -28,13 +32,13 @@ public abstract class ImageService<T extends Image> { | |||
|
|
|||
| protected abstract T toEntity(ImageRequest imageRequest); | |||
|
|
|||
| public T getByIdOrThrow(Long id) { | |||
| public T getById(Long id) { | |||
There was a problem hiding this comment.
p4; 개인 의견이지만 제네릭보다 String으로 받아서 Class로 objectMapper을 사용하길 권장드립니다.
이 부분에서 에러가 제일 많이 발생할 것으로 보여서, 로깅도 꼼꼼히 하면 좋겠네요
| .size(10L) | ||
| .extension(".jpeg") | ||
| .build(); | ||
| var request = ImageRequest.of(imageStorage.save(imageUrl, subPath), 123123L, "jpg"); |
There was a problem hiding this comment.
p3; 아직 테스트 값이 남아있네요 이 부분은 수정하실거라 생각하겠습니다
| @@ -16,7 +16,17 @@ public enum ErrorCode { | |||
|
|
|||
| FAIL_TO_READ_IMAGE(HttpStatus.CONFLICT.value(), "-20400", "Fail to read image"), | |||
|
|
|||
| ENTITY_PARAM_IS_NULL(HttpStatus.BAD_REQUEST.value(), "-20400", "%s is null"); | |||
| FAIL_TO_PAY(HttpStatus.CONFLICT.value(), "-20400", "Fail to %s"), | |||
There was a problem hiding this comment.
p5; 이전에 물어본적이 없었던 것 같은데 왜 -20400 일까요?
| .token(createToken(identifier.get())) | ||
| .build(); | ||
| // 소셜 계정의 회원가입 처리 | ||
| MemberResponse welcomeMemberResponse = memberService.createMember(MemberCreateRequest.builder() |
There was a problem hiding this comment.
p4; Request 내부에서 entity를 생성해서 반환하도록 하는 게 좋을 것 같아요
| @@ -22,6 +25,11 @@ public class CurationCardRequest { | |||
|
|
|||
| private List<Long> imageIds; | |||
|
|
|||
| @AssertTrue(message = "이미지는 최대 5개까지 등록할 수 있습니다.") | |||
| public boolean isImageSizeValid() { | |||
| return imageIds == null || imageIds.size() <= 5; | |||
There was a problem hiding this comment.
p2; Id가 not null인 것을 잘 못 표현한 것으로 보이네요
| @@ -64,7 +65,7 @@ public boolean isEndTimeAfterStartTime() { | |||
|
|
|||
| @AssertTrue(message = "이미지는 최대 5개까지 등록할 수 있습니다.") | |||
| public boolean isImageSizeValid() { | |||
| return imageIds.size() <= 5; | |||
| return imageIds == null || imageIds.size() <= 5; | |||
There was a problem hiding this comment.
p2; ids != null을 의도하신 건 아닌지 확인 부탁드릴게요
| import org.springframework.web.context.WebApplicationContext; | ||
|
|
||
| @WebMvcTest(CurationController.class) | ||
| class CurationControllerUnitTest { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
읽어주셔서 감사드립니다. 그리고 PR을 합병하실 때, Squash and merge를 해주시면 감사하겠습니다!