Skip to content

Feature/#1357 로드맵 답변 수정 기능 #1364

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

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

nuyh99
Copy link
Contributor

@nuyh99 nuyh99 commented Jul 5, 2023

#️⃣연관된 이슈

연관된 이슈 번호를 모두 작성해주세요

#1357

📝작업 내용

  • 로드맵 관련 큐컴버 테스트 추가
  • 로드맵 답변 수정 기능 추가

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@nuyh99 nuyh99 merged commit c86d537 into develop Jul 5, 2023
@Cl8D Cl8D requested review from Cl8D, java-saeng and amaran-th July 5, 2023 12:45
Copy link
Contributor

@Cl8D Cl8D left a comment

Choose a reason for hiding this comment

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

안녕하세요 연어~
간단하게 리뷰 달았는데 확인 부탁드립니다! 😊

essayAnswerService.deleteEssayAnswer(essayAnswerId, member.getId());
return ResponseEntity.noContent().build();
}

@GetMapping("/quizzes/{quizId}")
public ResponseEntity<QuizResponse> findQuizById(@PathVariable Long quizId) {
public ResponseEntity<QuizResponse> findEssayAnswerById(@PathVariable Long quizId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

quizService에서 정보를 받아와서 QuizResponse를 반환하고 있는데 메서드명을 findEssayAnswerById()로 하신 이유가 있나요?_?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

멍 때리고 한 것 같습니다
반영했습니다!

public ResponseEntity<Void> updateById(@PathVariable Long essayAnswerId,
@AuthMemberPrincipal LoginMember member,
@RequestBody EssayAnswerUpdateRequest request) {
essayAnswerService.updateEssayAnswer(essayAnswerId, request.getAnswer(), member.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 개인적으로 객체 자체를 service로 넘기는 것을 더 선호하는 편인데요! (EssayAnswerUpdateRequest에 대해 변경사항이 발생했을 때 Controller까지 영향을 받는 것을 막기 위함)
단일 인자여도 객체 자체를 넘기는 것이 더 좋지 않을까요?

Copy link
Contributor Author

@nuyh99 nuyh99 Jul 12, 2023

Choose a reason for hiding this comment

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

DTO가 변경됐을 때
컨트롤러가 변경되는 것 VS 서비스가 변경되는 것

위 둘의 차이일 것 같아요!
사실 저도 DTO를 그대로 넘기는 것을 선호하는데, 기존에 만들어둔 서비스의 메서드를 사용하기 위해 저렇게 했습니다.
서비스 메서드도 수정하는 것이 나을 것 같네요!

@Then("답변(들)이 조회된다")
public void 답변이조회된다() {
int statusCode = context.response.statusCode();
assertThat(statusCode).isEqualTo(HttpStatus.OK.value());
Copy link
Contributor

Choose a reason for hiding this comment

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

조회의 경우 상태 코드 말고 어떤 응답값이 내려왔는지도 함께 검증해야 하지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When이나 Given을 사용할 때 넣는 파라미터에 따라서 응답값의 바디가 달라집니다
따라서 범용적인 Then 절로 쓰기 힘듭니다

현재는 위와 같이 인수 테스트를 작성했기 때문에 Given, When을 정해진 데이터로 수행하도록 바꾸거나
현재 조회될 내용을 어딘가 저장해두거나 전달 해야할 것 같습니다.

그리고 서비스 레이어의 테스트에서 검증이 되고 있고, 컨트롤러에서는 단순 호출만 하니까 상태코드로도 충분하다고 생각해요!!


@When("{long}번 퀴즈에 대한 답변들을 조회하면")
public void 퀴즈에대한답변을들조회하면(final long quizId) {
context.invokeHttpGet("/quizzes/" + quizId + "/essay-answers");
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 마찬가지로 조회이기 때문에 응답 값 정도는 함께 검증해야 될 것 같습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants