-
Notifications
You must be signed in to change notification settings - Fork 8
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
[BE] 피드백 미작성시 받은 피드백 안보이는 기능(#640) #643
Conversation
Test Results 59 files 59 suites 7s ⏱️ Results for commit 4469fd4. ♻️ This comment has been updated with latest results. |
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.
수고하셨습니다~!
말로 전달해드린 부분만 TODO 달아주고 넘어가도 될 것 같아요!~
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.
코드 계속 보는 중인데 UserFeedbackService
로직이 이해가 안 가네요...
이 부분은 내일 출근해서 여쭤보겠습니다
@@ -23,6 +24,9 @@ public record FeedbackOutput(@Schema(description = "피드백 아이디", exampl | |||
@Schema(description = "유저 이름", example = "jcoding-play") | |||
String username, | |||
|
|||
@Schema(description = "내가 상대방의 피드백 작성을 완료하였는지 여부", example = "false") | |||
boolean isWrited, |
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.
isFeedbackCompleted
같은 이름이 더 어울리는 것 같기는 하네요...
MatchResultResponse
에서도 마찬가지입니다.
대신 바꾸게 되면 프론트에 노티줘야 할듯. 👀
@@ -20,6 +22,9 @@ public record FeedbackResponse(@Schema(description = "피드백 아이디", exam | |||
@Schema(description = "유저 이름", example = "jcoding-play") | |||
String username, | |||
|
|||
@Schema(description = "내가 상대방의 피드백 작성을 완료하였는지 여부", example = "false") |
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.
마찬가지입뉘다.
@@ -39,6 +43,7 @@ public static FeedbackOutput fromReceiver(DevelopFeedback developFeedback) { | |||
developFeedback.getDeliver().getId(), | |||
developFeedback.getDeliver().getThumbnailUrl(), | |||
developFeedback.getDeliver().getUsername(), | |||
true, |
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.
지금 하드코딩으로 박혀있는 boolean
값들을 static 상수로 빼보시는 건 어떤가요? 👀
private static final String `FEEDBACK_COMPLETED` = true
private static final String `FEEDBACK_INCOMPLETED` = false 같은 너낌...
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.
괜찮은듯요
@@ -160,4 +160,34 @@ void getFeedback_when_no_feedback_written() { | |||
assertThat(receivedFeedback.feedbacks()).isEmpty(); | |||
assertThat(deliveredFeedback.feedbacks()).isEmpty(); | |||
} | |||
|
|||
@Test | |||
@DisplayName("받은 피드백 중 피드백을 작성하지 않은 상대의 피드백은 빈 응답을 반환한다.") |
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.
@DisplayName("받은 피드백 중 피드백을 작성하지 않은 상대의 피드백은 빈 응답을 반환한다.") | |
@DisplayName("내가 피드백을 작성하지 않은 상대가 나에게 쓴 피드백은 빈 응답을 반환한다.") |
이런 수정은 어떠세요? 이것도 명확하지 않은 것 같긴 합니다...
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.
'내가 피드백을 작성하지 않은 상대로부터 받은 피드백은 빈 응답으로 치환한다.' 와 같이 바꿔보긴했는데...
좀 더 명확해졌을까요...?
assertThat(unmaskedFeedbackData.isWrited()).isTrue(); | ||
assertThat(unmaskedFeedbackData.feedbackText()).isNotEmpty(); | ||
assertThat(maskedFeedbackData.isWrited()).isFalse(); | ||
assertThat(maskedFeedbackData.feedbackText()).isEmpty(); |
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.
위 assertThat
까지 아래로 내려서 assertAll()
로 묶어주어도 좋을 것 같습니당.
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.
구두로 전달한 내용 남겨두었어요~
p3 - SocialFeedbackReader, DevelopFeedbackRader의 existsByDeliverAndReceiver 메서드에 대해서
괜찮은듯?
private boolean needToMasking(long receiverId, FeedbackOutput feedbackResponse, BiPredicate<Long, Long> biConsumer) { | ||
long deliverId = feedbackResponse.receiverId(); |
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 needToMasking(long receiverId, FeedbackOutput feedbackResponse, BiPredicate<Long, Long> biConsumer) { | |
long deliverId = feedbackResponse.receiverId(); | |
private boolean isMaskingRequired(long receiverId, FeedbackOutput feedbackOutput, BiPredicate<Long, Long> feedbackExistsPredicate) { | |
long deliverId = feedbackResponse.receiverId(); | |
// feedbackResponse, feedbackOutput에서 receiverId() -> peerId() 처럼 상대방의 id라는 걸 확인할 수 있도록 |
socialFeedbackOutput.forEach((key, value) -> socialFeedbackOutput.put(key, maskingFeedback(receiverId, value, false))); | ||
} | ||
|
||
private List<FeedbackOutput> maskingFeedback(long receiverId, List<FeedbackOutput> feedbackOutputs, boolean isDevelopFeedback) { |
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 List<FeedbackOutput> maskingFeedback(long receiverId, List<FeedbackOutput> feedbackOutputs, boolean isDevelopFeedback) { | |
private List<FeedbackOutput> maskingFeedback(long receiverId, List<FeedbackOutput> feedbackOutputs, boolean isSocialFeedbackNeeded) { |
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.
마스킹 하는게 생각보다 어렵네용.
차후, 리팩토링 하면 될듯요 수고했씁니다🫡
@@ -39,6 +43,7 @@ public static FeedbackOutput fromReceiver(DevelopFeedback developFeedback) { | |||
developFeedback.getDeliver().getId(), | |||
developFeedback.getDeliver().getThumbnailUrl(), | |||
developFeedback.getDeliver().getUsername(), | |||
true, |
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.
괜찮은듯요
@@ -42,4 +42,8 @@ public DevelopFeedback findDevelopFeedback(long roomId, long deliverId, String u | |||
return developFeedbackRepository.findByRoomIdAndDeliverIdAndReceiverUsername(roomId, deliverId, username) | |||
.orElseThrow(() -> new CoreaException(ExceptionType.FEEDBACK_NOT_FOUND)); | |||
} | |||
|
|||
public boolean existsByDeliverAndReceiver(long deliverId, long receiverId) { |
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.
구두 말했듯이 RoomId 까지 있어야 UNIQUE 하게 식별 가능할듯요~
📌 관련 이슈
✨ PR 세부 내용
받은 피드백 조회 시 내가 작성하지 않은 사람으로부터 받은 피드백이 있는 경우 이를 빈 문자열로 반환하는 기능입니다.
p4 - isWrited 이름에 대해서
MatchResultResponse 에서 비슷한 의미로 isWrited 를 사용해서, 이번에도 같은 네이밍을 가져갔습니다.
또한 프론트에게도 DTO 가 바뀌는 것에 대해 전달했구요.
다만, 이게 적절한 이름인지는 논의해보고 싶네요.
'내가 상대방에게 피드백을 작성했다' 라는 의미에서 isWrited 인데, 이는 쓰여졌는지를 말하는 변수인 것 같아 다른 적절한 네이밍이 있다면 추천해주세요!
p3 - SocialFeedbackReader, DevelopFeedbackRader의 existsByDeliverAndReceiver 메서드에 대해서
지금 해당 메서드는 정말 repository 의 메서드를 중계하는 역할밖에 하지 않습니다.
근데 그렇다고 이 메서드가 쓰이는 로직을 reader 로 내리자니, 좀 안맞는 느낌입니다.
이 메서드의 존재와 이름에 대해서 다들 어떻게 생각하시나요?
Reader, Writer 와 Service 를 분리하는 차원에서 제 구현의 방향성이 다를수도 있을 것 같아요.
다르다면 편하게 말씀주세요, 바로 수정점 반영하겠습니다!
아래는 잡담입니다.