-
Notifications
You must be signed in to change notification settings - Fork 0
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] #259 - Booking 엔티티 수정 및 환불/취소 요청 API 구현 #260
Changes from 19 commits
e56a150
d5d9445
42f9920
c3bb5a5
5826f62
630bcee
c12fc4a
ba35326
0a20268
fd90186
cae8909
3bf6f39
fffafcb
3a8d5be
7569aac
b7ea086
b108620
45f722a
689e8d5
6b18618
8578b82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
package com.beat.domain.booking.application; | ||
|
||
import org.springframework.stereotype.Service; | ||
|
||
import com.beat.domain.booking.application.dto.BookingCancelRequest; | ||
import com.beat.domain.booking.application.dto.BookingCancelResponse; | ||
import com.beat.domain.booking.application.dto.BookingRefundRequest; | ||
import com.beat.domain.booking.application.dto.BookingRefundResponse; | ||
import com.beat.domain.booking.dao.BookingRepository; | ||
import com.beat.domain.booking.domain.Booking; | ||
import com.beat.domain.booking.domain.BookingStatus; | ||
import com.beat.domain.booking.exception.BookingErrorCode; | ||
import com.beat.domain.schedule.dao.ScheduleRepository; | ||
import com.beat.domain.schedule.domain.Schedule; | ||
import com.beat.global.common.exception.NotFoundException; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class BookingCancelService { | ||
|
||
private final BookingRepository bookingRepository; | ||
private final ScheduleRepository scheduleRepository; | ||
|
||
public BookingRefundResponse refundBooking(BookingRefundRequest request) { | ||
Booking booking = bookingRepository.findById(request.bookingId()) | ||
.orElseThrow(() -> new NotFoundException(BookingErrorCode.NO_BOOKING_FOUND)); | ||
|
||
booking.setRefundInfo(request.bankName(), request.accountNumber(), request.accountHolder()); | ||
booking.setBookingStatus(BookingStatus.REFUND_REQUESTED); | ||
bookingRepository.save(booking); | ||
|
||
return BookingRefundResponse.of( | ||
booking.getId(), | ||
booking.getBookingStatus(), | ||
booking.getBankName(), | ||
booking.getAccountNumber(), | ||
booking.getAccountHolder() | ||
); | ||
Comment on lines
+34
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 메서드의 인자로 필요한 매개변수 대신 객체를 넘겨주고 해당 getter 들을 DTO에서 처리하는거에 대해서는 어떻게 생각하시나요?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 개인적으로 dto는 데이터 전달에만 집중해야한다고 생각해 get을 dto에서 하는 것이 적합한 것 같지 않습니다..!! response는 클라이언트와 controller 사이의 dto이므로 response에 도메인쪽 import를 추가하지 않는 게 낫지 않을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 혜린님 말씀대로 "dto는 데이터 전달에만 집중해야한다!" 라는 말에 동의합니다. 제가 말씀드렸던 이유는 getter zone이 Service 계층에서 발생하는게 객체지향적이지 못한 코드라고 생각이 들었습니다. 도메인 객체의 내부 상태를 서비스 계층에서 과도하게 노출하는 게 아닌가? 라는 생각이 들기도 합니다. 그렇다면 getter 없이 DTO가 데이터 전달에만 집중해서 사용하는 방법에는 어떤게 있을 지 찾아봤는데, MapStruct나 커스텀 매퍼 클래스를 사용하는 방법이 있더라고요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 최근 이 부분에 대해서 공부하는 중입니다!
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 공부해서 추후 리팩토링 하면 좋을 것 같네요 😊 |
||
} | ||
|
||
public BookingCancelResponse cancelBooking(BookingCancelRequest request) { | ||
Booking booking = bookingRepository.findById(request.bookingId()) | ||
.orElseThrow(() -> new NotFoundException(BookingErrorCode.NO_BOOKING_FOUND)); | ||
|
||
booking.setBookingStatus(BookingStatus.BOOKING_CANCELLED); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. setter를 호출하는 대신 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 상단 답변 코멘트와 마찬가지로 setter 호출이 아닌 엔티티에 직접 작성한 메소드를 호출해서 사용했습니다! |
||
bookingRepository.save(booking); | ||
|
||
Schedule schedule = booking.getSchedule(); | ||
schedule.decreaseSoldTicketCount(booking.getPurchaseTicketCount()); | ||
scheduleRepository.save(schedule); | ||
|
||
return BookingCancelResponse.of( | ||
booking.getId(), | ||
booking.getBookingStatus() | ||
Comment on lines
+54
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 매개변수 개수가 2개 밖에 없어서 매개변수를 넘겨도 될 것 같긴하나, 만약 위에서 booking 객체를 넘기는 방식으로 바꾼다면 통일성을 위해 해당 부분도 바꾸는게 좋아보이는데 혜린님 생각은 어떠신가요? |
||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
package com.beat.domain.booking.application; | ||
|
||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
import com.beat.domain.booking.application.dto.GuestBookingRequest; | ||
import com.beat.domain.booking.application.dto.GuestBookingResponse; | ||
import com.beat.domain.booking.dao.BookingRepository; | ||
|
@@ -15,9 +18,6 @@ | |
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
|
||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Slf4j | ||
@Service | ||
@RequiredArgsConstructor | ||
|
@@ -61,6 +61,9 @@ public GuestBookingResponse createGuestBooking(GuestBookingRequest guestBookingR | |
guestBookingRequest.bookingStatus(), | ||
guestBookingRequest.birthDate(), | ||
guestBookingRequest.password(), | ||
null, | ||
null, | ||
null, | ||
Comment on lines
+64
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. null 처리하신 이유가 궁금합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 환불계좌 정보는 환불요청했을 시에만 예매자가 등록하게 되기 때문에 처음 booking을 생성할 때는 null로 들어가게 됩니다. |
||
schedule, | ||
users | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package com.beat.domain.booking.application.dto; | ||
|
||
public record BookingCancelRequest( | ||
long bookingId | ||
) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package com.beat.domain.booking.application.dto; | ||
|
||
import com.beat.domain.booking.domain.BookingStatus; | ||
|
||
public record BookingCancelResponse( | ||
long bookingId, | ||
BookingStatus bookingStatus | ||
) { | ||
public static BookingCancelResponse of( | ||
long bookingId, | ||
BookingStatus bookingStatus) { | ||
return new BookingCancelResponse( | ||
bookingId, | ||
bookingStatus); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package com.beat.domain.booking.application.dto; | ||
|
||
import com.beat.domain.performance.domain.BankName; | ||
|
||
public record BookingRefundRequest( | ||
long bookingId, | ||
BankName bankName, | ||
String accountNumber, | ||
String accountHolder | ||
) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package com.beat.domain.booking.application.dto; | ||
|
||
import com.beat.domain.booking.domain.BookingStatus; | ||
import com.beat.domain.performance.domain.BankName; | ||
|
||
public record BookingRefundResponse( | ||
long bookingId, | ||
BookingStatus bookingStatus, | ||
BankName bankName, | ||
String accountNumber, | ||
String accountHolder | ||
) { | ||
public static BookingRefundResponse of( | ||
long bookingId, | ||
BookingStatus bookingStatus, | ||
BankName bankName, | ||
String accountNumber, | ||
String accountHolder) { | ||
return new BookingRefundResponse( | ||
bookingId, | ||
bookingStatus, | ||
bankName, | ||
accountNumber, | ||
accountHolder); | ||
} | ||
} |
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.
해당 부분 setter를 호출하지 않고 Booking 엔티티에서 refund라는 도메인 메서드에서 해당 비즈니스 로직을 적고. booking.refund() 형식으로 외부에서 호출하는 것이 나아보입니다!
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.
setter 호출이 아닌 엔티티에 직접 작성한 메소드를 호출했습니다!
Booking.java
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.
비즈니스 로직을 담고 있더라도 네이밍을 setter로 짓는것은 수정자로 오해를 줄 수 있을 것 같습니다.
setRefundInfo
와setBookingStatus
모두에 해당하는 말입니다!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.
해당 메서드들을
refund()
라는 도메인 비즈니스 메서드에서 처리를 하도록 하는게 어떨까요?그리고 해당 메서드에 대한 예외 처는 도메인 비즈니스 메서드에서 해주면 될 것 같습니다.
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.
네이밍 바꾸는 게 더 좋겠네요! 최신 pr에 반영하도록 하겠습니다~