-
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
[feat] 질문 기능 개발 #10
[feat] 질문 기능 개발 #10
Conversation
- 서비스와 컨트롤러 정상 동작에 대한 테스트만 작성(에러 상황에 대한 테스트 x) - 테스트 완료 - 이 후, 리팩토링이 필요한 부분은 있음
@NotNull | ||
Long memberId, | ||
@NotBlank | ||
String title, | ||
@NotBlank | ||
String content, | ||
String imageUrl, | ||
@NotNull | ||
List<String> skills |
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.
들어오는 @RequestBody
객체에 대한 정보를 검증할 수도 있군요. 배워갑니다.
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.
^-^
- 바뀐 application.yml에서의 db 설정 적용
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.
다른 도메인 건드릴 시 분리해서 올리셔주세요
- MemberControllerTest과 QuestionControllerTest 파일들을 통일된 코드 형식으로 변경(stubbing -> any / perform -> 만든 변수 / verify -> any / andExpect -> is())
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.
리뷰 남긴거 확인 부탁드려요
.title(createQuestionRequest.title()) | ||
.content(createQuestionRequest.content()) | ||
.imageUrl(createQuestionRequest.imageUrl()) | ||
.viewCount(0L) |
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.
record를 사용하지 않고 class를 썼다면 다른 식으로 기본 값을 설정할 수 있었을 텐데.... 다른 방법이 없을까요? @Hju95 @RossKWSang
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.
이 방법은 별로인가요?
별로라고 생각하시면 이유를 알려주세요!
다른 방법으로 entity 파일에서 기본값을 설정하는 방법을 생각하긴 했는데, create 말고 다른 곳에도 빌더를 쓰는 경우가 있다면 안될 것 같아서 일단 저렇게 코드를 짰습니다.
src/main/java/com/kernel360/kernelsquare/domain/question/controller/QuestionController.java
Show resolved
Hide resolved
src/main/java/com/kernel360/kernelsquare/domain/question/dto/PutQuestionRequest.java
Outdated
Show resolved
Hide resolved
|
||
import jakarta.persistence.Column; | ||
import jakarta.persistence.ConstraintMode; | ||
import jakarta.persistence.Entity; | ||
import jakarta.persistence.FetchType; | ||
import jakarta.persistence.ForeignKey; | ||
import jakarta.persistence.GeneratedValue; | ||
import jakarta.persistence.GenerationType; | ||
import jakarta.persistence.Id; | ||
import jakarta.persistence.JoinColumn; | ||
import jakarta.persistence.ManyToOne; |
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.
단축키로 했습니다.
@ManyToOne(fetch = FetchType.LAZY) | ||
@JoinColumn(name = "created_by", columnDefinition = "bigint(50)", foreignKey = @ForeignKey(value = ConstraintMode.NO_CONSTRAINT)) | ||
@JoinColumn(name = "created_by", columnDefinition = "bigint", foreignKey = @ForeignKey(value = ConstraintMode.NO_CONSTRAINT)) |
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.
제가 초기 설정에 올리고 수정했어야 하는데 여기 created_by
대신에 member_id
가 들어오도록 수정이 필요합니다.
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.
저는 컬럼 의미도 알려주려고 하신 줄 알았습니다.
수정하겠습니다.
public ResponseEntity<ApiResponse> handleBusinessException(BusinessException e, | ||
HttpServletRequest request) { | ||
return ResponseEntity.status(e.getErrorCode().getStatus()) | ||
.body(ApiResponse.of(e.getErrorCode())); | ||
public ResponseEntity<ApiResponse> handleBusinessException(BusinessException e) { | ||
|
||
return ResponseEntityFactory.toResponseEntity(e.getErrorCode()); |
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.
NICE
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.
그 enum type을 다음과 같은 양식으로 통일하는게 좋을거 같아요.. QUESTION_NOT_FOUND
더 나은 의견이 있으시다면 comment로 남겨주세요.
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.
오.. 저의 실수군요.
병룡님이 하신대로 양식 수정했었는데, 에러코드 쪽은 다 안바꿨군요 ㅋㅋ..
//error | ||
QUESTION_NOT_FOUND(2100), | ||
PAGE_NOT_FOUND(2101), | ||
|
||
//success | ||
QUESTION_CREATED(2140), | ||
QUESTION_FOUND(2141), | ||
QUESTION_ALL_FOUND(2142), | ||
QUESTION_UPDATED(2143), | ||
QUESTION_DELETED(2144); |
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.
저번에 찬욱님께서 말씀해주셨던대로 Generic을 T로 받으셔도 돼요! 다른 코드는 T로 받는 것으로 바꿨어요
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.
T로 변경하겠습니다.
assertThat(testFindQuestionResponse.skills()).isEqualTo(testQuestion.getTechStackList() | ||
.stream().map(x -> x.getTechStack().getSkill()).toList()); | ||
//ToDo 답변에 대한 로직이 구현된 후 해당 질문에 대한 답변 list가 잘담기는지 테스트해야 하는지 생각해볼 필요가 있음 | ||
// assertThat(); |
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.
todo를 남겨주신건 좋은데 지금 당장 검증할때 사용하지 않을 주석은 지워주시는게 좋을것 같아요!
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.
LGTM
- 최종 수정
PR을 보내기 전에 확인해주세요!!
관련 이슈
close: #5
개요
상세 내용
API 명세서와 ERD 기준으로 CRUD를 위한 코드 작성과 함께 약간의 코드 리팩토링 진행
테스트는 서비스와 컨트롤러 정상 동작에 대한 것만 작성(에러 상황에 대한 테스트 x)
이 후, 코드 품질이나 다른 도메인 기능 개발에 따른 리팩토링이 필요한 부분은 있음
기능 구현을 위해 다른 도메인이지만 필요한 코드 또는 파일 추가
data.sql을 통해 기능 동작 확인을 위한 최소한의 데이터를 넣음
이 data.sql은 테스트를 할 때 실행시키지 않기 위해 test.resources에 테스트용 application.yml 파일 추가