-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/seat reservation and query test + 예약 결제 구현 #9
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
base: main
Are you sure you want to change the base?
Conversation
…eat eventId 컬럼 및 Event 엔티티 설저 변경 ReservationBookingRequest 생성자 패턴 builder 로 통일
SeatService - createValidSeat 테스트 Foreign Key 검증을 위한 ForeignKeyValidator 생성
SeatService 에 seat 저장시 유효성 검증 추가
| if (this.status == null) { | ||
| this.status = SeatStatus.AVAILABLE; | ||
| @Getter | ||
| public static class InvalidEventForeignKeyException extends RuntimeException { |
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.
Exception들은 따로 분리하는게 어떨까요??
또한, BookingException 과 같은 서비스용 exception을 만들어서 사용하면 좋을것 같아요
| import personnel.jupitorsendsme.pulseticket.entity.Seat; | ||
|
|
||
| @Getter | ||
| public class InvalidStatusException extends RuntimeException { |
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.
보통 BookingException 은 서비스용 exception을 만들고
new BookingException(ResponseStatus.INVALID_STATUS, HttpStatus.BAD_REQUEST)와 같이 쓰면 enum으로 에러 코드도 명시하고, 알맞은 응답 코드를 반환할 수 있어 좋습니다~
| public UserNotFoundException(String loginId) { | ||
| super("사용자 조회 안됨 : " + loginId); | ||
| this.loginId = loginId; | ||
| public UserNotFoundException(ReservationRequest request) { |
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.
오 매우 객체지향적!
|
|
||
| @Query("SELECT r FROM Reservation r " | ||
| + "LEFT JOIN FETCH r.event " | ||
| + "LEFT JOIN FETCH r.seat " |
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.
왜 jpql을 사용하셨나요? +_+ (두근)
| public class EventManagementService { | ||
|
|
||
| // TODO : 테스트, 예외 처리 필요 -> event, request 유효한지 | ||
| boolean isPaymentAmountEnough(Event event, ReservationRequest request) { |
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.
public 으로 만들면 어떨까요??
@Transactional 은 public 에만 걸리는걸로 알고있어요~
| switch (this.status) { | ||
| case AVAILABLE -> throw new IllegalStateException("예약 가능한 좌석 상태. 예약이 되고 나서 결제해야 함"); | ||
| case SOLD -> throw new IllegalStateException("이미 판매됨"); | ||
| public void proceed() { |
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.
reserve, sold 와 같은 명시적인 이름에서 범용적인 이름인 proceed 로 변경된 이유가 궁금해요~
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| @Transactional(readOnly = true) |
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.
음.. 역시 @Transaction 과 @Transaction(readOnly = true) 가 함께 사용될 땐
각각 메소드에 annotation을 붙이는게 좋은것 같아요.
왜냐하면 어디는 있고, 어디는 없어서 일관성이 좀 깨지는것 같아요.
사실 클래스에 @Transactional 을 붙여서 잘 사용하진 않았던거 같아요.
| * @return Textual Diagram <br> | ||
| */ | ||
| public List<SeatStatusResponse> statusOfSeatsOfTheEvent(ReservationRequest request) { | ||
| List<Seat> seats = seatRepository.findByEvent_Id(request.getEventId()); |
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.
결과가 없으면 어떻게 되나요??
| public Seat getAvailableSeat(ReservationRequest request) { | ||
| Seat seat = getSeat(request); | ||
|
|
||
| if (seat.getStatus() != Seat.SeatStatus.AVAILABLE) |
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.
seat.isUnavailable() 과 같이 주요 로직이 model로 들어가면 더 객체지향적인 코드가 되고,
테스트를 작성하기에더 좋은것 같아요
| () -> new Seat.InvalidEventForeignKeyException(seat)); | ||
|
|
||
| Integer seatNumber = seat.getSeatNumber(); | ||
| if (seatNumber == null || seatNumber < 1 || seatNumber > event.getTotalSeats()) { |
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.
seat.isOutOfRange() 와 같은 함수를 만들면 더 가독성이 좋을것 같은데 어떻게 생각하시나요~?
| */ | ||
| @Transactional | ||
| public Seat createSeat(Seat seat) { | ||
| Seat created = null; |
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.
갑자기..??
| throw new SeatNumberDuplicateException(seat); | ||
| } | ||
|
|
||
| created = seatRepository.save(seat); |
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.
return seatRepository.save(seat);와 같이 작성하면 불필요한 변수를 제거할 수 있을것 같아요.
| @Import({PayManagementService.class, HashingServiceArgon2id.class, ReservationQueryService.class, | ||
| UserManagementService.class, EventManagementService.class, TestEntityManager.class}) | ||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| public class PayManagementServiceTest { |
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.
여기에 @Transactional 을 붙이면 어떻게 될까요??
| @RequiredArgsConstructor(onConstructor_ = @Autowired) | ||
| @Import({PayManagementService.class, HashingServiceArgon2id.class, ReservationQueryService.class, | ||
| UserManagementService.class, EventManagementService.class, TestEntityManager.class}) | ||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) |
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.
요건 무엇을 하는걸까요??
| * 예약 결제 - 유효하지 않는 사용자 | ||
| */ | ||
| @Test | ||
| void payReservation_userNotFound() { |
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.
@DisplayName("유효하지 않은 사용자가 예약 결제를 진행하면 UserNotFoundException를 반환한다")와 같이
@DisplayName을 사용하여 무엇을 테스트하는지 명시하면 더 좋을것 같아요.- "xxx이면 yyy를 반환한다." 와 같이 더 구체적으로 작성하면 좋겠어요
| } | ||
|
|
||
| /** | ||
| * 예약 결제 기본 테스트 |
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.
위의 리뷰 를 같이 반영하면 좋겠어요
| private final SeatRepository seatRepository; | ||
|
|
||
| /** | ||
| * 예약 테스트 |
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.
예약이 성공하면 어떻게 되는지 설명이 구체적으로 추가되면 좋겠어요~
|
테스트 코드가 생겼네요!! |
db_creation_query.sql indent 조정 @ModelAttribute 어노테이션 삭제 Seat->Event f key 위반 Exception 위치 변경
모든 Exception 에 적당한 HttpStatus 설정
그리고, F key 관계를 가지는 엔티티에 대해서 F key constraint 가 생성되지 않도록 설정 (foreignKey = @foreignkey(ConstraintMode.NO_CONSTRAINT)
메서드에 public modifier 추가, @transactional annotation 을 메서드로 위치 변경
|



Test Code