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

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

Merged
merged 26 commits into from
Jul 29, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ef0e0ff
fix: 생성일자에 null이 들어와서 검증 시 발생하는 예외 처리
sc0116 Jul 26, 2022
38dad80
test: 필수 데이터 없는 경우 401 반환하는 인수 테스트 추가
sc0116 Jul 26, 2022
7ea0057
feat: 필수 데이터 없는 경우 401 반환하는 기능 구현
sc0116 Jul 26, 2022
c7fa3cc
feat: 스터디 시작 전에 후기 작성 시 예외 반환 기능 구현
sc0116 Jul 26, 2022
1a42d6b
feat: 스터디 참여 여부 기능 구현
sc0116 Jul 26, 2022
51fb4d8
feat: 리뷰 작성 기능 구현
sc0116 Jul 26, 2022
b07fc33
refactor: BaseTime 제거
sc0116 Jul 26, 2022
1230c32
test: 리뷰 관련 인수테스트 실패
sc0116 Jul 27, 2022
56f8bc3
test: 리뷰 관련 인수테스트 작성
sc0116 Jul 27, 2022
54644f4
test: 리뷰 인수테스트 및 HTTP 요청 테스트 추가
sc0116 Jul 27, 2022
4d0b365
refactor: 예외 메세지 및 메소드명 수정
sc0116 Jul 27, 2022
7cdfb2f
refactor: 사용하지 않는 클래스 제거 및 메소드명 수정
sc0116 Jul 27, 2022
eea26ed
refactor: 스터디 시작 전에 후기 작성 시 예외 반환 기능 로직 변경
sc0116 Jul 27, 2022
81b19ff
style: EOF 처리
sc0116 Jul 27, 2022
94dcdf4
style: 상수명 대문자로 변경
sc0116 Jul 28, 2022
68cac97
test: DisplayName 및 메소드명 수정
sc0116 Jul 28, 2022
b07373d
refactor: 정적 팩터리 메소드 대신 new 생성자로 변경
sc0116 Jul 28, 2022
196b3dc
refactor: 리뷰를 작성할 스터디에 참여했는지 판단하는 로직 수정
sc0116 Jul 28, 2022
dca3c70
refactor: 리뷰 작성 로직에서 검증과 생성 순서 변경
sc0116 Jul 28, 2022
643bfb0
test: 리뷰 관련 테스트 수정
sc0116 Jul 28, 2022
2bb5f55
refactor: 리뷰 관련 Controller와 Service에서 명령을 위한 것과 사용자 조회를 위한 것으로 분리
sc0116 Jul 28, 2022
36d8a34
style: EOF 처리
sc0116 Jul 29, 2022
11fe2f1
refactor: 리뷰 작성 시 해당 스터디 참여 여부 로직에서 스터디장 판단 로직 추가
sc0116 Jul 29, 2022
6a61f2b
chore: develop merge 충돌 해결
sc0116 Jul 29, 2022
f41383e
refactor: 사용하지 않는 부분 제거
sc0116 Jul 29, 2022
9bab968
chore: develop merge 충돌 해결
sc0116 Jul 29, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public boolean preHandle(final HttpServletRequest request, final HttpServletResp
return true;
}

if (request.getMethod().equals("POST") && request.getServletPath().equals("/api/studies")) {
if (request.getMethod().equals("POST") && validatePath(request)) {
String token = AuthenticationExtractor.extract(request);
validateToken(token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

흠..🤔

    @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가 추가되어 코드가 변경되어야할 것 같아요 ㅠ.ㅠ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


Expand All @@ -41,4 +41,9 @@ private void validateToken(String token) {
throw new UnauthorizedException("유효하지 않은 토큰입니다.");
}
}

private boolean validatePath(final HttpServletRequest request) {
return request.getServletPath().equals("/api/studies") ||
request.getServletPath().matches("/api/studies/\\d+/reviews");
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package com.woowacourse.moamoa.common.advice;

import com.woowacourse.moamoa.common.exception.UnauthorizedException;
import com.woowacourse.moamoa.common.advice.response.ErrorResponse;
import com.woowacourse.moamoa.common.exception.BadRequestException;
import com.woowacourse.moamoa.common.exception.InvalidFormatException;
import com.woowacourse.moamoa.common.exception.NotFoundException;
import com.woowacourse.moamoa.common.exception.UnauthorizedException;
import com.woowacourse.moamoa.study.domain.exception.InvalidPeriodException;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
Expand All @@ -14,14 +16,24 @@ public class CommonControllerAdvice {

@ExceptionHandler({
InvalidFormatException.class,
InvalidPeriodException.class
InvalidPeriodException.class,
BadRequestException.class
})
public ResponseEntity<ErrorResponse> handleBadRequest(final Exception e) {
return ResponseEntity.badRequest().body(new ErrorResponse(e.getMessage()));
}

@ExceptionHandler
public ResponseEntity<Void> handleUnauthorized(final UnauthorizedException e) {
@ExceptionHandler({
UnauthorizedException.class
})
public ResponseEntity<Void> handleUnauthorized(final Exception e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}

@ExceptionHandler({
NotFoundException.class
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도!!

public ResponseEntity<ErrorResponse> handleNotFound(final Exception e) {
return ResponseEntity.status(HttpStatus.NOT_FOUND).body(new ErrorResponse(e.getMessage()));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.woowacourse.moamoa.common.exception;

public class BadRequestException extends RuntimeException {

public BadRequestException(final String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.woowacourse.moamoa.common.exception;

public class NotFoundException extends RuntimeException {

public NotFoundException(final String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,20 @@ public MemberDao(final JdbcTemplate jdbcTemplate) {
this.jdbcTemplate = jdbcTemplate;
}

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 "
Comment on lines -29 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

굿굿!!👍

+ "FROM member JOIN study_member ON member.id = study_member.member_id "
+ "WHERE study_member.study_id = ?";
return jdbcTemplate.query(sql, ROW_MAPPER, studyId);
}

public boolean existsByStudyIdAndMemberId(final Long studyId, final Long memberId) {
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 = ? "
+ ")";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return Boolean.TRUE.equals(jdbcTemplate.queryForObject(sql, Boolean.class, studyId, memberId));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public void saveOrUpdate(final Member member) {

public MemberData searchBy(final Long githubId) {
final Member member = memberRepository.findByGithubId(githubId)
.orElseThrow(() -> new InvalidMemberException("사용자를 찾을 수 없습니다."));
.orElseThrow(InvalidMemberException::new);
return new MemberData(member.getGithubId(), member.getUsername(), member.getImageUrl(), member.getProfileUrl());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import com.woowacourse.moamoa.common.exception.UnauthorizedException;

public class InvalidMemberException extends UnauthorizedException {
public InvalidMemberException(final String message) {
super(message);

public InvalidMemberException() {
super("회원을 찾을 수 없습니다.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 Service 쪽 코드보다 Exception 에서 예외 메시지를 관리하는게 좋다고 생각합니다!

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.woowacourse.moamoa.member.service.exception;

import com.woowacourse.moamoa.common.exception.UnauthorizedException;

public class NotParticipatedMemberException extends UnauthorizedException {

public NotParticipatedMemberException() {
super("스터디에 참여한 회원만 후기를 작성할 수 있습니다.");
}
}
Original file line number Diff line number Diff line change
@@ -1,29 +1,44 @@
package com.woowacourse.moamoa.review.controller;

import com.woowacourse.moamoa.auth.config.AuthenticationPrincipal;
import com.woowacourse.moamoa.review.service.ReviewService;
import com.woowacourse.moamoa.review.service.request.WriteReviewRequest;
import com.woowacourse.moamoa.review.service.response.ReviewsResponse;
import java.net.URI;
import javax.validation.Valid;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/api/studies")
@RequestMapping("/api/studies/{study-id}/reviews")
@RequiredArgsConstructor
public class ReviewController {

private final ReviewService reviewService;

@GetMapping("/{study-id}/reviews")
@GetMapping
public ResponseEntity<ReviewsResponse> getReviews(
@PathVariable(name = "study-id") Long studyId,
@RequestParam(required = false) Integer size
@PathVariable(name = "study-id") final Long studyId,
@RequestParam(required = false) final Integer size
) {
final ReviewsResponse reviewsResponse = reviewService.getReviewsByStudy(studyId, size);

return ResponseEntity.ok(reviewsResponse);
}

@PostMapping
public ResponseEntity<Void> writeReview(
@AuthenticationPrincipal final Long githubId,
@PathVariable(name = "study-id") final Long studyId,
@Valid @RequestBody final WriteReviewRequest writeReviewRequest
) {
final Long id = reviewService.writeReview(githubId, studyId, writeReviewRequest);
return ResponseEntity.created(URI.create("/api/studies/" + studyId + "/reviews/" + id)).build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
import lombok.NoArgsConstructor;

@Embeddable
@NoArgsConstructor(access = PROTECTED)
@AllArgsConstructor
@Getter
@NoArgsConstructor(access = PROTECTED)
public class AssociatedStudy {

@Column(name = "study_id", nullable = false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
import static lombok.AccessLevel.PROTECTED;

import com.woowacourse.moamoa.member.domain.Member;
import com.woowacourse.moamoa.review.domain.exception.InvalidReviewException;
import com.woowacourse.moamoa.study.domain.Study;
import java.time.LocalDate;
import java.time.LocalDateTime;
import javax.persistence.Column;
import javax.persistence.Embedded;
Expand All @@ -19,9 +22,9 @@
import org.springframework.data.annotation.LastModifiedDate;

@Entity
@Getter
@NoArgsConstructor(access = PROTECTED)
@AllArgsConstructor
@Getter
public class Review {

@Id
Expand All @@ -35,14 +38,43 @@ public class Review {
@JoinColumn(name = "member_id")
private Member member;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


// @Embedded
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 Author

Choose a reason for hiding this comment

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

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

// private Reviewer reviewer;

@Column(nullable = false)
private String content;

@CreatedDate
@Column(nullable = false)
@Column(nullable = false, updatable = false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

updatable false 굿!!

private LocalDateTime createdDate;

@LastModifiedDate
@Column(nullable = false)
private LocalDateTime lastModifiedDate;

public static Review of(final AssociatedStudy associatedStudy, final Member member, final String content) {
return new Review(associatedStudy, member, content);
}
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

Choose a reason for hiding this comment

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

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


public Review(final AssociatedStudy associatedStudy, final Member member, final String content) {
this(null, associatedStudy, member, content);
}

public Review(final Long id, final AssociatedStudy associatedStudy, final Member member, final String content) {
this.id = id;
this.associatedStudy = associatedStudy;
this.member = member;
this.content = content;
this.createdDate = LocalDateTime.now();
this.lastModifiedDate = LocalDateTime.now();
}

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();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.woowacourse.moamoa.review.domain.exception;

import com.woowacourse.moamoa.common.exception.BadRequestException;

public class InvalidReviewException extends BadRequestException {

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

public InvalidReviewException() {
super(MESSAGE);
}

public InvalidReviewException(final String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,7 @@

public interface ReviewRepository {

Review save(Review review);

List<Review> findAllByAssociatedStudy(AssociatedStudy associatedStudy);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,20 @@

import static java.util.stream.Collectors.toList;

import com.woowacourse.moamoa.member.domain.Member;
import com.woowacourse.moamoa.member.domain.repository.MemberRepository;
import com.woowacourse.moamoa.member.query.MemberDao;
import com.woowacourse.moamoa.member.service.exception.InvalidMemberException;
import com.woowacourse.moamoa.member.service.exception.NotParticipatedMemberException;
import com.woowacourse.moamoa.review.domain.AssociatedStudy;
import com.woowacourse.moamoa.review.domain.Review;
import com.woowacourse.moamoa.review.domain.repository.ReviewRepository;
import com.woowacourse.moamoa.review.service.request.WriteReviewRequest;
import com.woowacourse.moamoa.review.service.response.ReviewResponse;
import com.woowacourse.moamoa.review.service.response.ReviewsResponse;
import com.woowacourse.moamoa.study.domain.Study;
import com.woowacourse.moamoa.study.domain.repository.StudyRepository;
import com.woowacourse.moamoa.study.service.exception.StudyNotFoundException;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
Expand All @@ -18,6 +27,9 @@
public class ReviewService {

private final ReviewRepository reviewRepository;
private final MemberRepository memberRepository;
private final StudyRepository studyRepository;
private final MemberDao memberDao;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


public ReviewsResponse getReviewsByStudy(Long studyId, Integer size) {
final AssociatedStudy associatedStudy = new AssociatedStudy(studyId);
Expand All @@ -38,4 +50,25 @@ private ReviewsResponse makeReviewsResponse(final List<Review> reviews, final in

return new ReviewsResponse(reviewResponses, totalCount);
}

@Transactional
public Long writeReview(final Long githubId, final Long studyId, final WriteReviewRequest writeReviewRequest) {
final AssociatedStudy associatedStudy = new AssociatedStudy(studyId);
final Member member = memberRepository.findByGithubId(githubId)
.orElseThrow(InvalidMemberException::new);
checkParticipate(studyId, member.getId());

final Review review = writeReviewRequest.toReview(associatedStudy, member);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new로 할지 toReview로 할지 고민중

final Study study = studyRepository.findById(studyId)
.orElseThrow(StudyNotFoundException::new);
review.writeable(study);

return reviewRepository.save(review).getId();
}

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.woowacourse.moamoa.review.service.request;

import com.woowacourse.moamoa.member.domain.Member;
import com.woowacourse.moamoa.review.domain.AssociatedStudy;
import com.woowacourse.moamoa.review.domain.Review;
import javax.validation.constraints.NotBlank;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;

@NoArgsConstructor
@AllArgsConstructor
@Getter
public class WriteReviewRequest {

@NotBlank(message = "내용을 입력해 주세요.")
private String content;

public Review toReview(final AssociatedStudy associatedStudy, final Member member) {
return Review.of(associatedStudy, member, content);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
import java.util.Objects;
import javax.persistence.Column;
import javax.persistence.Embeddable;
import lombok.Getter;

@Embeddable
@Getter
public class Period {

private LocalDate enrollmentEndDate;
Expand Down
Loading