-
Notifications
You must be signed in to change notification settings - Fork 0
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
회원가입 약관 동의 api 수정 및 온보딩 관심 아티스트 등록 api 추가 #25
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.
수고 정말 많으셨습니다..!
비즈니스 코드쪽만 읽어보았고, 테스트 코드는 천천히 읽어보도록 하겠습니다
작성하신 코드와 별개로 여쭤보고 싶은 게 있어 여기에 적습니다!
현재 sign-in과 sign-up을 분리하셨는데, sign-in을 무조건 시도해보고 예외에 의한 에러 응답을 얻는다면 sign-up을 요청하는 로직을 생각하신 게 맞을까요?
Optional.ofNullable(imageUrl) | ||
.ifPresent(DomainUtils::checkUrl); |
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.
이 부분과 DTO 쪽 읽고 알게 된 건데, 아티스트 이미지가 없는 경우도 허용되는 걸까요?
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.
이미지같은 경우는 데이터를 어떤 식으로 저장하게 될지 모르기도 하고 추후에 애플리케이션이 좀 성숙해졌을 때 결정할 일인 것 같아서 우선 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.
네 이해했습니다 감사합니다!!
@Override | ||
protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException { | ||
String path = request.getRequestURI(); | ||
return Arrays.asList(excludePath).contains(path); | ||
} | ||
|
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.
exceptionHandler
방식과 requestMatchers("/api/**").authenticated()
을 동시에 사용하는 경우에는 이 방법 외에는 해결할 방법이 잘 보이지 않더라구요..ㅎㅎ
public static <T extends CursorResponse> CursorBasePaginatedResponse<T> of(Slice<T> slice) { | ||
return new CursorBasePaginatedResponse<>( | ||
slice.getContent().isEmpty() ? null : slice.getContent().getLast().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.
CursorResponse
여기서도 사용하기 좋은 것 같네요!
public static BooleanExpression gtCursorId(Long cursor, NumberPath<Long> id) { | ||
return cursor == null ? null : id.gt(cursor); |
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.
혹시 goe에서 gt로 가게 되면서 버그가 날 수 있는 부분은 없을까요?
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.
cursor값이 어떻게 들어오냐에 따라 달라질 수 있을 것 같네요
저번 리팩터링에서 nextCursor을 주는 방식을 바꿨기 때문에 현재로는 goe가 들어오면 버그가 발생합니다 🙂
); | ||
} | ||
|
||
public static User of(String email, Auth auth, String username, Gender gender, int birthYear, TermsAgreements termsAgreements) { | ||
public static User of(String email, Auth auth, String username, Gender gender, int birthYear, List<TermsAgreements> termsAgreements) { |
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.
of
를 테스트 용도로만 사용하고 있는 것 같은데,
도메인에 테스트만을 위한 로직이 있어도 될까 하는 의문이 있습니다...
🔥 Task