-
Notifications
You must be signed in to change notification settings - Fork 3
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: 거래 내역은 경매 식별자, 구매(입찰) 가격, 구매 수량, 거래 유형(입찰, 환불), 구매자, 판매자, 생성일자를 포함한다. #178
Conversation
- ZonedDateTime을 지원하지 않아 `PrePersist`, `PreUpdate`를 통해 값을 지정한다. - BidHistory에서 명시적으로 prePersists를 호출한다. (JPA가 존재하는 경우 해당 작업이 자동으로 동작함)
- ZonedDateTime 형식으로 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.
JPA를 쓰게 되어도 도메인 엔티디에서는 의존성을 제거하는 것이 좋다고 생각합니다.
import lombok.Builder; | ||
import lombok.Getter; | ||
|
||
@Getter | ||
public class BidHistory { | ||
public class BidHistory extends BaseTimeEntity { |
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.
시간 관련 정보를 확장해서 사용하는 것은 너무 좋은 것 같아요!
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.
좋은 의견 감사합니당!
한가지 궁금한 점이 있는데 현재 도메인 Entity가 영속성 맵핑도 함께 담당하게 될 것이라고 이해하고 있었는데요.
현재 저희가 활용하는 도메인 엔티티는 순수하게 남겨두고, 이후에 JPA Entity가 추가로 생성되고 여기에서만 JPA 기술 관련 의존을 해야한다는 느낌으로 이해하는게 맞을까요? 아니면 다른 생각이신지 궁금합니다!!
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.
현재 저희가 활용하는 도메인 엔티티는 순수하게 남겨두고, 이후에 JPA Entity가 추가로 생성되고 여기에서만 JPA 기술 관련 의존을 해야한다는 느낌
저는 이런 방향을 생각하고 있었습니다. 왜냐하면 jpa와 매핑이 되는 순간 데이터 중심적으로 도메인을 설계가 되어서 확장에 제약이 생길 수 있다고 생각을 했기 때문입니다.
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.
도메인, 엔티티를 분리한다면 엔티티에 BaseTimeEntity를 상속받게 하는게 맞을것 같아요!
- 추가로 ZonedDateTime은 JPA AUTO Auditing기능을 사용하는데 제약이 있어서 LocalDateTime으로 변경해야 할 수 도..?
- 시간 Auditing을 하는 다른 방식으로 BaseTimeEntity상속 외에도 임베디드 객체로 정의해도 가능하답니당~
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.
라이브이야기: 도메인 Entity, 영속성 Entity를 분리하는 방향으로! 좋습니다!
BidStatus bidStatus, | ||
long auctionId, | ||
Member seller, | ||
Member buyer | ||
Member buyer, | ||
ZonedDateTime createdAt |
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.
BidHistory가 DB에 저장될 때 입찰과 환불이 따로 다른 레코드로 저장되는 것이 아니라,
환불하면 기존 입찰 내역에서 상태만 변경하도록 설계를 해놨읍니다... 미리 얘기했어야 했는데 죄삼돠...
[할 말 세줄 요약]
- 여기에 updatedAt(환불시점)이 추가되기
- or BidHistory DB가 입찰 환불 내역을 다 가지고 있기
- BidHistory 상태가 입찰, 환불에 큰 영향이 없어서 둘다 무리 없을 것 같습니다~
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.
아하 이해했습니다~~ 감사합니당~
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.
- 거래 입찰 시
- CreatedAt이 현재 시간으로 생성된다.
- UpdatedAt이 현재 시간으로 생성된다.
- 거래 환불 시
- UpdatedAt이 현재 시간으로 갱신된다.
- 거래 이력 상태가 입찰에서 환불로 변경된다.
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.
맞습니다!
- createdAt, updatedAt 필드를 추가한다. - BaseTimeEntity를 제거한다.
- BidHistory.markAsRefund를 통해 환불 상태로 변경을 요청한다. - 이미 환불 상태인 경우 P003 에러가 발생한다.
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.
역시는 역시 코드가 너무 보기 편해요! 😊
질문 하나 있어서 남기고 갑니당
Member savedBuyer = memberRepository.save(buyer); | ||
Member savedSeller = memberRepository.save(seller); | ||
bidHistoryRepository.save(BidHistory.builder() | ||
.id(refundTargetBidHistory.getId()) | ||
.auctionId(refundTargetBidHistory.getAuctionId()) | ||
.productName(refundTargetBidHistory.getProductName()) | ||
.price(price) | ||
.quantity(quantity) | ||
.bidStatus(BidStatus.REFUND) | ||
.seller(savedSeller) | ||
.buyer(savedBuyer) | ||
.build()); | ||
bidHistoryRepository.save(refundTargetBidHistory); // 정상적으로 환불 처리된 경우 해당 이력을 '환불' 상태로 변경 |
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.
와 생각해보니 변경만 하면 굳이 빌더 안써도 되는군요... 깔끔하네요! 좋습니다
질문 : Buyer와 Seller의 포인트 수정내역 저장을 지워도 되는건가요?
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를 이용해서 테스트를 진행할 때, 테스트에 활용하던 Buyer와 Seller 객체가 그대로 메모리에 남아있어 테스트로는 확인이 안되던 문제였습니다. (메모리 상에서는 포인트가 변경되었지만, 실제 영속적으로 반영되지는 않음)
별도의 Task로 분리하겠습니다..!
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.
제 더러운 코드를 청소해주셔서 더 감사합니다! 😊
- BidHistory.markAsRefund를 통해 환불 상태로 변경을 요청한다. - 환불 표시에 성공한 경우 UpdatedAt 시간을 변경한다. - 이미 환불 상태인 경우 P003 에러가 발생한다. - UpdatedAt이 CreatedAt보다 이전인 경우 B006 에러가 발생한다.
변경 내용
|
질문: 요 부분은 JPA를 사용하면 자동으로 처리해줄것 같은데 직접 처리하신 이유가 있나요?! |
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.
👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍
마음에 들어요!
👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍
if (bidStatus.equals(BidStatus.REFUND)) { | ||
throw new BadRequestException("이미 환불된 입찰 내역입니다.", ErrorCode.B005); | ||
} |
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 하는 것은 어떨까요?
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.
좋은 의견입니다! 사실 '거래 내역' 입장에서는 자신의 상태가 환불됨
-> 환불됨
으로 변경된다고 해서 예외로 인식하지 않아도 될 것 같아요!
하지만 해당 로직이 PaymentService에서 사용되던 것을 BidHistory 내부로 이전한 것이라, PaymentService와 PaymentServiceTest 등에도 영향이 있어, 별도의 태스크를 생성하고 그곳에서 처리하는 것이 좋을 것 같습니다..!!
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.
어떤 이름의 이슈를 생성할지 모르겠네요. 일단 Priority2의 리펙터링 이슈를 생성할께요
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.
현재는 JPA 사용을 전혀 고려하지 않은 코드이기 때문에 직접 처리를 진행해줬습니다! 경매가 환불 될 때, 거래 내역의 업데이트 시간이 수정되야하는 요구 사항이 있어 이를 반영하기 위해 작성했는데용 추후에 JPA를 통해 자동 생성되는 값이랑 시간 차이가 발생할 것 같아, 해당 시점에 고민해보면 좋을 것 같습니다.! |
생각하지 못한 부분이었는데 살펴보니 updateAt이 도메인 로직에서 중요하지 않다면 제거해도 괜찮을 것 같은데요? |
그럼 UpdateAt 갱신하는 메서드를 제거하는 걸로?! 하겠습니다! |
필드 제거하는 건 어떤가요? |
필드는 사용자에게 전달해줘야해서 어딘가에 저장은 해둬야 할 것 같아요! |
좋아요! |
- setUpdatedAt 메서드를 제거한다.
BidHistory에서 업데이트시간 관리 안함!! 찐막!!! |
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.
진짜 고생많았어요!!!! :)
🥇 🥇 🥇 🥇 🥇 🥇 🥇 🥇 🥇 🥇 🥇 🥇 🥇
📄 Summary
거래 내역에서 거래 일자 정보를 확인할 수 없어, 해당 필드를 추가한 작업입니다!
BidHistoryInfo
수정BidHistory
수정BaseTimeEntity
추가하고 이를 상속받는다.🙋🏻 More
BidHistory
내부에서 명시적으로 BaseTimeEntity의생성 시간 기록 로직
을 실행하도록 했는데 좀 이상한 것 같아요 ^^;;close #111