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

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

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

import static com.woowacourse.zzimkkong.dto.ValidatorMessage.DATETIME_FORMAT;
import static com.woowacourse.zzimkkong.dto.ValidatorMessage.DATE_FORMAT;
import static com.woowacourse.zzimkkong.infrastructure.datetime.TimeZoneUtils.UTC;

@LogMethodExecutionTime(group = "controller")
@RestController
Expand Down Expand Up @@ -153,6 +152,25 @@ public ResponseEntity<Void> update(
return ResponseEntity.ok().build();
}

@PatchMapping("/maps/{mapId}/spaces/{spaceId}/reservations/{reservationId}")
public ResponseEntity<Void> updateEndTime(
@PathVariable final Long mapId,
@PathVariable final Long spaceId,
@PathVariable final Long reservationId,
@RequestBody @Valid final ReservationUpdateWithPasswordWithoutEndTimeRequest reservationUpdateWithPasswordWithoutEndTimeRequest,
@LoginEmail(isOptional = true) final LoginUserEmail loginUserEmail) {
ReservationUpdateDto reservationUpdateDto = ReservationUpdateDto.of(
mapId,
spaceId,
reservationId,
reservationUpdateWithPasswordWithoutEndTimeRequest,
loginUserEmail,
ReservationType.Constants.GUEST);
SlackResponse slackResponse = reservationService.updateReservationEndTime(reservationUpdateDto);
slackService.sendUpdateMessage(slackResponse);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return ResponseEntity.ok().build();
}

@DeleteMapping("/maps/{mapId}/spaces/{spaceId}/reservations/{reservationId}")
public ResponseEntity<Void> delete(
@PathVariable final Long mapId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import net.logstash.logback.encoder.org.apache.commons.lang3.StringUtils;

import javax.persistence.*;
import java.time.DayOfWeek;
Expand Down Expand Up @@ -82,6 +81,10 @@ public void update(final Reservation updateReservation) {
this.space = updateReservation.space;
}

public void updateReservationTime(final ReservationTime updateReservationTime) {
this.reservationTime = updateReservationTime;
}

public LocalDateTime getStartTime() {
return reservationTime.getStartTime();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,3 @@ public String toString() {
return startTime + " ~ " + endTimeAsString;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.woowacourse.zzimkkong.dto.reservation;

import lombok.Getter;
import lombok.NoArgsConstructor;

import javax.validation.constraints.Pattern;
import java.time.ZonedDateTime;

import static com.woowacourse.zzimkkong.dto.ValidatorMessage.RESERVATION_PW_FORMAT;
import static com.woowacourse.zzimkkong.dto.ValidatorMessage.RESERVATION_PW_MESSAGE;

@Getter
@NoArgsConstructor
public class ReservationUpdateWithPasswordWithoutEndTimeRequest extends ReservationCreateUpdateRequest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

@Pattern(regexp = RESERVATION_PW_FORMAT, message = RESERVATION_PW_MESSAGE)
private String password;

public ReservationUpdateWithPasswordWithoutEndTimeRequest(
final ZonedDateTime startDateTime,
final String password,
final String name,
final String description) {
super(startDateTime, ZonedDateTime.now(), name, description);
this.password = password;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.woowacourse.zzimkkong.exception.reservation;

import com.woowacourse.zzimkkong.exception.ZzimkkongException;
import org.springframework.http.HttpStatus;

public class InvalidMinimumDurationTimeInEarlyStopException extends ZzimkkongException {

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

Choose a reason for hiding this comment

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

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

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


public InvalidMinimumDurationTimeInEarlyStopException() {
super(MESSAGE, HttpStatus.BAD_REQUEST);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.woowacourse.zzimkkong.exception.reservation;

import com.woowacourse.zzimkkong.exception.ZzimkkongException;
import org.springframework.http.HttpStatus;

public class NotCurrentReservationException extends ZzimkkongException {
private static final String MESSAGE = "사용중인 예약에 대해서만 조기종료가 가능합니다.";

public NotCurrentReservationException() {
super(MESSAGE, HttpStatus.BAD_REQUEST);
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,39 @@
package com.woowacourse.zzimkkong.service;

import com.woowacourse.zzimkkong.domain.*;
import com.woowacourse.zzimkkong.domain.Map;
import com.woowacourse.zzimkkong.domain.Member;
import com.woowacourse.zzimkkong.domain.Reservation;
import com.woowacourse.zzimkkong.domain.ReservationTime;
import com.woowacourse.zzimkkong.domain.ServiceZone;
import com.woowacourse.zzimkkong.domain.Setting;
import com.woowacourse.zzimkkong.domain.Settings;
import com.woowacourse.zzimkkong.domain.Space;
import com.woowacourse.zzimkkong.domain.TimeSlot;
import com.woowacourse.zzimkkong.dto.member.LoginUserEmail;
import com.woowacourse.zzimkkong.dto.reservation.*;
import com.woowacourse.zzimkkong.dto.reservation.ReservationAuthenticationDto;
import com.woowacourse.zzimkkong.dto.reservation.ReservationCreateDto;
import com.woowacourse.zzimkkong.dto.reservation.ReservationCreateResponse;
import com.woowacourse.zzimkkong.dto.reservation.ReservationFindAllDto;
import com.woowacourse.zzimkkong.dto.reservation.ReservationFindAllResponse;
import com.woowacourse.zzimkkong.dto.reservation.ReservationFindDto;
import com.woowacourse.zzimkkong.dto.reservation.ReservationFindResponse;
import com.woowacourse.zzimkkong.dto.reservation.ReservationInfiniteScrollResponse;
import com.woowacourse.zzimkkong.dto.reservation.ReservationResponse;
import com.woowacourse.zzimkkong.dto.reservation.ReservationUpdateDto;
import com.woowacourse.zzimkkong.dto.slack.SlackResponse;
import com.woowacourse.zzimkkong.exception.map.NoSuchMapException;
import com.woowacourse.zzimkkong.exception.member.NoSuchMemberException;
import com.woowacourse.zzimkkong.exception.reservation.*;
import com.woowacourse.zzimkkong.exception.reservation.DeleteExpiredReservationException;
import com.woowacourse.zzimkkong.exception.reservation.DeleteReservationInUseException;
import com.woowacourse.zzimkkong.exception.reservation.InvalidMaximumDurationTimeException;
import com.woowacourse.zzimkkong.exception.reservation.InvalidMinimumDurationTimeException;
import com.woowacourse.zzimkkong.exception.reservation.InvalidMinimumDurationTimeInEarlyStopException;
import com.woowacourse.zzimkkong.exception.reservation.InvalidReservationEnableException;
import com.woowacourse.zzimkkong.exception.reservation.InvalidStartEndTimeException;
import com.woowacourse.zzimkkong.exception.reservation.InvalidTimeUnitException;
import com.woowacourse.zzimkkong.exception.reservation.NoSuchReservationException;
import com.woowacourse.zzimkkong.exception.reservation.NotCurrentReservationException;
import com.woowacourse.zzimkkong.exception.reservation.ReservationAlreadyExistsException;
import com.woowacourse.zzimkkong.exception.setting.MultipleSettingsException;
import com.woowacourse.zzimkkong.exception.setting.NoSettingAvailableException;
import com.woowacourse.zzimkkong.exception.space.NoSuchSpaceException;
Expand All @@ -15,7 +42,11 @@
import com.woowacourse.zzimkkong.repository.MapRepository;
import com.woowacourse.zzimkkong.repository.MemberRepository;
import com.woowacourse.zzimkkong.repository.ReservationRepository;
import com.woowacourse.zzimkkong.service.strategy.*;
import com.woowacourse.zzimkkong.service.strategy.ExcludeReservationCreateStrategy;
import com.woowacourse.zzimkkong.service.strategy.ExcludeReservationStrategy;
import com.woowacourse.zzimkkong.service.strategy.ExcludeReservationUpdateStrategy;
import com.woowacourse.zzimkkong.service.strategy.ReservationStrategies;
import com.woowacourse.zzimkkong.service.strategy.ReservationStrategy;
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Slice;
import org.springframework.stereotype.Service;
Expand All @@ -24,6 +55,8 @@
import java.time.DayOfWeek;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.temporal.ChronoUnit;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
Expand Down Expand Up @@ -219,6 +252,52 @@ public SlackResponse updateReservation(final ReservationUpdateDto reservationUpd
return SlackResponse.of(reservation, map);
}

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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if (!reservation.isInUse(now)) {
throw new NotCurrentReservationException();
}

if (ChronoUnit.MINUTES.between(reservation.getStartTime(), reservationUpdateDto.getEndDateTime()) < 5L) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

  1. 검증은 endTime(종료하려는 시점의 시간)이 startTime 보다 이후인지만 확인하도록 하고 (isInUse)
  2. 사용시간(endTime - startTime)을 확인해서
  • reservationTimeUnit 보다 크다면 : 가장 가까운 과거의 reservationTimeUnit로 나누어 떨어지는 시간
  • reservationTimeUnit 보다 작다면 : 가장 가까운 미래의 reservationTimeUnit로 나누어 떨어지는 시간

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

네 저도 회의때 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) 제거 입니다 (사진 참고)

image

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

  • FE 측 관련 코드 제거 (관련해서 FE 근로 멤버들에게 가이드 주실 수 있으면 부탁드려요 @Puterism @SunYoungKwon @yujo11 )
  • BE 측 관련 코드 제거
    • 예약 생성, 업데이트 로직에서 Setting.reservationMinimumTimeUnit 관련 코드 제거 (ReservationService.validateSpaceSetting 로직에서 Setting.cannotAcceptDueToMinimumTimeUnit 관련 로직 참고)
    • Space 생성, 업데이트 로직에서 Setting.reservationMinimumTimeUnit 관련 코드 제거
  • DB Setting table에서 reservation_minimum_time_unit column 제거

throw new InvalidMinimumDurationTimeInEarlyStopException();
}

reservation.updateReservationTime(
ReservationTime.of(
reservation.getStartTime(),
LocalDateTime.of(
reservation.getDate(),
LocalTime.of(
reservation.getEndTime().getHour(),
floorByFiveMinutes(reservationUpdateDto.getEndDateTime())
)
),
Copy link
Collaborator

@sakjung sakjung Aug 9, 2023

Choose a reason for hiding this comment

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

  • 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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

map.getServiceZone(),
false)
);

map.activateSharingMapId(sharingIdGenerator);

return SlackResponse.of(reservation, map);
}

private int floorByFiveMinutes(final LocalDateTime baseTime) {
return (baseTime.getMinute() - 5) / 5 * 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@sakjung sakjung Aug 9, 2023

Choose a reason for hiding this comment

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

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

image

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

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

}

public SlackResponse deleteReservation(final ReservationAuthenticationDto reservationAuthenticationDto) {
ReservationStrategy reservationStrategy = reservationStrategies.getStrategyByReservationType(reservationAuthenticationDto.getReservationType());
Long mapId = reservationAuthenticationDto.getMapId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,4 @@ private static Stream<Arguments> provideTimeSlotArguments_isNotWithin() {
false)
);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import java.util.Optional;

import static com.woowacourse.zzimkkong.Constants.*;
import static com.woowacourse.zzimkkong.infrastructure.datetime.TimeZoneUtils.UTC;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Expand Down