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

refactor(server): Donation Status를 변경한다 #398

Merged
merged 4 commits into from Oct 1, 2021

Conversation

Rok93
Copy link
Collaborator

@Rok93 Rok93 commented Sep 29, 2021

작업내용

  • DonationStatus 를 기존 4개(‘환불가능’, ‘정산가능’, ‘정산완료’, ‘취소됨’)에서 2개로 (WAITING_FOR_EXCHANGE, EXCHANGED) 변경
  • Donation의 DonationStatus를 CANCELLED, EXCHANGEABLE로 변경하는 기능
  • Donation.toCancelled() , Donation.toExchangeable() 기능 제거
    • 관련 테스트도 모두 삭제
  • Donation을 처음 생성했을 때 처음 DonationStatus를 WAITING_FOR_EXCHANGE로 초기화
  • 공개 도네이션
    • findFirst5ByMemberAndStatusNotOrderByCreatedAtDesc() 기능 status로 필터링하지 않도록 변경
  • DonationRepositoryTest에 불필요한 PaymentRepository 필드 제거

고민되는 부분

DonationRepositoryImpl 클래스

변경전

BE823B94-70A6-4DFA-B0F0-208747378469

DonationStatus가 2개로 변경되면서 QueryDsl로 작성한 exchangeablePointcurrentPoint 가 동일한 로직으로 보임

변경후

D3344F5C-80B3-48D3-A91B-4324C9EB0875

  • currentPoint의 의미는 정산 가능 + 정산 불가 를 합한 총 포인트
    • But 정산 불가능한 포인트는 없다.
    • currentPoint 기능은 없어져야할 것 같음. (테스트도 같이)
    • 일단은 안 지우고 그대로 뒀는데… 혹시나 지우면 안될까봐 일단 뒀는데 지워도 될지 안될지 코멘트 주면 좋을 것 같음
      53F47E5E-4B5B-4C0C-B6EB-664A48FC4B22

MemberService#validateExchangeable 메서드

  • 정산 금액이 10,000원 이하인 경우 예외 발생하도록 되어있는 로직이 이 코드인데…
  • 10,000 미만인 경우에만 정산 불가능하도록 변경 (이거 내가 해도 되는거지…?)
    9A3407BC-D714-4E33-B2D0-DF95561196E0

결론

DonationRepositoryImpl 클래스에 currentPoint(...) 삭제해도 되는지...? 🤔
(삭제해도 된다고 하면 메서드 삭제 + 테스트 코드 삭제할 예정)

번외…

여러분 … DonationType 이거는 뭐죠…?
이거 프론트 크루들이 만든거같은데 이게 뭐지…?
우리 도메인에 쓰이는 곳 없음. 필요없는거면 삭제하면 될 듯
Release할 때 생긴걸로 봤을 때는 force push 하다가 생긴게 아닐까…?

91B6B411-5882-4661-BBE3-036B80095B64

Closes #397

WAITING_FOR_EXCHANGE, EXCHANGED 만 존재하도록 변경
@Rok93 Rok93 self-assigned this Sep 29, 2021
@Rok93 Rok93 added refactor 기능에 변경이 없는 코드 리팩터링 server 백엔드 관련 이슈 labels Sep 29, 2021
@Joyykim
Copy link
Collaborator

Joyykim commented Sep 30, 2021

  1. exchangeablePointcurrentPoint는 하나로 합치면 되겠다!
  2. DonationType은 내 생각엔 우리가 지운 클래스가 프론트에는 남아있어서 브랜치 합치는 과정에서 넘어온듯! 지우자!
  3. MemberService.validateExchangeable() 수정해주면 땡큐!!

@DWL5
Copy link
Collaborator

DWL5 commented Sep 30, 2021

나는 exchangablePoint의 네이밍을 currentPoint로 바꾸면 좀 더 좋을 듯?
현재 currentPoint (정산 가능 + 정산 불가)의 로직은 없어져도 될 거 같애!

@Rok93
Copy link
Collaborator Author

Rok93 commented Sep 30, 2021

수리랑 조이 의견 반영해서 수정했어!! 시간될 때 다시 리뷰부탁해 🙏

Copy link
Collaborator

@Be-poz Be-poz left a comment

Choose a reason for hiding this comment

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

로키 확인 다 했다~! 앞에서 이미 한 차례 피드백 반영이 돼서 딱히 모난데가 없었던 것 같어~!
내거랑 충돌이 좀 생길 것 같은데 이 pr 머지시키고 내 브랜치에서 풀땡겨서 작업하면 될 것 같다!
로키 수고했으~!

@@ -140,7 +139,7 @@ private void validateExchangeable(Member member, Long exchangeablePoint) {
if (exchangeRepository.existsByPageName(member.getPageName())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

코드 변경은 없는 부분인데 우리가 Exchange를 기존에는 처리하고 난 다음에 지워줬었는데, 이제 지우지 않고 내버려두는 방식으로 하게 된다면 이 부분 검증 로직이 바뀌어야 될 것 같네

Copy link
Collaborator

Choose a reason for hiding this comment

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

이건 내쪽에서 수정할게!

Copy link
Collaborator

Choose a reason for hiding this comment

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

내 쪽에서 exchangeRepository 수정하면서 로직, 테스트코드 바꾸고 있으니까 이런건 넘겨도 될듯

Copy link
Collaborator

@Joyykim Joyykim left a comment

Choose a reason for hiding this comment

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

고생했어 로키! 깔끔하게 리팩토링됐네! API문서 수정만 하면 될거같아.

private Long exchangedTotalPoint;

public DetailedPointResponse(Long currentPoint, Long exchangeablePoint, Long exchangedTotalPoint) {
public DetailedPointResponse(Long currentPoint, Long exchangedTotalPoint) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

프론트에도 알려주고 API문서 수정해야할듯!

@@ -140,7 +139,7 @@ private void validateExchangeable(Member member, Long exchangeablePoint) {
if (exchangeRepository.existsByPageName(member.getPageName())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

내 쪽에서 exchangeRepository 수정하면서 로직, 테스트코드 바꾸고 있으니까 이런건 넘겨도 될듯

@Joyykim Joyykim merged commit cc35369 into develop Oct 1, 2021
@Joyykim Joyykim deleted the refactor/donation-status branch October 1, 2021 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor 기능에 변경이 없는 코드 리팩터링 server 백엔드 관련 이슈
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Donation Status를 변경한다
4 participants