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

[FU-357] 날짜별 스케줄 검증로직 개선 및 리팩터링 #91

Merged
merged 4 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -9,7 +9,8 @@ public enum ScheduleErrorCode implements ErrorCode {
DAILY_SCHEDULE_NOT_FOUND(500, "해당하는 날짜별 스케줄을 찾을 수 없습니다."),
DAILY_SCHEDULE_OVERLAP(400, "해당 일정에 이미 등록된 스케줄이 있습니다."),
DAILY_SCHEDULE_IN_PAST(400, "현재 시점 이전의 스케줄은 등록할 수 없습니다."),
START_TIME_AFTER_END_TIME(400, "시작시간과 종료시간이 올바르지 않습니다.");
START_TIME_AFTER_END_TIME(400, "시작시간과 종료시간이 올바르지 않습니다."),
INVALID_SCHEDULE_UNIT(400, "기본스케줄 단위와 일치하지 않습니다.");

private final int httpStatus;
private final String message;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import com.fasterxml.jackson.annotation.JsonProperty;

public enum ScheduleUnit {

@JsonProperty("THIRTY_MINUTES")
THIRTY_MINUTES,
@JsonProperty("SIXTY_MINUTES")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import com.foru.freebe.member.entity.Member;
import com.foru.freebe.schedule.entity.DailySchedule;
import com.foru.freebe.schedule.entity.ScheduleStatus;

public interface DailyScheduleRepository extends JpaRepository<DailySchedule, Long> {
Optional<DailySchedule> findByMemberAndId(Member member, Long scheduleId);
Expand All @@ -18,7 +19,8 @@ public interface DailyScheduleRepository extends JpaRepository<DailySchedule, Lo

@Query("SELECT ds FROM DailySchedule ds WHERE ds.member = :photographer "
+ "AND ds.date = :date "
+ "AND ((ds.startTime < :endTime AND ds.endTime > :startTime))")
List<DailySchedule> findOverlappingSchedules(Member photographer, LocalDate date, LocalTime startTime,
LocalTime endTime);
+ "AND ((ds.startTime < :endTime AND ds.endTime > :startTime))"
Copy link
Contributor

@yuseok0215 yuseok0215 Jan 23, 2025

Choose a reason for hiding this comment

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

어떤 의미로 조건을 달아준건지 헷갈려서 설명 부탁해도 될까??🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

findConflictingSchedulesByStatuses는 추가하려는 날짜별 스케줄과 기존에 등록되어 있는 날짜별 스케줄 간의 충돌 여부를 검증하기 위해서 작성했어! 위 쿼리는 충돌하는 스케줄을 모두 조회해오는 용도이구 충돌은 아래 두 기준을 모두 만족했을 때 발생해!

  1. 추가하려는 스케줄과 기존 스케줄의 레벨(위계)이 같은 경우
  2. 추가하려는 스케줄이 기존 스케줄의 시간대와 겹치는 경우

너가 물어봐준 조건절은 (2)번에 해당하는 데이터를 필터링 하는건데, 시간대가 겹친다는 건 이런 경우야

  • 기존 스케줄: 추가 오픈 1월 23일 10:00 ~ 12:00
  • 추가하려는 스케줄: 예약 중지 1월 23일 09:00 ~ 11:00

즉, 추가하려는 스케줄의 시작시간은 기존 스케줄의 종료시간보다 이르면서, 종료시간은 기존 스케줄의 시작시간보다 늦는 상황이야
이렇게 하면 충돌하는 시간대를 찾을 수 있더라구!

Copy link
Contributor

@yuseok0215 yuseok0215 Jan 23, 2025

Choose a reason for hiding this comment

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

예시까지 들어서 설명해줘서 고마워!!ㅎㅎ
22번째 라인에서 ds.startTime < :endTime까지 읽고 겹치는 부분이겠다 싶었는데 뒤에 따라오는 AND ds.endTime > :startTime 이 부분을 읽고 다른의도가 있는건가? 다른 반례가 있어서 세팅해놓은건가? 싶었거든! ds.startTime < :endTime 여기까지만 쿼리문으로 줘도 충분히 말해준 2번 조건을 충족하지 않을까 싶었어!

이 부분에 대해서도 의견 부탁해!

Copy link
Member Author

Choose a reason for hiding this comment

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

ds.startTime < :endTime 이 조건만으로는 시간대가 겹치지 않는 스케줄까지도 조회할 가능성이 있어!

  • 기존 스케줄: 1월 23일 09:00 ~ 10:00
  • 추가하려는 스케줄: 1월 23일 10:00 ~ 11:00

위와 같은 상황처럼 충돌하지 않는 스케줄도 가져오게 되거든 🫨

+ "AND ds.scheduleStatus IN (:scheduleStatuses)")
List<DailySchedule> findConflictingSchedulesByStatuses(Member photographer, LocalDate date, LocalTime startTime,
LocalTime endTime, List<ScheduleStatus> scheduleStatuses);
}
Original file line number Diff line number Diff line change
@@ -1,24 +1,23 @@
package com.foru.freebe.schedule.service;

import java.time.LocalTime;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;

import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import com.foru.freebe.errors.errorcode.MemberErrorCode;
import com.foru.freebe.errors.errorcode.ScheduleErrorCode;
import com.foru.freebe.errors.exception.RestApiException;
import com.foru.freebe.member.entity.Member;
import com.foru.freebe.member.repository.MemberRepository;
import com.foru.freebe.schedule.dto.BaseScheduleDto;
import com.foru.freebe.schedule.dto.ScheduleUnitDto;
import com.foru.freebe.schedule.entity.BaseSchedule;
import com.foru.freebe.schedule.entity.DayOfWeek;
import com.foru.freebe.schedule.entity.OperationStatus;
import com.foru.freebe.schedule.repository.BaseScheduleRepository;
import com.foru.freebe.errors.errorcode.MemberErrorCode;
import com.foru.freebe.errors.errorcode.ScheduleErrorCode;
import com.foru.freebe.errors.exception.RestApiException;
import com.foru.freebe.member.entity.Member;
import com.foru.freebe.member.repository.MemberRepository;

import lombok.RequiredArgsConstructor;

Expand Down Expand Up @@ -62,7 +61,7 @@ public void updateSchedule(BaseScheduleDto baseScheduleDto, Member photographer)
}

public void createDefaultSchedule(Member photographer) {
for(DayOfWeek dayOfWeek : DayOfWeek.values()) {
for (DayOfWeek dayOfWeek : DayOfWeek.values()) {
BaseSchedule baseSchedule = BaseSchedule.builder()
.photographer(photographer)
.dayOfWeek(dayOfWeek)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
package com.foru.freebe.schedule.service;

import java.time.Clock;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.util.List;
import java.util.stream.Collectors;

Expand All @@ -26,7 +23,7 @@
@Transactional
public class DailyScheduleService {
private final DailyScheduleRepository dailyScheduleRepository;
private final Clock clock;
private final DailyScheduleValidator validator;

public List<DailyScheduleResponse> getDailySchedules(Member photographer, DailyScheduleMonthlyRequest request) {
return dailyScheduleRepository.findByMember(photographer)
Expand All @@ -38,9 +35,10 @@ public List<DailyScheduleResponse> getDailySchedules(Member photographer, DailyS
}

public DailyScheduleAddResponse addDailySchedule(Member photographer, DailyScheduleRequest request) {
validateTimeRange(request.getStartTime(), request.getEndTime());
validateScheduleInFuture(request);
validateScheduleOverlap(photographer, request);
validator.validateTimeRange(request.getStartTime(), request.getEndTime());
validator.validateScheduleUnit(photographer.getScheduleUnit(), request.getStartTime(), request.getEndTime());
validator.validateScheduleInFuture(request);
validator.validateConflictingSchedules(photographer, request);

DailySchedule dailySchedule = DailySchedule.builder()
.member(photographer)
Expand All @@ -55,13 +53,14 @@ public DailyScheduleAddResponse addDailySchedule(Member photographer, DailySched
}

public void updateDailySchedule(Member photographer, Long scheduleId, DailyScheduleRequest request) {
validateTimeRange(request.getStartTime(), request.getEndTime());
validator.validateTimeRange(request.getStartTime(), request.getEndTime());
validator.validateScheduleUnit(photographer.getScheduleUnit(), request.getStartTime(), request.getEndTime());

DailySchedule dailySchedule = dailyScheduleRepository.findByMemberAndId(photographer, scheduleId)
.orElseThrow(() -> new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_NOT_FOUND));

validateScheduleInFuture(request);
validateScheduleOverlap(photographer, request, scheduleId);
validator.validateScheduleInFuture(request);
validator.validateConflictingSchedules(photographer, request, scheduleId);

dailySchedule.updateScheduleStatus(request.getScheduleStatus());
dailySchedule.updateDate(request.getDate());
Expand All @@ -74,41 +73,6 @@ public void deleteDailySchedule(Member photographer, Long scheduleId) {
.orElseThrow(() -> new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_NOT_FOUND)));
}

private void validateScheduleOverlap(Member member, DailyScheduleRequest request) {
List<DailySchedule> overlappingSchedules = dailyScheduleRepository.findOverlappingSchedules(member,
request.getDate(), request.getStartTime(), request.getEndTime());

if (!overlappingSchedules.isEmpty()) {
throw new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_OVERLAP);
}
}

private void validateTimeRange(LocalTime startTime, LocalTime endTime) {
if (startTime.isAfter(endTime) || startTime.equals(endTime)) {
throw new RestApiException(ScheduleErrorCode.START_TIME_AFTER_END_TIME);
}
}

private void validateScheduleInFuture(DailyScheduleRequest request) {
LocalDateTime requestDateTime = request.getDate().atTime(request.getStartTime());

if (requestDateTime.isBefore(LocalDateTime.now(clock))) {
throw new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_IN_PAST);
}
}

private void validateScheduleOverlap(Member member, DailyScheduleRequest request, Long scheduleId) {
List<DailySchedule> overlappingSchedules = dailyScheduleRepository.findOverlappingSchedules(member,
request.getDate(), request.getStartTime(), request.getEndTime());

if (overlappingSchedules.size() == 1 && overlappingSchedules.get(0).getId().equals(scheduleId)) {
return;
}
if (!overlappingSchedules.isEmpty()) {
throw new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_OVERLAP);
}
}

private DailyScheduleResponse toDailyScheduleResponse(DailySchedule dailySchedule) {
return DailyScheduleResponse.builder()
.scheduleId(dailySchedule.getId())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package com.foru.freebe.schedule.service;

import java.time.Clock;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.util.List;

import org.springframework.stereotype.Service;

import com.foru.freebe.errors.errorcode.ScheduleErrorCode;
import com.foru.freebe.errors.exception.RestApiException;
import com.foru.freebe.member.entity.Member;
import com.foru.freebe.member.entity.ScheduleUnit;
import com.foru.freebe.schedule.dto.DailyScheduleRequest;
import com.foru.freebe.schedule.entity.DailySchedule;
import com.foru.freebe.schedule.entity.ScheduleStatus;
import com.foru.freebe.schedule.repository.DailyScheduleRepository;

import jakarta.transaction.Transactional;
import lombok.RequiredArgsConstructor;

@Service
@RequiredArgsConstructor
@Transactional
public class DailyScheduleValidator {
private final Clock clock;
private final DailyScheduleRepository dailyScheduleRepository;

public void validateTimeRange(LocalTime startTime, LocalTime endTime) {
if (startTime.isAfter(endTime) || startTime.equals(endTime)) {
throw new RestApiException(ScheduleErrorCode.START_TIME_AFTER_END_TIME);
}
}

public void validateScheduleUnit(ScheduleUnit scheduleUnit, LocalTime startTime, LocalTime endTime) {
switch (scheduleUnit) {
case SIXTY_MINUTES -> {
if (startTime.getMinute() != 0 || endTime.getMinute() != 0) {
throw new RestApiException(ScheduleErrorCode.INVALID_SCHEDULE_UNIT);
}
}
case THIRTY_MINUTES -> {
if (startTime.getMinute() != 0 && startTime.getMinute() != 30) {
throw new RestApiException(ScheduleErrorCode.INVALID_SCHEDULE_UNIT);
} else if (endTime.getMinute() != 0 && endTime.getMinute() != 30) {
throw new RestApiException(ScheduleErrorCode.INVALID_SCHEDULE_UNIT);
}
}
}
}

public void validateScheduleInFuture(DailyScheduleRequest request) {
LocalDateTime requestDateTime = request.getDate().atTime(request.getStartTime());

if (requestDateTime.isBefore(LocalDateTime.now(clock))) {
throw new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_IN_PAST);
}
}

public void validateConflictingSchedules(Member member, DailyScheduleRequest request) {
List<ScheduleStatus> scheduleStatuses = determineConflictingStatuses(request.getScheduleStatus());

List<DailySchedule> overlappingSchedules = dailyScheduleRepository.findConflictingSchedulesByStatuses(member,
request.getDate(), request.getStartTime(), request.getEndTime(), scheduleStatuses);

if (!overlappingSchedules.isEmpty()) {
throw new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_OVERLAP);
}
}

public void validateConflictingSchedules(Member member, DailyScheduleRequest request, Long scheduleId) {
List<ScheduleStatus> scheduleStatuses = determineConflictingStatuses(request.getScheduleStatus());

List<DailySchedule> conflictingSchedules = dailyScheduleRepository.findConflictingSchedulesByStatuses(member,
request.getDate(), request.getStartTime(), request.getEndTime(), scheduleStatuses);

if (conflictingSchedules.isEmpty() || isSelfConflictOnly(scheduleId, conflictingSchedules)) {
return;
}

throw new RestApiException(ScheduleErrorCode.DAILY_SCHEDULE_OVERLAP);
}

private List<ScheduleStatus> determineConflictingStatuses(ScheduleStatus scheduleStatus) {
if (scheduleStatus == ScheduleStatus.CONFIRMED) {
return List.of(ScheduleStatus.CONFIRMED);
} else {
return List.of(ScheduleStatus.OPEN, ScheduleStatus.CLOSED);
}
}

private boolean isSelfConflictOnly(Long scheduleId, List<DailySchedule> conflictingSchedules) {
return conflictingSchedules.size() == 1 && conflictingSchedules.get(0).getId().equals(scheduleId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.mockito.Mockito.*;

import java.time.LocalTime;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
Expand All @@ -11,6 +12,7 @@
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import com.foru.freebe.member.entity.Member;
import com.foru.freebe.member.entity.Role;
import com.foru.freebe.schedule.dto.BaseScheduleDto;
Expand All @@ -21,7 +23,6 @@

@ExtendWith(MockitoExtension.class)
class BaseScheduleServiceTest {

@Mock
private BaseScheduleRepository baseScheduleRepository;

Expand Down Expand Up @@ -92,10 +93,12 @@ void updateScheduleForActivation() {
Assertions.assertEquals(LocalTime.of(9, 0), existingBaseSchedule.getStartTime());
Assertions.assertEquals(LocalTime.of(18, 0), existingBaseSchedule.getEndTime());

verify(baseScheduleRepository, times(1)).findByDayOfWeekAndPhotographerId(baseScheduleDto.getDayOfWeek(), photographer.getId());
verify(baseScheduleRepository, times(1)).findByDayOfWeekAndPhotographerId(baseScheduleDto.getDayOfWeek(),
photographer.getId());
}

private BaseScheduleDto createBaseScheduleDto(DayOfWeek dayOfWeek, LocalTime startTime, LocalTime endTime, OperationStatus operationStatus) {
private BaseScheduleDto createBaseScheduleDto(DayOfWeek dayOfWeek, LocalTime startTime, LocalTime endTime,
OperationStatus operationStatus) {
return BaseScheduleDto.builder()
.dayOfWeek(dayOfWeek)
.startTime(startTime)
Expand Down
Loading