-
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: JD 등록, 자소서 등록 API 구현(#56) #60
Conversation
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.
확인했습니다! 고생해써~
data class CreateApplyContent ( | ||
val question: String, | ||
val answer: String | ||
) |
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.
CreateApply랑 별도 파일로 작업하시는 이유가 있나요?!
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.
이 부분은 저 또한 분리할 이유가 없다 생각이 들어 리펙터링 할 때 로직을 합치려고 했습니다! 좋은 리뷰 감사합니다:)
request.contents.forEach { content -> | ||
applyContentAppender.appendApplyContent( | ||
applyId = apply.id, | ||
question = content.question, | ||
answer = content.answer | ||
) | ||
} |
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.
bulk 작업 처리 되면 좋을 것 같아요
fun toDomain( | ||
id: UUID, | ||
title: String, | ||
writeStatus: WriteStatus, | ||
createdAt: LocalDateTime, | ||
updatedAt: LocalDateTime, | ||
jobDescriptionId: UUID | ||
): Apply { | ||
return Apply( | ||
id = id, | ||
title = title, | ||
writeStatus = writeStatus, | ||
createdAt = createdAt, | ||
updatedAt = updatedAt, | ||
jobDescriptionId = jobDescriptionId | ||
) | ||
} |
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.
좋은 리뷰 감사합니다! 관련해서 리팩터링 해보도록 하겠습니다:)
val updatedAt: LocalDateTime, | ||
val startedAt: LocalDateTime, | ||
val endedAt: LocalDateTime, | ||
val userId: UUID, |
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.
도메인의 이해가 부족해서인지 jd에 userId가 들어가야 하는지 이해하기 어려운 것 같아요!
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.
사용자 별로 JD를 등록할 수 있고 작성한 JD를 따로 조회하는 로직이 있어 추가했었습니다!
applyContent.answer shouldBe answer | ||
} | ||
|
||
test("applyContent 입력값으로 공백이 들어오면 에러 반환") { |
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.
입력 파라미터에 따라 테스트 분리하고 공백 외에도 whitespace도 고려해보면 좋을 것 같아요.
✨ PR 유형
어떤 변경 사항이 있나요??
🛠️ 작업내용
📋 추후 진행 사항
📌 리뷰 요청 사항