Skip to content
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

Feature/#324 행사 검색 기능 구현 #686

Merged
merged 10 commits into from
Oct 9, 2023

Conversation

amaran-th
Copy link
Collaborator

#️⃣연관된 이슈

#324

📝작업 내용

행사 이름을 키워드로 검색하는 기능을 구현하였습니다.

예상 소요 시간 및 실제 소요 시간

예상 - 10/5
실제 - 10/5

@amaran-th amaran-th added Backend 백엔드 관련 이슈 기능 추가 새로운 기능 추가 및 기존 기능 변경 High Priority 리뷰 우선순위가 높은 PR labels Oct 5, 2023
@amaran-th amaran-th added this to the 6차 스프린트 milestone Oct 5, 2023
@amaran-th amaran-th self-assigned this Oct 5, 2023
Copy link
Collaborator

@hyeonjerry hyeonjerry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다!
궁금한 점이 있어서 코멘트 남겼어요.
확인 부탁드립니다 :)

@@ -43,9 +43,11 @@ public ResponseEntity<List<EventResponse>> findEvents(
@RequestParam(name = "start_date", required = false) final String startDate,
@RequestParam(name = "end_date", required = false) final String endDate,
@RequestParam(required = false) final List<String> tags,
@RequestParam(required = false) final List<EventStatus> statuses) {
@RequestParam(required = false) final List<EventStatus> statuses,
@RequestParam(required = false) final String keyword) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

나중에 필터링 관련 파라미터끼리 하나의 DTO로 묶어도 좋을 것 같네요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀을 듣고 고민해봤는데 DTO 객체로 매핑시켰을 때 required=false 설정을 어떻게 적용시킬 수 있을지 방법이 떠오르지 않아서, 추후 EventService 리팩토일 작업을 할 때 고민해봐야할 것 같아요.
좋은 의견 감사합니다~!

}

private boolean isExistKeyword(final String keyword) {
return keyword != null && !keyword.trim().isEmpty();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyword.isBlank()로 표현할 수 있지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영했습니다!

@ParameterizedTest
@ValueSource(strings = {"", " ", "컨퍼", "인프콘"})
@DisplayName("2023년 7월 21일에 컨퍼런스 행사를 조회할 때, 목록이 비어있을 때 어떤 키워드가 들어오든 빈 행사 목록을 반환한다.")
void findEvents_empty_events_search(final String keyword) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 이 테스트는 어떤 의도로 작성하신건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예전에 행사 목록 조회 API를 호출할 때 DB에 Event가 비어있을 경우 예상치 못하게 버그가 발생하는 문제가 있었습니다.
그래서 그 때 event 테이블이 비어있는 경우에 대해서도 테스트를 작성했었고 이번에 새로운 로직을 추가하면서 같은 종류의 테스트를 작성해준 것입니다!
하지만 다시 생각해보니 지금은 필터링 로직들이 모두 동적 쿼리를 사용하는 것으로 수정되어서, 필터링 요소별로 행사 목록이 비어있는 케이스를 검증하지 않아도 될 것 같네요.

빈 목록을 반환하는 테스트 케이스 하나를 제외하고는 모두 불필요한 것 같아 삭제해주었습니다!

Copy link
Collaborator

@java-saeng java-saeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼한 테스트 잘 봤습니다 !

딱히 수정할 부분이 없어보입니다 고생하셨습니다

Copy link
Collaborator

@hong-sile hong-sile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전반적으로 Specifiaction을 사용하는 방법이 유사해서, 읽기가 어렵진 않았네요.
잘 봤습니다.

의견 하나 남겨뒀는데 확인 부탁드려요. 어떻게 뺄지도 한번 고민해보면 좋을 것 같네요.
크게 수정할 부분은 없어서 approve 하겠습니다.

@@ -73,27 +74,30 @@ public EventDetailResponse findEvent(final Long id, final LocalDate today) {
@Transactional(readOnly = true)
public List<EventResponse> findEvents(final EventType category,
final LocalDate nowDate, final String startDate, final String endDate,
final List<String> tagNames, final List<EventStatus> statuses) {
final List<String> tagNames, final List<EventStatus> statuses, final String keyword) {
Specification<Event> spec = Specification.where(filterByCategory(category));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specification을 잘 활용해주신 것 같아요.

specification을 활용하는 코드가 많은데 별도의 클래스로 분리해보는 건 어떨까요?
(이건 단순 의견입니다. 반영 안 해주셔도 괜찮을 것 같아요)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추후 EventService를 리팩토링할 때 고민해보면 좋을 것 같네요!
querydsl로 변경해주어야하기도 하구요

Copy link
Collaborator

@hyeonjerry hyeonjerry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 반영해 주셨네요!!
수고하셨습니다~!

@amaran-th amaran-th merged commit ce3e2a3 into backend-main Oct 9, 2023
1 check passed
@chws0508 chws0508 deleted the Feature/#324-행사_검색_기능_구현 branch October 11, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend 백엔드 관련 이슈 High Priority 리뷰 우선순위가 높은 PR 기능 추가 새로운 기능 추가 및 기존 기능 변경
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants