-
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
feat: Apply 관련 기능 개발 #199
feat: Apply 관련 기능 개발 #199
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.
코멘트 남겼습니다. 확인 부탁드릴게요! 고생 많으셨습니다😄
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.
여유되시면, @Schema
이용해서 Swagger에 예시 삽입해주시면 더 좋을 것 같아요
|
||
@Size(max = 20) | ||
@NotNull | ||
ApplyStatus status, |
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.
이렇게 반환하게 되면, 영어문자로 반환되어 프론트에서 추가적인 변환이 필요할 것 같아요.
String status
로 바꾸면 좋을 것 같아요. 아래의 다른 리뷰랑 엮어서 같이 보시면 이해될 것 같습니다!
.portfolioId(apply.getPortfolioInfo().getPortfolio() != null ? apply.getPortfolioInfo().getPortfolio().getId() : null) | ||
.portfolioTitle(apply.getPortfolioInfo().getPortfolio() != null ? apply.getPortfolioInfo().getPortfolio().getTitle() : null) | ||
.body(apply.getBody()) | ||
.status(apply.getStatus()) |
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.
이 부분을 String으로 바뀌었으니, apply.getStatus().getStatus()
하면 Enum의 한국어 설명이 반영될 것 같아요.
다만, getStatus().getStatus()
는 가독성이 좋은 네이밍이 아니니, 아래의 Enum에서 koreanName()
과 같이 변경해서 활용하면 더 좋을 것 같네요!
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.
질문해주신, 비공개와 관련된 측면은 말씀해주신 것처럼 하시면 큰 문제 없을 것 같아요!
아니면, 별도로 칼럼을 만들어 is_public
과 같이 비공개/공개 여부를 관리하셔도 되고요.
정책적인 측면이라, 하나로 고정만 하면 큰 문제 없을 것 같다는게 현재 제 생각입니다!
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.
아래에 보니까, isPrivate
가 존재하는데, 이게 제가 얘기하고자 하는 것과 관련된 부분인 것 같아요..
맞다면, 신경쓰지 않으셔도 될 것 같아요!
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.
현재 이 쿼리는 연관된 데이터들의 논리적 삭제는 고려하지 않는 것으로 보여요. 특별한 이유가 있을까요?
특별한 이유가 없다면, Join도 논리적 삭제에 대한 필터링이 필요할 것 같아요!!
|
||
public ApplyRes apply(Member member, Long teamId, ApplyReq applyReq) { | ||
//Validation: member, teamId, 지원 가능 기간인지, 리더가 지원하는지, 이미 지원했는지, 본인의 포트폴리오인지 유효성 검사 | ||
if(member == 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.
이렇게 하기보다는, Spring Security에서 해당 엔드포인트로 접근할 때는 사용자 정보가 있어야 한다는 식으로 관리하는 것은 어떨까요?
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.
여기는, 최근 업데이트 날짜까지 같이 반환하고 있으면 더 좋지 않을까 싶어요! 아마 기획/디자인 팀에서도 최근 업데이트일자를 알고 있는게 사용자에게 더 좋은 정보라고 생각할 것 같아서, 이건 제가 요청해놓을게요!
public interface TeamRepositoryCustom { | ||
|
||
Optional<Team> findTeamById(Long id); |
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.
JPA로도 구현이 가능할 것 같은데, QueryDSL을 활용하신 이유가 궁금해요!
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.
Team을 가져오는 과정에서 member테이블과의 fetch join을 사용해야해서 활용하였습니다!
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.
지원자를 선발/미선발하는 기능을 하나의 controller에서 해결하기 위해 status를 받는 방식으로 해두었습니다! 파일구조 정리하는 과정에서 domain을 team으로 잘못둔 것 같은데 apply domain쪽으로 옮겨두겠습니다!
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.
지원서 열람 API와 지원서를 가져오는 API를 구분한 이유가 궁금해요!
이걸, 사용자 정보를 가져올테니, 조건 분기로 처리하는 게 API 2번 요청에서 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.
수정사항까지 반영하시느라 너무 고생 많으셨습니다!
#️⃣ 연관된 이슈
📝 작업 내용
스크린샷 (선택)
💬 리뷰 요구사항(선택)