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

[YS-141] feat: 실험자 본인이 작성한 공고 게시글 조회 API 구현 #34

Merged
merged 18 commits into from
Jan 17, 2025

Conversation

Ji-soo708
Copy link
Member

@Ji-soo708 Ji-soo708 commented Jan 15, 2025

💡 작업 내용

  • 실험자 본인이 작성한 공고 게시글 조회 API를 구현했습니다

DESC 정렬
image

ASC 정렬
스크린샷 2025-01-15 오후 4 10 49

잘못된 정렬 파라미터에 대한 예외
스크린샷 2025-01-15 오후 4 11 00

✅ 셀프 체크리스트

  • PR 제목을 형식에 맞게 작성했나요?
  • 브랜치 전략에 맞는 브랜치에 PR을 올리고 있나요?
  • 테스트는 잘 통과했나요?
  • 빌드에 성공했나요?
  • 본인을 assign 해주세요.
  • 해당 PR에 맞는 label을 붙여주세요.

🙋🏻‍ 확인해주세요

  • 수정님과 협의할 부분이 남아 테스트 코드는 작성하지 않았습니다. 협의 후에 다시 작성하겠습니다.

🔗 Jira 티켓


https://yappsocks.atlassian.net/browse/YS-141

@Ji-soo708 Ji-soo708 added ✨ FEATURE 기능 추가 🔒 HOLD 홀드 labels Jan 15, 2025
@Ji-soo708 Ji-soo708 self-assigned this Jan 15, 2025
Copy link
Member Author

@Ji-soo708 Ji-soo708 left a comment

Choose a reason for hiding this comment

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

isLast, page, size와 같은 응답 필드가 추가되면서 수정님과 이 부분을 어떻게 처리할지에 대해 협의를 하면 좋겠다고 생각이 들었습니다!

의문이 가는 점이나 더 좋은 방안이 생각나신다면 코멘트로 알려주세요~

Comment on lines 45 to 59
fun getMyExperimentPosts(input: GetMyExperimentPostUseCase.Input): List<GetMyExperimentPostUseCase.Output> {
validateSortOrder(input.pagination.order)
return getMyExperimentPostUseCase.execute(input)
}

private fun validateSortOrder(sortOrder: String): String {
return when (sortOrder) {
"ASC", "DESC" -> sortOrder
else -> throw InvalidParameterException("Invalid sort order. Please use 'ASC' or 'DESC'")
}
}

fun getMyExperimentPostsCount(input: GetTotalMyExperimentPostCountUseCase.Input): GetTotalMyExperimentPostCountUseCase.Output {
return getTotalMyExperimentPostCountUseCase.execute(GetTotalMyExperimentPostCountUseCase.Input(input.memberId))
}
Copy link
Member Author

@Ji-soo708 Ji-soo708 Jan 15, 2025

Choose a reason for hiding this comment

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

페이지네이션된 결과를 가져오는 유즈케이스isLast와 같은 필드 값을 계산하기 위해 자신이 작성한 게시글의 개수를 조회하는 유즈케이스를 둘로 나누어 진행한 이유는, 각 유즈케이스가 고유의 책임에 집중하도록 하기 위해서입니다.

페이지네이션된 결과 조회는 사용자에게 실험 공고 게시글 목록을 반환하는 데 집중하고, 게시글 개수 조회는 이후 isLast 필드 값을 계산하기 위해 전체 게시글 수만을 계산하는 역할을 하도록 했습니다.

Copy link
Member

Choose a reason for hiding this comment

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

네 저도 지수님 코드 보면서 해당 유즈케이스들이 SRP를 준수한 채 잘 짜여진 코드라고 생각했습니다!

  • 목록 반환 : GetMyExperimentPostUseCase
  • isLast 필드를 통한 전체 게시글 개수 계산: GetTotalMyExperimentPostCountUseCase

다만, '여러 개의 포스트들을 조회한다' 라는 유즈케이스의 기능을 고려하여,
유즈케이스 명을 ~~Post 보다는 ~~Posts 로 리네이밍하면 어떨지 조심스레 의견 내봅니다 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

네 저도 복수형으로 통일하면 더 일관성있을 거 같습니다. 👍

Comment on lines 45 to +46
data class PostInfoOutput(
val postId: Long,
val experimentPostId: Long,
Copy link
Member Author

Choose a reason for hiding this comment

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

지금 배포된 API에서 experimentPostId로 다 내보내고 있어서 통일성을 위해 수정했습니다~

Comment on lines 89 to 95
val pagination = MemberMapper.toUseCasePagination(page, count, order)
val input = MemberMapper.toGetMyExperimentPosts(pagination)
val posts = memberService.getMyExperimentPosts(input)

val totalCountInput = MemberMapper.toGetTotalMyExperimentPostCountUseCaseInput()
val totalCount = memberService.getMyExperimentPostsCount(totalCountInput).totalPostCount
return MemberMapper.toGetMyExperimentPostsResponse(posts, page, totalCount)
Copy link
Member Author

Choose a reason for hiding this comment

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

다른 유즈케이스처럼 input과 output만 사용하는 방식도 고려했으나, 고민 끝에 저희가 정한 아키텍처 원칙에 따르면 poststotalCount를 별도로 처리하여 넘겨주는 방식이 더 적합하다고 판단했습니다. 이렇게 작성한다고 해서 클린 아키텍처 원칙을 어기는 것도 아니라 생각하여, 이 방식으로 진행하게 되었습니다.

만약 더 나은 방법이 있으면 의견 부탁드립니다!

Comment on lines 3 to 8
data class PaginatedResponse<T>(
val content: List<T>,
val page: Int,
val size: Int,
val isLast: Boolean
)
Copy link
Member Author

Choose a reason for hiding this comment

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

이 응답 DTO를 앞으로의 페이지네이션 기능에서 쓰면 좋을 거 같습니다!

},
page = page,
size = output.size,
isLast = isLastPage(totalCount, output.size, page),
Copy link
Member Author

@Ji-soo708 Ji-soo708 Jan 15, 2025

Choose a reason for hiding this comment

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

isLast 여부를 판단하는 로직을 작성하는 위치가 애매했지만, 저희 로직 상으로는 Mapper 위치가 적합하다고 생각했습니다. 다만, Mapper는 DTO 변환만 담당하는 것이 좋다는 원칙에 따라, isLastPage 계산 로직을 별도의 util 클래스로 분리하여 처리했습니다.

이렇게 하면 클린 아키텍처 원칙을 지키면서 SRP를 함께 유지할 수 있을 것으로 판단하여 이 방식으로 진행했습니다. 이 방식에 대해 수정님의 의견도 듣고 싶습니다!

Copy link
Member

Choose a reason for hiding this comment

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

isLast 여부를 판단하는 로직이 Mapper 에 속하지 않아야한다는 점에 동의합니다!

지수님께서는 재사용성을 고려하시고 페이지네이션 결과를 보조적으로 평가하기 때문에, 부가로직이라는 판단 하에 Util 패키지에 위치하신 것 같아요.

다만, 현재 저희 프로젝트에서 커스텀 필터 로직에 따라 반환해야 하는 공고 데이터들이 달라지고, 해당 필터 로직의 복잡도가 1차 MVP 이후 단계에서 더 복잡해질 수도 있겠다는 생각이 들어요.

이에 따라, 마지막 페이지인지에 따라 추가 작업을 서버 단에서 트리거한다든가 하는 추가 요구사항 등이 늘어난다면, 더 이상 페이지네이션 결과를 보조적으로 평가하는 게 아닌 비즈니스의 핵심 로직이라고 생각합니다.

그래서 저는 해당 부분이 application 계층에 위치해야 한다고 생각해요!
지수님께서는 어떻게 생각하실까요? 🤔

Copy link
Member Author

@Ji-soo708 Ji-soo708 Jan 16, 2025

Choose a reason for hiding this comment

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

isLast 여부를 판단하는 로직은 특정 비즈니스 유즈케이스의 실행 흐름 그 자체라기보다는, 비즈니스 도메인에서 공통적으로 활용되는 부가적인 로직에 더 가깝다고 생각했습니다. 따라서 이를 유즈케이스 내부에 포함시키는 것은 오히려 유즈케이스의 목적과 역할을 모호하게 만들 수 있어서 별도의 Util로 빼서 계산하도록 구현했습니다.

물론, 수정님의 의견을 듣고 isLast를 판단하는 여부 로직은 application 계층 내에서 계산을 하는 것이 좋을 거 같다는 의견에 공감하고 좋은 방향이라 생각합니다. 그래서 제 생각에는 utils에 있던 isLastPage 함수를 application 계층의 PaginationService로 옮겨서 Controller 단에서 사용을 하면 좋지 않을까 생각이 듭니다. 아니면 기존 도메인 서비스에서 isLastPage 함수를 생성해서 쓰는 방법도 있는데 Pagination 기능은 여러 도메인에서 사용될 수 있기에 별도의 서비스로 빼는 것이 더 좋지 않을까 생각이 듭니다.

이 방식을 채택하면 이후 페이지네이션 관련 로직이 확장될 경우에도 유연하게 대응할 수 있다고 생각하는데 어떻게 생각하시는 지 궁금해요!

Copy link
Member

Choose a reason for hiding this comment

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

application 내에서의 비즈니스 중 부가 로직이라는 점에 적극 공감합니다! :D
PaginationService 로 옮겨서 별도로 모듈화를 시키면 비즈니스 로직에 좀 더 유연한 확장성을 확보할 수 있다는 방향인 것 같아요!

@Ji-soo708 Ji-soo708 requested a review from chock-cho January 15, 2025 07:30
@github-actions github-actions bot changed the title feat: 실험자 본인이 작성한 공고 게시글 조회 API 구현 [YS-141] feat: 실험자 본인이 작성한 공고 게시글 조회 API 구현 Jan 15, 2025
Copy link
Member

@chock-cho chock-cho left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!
여러 가지 고민하시고 구현하신 게 느껴져서 좋았어요.
관련해서 논의할 내용 남겨두었으니 확인 부탁드려요.

1. 공고 전체 조회 / 연구자 회원이 등록한 내 공고 조회 응답 방식 통일

  • 두 개의 API 응답 방식을 굳이 통일하지 않아도 되지 않나 제안합니다.
    디자인 시스템 보면 아시겠지만... 둘이 반환 시 필요한 필드값 종류 자체가 다릅니다!
    그래서 동일한 응답 dto를 강제하는 것이 오히려 유지보수성 관점에서는 떨어질 수도 있지 않나 싶네요 🤔

    1. 저는 공고 전체 조회에서 한 블럭에 필요한 정보값들을 PostInfo 클래스로 묶었어요. 공고 전체 조회에서는 애초에 요청 필터링 기능 자체에 recruitDone의 여부를 체크할 수 있어서, 해당 값에 맞는 공고들만 조회하므로 반환값에서의 필수필드가 아니에요.
    1. 연구자 회원이 자신이 올린 공고를 조회하는 것에 있어서는, '공고 마감'을 처리할 수 있는 recruitDone이 필수 필드예요.

2. isLast 관련 페이지 계산하는 로직의 패키지 위치

  • 지수님께서는 Util 패키지로 빼셨지만, 저는 application 계층에 위치해야 한다는 입장이에요. 관련해서 이유는 코멘트 남겨드렸으니 참고 부탁드려요!

Comment on lines 45 to 59
fun getMyExperimentPosts(input: GetMyExperimentPostUseCase.Input): List<GetMyExperimentPostUseCase.Output> {
validateSortOrder(input.pagination.order)
return getMyExperimentPostUseCase.execute(input)
}

private fun validateSortOrder(sortOrder: String): String {
return when (sortOrder) {
"ASC", "DESC" -> sortOrder
else -> throw InvalidParameterException("Invalid sort order. Please use 'ASC' or 'DESC'")
}
}

fun getMyExperimentPostsCount(input: GetTotalMyExperimentPostCountUseCase.Input): GetTotalMyExperimentPostCountUseCase.Output {
return getTotalMyExperimentPostCountUseCase.execute(GetTotalMyExperimentPostCountUseCase.Input(input.memberId))
}
Copy link
Member

Choose a reason for hiding this comment

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

네 저도 지수님 코드 보면서 해당 유즈케이스들이 SRP를 준수한 채 잘 짜여진 코드라고 생각했습니다!

  • 목록 반환 : GetMyExperimentPostUseCase
  • isLast 필드를 통한 전체 게시글 개수 계산: GetTotalMyExperimentPostCountUseCase

다만, '여러 개의 포스트들을 조회한다' 라는 유즈케이스의 기능을 고려하여,
유즈케이스 명을 ~~Post 보다는 ~~Posts 로 리네이밍하면 어떨지 조심스레 의견 내봅니다 ㅎㅎ

},
page = page,
size = output.size,
isLast = isLastPage(totalCount, output.size, page),
Copy link
Member

Choose a reason for hiding this comment

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

isLast 여부를 판단하는 로직이 Mapper 에 속하지 않아야한다는 점에 동의합니다!

지수님께서는 재사용성을 고려하시고 페이지네이션 결과를 보조적으로 평가하기 때문에, 부가로직이라는 판단 하에 Util 패키지에 위치하신 것 같아요.

다만, 현재 저희 프로젝트에서 커스텀 필터 로직에 따라 반환해야 하는 공고 데이터들이 달라지고, 해당 필터 로직의 복잡도가 1차 MVP 이후 단계에서 더 복잡해질 수도 있겠다는 생각이 들어요.

이에 따라, 마지막 페이지인지에 따라 추가 작업을 서버 단에서 트리거한다든가 하는 추가 요구사항 등이 늘어난다면, 더 이상 페이지네이션 결과를 보조적으로 평가하는 게 아닌 비즈니스의 핵심 로직이라고 생각합니다.

그래서 저는 해당 부분이 application 계층에 위치해야 한다고 생각해요!
지수님께서는 어떻게 생각하실까요? 🤔

@Ji-soo708
Copy link
Member Author

Ji-soo708 commented Jan 16, 2025

공고 전체 조회 / 연구자 회원이 등록한 내 공고 조회 응답 방식 통일

공고 전체 조회 / 연구자 회원이 등록한 내 공고 조회 응답 방식 통일에 대해 통일하지 않았으면 좋겠다고 말씀하셨는데 혹시 제가 말씀드렸던 페이지네이션 응답 형식을 통일하자는 부분을 오해하신 걸까요? 아니면, 페이지네이션 응답 형식을 통일하는 것이 유지보수에 부정적이라는 의미로 말씀하신 걸까요? 🤔

제 의견은 페이지네이션 기능이 있는 API의 경우, 원래 응답해야 할 data를 담은 Response를 isLast, page, size 같은 정보를 포함한 PaginatedResponse<T>로 감싸는 방식이 좋을 것 같다는 것이었습니다. 이렇게 하면 서버에서 응답 형식을 통일할 수 있어 프론트엔드에서도 처리하기 편리하고, 저희 서버 측에서도 유지보수 측면에서 더 효율적이라고 판단했습니다!

혹시 이 방식에 대해서도 공감이 가지 않으시다면, 이 부분은 함께 논의해보면 좋을 것 같습니다. 다만, 지금까지의 프로젝트에서는 페이지네이션 기능을 포함한 API의 응답 형식을 통일해왔고, 그렇게 해야 프론트엔드에서 작업하기 더 편하다는 피드백을 받아왔기 때문에 저는 이 방향으로 진행하는 것이 적절하다고 생각했습니다!

@chock-cho
Copy link
Member

chock-cho commented Jan 16, 2025

공고 전체 조회 / 연구자 회원이 등록한 내 공고 조회 응답 방식 통일

아! 제가 전달 드리고자 하는 바에서 약간 오해가 있었던 것 같네요 😅
제가 본래 전하고 싶었던 것은 PaginatedResponse<T> 인터페이스로 페이지네이션 관련 응답의 틀을 통일하는 것에 대해서는 유지 보수성, 프론트와의 소통의 용이성 측면에서 적극 동의하는데,
각자 반환하고자 하는 ExperimentPost 내부의 정보값들이 달라서 해당 정보들은 통일하지 않아도 되겠다고 말씀드린 거였어요!

지수님께서 이해하신대로 진행하셔도 좋습니다! 해당 방식에 대해서 저도 동의합니다! 😊😊

@Ji-soo708 Ji-soo708 removed the 🔒 HOLD 홀드 label Jan 16, 2025
@Ji-soo708 Ji-soo708 requested a review from chock-cho January 17, 2025 00:33
chock-cho
chock-cho previously approved these changes Jan 17, 2025
Copy link
Member

@chock-cho chock-cho left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다!
리뷰로 의견 나눈 부분이 잘 반영되어서 approve합니다.
conflict 난 부분만 확인하셔서 머지해주시면 감사하겠습니다 :D 💪

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
74.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@Ji-soo708 Ji-soo708 merged commit 006781f into dev Jan 17, 2025
2 of 3 checks passed
@Ji-soo708 Ji-soo708 deleted the feat/YS-141 branch January 17, 2025 10:26
Ji-soo708 added a commit that referenced this pull request Jan 26, 2025
* feat: add Repository's method for GetMyExperimentPostUseCase

* feat: add MemberMapper for GetMyExperimentPostUseCase

* feat: add ParginatedResponse for pagination

* refact: rename field or file name for consistency

* feat: add MyExperimentPostResponse for get method

* feat: add GetMyExperimentPosts's logic

* refact: extract pagination logic into utility function in response mapper

* feat: add sorting logic

* refact: rename usecase name

* refact: add PaginationService

* test: add GetMyExperimentPostsUseCase Test

* refact: rename usecase name

* test: add GetMyExperimentPostTotalCountUseCase Test code

* style: delete unused import

* refact: delete unused utils class and rename controller tag name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ FEATURE 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants