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] 회원 데일리 루틴 삭제 #37

Merged
merged 10 commits into from
Jan 10, 2024
Merged

Conversation

thguss
Copy link
Member

@thguss thguss commented Jan 10, 2024

✨ Related Issue

📝 기능 구현 명세

Uploading image.png…

🐥 추가적인 언급 사항

@thguss thguss added sohyeon 소현 작업 hold review 리뷰 보류 (이전 PR 영향을 받을 때) delete labels Jan 10, 2024
@thguss thguss self-assigned this Jan 10, 2024
Copy link
Contributor

@csb9427 csb9427 left a comment

Choose a reason for hiding this comment

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

너무 고생하셨습니다!! 다만 궁금한 점이 같은 파일에 작성하는 코드는 계속 이렇게 합쳐서 보이는 걸까요?? 뭔가 삭제 pr인데 루틴 추가 코그도 같이 보여서..

@Getter
public enum ErrorMessage {
INVALID_MEMBER("유효하지 않은 회원입니다."),
INACCESSIBLE_ROUTINE("해당 회원이 접근할 수 없는 루틴입니다."),
Copy link
Contributor

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.

@csb9427 오호 그럴 수도 있겠네요! 그럼 "회원의 루틴이 아닙니다" 메세지는 좀 더 직관적일까요?? 그 외에 추천해주실 메세지가 있다면 추천 부탁드려요!

Copy link
Contributor

Choose a reason for hiding this comment

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

회원의 루틴이 아닙니다 좋은 거 같아요!!


private final MemberDailyRoutineService memberDailyRoutineService;

@PostMapping
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 밑에 루틴 추가 pr이 있어서 합쳐서 보여지는 건가?

@thguss
Copy link
Member Author

thguss commented Jan 10, 2024

너무 고생하셨습니다!! 다만 궁금한 점이 같은 파일에 작성하는 코드는 계속 이렇게 합쳐서 보이는 걸까요?? 뭔가 삭제 pr인데 루틴 추가 코그도 같이 보여서..

@csb9427 직전 PR이 병합이 되지 않아서 함께 포함되는 형상입니다ㅠㅠ(어제 언급했던 이슈!) 그래서 hold-review 태그를 걸어놓긴 했는데 참고해주시면 좋을 것 같아요 :)

@csb9427
Copy link
Contributor

csb9427 commented Jan 10, 2024

너무 고생하셨습니다!! 다만 궁금한 점이 같은 파일에 작성하는 코드는 계속 이렇게 합쳐서 보이는 걸까요?? 뭔가 삭제 pr인데 루틴 추가 코그도 같이 보여서..

@csb9427 직전 PR이 병합이 되지 않아서 함께 포함되는 형상입니다ㅠㅠ(어제 언급했던 이슈!) 그래서 hold-review 태그를 걸어놓긴 했는데 참고해주시면 좋을 것 같아요 :)

아하 그렇군요.. 흐음 저도 같은 상황에 hold-review 태그 걸겠습니당

@thguss thguss removed the hold review 리뷰 보류 (이전 PR 영향을 받을 때) label Jan 10, 2024
Copy link
Contributor

@Chan531 Chan531 left a comment

Choose a reason for hiding this comment

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

젠장.. 또 대소현이야.. 잘라했는데... 이 리뷰만 남기고 자야겠다... 그저 GOAT... 숭배해야만해...

.orElseThrow(() -> new EntityNotFoundException(INVALID_ROUTINE.getMessage()));
}

private void checkRoutineForMember(Member member, MemberDailyRoutine routine) {
Copy link
Contributor

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.

논의 완료!

@@ -44,4 +47,11 @@ private URI getURI() {
.buildAndExpand()
.toUri();
}

@DeleteMapping("/routine/{routineId}")
Copy link
Contributor

Choose a reason for hiding this comment

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

"/routine/{routineId}" 말고 "/{routineId}"는 좀 그럴려나요??

Copy link
Member Author

Choose a reason for hiding this comment

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

자원의 표현성으로 의논 완료!

@thguss thguss merged commit 9328a5a into develop Jan 10, 2024
1 check passed
@thguss thguss deleted the feature/#35-routine-delete-api branch January 10, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delete sohyeon 소현 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 회원 데일리 루틴 삭제
3 participants