-
Notifications
You must be signed in to change notification settings - Fork 2
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: 내 피드 리스트 공개 상태 여부 정책 변경에 따른 API 수정 #290
Conversation
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.
오랜만의 PR과 코드 리뷰.. 조금 어색하지만 오랜만에 하니 재밌네요 ㅎㅎ
기능 수정 내용 확인했고, 코드와 테스트 코드도 확인했습니다.
그런데 함께 의논하고 싶은 내용이 있어 코멘트 남겨두었으니 확인 부탁드릴게요!
src/main/java/com/listywave/user/application/service/UserService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/listywave/acceptance/user/UserAcceptanceTestHelper.java
Outdated
Show resolved
Hide resolved
src/test/java/com/listywave/acceptance/user/UserAcceptanceTestHelper.java
Outdated
Show resolved
Hide resolved
src/test/java/com/listywave/acceptance/user/UserAcceptanceTestHelper.java
Outdated
Show resolved
Hide resolved
src/test/java/com/listywave/acceptance/user/UserAcceptanceTest.java
Outdated
Show resolved
Hide resolved
아 그리고 "리팩터링, 테스트"로 되어 있던 Label을 "기능, 리팩터링"으로 변경했습니다. |
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.
로직 너무너무 깔끔합니다~
두 가지 논의드릴 사항이 있어 리뷰 남겼습니다.
이 부분만 해결되면 바로 Approve 해도 될 것 같아요!
public void changeVisibility(Long loginUserId, Long listId) { | ||
User user = userRepository.getById(loginUserId); | ||
ListEntity list = listRepository.getById(listId); | ||
list.validateOwner(user); | ||
list.updateVisibility(); | ||
} |
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.
상당히 깔끔하군요 👍
src/main/java/com/listywave/list/presentation/controller/ListController.java
Outdated
Show resolved
Hide resolved
src/test/java/com/listywave/acceptance/list/ListAcceptanceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/listywave/acceptance/list/ListAcceptanceTestHelper.java
Outdated
Show resolved
Hide resolved
@@ -183,7 +183,7 @@ class ListEntityTest { | |||
void 공개_여부를_수정할_수_있다() { | |||
assertThat(list.isPublic()).isTrue(); | |||
|
|||
list.updateVisibility(false); | |||
list.updateVisibility(); |
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.
반영해주셔서 감사합니다
수고하셨어요!!
@@ -120,7 +120,7 @@ ResponseEntity<Void> deleteLists( | |||
@RequestBody ListsDeleteRequest request | |||
) { | |||
listService.deleteLists(userId, request.listId()); | |||
return ResponseEntity.ok().build(); | |||
return ResponseEntity.noContent().build(); |
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.
👍
ea5dab7
to
e1d3b74
Compare
Description
🛠️ 변경한 사항
ex) 1번 리스트가 공개인 경우
ASIS
그런데 이 방식은 프론트측에서 리스트 상세에서 받는 isPublic과 뭔가 매칭이 안되서 햇갈린다고 통일을 요청함
TOBE
해당 리스트가 공개일때 이를 비공개로 바꾸고자 할때 isPublic = false로 보냄( 내가 바꾸고 싶은 상태를 보내는 방식)
해당 부분에 테스트 코드가 존재하지 않아서 테스트 코드를 추가함!
Relation Issues