[BE] 약속 생성시 과거 날짜 검증 추가 #149

ikjo39 · 2024-08-02T08:12:38Z

작업 내용

약속 생성시 과거 날짜가 포함되어 있다면 예외가 발생하도록 검증을 추가하였습니다.
관련한 테스트도 추가하였어요.

특이 사항

리뷰 요구사항 (선택)

추가 검토 사항: 코드, 디자인, 구현 방식 등에 대한 추가적인 검토가 필요한 사항이 있나요?

현재 날짜(LocalDate) 객체 생성이 적절한 계층에 수행되고 있는지 궁금합니다. 과거 시간 검증은 API가 요청된 시점의 오늘을 기준으로 이전 날짜인지 검증하게 되는데요. 이때 비즈니스 로직에 대한 테스트를 위해 Persistence Layer(MeetingController)에 위치하고 있습니다. 이것이 적절한지 궁금합니다.

논의가 필요한 부분: 코드 리뷰 중 논의가 필요해 보이는 부분은 무엇인가요?

과거 시간 검증에 대한 인수 테스트(ControllerTest)가 필요한지 궁금합니다. 현재 테스트 내 400 Bad Request 상황에 올바른 상태코드를 반환하는 테스트가 이미 있는데요. 과거 검증 인수테스트를 추가한다면 문맥은 다르나 테스트하고자 하는 목적이 400 Bad Request 반환으로 동일할 것 같다 생각하기 때문입니다. 여러분의 의견이 궁금하네요. 필요하다면 바로 반영하겠습니다.

github-actions · 2024-08-02T08:13:55Z

Test Results

78 tests 78 ✅ 5s ⏱️
16 suites 0 💤
16 files 0 ❌

Results for commit 83289ef.

♻️ This comment has been updated with latest results.

hw0603

고생하셨어요. 다온!

네이밍 관련 리뷰 남겨 두었습니다.

hw0603 · 2024-08-03T08:37:55Z

backend/src/main/java/kr/momo/controller/meeting/MeetingController.java

+        LocalDate today = LocalDate.now();
+        MeetingSharingResponse response = meetingService.create(request, today);


현재 날짜를 검증하는 로직은 Service 계층에 존재해야 할 것 같아요.

테스트의 어려움이 걱정이셨다면 Clock 객체를 Fake로 만들어 타임머신 타고(ㅋㅋ) 테스트하는 방법도 있습니다.

오오 Clock 객체는 처음보는군요.
관련 정보 찾아보고 적용해볼게요!

hw0603 · 2024-08-03T08:41:17Z

backend/src/main/java/kr/momo/service/meeting/MeetingService.java

@@ -42,8 +42,11 @@ private Meeting saveMeeting(String meetingName, LocalTime startTime, LocalTime e
        return meetingRepository.save(meeting);
    }

-    private void saveAvailableDates(List<LocalDate> dates, Meeting meeting) {
+    private void saveAvailableDates(List<LocalDate> dates, Meeting meeting, LocalDate date) {


마지막 파라미터 LocalDate date는 얼핏 봐서는 어떤 값이 전달되는지 알기 어려울 것 같아요. 변수명을 수정해 보는 건 어떨까요?

hw0603 · 2024-08-03T08:43:58Z

backend/src/main/java/kr/momo/domain/availabledate/AvailableDates.java

+    public boolean isAllBefore(LocalDate other) {
+        return availableDates.stream()
+                .anyMatch(date -> date.isBefore(other));
+    }


메서드명은 isAllBefore 인데 실제 로직은 일급 컬렉션이 감싸고 있는 AvailableDate 중에 other보다 과거시점인 날짜가 하나라도 있으면 true를 반환하지 않나요? 🥲

isAnyBefore()이 더 적절한 이름이었겠네요 🤕
반영해보겠습니다!

seunghye218

고생하셨어요~ 에러코드에 대한 리뷰를 남겼는데 다온의 생각이 궁금해요~!

seunghye218 · 2024-08-03T10:47:04Z

backend/src/main/java/kr/momo/exception/code/MeetingErrorCode.java

@@ -7,7 +7,8 @@ public enum MeetingErrorCode implements ErrorCodeType {
    INVALID_UUID(HttpStatus.BAD_REQUEST, "유효하지 않은 UUID 입니다."),
    NOT_FOUND_MEETING(HttpStatus.NOT_FOUND, "존재하지 않는 약속 정보 입니다."),
    INVALID_TIME_RANGE(HttpStatus.BAD_REQUEST, "끝 시간은 시작 시간 이후가 되어야 합니다."),
-    MEETING_LOCKED(HttpStatus.BAD_REQUEST, "약속이 잠겨있어 수행할 수 없습니다.");
+    MEETING_LOCKED(HttpStatus.BAD_REQUEST, "약속이 잠겨있어 수행할 수 없습니다."),
+    NOT_AVAILABLE_MEETING(HttpStatus.BAD_REQUEST, "약속 생성이 불가능한 날짜입니다.");


지나간 날짜에 대한 설명이 있으면 좋을 것 같아요. 사용자가 다양한 날짜를 선택했는데 위와 같은 메시지를 받는다면 어떤 대처를 해야할 지 정보가 부족할 수 있어 보여요! 또 AvailableDateErrorCode 에 위치�하는 건 어떤가요?

지나간 날짜에 대한 설명이 있으면 좋을 것 같아요.

좋은 의견이네요! 과거 날짜가 입력되었다는 말을 추가해야겠네요!

AvailableDateErrorCode 에 위치하는 건 어떤가요?

AvalidableDate의 연결성이 부족하다 생각했어요!
AvailableDate는 과거 여부만 판단할 뿐, 예외가 던져지는 시점은 Meeting이 생성되는 단 하나의 컨텍스트라 생각하였습니다.
다만, 의미론상 Not_Permitted_Past와 같이 Enum 명을 수정할 필요는 느껴지네요.

제가 잘못 생각하는 것일 수도 있기에 마크는 왜AvailableDateErrorCode 에 위치한다고 생각하셨는지 궁금해요!

날짜에 관한 에러라 어떤지 가볍게 얘기해봤어요. 말하신대로 이 예외가 던져지는 시점은 Meeting이 생성될 때 하나라 MeetingErrorCode에 맞다고도 생각이 드네요!

hw0603 · 2024-08-03T14:31:48Z

backend/src/test/java/kr/momo/service/meeting/MeetingServiceTest.java

+    private static final Clock FIXED_CLOCK =
+            Clock.fixed(Instant.parse("2024-08-01T10:15:30Z"), ZoneId.of("Asia/Seoul"));


Suggested change

private static final Clock FIXED_CLOCK =

Clock.fixed(Instant.parse("2024-08-01T10:15:30Z"), ZoneId.of("Asia/Seoul"));

private static final Clock FIXED_CLOCK = Clock.fixed(

Instant.parse("2024-08-01T10:15:30Z"), ZoneId.of("Asia/Seoul")

);

hw0603 · 2024-08-03T14:32:31Z

backend/src/test/java/kr/momo/service/meeting/MeetingServiceTest.java


 @IsolateDatabase
 @SpringBootTest(webEnvironment = WebEnvironment.NONE)
 class MeetingServiceTest {

+    private static final Clock FIXED_CLOCK =


나중에 글로벌 픽스쳐를 하나 만들어서 Fake Clock 같은건 픽스쳐에서 관리해도 좋을 것 같아요.

추가적으로 현재 시간과 비교 로직과 같은 상황이 반복적으로 일어날 경우 Fixture 사용을 고려해볼 수 있겠네요!
이후 비슷한 로직이 있다면 잊지 않고 저도 리뷰 남기겠습니다~

hw0603

주말에 고생하셨습니다!
저는 이만 Approve 할게요👍

seokmyungham

다온 고생하셨습니다. 🔥
받으면 내일 좋은 일이 생기는 제 RC를 드립니다!

seokmyungham · 2024-08-04T13:44:11Z

backend/src/main/java/kr/momo/config/TimeConfig.java

+    private static final ZoneId CLOCK_TIME_ZONE = ZoneId.of("Asia/Seoul");
+
+    @Bean
+    public Clock clock() {
+        return Clock.system(CLOCK_TIME_ZONE);
+    }


좋은 설계네요 👍
나중에 타임 존이 다른 지역에서 요청이 왔을 때 유연하게 대처가 가능할 것 같습니다.

seokmyungham · 2024-08-04T15:51:52Z

backend/src/main/java/kr/momo/domain/availabledate/AvailableDates.java

+    public boolean isAnyBefore(LocalDate other) {
+        return availableDates.stream()
+                .anyMatch(date -> date.isBefore(other));
+    }


자바 TreeSet 구현체를 사용해보는 건 어떨까요? 정렬이라는 특징을 이용해서 검증을 더 효율적으로 할 수 있을 것 같아요. 더불어 필드 자체를 Set으로 변경하면 중복 검증로직을 생략할 수 있겠네요. 😉

ehBeak

제가 너무 늦게 확인했네요🥲 이미 이야기가 다 나누어진 상태이기도 하고 변경할 부분도 없을 것 같아, approve드립니다! 고생 많으셨어요~

ehBeak · 2024-08-05T02:32:02Z

backend/src/main/java/kr/momo/config/TimeConfig.java

+    @Bean
+    public Clock clock() {
+        return Clock.system(CLOCK_TIME_ZONE);
+    }


이런 방법도 있군요! 하나 배워가요~

seokmyungham

고생하셨습니다!! 😊

seunghye218

저도 참고해서 필요할때 Clock을 활용하여 테스트를 작성해봐야겠네요. 고생하셨어요!

ikjo39 added 🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) ♻️ 리팩터링 코드를 깎아요 :) labels Aug 2, 2024

ikjo39 added this to the 3차 데모데이 milestone Aug 2, 2024

ikjo39 self-assigned this Aug 2, 2024

ikjo39 requested review from ehBeak, hw0603, seokmyungham and seunghye218 as code owners August 2, 2024 08:12

hw0603 requested changes Aug 3, 2024

View reviewed changes

seunghye218 reviewed Aug 3, 2024

View reviewed changes

ikjo39 requested review from seunghye218 and hw0603 August 3, 2024 11:47

ikjo39 force-pushed the refactor/135-valdiate-available-dates-past branch 2 times, most recently from 16d8edc to dcc14ef Compare August 3, 2024 12:27

hw0603 reviewed Aug 3, 2024

View reviewed changes

ikjo39 requested a review from hw0603 August 4, 2024 10:49

hw0603 approved these changes Aug 4, 2024

View reviewed changes

seokmyungham requested changes Aug 4, 2024

View reviewed changes

ehBeak approved these changes Aug 5, 2024

View reviewed changes

ikjo39 force-pushed the refactor/135-valdiate-available-dates-past branch from 45d152f to 7e6ada0 Compare August 5, 2024 12:45

ikjo39 requested a review from seokmyungham August 5, 2024 12:45

seokmyungham approved these changes Aug 5, 2024

View reviewed changes

seunghye218 approved these changes Aug 6, 2024

View reviewed changes

ikjo39 added 6 commits August 6, 2024 10:17

feat(AvailableDates): 과거 시간 비교 로직 추가

cdf21d8

feat(MeetingService): 과거 시간으로 약속 생성시 검증 추가

7c25141

feat(MemberController): 현재시간 추가

e8b0f96

fix(MemberControllerTest): 약속 생성 테스트시 요청 내 과거 시간 수정

5a4a330

refactor(AvailableDate): 불분명한 메서드명 수정

03e0140

feat: Clock 객체 빈 등록

ecf7e2d

ikjo39 added 3 commits August 6, 2024 10:17

refactor: 현재 시간 로직 clock 변경 및 계층 이동

31278a3

refactor: 불분명한 예외 메세지, 에러코드 수

15a21a2

test(MeetingServiceTest): Clock 상수 선언 줄바꿈 컨벤션 적

83289ef

ikjo39 force-pushed the refactor/135-valdiate-available-dates-past branch from 7e6ada0 to 83289ef Compare August 6, 2024 01:18

ikjo39 merged commit 1617256 into develop Aug 6, 2024
6 checks passed

ikjo39 deleted the refactor/135-valdiate-available-dates-past branch August 6, 2024 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 약속 생성시 과거 날짜 검증 추가 #149

[BE] 약속 생성시 과거 날짜 검증 추가 #149

ikjo39 commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

hw0603 left a comment

hw0603 Aug 3, 2024

ikjo39 Aug 3, 2024

hw0603 Aug 3, 2024

hw0603 Aug 3, 2024

ikjo39 Aug 3, 2024

seunghye218 left a comment •

edited

Loading

seunghye218 Aug 3, 2024

ikjo39 Aug 3, 2024

seunghye218 Aug 4, 2024

hw0603 Aug 3, 2024

hw0603 Aug 3, 2024

ikjo39 Aug 4, 2024

hw0603 left a comment

seokmyungham left a comment

seokmyungham Aug 4, 2024

seokmyungham Aug 4, 2024 •

edited

Loading

ehBeak left a comment

ehBeak Aug 5, 2024

seokmyungham left a comment

seunghye218 left a comment

		LocalDate today = LocalDate.now();
		MeetingSharingResponse response = meetingService.create(request, today);

		private static final Clock FIXED_CLOCK =
		Clock.fixed(Instant.parse("2024-08-01T10:15:30Z"), ZoneId.of("Asia/Seoul"));

[BE] 약속 생성시 과거 날짜 검증 추가 #149

[BE] 약속 생성시 과거 날짜 검증 추가 #149

Conversation

ikjo39 commented Aug 2, 2024 • edited Loading

관련 이슈

작업 내용

특이 사항

리뷰 요구사항 (선택)

github-actions bot commented Aug 2, 2024 • edited Loading

Test Results

hw0603 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

seunghye218 left a comment • 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

Choose a reason for hiding this comment

hw0603 left a comment

Choose a reason for hiding this comment

seokmyungham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokmyungham Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

ehBeak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokmyungham left a comment

Choose a reason for hiding this comment

seunghye218 left a comment

Choose a reason for hiding this comment

ikjo39 commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

seunghye218 left a comment •

edited

Loading

seokmyungham Aug 4, 2024 •

edited

Loading