-
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
[YS-173] refact: ExperimentPost의 recruitDone
컬럼명 변경 및 공고 전체 조회 API, 지역별 공고 개수 조회 API 로직 리팩터링
#42
Conversation
recruitDone
컬럼명 변경 및 공고 전체 조회 API, 지역별 공고 개수 조회 API 로직 리팩터링recruitDone
컬럼명 변경 및 공고 전체 조회 API, 지역별 공고 개수 조회 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.
컬럼명을 변경하면서 조회 로직을 다시 살펴봤는데 공고 전체 조회와 지역별 공고 개수 조회 API에 대해 기획적으로 맞지 않는 부분들이 보여 함께 수정하느라 많은 파일이 변경되었습니다. 😂 커밋별로 봐주시면 감사합니다...
추가적으로 해당 PR이 머지된다면 개발 서버의 DB에 있던 기존 컬럼을 삭제해야 해서 리뷰만 부탁드립니다!
val recruitStatusCondition = when (customFilter.recruitStatus) { | ||
RecruitStatus.ALL -> null | ||
RecruitStatus.OPEN -> post.recruitStatus.eq(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.
기존 로직에서는 모집 중인 공고 / 모집 완료인 공고
둘로 나누어 진행이 되던데 기획 상으로는 모집 중인 공고 / 모든 공고
둘로 나누는 것이 맞다고 생각해 수정했습니다! 틀렸다면 알려주세요~
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.
CustomFilter에서는 기획 요청 상 ALL
과 OPEN
으로 나누어보는 것이 맞는 것 같습니다. 👍👍
@RequestParam(required = false) region: String?, | ||
@RequestParam(required = false, defaultValue = "ALL") recruitStatus: 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.
생각해보니 공고 수 조회 API에서도 recruitStatus를 고려해야 할 거 같아 추가했습니다! 아니라면 알려주세요~
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.
단순히 recruitDone
필드를 recruitStatus
필드로 바꾸는 리팩터링 작업인데, 생각보다 수정사항이 굉장히 크군요. 😅💦
그래도 꼼꼼하게 작업해주신 점이 매우 인상 깊었습니다!
관련해서 코멘트 남겨드렸으니 한번만 확인 부탁드립니다
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.
해당 쿼리에 GROUP BY
를 사용하는 이유가 특정 시/도(region)과 recruitStatus
조건에 맞는 ExperimentPost를 시/군/구(area) 별로 집계하기 위해서라고 받아들였습니다.
만약 ExperimentPost 테이블의 데이터가 많아졌을 때는 해당 쿼리는 빈번히 호출될 것으로 생각됩니다. region
과 recruitStatus
는 자주 조회조건으로 사용되니까, 필드에 대한 복합 인덱스를 추가하는 것도 추후의 개선 작업에서 고려해볼만한 사항이라고 생각되네요!
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.
현재 enum값으로 정의한 recruitStatus
를 왜 쿼리 파라미터로 받아오실때는 String으로 받아오시는지 궁금합니다!
앞서 다른 Region
이나 Area
같이 다른 enum 클래스의 경우에는 그냥 enum의 형태로 파라미터를 가져왔어서요!
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으로 수정하겠습니다! 감사합니다 👍
Quality Gate passedIssues Measures |
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.
수정사항 반영해주셔서 감사합니다! 😊😊
💡 작업 내용
recruitDone
컬럼명 변경✅ 셀프 체크리스트
🙋🏻 확인해주세요
🔗 Jira 티켓
https://yappsocks.atlassian.net/browse/YS-173