Skip to content
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 : 티켓상품에 옵션 적용하기 #205

Merged
merged 1 commit into from
Jan 28, 2023
Merged

Conversation

kim-wonjin
Copy link
Member

개요

작업사항

  • 옵션을 티켓상품에 적용하는 API 입니다.
  • 헷갈리는 네이밍
    optionGroup == 옵션 질문 (ex. 뒷풀이 참여하십니까, 추천인 이름을 적어주세요)
    option == 옵션 응답 (ex. Y, 김원진)
    itemOptionGroup == 티켓상품-옵션그룹의 매핑엔티티 (적용)
  • 티켓상품쪽 리펙토링 하면서 "/v1/{eventId}/ticketItems/option" 으로 uri 변경 예정

변경로직

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 230 Code Smells

11.9% 11.9% Coverage
1.2% 1.2% Duplication

Copy link
Member

@ImNM ImNM left a comment

Choose a reason for hiding this comment

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

한번 구조에 대해서 고민해주시고 코멘트 남겨주세요!
밤늦게 수고하셨습니다!

Comment on lines +46 to +49
// 티켓상품과 옵션이 해당 이벤트 소속인지 확인
itemOptionGroupService.checkEventId(eventId, ticketItem, optionGroup);
// 이미 적용된 옵션인지 확인
itemOptionGroupService.checkApply(ticketItem, optionGroup);
Copy link
Member

Choose a reason for hiding this comment

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

해당 로직들은 usecase 레이어말고
createItemOptionGroup 메소드 안에 포함시키는게 좋을것 같아유!
검증 메소드 이니깐 ( 위에 권한체크같은건 usecase 안에 있는게 맞는데 )
usecase 가 클라이언트라고 생각하면
저장하다가 빼먹을수 있는? 거기도 하니깐

Copy link
Member Author

Choose a reason for hiding this comment

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

그러네용 규칙검증 로직은 도메인 서비스로 내려보겠습니당!

Comment on lines +6 to +10

public interface ItemOptionGroupRepository extends JpaRepository<ItemOptionGroup, Long> {

Boolean existsByItemIdAndOptionGroupId(Long itemId, Long optionGroupId);
}
Copy link
Member

Choose a reason for hiding this comment

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

ItemOptionGroupRepository 가 필요한 상황인가? 라는 생각도 들긴해요

  • 판매된 티켓이 있으면 옵션의 수정이 불가능 하다.
    라는 비즈니스 규칙이있으니
    티켓의 옵션의 정보변경 또한 어찌보면 티켓 재고관리와 함께 락이 잡혀야 할 수도 있을것 같아요.

그러면 ItemOptionGroupRepository 를 없애고
TicketItem을 aggregate root 로 만들고
cascade 옵션줘서 아이템 옵션 그룹을 관리한다면,
저장하기도 쉽고 변경사항 체크하기도 쉽고

티켓 아이템만이 아이템 옵션그룹을 탐색 할 수 있는 구조로 만들면 좀더 편리하게 관리 할 수 있지 않을까 생각해요

현 상황으로는 ItemOptionGroupRepository 로 아이템 옵션 그룹 을 조회하는 비즈니스가 없으니

마치 경민형쪽에서 호스트-호스트구성원 을 묶어서 관리하듯이?

Copy link
Member

Choose a reason for hiding this comment

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

위처럼 TicketItem 에서 아이템옵션그룹을 관리한다면
분산락을 잡기에도 훨씬 수월해질것같아요
TicketItem에 락을걸지
ItemOptionGroup에 락을걸진 않을꺼니깐!

Comment on lines +21 to +29

public void checkEventId(Long eventId, TicketItem ticketItem, OptionGroup optionGroup) {
if (!ticketItem.getEvent().getId().equals(eventId)) {
throw InvalidTicketItemException.EXCEPTION;
}
if (!optionGroup.getEvent().getId().equals(eventId)) {
throw InvalidOptionGroupException.EXCEPTION;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

이런 로직들이 ItemOptionGroupRepository 를 없애고
티켓 아이템 쪽에서 옵션을 관리하는 형태면
한데 묶여서 좀더 관리하기 편해질것 같아요

Copy link
Member

Choose a reason for hiding this comment

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

찬진님 말처럼 티켓 도메인에서 검증을 충분히 할 수 있을 거 같아요
Host <-> HostUser 에 비슷하게 구현해놨는데 참고하시면 좋을 거 같습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

호스트 쪽 참고해서 좀 더 편리하게 구조 수정해보겠습니다!

Copy link
Member

@sanbonai06 sanbonai06 left a comment

Choose a reason for hiding this comment

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

찬진님 말대로 usecase에 있는 규칙 검증 로직은 도메인 서비스로 내리는게 더 좋을 거 같아요! 수고하셨습니다. LGTM ⭐

Copy link
Member

@gengminy gengminy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines +35 to +42

@Operation(summary = "옵션을 티켓상품에 적용합니다.")
@PostMapping("/{eventId}/option")
public ApplyTicketOptionResponse createTicketOption(
@RequestBody @Valid ApplyTicketOptionRequest applyTicketOptionRequest,
@PathVariable Long eventId) {
return applyTicketOptionUseCase.execute(applyTicketOptionRequest, eventId);
}
Copy link
Member

Choose a reason for hiding this comment

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

티켓 아이템 관련 컨트롤러니까 RESTful 하게 짜려면 @PathVariable 에다가 ticketOptionId 를 주고 eventId 를 request DTO 안에 넣는게 좋을 거 같아용

Copy link
Member Author

Choose a reason for hiding this comment

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

후에 {eventId}/ticketItems/option 으로 수정할 생각이였습니당
티켓상품에 접근하는 경로에 속해있는 이벤트를 포함시키는 것이 좋겠다고 판단했는데,
이 경우에 ticketOptionId도 PathVariable로 주는게 좋을까요?

Copy link
Member

Choose a reason for hiding this comment

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

굳이 필요하지 않다면 빼고, eventId 를 requestDTO 에 넣는 방식이 좋을 거 같네용

Comment on lines +13 to +15
@NotNull
@Schema(nullable = false, example = "1")
private Long ticketItemId;
Copy link
Member

Choose a reason for hiding this comment

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

Long 은 @Positive 로 검증하는게 어떨까요?

Comment on lines +39 to +41
// 권한 체크 ( 해당 이벤트의 호스트인지 )
host.hasHostUserId(user.getId());

Copy link
Member

Choose a reason for hiding this comment

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

hostService.validateHostUser() 사용해주시면 감사하겠습니다

Comment on lines +21 to +29

public void checkEventId(Long eventId, TicketItem ticketItem, OptionGroup optionGroup) {
if (!ticketItem.getEvent().getId().equals(eventId)) {
throw InvalidTicketItemException.EXCEPTION;
}
if (!optionGroup.getEvent().getId().equals(eventId)) {
throw InvalidOptionGroupException.EXCEPTION;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

찬진님 말처럼 티켓 도메인에서 검증을 충분히 할 수 있을 거 같아요
Host <-> HostUser 에 비슷하게 구현해놨는데 참고하시면 좋을 거 같습니다

@kim-wonjin kim-wonjin merged commit 6f22452 into dev Jan 28, 2023
@kim-wonjin kim-wonjin deleted the feature/192-apply-option branch January 28, 2023 06:43
@kim-wonjin kim-wonjin changed the title feat(itemOptionGroup) : 티켓상품에 옵션 적용하기 feat : 티켓상품에 옵션 적용하기 Jan 30, 2023
@kim-wonjin kim-wonjin added For: API [이슈 대상] 외부 API Type: Feature [이슈 목적] 새로운 기능 추가 labels Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For: API [이슈 대상] 외부 API Type: Feature [이슈 목적] 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 [feature] 티켓상품에 옵션그룹 적용하기
4 participants