Skip to content

Conversation

@songsunkook
Copy link
Contributor

▶ Request

Content

#227

as-is

사장님이 상점을 직접 등록하는 API가 존재하지 않아 수동으로 상점을 추가해주어야 했음.

to-be

사장님이 자신이 소유한 상점 정보를 직접 등록할 수 있도록 하는 API를 작성함.

참고사항
요청을 보낼 때 상점 정보 json에 들어있는 open 리스트 (상점 운영시간) 의 길이는 반드시 7이어야 함.
(일주일 요일 수)

INSERT 대상 테이블

shops
shop_opens
shop_category_map
shop_images

✅ Check List

  • 의도치 않은 변경이 일어나지 않았는지.
    • 실수로 라이브러리(pom.xml) 변경이 일어나지 않았는지
    • 병합시 컴파일 & 런타임 에러가 발생하지 않는지
    • 기존 클라이언트와의 호환성 고려가 잘 이루어졌는지
  • 작성한 코드가 프로젝트에 반영됨을 명심하였는지
    • 타인도 알아보고 변경할 수 있는 코드를 작성하였는지
    • 코드 & 커밋 컨벤션을 준수하였는지
    • (필요한) 문서화가 진행되었는지
  • (기능 추가의 경우) 클라이언트의 입장에 대한 충분한 고려가 이루어졌는지
    • 클라이언트 측과 협의가 된 내용인 경우
    • 클라이언트의 요구사항을 잘 반영하는지
    • API 문서에 논리적인 오류 & 가시성이 떨어지는 내용이 없는지

📸 API Document ScreenShot

image

🧪 Test

  • 입력된 데이터가 정확하게 DB에 insert되는지 확인
  • 기존에 동일한 상점명으로 등록된 상점이 존재하더라도 별개의 상점으로써 최초 등록이 이루어지는지 확인

1. Response를 ShopResponse에서 EmptyResponse로 변경하여 아무것도 반환하지 않도록 변경
2. CreateShop 요청에 대한 DTO to Domain 변환을 update 메서드를 추가하여 진행
3. ShopConverter 오타 수정
4. SQL문 수정
  - 불필요한 insert value 제거
  - selectkey를 통하여 insert 완료된 행의 id를 반환하도록 작성
1. 메서드명 변경 (다중정의 해제)
2. 파라미터 제거
@songsunkook songsunkook requested a review from Invidam May 27, 2023 08:32
Copy link
Contributor

@Invidam Invidam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰 남겼으니 확인부탁드려요~

자바 스터디 하며 배웠던 내용들을
실제 프로젝트에도 적용해보는 건 어렵지만
본인에게 큰 도움이 될테니 이런 기회에 시도해보면 좋을 것 같아요!

@NotEmpty(message = "소속시킬 상점 카테고리는 최소 1개 선택하여야합니다.")
@ApiModelProperty(notes = "상점 카테고리 고유 id 리스트 \n" +
"- not null \n" +
"- 최소 1개", example = "[1, 4]", required = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swagger 예시 응답

image
대신
image

처럼 넘길 순 없을까요? (AllShopsResponse 참고)

참고

List<String>을 응답하는 경우는

allowableValues = "https://static.koreatech.in/example1.png", 처럼 설정하면
image

이렇게는 되더라구요

참고 swagger-api/swagger-core#1855

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스웨거에서 리스트를 Example Value에 작성하는 방법을 찾을 수 없었습니다..
따라서 기존 AllShopsResponse와 같이 example을 제거하는 방향으로 진행하였습니다.

})
@ParamValid
@ResponseStatus(HttpStatus.CREATED)
@RequestMapping(value = "", method = RequestMethod.POST)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PUT /admin/shops 에서 s가 꼭 붙어야 할까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 단의 RequestMapping에서 클래스 단의 매핑값을 지우는 방법은 찾지 못해서 이렇게 작성했습니다..

Comment on lines 92 to 105
// ======= shop_opens 테이블 =======
List<ShopOpen> shopOpens = generateShopOpensAndGetForCreate(request.getOpen(), shop.getId());
shopMapper.createShopOpens(shopOpens);

// ======= shop_category_map 테이블 =======
checkShopCategoriesExistInDatabase(request.getCategory_ids());

List<ShopCategoryMap> shopCategoryMaps = generateShopCategoryMapsAndGet(shop.getId(), request.getCategory_ids());
shopMapper.createShopCategoryMaps(shopCategoryMaps);

// ======= shop_images 테이블 =======
List<ShopImage> shopImages = generateShopImagesAndGet(shop.getId(), request.getImage_urls());
shopMapper.createShopImages(shopImages);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰

  1. 주석으로 코드들을 구분한 것은 좋은데, 주석을 대신하여 별도 메서드로 추출하는 방법도 있을 것 같아요.
  2. Service 계층에서 Presentation 계층인 테이블 정보가 주석에 나와있습니다. Nosql로 DB를 변경하게 된다면 Service 계층의 코드들까지 변경이 일어날 것 같아요.
  3. 메서드 명이 전반적으로 너무 길어 이해하기가 어렵네요 ㅠ

아래와 같은 규칙들을 적용해볼 수 있을 것 같아요

추천 규칙

  1. (외부에서 바라보는) 한 가지 역할만을 명시한다: generateShopImagesAndGet()-> generateShopImages()
  1. 반환 타입, 인자를 이용하여 표현한다.

List<ShopImage> generateFor(Integer shopId, List<String> imageUrls)

(극단적으로 줄인 경우)

위 메서드 명만 있어도
shopId, imageUrls을 통해 List<ShopImage>generate 한다는 걸 알 수 있을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정하겠습니다!

@songsunkook songsunkook marked this pull request as draft May 29, 2023 12:03
@songsunkook
Copy link
Contributor Author

추가적으로 진행한 테스트

  • shop open에 대한 예외처리가 정상적으로 동작하는지 확인

@songsunkook songsunkook marked this pull request as ready for review June 4, 2023 16:07
Comment on lines 135 to 147
boolean hasEmptyTimeInformation = this.open.stream()
.filter(open -> !open.getClosed())
.anyMatch(open -> open.getOpen_time() == null || open.getClose_time() == null);

for (Open open : this.open) {
DayOfWeek dayOfWeek = open.getDay_of_week();
Boolean closed = open.getClosed();
String openTime = open.getOpen_time();
String closeTime = open.getClose_time();

if (!closed) {
if (openTime == null || closeTime == null) {
throw new BaseException(TIME_INFORMATION_IS_REQUIRED_UNLESS_CLOSED);
}
}

dayOfWeeksWithoutDuplication.add(dayOfWeek);
if (hasEmptyTimeInformation) {
throw new BaseException(TIME_INFORMATION_IS_REQUIRED_UNLESS_CLOSED);
}

if (dayOfWeeksWithoutDuplication.size() != 7) {
boolean hasDuplicateDayOfWeek = this.open.stream()
.map(Open::getDay_of_week)
.distinct()
.count() != 7;

Copy link
Contributor

@Invidam Invidam Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
리팩터링 이후 테스트도 다시 된거죠??

@ApiModelProperty(notes = "상점 카테고리 고유 id 리스트 \n" +
"- not null \n" +
"- 최소 1개", example = "[1, 4]", required = true)
"- 최소 1개", required = true)
Copy link
Contributor

@Invidam Invidam Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example 없애구
allowableValues = "1, 2, 3, 4, 5",
추가하니 생기긴하네요!
image

추가로 아예 없애는 것 보단 별도로 설명을 달아놓는 게 좀 더 클라이언트가 이해하기 편리할 것 같아요 ㅎㅎㅎ


List<ShopCategoryMap> existingShopCategoryMaps = shopMapper.getShopCategoryMapsByShopId(existingShop.getId());

// IGNORE에 의하여 (shop_id, shop_category_id)가 중복일 경우는 insert가 무시된다.
Copy link
Contributor

@Invidam Invidam Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createShopCategoryMaps와 createShopImages에 사용된 이유

기존에 IGNORE이 사용된 이유에 대해 찾아봤는데,
카테고리 입력을 예로 들자면
집합의 업데이트에서 기존 카테고리들와 입력한 카테고리들의 교집합을 걸러내기 위해 IGNORE이 필요하네요

예시

기존 카테고리들: {1,2,3,4,5}
입력 카테고리들: {1,2,7,8,9}
교집합: {1,2}

  1. 입력 카테고리들: {1,2,7,8,9} -> createShopCategoryMaps 에 들어옴
  2. {7,8,9} 는 INSERT 처리
  3. {1,2} 는 INSERT 되나 duplicate되어 IGNOORE 처리
  4. 삭제할 카테고리들 : {3,4,5} -> deleteShopImages 로 제거
    image

결과

입력 이전 저장된 카테고리들: {1,2,3,4,5}
입력 이후 저장된 카테고리들: {1,2,7,8,9}

결론

-> IGNORE이 없다면? (3)에서 500에러가 터졌을 것
=> 리스트(집합)의 업데이트시 사용

@Invidam
Copy link
Contributor

Invidam commented Jun 5, 2023

  1. swagger example 수정
  2. 리팩터링 이후 기존 테스트가 진행되었는지 파악
    되면 머지해도 좋을 것 같아요!

@Invidam Invidam linked an issue Jun 6, 2023 that may be closed by this pull request
@songsunkook
Copy link
Contributor Author

songsunkook commented Jun 7, 2023

변경 사항

category_ids 관련 Swagger 문서 설명 추가

다시 진행된 테스트

  • 입력된 데이터가 정확하게 DB에 insert되는지 확인
  • 기존에 동일한 상점명으로 등록된 상점이 존재하더라도 별개의 상점으로써 최초 등록이 이루어지는지 확인
  • shop open에 대한 예외처리가 정상적으로 동작하는지 확인

@Invidam Invidam merged commit 832bae1 into develop Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

상점 생성 API

3 participants