feat: 예약 조기종료 기능 구현 #969

Choi-JJunho · 2023-07-31T07:23:55Z

구현 기능

(2023.08.07 업데이트)

기존에 예약을 종료하는 흐름에서 조기종료가 가능하도록 수정했습니다
조기종료 시 다음과 같은 내용이 추가되었습니다.
- 예약 조기종료 시 5분 단위의 가장 가까운 과거 시간으로 종료시간이 기록된다.
- 사용중인 예약에 대해서만 조기종료가 수행됩니다. (검증 추가)

기타

조기 종료에 대한 코드에서 현재 시간을 기준으로 종료시간이 업데이트되고 있습니다.
위 로직을 어떻게 테스트 해야하는지 감이 잘 오지 않네요 😅

Close [ALL] 예약 조기종료 기능 구현 #966

sakjung

고생 많으셨습니다~
리뷰 남겼는데 확인 한 번 부탁드립니다

sakjung · 2023-07-31T09:44:54Z

backend/src/main/java/com/woowacourse/zzimkkong/domain/ServiceZone.java

@@ -1,7 +1,9 @@
 package com.woowacourse.zzimkkong.domain;

 public enum ServiceZone {
-    KOREA("Asia/Seoul");
+    KOREA("Asia/Seoul"),
+    UTC("UTC"),


UTC 를 따로 만드신 이유가 있을까요?

추가로 ServiceZone 은 말 그대로 찜꽁이 서비스되고 있는 곳을 나타내는 ENUM 입니다. Timezone 과 헷갈리실 수 있지만 국가의 이름이 되는게 더 직관적일 것 같습니다. 현재는 한국에서만 서비스 되고 있는 찜꽁이므로 KOREA 만 들어가있는 상태입니다.

앗 초기에 테스트를 이해하는 과정에서 UTC 가 별도로 필요할 것 같다는 생각에 추가했었는데 이후 이해 과정에서 해당 내용이 필요없게 되었습니다!

말씀해주신 내용 이해했습니다!
UTC 필드를 제외하는 방향으로 리팩터링 진행하겠습니다.

sakjung · 2023-07-31T09:46:10Z

backend/src/test/java/com/woowacourse/zzimkkong/service/GuestReservationServiceTest.java

+    @DisplayName("예약 삭제 요청 시, 사용중인 예약이면 현재 시간을 기준으로 조기종료 된다.")
+    void deleteUsingReservation() {
+        //given, when
+        TimeZone.setDefault(TimeZone.getTimeZone("UTC"));


JVM 시간을 별도로 다시 UTC로 세팅하시는 이유가 있을까요?

이 내용 또한 초기에 테스트를 이해하는 과정에서 생긴 부산물(?)입니다 ㅠㅜ
수정하겠습니다

sakjung · 2023-07-31T09:52:21Z

backend/src/test/java/com/woowacourse/zzimkkong/Constants.java

@@ -22,6 +22,7 @@ private Constants() {
    public static final String ORGANIZATION = "우아한테크코스";
    public static final String POBI = "포비";
    public static final String SAKJUNG = "삭정";
+    public static final LocalDate TODAY = LocalDate.now();


test 에서 now 를 바로 쓰지 않는 컨벤션이 있습니다.
현재시간 이전, 이후에 대한 검증이 필요한 로직 관련 테스트에서 문제가 생길 수 있기 때문입니다.
그래서 보시면 THE_DAY_AFTER_TOMORROW 를 기준으로 모든 테스트가 작성되어있는데, 참고 부탁드립니다.

컨벤션이였군요..!! 미처 확인하지 못했던 것 같네요 😓

일단 THE_DAY_AFTER_TOMORROW를 사용하는 방향으로 리팩터링을 진행하겠습니다.

질문

말씀하신 내용 중 현재시간 이전, 이후에 대한 검증이 필요한 로직 관련 테스트에서 문제가 생길 수 있다에 대해 명확하게 이해되지 않아서 어떤 부분에서 문제가 발생할 수 있는지 여쭤봐도될까요?!

현재 이해가 잘 안되는 이유는 다음과 같습니다.

현재 시간을 기준으로 테스트를 수행하였을 때 문제가 생긴다면 현재 정의되어있는 THE_DAY_AFTER_TOMORROW에서도 동일하게 문제가 발생하는게 아닌가요?

public static final LocalDate THE_DAY_AFTER_TOMORROW = LocalDate.now().plusDays(2);

또한 결국 테스트를 작성함에 있어 THE_DAY_AFTER_TOMORROW.minusDays(1) 과 TODAY.plusDays(1)의 차이를 잘 모르겠습니다.

저희 첫 기획 시 서비스 요구사항 중, 현재 시간보다 과거의 예약은 생성할 수 없다 (단, 맵의 매니저 (관리자) 는 가능) 는 요구사항이 있었습니다. 해당 요구사항 때문에 테스트를 짜면서 번거로운 부분이 많았습니다.

이해를 도와드리기 위해 간단한 예를 들면, 2023-08-02 (오늘) 13:00 에 예약 생성 로직이 포함되는 어떤 테스트를 돌린다고 가정했을 때 2023-08-02 10:00 ~ 12:00 예약 생성은 실패합니다 (13:00 보다 과거의 예약이므로). 하지만 동일한 테스트를 2023-08-02 09:00 시에 돌리면 해당 테스트는 성공하게 됩니다. 이처럼 오늘로 기준을 잡고 테스트를 작성하면 해당 요구사항 부분까지 (테스트의 직접적인 관심사가 아닌데도) 불필요하게 신경을 써가며 작성을 해야했습니다.

이런 불편함을 없애고자 아얘 기준점을 모레(THE_DAY_AFTER_TOMORROW)로 잡아버려서 테스트를 작성하게 된 것입니다. 물론 말씀주신 것 처럼 차이가 없을 수 있는게, THE_DAY_AFTER_TOMORROW 로 기준점을 잡아도 예약 일자를 minus 3, 4 days 정도 해버리면 동일한 문제가 생깁니다. 하지만 통상적으로 저희가 기준점을 기준으로 시간만 어느정도 조정하는식으로 테스트를 짭니다. 그래서 의도적으로 몇 일씩 차이가 나는 예약을 생성하면서 테스트를 짤 경우는 드물다고 생각했습니다. 혹시 이해가 충분히 되셨을까요? 혹시 이런 방식이 좀 별로다 라고 생각되시면 좀 더 괜찮은 방법으로 생각해보시고 테스트 코드 리팩터링 해주셔도 좋습니다!

sakjung · 2023-07-31T10:06:03Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+
+    private void updateEndTimeByCurrentTime(Reservation reservation) {
+        ReservationTime reservationTime = reservation.getReservationTime();
+        ReservationTime updatedReservationTime = ReservationTime.ofDefaultServiceZone(


Suggested change

ReservationTime updatedReservationTime = ReservationTime.ofDefaultServiceZone(

ReservationTime updatedReservationTime = ReservationTime reservationTime = ReservationTime.of(

startDateTime,

endDateTime,

map.getServiceZone(),

false);

Map 마다 ServiceZone 이 귀속되어 있습니다 (현재는 모두 KOREA). 지금과 같이 짜도 물론 정상 동작하겠지만 기존에 설계했던 방향과 어긋나게 되는 코드입니다. 추후, 의도했던 다양한 service zone 에 대한 확장성, 코드 통일성을 위해서 위와 같은 형태로 작성해주시면 감사하겠습니다.

추가로 manageable flag 때문에 위 코드 243 라인 변경점 부분은 수정되어야할 것으로 보입니다. manageable 은 맵의 매니저가 아닌 사람이 현재 시간보다 과거의 예약을 컨트롤 할 수 없도록 제어 하기 위한 장치입니다. 자세한 사항은 관련 코드 참고 부탁드립니다!

넵! 확인 후 반영하겠습니다

sakjung · 2023-07-31T10:22:31Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+        if (reservation.isInUse(LocalDateTime.now())) {
+            updateEndTimeByCurrentTime(reservation);
+            return;
+        }


update 하고 로직이 끝나버리면 delete API 에서 update 를 하는 형국이 됩니다. 뭔가 직관적이지 않은데, 어떤 의도로 예약 조기종료 기능을 구현하려고 하셨는지 조금 더 디테일하게 설명해주시면 코드를 이해하는데 더 도움이 될 것 같습니다.

현재 공간마다 예약 조건이 있고 그 조건에 정확히 맞춰서 예약이 되게끔 구현되어있습니다. 개인적인 의견으로는 기존에 예약 했던 시간대는 업데이트하지 않고 그대로 두고, 조기종료를 판단할 수 있는 추가적인 요소를 통해 판별하는게 훨신 사이드 이펙트가 적지 않을까 생각이 듭니다. 왜냐하면 [공간 조건 - 예약 시간대] 간의 연결 고리는 이 API 말고도 다른 로직에서도 긴밀히 연결된 부분이 많기 때문에 다른 로직에 어떠한 영향을 줄 지 알 수 없습니다. 따라서 조금은 과감한 수정이라고 볼 수 있습니다. 그리고 이 케이스를 통해서 공간 조건에 위배되는 예외 케이스가 생성이 되므로 추후 개발하는데 있어서 복잡도가 추가됩니다. 최대한 저희가 가지고 있는 데이터는 클린하고 예측가능한 형태의 데이터여야 한다는게 견해 드리고 싶습니다.

제가 놓치고있거나 잘 못 해석한 부분이 있다면 정정 부탁드릴게요~

확실히 하나의 메서드에서 두개의 로직이 같이 수행되는 모습이 많이 복합해보이네요!

초기에는 사용중인 예약에 대해서 삭제한다 라는 로직을 생각하여 기존 삭제 메소드에 조건만 변경하면 된다는 생각으로 작업을 진행했습니다.
그러나 작업을 진행하는 도중에 사용을 시작한 예약에 대해서는 기록이 남아야겠다는 의견으로 수렴되면서 위와 같은 로직이 작성되었습니다.
삭정 말씀을 들어보니 별도의 조기종료에 대한 API 를 구성하는 것이 더 적절해 보이네요!

조기종료에 대한 API 를 구성하는 방향으로 리팩터링 진행하겠습니다~

Choi-JJunho · 2023-08-02T08:25:46Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+    public SlackResponse updateReservationEndTime(final ReservationUpdateDto reservationUpdateDto) {
+        ReservationStrategy reservationStrategy = reservationStrategies.getStrategyByReservationType(reservationUpdateDto.getReservationType());
+        Long mapId = reservationUpdateDto.getMapId();
+        LoginUserEmail loginUserEmail = reservationUpdateDto.getLoginUserEmail();
+
+        Map map = maps.findByIdFetch(mapId)
+                .orElseThrow(NoSuchMapException::new);
+        reservationStrategy.validateManagerOfMap(map, loginUserEmail);
+
+        Long reservationId = reservationUpdateDto.getReservationId();
+        Reservation reservation = reservations.findById(reservationId)
+                .orElseThrow(NoSuchReservationException::new);
+        reservationStrategy.validateOwnerOfReservation(reservation, reservationUpdateDto.getPassword(), loginUserEmail);
+
+        LocalDateTime now = LocalDateTime.now();
+        if (reservation.getStartTime().isAfter(now) || reservation.getEndTime().isBefore(now)) {
+            throw new NotCurrentReservationException();
+        }
+
+        reservation.updateReservationTime(
+                ReservationTime.of(
+                        reservation.getStartTime(),
+                        reservationUpdateDto.getEndDateTime(),
+                        map.getServiceZone(),
+                        false)
+        );
+
+        map.activateSharingMapId(sharingIdGenerator);
+
+        return SlackResponse.of(reservation, map);
+    }
+


예약 조기종료에 대한 서비스를 별도로 구성했습니다!
이에 테스트를 어떻게 해야할 지 감이 오지 않아 자문을 구하고싶습니다.

현재 서비스에서는 LocalDateTime.now()를 이용하여 현재 사용중인 예약정보인지에 대한 검증을 수행하고있습니다.

SpringBootTest를 이용해 통합테스트를 수행할 때 테스트에서 LocalDateTime.now를 사용하지 않고 해당 검증에 대한 부분을 어떻게 고려하고 테스트를 작성해야할지 감이 오지 않네요 ㅠㅠ

현재 수정하신 코드를 보면 이전 로직과 동일하게 endTime이 업데이트 되는 것으로 보입니다.

위 코멘트에서 말씀드린 부분이 좀 부족했던 것 같아서 좀 더 부가설명 드리겠습니다 죄송합니다.

앞서 말씀드렸다 시피 endTime이 수정되버리면 공간 조건에 위배되는 예외 케이스가 생성됩니다. 예를 들면, 10:00 ~ 12:00 예약이 10:00 ~ 11:53 과 같은식으로 예약이 변경될 수 있습니다. 지금 당장 우려가 되는 점은 추후 ReservationService.validateSpaceSetting 로직을 타는 API 들이 정상 동작하지 않을 것으로 예상되는 부분입니다. 그리고 해당 부분 말고도 아직 저도 알 수 없는 사이드 이펙트 발생지점이 있을 가능성이 높다고 생각합니다. 직접적인 원인부터 말씀드리면, 예약 시간과 관련된 도메인 객체들이 내부적으로 검증을 할 때 실패할 것이기 때문입니다.

찜꽁의 모든 예약 관련 시간은 기획 상 5의 배수 여야 합니다 (최소 예약 시간 단위 5, 10, 30, 60 이 존재). 시간 데이터를 엄격하게 관리하기 위해 찜꽁에서 사용할 예약 시간 관련 도메인 객체들 (e.g. TimeSlot, TimeUnit) 을 설계했습니다. 그리고 해당 객체들은 생성 시 마다 저희 서비스 요구사항에 맞는 validation 을 이행합니다.

10:00 ~ 12:00 예약이 10:00 ~ 11:53 변경된 시나리오에 대해서 코드와 함께 예시, 설명을 드리겠습니다. 코드 따라 가보시면서 확인 보시는 걸 추천드립니다.

10:00 ~ 12:00 예약 10:00 ~ 11:53 으로 변경 (조기종료) -> 해당 예약 (Reservation)에 대한 TimeSlot 객체 형성 (Reservation.getTimeSlot 호출) -> TimeSlot.getDurationMinute 호출 -> TimeUnit 객체 생성자 로직 -> TimeUnit.cannotDivideByMinimumTimeUnit 호출 -> 검증 실패 (113 분은 5의 배수가 아니므로 찜꽁 서비스 시간 체계에서 존재할 수 없는 시간 개념 (minutes) 임) -> IllegalTimeUnitValueException 에러

그래서 제가 제안 드렸던 바는, start, end time 은 실제로 예약 생성, 수정 시에만 건드리는 것으로 하고 조기 종료를 확인할 수 있는 새로운 무언가가 있는게 좋겠다 는 의도로 말씀을 드린 것 이었습니다. 예를 들면, 예약이 현재 사용 중인 경우일 때 FE 측에서 조기종료 버튼을 보여주고, 사용자가 해당 버튼을 누르면 서버측에서 해당 예약의 조기종료 flag (신규 컬럼)가 true 로 변경되고 해당 시간대도 별도 저장 (신규 컬럼)하는 식으로 관리하는 방법 같은 것을 생각했습니다 (이것은 그냥 이해를 돕기 위한 단순 브레인 스토밍 성 의견입니다). 만약 그런식으로 된다면, 예약 생성, 업데이트 시 예약 가능 여부를 판별할때 조기종료 여부와 조기 종료 시간도 별도로 추가 체크해야하겠네요. 목요일날 한 번 얘기 나눠봐요!

구체적인 설명 감사합니다!
저희가 해당 로직을 작성하면서 TimeSlot에 대한 검증이 이뤄지는 부분을 전체적으로 한번 훑어봤고, 이에 endTime에 대한 검증만 제거해도 괜찮지않을까? 라는 생각에 다음 작업을 수행헀습니다.

TimeSlot의 5분단위 검증에서 endTime에 대한 검증을 제거한다.

말씀을 들어보니 예약 생성, 수정에 있어서 전체적인 도메인 로직을 해치는 상황이라고 생각되네요!

혹시 다음과 같은 의견에 대해서는 어떻게 생각하시는지 궁금합니다!

예약 조기종료 시 5분 단위의 가장 가까운 미래시간으로 종료시간이 기록된다.

ex) 15:42분에 조기종료 수행 시 endTime은 15:45분이 된다.

$$({Minute} + 5) / 5 * 5$$

예약 조기종료 시 5분 단위의 가장 가까운 과거 시간으로 종료시간이 기록된다.
--> 올림 말고 내림으로!

정리 땅땅땅)

endTime 은 startTime 보다 이후여야 한다.

이용시간(endTime - startTime) 은 예약 시간 단위로 나누어 떨어져야 한다.

저도 동일하게 생각해봤던 부분입니다 ㅋㅋ. 하나의 대안이 될 수 있을것 같아요. 오늘 미팅이니까 다같이 얘기해봐요!

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

- 조기 종료 시 5분 단위의 가까운 과거 시간으로 종료 시간이 기록된다. - 종료 시간은 시작 시간과 같을 수 없다. - 사용 중인 예약에 대해서만 조기 종료를 할 수 있다. Co-authored-by: Go-Jaecheol <gojaech@naver.com>

xrabcde

금방 구현해주셨네요 👏
몇 가지 고민해볼만한 점 코멘트 남겼으니 같이 얘기해봅시당

xrabcde · 2023-08-08T05:23:49Z

backend/src/main/java/com/woowacourse/zzimkkong/controller/GuestReservationController.java

+        SlackResponse slackResponse = reservationService.updateReservationEndTime(reservationUpdateDto);
+        slackService.sendUpdateMessage(slackResponse);


본인의 예약을 종료시키는 상황이니까 슬랙으로 예약수정알림은 따로 안보내줘도 될 것 같아요
(수정보다는 조기종료에 대한 알림케이스를 새로 만들어서 발송해줘도 되구용)

조기 종료에 대한 알림 케이스를 새로 만드는 방식으로 구현해보겠습니다!

xrabcde · 2023-08-08T05:31:53Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+            throw new NotCurrentReservationException();
+        }
+
+        if (ChronoUnit.MINUTES.between(reservation.getStartTime(), reservationUpdateDto.getEndDateTime()) < 5L) {


이거 5분이 아니라 예약시간단위(reservationTimeUnit)로 체크하기로 하지 않았나욤?

+) 사용자는 어떻게든 조기종료만 되면 되니까 최소 이용시간을 체크해서 예외를 날리는 것보다는 예약시간단위로 나누어떨어지는 가장 가까운 시간으로 종료시간이 설정되주는 게 사용성은 가장 좋을 것 같다는 생각도 드네요.

검증은 endTime(종료하려는 시점의 시간)이 startTime 보다 이후인지만 확인하도록 하고 (isInUse)

사용시간(endTime - startTime)을 확인해서

reservationTimeUnit 보다 크다면 : 가장 가까운 과거의 reservationTimeUnit로 나누어 떨어지는 시간

reservationTimeUnit 보다 작다면 : 가장 가까운 미래의 reservationTimeUnit로 나누어 떨어지는 시간

으로 종료시간을 업데이트 하도록 하고
이에 대해 FE에서 공간의 예약조건에 맞는 가장 가까운 ~시로 종료처리되었다는 안내메시지를 보여주는건 어떤가요?

제가 놓치고 있는 게 있다면 알려주세욥 '-' @sakjung

네 저도 회의때 reservationTimeUnit 로 체크하는 것으로 이해했어요. 그리고 바다가 제안 주신 방법이 좋아보입니다.

reservationTimeUnit 보다 작다면 : 가장 가까운 미래의 reservationTimeUnit로 > 나누어 떨어지는 시간

근데 이 부분에서 좀 우려되는 게 있네요. 만약 reservationTimeUnit이 60 분이고 조기종료를 사용 후 5분 만에 했으면 60분 후 종료로 기록이 됩니다. 그러면 55분이나 붕뜨게 되고 조기 종료하는 의미가 퇴색될 것 같습니다. 그래서 그 경우에는 그냥 exception 내던가 아얘 삭제하던가 하는 식이 맞을 것 같아요. 아니면 프론트 측에서 reservationTimeUnit 시간 검증을 같이 해준다면 바다가 제안주신 방법과 비슷하게 가능할 것 같습니다.

[FE] reservationTimeUnit 보다 크다면 : [FE] PATCH API (=지금 이 PR에서 구현된 API) -> [BE] 가장 가까운 과거의 reservationTimeUnit로 나누어 떨어지는 시간

[FE] reservationTimeUnit 보다 작다면 : [FE] 사용 시간이 공간의 예약 시간 단위 (X분) 보다 짧아서 예약이 삭제될건데 괜찮냐는 류의 팝업 정도? -> YES -> [FE] DELETE API -> [BE] 구현 필요 X

아래는 reservationTimeUnit 를 가져올 수 있는 pseudo code 입니다. 객체 지향적으로 짠다면 해당 로직을 reservation 객체 내부에 메서드로 만드는게 좋겠네요! getMatchingSpaceSetting() 같은? (참고로 하나의 space에 다수의 setting이 귀속될 수 있습니다)

Setting matchSetting = reservation.getSpace().getSpaceSettings().getSettings().stream() .filter(setting -> setting.supports(reservation.getTimeSlot(), reservation.getDayOfWeek())) .findFirst() .orElseThrow(NoMatchingSettingException::new); TimeUnit reservationTimeUnit = matchSetting.getReservationTimeUnit();

또 추가로 필요한 것은 그 때 회의때 얘기했듯이, 공간 최소 예약 시간 조건 (reservation_minimum_time_unit) 제거 입니다 (사진 참고)

대략 예상 되는 작업은 다음과 같습니다.

FE 측 관련 코드 제거 (관련해서 FE 근로 멤버들에게 가이드 주실 수 있으면 부탁드려요 @Puterism @SunYoungKwon @yujo11 )

BE 측 관련 코드 제거

예약 생성, 업데이트 로직에서 Setting.reservationMinimumTimeUnit 관련 코드 제거 (ReservationService.validateSpaceSetting 로직에서 Setting.cannotAcceptDueToMinimumTimeUnit 관련 로직 참고)

Space 생성, 업데이트 로직에서 Setting.reservationMinimumTimeUnit 관련 코드 제거

DB Setting table에서 reservation_minimum_time_unit column 제거

xrabcde · 2023-08-08T05:32:11Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+    }
+
+    private int floorByFiveMinutes(final LocalDateTime baseTime) {
+        return (baseTime.getMinute() - 5) / 5 * 5;


여기도 5로 하드코딩 하시면 안되고 reservationTimeUnit 으로 계산해야 할 것 같아요

첨언하자면, 공식이 맞는지 모르겠네요. 예를 들면 baseTime.getMinute() 이 32 라면 30이 나와야 하는데 25가 나오는 것으로 보입니다. 유닛 테스트를 짜셔서 확실히 확인 해보면 좋을 것 같아요.

그리고 해당 메서드 (floorBy~)는 TimeUnit 객체 안으로 옮기면 더 객체지향적인 코드가 될 수 있을 것 같습니다. 제가 생각하는 그림은 다음과 같습니다.

TimeUnit endTimeFloor = reservationTimeUnit.floor(reservation.getEndTime())

sakjung

수고 많으셨습니다~ 리뷰 참고 부탁드립니다.
테스트 문의 주신 부분은 구현이 아직 결정이 안된 부분이 있으니 나중에 논의 해봐요~

sakjung · 2023-08-08T15:33:02Z

...oowacourse/zzimkkong/dto/reservation/ReservationUpdateWithPasswordWithoutEndTimeRequest.java

+
+@Getter
+@NoArgsConstructor
+public class ReservationUpdateWithPasswordWithoutEndTimeRequest extends ReservationCreateUpdateRequest {


제가 이해하기로는 조기종료 시 기존 예약 시간에서 endTime 만 현재시간으로 꺾는다 로 이해했습니다. 그러면 startTime 도 안받아도 되는 것 아닌가 하는 생각이드네요. 혹시 따로 startTime 을 받는 이유가 있나요? 제가 여러분들 구현 의도를 완전히 이해 못했을 수도 있어서 이 부분이 궁금하네요 ㅎㅎ

만약 startTime, endTime 이 모두 필요 없다면 굳이 Request DTO 를 상속할 필요없이 더 간단한 DTO를 만들 수 있을 것 같습니다~

Request DTO를 상속해서 사용하는 것이 컨벤션이라고 생각해서 위와 같이 구현했습니다..!

상속없이 필요한 값만 받아서 간단한 DTO를 받는 방식으로 처리하겠습니다.

sakjung · 2023-08-08T16:13:55Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+                .orElseThrow(NoSuchReservationException::new);
+        reservationStrategy.validateOwnerOfReservation(reservation, reservationUpdateDto.getPassword(), loginUserEmail);
+
+        LocalDateTime now = LocalDateTime.now();


만드신 DTO ReservationUpdateWithPasswordWithoutEndTimeRequest 를 보면 now 를 생성해서 주입하실 생각으로 보입니다. 그리고 여기서도 now가 호출되네요! 시간의 무결성을 생각한다면, 하나의 request 내에서 now 는 한번만 호출해서 돌려쓰는게 더 합리적으로 보입니다. 정말 미세한 시간 차이가 나겠지만, 다수의 now 호출 마다 시간 차이가 생기는 건 분명하고 로직에 구멍이 생기는 거니까요.

request에 선언되어 있던 now를 제거하고, service에서 한 번만 선언하여 사용하도록 수정하겠습니다!

sakjung · 2023-08-08T16:42:47Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+            throw new NotCurrentReservationException();
+        }
+
+        if (ChronoUnit.MINUTES.between(reservation.getStartTime(), reservationUpdateDto.getEndDateTime()) < 5L) {


네 저도 회의때 reservationTimeUnit 로 체크하는 것으로 이해했어요. 그리고 바다가 제안 주신 방법이 좋아보입니다.

reservationTimeUnit 보다 작다면 : 가장 가까운 미래의 reservationTimeUnit로 > 나누어 떨어지는 시간

근데 이 부분에서 좀 우려되는 게 있네요. 만약 reservationTimeUnit이 60 분이고 조기종료를 사용 후 5분 만에 했으면 60분 후 종료로 기록이 됩니다. 그러면 55분이나 붕뜨게 되고 조기 종료하는 의미가 퇴색될 것 같습니다. 그래서 그 경우에는 그냥 exception 내던가 아얘 삭제하던가 하는 식이 맞을 것 같아요. 아니면 프론트 측에서 reservationTimeUnit 시간 검증을 같이 해준다면 바다가 제안주신 방법과 비슷하게 가능할 것 같습니다.

[FE] reservationTimeUnit 보다 크다면 : [FE] PATCH API (=지금 이 PR에서 구현된 API) -> [BE] 가장 가까운 과거의 reservationTimeUnit로 나누어 떨어지는 시간

[FE] reservationTimeUnit 보다 작다면 : [FE] 사용 시간이 공간의 예약 시간 단위 (X분) 보다 짧아서 예약이 삭제될건데 괜찮냐는 류의 팝업 정도? -> YES -> [FE] DELETE API -> [BE] 구현 필요 X

아래는 reservationTimeUnit 를 가져올 수 있는 pseudo code 입니다. 객체 지향적으로 짠다면 해당 로직을 reservation 객체 내부에 메서드로 만드는게 좋겠네요! getMatchingSpaceSetting() 같은? (참고로 하나의 space에 다수의 setting이 귀속될 수 있습니다)

Setting matchSetting = reservation.getSpace().getSpaceSettings().getSettings().stream() .filter(setting -> setting.supports(reservation.getTimeSlot(), reservation.getDayOfWeek())) .findFirst() .orElseThrow(NoMatchingSettingException::new); TimeUnit reservationTimeUnit = matchSetting.getReservationTimeUnit();

또 추가로 필요한 것은 그 때 회의때 얘기했듯이, 공간 최소 예약 시간 조건 (reservation_minimum_time_unit) 제거 입니다 (사진 참고)

대략 예상 되는 작업은 다음과 같습니다.

FE 측 관련 코드 제거 (관련해서 FE 근로 멤버들에게 가이드 주실 수 있으면 부탁드려요 @Puterism @SunYoungKwon @yujo11 )

BE 측 관련 코드 제거

예약 생성, 업데이트 로직에서 Setting.reservationMinimumTimeUnit 관련 코드 제거 (ReservationService.validateSpaceSetting 로직에서 Setting.cannotAcceptDueToMinimumTimeUnit 관련 로직 참고)

Space 생성, 업데이트 로직에서 Setting.reservationMinimumTimeUnit 관련 코드 제거

DB Setting table에서 reservation_minimum_time_unit column 제거

sakjung · 2023-08-09T00:04:42Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+    }
+
+    private int floorByFiveMinutes(final LocalDateTime baseTime) {
+        return (baseTime.getMinute() - 5) / 5 * 5;


첨언하자면, 공식이 맞는지 모르겠네요. 예를 들면 baseTime.getMinute() 이 32 라면 30이 나와야 하는데 25가 나오는 것으로 보입니다. 유닛 테스트를 짜셔서 확실히 확인 해보면 좋을 것 같아요.

그리고 해당 메서드 (floorBy~)는 TimeUnit 객체 안으로 옮기면 더 객체지향적인 코드가 될 수 있을 것 같습니다. 제가 생각하는 그림은 다음과 같습니다.

TimeUnit endTimeFloor = reservationTimeUnit.floor(reservation.getEndTime())

sakjung · 2023-08-09T00:30:39Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+                ReservationTime.of(
+                        reservation.getStartTime(),
+                        LocalDateTime.of(
+                                reservation.getDate(),
+                                LocalTime.of(
+                                        reservation.getEndTime().getHour(),
+                                        floorByFiveMinutes(reservationUpdateDto.getEndDateTime())
+                                )
+                        ),


reservation.getStartTime() = UTC

LocalDateTime.of(reservation.getDate(),LocalTime.of(reservation.getEndTime().getHour() = KST X UTC 짬뽕 시간

reservation.getDate() 는 단순 indexing (조회) 용이고 service zone 기준 (KST 기준) 입니다. reservation.getEndTime() 은 UTC 시간입니다 (서버내의 모든 시간은 UTC로 관리).

이런 경우에는 다음과 같이 하면 될 것 같습니다 (아래 멘션드린 floor 관련 코드를 사용했습니다)

LocalDateTime endTime = reservation.getEndTime() TimeUnit endTimeFloor = reservationTimeUnit.floor(endTime) reservation.updateReservationTime( ReservationTime.of( reservation.getStartTime(), endTime.withMinute(endTimeFloor), map.getServiceZone(), false) );

쭉 리뷰를 달다보니까, 조기종료 로직을 reservation 이라는 객체 안에 모두 담을 수 있겠네요 (캡슐화).

서비스 코드 몸집 줄이기 -> 비즈니스 로직 흐름에 대한 가독성 증가

객체 내부에서 데이터 (필드)를 조작하므로 더 안전 -> Getter, Setter 류 지양

객체간의 메세징 -> 서로의 책임과 역할 명확히 분리됨 -> 유지보수 성 증가

최대한 다음과 같은 느낌으로 작성을 해보시는 건 어떨까요? 내부 구현은 저희가 단 리뷰들 참고하셔서 멋지게 작성해주세요 ㅎㅎ

# AS-IS reservation.updateReservationTime(...생략...) # TO-BE reservation.doEarlyClose(now)

제안해주신 코드를 바탕으로 리팩토링 진행해보겠습니다!

sakjung

리뷰가 늦어서 죄송합니다. 황금 연휴라 좀 논다고 ㅎㅎ...
잘 작성해주셨네요! 수고 너무 많으셨습니다 🙇🏾‍♂️
몇 가지 코멘트 더 드렸는데 확인 부탁드리겠습니다.
추가로 시간 되시면, 테스트 좀 더 짜보시는 것도 좋을 것 같아요.

sakjung · 2023-08-16T13:22:19Z

backend/src/main/java/com/woowacourse/zzimkkong/domain/Reservation.java

@@ -142,4 +160,36 @@ public String getUserName() {
        }
        return this.userName;
    }
+
+    public TimeUnit getReservationTimeUnit() {


Suggested change

public TimeUnit getReservationTimeUnit() {

private TimeUnit getReservationTimeUnit() {

Reservation 안에서만 쓰이는 것으로 보여요

sakjung · 2023-08-16T14:01:40Z

backend/src/main/java/com/woowacourse/zzimkkong/domain/Reservation.java

+            throw new NotCurrentReservationException();
+        }
+
+        if (ChronoUnit.MINUTES.between(this.getStartTime(), now) < this.getReservationTimeUnit().getMinutes()) {


this.getReservationTimeUnit()가 총 2번 호출되는 것으로 보입니다. 1번만 호출하는 게 로직이 더 효율적일 것 같습니다.

sakjung · 2023-08-16T14:11:20Z

backend/src/main/java/com/woowacourse/zzimkkong/domain/TimeUnit.java

+    public int floor(final LocalDateTime time) {
+        return time.getMinute() / this.minutes * this.minutes;
+    }


👍🏿👍🏿👍🏿👍🏿

sakjung · 2023-08-16T14:35:18Z

backend/src/main/java/com/woowacourse/zzimkkong/dto/reservation/ReservationEarlyStopDto.java

+    private String email;
+    private String password;


email 필드는 조기종료 기능에서는 따로 받을 필요 없을 것 같아요~ 이미 Controller 에서 @LoginEmail final LoginUserEmail loginUserEmail 를 받고 계시기 때문에 ReservationService에 작성해주신 것 처럼 해당 객체를 사용하면 됩니다.

좀 헷갈릴 수 있어서 추가 설명 드리면, 다른 DTO 들에서 보이는 email 필드는 공간 관리자 MANAGER (e.g. 잠실, 선릉 맵 매니저 = 포비, 제이슨 등) 가 다른 회원들의 예약을 대신 매니징해줄 때 이메일이 필요한 경우 (회원 유저)가 있어서 따로 받기 위해서 별도로 존재하는 필드라고 생각해주시면 됩니다.
관련 로직이 궁금하시면 ManagerLoginGuestReservationStrategy.buildReservation 부분 참고 해주시면 됩니다. 번외로 password 는 비회원 유저용 필드입니다!

sakjung · 2023-08-16T14:36:43Z

...wacourse/zzimkkong/exception/reservation/InvalidMinimumDurationTimeInEarlyStopException.java

+
+public class InvalidMinimumDurationTimeInEarlyStopException extends ZzimkkongException {
+
+    private static final String MESSAGE = "조기 종료는 최소 5분 이후부터 가능합니다.";


Suggested change

private static final String MESSAGE = "조기 종료는 최소 5분 이후부터 가능합니다.";

private static final String MESSAGE = "조기 종료는 최소 X분 이후부터 가능합니다.";

분이 가변적이어야 할 것 같아요~ Exception 생성자에 TimeUnit 관련 인자를 받아야겠네요. 참고할 만한 Exception 많으니 한 번 둘러보셔서 참고하시면 될 것 같습니다!

sakjung · 2023-08-16T14:54:53Z

backend/src/main/java/com/woowacourse/zzimkkong/dto/reservation/ReservationEarlyStopDto.java

+        this.password = request.getPassword();
+        this.email = request.getEmail();
+        this.loginUserEmail = loginUserEmail;
+        this.reservationType = ReservationType.of(apiType, getReservationEmail(apiType));


Suggested change

this.reservationType = ReservationType.of(apiType, getReservationEmail(apiType));

this.reservationType = ReservationType.of(ReservationType.Constants.GUEST, this.loginUserEmail);

매니저 Controller 쪽에도 동일한 API를 뚫을 게 아니라면 이렇게 고정되어도 괜찮겠네요. DTO 생성 시 불필요하게 주입받는 인자도 줄이고. 저희 기존 코드 틀을 지켜주시려고 신경을 많이 써주시다 보니 불필요하게 레퍼런스 되는 부분이 있는 것 같아요. 그래서 코드 이해를 도와드리고 사고를 더 확장? 시켜드리기 위해 드리는 제안입니다 ㅎㅎ. 확장하기 용이한 지금 이 형태가 좋다면 꼭 반영 안하셔도 됩니다! 회의 때 여러분이 나눈 의견에 더 적합한 방향으로 구현 해주세요.

sakjung · 2023-08-16T14:56:50Z

.../main/java/com/woowacourse/zzimkkong/dto/reservation/ReservationManagerEarlyStopRequest.java

+
+@Getter
+@NoArgsConstructor
+public class ReservationManagerEarlyStopRequest {


사용하는 곳이 없네요?

sakjung · 2023-08-16T14:59:07Z

...nd/src/main/java/com/woowacourse/zzimkkong/exception/setting/NoMatchingSettingException.java

+    private static final String MESSAGE = "일치하는 설정이 존재하지 않습니다.";
+
+    public NoMatchingSettingException() {
+        super(MESSAGE, HttpStatus.NOT_FOUND);


500과 같은 서버 에러가 적합해 보여요. setting 은 무조건 매칭되는 한개가 있어야합니다. 만약 없다면 모종의 이유로 발생한 데이터 이슈입니다.

sakjung · 2023-08-16T15:02:23Z

backend/src/main/java/com/woowacourse/zzimkkong/service/ReservationService.java

+    private int floorByFiveMinutes(final LocalDateTime baseTime) {
+        return baseTime.getMinute() / 5 * 5;
+    }


제거 해주세요~

Choi-JJunho added 🌟 feature New feature or request 🕋 backend labels Jul 31, 2023

Choi-JJunho requested review from dusdn1702, tributetothemoon, xrabcde and sakjung July 31, 2023 07:23

Choi-JJunho assigned Go-Jaecheol and Choi-JJunho Jul 31, 2023

sakjung reviewed Jul 31, 2023

View reviewed changes

Choi-JJunho commented Aug 2, 2023

View reviewed changes

Choi-JJunho and others added 11 commits August 7, 2023 17:00

test: 오늘 날짜 Fixture 추가

34a3798

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

refactor: 종료시간 10분단위 검증 제외

2265a54

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

feat: 예약 조기종료 기능 추가

5e10d61

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

refactor: 서비스 지역 UTC 제거

d0dea33

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

refactor: 기존 함수 유지하도록 수정

ed0580a

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

refactor: TODAY 제거

e19af2e

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

feat: 예약 조기종료 기능 추가

565b680

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

test: 테스트 롤백

cb43e8e

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

refactor: LocalDateTime으로 수정

c75ad9e

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

refactor: TimeSlot 수정사항 복원

25cbf2f

Co-authored-by: Go-Jaecheol <gojaech@naver.com>

Choi-JJunho force-pushed the feat/966-reservation-early-stop branch from 5bbe858 to eaa505b Compare August 7, 2023 08:00

chore: resolve conflict

d75d31b

xrabcde requested changes Aug 8, 2023

View reviewed changes

sakjung requested changes Aug 9, 2023

View reviewed changes

Choi-JJunho added 5 commits August 9, 2023 16:40

refactor: 조기종료를 위한 DTO 생성

76d3464

refactor: 예약 시간 단위 구하는 로직 추가

0f5e9bb

refactor: 예약 종료시간 로직 분리

d910d49

refactor: 조기 종료 로직 예약 객체에게 위임

bf0b974

refactor: 슬랙 조기종료 슬랙 발송메시지 추가

aaeb59e

sakjung reviewed Aug 16, 2023

View reviewed changes

-        ReservationTime updatedReservationTime = ReservationTime.ofDefaultServiceZone(
+        ReservationTime updatedReservationTime = ReservationTime reservationTime = ReservationTime.of(
+                startDateTime,
+                endDateTime,
+                map.getServiceZone(),
+                false);

		SlackResponse slackResponse = reservationService.updateReservationEndTime(reservationUpdateDto);
		slackService.sendUpdateMessage(slackResponse);

	public TimeUnit getReservationTimeUnit() {
	private TimeUnit getReservationTimeUnit() {


		public class InvalidMinimumDurationTimeInEarlyStopException extends ZzimkkongException {

		private static final String MESSAGE = "조기 종료는 최소 5분 이후부터 가능합니다.";

	this.reservationType = ReservationType.of(apiType, getReservationEmail(apiType));
	this.reservationType = ReservationType.of(ReservationType.Constants.GUEST, this.loginUserEmail);

feat: 예약 조기종료 기능 구현 #969

Are you sure you want to change the base?

feat: 예약 조기종료 기능 구현 #969

Conversation

Choi-JJunho commented Jul 31, 2023 • edited Loading

구현 기능

기타

sakjung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

질문

sakjung Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakjung Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choi-JJunho Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

$$({Minute} + 5) / 5 * 5$$

Choose a reason for hiding this comment

xrabcde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakjung Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

sakjung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakjung Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

sakjung Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakjung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choi-JJunho commented Jul 31, 2023 •

edited

Loading

sakjung Aug 2, 2023 •

edited

Loading

sakjung Aug 2, 2023 •

edited

Loading

Choi-JJunho Aug 3, 2023 •

edited

Loading

sakjung Aug 9, 2023 •

edited

Loading

sakjung Aug 9, 2023 •

edited

Loading

sakjung Aug 9, 2023 •

edited

Loading