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

⭐️3단계 - 즐겨찾기 기능 구현 #462

Open
wants to merge 16 commits into
base: boradol
Choose a base branch
from

Conversation

boradol
Copy link

@boradol boradol commented Apr 17, 2023

안녕하세요. 리뷰어님.
교육기간이 끝났음에도 리뷰 해주셔서 감사합니다.

지난번에 리뷰주신부분 반영 및 즐겨찾기 기능도 구현하였습니다.

이번에도 리뷰 잘 부탁드립니다. 🙏🏻

1. member와 연관관계 생각.
2. source와 target은 Station과 연관관계 생각.
1. 즐겨찾기 추가 test 코드 작성
2. 즐겨찾기 추가 시 유효하지 않은 토큰에 대한 test 코드 작성
1. 즐겨찾기 리스트 조회하기 API작성
2. controller ResponseEntity 변경
1. 즐겨찾기 리스트 조회하기 API작성
2. controller ResponseEntity 변경
1. 즐겨찾기 리스트 조회 인수테스트 작성
2. 즐겨찾기 삭제 인수테스트 작성
Copy link

@Flamme1004K Flamme1004K 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 +55 to +65
## 요구사항
### 기능 요구사항
>- [x] 요구사항 설명에서 제공되는 추가된 요구사항을 기반으로 즐겨 찾기 기능을 리팩터링하세요.
> - [x] 생성
> - [x] 조회
> - [x] 삭제
>- 추가된 요구사항을 정의한 인수 조건을 도출하세요.
> - [x] 내 정보 관리 / 즐겨 찾기 기능은 로그인 된 상태에서만 가능
> - [x] `FavoriteAcceptanceTest` 인수 테스트 만들기
> - 예외 케이스에 대한 검증도 포함하세요.
> - [x] 로그인이 필요한 API 요청 시 유효하지 않은 경우 401 응답 내려주기

Choose a reason for hiding this comment

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

요구사항 정리 👍


@Transactional
public Favorite create(String email, FavoriteRequestDto favoriteRequestDto) {
Member member = findMember(email);

Choose a reason for hiding this comment

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

인증이 된 member를 전달한다면 findMember가 없어도 되지 않을까요? 🤔

}

private boolean existsByIdAndMember(Long id, Member member) {
return favoriteRepository.existsByIdAndMember(id, member);

Choose a reason for hiding this comment

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

exist가 아닌 findById를 통하여 favorite을 찾고, 찾는 favorite의 memberId가 같은지 틀린지 validation을 해주는 건 어떨까요?

Comment on lines +15 to +25
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "member_id")
private Member member;

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "source_id")
private Station source;

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "target_id")
private Station target;

Choose a reason for hiding this comment

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

직접 참조가 아닌 간접 참조로 변경해볼까요?

변경해보고 차이점을 알려주세요!

@DisplayName("즐겨찾기를 추가에 성공한다.")
void addFavorite() {
// when
ExtractableResponse<Response> response = 즐겨찾기_추가_요청(accessToken, 강남역, 판교역);

Choose a reason for hiding this comment

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

타입추론을 사용해볼까요?

*/
@EmptySource
@ValueSource(strings = {"abc", "123"})
@ParameterizedTest

Choose a reason for hiding this comment

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

@ParameterizedTest 사용 좋습니다 👍

Comment on lines +87 to +103
@Test
@DisplayName("해당 유저의 즐겨찾기를 목록을 조회한다.")
void getFavoriteBydUserId() {
// given
즐겨찾기_추가_요청(accessToken, 강남역, 판교역);
즐겨찾기_추가_요청(accessToken, 양재역, 판교역);

// when
ExtractableResponse<Response> response = 즐겨찾기_목록_조회_요청(accessToken);

// then
assertAll(
() -> assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value()),
() -> assertThat(response.jsonPath().getList("source.id", Long.class)).isEqualTo(Arrays.asList(강남역, 양재역)),
() -> assertThat(response.jsonPath().getList("target.id", Long.class)).isEqualTo(Arrays.asList(판교역, 판교역))
);
}

Choose a reason for hiding this comment

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

인수테스트 훌륭하네요!

import static org.junit.jupiter.api.Assertions.assertAll;

@DisplayName("즐겨찾기 테스트")
class FavoriteTest {

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