-
Notifications
You must be signed in to change notification settings - Fork 6
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-be(dashboard): 특정 동아리의 대시보드(공고) 생성 API 구현 #215
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.
수고하셨습니다, 도비 :)
리뷰 남겼으니 확인해주세요.
backend/src/main/java/com/cruru/applyform/domain/ApplyForm.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applyform/domain/ApplyForm.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/cruru/applyform/domain/repository/ApplyFormRepositoryTest.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/cruru/applyform/domain/repository/ApplyFormRepositoryTest.java
Outdated
Show resolved
Hide resolved
This reverts commit 314654a.
…ASHBOARD_01 # Conflicts: # backend/src/main/java/com/cruru/DataLoader.java # backend/src/main/java/com/cruru/applyform/domain/ApplyForm.java # backend/src/main/java/com/cruru/applyform/domain/repository/ApplyFormRepository.java # backend/src/main/java/com/cruru/applyform/service/ApplyFormService.java # backend/src/main/java/com/cruru/question/domain/Question.java # backend/src/main/java/com/cruru/question/domain/QuestionType.java # backend/src/main/java/com/cruru/question/exception/QuestionNotFoundException.java # backend/src/test/java/com/cruru/applyform/domain/repository/ApplyFormRepositoryTest.java # backend/src/test/java/com/cruru/question/domain/repository/QuestionRepositoryTest.java # backend/src/test/java/com/cruru/util/fixture/ApplyFormFixture.java
1722484351.650359 |
📌 Test Coverage Report
|
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.
QuestionServiceTest와 DashboardFacadeTest가 누락된 것 같습니다. 생성해주세요.
backend/src/main/java/com/cruru/applyform/domain/ApplyForm.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/choice/controller/dto/ChoiceCreateRequest.java
Show resolved
Hide resolved
backend/src/main/java/com/cruru/choice/exception/ChoiceEmptyBadRequestException.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/choice/exception/ChoiceBadRequestException.java
Outdated
Show resolved
Hide resolved
|
||
@Transactional | ||
public List<Choice> createAll(List<ChoiceCreateRequest> requests, Question question) { | ||
Question targetQuestion = questionRepository.findById(question.getId()) |
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.
savedQuestion을 사용하는데 다시 questionRepository에서 찾아오는 이유가 있나요?
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.
여기서는 상위 파사드 서비스에서 savedQuestion을 전달해줬습니다. 일반적인 use case에서 저장되지 않은 Question에 대해 Choice 저장을 시도하는 것을 미연에 방지를 위함입니다.
근데 적어놓고 보니 의도를 위해서는 Question을 파라미터로 받는 것이 아닌 Question Id를 전달받는 것이 나아보입니다
backend/src/test/java/com/cruru/choice/service/ChoiceServiceTest.java
Outdated
Show resolved
Hide resolved
// given | ||
ApplyForm applyForm = createBackendApplyForm(); | ||
ApplyForm applyForm = ApplyFormFixture.createBackendApplyForm(null); |
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.
제가 작성한 코드에는 모두 가독성을 위해 클래스명을 명시했습니다. 컨벤션에 대해 논의가 필요해보입니다
|
||
// when & then | ||
assertAll(() -> { | ||
assertThrows(ChoiceBadRequestException.class, |
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.
assertJ의 assertThatThrownBy를 사용하지 않으신 이유가 있나요?
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.
습관적으로 사용했습니다. 혹시 assertThatThrownBy를 사용해야하는 이유가 있을까요?
backend/src/test/java/com/cruru/choice/service/ChoiceServiceTest.java
Outdated
Show resolved
Hide resolved
- Question question -> long QuestionId
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.
수고하셨습니다 도비! 의존성 생각하느라 복잡했을것 같네요
간단한 코멘트 남겨두었습니다.
backend/src/main/java/com/cruru/choice/service/ChoiceService.java
Outdated
Show resolved
Hide resolved
@@ -30,9 +31,10 @@ public class ApplyForm extends BaseEntity { | |||
|
|||
private String description; | |||
|
|||
@Setter |
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.
다른 도메인에서(Process)는 @Setter
를 직접 사용하지 않고 update필드명
으로 메서드명을 지어준 것 같습니다. 이에 대해 혹시 어떻게 생각하시나용?
- 하나로 통일하는 것(세터가 필요한 곳에
@Setter
또는update필드명
- 혼용해서 쓰자
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.
해당 Process에서는 기존에 존재하는 정보를 '변경'하는 메서드로 update가 맞지만, 생성되기 전까지 Url이 존재하지 않는 이 케이스에 관해서는 setter의 의미가 더 적절한 것 같습니다.
오히려 update의 의미를 가진 메서드를 만들면 URL이 변동 가능성이 있는 field로 받아들여질 것 같습니다.
저는 필요와 의미에 따라 혼용하는 것을 선택할 것 같습니다
SHORT_ANSWER("SHORT_ANSWER", false), | ||
LONG_ANSWER("LONG_ANSWER", false), | ||
DROPDOWN("DROPDOWN", true), | ||
CHECK_BOX("CHECK_BOX", true), | ||
MULTIPLE_CHOICE("MULTIPLE_CHOICE", true), |
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명과 동일한 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.
valueOf로 우선적으로 사용하였지만, client단에서 전달된 파라미터가 Enum명에 직접적으로 영향을 줄 수도 있어 우선 field로 완충 작용을 위해 적용하였습니다.
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
|
||
public ApplyForm( | ||
String title, String description, LocalDateTime openDate, LocalDateTime dueDate, | ||
Dashboard dashboard) { |
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.
개행을 다시 확인해주세요.
|
||
import com.cruru.advice.badrequest.BadRequestException; | ||
|
||
public class ChoiceEmptyBadRequestException extends BadRequestException { |
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~
authored-by: Do Yeop Kim <113661364+Dobby-Kim@users.noreply.github.com>
Original issue description
목적
작업 세부사항
참고 사항
CREATE_DASHBOARD_01
closes #214