-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT/#287] 단일 Banner 상세 조회 기능 #288
base: develop
Are you sure you want to change the base?
Conversation
- 조회 성공 상태에 대한 Success Status Code Enum 추가 구현했습니다. (`BannerSuccessCode`)
소문자인 외부 반환 데이터를 관리하는 필드 `value`을 선언했습니다. 또한 외부 조회 등에 사용될 경우, `value`를 통해 조회할 수 있도록 하기 위해 `getByValue` 메서드를 정의했고 조회하려는 값에 해당하는 Enum이 없을 경우 Banner Exception이 발생하도록 했습니다. - ContentType : 게시물 유형 - PublishLocation : 게시 위치 - PublishStatus : 게시 상태
단일 Entity 내에 모든 필드를 관리할 경우, 관리 및 유지보수에 어려움이 있을 것이며 관심사가 혼재될 것을 우려하여 Embeddable 객체로 분리하여 정의했습니다. - `BannerImage` : PC/Mobile 용 이미지 객체 - `PublishPeriod` : 게시 기간 객체 - 게시 시작 날짜 & 게시 종료 날짜 기반으로 `PublishStatus`를 연산하여 반환하는 메서드 `getPublishStatus`를 정의했습니다.
- 신규 도메인 데이터 테이블로 생성할 Banner (`banners`) 테이블에 대해 복수형 전환 분기 처리를 추가했습니다. * `spring.jpa.properties.hibernate.globally_quoted_identifiers` 속성이 true로 지정될 경우, 자동으로 double quote 가 생성되며 H2 DB에서 테이블 인식에 문제가 생기는 이유로 해당 속성을 제거했습니다.
Embedded 객체 변경이 정상적으로 처리되는지 중점적으로 확인했습니다. * 구현 과정에서 발생했던 오탈자 수정을 진행했습니다.(`BannerImage#updateMobileImage`)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰가 너무 늦어져서 죄송합니다 ㅜ.ㅜ..
잘 구현해주셔서 제 리뷰가 가독성 측면에서 개선되면 좋을 점들을 적은 리뷰라 일단 어프루브 했습니다!
@RequiredArgsConstructor(access = PRIVATE) | ||
public enum BannerSuccessCode implements SuccessCode { | ||
SUCCESS_GET_BANNER_DETAIL(HttpStatus.OK, "배너 상세 정보 조회 성공"), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p4
여기 공백 불필요할 것 같습니다!
SUCCESS_GET_BANNER_DETAIL(HttpStatus.OK, "배너 상세 정보 조회 성공"), | ||
|
||
; | ||
private final HttpStatus status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p4
여기에 공백이 들어가면 좋을 것 같아요!
|
||
import org.sopt.makers.operation.common.domain.BaseEntity; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
여기 공백 한줄만 있으면 좋을 것 같습니다!
@Embedded | ||
@AttributeOverrides({ | ||
@AttributeOverride(name = "pcImageUrl", column = @Column(name = "img_url_pc", nullable = false)), | ||
@AttributeOverride(name = "mobileImageUrl", column = @Column(name = "img_url_mobile", nullable = false)) | ||
}) | ||
private BannerImage image; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p5
Embedded 타입쓴거 좋네요 ㅎ
CREW_MAIN("cr_main"), | ||
CREW_FEED("cr_feed"), | ||
OFFICIAL_PAGE("org"), | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
여기 공백한 줄 있으면 좋을 것 같습니다!
} | ||
|
||
public PublishStatus getPublishStatus(LocalDate date) { | ||
if (date.isAfter(endDate)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
boolean 타입으로 설명적 변수로 위에 정의해주면 조건문 확인 때 더 유용할 것 같아요!
boolean isDone = date.isAfter(endDate);
boolean isInProgress = date.isAfter(startDate);
if(isDone) {
return PublishStatus.DONE;
} else if(isInProgress) {
return PublishStatus.IN_PROGRESS;
}
return PublishStatus.RESERVED;
RESERVED("reserved"), | ||
IN_PROGRESS("in_progress"), | ||
DONE("done"), | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
공백 한 줄 넣어서 변수 선언부랑 분리되면 좋을 것 같습니다!
@@ -58,6 +61,7 @@ private String convertToSnake(String camel) { | |||
} | |||
index++; | |||
} | |||
System.out.println(builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
print로 확인하는 이유가 혹시 있을까요??
|
||
// then | ||
Banner resultBanner = bannerRepository.findById(TEST_BANNER_ID).get(); | ||
assertThat(resultBanner.getPeriod().getStartDate()).isEqualTo(givenUpdateStartDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
LocalDate resultBannerStartDate = resultBanner.getPeriod().getStartDate();
assertThat(resultBannerStartDate).isEqualTo(givenUpdateStartDate);
로 작성하는 건 어떨까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 클래스에 있는 assert문들 내의 긴 변수들을 설명적 변수로 분리하면 테스트에서 비교하는 내용을 더 명확하게 이해할 수 있을 것 같다는 생각입니다!
assertThat(resultPeriod.getStartDate()).isEqualTo(TEST_BANNER_START_DATE); | ||
assertThat(resultPeriod.getEndDate()).isEqualTo(TEST_BANNER_END_DATE); | ||
} | ||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
여기 띄어쓰기가 누락된 것 같습니다!
Related Issue 🚀
Work Description ✏️
/api/v1/banners/{bannerId}
GET
PR Point 📸
※
@Embeddable
서브 도메인 객체 구현 -@Embedded
적용※
@WebMvcTest
사용아래와 같은 상황과 고민이 있었습니다.