[FU-191] 기본 스케줄 관리 #89
Conversation
| } | ||
|
|
||
| private void validateScheduleTime(LocalTime startTime, LocalTime endTime) { | ||
| if (startTime.isAfter(endTime)) { |
There was a problem hiding this comment.
여기에 시작 시간이 종료 시간과 일치하는 것도 걸리나?? 휴무일의 경우에는 디폴트 운영 시간으로 갖고 있게 처리해 준 것 같아서 시간 시간이 종료 시간과 같으면 유효하지 않은 걸로 봐도 괜찮지 않을까 싶어!
(+ 입력창 자체에서 시작 시간 < 종료 시간으로만 선택되게 만들고 있긴 한데, 이건 PR 때 의견 들어보고 뺄 수도 있을 것 같긴 하당)
There was a problem hiding this comment.
시작시간과 종료시간이 같은 경우일 때도 추가해놓을게! 그리고 입력창 자체에서도 시작 시간 < 종료 시간 이렇게 되도록 검증해주면 좋을 것 같아! 빼게 되면 요청을 받고 나서야 잘못 작성한 걸 인지할테니까 입력하자마자 검증해줘서 바로 수정할 수 있게 해주면 사용성에 동움이 될 것 같아!
| public void createDefaultSchedule(Member photographer) { | ||
| for(DayOfWeek dayOfWeek : DayOfWeek.values()) { | ||
| BaseSchedule baseSchedule = BaseSchedule.builder() | ||
| .photographer(photographer) | ||
| .dayOfWeek(dayOfWeek) | ||
| .startTime(DEFAULT_START_TIME) | ||
| .endTime(DEFAULT_END_TIME) | ||
| .operationStatus(OperationStatus.ACTIVE) | ||
| .build(); | ||
|
|
||
| baseScheduleRepository.save(baseSchedule); | ||
| } | ||
| } | ||
|
|
||
| public void deleteBaseSchedule(Member photographer) { | ||
| List<BaseSchedule> baseSchedules = baseScheduleRepository.findByPhotographerId(photographer.getId()); | ||
| baseScheduleRepository.deleteAll(baseSchedules); | ||
| } |
There was a problem hiding this comment.
(이건 궁금해서 달아놓는 거!) createDefaultSchedule의 로직 일부는 나중에 시간 단위 변경시 기본 일정 초기화할 때도 재사용될 수 있을 것 같다는 생각이 들었는데 보다 보니까 이건 아예 가입했을 때 새롭게 만드는 역할인 것 같네! 초기화의 경우에 deleteBaseSchedule > createDefaultSchedule 같은 식으로 기존 함수를 활용하게 되나…?? 아님 성격이 좀 달라서 그냥 기존에 만든 스케줄을 디폴트 스케줄로 업데이트하는 로직을 따로 쓰는 게 나은가 🤔
There was a problem hiding this comment.
일단 생성하는 것과 업데이트하는 것에는 성격이 달라서 재사용할 생각은 안했던 것 같아! 말해준 방식도 생각해보면 좋을 것 같기는 한데 예를 들어, 기존의 스케줄에 토,일을 말해준 deleteBaseSchedule > createDefaultSchedule 이 방법을 채택하게 되면 현재 레코드들은 월~일이 순차적으로 DB에 쌓이게 되는데 삭제 후에 생성을 하게되면 순차적이었던 요일 7개가 5개 + 2개로 흩어져서 기록돼서 인덱스로 조회할 때 약간의 성능저하가 될 수도 있을 것 같고..!? (삭제하고 생성할 때 발생하는 I/O 연산량이 업데이트보다 많아지는 그런 면에서..!)
그래서 현재 로직으로 유지하는게 좋은 것 같아! 고민해볼만한 리뷰여서 좋다.👍
There was a problem hiding this comment.
@yuseok0215 코멘트 읽어보는데 삭제 후에 생성을 하게되면 순차적이었던 요일 7개가 5개 + 2개로 흩어져서 기록 이 부분이 이해가 안가네..! 5개 + 2개로 흩어지는건 어떤거야?
There was a problem hiding this comment.
회원가입을 하면 DB 레코드 상에 월요일부터 일요일까지 baseSchedule 7개가 생성이 돼! 여기서 유진이가 말해준 방식대로 토,일의 스케줄을 변경할 때 (삭제 후 생성) deleteBaseSchedule > createDefaultSchedule 방식을 사용하면 연속적으로 저장되어 있던 레코드에는 월,화,수,목,금에 대한 기본 스케줄이 남고 AUTO_INCREMENT에 의해 맨 마지막인 가장 최근 레코드로 토,일 기본 스케줄이 생성될거야!
이렇게 월금 따로 토일 따로 레코드가 존재하는 인덱스 파트가 분리되면서 조회 성능에 저하가 있을 것 같다는 의견이었어!
| public ResponseEntity<ResponseBody<Void>> updateScheduleUnit(@RequestBody ScheduleUnitDto request, @AuthenticationPrincipal MemberAdapter memberAdapter) { | ||
|
|
||
| Member photographer = memberAdapter.getMember(); | ||
| baseScheduleService.updateScheduleUnit(photographer.getId(), request); |
There was a problem hiding this comment.
나중에 추가할 예정인가 싶기도 한데 기존 단위와 바꾸려는 단위가 같으면 에러 던지는 것도 괜찮을 것 같아! 서비스 상에서 지금 설정된 것과 같은 단위로 요청을 보낼 수 있게 되어 있지는 않지만 혹시나 그런 상황이 발생하면 불필요하게 스케줄이 초기화되어 버릴 것 같아서 😓
There was a problem hiding this comment.
추후에 업데이트에 관련된 내용들은 한번에 추가 구현하려고 변경로직 이외에는 넣지 않았는데, 일단 유리가 개발한 부분 제외하고 이번 PR에서 구현해두고 유리가 나중에 리베이스해서 PR올리는 방법도 있을 것 같아서 시간될 때 말한 부분 포함해서 구현해둘게!
티켓만들기에 볼륨이 적어서 내 도메인 쪽에서 구현할 것들 마저해서 PR 병합하고 나서 유리가 리베이스해서 마무리하는 것도 좋을 것 같아! |
| private void initializeScheduleInfo(Member member, Member photographer) { | ||
| baseScheduleService.createDefaultSchedule(photographer); | ||
| member.initializeScheduleUnit(); | ||
| memberRepository.save(member); | ||
| } | ||
|
|
There was a problem hiding this comment.
멤버 객체는 photographer만 사용하면 될 것 같아보여! 👀
save 호출도 따로 안해도 괜찮을 것 같구!
| List<BaseScheduleDto> baseScheduleDtoList = new ArrayList<>(); | ||
| for (BaseSchedule baseSchedule : baseSchedules) { | ||
| convertBaseScheduleDto(baseSchedule, baseScheduleDtoList); | ||
| } |
There was a problem hiding this comment.
빈 어레이리스트 생성 + for문 순회하면서 리스트에 요소 추가 이 부분도 convertBaseScheduleDto 메서드 안에 포함되어도 괜찮지 않을까 싶어!
There was a problem hiding this comment.
오 그렇네! for문 자체를 분리해줄 생각은 못했던 것 같아 짚어줘서 고마워! 같은 클래스에 비슷한 구조로 작성된 부분도 같이 수정해볼게👍
|
|
||
| @PutMapping("/schedule/base") | ||
| public ResponseEntity<ResponseBody<Void>> updateBaseSchedule(@AuthenticationPrincipal MemberAdapter memberAdapter, | ||
| @RequestBody List<BaseScheduleDto> request) { |
There was a problem hiding this comment.
요청 dto는 BaseScheduleRequest 혹은 BaseScheduleUpdateRequest 이런식으로 네이밍 하기로 했던 것 같은데 Dto를 붙인 이유가 있는지 궁금해!
There was a problem hiding this comment.
요청과 응답, 두 경우에서 모두 쓰일 때는 어떤 하나로 명명하기 모호해서 Dto로 명명하기로 했던걸로 알고 있어서 이렇게 작성했어!
| public void updateScheduleTime(LocalTime startTime, LocalTime endTime, OperationStatus operationStatus) { | ||
| if (operationStatus == OperationStatus.INACTIVE) { | ||
| this.startTime = LocalTime.of(9,0,0); | ||
| this.endTime = LocalTime.of(18,0,0); | ||
| } else { |
There was a problem hiding this comment.
회원가입 할 때 기본스케줄을 9to6으로 초기화하는 건 괜찮지만, 추후에 사용자가 특정요일에 대해 예약을 받지 않으려고 비활성화 하는 시점에서도 이렇게 9to6으로 세팅하는게 괜찮으려나? 이렇게 하면 결국 비활성화 해도 9to6 동안 예약이 열리게 되는게 아닌지 싶어서 물어봐
There was a problem hiding this comment.
이 부분은 전에 내가 부탁했던 부분이라 코멘트 달아놔! 비활성화할 때 스케줄은 00:00~00:00 같은 임의의 값이나 디폴트 스케줄로 저장되면 좋겠다고 했었는데 (이유는 슬랙 12/11 스레드에 언급되어 있어! 💭) 디폴트 스케줄로 저장되는 게 좀 더 편하긴 해서! 😓
프론트에서는 OperationStatus를 먼저 보고 비활성화되어 있다면 영업 시간은 무시하는 방식으로 하는 중이고, 이렇게 초기화하면 백엔드에서도 검증할 때 같은 방식으로 해야 할 것 같은데 요게 부적절할 것 같다면 다른 값으로 저장해도 괜찮아!
There was a problem hiding this comment.
아~ OperationStatus만 보고 결정해도 되니까 예약이 활성화되는 문제는 고민하지 않아도 되겠다!
디폴트 스케줄로 저장되는게 편하면 이 방식이 맞는 것 같아 ㅎㅎ 코멘트 고마오
| LocalTime startTime = baseScheduleDto.getStartTime(); | ||
| LocalTime endTime = baseScheduleDto.getEndTime(); | ||
|
|
||
| validateScheduleTime(startTime, endTime); |
There was a problem hiding this comment.
만약 기본 스케줄 단위를 1시간으로 설정하게 되면 UI 상에서는 시작시간이나 종료시간을 선택할 때 한시간 단위로만 선택 가능하게끔 보여주긴 하지만 (ex 10:00, 11:00, 12:00 ... ) 혹시나 모를 요청에 대비해서 startTime, endTime이 정시 혹은 30분 단위가 맞는지 검증해 줄 필요는 없으려나?
예를들면 시작시간이 10시 12분 이런식으로 요청이 들어올 경우에 대해 생각해 보자는거지! 물론 이런식으로 생각하면 검증해야 할 게 끝이 없긴 하지만😂 이런 부분에 대한 너 생각이 궁금해서 코멘트 남겨!
There was a problem hiding this comment.
말해준 부분을 서비스 로직에서 검증해준다면 아마 Member에서 ScheduleUnit을 조회해서 비교해볼텐데 현재 프론트에서 그 값을 조회해서 스케줄 단위를 띄워주는거라 결국 검증해주는 것과 동일한 역할을 현재 하고 있는 것 같아!
만약에 프론트쪽에서 조회해서 검증에 필요할 상태값을 가지고 있지 않는 상황에서는 검증이 필요할 것 같아보여! 역시 꼼꼼해... 충분히 생각해보고 넘어갈만한 부분이었다👍👍
(읽어보고 다른 의견 있으면 남겨줘!)
There was a problem hiding this comment.
오호 이해 완료!!💡지금은 이 정도로도 괜찮을 것 같네 ㅎㅎ 🙌
There was a problem hiding this comment.
여기도 마찬가지로 ScheduleUnitResponse 등으로 네이밍하지 않은 이유가 있을까?
There was a problem hiding this comment.
이것도 위에 코멘트와 같은 이유로 요청과 응답에 모두 사용되는 객체라서 Dto로 명명했어!
| if (photographer.getScheduleUnit() == scheduleUnitDto.getScheduleUnit()) { | ||
| throw new RestApiException(ScheduleErrorCode.CANNOT_CHANGE_SAME_SCHEDULE_UNIT); | ||
| } |
There was a problem hiding this comment.
이 경우에 예외를 던지는게 적합한가에 대한 고민이 드네🧐
클라이언트 측에 예외의 세부적인 내용(=변경사항이 없으므로 아무런 업데이트를 진행하지 않음)을 알릴 필요가 있는지, 아니면 이 예외를 알리는게 클라이언트 측에 어떤 행동 변화를 유발하지 않는 사항이니까 예외보다는 차라리 return을 해버리고 200 ok를 응답하도록 해도 상관이 없을지 이런 측면에서!
There was a problem hiding this comment.
좋은 의견이야! 기존에 스케줄 단위가 30분으로 설정되어 있는 상황에서 30분으로 요청을 보내더라도 스케줄자체가 변경되지 않아서 말해준대로 어떤 반응도 일어나지 않으면 되는거라 if (photographer.getScheduleUnit() == scheduleUnitDto.getScheduleUnit()) 이 경우에는 그냥 리턴해주면 되겠다!
그리고 아마 클라이언트단에서도 기존의 스케줄 단위를 가지고 있을거라서 블락처리가 되어있을 것 같기도 하네! 그러면 에러메세지가 더더욱 없어도 될 것 같다!
| List<BaseSchedule> baseScheduleList = baseScheduleRepository.findByPhotographerId(photographer.getId()); | ||
| for (BaseSchedule baseSchedule : baseScheduleList) { | ||
| baseSchedule.initializeSchedule(); | ||
| } |
| public void createDefaultSchedule(Member photographer) { | ||
| for(DayOfWeek dayOfWeek : DayOfWeek.values()) { | ||
| BaseSchedule baseSchedule = BaseSchedule.builder() | ||
| .photographer(photographer) | ||
| .dayOfWeek(dayOfWeek) | ||
| .startTime(DEFAULT_START_TIME) | ||
| .endTime(DEFAULT_END_TIME) | ||
| .operationStatus(OperationStatus.ACTIVE) | ||
| .build(); | ||
|
|
||
| baseScheduleRepository.save(baseSchedule); | ||
| } | ||
| } | ||
|
|
||
| public void deleteBaseSchedule(Member photographer) { | ||
| List<BaseSchedule> baseSchedules = baseScheduleRepository.findByPhotographerId(photographer.getId()); | ||
| baseScheduleRepository.deleteAll(baseSchedules); | ||
| } |
There was a problem hiding this comment.
@yuseok0215 코멘트 읽어보는데 삭제 후에 생성을 하게되면 순차적이었던 요일 7개가 5개 + 2개로 흩어져서 기록 이 부분이 이해가 안가네..! 5개 + 2개로 흩어지는건 어떤거야?
|
이미 completePhotographerSignup()메서드에서 photographer에 대한 JPA 영속화가 진행되어 더티 체킹이 일어나서 없어도 저장가능
나는 보통 public 메서드는 api 요청으로부터 시작되니까 api 1개 = 하나의 단위 라고 생각하고, 이 단위 안에서 엣지 케이스를 나누는 테스트케이스를 많이 고려했던 거 같아! public 메서드가 호출하는 private 메서드가 검증/예외처리 등 엣지 케이스 판별에 중요한 역할을 하고 있다면 private 메서드가 유발하는 결과를 테스트케이스에서 사용하는 식으로..? 말로 하려니까 어려운 것 같은데 예를 들면 이 프라이빗 메서드를 퍼블릭으로 바꿔서 테스트코드에서 직접 호출해서 검증하기 보다는 그리고 '상품등록' 메서드가 호출하는 S3에 이미지 저장하는 private 메서드를 테스트할 때는 (근데 사실 나도 테스트에 대해 깊이 아는게 아니라서 .. 꼭 맞는 말은 아닐수도 있어 ㅎㅎ;) |
자세하게 설명해줘서 너무 고마워🥹 기존에 나도 Service클래스에 있는 private를 어떻게 테스트하면 좋을지에 대한 고민이 있었는데 상세한 케이스 분류를 통해서 public 메서드를 통해 private 메서드를 검증하는 방법이 있을 수 있겠네! 충분히 근거있는 좋은 접근법인 것 같아!👍👍 |
체크리스트
작업 내역
고민한 사항
updateBaseSchedule()메서드의 테스트 코드를 작성하기는 쉽지 않았습니다. 세팅해줘야 할 값도 그만큼 많아지기 때문에 메서드 분리를 통해 순회하는 내부 로직만 테스트하고자 했습니다. 이때 보편적으로 메서드를 분리하게 되면 해당 클래스에서 private 접근제어자를 통해 메서드를 생성하게 되는데, private 레벨에서는 테스트 코드를 작성할 수 없어서 (1)public으로 변경할 지, 아니면 (2)새로운 클래스를 만들어 해당 메서드를 public 접근제어자로 정의하고 기존의 Service 클래스에 새로운 클래스를 주입시키는 방법 두 가지 중에 고민을 하였습니다. 일단 (1)의 방법으로 적용시켰지만 테스트를 용이하게 하기 위해 클래스 분리를 하는 것이 좋을지 고민입니다. (클래스를 분리하는 과정에서 비슷한 목적의 메서드들을 분리할 수 있다면 더 좋은 선택이 될 수 있을 것 같습니다.)리뷰 요청사항