-
Notifications
You must be signed in to change notification settings - Fork 8
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] 약속 잠금 해제 기능 추가 #127
[BE] 약속 잠금 해제 기능 추가 #127
Conversation
Test Results65 tests 65 ✅ 4s ⏱️ Results for commit 64dfca4. ♻️ This comment has been updated with latest results. |
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.
코드를 읽기 쉽게 작성해 주셔서 직관적으로 파악하기 좋았어요 😊
간단하게 코멘트만 남겨요! 고생 많으셨습니다~
AttendeeLoginRequest request = new AttendeeLoginRequest(attendee.name(), attendee.password()); | ||
|
||
String token = RestAssured.given().log().all() | ||
.contentType(ContentType.JSON) | ||
.body(request) | ||
.when().post("/api/v1/meetings/{uuid}/login", meeting.getUuid()) | ||
.then().log().all() | ||
.statusCode(HttpStatus.OK.value()) | ||
.extract().jsonPath().getString("data.token"); |
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.
배키 의견에 동의합니다! 관련 로직 메서드 분리하도록 할게요 :)
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.
다온 고생하셨습니다 🌞
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.
수고하셨습니다 다온!
코멘트 몇 가지 남겨 두었어요~
Meeting meeting = meetingRepository.findByUuid(uuid) | ||
.orElseThrow(() -> new MomoException(MeetingErrorCode.NOT_FOUND_MEETING)); | ||
Attendee attendee = attendeeRepository.findByIdAndMeeting(id, meeting) | ||
.orElseThrow(() -> new MomoException(AttendeeErrorCode.NOT_FOUND_ATTENDEE)); |
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
값을 보니 404를 내리는 것 같은데, 400이 더 적절할 것 같아요!
.then().log().all() | ||
.statusCode(HttpStatus.OK.value()) | ||
.extract().jsonPath().getString("data.token"); | ||
String token = getToken(attendee, meeting); |
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.
메서드 추출 👍
.statusCode(HttpStatus.OK.value()); | ||
} | ||
|
||
@DisplayName("존재하지 않는 약속을 잠금 해제 시도하면 404 Not Found를 반환한다.") |
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.
위에 언급한 것과 같이 400 Bad Request
는 어떨까요?
64dfca4
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.
굿굿👍
관련 이슈
작업 내용