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

[Feat] 예약 관련 컨트롤러 작성 및 통합 테스트 작성 #6

Merged
merged 22 commits into from
Dec 2, 2024

Conversation

uijin31
Copy link
Collaborator

@uijin31 uijin31 commented Nov 27, 2024

🔗 관련 이슈

Resolves #5

👩🏻‍💻 구현 내용

  • 공통 API 응답을 위한 ApiResult DTO 생성
  • Auth, Booking, Notification 도메인 관련 Rest Controller(XXXApi.class) 작성
  • BookingApi 통합 테스트 작성

💬 코멘트

  • 멘토님께서 "지금 작성한 컨트롤러 통합 테스트를 끝까지 가져갈 수 있는 방법을 고민해 보세요"라고 하셨는데, TDD 방식으로 일단 통합 테스트를 작성한 뒤 이 테스트를 통과하도록 꼼수(?)를 부리고.. 이후 기능 구현 시 이 통합 테스트가 깨지지 않도록 진행하면 될 것 같다는 생각을 했습니다!
  • 시간이 조금 부족해서.. 일단 예약 관련 테스트만 작성했습니다! 추후 결제, 인증, 알림 부분도 작성하겠습니다!

💬 궁금한 점

  • API가 크게 고객을 위한 기능가게 매니저를 위한 기능(ex. 예약 확정)이 나눠져 있는데, 애초에 모듈이나 API를 구분하는 것이 나을까요? 예약 현황 조회 기능 같은 경우에도.. 요청한 사람이 고객인지 가게 매니저인지에 따라 다른 응답을 기대할 것 같아서요!

  • 현재는 예약 요청 후 예약금 결제가 필요하더라도, 일단 예약 상태가 '결제 대기 중'인 예약 리소스가 생성되고, 예약금 조회나 결제 요청을 다시 하도록 API가 분리되어 있는데요! 사실 일반적인 어플에서는 결제가 완료되야 정상적으로 예약이 생성되는 것처럼 보이는데.. 혹시 이 방식에 대해서 의견을 들을 수 있을까요?

  • 결제는 카카오 페이나 네이버 페이를 통해 간편 결제로 구현을 고려 중인데.. 제가 결제 시스템을 개발해 본 적이 없어서 지금 작성한 결제 API가 맞는지 의문이 드네요.. 결제 같은 경우에는 좀 더 공부를 하고 API를 작성하는 것이 나을까요..?

  • 현재 통합 테스트에는 해피 케이스 밖에 없는데요! 예외 케이스도 추가하는게 좋을 것 같은데, 비즈니스적인 예외말고도, 단순히 입력 형식이 올바르지 않은 모든 경우를 다 만드는 것이 좋을까요?

- 관련 의존성끼리 모이도록 정리
- 소셜 로그인 페이지 조회
- 소셜 로그인 요청
- 토큰 재발급
- 가게의 예약 현황 조회
- 테이블 예약
- 예악금 정보 조회
- 예약 정보 상세 조회
- 예약 확정
- 예약 취소
- 테스트가 통과하도록 프로덕션 코드 변경 (Red-Green)
- 테스트가 통과하도록 프로덕션 코드 변경(Red-Green)
- 테스트가 통과하도록 프로덕션 코드 변경(Red-Green)
- placeName: Long → String
- placeDescription: Long → String
- paymentAmount: String → Integer
- 테스트가 통과하도록 프로덕션 코드 변경(Red-Green)
- 테스트가 통과하도록 프로덕션 코드 변경(Red-Green)
- 테스트가 통과하도록 프로덕션 코드 변경(Red-Green)
@uijin31 uijin31 added the ✨ feature New feature or request label Nov 27, 2024
@uijin31 uijin31 requested a review from f-lab-lyan November 27, 2024 09:13
@uijin31 uijin31 self-assigned this Nov 27, 2024
@uijin31 uijin31 changed the title Feat/ #5 create booking scenario api [Feat] 예약 관련 컨트롤러 작성 및 통합 테스트 작성 Nov 27, 2024
implementation 'org.springframework.boot:spring-boot-starter-web'
implementation 'org.springframework.boot:spring-boot-starter-validation'

// Lombok
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lombok gradle plugin?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

d1f3854 반영했습니다:)

* @return 소셜 로그인 페이지 응답
*/
@GetMapping("/oauth/login/{socialType}")
public ResponseEntity<ApiResult<GetLoginPageRes>> getLoginPage(
Copy link
Collaborator

Choose a reason for hiding this comment

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

ResponseEntity는 왜 붙인 것인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ResponseEntity를 사용하면 응답 상태나 http 헤더 부분을 조정할 수 있어서 사용했습니다!
그런데 기본적으로 응답이 200으로 내려가니, 다른 응답 상태가 필요하거나 헤더를 조정할 일이 있을 때만 사용하는 것이 더 깔끔할 것 같네요!
e841cc4 반영했습니다☺️

* @param time 예약 시간
* @param partySize 예약 인원 (default: 1)
*/
@Builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

record 타입의 의도와 builder를 썼을 때, 사람들이 어떻게 받아들일 지에 대해서

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

record는 불변 데이터 객체를 표현하는 자바의 키워드이고, 일반적으로 "해당 클래스가 단순히 데이터를 담는 컨테이너 역할을 함"을 의미합니다!

저는 단순히 저의 입장에서 조금 더 가독성있고, 편하게 BookingReq를 생성하려고 @Builder를 썼습니다! 그런데 제 3자의 입장에서 @Builder를 보게 된다면 _'해당 record를 생성할 때, 필드를 선택적으로 설정할 수 있다'_라는 의미로 받아들여 질 수 있다는 것을 생각하지 못했네요😂

75439b7 반영했습니다!

class BookingApiIntegrationTest {

@Autowired
private MockMvc mockMvc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private MockMvc mockMvc;
private TestRestTemplate testRestTemplate;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

e5d9859 반영했습니다:)

그런데 TestRestTemplate으로 변경하니.. given, when 부분에서 조금 가독성이 떨어지네요..ㅜㅜ 아무래도 응답이 ApiResult<T>로 내려가서 타입 추론을 위한 설정이 필요해진 것도 한몫한 것 같습니다😢 ApiResult를 그냥 없앨까 싶기도 하네요.. 사실 프론트엔드가 없는 플젝이라 장점이 크게 두드러지지 않는 것 같아서요..

Copy link
Collaborator

Choose a reason for hiding this comment

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

의도하지 않은 필드가 들어갔는지 아닌지는 어떻게 테스트할 수 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

json-schema-validator와 같은 JSON 검증 라이브러리를 사용하면, API 응답이 사전에 정의된 JSON 스키마를 따르는지 검증할 수 있습니다! 혹시 이 방법을 의도하신 것이 맞을까요?

import lombok.Builder;

@Builder
public record GetBookingInfoRes(
Copy link
Collaborator

Choose a reason for hiding this comment

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

각각의 필드들을 다 풀어서 쓴 거 좋네요. 중첩된 구조가 더 좋다고들 생각하는데, 사실 완전히 겹치는 내용이 있는게 아니라면, 중첩 구조보다는 풀어서 써주는게 client입장에서 더 도움이 되는 것 같습니다.

Copy link
Collaborator Author

@uijin31 uijin31 Nov 30, 2024

Choose a reason for hiding this comment

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

사실 여쭤보려고 했던 부분입니다:)
이전에 프로젝트를 진행할 때, 프론트엔드에서는 depth가 깊어지지 않는 것이 더 좋다고 하셨는데 사실 인가 보군요!

@RequestBody @Valid BookingReq request
) {
// TODO: 테이블 예약 비즈니스 로직 호출
ApiResult<BookingRes> result = ApiResult.of(
Copy link
Collaborator

Choose a reason for hiding this comment

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

이정도는 전혀 문제 없어보이긴합니다만, 뭐랄까 ... ApiResult가 편한 것 같으면서도 약간 코드양이 늘어나는 부분이 있는 것 같긴하네요. ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞습니다 허허 ApiResult를 도입한게 client 편의성을 위한 것이기도 하고, 개인적으로 응답 포맷이 맞춰져 있는게 더 기분이 좋아서였는데요😂 사실 생산성이 저하되면 안 쓰고 싶기도 해서.. 고민이 되긴하네요😢

Copy link
Collaborator

@f-lab-lyan f-lab-lyan left a comment

Choose a reason for hiding this comment

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

👍 고생하셨어요. 추후 변경사항은 다른 PR로 작업하시면 좋을 것 같습니다!!!

- 기존 의존성은 제거
@uijin31 uijin31 merged commit 6a06bd2 into main Dec 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🎟️ 예약 관련 컨트롤러 작성 및 통합 테스트 작성
2 participants