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: 차량 관리 기능과, 유저의 차량을 등록할 수 있는 기능을 구현한다 #464
Conversation
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.
양이 많았는데 개발하느라 수고하셨어요~
자잘한 부분 몇개 짚었습니당
public static CarsResponse from(List<Car> cars) { | ||
return cars.stream() | ||
.map(CarResponse::from) | ||
.collect(Collectors.collectingAndThen(Collectors.toList(), CarsResponse::new)); |
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.
이게 toList()랑 어떻게 달라여? 궁금
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.
toList()를 한 후에 CarsResponse
Dto로 다시 감싸주는 역할을 합니다~
.stream() | ||
.filter(it -> !isAlreadyExisted(it)) | ||
.map(it -> carRepository.save(Car.from(it.name(), it.vintage()))) | ||
.collect(Collectors.toList()); |
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.
toList()로 바꿔도 되지 않나여?
똑같긴 한데 오늘 블로그보다가 자바 17 얘기가 나왓어서ㅎㅎ
글고 다른 곳에선 또 그렇게 사용하신것같아서요
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.
좋은 지적입니다! 변경할게요
.collect(Collectors.toList()); | ||
} | ||
|
||
@Transactional |
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.
기본적으로 서비스의 CRUD 메서드에는 @Transacrtion 어노테이션을 붙여야하는거군용. read할 때만 readOnly=true로.. 메모..
@@ -38,4 +44,19 @@ public ResponseEntity<FiltersResponse> addMemberFilters(@PathVariable Long membe | |||
List<MemberFilter> memberFilters = memberService.addMemberFilters(memberId, loginMember, filtersRequest); | |||
return ResponseEntity.ok(FiltersResponse.fromMemberFilters(memberFilters)); | |||
} | |||
|
|||
@GetMapping("/me") | |||
public ResponseEntity<MemberCarInfoResponse> findMemberCarInfo(@AuthMember Long loginMember) { |
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.
loginMember를 memberId로 바꾸는 것은 어떤가요?
뭔가 Member를 받아오는 것처럼 보여서요
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.
밑에 리뷰에서 설명하겠습니다!
public record MemberCarInfoResponse(Long memberId, | ||
CarResponse car | ||
) { |
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.
괄호 뒤에 개행해주세요(이게 우리 컨벤션...maybe)
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.
우테코 스타일로 만들 때 계속 정렬하면 바뀌어서 일단 냅둘게요!
다같이 얘기해보고 추후에 바꾸겠습니다
public Car addMemberCar(Long memberId, | ||
Long loginMember, | ||
CarRequest carRequest) { | ||
Member member = findMember(memberId, loginMember); |
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.
memberId의 주인인 멤버는 누구고 loginMember의 멤버는 누군가여
도메인 설명 부탁드립니댜
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.
저번에 대화할 때 restful로 만들기로 해서 이와 같이 만들었습니다.
즉 memberId
는 차량을 등록할 때 어떤 멤버인지를 의미하고,
현재 요청을 보낸 유저는 loginMember
입니다.
즉, 차량을 추가하려는 memberId가 현재 요청보낸 loginId와 같은지 확인해야합니다.
왜냐하면 이렇게 하지 않으면 다른 유저가 다른 유저의 차량을 추가할 수 있기 때문입니다.
memberId를 빼고 loginId만 사용해도 되긴합니다.
/api/members/cars (header => loginId)
하지만 위에서 말씀드린 것처럼 RESTful을 따르기 위해 이렇게 만들었습니다!
위에서 남겨주신 리뷰는 memberId, loginId가 혼재하는 해당 컨트롤러에서 헷갈리지 않도록 구별해서 변수명을 지었습니다.
CarsRequest request = new CarsRequest( | ||
List.of( | ||
new CarRequest("아이오닉5", "2022-A"), | ||
new CarRequest("아이오닉5", "2022-A") | ||
) | ||
); |
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.
이 친구를 fixture로 만들어놓으면 좋을 것 같아요! 다른 곳에서도 쓰이는 듯?
|
||
@Test | ||
void 차량을_제거한다() throws Exception { | ||
doNothing().when(carService).deleteCar(1L, 2L); |
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.
given...!
backend/src/test/java/com/carffeine/carffeine/car/controller/CarControllerTest.java
Show resolved
Hide resolved
@Test | ||
void 차량을_삭제한다() { | ||
// given | ||
Car savedCar = carRepository.save(createCar()); |
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.
얘를 beforeEach에 넣으면 이를 사용하지 않는 테스트에서 문제가 생기려나요?
반복적으로 사용하는 곳이 많아서요
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.
수고하셨습니다.
flyway를 적용했기 때문에 해당 부분만 추가해주시면 감사하겠습니다.
public static Car from(String name, String vintage) { | ||
return new Car(name, vintage); | ||
} |
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.
Builder를 사용할 수도 있었지만, 서비스 레이어에서 지저분해져서 빌더를 사용하지는 않았습니다.
프로젝트의 통일성에 문제가 된다면 추후 리팩토링 진행하겠습니다!
public CarFilter(Car car, Filter filter) { | ||
this.car = car; | ||
this.filter = filter; | ||
} |
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.
이 부분도 위에서와 같이 현재 코드에서 빌더를 사용하는 것보다 깔끔해서 사용했습니다!
|
||
public enum CarExceptionType implements ExceptionType { | ||
|
||
NOT_FOUND_EXCEPTION(Status.NOT_FOUND, 1001, "해당 차량을 찾을 수 없습니다."); |
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.
1001 번 에러는 다른 곳에서 사용중인 것으로 아는데 수정해주셨으면 좋겠습니다~
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.
넵 5000번대로 변경했습니다
Car car = findCar(carId); | ||
carRepository.deleteById(car.getId()); |
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.
find하고 찾을 이유가 있을까요? 그냥 삭제 쿼리만 날려도 될 것 같아서요
원래부터 삭제할 자동차가 없다면 삭제된 거나 마찬가지 일 것 같아서요
.map(it -> filterRepository.findByName(it.name()) | ||
.orElseThrow(() -> new FilterException(FilterExceptionType.FILTER_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.
이 부분 메서드 분리해주시면 안될까요 가독성에 도움이 될 것 같아서요
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.
넵 분리하겠습니다
if (isExistedCar) { | ||
map.remove(id); | ||
this.id--; |
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.
아이디를 --하는 것은 아닌 것 같아요 삭제된다고 id가 줄어들지는 않으니깐요
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.
고생 많으셨어요! 👍
.stream() | ||
.filter(it -> !isAlreadyExisted(it)) | ||
.map(it -> carRepository.save(Car.from(it.name(), it.vintage()))) | ||
.collect(toList()); |
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.
바로 toList 로 해도 좋을 것 같아요
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.
넵!
수고하셨어요!라고 적고 approve버튼이 안보이네.. |
* feat: 차량 등록 기능 추가 * test: 차량 등록 기능 테스트 * feat: 유저의 개인 차량을 등록 및 조회하는 기능 추가 * test: 유저의 개인 차량을 등록 및 조회하는 기능 테스트 * feat: 차량 필터 기능 추가 * test: 차량 필터 기능 테스트 * refactor: 변수명 변경 * fix: 테스트 깨지는 버그 해결 * refactor: 코드 리팩토링 * refactor: 연관관계 매핑 OneToOne으로 변경 * refactor: 불필요한 기능 제거 * refactor: 제약조건 추가
📄 Summary
차량 개인화를 위해 Car 테이블을 만들었습니다. 차량 정보는 어드민 페이지에서 추가할 수 있습니다.
또한 차량에 해당하는 필터를 관리하기 위해 CarFilter 테이블을 만들고, 이는 관리자 페이지에서 적용할 수 있습니다.
마지막으로 유저가 개인의 차량을 등록하고 조회할 수 있는 기능을 구현했습니다.
테이블 구조는 다음과 같습니다.
🕰️ Actual Time of Completion
🙋🏻 More
close #455