-
Notifications
You must be signed in to change notification settings - Fork 5
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
Changes from 14 commits
ef0e0ff
38dad80
7ea0057
c7fa3cc
1a42d6b
51fb4d8
b07fc33
1230c32
56f8bc3
54644f4
4d0b365
7cdfb2f
eea26ed
81b19ff
94dcdf4
68cac97
b07373d
196b3dc
dca3c70
643bfb0
2bb5f55
36d8a34
11fe2f1
6a61f2b
f41383e
9bab968
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 기존에는 여러 개를 받고 있어서 일단은 { }로 감쌌는데 병합하면서 수정하겠슴당 |
||
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); | ||
} | ||
|
||
@ExceptionHandler({ | ||
NotFoundException.class | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = ? " | ||
+ ")"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 서브쿼리를 사용하셨군요..!! inner 쿼리에 |
||
return Boolean.TRUE.equals(jdbcTemplate.queryForObject(sql, Boolean.class, studyId, memberId)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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("회원을 찾을 수 없습니다."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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; | ||
|
@@ -19,9 +22,9 @@ | |
import org.springframework.data.annotation.LastModifiedDate; | ||
|
||
@Entity | ||
@Getter | ||
@NoArgsConstructor(access = PROTECTED) | ||
@AllArgsConstructor | ||
@Getter | ||
public class Review { | ||
|
||
@Id | ||
|
@@ -35,14 +38,43 @@ public class Review { | |
@JoinColumn(name = "member_id") | ||
private Member member; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 어 이거 병합하고 수정하자고 합의된거 아니었나욤..? |
||
|
||
// @Embedded | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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(); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = "스터디 시작 전 후기를 작성할 수 없습니다."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 요거 바로 수정할게욤 |
||
|
||
public InvalidReviewException() { | ||
super(message); | ||
} | ||
|
||
public InvalidReviewException(final String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -18,6 +27,9 @@ | |
public class ReviewService { | ||
|
||
private final ReviewRepository reviewRepository; | ||
private final MemberRepository memberRepository; | ||
private final StudyRepository studyRepository; | ||
private final MemberDao memberDao; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ReviewService에 Repository와 Dao 두가지를 사용하고 있는데 사용하는 용도가 서로 달라 분리시키는게 좋아 보입니다. |
||
|
||
public ReviewsResponse getReviewsByStudy(Long studyId, Integer size) { | ||
final AssociatedStudy associatedStudy = new AssociatedStudy(studyId); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. checkParticipate는 어떤걸 체크하는지에 대한 의미를 전혀 전달하지 못하는것 같아요... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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); | ||
} | ||
} |
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.
여기도
스터디 생성
뿐 아니라리뷰 작성
에 대해서도 토큰이 필요해지면서 변경이 필요해졌군요..!!그린론 코드에서는
내 스터디 조회
시에도 토큰일 필요하게 되어 변경이 발생하더라구요!흠..🤔
이와 같이
토큰이 있는 경우
즉 토큰이 포함된 요청인 경우에 토큰의 유효성을 검사하는 방향으로 수정하면 어떨까요??기존의 코드는 새로운
토큰을 동반한 요청
의 경우 계속해서 case가 추가되어 코드가 변경되어야할 것 같아요 ㅠ.ㅠ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.
이것도 정말 좋은데 이렇게 되면 토큰이 필요없는 전체 목록 조회같은 기능에 유효하지 않은 토큰이 포함된다면 예외가 나지 않을까요? 원래같으면 토큰이 필요없는 비회원도 목록을 볼 수 있을텐데..!