-
Notifications
You must be signed in to change notification settings - Fork 1
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: Recruitment 생성 API 구현 #345
Conversation
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
import org.springframework.web.bind.annotation.RestController; | ||
|
||
@Tag(name = "Admin Recruitment", description = "어드민 리쿠르트먼트 관리 API입니다.") | ||
@RestController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희 근데 리크루팅으로 하자고 하지 않았나용? 기억이 안나네욥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무 길다고 '리쿠르팅' 쓰기로 했던 것 같아요.
테이블 명이랑 안 맞는게 좀 아쉽긴하지만 swagger 문서에는 모두 리쿠르팅으로 변경하겠습니다.
++ 영문은 recruitment로 두는게 api 경로랑 혼동이 적겠죠?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵넵 리크루팅은 단지 프론트/기획/백엔드 용어 통일을 위해 정한 것 같고 영어로 recruitment는 이게 리쿠르팅이다 라는것을 저희가 백끼리 이해만 하면될거같아요
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
@@ -10,4 +11,11 @@ public enum SemesterType { | |||
SECOND("2학기"); | |||
|
|||
private final String value; | |||
|
|||
public static SemesterType from(LocalDateTime dateTime) { | |||
if (dateTime.getMonthValue() < 7) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 조건도 추출해서 의미있는 이름으로 만들면 좋을듯 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 semesterType에 대한 정책도 달라져야 할 것 같습니다.
학기의 시작은 3월 1일이므로 해당 기준에 맞춰서 설정되어야 하고,
이때 유의해야 할 점은 recruitment의 startDate와 endDate는 학기 시작일 이전일 수 있다는 겁니다. 가령 3월에 1학기 시작이 이루어지더라도, 모집일 자체는 2월일 수 있겠지요. 이러한 부분을 고려하여 구현하셔야 합니다.
그리고 해당 정책도 적절히 문서화해서 공유할 수 있는 방법을 찾아봐야 할듯 합니다.
import java.time.LocalDateTime; | ||
|
||
public record RecruitmentCreateRequest( | ||
@NotBlank @Schema(description = "이름") String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학년 / 학기 정보를 클라이언트로부터 받는게 낫지 않을까요?
그리고 시작일 / 종료일이 해당 학년 / 학기 정보에 대하여 유효한지 validate만 하면 될 것 같습니다.
정회원들을 준회원으로 강등하는 로직이 빠졌네요. |
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
@@ -10,4 +14,30 @@ public enum SemesterType { | |||
SECOND("2학기"); | |||
|
|||
private final String value; | |||
|
|||
public static SemesterType from(LocalDateTime dateTime) { | |||
return getSemesterType(dateTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
분리하신 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 조건도 추출해서 의미있는 이름으로 만들면 좋을듯 합니다.
분리하자는 의미 아니었나요?
|
||
private static SemesterType getSemesterType(LocalDateTime dateTime) { | ||
int year = dateTime.getYear(); | ||
LocalDateTime firstSemesterStartDate = LocalDateTime.of(year, 3, 1, 0, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직 넘버는 상수로 추출해주세요~
ex) 3월 1일, 2주, 7월
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3월 1일과 9월 1은 아래와 같이 처리하면 좋을 것 같네요👍👍
public enum SemesterType {
FIRST("1학기", MonthDay.of(3, 1)),
SECOND("2학기", MonthDay.of(9, 1));
private final String value;
private final MonthDay startDate;
}
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
|
||
private final String value; | ||
private final MonthDay startDate; | ||
|
||
public static SemesterType from(LocalDateTime dateTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 메서드가 적절한 책임을 가지지 못하는 것 같습니다.
저라면 이 메서드를 보고 "대충 enum 멤버로 학기 시작일이 있으니까, 인자로 받은 dateTime이 학기 시작일 ~ 종료일 사이이면 해당 학기타입을 반환하겠구나?" 라고 생각할 것 같아요.
그런데 실제 로직을 보면 학기 시작일 2주 전에 해당하는 dateTime도 해당 학기타입으로 매핑해주고 있습니다. 이렇게 하면 처음 보는 사람은 조금 혼란스러울 것 같다는 생각이 듭니다.
저희가 이렇게 했던 이유가 뭐였나요? 인자로 들어오는 값이 리쿠르팅의 시작일과 마감일이었기 때문이었죠.
여기서 두 가지 문제가 있는데요,
- SemesterType의 내부 로직이, 이러한 리쿠르팅에 의존적인 컨텍스트 정보를 알 필요가 없고, 안다 하더라도 리쿠르팅이 가져야 할 책임을 학기타입이 가져가는 것이기 때문에 SRP 위반이고요,
SemesterType::from
으로 들어오는 값이 항상 리쿠르팅의 시작일과 마감일이라는 보장이 없다는 겁니다. 다른 로직에서 언제든지 실수하고 아, LocalDateTime을 SemesterType으로 매핑해주는 정적 메서드구나 하고 쓸 여지가 많습니다.
2번의 경우 인자를 Recruitment로 받거나 하면 해결될 수 있지만 1번을 해결 못했기 때문에 근본적인 해결책은 아니죠.
결국 학기타입 매핑 로직은 리쿠르팅 서비스로 빼내는 것이 적절해보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇네요.
저도 SemesterType.from이 항상 LocalDateTime만 받는게 맞을지, 그리고 받는 LocalDateTime에 대해 항상 같은 로직으로 SemesterType을 반환하는게 맞을지에 대한 고민이 있었는데 서비스로 옮기니 마음에 걸리던데 해소된 느낌이네요👍
LocalDateTime secondSemesterStartDate = | ||
LocalDateTime.of(year, SECOND.startDate.getMonth(), SECOND.startDate.getDayOfMonth(), 0, 0); | ||
|
||
if (dateTime.isAfter(firstSemesterStartDate.minusWeeks(TWO_WEEKS)) && dateTime.getMonthValue() < JULY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고, "리쿠르팅의 경우 개강일 전에 시작하므로, 리쿠르팅 시작일이 2주 전이더라도 1학기로 매핑한다" 같은 식의 컨텍스트 정보를 주석에 쓰는 것도 나쁘지 않을 것 같아요. 저는 정책적인 부분은 주석으로 남기는 거 충분히 의미있다고 생각합니다.
리쿠르팅 시작이 2주 전이라는 것도 뭔가 빡빡해보이는데, 2주로 할지 4주로 할지에 대한 것도 운영진 쪽에서 컨펌받고 진행해야 하지 않을까 싶네요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앞뒤로 2주라는 기간은 운영진과 협의해서 정한 부분입니다!
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
|
||
@Tag(name = "Admin Recruitment", description = "어드민 리쿠르팅 관리 API입니다.") | ||
@RestController | ||
@RequestMapping("/admin/recruitment") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RequestMapping("/admin/recruitment") | |
@RequestMapping("/admin/recruitments") |
/* | ||
개강일 기준으로 2주 전까지는 같은 학기로 간주한다. | ||
*/ | ||
if (dateTime.isAfter(firstSemesterStartDate.minusWeeks(TWO_WEEKS)) && dateTime.getMonthValue() < JULY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 상수명을 TWO_WEEKS로 하면 나중에 3주나 4주로 정책이 변경되었을 때 상수명을 전부 바꿔야 하지 않을까요?
상수에도 적절한 이름을 붙여야 할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 기간을 학기 준비 기간
이라는 이름으로 정책에 추가했습니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
/* | ||
개강일 기준으로 2주 전까지는 같은 학기로 간주한다. | ||
*/ | ||
if (dateTime.isAfter(firstSemesterStartDate.minusWeeks(PRE_SEMESTER_TERM)) && dateTime.getMonthValue() < JULY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JULY 대신 java에서 제공하는 Month를 사용하면 좋을 것 같습니다.
public static final int JULY = 7; | ||
|
||
// 학기 준비 기간 | ||
public static final int PRE_SEMESTER_TERM = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'주' 단위라는 것도 명시되어야 할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
위 내용만 수정하시면 될 것 같습니다.
@@ -66,7 +66,12 @@ public enum ErrorCode { | |||
// Recruitment | |||
DATE_PRECEDENCE_INVALID(HttpStatus.BAD_REQUEST, "종료일이 시작일과 같거나 앞설 수 없습니다."), | |||
RECRUITMENT_NOT_OPEN(HttpStatus.CONFLICT, "리크루트먼트 모집기간이 아닙니다."), | |||
RECRUITMENT_NOT_FOUND(HttpStatus.NOT_FOUND, "열려있는 리크루트먼트가 없습니다."); | |||
RECRUITMENT_NOT_FOUND(HttpStatus.NOT_FOUND, "열려있는 리크루트먼트가 없습니다."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한글 워딩은 '리쿠르팅'으로 통일해야 합니다. 해당 건은 별도 이슈로 작업하셔도 됩니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전범위로 검색해보니 여기에만 있네요
자잘한 부분이니 여기서 처리할게요
꼼꼼히 봐주셔서 감사드려요
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
🌱 관련 이슈
📌 작업 내용 및 특이사항
academicYear
와semesterType
을 dto로 받을지 고민이었는데 일단은 모집기간 시작일을 기준으로 서버에서 처리하도록 구현했습니다.dto로 입력 받는다면 모집기간 시작일과 어긋나지 않는지 (예를 들어, 모집 기간 시작일은 24년 2월인데academicYear
와semesterType
을 2023,SECOND
로 입력한 경우) 검증이 필요해지므로,서버에서 직접 처리하는게 좋다고 판단했습니다.
semesterType은 시작일의 Month를 기준으로 1 ~ 6월은FIRST
, 7 ~ 12월은SECOND
가 되도록 했습니다.ii의 경우 dto에서 어노테이션으로 검증되는 부분이라 test를 작성하지는 않았습니다.
📝 참고사항
이 부분은 코드 몇 줄로 끝나지 않을 것 같고 별도 이슈로 만들어 처리하는게 pr 리뷰하시기에 편할 것 같아서 todo 주석만 남겨뒀습니다.
📚 기타