[BE] 약속 일정 확정 API 추가

[seunghye218]

관련 이슈

• resolves: [BE] 약속 확정 기능을 추가해요 :) #118

작업 내용

• 주최자가 날짜, 시간 등의 약속 일정을 확정하가 위한 약속 일정 확정 API 추가하였습니다.

• 일정 확정 검증

  1. uuid에 해당하는 약속이 존재하는지 검증합니다.
  2. 주최자만 확정할 수 있기에 참여자가 주최자 권한인지 검증합니다.
  3. 약속이 잠금 상태되어야 확정할 수 있기에 이를 검증합니다.
  4. +) 이미 확정된 일정이 없는지 검증합니다.
  5. 주어진 날짜가 약속에 속한 날짜인지 검증합니다.
  6. 주어진 시간이 약속의 시간 범위에 포함되는지 검증합니다.

• 연속된 날짜를 검증하기 위해 아래 검증을 추가했습니다.

  1. 우선 하루 안에 포함된 일정이라면 그 날짜가 포함되어있는지 검증합니다.
  2. 연속된 날짜라면 Meeting의 입력 시간 제한이 fullTime(0000~2330)이어야 합니다.
  3. 일정의 날짜 범위가 약속의 가능 날짜에 포함되어있는지 O(nlogn) 시간 복잡도로 검증합니다.
  4. 해당 미팅의 가능 날짜들 정렬합니다.
  5. 시작 날짜 이상 ~ 끝 날짜 이하 라면 count++
  6. count 와 (끝날짜 - 시작날짜) + 1가 같다면 포함된 날짜입니다.

특이 사항

• 약속을 확정 짓는다는 뜻으로 ConfirmedMeeting로 엔티티 이름을 지었습니다.
• 확정 일정의 시간에 해당하는 참가자의 스케줄이 존재하지 않더라도 확정 일정이 약속의 범위에 포함되면 일정 확정 가능합니다.
• 스케줄에 포함시키려 했지만 거대해져 새로운 도메인으로 분리했습니다.
• 이미 확정된 약속이라면 예외가 발생합니다. 이에 대한 ErrorCodeType을 추가했습니다.
• MeetingController 에 속한 기능이며 MeetingConfirmService를 Meeting 패키지 안에 위치하도록 했습니다.
• entity 추가

리뷰 요구사항 (선택)

• 참가자가 주최자가 아니라면 예외를 발생하는 validateHostPermission 메서드가 서로 다른 서비스에서 중복적으로 발생합니다. 참가자가 책임지자는 재즈의 의견 적용할지 생각이 궁금합니다.

• 가능날짜-확정 일정 일대일 연관 관계에 대해 �의문점이 있습니다. 여행 같은 약속의 경우 여러 날짜가 가능하고 추천 일정이 밤12시에 걸쳐 이어진 시간이라면 일대다 관계가 되어야할 것 같아요. 어떻게 생각하시는지 궁금합니다.

• ⭐️ endTime 저장 시 30분 차감하는 로직이 일정 확정 기능에도 영향이 있습니다. 비슷하면서도 다른 코드들이 생겨나 헷갈릴 수 있고 차후 반복될 것으로 예상됩니다. [BE] 하루의 마지막 타임슬롯이 포함된 일정 등록 시 오류가 발생해요 :(  #88 이슈와 관련되어 있으며 회의에서 확실하게 정하고 넘어가야 나중에 유지보수하기 편해질 것 같습니다.

• 최대 가능 시간(23:59)을 처리하는 로직이 없기에 연속된 날짜 테스트가 불가능 합니다.

[github-actions]

Test Results

93 tests   93 ✅  6s ⏱️
17 suites   0 💤
17 files     0 ❌

Results for commit f194cfe.

♻️ This comment has been updated with latest results.

[ikjo39]

마크 기능 구현하느라 고생하셨습니다 👍
몇 가지 리뷰 남겼으니 확인 부탁드려요!

추가로 PR 본문에 대해 드리고 싶은 말도 남겨보아요.
PR 본문을 읽어보며 개인적으로 뭔가 컨텍스트 설명이 부족하다 느꼈어요.
일부분은 코드를 따라가봐야 이해되서 좀 당황스럽기도 했는데요.
작업한 결과를 나열하기보단 보다 문장 형태로 명확하고 근거와 함께 적어주시면 리뷰할 때보다 수월할 거 같아요!
수고스럽겠지만 PR 본문도 함께 관리해야하는 문서로서 제안드립니다. 🙏

[ikjo39]

테이블 연관관계를 이렇게 설계한 마크의 이유가 궁금합니다 :)

[seunghye218]

확정 일정에 필요한 정보로 약속에 대한 날짜, 시작 시간, 끝 시간이 있습니다. 연관관계에 대해서 약속 당 하나의 확정 일정이 필요하기 때문에 약속-확정일정 일대일 연관 관계 맵핑했어요.

일단 확정 일정 당 하나의 날짜가 필요하다 생각하여 가능날짜-확정 일정 일대일 연관 관계 맵핑했어요.
하지만 여행 같은 약속의 경우 여러 날짜가 가능합니다. 추가적으로 추천 일정이 밤12시에 걸쳐 이어진 시간이라면 일대다 관계가 되어야할 것 같아요. 이 부분은 논의가 필요할 것 같습니다!

[ikjo39]

하지만 여행 같은 약속의 경우 여러 날짜가 가능합니다.

이 경우엔 시간들이 의미 없어지니 주최자가 약속 생성시 탭 기능을 이용해 시간 값이 없도록 설정할 수 있게끔 확장하는 것도 고려해볼 수 있을 거 같아요!
지금은 시간이 있는 약속 설정만 가능하게끔 초점을 맞춰보는 것도 좋아보입니다.

추가적으로 추천 일정이 밤12시에 걸쳐 이어진 시간이라면 일대다 관계가 되어야할 것 같아요.

제 생각엔 이럴 때는 주최자가 00:00-23:30로 모든 시간에 대한 약속을 설정하고, 그 다음날까지 이용가능일로 특정한 뒤
주최자가 가능한 시간대를 표시하게끔 하는 방식도 고려할 수 있을 것 같아요.

연관관계에 대해서 약속 당 하나의 확정 일정이 필요하기 때문에 약속-확정일정 일대일 연관 관계 맵핑했어요.

현재 마크가 보여준 Entity 설계가 30분씩 시간이 줄어드는 오류에 관한 이슈만 해결된다면 MVP를 충분히 만족하고 있다고 생각합니다 👍

[seokmyungham]

마크 고생하셨습니다. 😊
받으면 행운이 깃들고 열정이 샘솟는 제 RC를 드립니다. 🎉
궁금한 점 있으면 언제든지 코멘트 남겨주세요!!

[ehBeak]

마크 고생 많으셨어요! 리뷰 남깁니다~~ 👍

[ehBeak]

TimeslotInterval에서 endTimeslot은 시간 한 칸을 의미하고 해당 시간까지 포함되는 것으로 알고 있어요!
때문에 endTimeslot이 13:00이면 실제 시간 경계의 끝은 13:30이 됩니다. 현재 ConfirmedSchedule에서 사용하는 endTimeSlot은 TimeslotInterval에서 endTimeslot과 다른 의미로 쓰이는 건가요? 어떤 의미로 쓰이는지, 어떤 의도로 minusMinutes(30) 함수를 사용하셨는지 궁금합니다~

[seunghye218]

Meeting의 생성자를 보시면 위와 같고 말씀하신 의미 그대로 사용하고 있습니다.
13:00 ~ 13:30 으로 요청이 들어온다면 startTime = 13:00, endTime = 13:00 으로 저장되는 것이 맞아요. 그렇기 때문에 서비스에서

ConfirmedSchedule confirmedSchedule = new ConfirmedSchedule(meeting, date, request.startTime(), request.endTime());
confirmedScheduleRepository.save(confirmedSchedule);

요청 그대로 생성자에 인자를 넘기고 생성자에서 endTime에 30분을 차감하게 됩니다.

코드를 다시보니 30분 차감을 Meeting과 ConfirmedSchedule에서 하는 것보다 TimeslotInterval이 책임지는 것이 나아보이기도 하네요.

[hw0603]

다른 분들이 이미 변경점들을 많이 짚어 주셔서 일반 코멘트로 남겨요!

참가자가 주최자가 아니라면 예외를 발생하는 validateHostPermission 메서드가 서로 다른 서비스에서 중복적으로 발생합니다. 참가자가 책임지자는 재즈의 의견 적용할지 생각이 궁금합니다.

Host가 아닐 때 어떤 예외를 터트려야 할지 서비스 플로우마다 다를 수 있지 않을까요? 일괄적으로 도메인이 그 예외를 지정해서 던지는 것은 서비스 문맥에 따라 어색한 상황이 발생할 수 있을 것 같아요

가능날짜-확정 일정 일대일 연관 관계에 대해 의문점이 있습니다. 여행 같은 약속의 경우 여러 날짜가 가능하고 추천 일정이 밤12시에 걸쳐 이어진 시간이라면 일대다 관계가 되어야할 것 같아요. 어떻게 생각하시는지 궁금합니다.

제 경험상 여행과 같은 일정을 잡는 경우 보통 시간을 생략하고 날짜만 투표받는 경우가 대부분이였는데요, 저희 초기 목표 기능 중에 날짜만 투표받는 기능도 있었던 것으로 기억해요.
현재는 후순위 구현 사항이 되었으니 우선 언급해 주신 부분은 잠시 보류하는 것도 괜찮을 것 같아요.
하지만... 밤12시에 걸쳐 이어진 시간이라면... 이 부분은 어떻게 해결할지 의논이 필요해 보이네요😲

endTime 저장 시 30분 차감하는 로직이 일정 확정 기능에도 영향이 있습니다. 비슷하면서도 다른 코드들이 생겨나 헷갈릴 수 있고 차후 반복될 것으로 예상됩니다. [BE] 하루의 마지막 타임슬롯이 포함된 일정 등록 시 오류가 발생해요 :( #88 이슈와 관련되어 있으며 회의에서 확실하게 정하고 넘어가야 나중에 유지보수하기 편해질 것 같습니다.

이 부분은 우선적으로 리팩토링 진행해 보겠습니다🥲

[ikjo39]

마크 리뷰 반영하시느라 수고많으셨어요!
다만 팀원들과 몇가지 조율할 논제들이 있어보여 댓글로 의견 남기도록 하겠습니다.

[ikjo39]

하지만 여행 같은 약속의 경우 여러 날짜가 가능합니다.

이 경우엔 시간들이 의미 없어지니 주최자가 약속 생성시 탭 기능을 이용해 시간 값이 없도록 설정할 수 있게끔 확장하는 것도 고려해볼 수 있을 거 같아요!
지금은 시간이 있는 약속 설정만 가능하게끔 초점을 맞춰보는 것도 좋아보입니다.

추가적으로 추천 일정이 밤12시에 걸쳐 이어진 시간이라면 일대다 관계가 되어야할 것 같아요.

제 생각엔 이럴 때는 주최자가 00:00-23:30로 모든 시간에 대한 약속을 설정하고, 그 다음날까지 이용가능일로 특정한 뒤
주최자가 가능한 시간대를 표시하게끔 하는 방식도 고려할 수 있을 것 같아요.

연관관계에 대해서 약속 당 하나의 확정 일정이 필요하기 때문에 약속-확정일정 일대일 연관 관계 맵핑했어요.

현재 마크가 보여준 Entity 설계가 30분씩 시간이 줄어드는 오류에 관한 이슈만 해결된다면 MVP를 충분히 만족하고 있다고 생각합니다 👍

[ikjo39]

안녕하세요 마크!
복잡한 로직 구현하시느라 고생 많으셨어요!
몇가지 리뷰 남겼으니 확인해주세요🙏

[ikjo39]

1. if문 내에 2개 이상의 비교문이 있는 경우 메서드 분리하면 가독성이 오를 것 같아요!
2. 추가적으로 (date.isEqual(startDate) || date.isEqual(endDate))와 (date.isAfter(startDate) && date.isBefore(endDate)) 모두 무엇을 의미하는지 와닿지 않아서요.
   추가적인 설명 부탁드립니다!

[seunghye218]

1. 부정연산자를 사용하여 2개의 비교문으로 아래 리뷰와 함께 리팩터링해볼게요!
2. 정렬된 약속의 날짜들 중 확정 일정에 해당하는 경우를 카운트하기 위한 조건문입니다. isAfter 와 isBefore는 비교 대상의 날짜를 포함하지 않기 때문에 isEqual 까지 추가한 모습이에요.
   -> 요약: 날짜 초과, 미만이 아니라 이상, 이하를 검증.

[ikjo39]

연속된 날짜를 판단하는 로직으로 이해했는데 맞을까요?
그렇다면 첫 날짜부터 하루씩 증가하며 연속인지 판단해보는건 어떨까요?

public boolean isConsecutiveDay(List<LocalDate> dates) {

    // Sorted 로직

    if (dates == null || dates.size() < 2) {
        return true;
    }

    for (int i = 1; i < dates.size(); i++) {
        LocalDate previousDate = dates.get(i - 1);
        LocalDate currentDate = dates.get(i);
            
        // 두 날짜가 연속되지 않으면 false를 반환
        if (!previousDate.plusDays(1).equals(currentDate)) {
            return false;
        }
    }

    return true;
}

[hw0603]

@ikjo39 연속될 뿐만 아니라, AvailableDates 내의 날짜 범위의 부분집합이여야 합니다.
따라서 제시해 주신 로직만으로는 모든 검증을 다 할 수 없어요.

흐름을 따라가 보면, 닫힌구간 [d, d+3]일이 가능한 날짜 리스트의 부분집합이라면 d, d+1, d+2, d+3 이렇게 총 4개의 날짜가 모두 AvailableDates 내에 존재해야 합니다.

따라서 AvailableDates를 오름차순 정렬한 후, 가장 빠른 일자부터 d+3일 까지 순회하며,
현재 탐색중인 availableDate가

• d일 혹은 d+3일 이거나
• d일 초과 d+3일 미만인
  날짜들의 개수를 카운트하여 카운트 값이((d+3) - d) + 1 [==4] 와 일치하는지 확인하는 로직입니다.

가장 빠른 일자부터 탐색하지 않고 이분탐색을 활용하여 d일을 탐색 시작점으로 잡고 카운트하는 로직을 그대로 수행하면 조금 더 최적화할 수 있습니다.

---

@seunghye218 기가 막힌 로직이긴 한데요, public 메서드임에도 해당 로직을 따라가기가 쉽지 않았습니다.
이러한 로직들은 이후 유지보수에 걸림돌이 될 수 있고, 작성자 본인만 수정할 수 있는 로직이 될 가능성이 커요.

특히, public 메서드의 흐름은 도메인 로직을 잘 모르는 개발자가 읽더라도 그 흐름이 명확해야 한다고 생각합니다.

위와 같은 복잡한 로직은 private메서드로 분리하거나 (물론 변수명/메서드명을 잘 지어서 흐름을 따라가는데 무리가 없어야겠죠), 메서드에 주석을 추가하는 한이 있더라도 의도를 설명해야 합니다.

[seunghye218]

피드백이 필요했었는데 좋은 리뷰들 감사합니다. 가독성 좋게 메서드를 분리해볼게요!

[seunghye218]

스트림으로 로직을 선언형으로 간결하겨 리팩터링했습니다~

long availableDateRangeCount = availableDates.stream()
                .sorted(comparing(AvailableDate::getDate))
                .dropWhile(date -> date.isBefore(startDateInclusive))
                .takeWhile(date -> !date.isAfter(endDateInclusive))
                .count();

[hw0603]

고생하셨어요 마크!
꽤 예외사항이 많은 로직이였는데 꼼꼼하게 잘 작성해 주셨네요!

몇 가지 변경 요청과 함께 테스트 코드에 코멘트 남겨 두었습니다. 코드와 일치하지 않는 테스트가 있어서 CI가 통과하지 않더라구요.

최대 가능 시간(23:59)을 처리하는 로직이 없기에 연속된 날짜 테스트가 불가능 합니다.

말씀해 주신 것과 같이 현재 테스트가 되고 있지 않은데, 이 부분을 해결하고 머지를 진행할지, #88 번 이슈에서 같이 해결할지는 논의가 필요해 보여요🤔

[hw0603]

어차피 저희는 '정렬되지 않은' 날짜들은 크게 필요가 없으니, TreeSet을 도입하여 정렬된 상태를 유지하는 것도 고려해 볼 만 하겠네요.

다만, 다온의 사례와 같이 테스트 코드에서 프록시 객체 문제가 발생하는 경우가 있어 지금 바로 적용하기보다는 조금 더 알아보고 도입하면 좋을 것 같아요.

[seunghye218]

저도 도입해도 좋다고 생각했는데 그런 문제가 발생하는군요... 필요성이 더 커질 때 확실히 알아보고 적용해보죠.

[hw0603]

@ikjo39 연속될 뿐만 아니라, AvailableDates 내의 날짜 범위의 부분집합이여야 합니다.
따라서 제시해 주신 로직만으로는 모든 검증을 다 할 수 없어요.

흐름을 따라가 보면, 닫힌구간 [d, d+3]일이 가능한 날짜 리스트의 부분집합이라면 d, d+1, d+2, d+3 이렇게 총 4개의 날짜가 모두 AvailableDates 내에 존재해야 합니다.

따라서 AvailableDates를 오름차순 정렬한 후, 가장 빠른 일자부터 d+3일 까지 순회하며,
현재 탐색중인 availableDate가

• d일 혹은 d+3일 이거나
• d일 초과 d+3일 미만인
  날짜들의 개수를 카운트하여 카운트 값이((d+3) - d) + 1 [==4] 와 일치하는지 확인하는 로직입니다.

가장 빠른 일자부터 탐색하지 않고 이분탐색을 활용하여 d일을 탐색 시작점으로 잡고 카운트하는 로직을 그대로 수행하면 조금 더 최적화할 수 있습니다.

---

@seunghye218 기가 막힌 로직이긴 한데요, public 메서드임에도 해당 로직을 따라가기가 쉽지 않았습니다.
이러한 로직들은 이후 유지보수에 걸림돌이 될 수 있고, 작성자 본인만 수정할 수 있는 로직이 될 가능성이 커요.

특히, public 메서드의 흐름은 도메인 로직을 잘 모르는 개발자가 읽더라도 그 흐름이 명확해야 한다고 생각합니다.

위와 같은 복잡한 로직은 private메서드로 분리하거나 (물론 변수명/메서드명을 잘 지어서 흐름을 따라가는데 무리가 없어야겠죠), 메서드에 주석을 추가하는 한이 있더라도 의도를 설명해야 합니다.

[hw0603]

    @DisplayName("start일자부터 end일 사이의 모든 날짜가 가능한 날짜인지 확인한다.")
    @Test
    void testMethod() {
        // given
        AvailableDates availableDates = new AvailableDates(
                List.of(
                        LocalDate.of(2024, 8, 1),
                        LocalDate.of(2024, 8, 2),
                        LocalDate.of(2024, 8, 3),
                        LocalDate.of(2024, 8, 4),
                        LocalDate.of(2024, 8, 5),
                        LocalDate.of(2024, 8, 6),
                        LocalDate.of(2024, 8, 7),
                        LocalDate.of(2024, 8, 8),
                        LocalDate.of(2024, 8, 9),
                        LocalDate.of(2024, 8, 10)
                ), null
        );

        // when
        boolean actual = availableDates.isNotConsecutiveDay(
                LocalDate.of(2024, 8, 2),
                LocalDate.of(2024, 8, 10)
        );

        // then
        boolean expected = false;
        assertThat(actual).isEqualTo(expected);
    }

이런 느낌으로 AvailableDates.isNotConsecutiveDay() 에 대한 테스트도 가능할 것 같아요!

@DisplayName은 임시로 적어둔 거니 참고만 하시구요ㅎㅎ
@MethodSource 를 활용해서 true 일 때와 false일 때 모두 테스트하면 좋겠네요🙂

[seokmyungham]

마크 고생 많으셨어요. 🔥🎉🔥🎉🔥🎉🔥🎉

[ikjo39]

고생했어요 마크~

[ehBeak]

마크 고생 많으셨습니다~!!💯💯

[hw0603]

고생 많으셨습니다🚀
댓글 76개를 끝으로 이만 이 PR은 닫도록 하죠ㅎㅎ Approve 합니다!