[BE] issue120: 스터디 리뷰 작성 #140

sc0116 · 2022-07-27T04:27:52Z

요약

스터디 리뷰 작성 기능

세부사항

내가 참여한 스터디에 대해서만 후기를 작성할 수 있다.
내가 참여하지 않은 스터디에 후기를 작성 시 에러를 응답한다.

close #120

tco0427

구현 굿굿!!
빠트린 final 키워드와 같이 꼼꼼하게 체크해주신 것 같아 좋았습니다!😃
몇가지 궁금한 점들 포함하여 코멘트 남겼어요!

tco0427 · 2022-07-27T23:24:40Z

backend/src/main/java/com/woowacourse/moamoa/auth/controller/AuthenticationInterceptor.java

+        if (request.getMethod().equals("POST") && validatePath(request)) {
            String token = AuthenticationExtractor.extract(request);
            validateToken(token);


여기도 스터디 생성 뿐 아니라 리뷰 작성에 대해서도 토큰이 필요해지면서 변경이 필요해졌군요..!!
그린론 코드에서는 내 스터디 조회 시에도 토큰일 필요하게 되어 변경이 발생하더라구요!

흠..🤔

@Override public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) { if (isPreflight(request)) { return true; } final String token = AuthenticationExtractor.extract(request); if (token != null) { validateToken(token); request.setAttribute("payload", tokenProvider.getPayload(token)); } return true; } private void validateToken(String token) { if (!tokenProvider.validateToken(token)) { throw new UnauthorizedException("유효하지 않은 토큰입니다."); } }

이와 같이 토큰이 있는 경우 즉 토큰이 포함된 요청인 경우에 토큰의 유효성을 검사하는 방향으로 수정하면 어떨까요??
기존의 코드는 새로운 토큰을 동반한 요청의 경우 계속해서 case가 추가되어 코드가 변경되어야할 것 같아요 ㅠ.ㅠ

이것도 정말 좋은데 이렇게 되면 토큰이 필요없는 전체 목록 조회같은 기능에 유효하지 않은 토큰이 포함된다면 예외가 나지 않을까요? 원래같으면 토큰이 필요없는 비회원도 목록을 볼 수 있을텐데..!

tco0427 · 2022-07-27T23:25:42Z

backend/src/main/java/com/woowacourse/moamoa/common/advice/CommonControllerAdvice.java

-    @ExceptionHandler
-    public ResponseEntity<Void> handleUnauthorized(final UnauthorizedException e) {
+    @ExceptionHandler({
+            UnauthorizedException.class
+    })
+    public ResponseEntity<Void> handleUnauthorized(final Exception e) {


혹시 이와같이 변경한 이유가 무엇일까요??

현재 잡는 예외가 하나라서 @ExceptionHandler(UnauthorizedException.class)와 같이 해도 좋을 것 같아요!

기존에는 여러 개를 받고 있어서 일단은 { }로 감쌌는데 병합하면서 수정하겠슴당

tco0427 · 2022-07-27T23:26:08Z

backend/src/main/java/com/woowacourse/moamoa/common/advice/CommonControllerAdvice.java

+    @ExceptionHandler({
+            NotFoundException.class
+    })


여기도!!

tco0427 · 2022-07-27T23:26:50Z

backend/src/main/java/com/woowacourse/moamoa/member/query/MemberDao.java

-    public List<MemberData> findMembersByStudyId(Long studyId) {
-        String sql = "SELECT github_id, username, image_url, profile_url "
+    public List<MemberData> findMembersByStudyId(final Long studyId) {
+        final String sql = "SELECT github_id, username, image_url, profile_url "


굿굿!!👍

tco0427 · 2022-07-27T23:27:45Z

backend/src/main/java/com/woowacourse/moamoa/member/query/MemberDao.java

+        final String sql = "SELECT EXISTS "
+                + "( "
+                + "SELECT * "
+                + "FROM member JOIN study_member ON member.id = study_member.member_id "
+                + "WHERE study_member.study_id = ? AND study_member.member_id = ? "
+                + ")";


서브쿼리를 사용하셨군요..!! inner 쿼리에 * 대신 id 정도만 조회해봐도 동일한 결과가 나올 것 같아요!

tco0427 · 2022-07-27T23:29:22Z

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

    @Column(nullable = false)
    private String content;

    @CreatedDate
-    @Column(nullable = false)
+    @Column(nullable = false, updatable = false)


updatable false 굿!!

tco0427 · 2022-07-27T23:30:06Z

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

+
+    public static Review of(final AssociatedStudy associatedStudy, final Member member, final String content) {
+        return new Review(associatedStudy, member, content);
+    }


음..🤔 생성자 체이닝이 아닌 정적 팩토리 메소드로 하신 이유가 있을까요??

이미 아래에 생성자가 있어서 정적 팩토리 메서드를 만들 이유가 없어 보입니다.

tco0427 · 2022-07-27T23:33:02Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Study.java

+        final LocalDateTime createdAt = LocalDateTime.now();
+        validatePeriod(period, createdAt);
+
        this.id = id;
        this.details = details;
        this.participants = participants;
        this.period = period;
-        this.createdAt = LocalDateTime.now();
+        this.createdAt = createdAt;
        this.attachedTags = attachedTags;

-        validatePeriod(period);


혹시 기존의 코드에서 변경한 이유가 있을까요?? 짱구의 생각이 궁금합니다!

검증 로직이 위에 있어야 한다는 생각에서 변경하게 되었습니다! 검증 메소드에서 사용하는 now()와 실제 createAt에 넣어줄 now()를 동일하게 하기 위해 검증 전 선언했습니당

tco0427 · 2022-07-27T23:33:46Z

...end/src/main/java/com/woowacourse/moamoa/study/service/exception/StudyNotFoundException.java

-public class StudyNotFoundException extends RuntimeException {
+import com.woowacourse.moamoa.common.exception.NotFoundException;
+
+public class StudyNotFoundException extends NotFoundException {


아하..! 이렇게 상속의 용도로 NotFoundException을 만드신 거군요..!!
음..🤔 좋은 것 같은데요...??

넵! NotFound 뿐만 아니라 BadRequest나 Unauthorized 관련 예외 클래스를 각각 만들면 controller advice에 계속 추가해줘야 하는 번거로움 때문에 한번 더 묶어봤습니당

tco0427 · 2022-07-27T23:34:25Z

backend/src/test/java/com/woowacourse/moamoa/review/domain/ReviewTest.java

+    @DisplayName("스터디 시작 전에 후기를 작성하면 400 에러를 반환한다.")
+    @Test
+    void writeable() {


jaejae-yoo

짱구 구현하느라 고생하셨어요! 코멘트 확인 부탁드려요 ~

jaejae-yoo · 2022-07-28T01:33:14Z

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

@@ -35,14 +38,43 @@ public class Review {
    @JoinColumn(name = "member_id")
    private Member member;

+//    @Embedded


주석 제거해주시면 좋을 것 같아요 ~

아 여기 병합 후 바꿀 예정이라 남겨두었어요

jaejae-yoo · 2022-07-28T01:33:53Z

...end/src/main/java/com/woowacourse/moamoa/review/domain/exception/InvalidReviewException.java

+
+public class InvalidReviewException extends BadRequestException {
+
+    private static final String message = "스터디 시작 전 후기를 작성할 수 없습니다.";


상수 컨벤션 대문자면 좋을 것 같아요 !

요거 바로 수정할게욤

jaejae-yoo · 2022-07-28T01:38:25Z

backend/src/test/java/com/woowacourse/moamoa/review/webmvc/BadRequestReviewWebMvcTest.java

+                .andExpect(status().isBadRequest());
+    }
+
+    @DisplayName("필수 데이터인 후기 내용이 없는 경우 400을 반환한다.")


위의 테스트와 차이가 있는지 궁금해요!

아마 ""와 null인데 DisplayName이 중복되었군요

verus-j · 2022-07-28T02:03:19Z

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

@@ -35,14 +38,43 @@ public class Review {
    @JoinColumn(name = "member_id")
    private Member member;


연관 매핑을 사용한 이유가 있나요??
아래 주석처럼 Embedded로 memberId를 가진 Reviewer로 써도 될꺼 같아요

어 이거 병합하고 수정하자고 합의된거 아니었나욤..?

verus-j · 2022-07-28T02:04:54Z

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

+
+    public static Review of(final AssociatedStudy associatedStudy, final Member member, final String content) {
+        return new Review(associatedStudy, member, content);
+    }


이미 아래에 생성자가 있어서 정적 팩토리 메서드를 만들 이유가 없어 보입니다.

verus-j · 2022-07-28T02:05:36Z

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

+    public void writeable(final Study study) {
+        final LocalDate createdDate = this.createdDate.toLocalDate();
+        final LocalDate studyStartDate = study.getPeriod().getStartDate();
+
+        if (createdDate.isBefore(studyStartDate)) {
+            throw new InvalidReviewException();
+        }
+    }


Review에 두지 말고 Service 계층에 두는건 어떨까요?

verus-j · 2022-07-28T02:06:31Z

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

+    private void checkParticipate(final Long studyId, final Long memberId) {
+        if (!memberDao.existsByStudyIdAndMemberId(studyId, memberId)) {
+            throw new NotParticipatedMemberException();
+        }
+    }


checkParticipate는 어떤걸 체크하는지에 대한 의미를 전혀 전달하지 못하는것 같아요...

조금 더 명확하게 수정했습니다

verus-j · 2022-07-28T02:29:47Z

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

@@ -18,6 +27,9 @@
 public class ReviewService {

    private final ReviewRepository reviewRepository;
+    private final MemberRepository memberRepository;
+    private final StudyRepository studyRepository;
+    private final MemberDao memberDao;


ReviewService에 Repository와 Dao 두가지를 사용하고 있는데 사용하는 용도가 서로 달라 분리시키는게 좋아 보입니다.
Repository는 명령을 위해 DB에서 데이터를 조회해 객체로 매핑하는 용도이고, Dao는 조회를 위해 DB에서 데이터를 조회해 DTO로 변환해 전달하는 용도입니다. 이 부분을 별도의 Service 클래스를 두어 분리시켜두는게 좋을 것 같습니다.

verus-j · 2022-07-28T02:31:30Z

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

+    private void checkParticipate(final Long studyId, final Long memberId) {
+        if (!memberDao.existsByStudyIdAndMemberId(studyId, memberId)) {
+            throw new NotParticipatedMemberException();
+        }
+    }


이 부분은 데이터를 외부로 전달하기 위한 조회가 아닌 명령을 위해서 DB를 조회하기 때문에, Repository에서 확인하는게 좋아보입니다.

sc0116 · 2022-07-28T08:15:16Z

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

+        }
+
+        final AssociatedStudy associatedStudy = new AssociatedStudy(studyId);
+        final Review review = writeReviewRequest.toReview(associatedStudy, member);


new로 할지 toReview로 할지 고민중

sc0116 · 2022-07-28T08:22:51Z

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

+                .orElseThrow(InvalidMemberException::new);
+        final Participant participant = new Participant(member.getId());
+
+        if (!study.isParticipant(participant)) {


isParticipated 로 수정하는게 좋을 것 같음

sc0116 · 2022-07-28T08:25:28Z

backend/src/test/java/com/woowacourse/acceptance/AcceptanceTest.java

@@ -56,7 +57,7 @@ public class AcceptanceTest {
    private MockRestServiceServer mockServer;

    @BeforeEach
-    void setUp() {
+    protected void setUp() {


AcceptanceTest의 setUp()을 setRestAssuredPort() 로 바꿔주는 게 좋을듯
(BeforeEach의 순서를 정할 수 있는지 알아보기)

sc0116 · 2022-07-28T08:28:16Z

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

+    void writeReviewByNonParticipateStudy() {
+        final WriteReviewRequest writeReviewRequest = new WriteReviewRequest("후기 내용입니다.");
+        final String jwtToken = getBearerTokenBySignInOrUp(
+                new GithubProfileResponse(2L, "jjanga", "https://image", "github.com"));


jjanga버리기

sc0116 · 2022-07-28T08:31:13Z

backend/src/test/java/com/woowacourse/moamoa/review/webmvc/BadRequestReviewWebMvcTest.java

+    @MockBean
+    private ReviewService reviewService;
+
+    @DisplayName("필수 데이터인 후기 내용이 비어있는 경우 400을 반환한다.")


설명을 공백과 null로 구분하는게 좋을듯

tco0427

추가적인 피드백 반영까지 고생하셨습니다!!
Approve 해도 될 것 같습니다!👍

jaejae-yoo · 2022-07-29T02:44:55Z

backend/src/main/java/com/woowacourse/moamoa/member/service/MemberService.java

@@ -24,7 +24,7 @@ public void saveOrUpdate(final Member member) {

    public MemberData searchBy(final Long githubId) {
        final Member member = memberRepository.findByGithubId(githubId)
-                .orElseThrow(InvalidMemberException::new);
+                .orElseThrow(MemberNotFoundException::new);


명확해서 좋아요 👍

jaejae-yoo · 2022-07-29T02:47:12Z

backend/src/test/java/com/woowacourse/moamoa/study/domain/ParticipantsTest.java

+
+        assertThat(participants.contains(participant)).isTrue();
+    }
+}


EOF만 확인 부탁드려요 ~

sc0116 added 8 commits July 26, 2022 13:46

fix: 생성일자에 null이 들어와서 검증 시 발생하는 예외 처리

ef0e0ff

test: 필수 데이터 없는 경우 401 반환하는 인수 테스트 추가

38dad80

feat: 필수 데이터 없는 경우 401 반환하는 기능 구현

7ea0057

feat: 스터디 시작 전에 후기 작성 시 예외 반환 기능 구현

c7fa3cc

feat: 스터디 참여 여부 기능 구현

1a42d6b

feat: 리뷰 작성 기능 구현

51fb4d8

refactor: BaseTime 제거

b07fc33

test: 리뷰 관련 인수테스트 실패

1230c32

sc0116 self-assigned this Jul 27, 2022

sc0116 added 🚀 feature New feature or request 🖥 backend New backend feature labels Jul 27, 2022

sc0116 requested review from jaejae-yoo, tco0427 and verus-j July 27, 2022 04:28

sc0116 added 6 commits July 27, 2022 16:09

test: 리뷰 관련 인수테스트 작성

56f8bc3

test: 리뷰 인수테스트 및 HTTP 요청 테스트 추가

54644f4

refactor: 예외 메세지 및 메소드명 수정

4d0b365

refactor: 사용하지 않는 클래스 제거 및 메소드명 수정

7cdfb2f

refactor: 스터디 시작 전에 후기 작성 시 예외 반환 기능 로직 변경

eea26ed

style: EOF 처리

81b19ff

tco0427 reviewed Jul 27, 2022

View reviewed changes

jaejae-yoo requested changes Jul 28, 2022

View reviewed changes

sc0116 added 2 commits July 28, 2022 10:49

style: 상수명 대문자로 변경

94dcdf4

test: DisplayName 및 메소드명 수정

68cac97

verus-j requested changes Jul 28, 2022

View reviewed changes

sc0116 added 2 commits July 28, 2022 11:42

refactor: 정적 팩터리 메소드 대신 new 생성자로 변경

b07373d

refactor: 리뷰를 작성할 스터디에 참여했는지 판단하는 로직 수정

196b3dc

sc0116 commented Jul 28, 2022

View reviewed changes

sc0116 added 2 commits July 29, 2022 01:33

refactor: 리뷰 작성 로직에서 검증과 생성 순서 변경

dca3c70

test: 리뷰 관련 테스트 수정

643bfb0

refactor: 리뷰 관련 Controller와 Service에서 명령을 위한 것과 사용자 조회를 위한 것으로 분리

2bb5f55

tco0427 approved these changes Jul 29, 2022

View reviewed changes

jaejae-yoo approved these changes Jul 29, 2022

View reviewed changes

sc0116 added 3 commits July 29, 2022 11:50

style: EOF 처리

36d8a34

refactor: 리뷰 작성 시 해당 스터디 참여 여부 로직에서 스터디장 판단 로직 추가

11fe2f1

chore: develop merge 충돌 해결

6a61f2b

verus-j approved these changes Jul 29, 2022

View reviewed changes

sc0116 added 2 commits July 29, 2022 15:08

refactor: 사용하지 않는 부분 제거

f41383e

chore: develop merge 충돌 해결

9bab968

sc0116 merged commit cdeb842 into develop Jul 29, 2022

verus-j deleted the feat/120-write-review branch July 29, 2022 07:09


		public class InvalidReviewException extends BadRequestException {

		private static final String message = "스터디 시작 전 후기를 작성할 수 없습니다.";

		@@ -35,14 +38,43 @@ public class Review {
		@JoinColumn(name = "member_id")
		private Member member;

[BE] issue120: 스터디 리뷰 작성 #140

[BE] issue120: 스터디 리뷰 작성 #140

Conversation

sc0116 commented Jul 27, 2022

요약

세부사항

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

jaejae-yoo 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

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