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] 약속 잠금 해제 기능 추가 #127

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Conversation

ikjo39
Copy link
Contributor

@ikjo39 ikjo39 commented Jul 31, 2024

관련 이슈

작업 내용

  • 약속 잠금 해제 기능을 구현하였어요 :)

@ikjo39 ikjo39 added 🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :) labels Jul 31, 2024
@ikjo39 ikjo39 added this to the 3차 데모데이 milestone Jul 31, 2024
@ikjo39 ikjo39 self-assigned this Jul 31, 2024
Copy link

github-actions bot commented Jul 31, 2024

Test Results

65 tests   65 ✅  4s ⏱️
15 suites   0 💤
15 files     0 ❌

Results for commit 64dfca4.

♻️ This comment has been updated with latest results.

seunghye218
seunghye218 previously approved these changes Jul 31, 2024
Copy link
Contributor

@seunghye218 seunghye218 left a comment

Choose a reason for hiding this comment

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

기능의 중요한 동작들을 꼼꼼히 테스트 했네요👍

Copy link
Contributor

@ehBeak ehBeak left a comment

Choose a reason for hiding this comment

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

코드를 읽기 쉽게 작성해 주셔서 직관적으로 파악하기 좋았어요 😊
간단하게 코멘트만 남겨요! 고생 많으셨습니다~

Comment on lines 226 to 234
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");
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.

배키 의견에 동의합니다! 관련 로직 메서드 분리하도록 할게요 :)

seunghye218
seunghye218 previously approved these changes Aug 1, 2024
Copy link
Contributor

@seunghye218 seunghye218 left a comment

Choose a reason for hiding this comment

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

배키의 리뷰를 적용한 뒤 한걸음 더 간결해졌네요. 저도 테스트에서 토큰 얻는 코드가 중복되고 있었는데 참고해야겠네요~

seokmyungham
seokmyungham previously approved these changes Aug 1, 2024
Copy link
Contributor

@seokmyungham seokmyungham left a comment

Choose a reason for hiding this comment

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

다온 고생하셨습니다 🌞

ehBeak
ehBeak previously approved these changes Aug 1, 2024
Copy link
Contributor

@ehBeak ehBeak left a comment

Choose a reason for hiding this comment

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

🎉🎉

Copy link
Member

@hw0603 hw0603 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 다온!
코멘트 몇 가지 남겨 두었어요~

Comment on lines 90 to 93
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));
Copy link
Member

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);
Copy link
Member

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를 반환한다.")
Copy link
Member

Choose a reason for hiding this comment

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

위에 언급한 것과 같이 400 Bad Request는 어떨까요?

Copy link
Contributor

@seunghye218 seunghye218 left a comment

Choose a reason for hiding this comment

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

상황에 맞는 예외 코드가 잘 적용됐네요 👍

Copy link
Contributor

@ehBeak ehBeak left a comment

Choose a reason for hiding this comment

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

굿굿👍

@ikjo39 ikjo39 merged commit 6440e0b into develop Aug 1, 2024
14 checks passed
@ikjo39 ikjo39 deleted the feat/120-add-unlock-meeting branch August 1, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BE] 약속 잠금 해제 기능을 구현해요 :)
5 participants