[BE] issue195: 리뷰 수정 및 삭제 #198

sc0116 · 2022-08-03T08:43:36Z

요약

리뷰를 수정하거나 삭제한다.

세부사항

리뷰 수정

리뷰를 수정한다.
내가 작성하지 않은 리뷰를 수정하려는 경우 예외를 반환한다.

리뷰 삭제

리뷰를 삭제한다.
내가 작성하지 않은 리뷰를 삭제하려는 경우 예외를 반환한다.

close #195

tco0427 · 2022-08-03T09:12:20Z

backend/src/main/java/com/woowacourse/moamoa/review/domain/Reviewer.java

+    public boolean isSame(final Long memberId) {
+        return Objects.equals(this.memberId, memberId);
+    }


eq & hc 오버라이딩해도 될 것 같습니다!

저도 디우의 말을 듣고 만들까 고민했는데 Review에서 Reviewer 두개를 비교하는게 아니라 Reviewer 객체한테 memberId 넘겨주고 비교하는 거라서 기존이 더 깔끔할 것 같은데 어떻게 생각하세요?
만약 eq&hc로 비교하려면 Review에서 memberId를 Reviewer로 감싸줘야 할것같은데 굳이라는 생각이 드네요..!

Reviewer reviewer = new Reviewer(1L); Reviewer anotherReviewer = new Reviewer(2L); reviewer.equals(anotherReviewer);

위의 코드 보다는

Reviewer reviewer = new Reviewer(1L); Long memberId = 2L; reviewer.isSame(memberId);

이 더 깔끔할 것 같다라고 이해하면 될까요??

현재 기능 동작같은 경우에는 둘이 논리적으로 동일하게 동작하고 있고, 인자로 넘겨주는 memberId 가 리뷰를 작성한 사람의 memberId 와 같은가라는 입장에서 보았을 때, 짱구의 말대로 eq&hc(동일한 객체인가 보다는 리뷰를 작성한 사람의 memberId와 같은가 라는 관점으로 보았을 때) 는 적절하지 못할수도 있겠다는 생각이 드네요!

그렇다면, isSame 이 아니라 isSameMember(예시) 와 같은 네이밍이면 보다 명확할 것 같네요!!
(개인적으로 isSame은 Reviewer 객체가 같은지를 묻는 것 같아서..)

앗 isSameMember로 변경하겠습니당!

tco0427

put 응답 포함해서 몇가지 사소한 부분 코멘트 남겼습니다~_~

tco0427 · 2022-08-03T09:14:24Z

backend/src/main/java/com/woowacourse/moamoa/review/service/request/EditReviewRequest.java

+@NoArgsConstructor
+@AllArgsConstructor
+@Getter
+public class EditReviewRequest {


Edit이 검색해보니 동사여서..명사로 해주면 좋을 것 같습니다!

앗..! Editing으로 수정했습니다!

tco0427 · 2022-08-03T09:18:08Z

backend/src/test/java/com/woowacourse/moamoa/docs/ReviewDocumentationTest.java

@@ -43,7 +43,7 @@ void create() throws Exception {
    @DisplayName("스터디에 리뷰를 전체 조회한다.")


리뷰 수정 및 삭제에 대한 REST DOCS 테스트도 추가해주세요!

감사함둥 ㅠㅠㅠㅠ 진짜 생각도 못했네요..!

jaejae-yoo · 2022-08-04T02:56:24Z

.../src/main/java/com/woowacourse/moamoa/review/service/exception/UnwrittenReviewException.java

+import com.woowacourse.moamoa.common.exception.BadRequestException;
+
+public class UnwrittenReviewException extends BadRequestException {
+    public UnwrittenReviewException() {


한 칸 띄어도 될 것 같아요

jaejae-yoo · 2022-08-04T03:03:14Z

backend/src/main/java/com/woowacourse/moamoa/auth/config/AuthRequestMatchConfig.java

                .addUpAuthenticationPath(HttpMethod.GET, "/api/my/studies")
+                .addUpAuthenticationPath(HttpMethod.PUT, "/api/studies/\\d+/reviews/\\d+")
+                .addUpAuthenticationPath(HttpMethod.DELETE, "/api/studies/\\d+/reviews/\\d+")


jaejae-yoo · 2022-08-04T03:06:23Z

backend/src/main/java/com/woowacourse/moamoa/review/service/ReviewService.java

+        final Review review = reviewRepository.findById(reviewId)
+                .orElseThrow(ReviewNotFoundException::new);
+
+        if (!review.isReviewer(member.getId())) {


밑에 같은 검증을 하는 코드가 있어서, 메서드로 분리해도 좋을 것 같아요 !

jaejae-yoo · 2022-08-04T03:07:12Z

backend/src/test/java/com/woowacourse/acceptance/review/ReviewsAcceptanceTest.java

+                .then().statusCode(HttpStatus.NO_CONTENT.value());
+    }
+
+    @DisplayName("내가 작성하지 않은 리뷰를 삭제할 수 없다.")


꼼꼼한 테스트 👍

verus-j · 2022-08-04T06:13:22Z

backend/src/main/java/com/woowacourse/moamoa/auth/config/AuthRequestMatchConfig.java

-                .addUpAuthenticationPath(HttpMethod.POST, "/api/studies", "/api/studies/\\d+/reviews")
+                .addUpAuthenticationPath(HttpMethod.POST, "/api/studies", "/api/studies/\\d+/reviews", "/api/studies/\\d+/reviews/\\d+")
                .addUpAuthenticationPath(HttpMethod.GET, "/api/my/studies")
+                .addUpAuthenticationPath(HttpMethod.PUT, "/api/studies/\\d+/reviews/\\d+")
+                .addUpAuthenticationPath(HttpMethod.DELETE, "/api/studies/\\d+/reviews/\\d+")


중복된 URL이 너무 많은데 어떻게 하면 중복을 최대한 없앨 수 있을지 addUpAuthenticationPath의 시그니처를 변경해서 생각해보면 좋을 것 같아요.

verus-j · 2022-08-04T06:14:56Z

backend/src/main/java/com/woowacourse/moamoa/review/domain/Review.java

+    public boolean isReviewer(final Long memberId) {
+        return reviewer.isSame(memberId);
+    }


Suggested change

public boolean isReviewer(final Long memberId) {

return reviewer.isSame(memberId);

}

public boolean isReviewer(final Long memberId) {

return reviewer.equals(new Reviewer(memberId));

}

이건 어떤신가요?

verus-j · 2022-08-04T06:18:39Z

backend/src/main/java/com/woowacourse/moamoa/review/service/ReviewService.java

+        if (!review.isReviewer(member.getId())) {
+            throw new UnwrittenReviewException();
+        }
+
+        review.updateContent(editingReviewRequest.getContent());


Suggested change

if (!review.isReviewer(member.getId())) {

throw new UnwrittenReviewException();

}

review.updateContent(editingReviewRequest.getContent());

review.updateContent(new Reviewer(member.getId()), editingReviewRequest.getContent());

위와 같이 하고 updateContent 안에서 예외를 던지도록 하는게 좋아 보입니다.
그 이유는 updateContent 안에 넣어주면 domain 단위 테스트에서 예외가 던져지는 케이스를 확인할 수 있어요.
controller나 service 테스트에서는 NotFound 케이스와 전체적인 성공 케이스(얘는 인수에서 봐도 될 듯 합니다.) 하나만 봐도 될 꺼 같아요.

이렇게 바꾸니까 Review에서 단위 테스트로 충분히 가능하네요!

verus-j · 2022-08-04T06:21:04Z

backend/src/test/java/com/woowacourse/acceptance/review/ReviewsAcceptanceTest.java

+    @DisplayName("내가 작성하지 않은 리뷰를 삭제할 수 없다.")
+    @Test
+    void deleteNotWriteReview() {


이 케이스는 Controller나 Service 테스트로 옮겨도 될 것 같습니다.

옮겼습니다!

verus-j · 2022-08-04T06:21:25Z

backend/src/test/java/com/woowacourse/acceptance/review/ReviewsAcceptanceTest.java

+
+    @DisplayName("내가 작성하지 않은 리뷰를 수정할 수 없다.")
+    @Test
+    void updateNotWriteReview() {


이 케이스도 동일하게 Controller나 Service로 옮겨도 될 것 같습니다.

옮겼습니다!

sc0116 added 7 commits August 3, 2022 02:51

test: 내가 작성한 리뷰 삭제하는 인수 테스트 작성

6732515

feat: 내가 작성하지 않은 리뷰 삭제 시 예외 처리 구현

e0bed44

refactor: 리뷰 삭제 시 리뷰 Id로만 리뷰 불러오도록 수정

e32e340

test: 내가 작성한 리뷰 수정하는 인수 테스트 작성

87d6bd6

feat: 내가 작성하지 않은 리뷰 수정 시 예외 처리 구현

715422f

Merge branch 'develop' into feat/195-update-or-delete-review

7699405

test: 리뷰 관련 테스트 추가

0a7f7a0

sc0116 requested review from verus-j, tco0427 and jaejae-yoo August 3, 2022 08:43

sc0116 self-assigned this Aug 3, 2022

sc0116 added 🚀 feature New feature or request 🖥 backend New backend feature labels Aug 3, 2022

sc0116 linked an issue Aug 3, 2022 that may be closed by this pull request

[BE] 리뷰 수정 및 삭제 #195

Closed

test: 리뷰 관련 테스트 수정

c28ab23

tco0427 reviewed Aug 3, 2022

View reviewed changes

tco0427 requested changes Aug 3, 2022

View reviewed changes

sc0116 added 3 commits August 4, 2022 03:30

test: 리뷰 수정 및 삭제 REST Docs 테스트 추가

9761e1d

feat: 인증 관련 설정

b5588dc

chore: develop merge 충돌 해결

df6ac53

jaejae-yoo approved these changes Aug 4, 2022

View reviewed changes

test: 리뷰 수정 및 삭제 인수테스트 추가

1507f98

tco0427 approved these changes Aug 4, 2022

View reviewed changes

verus-j requested changes Aug 4, 2022

View reviewed changes

feat: 리뷰 삭제 시 Soft Delete로 변경 및 테스트 추가

c93db00

verus-j approved these changes Aug 4, 2022

View reviewed changes

verus-j merged commit 245d832 into develop Aug 4, 2022

verus-j deleted the feat/195-update-or-delete-review branch August 4, 2022 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] issue195: 리뷰 수정 및 삭제 #198

[BE] issue195: 리뷰 수정 및 삭제 #198

sc0116 commented Aug 3, 2022

tco0427 Aug 3, 2022

sc0116 Aug 4, 2022

tco0427 Aug 4, 2022

sc0116 Aug 4, 2022

tco0427 left a comment

tco0427 Aug 3, 2022

sc0116 Aug 4, 2022

tco0427 Aug 3, 2022

sc0116 Aug 4, 2022

jaejae-yoo Aug 4, 2022

jaejae-yoo Aug 4, 2022

jaejae-yoo Aug 4, 2022

jaejae-yoo Aug 4, 2022

verus-j Aug 4, 2022

verus-j Aug 4, 2022

verus-j Aug 4, 2022

sc0116 Aug 4, 2022

verus-j Aug 4, 2022

sc0116 Aug 4, 2022

verus-j Aug 4, 2022

sc0116 Aug 4, 2022

		@@ -43,7 +43,7 @@ void create() throws Exception {
		@DisplayName("스터디에 리뷰를 전체 조회한다.")

[BE] issue195: 리뷰 수정 및 삭제 #198

[BE] issue195: 리뷰 수정 및 삭제 #198

Conversation

sc0116 commented Aug 3, 2022

요약

세부사항

리뷰 수정

리뷰 삭제

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tco0427 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment