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
[#326] 후원 및 환불 api 리팩토링 및 구현 #327
Conversation
오우 이해하는데 힘들었따.. 일단 지금 코드는 안보고 파즈가 작성한 글만 읽었어 고민해 봤는데 donation에 from, to 같은 인자를 만들어서 form이 후원자, to가 창작자 아이디 들가게 하는 건 어때 내일 아침에 일어나서 또 코드리뷰 할게!! |
오호 나는 그 생각에 대해서 |
Donation이 Member를 2개 가지고 있을 수 없으니 안되지! |
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.
바뀐 도메인 로직 이해하는 식으로 코드리뷰 했어..!!
테스트 짤 때 좀 참고하려고!!
변경된 도네이션 로직
- Login된 멤버만 후원을 할 수 있다.
- donator가 후원할 충분한 금액이 있는지 체크한다.
- validateEnoughPoint
- donator의 nickname이 message의 name이 된다.
->>>>>>>> 여기 보니까 creator에 donation을 넣어주고 있는데, donation이 creator하고, donator를 가지고 있는게 더 좋은지 얘기해 보자
변경 된Payment 관련 로직
-
createPayment -사용자가 충전할 item을 설정하고 우리는 Payment 새 엔티티를 만들어 저장 하는 결제 준비 단계
- 충전은 login상태에서 가능하다.
- ItemId를 통해 구매 될 Item을 찾는다. (충전금액)
-
변경 된 환불 변경 로직
- 환불러한테 환불하기 충분한 금액이 있는지 확인한다. (이번에 추가)
- isAfterRefundGuaranteeDuration 메소드 추가 (7일 체크)
@@ -0,0 +1,5 @@ | |||
package com.example.tyfserver.donation.domain; | |||
|
|||
public enum DonationType { |
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.
Donation이 창작자, 후원자로 바뀌게 한다면 얘는 필요 없어지겠다
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.
응 맞어! 그리고 지금 코드도 이 enum을 사용하고 있지는 않아 미리 만들어뒀어! 그리고 Donation 엔티티 필드에 DonationStatus 가 아직 있는데 이것도 삭제해야하는데 아직 삭제안했업
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.
말 나온김에 DonationStatus Donation 필드에서 빼놔야겠다
@@ -28,4 +28,8 @@ public void reduce(final long amount) { | |||
} | |||
this.point -= amount; | |||
} | |||
|
|||
public boolean lessThan(Long point) { |
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.
근데 payment도메인의 reduce 함수에서 "포인트가 총액보다 적게 있습니다" 예외가 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.
오 땡큐 이거 예외 만들어놓앗는데 여기다가 적용안했네 굿
private boolean bankRegistered; | ||
|
||
public MemberResponse(Member member) { | ||
this(member.getEmail(), member.getNickname(), member.getPageName(), | ||
member.getBio(), member.getProfileImage(), isBankRegistered(member)); | ||
member.getBio(), member.getProfileImage(), member.getAvailablePoint(), isBankRegistered(member)); |
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.
MemberResponse에 AvailablePoint만 주는 이유는
members/me/point API가 있어서 겠쥬?
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.
이 API는 현재 보유 포인트 달라고 했어서 이거 준거야!
그때 얘기하기로 도네받은 포인트는 그 정산탭 가면 보이니깐 어쩌고 저쩌고 했던걸로 알아서!
@@ -166,4 +168,12 @@ public boolean isRefundBlocked() { | |||
public boolean isNotPaid() { | |||
return status != PaymentStatus.PAID; | |||
} | |||
|
|||
public boolean isAfterRefundGuaranteeDuration() { | |||
return LocalDateTime.now().isAfter(getCreatedAt().plusDays(7)); |
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.
환불 비즈니스 로직
- 7일 이전 AND 환불러가 환불요청한 금액보다 많은 포인트를 가질 경우
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.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 수리 꼼꼼해~~ 👍
이 부분은 LocalDateTime.now()를 now
변수로 빼도 좋을 것 같네 😃
return LocalDateTime.now().isAfter(getCreatedAt().plusDays(7)); | |
LocalDateTime now = LocalDateTime.now(); | |
return now.isAfter(getCreatedAt().plusDays(7)); |
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 LocalDateTime.now
.isAfter(getCreatedAt().plusDays(7));
는 어때 ?
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.
줄바꿈 하는 것도 나쁘지 않네 이정도면 난 OK 👌
private String nickname; | ||
private String pageName; | ||
@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy'/'MM'/'dd'/' HH:mm:ss", timezone = "Asia/Seoul") | ||
private LocalDateTime 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.
우리 그때 날짜 형식도 협의했었나???
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.
이거는 로키 조이가 작업해놨던거 재사용한거얌. 이걸 해준 이유는 LocalDateTime
은 직렬화 안돼서 저렇게 해줬을거야 ..?
https://jojoldu.tistory.com/361 관련 내용 링크야
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.
따로 협의한건 없는데, 그 전에 LocalDateTime을 포맷팅하지않고 바로 보냈을 때, 형식이 별로라고해서 일반적인 형식(?)으로 변경했었어
return ResponseEntity.status(HttpStatus.CREATED) | ||
.body(paymentService.completePayment(paymentCompleteRequest)); | ||
} | ||
|
||
@PostMapping("/refund/verification/ready") | ||
public ResponseEntity<RefundVerificationReadyResponse> refundVerificationReady(@Valid @RequestBody RefundVerificationReadyRequest verificationReadyRequest, BindingResult result, | ||
LoginMember loginMember) { |
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.
여기서 LoginMember를 사용하지 않는데,
인터셉터에 걸리게만 해놓는건 어때?
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.
이게... 처음에 토큰 필요한 애들이니깐 미리 LoginMember 써놔야지~! 했다가 안쓰는 애들이 많겠구나 싶어서 놔둔거였어 !! 테스트코드하면서 필요하면 추가하고 안쓰면 빼고 해주면 좋을 것 같어!
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.
지금은 안쓰지만, 앞으로 변경 될 후원로직을 생각하면 LoginMember
가 필요할거 같아서 굳이 안지워도 될 것 같어!
혹시라도 안쓴다면 나중에 제거해도 될 부분인거 같어 🤔
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 2개를 가지고 있을 수 없어서 String 이나 int로 후원자와 창작자에 대한 한 쪽의 정보를 두자고 한거였어~! |
내가 말한 형식으로 하려면 아래처럼 하면 되는거아니야? 좀 더 생각의 흐름을 보충하자면 테이블로 생각해봤어!!!!! 그래서 Member는 자기 id가 걸린 Donation을 땡겨 올 수 있음 Dontation에
Member에
|
당연히 안된다고 생각했는데 혹시 몰라서 프로젝트 따로파서 했는데 되는줄 알고 식겁했네 안된다~! 그냥 단순히 DB와의 연관관계라고 생각하고 보면 돼. 객체의 관점에서 수리의 말을 보면 +) 아 어떤식으로 안됐냐면, 테이블 자체는 만들어졌어! DB 관점에서는 |
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 ResponseEntity.status(HttpStatus.CREATED) | ||
.body(paymentService.completePayment(paymentCompleteRequest)); | ||
} | ||
|
||
@PostMapping("/refund/verification/ready") | ||
public ResponseEntity<RefundVerificationReadyResponse> refundVerificationReady(@Valid @RequestBody RefundVerificationReadyRequest verificationReadyRequest, BindingResult result, | ||
LoginMember loginMember) { |
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.
지금은 안쓰지만, 앞으로 변경 될 후원로직을 생각하면 LoginMember
가 필요할거 같아서 굳이 안지워도 될 것 같어!
혹시라도 안쓴다면 나중에 제거해도 될 부분인거 같어 🤔
server/src/main/java/com/example/tyfserver/payment/domain/Item.java
Outdated
Show resolved
Hide resolved
@@ -166,4 +168,12 @@ public boolean isRefundBlocked() { | |||
public boolean isNotPaid() { | |||
return status != PaymentStatus.PAID; | |||
} | |||
|
|||
public boolean isAfterRefundGuaranteeDuration() { | |||
return LocalDateTime.now().isAfter(getCreatedAt().plusDays(7)); |
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.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 수리 꼼꼼해~~ 👍
이 부분은 LocalDateTime.now()를 now
변수로 빼도 좋을 것 같네 😃
return LocalDateTime.now().isAfter(getCreatedAt().plusDays(7)); | |
LocalDateTime now = LocalDateTime.now(); | |
return now.isAfter(getCreatedAt().plusDays(7)); |
return LocalDateTime.now().isAfter(getCreatedAt().plusDays(7)); | ||
} | ||
|
||
public void validateMemberHasEnoughPoint() { |
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.
환불을 하기전에 포인트가 충분히 있는건지 확인하는 로직인 것 같은데, Payment가 member를 필드로 가지다보니까, 내부에서 member.validateEnoughPoint(point);
해버리니까 이게 무슨 기능인지 조금 헷갈리네 🤔
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.
응 네이밍을 조금 변경하면 되지 않을까 싶은데 음... validateMemberHasRefundablePoint()
(이게 좋은지는 잘 모르겠는데) 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.
오 맞어 Payment
에서 환불관련해서 저 메서드를 호출하는건데 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.
일단 적용해둘게~
private String nickname; | ||
private String pageName; | ||
@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy'/'MM'/'dd'/' HH:mm:ss", timezone = "Asia/Seoul") | ||
private LocalDateTime 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.
따로 협의한건 없는데, 그 전에 LocalDateTime을 포맷팅하지않고 바로 보냈을 때, 형식이 별로라고해서 일반적인 형식(?)으로 변경했었어
나도 궁금해서 직접 해보면서 이해할려고 테스트 짜봤는데 아니 왜 난 되냐구ㅜㅜ 대략적인 코드
테스트 코드
ps. |
JPA 책 몇쪽이야? |
책은 JPA 단원 마지막에 실습예제 한 거야!! 글구 마침 파즈가 얘기한 것 처럼 테스트해보고 있었는데 통과해!
|
Co-authored-by: Gyeonglok Kim <goodboy302@naver.com>
이거는 보니깐 중간테이블 낀 형태같은데 |
엇 JoinColumn 지정안하면 테이블 생성되는 그건가!!! 일단 Purchasing, Member, Delivery 코드 올릴게!!
|
getTest해도 이렇게 부르는거 맞나?
혹시 몰라서 test get하는거 말고 주석처리해쓰
|
…oowacourse-teams/2021-tyf into feature/chargeRefundReorganize
그건 아마 그때 딱 쿼리 찍어서 그대로 반환해줘서 그런 것 같어~! 원래대로라면 객체안에 집어넣고 그 값을 끄집어 내야하는데 |
파즈 답변고마워~ 왜 헷갈리게 get하면 값이 나오고 난리여 ㅜ |
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.
고생했어!!!
Context context = new Context(); | ||
context.setVariable("page_url", CREATOR_PREFIX_DOMAIN + payment.getItemName()); |
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.
이제 쓰이지 않는 CREATOR_PREFIX_DOMAIN
상수는 제거하자
public boolean isAfterRefundGuaranteeDuration() { | ||
return LocalDateTime.now() | ||
.isAfter(getCreatedAt().plusDays(7)); | ||
} |
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 boolean isAfterRefundGuaranteeDuration() { | |
return LocalDateTime.now() | |
.isAfter(getCreatedAt().plusDays(7)); | |
} | |
public boolean isAfterRefundGuaranteeDuration() { | |
LocalDate createdDate = getCreatedAt().toLocalDate(); | |
LocalDate nowDate = LocalDate.now(); | |
return nowDate.isAfter(createdDate.plusDays(6)); | |
} |
환불 규정은 7일이내
야. 산 날짜로부터 7일 이내에는 0시 1분
이던 23시 59분
이던 시간에 관계없이 환불할 수 있어야 해.
예)
- 1일 1시 0분 구매했다면
- 7일 23시 59분 까지 환불 가능해야함
- 8일 0시 0분 부터는 환불 불가해야함
지금은 LocalDateTime로 비교하고 있어서 같은 날짜라도 시간에 따라 환불 가능/불가가 달라져.
그리고 plusDays()를 6으로 한건 1일에 구매했다면 7일까지는 환불이 가능해야해서야.
아래는 내가 테스트해본 코드야.
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.
직접 테스트해볼거면 이 코드 써~
import org.junit.jupiter.api.Test;
import java.time.LocalDate;
import java.time.LocalDateTime;
public class LocalDateTimeTest {
@Test
void name() {
int year = 2020;
int month = 1;
// 1일 1시 0분 구매
LocalDateTime createdAt = LocalDateTime.of(year, month, 1, 1, 0);
// 7일 23시 59분 까지 환불 가능
LocalDateTime now = LocalDateTime.of(year, month, 7, 23, 59);
assert !isAfter2(createdAt, now);
// 8일 0시 0분 부터는 환불 불가해야함
now = LocalDateTime.of(year, month, 8, 0, 0);
assert isAfter2(createdAt, now);
// 현재 코드(AS-IS)는 8일 1시 1분 부터 환불 불가임
now = LocalDateTime.of(year, month, 8, 1, 1);
assert isAfter2(createdAt, now);
}
/** AS-IS */
private boolean isAfter1(LocalDateTime createdAt, LocalDateTime now) {
return now.isAfter(createdAt.plusDays(7));
}
/** TO-BE */
private boolean isAfter2(LocalDateTime createdAt, LocalDateTime now) {
LocalDate createdDate = createdAt.toLocalDate();
LocalDate nowDate = now.toLocalDate();
return nowDate.isAfter(createdDate.plusDays(6));
}
}
포인트 충전 api 절차
/payments/charge/ready
request: itemId
response : merchantUid, itemName, amount, email
/payments/charge
request : impUid, merchantUid
response : point
후원 절차
/donations
request : pageName, point
response : donationId, name, message, amount, createdAt
/donations/{donationId}/message
request : name, message, secret
response : -
환불은 내부로직만 바뀌고 엔드포인트 바뀐 것 없음
본인이 보낸 후원을 나중에 볼 수 있게끔 하기위해서
Donation
에DonationType
이라는 enum 타입 필드를 추가해주고,후원 요청 시에 2개의
Donation
을 각각 만들어서 creator와 donator가 가지고 있는 List에 집어넣어줬을 경우,/donations/{donationId}/message
호출 시에 하나의donationId
만 들어올텐데 두 도네이션의Message
를 다 업데이트 하기가 애매한 것 같음!후원자가 본인이 보낸 후원 메세지에 대한 정보가 defaultMessage 여도 상관없다면 괜찮지만... 아니면 프론트한테
/donations
이후에 response 로 donationId를 2개를 response 해주고 message API 에서 2개의 id를 모두 받는 방법도 있긴하지만!Payment
환불 시에 환불 기간 지났는지에 대한 로직을이걸로 짰는데 테스트 잊으면 안됑!
defaultName
이 후원자의 nickname이니깐 이 부분 때문에 생성자가 바뀌었을거야 !!더 말할게 있었던 것 같은데 . .. 기억안나네... 일단 바뀐점 보면서 궁금한거 다 물어봐!!
내가 놓친부분이 무조건 있을 것 같은데, 아마 테스트 짜면서 속속들이 발견될 것 같음 . . !!