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

사용자 가입 시 신뢰도를 null로 설정 #536

Closed
wants to merge 6 commits into from

Conversation

kwonyj1022
Copy link
Collaborator

📄 작업 내용 요약

사용자 가입 시 신뢰도를 null로 설정

🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드

  • dto 부분도 double로 되어있어서 Double로 변경하였습니다.
  • cmd+shift+f 이용해서 double인 것을 Double로 바꾸긴 했는데 그래도 혹시나 제가 생각하지 못했지만 꼭 필요한 수정 사항들이 있을 수 있기 때문에 꼭 intelliJ로 확인해주시면 감사하겠습니다!

질문

커밋은 하지 않았지만 개인적으로 신뢰도 정렬 테스트코드 픽스처에 신뢰도가 null인 판매자의 경매를 추가하고 신뢰도 정렬을 해보았는데, 신뢰도가 null이면 제일 후순위이더라구요. 그래서 신뢰도 정렬을 했을 때 신뢰도가 0점인 사람보다 뒤에 보이게 되는데, 이 부분 관련해서 정책 회의가 필요할 것 같습니다.

📎 Issue 번호

@kwonyj1022 kwonyj1022 added backend 백엔드와 관련된 이슈나 PR에 사용 feature 기능 추가 시 labels Oct 4, 2023
@kwonyj1022 kwonyj1022 added this to the 최종 데모데이 milestone Oct 4, 2023
@kwonyj1022 kwonyj1022 self-assigned this Oct 4, 2023
Copy link
Collaborator

@apptie apptie left a comment

Choose a reason for hiding this comment

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

인텔리제이로 확인해봤는데 제가 확인했을 때에는 다 잘 바꾼 것 같아요..?
맞겠죠...?

일단 approve 하도록 하겠습니다

@@ -23,7 +23,7 @@
@Entity
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@EqualsAndHashCode(of = "id")
@EqualsAndHashCode(of = "id", callSuper = false)
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

@swonny swonny left a comment

Choose a reason for hiding this comment

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

테스트 관련해 질문 하나 남겨서 rc하겠습니다!
답변 확인하는대로 approve할게요.
고생하셨습니다 엔초~~

@@ -195,6 +196,15 @@ class AuctionServiceTest extends AuctionServiceFixture {
.hasMessage("지정한 아이디에 대한 경매를 찾을 수 없습니다.");
}

@Test
void 지정한_아이디에_해당하는_경매의_판매자_신뢰도가_null이라면_서비스에서_반환하는_dto에서_판매자_신뢰도를_나타내는_부분도_null이다() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

선택

굉장히 자세한 메서드 네이밍이네요..!
그런데 해당 로직은 auction보다는 dto의 getter를 테스트하는 느낌이 더 강한 것 같습니다.
auctionServiceTest에서 진행해주신 이유가 있으신지 궁금합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

혹시나 NPE가 발생하는지 궁금해서 개인적인 테스트를 해보았는데, 원래는 결과만 보고 롤백하려고 했는데 나중에 또 궁금할까 봐 있으면 좋을 것 같아서 커밋하였습니다. 처음 의도는 원래 커밋은 할 생각을 안 했고 그냥 개인적으로 확인차 테스트했던 거라 픽스처 클래스 만들고 하기 귀찮아서 있던 테스트에서 같이 했는데 커밋할 때는 또 생각 없이 해버렸네요ㅠ
그래서 결론은 dto 테스트로 옮기겠습니다~
예리하시네요 메리!

Copy link
Member

@JJ503 JJ503 left a comment

Choose a reason for hiding this comment

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

엔초 고생 많으셨습니다!
빠르시군요 😎

이번 pr에서는 값 객체는 생각하지 말라고 했지만, 적용되었을 때의 상태가 궁금해 한 가지 질문드렸습니다.
해당 부분만 확인해주시면 감사하겠습니다!

@@ -67,7 +67,7 @@ private User findOrPersistUser(final Oauth2Type oauth2Type, final UserInformatio
final User user = User.builder()
.name(oauth2Type.calculateNickname(calculateRandomNumber()))
.profileImage(findDefaultProfileImage())
.reliability(0.0d)
.reliability(null)
Copy link
Member

Choose a reason for hiding this comment

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

질문

값 객체로 변경하게 된다면 null을 직접적으로 넣지 않고, 미리 null로 설정해 둔 값객체에 대한 static 필드를 사용하게 될까요?

Copy link
Collaborator Author

@kwonyj1022 kwonyj1022 Oct 5, 2023

Choose a reason for hiding this comment

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

넵 값객체 적용한 pr에서 User 생성 시 reliability가 null이면 static 필드를 사용하도록 구현되어 있습니다!

Copy link
Collaborator

@swonny swonny left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 엔초!

@JJ503 JJ503 closed this Oct 10, 2023
@JJ503 JJ503 deleted the feature/535 branch October 10, 2023 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 백엔드와 관련된 이슈나 PR에 사용 feature 기능 추가 시
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants