-
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: 프로세스 목록 조회 API 필터링 및 정렬 구현 #756
Conversation
1727854674.677589 |
📌 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.
안녕하세요, 냥인! 😸
필터링 기능 구현하느라 수고하셨습니다!
관련해서 리뷰 남겼으니, 확인해주세요!
backend/src/test/java/com/cruru/applicant/service/ApplicantCardFilterTest.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/service/ApplicantCardFilter.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/service/ApplicantCardSorter.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/service/ApplicantService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/service/ApplicantService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/process/controller/ProcessController.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/process/controller/ProcessController.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/service/ApplicantCardFilter.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/process/facade/ProcessFacade.java
Outdated
Show resolved
Hide resolved
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.
고생하셨습니다 냥인. 리뷰 남겨놓았으니 확인해주세요~
List<ApplicantCard> applicantCards = applicantRepository.findApplicantCardsByProcesses(processes); | ||
|
||
return applicantCards.stream() | ||
.filter(card -> ApplicantCardFilter.filterByScore(card, minScore, maxScore)) |
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.
메서드를 보니 applicantCard에 로직이 있으면 깔끔했겠다는 생각은 드네용. 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.
😿
backend/src/main/java/com/cruru/process/controller/ProcessController.java
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/service/ApplicantService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/process/controller/ProcessController.java
Outdated
Show resolved
Hide resolved
LocalDateTime.of(2024, 10, 2, 20, 0), false, 5, 3.00, 1L); | ||
ApplicantCard applicantCard2 = new ApplicantCard(2L, "최냥인", | ||
LocalDateTime.of(2024, 10, 3, 20, 0), false, 0, 0.00, 1L); | ||
List<ApplicantCard> applicantCards = Arrays.asList(applicantCard1, applicantCard2); |
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.
List<ApplicantCard> applicantCards = Arrays.asList(applicantCard1, applicantCard2); | |
List<ApplicantCard> applicantCards = List.of(applicantCard1, applicantCard2); |
들어가는 인자가 array라고 착각할 수도 있을 것 같아요~
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.
정렬을 해야 해서 List.of()는 불가능입니다..ㅎ ㅎ
backend/src/main/java/com/cruru/applicant/service/ApplicantCardSorter.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/cruru/process/facade/ProcessFacadeTest.java
Outdated
Show resolved
Hide resolved
+) 변수명 통일
ApplicantCard, DefaultFilterAndOrderFixture
- ApplicantCardFilter -> EvaluationStatus, ApplicantService - ApplicantCardSorter -> ApplicantSortOption
[주요 변경 사항 요약]
|
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.
구현과 리팩터링 양이 꽤 되었을텐데, 고생 많으셨습니다 :)
간단한 리뷰 남겼는데, 엄청 중요한 부분은 아니여서 approve하겠습니다!
수고 많으셨어요!
backend/src/main/java/com/cruru/applicant/domain/EvaluationStatus.java
Outdated
Show resolved
Hide resolved
@Chocochip101 |
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.
고생하셨습니다 냥인. RESTDocs 관련 코멘트도 남겨놓았어요.
import java.util.Arrays; | ||
import java.util.Comparator; | ||
|
||
public enum ApplicantSortOption { |
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.
raw type 에러가 뜨는 게 보기 싫어서 아래와 같이 바꿔보았는데 어떤가요..? 다른 방법도 좋습니다 그냥 노란 줄을 없애고 싶어요. (여러 번 시도해봤는데 어렵네요 제네릭 ㅠㅠ)
package com.cruru.applicant.domain;
import com.cruru.applicant.domain.dto.ApplicantCard;
import com.cruru.applicant.exception.badrequest.ApplicantSortException;
import java.util.Arrays;
import java.util.Comparator;
public class ApplicantSortOption {
public static Comparator<ApplicantCard> getCombinedComparator(String sortByCreatedAt, String sortByScore) {
Comparator<ApplicantCard> createdAtComparator = CreatedAtSortOption.convertToComparator(sortByCreatedAt);
Comparator<ApplicantCard> scoreComparator = ScoreSortOption.convertToComparator(sortByScore);
return createdAtComparator.thenComparing(scoreComparator);
}
enum CreatedAtSortOption {
ASC(Comparator.comparing(ApplicantCard::createdAt, Comparator.naturalOrder())),
DESC(Comparator.comparing(ApplicantCard::createdAt, Comparator.reverseOrder()));
private final Comparator<ApplicantCard> comparator;
CreatedAtSortOption(Comparator<ApplicantCard> comparator) {
this.comparator = comparator;
}
private static Comparator<ApplicantCard> convertToComparator(String sortOption) {
return Arrays.stream(CreatedAtSortOption.values())
.filter(option -> option.name().equalsIgnoreCase(sortOption))
.findAny()
.orElseThrow(ApplicantSortException::new)
.comparator;
}
}
enum ScoreSortOption {
ASC(Comparator.comparing(ApplicantCard::averageScore, Comparator.naturalOrder())),
DESC(Comparator.comparing(ApplicantCard::averageScore, Comparator.reverseOrder()));
private final Comparator<ApplicantCard> comparator;
ScoreSortOption(Comparator<ApplicantCard> comparator) {
this.comparator = comparator;
}
private static Comparator<ApplicantCard> convertToComparator(String sortOption) {
return Arrays.stream(ScoreSortOption.values())
.filter(option -> option.name().equalsIgnoreCase(sortOption))
.findAny()
.orElseThrow(ApplicantSortException::new)
.comparator;
}
}
}
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.
노란 줄.. 시각적 효과가 굉장하죠.
저도 없애보려다가 실패 좀 맛봤는데요.
이 부분은 좀 더 얘기를 나눠볼게요...
requestCookies(cookieWithName("token").description("사용자 토큰")), | ||
queryParameters( | ||
parameterWithName("dashboardId").description("대시보드의 id"), | ||
parameterWithName("minScore").description("지원자 최소 평균 점수: 0.00(default) ~ 5.00"), |
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.
parameterWithName("minScore").description("지원자 최소 평균 점수: 0.00(default) ~ 5.00"), | |
parameterWithName("minScore").description("지원자 최소 평균 점수: 0.00(default) ~ 5.00").optional(), |
optional() 필드를 추가하고 커스텀 스니펫을 생성하면 optional이 표시되도록 만들 수 있습니다.
참고
backend/src/main/java/com/cruru/applicant/service/ApplicantService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/cruru/applicant/domain/EvaluationStatus.java
Outdated
Show resolved
Hide resolved
matchesEvaluationStatus -> matches
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.applicant.domain.EvaluationStatus; | ||
import com.cruru.applicant.domain.dto.ApplicantCard; | ||
|
||
public class ApplicantCardFilter { |
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.
Filter
라는 클래스명을 사용하면 기존의 Spring의 Filter
와 혼동 가능성이 있을 수 있으니 다른 이름을 고안해보시는건 어떨까요!
사실 저는 당장 생각이 나지는 않네요..ㅎㅎ
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.
이제는 ApplicantCard
가 DTO로서의 역할에서 많이 벗어난 것 같습니다!
ApplicantCard
를 엔티티+도메인인 Applicant
와 별개인 도메인으로 다루는 클래스로 따로 빼는 것이 좋지 않을까요?
또한 이 도메인을 Comparator
interface를 상속받는 구현체로 만들어 compare
메서드를 오버라이딩하는 식으로 정렬 기준을 정해주면 전체 코드 복잡도를 줄일 수 있을 것 같습니다
@@ -33,9 +33,17 @@ public class ProcessController { | |||
@RequireAuthCheck(targetId = "dashboardId", targetDomain = Dashboard.class) | |||
@GetMapping | |||
public ResponseEntity<ProcessResponses> read( | |||
LoginProfile loginProfile, @RequestParam(name = "dashboardId") Long dashboardId |
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.
기존의 read 메서드를 변경한 이유가 있을까요?
기존 API와 분리되어야되지 않나 싶습니다!
또한, 이 정도 사이즈의 파라미터 개수이고, 변경될 Api라면 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.
[참고]ApplicantCard
에 남긴 리뷰와 연관됩니다!
ApplicantCard를 DTO와 분리된 도메인 클래스로 취급하고 Comparator
interface를 구현하도록 만들면 이 클래스에서 Compartor
반환 책임을 분리할 수 있을 것 같습니다
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cutehumanS2 <oddpinkjadeite@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cutehumanS2 <oddpinkjadeite@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kwoun Ki Ho <fingercut3822@gmail.com> feat-be: 프로세스 목록 조회 API 필터링 및 정렬 구현 (#756) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cutehumanS2 <oddpinkjadeite@gmail.com> feat-be: 다중 불합격자 해제 기능 구현 (#759) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kwoun Ki Ho <fingercut3822@gmail.com> feat-be: 대시보드 삭제 api 구현 (#749) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: HyungHoKim00 <hkim1109@naver.com> fix-be: 예외 처리 과정에서 NPE가 발생하지 않도록 수정 (#753) feat-be: 다중 불합격자 기능 구현 (#751) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kwoun Ki Ho <fingercut3822@gmail.com> chore-be(Actions-Prod): 무중단 배포 설정 수정
82a5d2d (HEAD -> be/main, origin/be/main) fix-be: OSIV 오류 해결 (#746) 7434b61 feat-be: 프로세스 목록 조회 API 필터링 및 정렬 구현 (#756) 2e05f2b feat-be: 다중 불합격자 해제 기능 구현 (#759) 4c60d92 feat-be: 대시보드 삭제 api 구현 (#749) fbbfee5 fix-be: 예외 처리 과정에서 NPE가 발생하지 않도록 수정 (#753) a55783d feat-be: 다중 불합격자 기능 구현 (#751) 071cebc chore-be(Actions-Prod): 무중단 배포 설정 수정
목적
프로세스 목록 조회 API에 필터링 및 정렬을 구현합니다.
작업 세부사항
참고 사항
필터링 기준과 정렬 기준은 다음과 같습니다.
[필터링 기준]
EvaluationStatus.ALL
(필터링 적용 안 함)입니다.EvaluationStatus.NOT_EVALUATED
: 평가가 없는 상태(evaluationCount = 0)입니다.EvaluationStatus.EVALUATED
: 평가가 있는 상태(evaluationCount > 0)입니다.[정렬 기준]
PROC_READ_06
closes #755