코드리뷰 PR #62
코드리뷰 PR #62
Conversation
공통 파일 생성
[/api/annual?page=int.class] 모든 연차 조회 컨트롤러 테스트 - 정상 응답 에러 수정
올바른 요청에도 적절한 응답을 보내지 못함 -> 분기문 수정
올바른 요청에도 적절한 응답을 보내지 못함 -> 분기문 수정
회원 검색 쿼리 구현
매일 자정 상태 변경 및 연차 갯수 수정
스케줄러를 활용한 상태 변경
빌드 실패로 인한 테스트 수정
1차 테스트 작성
테스트 패키지 변경
|
안녕하세요. 5조 admin 연차 당직 담당입니다. 이곳에 다는것이 맞을까요..? |
안녕하세요! 리뷰 남겨주셔서 감사합니다😍 |
|
|
||
| private final AdminRepository adminRepository; | ||
|
|
||
| public Admin login(HttpServletRequest request, AdminLoginRequestDto loginRequestDto) { |
There was a problem hiding this comment.
HttpServletRequest와 관련된 로직은 컨트롤러 레이어에서 처리를 진행하는 게 어떠신지 조심스레 리뷰 남기고 갑니다.
현재 세션에 어드민 정보를 저장하는 로직이 서비스 레이어에서 진행되고 있는데 역할을 분리시키면 코드가 조금 더 좋은 방향성을 가지게 될 것 같아요.
이유
- 클라이언트의 요청과 직접적으로 연관이 있는 웹 레이어에서 처리하는 것이 바람직 함.
HttpServletRequest를 목 오브젝트로 치환하는 과정에서 어려움 발생, 비즈니즈 로직에서HttpServletRequest를 직접적으로 사용하면 단위테스트에 어려움이 있진 않으셨나요??
그런데 리뷰를 적다보니, 어드민 로직 쪽에서도 이런 걸 신경써야하는지 의문이긴 하네요.
어쩌면 기능이 별로 없는 관리자 페이지이기 때문에 한곳에 코드를 몰아 넣는 것이 유지보수가 용이할지도
There was a problem hiding this comment.
항상 컨트롤러 단에서 어떤 부분을 신경써야될 지 애매했는데, 웹 레이어라고 생각하니까 조금 편하네요. 좋은 의견 감사합니다!
Request 테스트에서는 MockRequest라는 부분이 있어서 해당 목 객체 내부에 세션을 직접 할당하고 테스트하는 방식으로 했던 것 같아요!
| result | ||
| .andExpect(status().isOk()) | ||
| .andExpect(content().contentType(MediaType.APPLICATION_JSON)) | ||
| .andExpect(jsonPath("$.statusCode").exists()) | ||
| .andExpect(jsonPath("$.message").exists()) | ||
| .andExpect(jsonPath("$.data").doesNotExist()); |
|
|
||
| @Transactional | ||
| public UpdateAnnualResponseDto update(Long annualId, UpdateAnnualRequestDto updateAnnualRequestDto) { | ||
| Annual annual = get(annualId); |
There was a problem hiding this comment.
여기서 get(annualId) -> getAnnual(annualId) 이렇게 메서드를 호출하는 게 조금 읽기 어렵다고 느껴지네요!
get()이 getAnnual() 필연적으로 호출해야하는 형식인데 하나로 통합시켜도 좋을 거 같아요
There was a problem hiding this comment.
이 부분도 이번 프로젝트 같이 하면서 배우고 싶습니다. 서비스 단에서 코드를 통일화 시키는 부분이 조금 명확하지 않더라구요. 코드를 한번 다시 살펴보겠습니다. 감사합니다!
No description provided.