-
Notifications
You must be signed in to change notification settings - Fork 1
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/#430] 밀린 문제 ID 조회 API 추가 #433
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.
작업내용 코맨트드립니다
/** | ||
* 유저가 구독한 워크북들에 속한 아티클 개수를 조회함 | ||
* 이때 아티클 개수는 현 시점 기준으로 이메일이 전송된 아티클 개수까지만 조회함 | ||
*/ | ||
val subscriptionProgresses = subscriptionDao.selectWorkbookIdAndProgressByMember( | ||
SelectSubscriptionSendStatusQuery(useCaseIn.memberId) | ||
).takeIf { it.isNotEmpty() } ?: throw NotFoundException("subscribe.workbook.notexist") | ||
|
||
/** | ||
* 위에서 조회한 워크부에 속한 아티클 개수에 대해 article_id 들을 조회함 | ||
*/ | ||
val sentArticleIds = subscriptionProgresses.flatMap { subscriptionProgress -> | ||
articleDao.selectArticleIdByWorkbookIdLimitDay( | ||
SelectAritlceIdByWorkbookIdAndDayQuery( | ||
subscriptionProgress.workbookId, | ||
subscriptionProgress.numOfReadArticle | ||
) | ||
).articleIds | ||
}.toSet() | ||
|
||
/** | ||
* 위에서 구한 아티클에 속한 모든 problem_id를 조회함 | ||
*/ | ||
val allProblemIdsToBeSolved = problemDao.selectProblemIdByArticleIds( | ||
SelectProblemIdByArticleIdsQuery(sentArticleIds) | ||
).problemIds | ||
|
||
/** | ||
* 위에서 구한 문제들에 대해 풀이 이력이 존재하는 problem_id만 추출 후 | ||
* 유저가 풀어야 할 전체 problem_id에 대해 여집합 연산 | ||
*/ | ||
val submittedProblemIds = submitHistoryDao.selectProblemIdByProblemIds( | ||
SelectSubmittedProblemIdsQuery(useCaseIn.memberId, allProblemIdsToBeSolved) | ||
).problemIds | ||
|
||
val unsubmittedProblemIds = allProblemIdsToBeSolved.filter { it !in submittedProblemIds } | ||
|
||
return BrowseProblemsUseCaseOut(unsubmittedProblemIds) | ||
} |
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.
로직은 간단해서 이것만 보시면 될거 같아요 주석만 보시면 이해 바로 하실거같습니다
given("밀린 문제 ID 조회 요청이 온 상황에서") { | ||
val memberId = 0L | ||
val useCaseIn = BrowseUndoneProblemsUseCaseIn(memberId = memberId) | ||
|
||
`when`("밀린 문제가 존재할 경우") { | ||
every { subscriptionDao.selectWorkbookIdAndProgressByMember(any()) } returns listOf( | ||
SubscriptionProgress(1L, 3), | ||
SubscriptionProgress(2L, 3) | ||
) | ||
|
||
every { articleDao.selectArticleIdByWorkbookIdLimitDay(any()) } returns ArticleIdRecord(listOf(1L, 2L)) | ||
|
||
every { problemDao.selectProblemIdByArticleIds(any()) } returns ProblemIdsRecord(listOf(1L, 2L)) | ||
|
||
every { submitHistoryDao.selectProblemIdByProblemIds(any()) } returns SubmittedProblemIdsRecord( | ||
listOf( | ||
1L, | ||
2L | ||
) | ||
) | ||
|
||
then("밀린 문제 ID 목록을 반환한다") { | ||
useCase.execute(useCaseIn) | ||
|
||
verify(exactly = 1) { subscriptionDao.selectWorkbookIdAndProgressByMember(any()) } | ||
verify(exactly = 1) { articleDao.selectArticleIdByWorkbookIdLimitDay(any()) } | ||
verify(exactly = 1) { problemDao.selectProblemIdByArticleIds(any()) } | ||
verify(exactly = 1) { submitHistoryDao.selectProblemIdByProblemIds(any()) } | ||
} | ||
} |
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.
고생하셨어요! 코멘트 남겼습니다.!!
private val subscriptionDao: SubscriptionDao, | ||
private val articleDao: ArticleDao, | ||
private val submitHistoryDao: SubmitHistoryDao, |
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.
추가된 쿼리도 익스플레인 볼 수 있게 테스트 작성해주세요.!
fun selectWorkbookIdAndProgressByMemberQuery(query: SelectSubscriptionSendStatusQuery) = | ||
dslContext.select( | ||
SUBSCRIPTION.TARGET_WORKBOOK_ID.`as`(SubscriptionProgress::workbookId.name), | ||
SUBSCRIPTION.PROGRESS.add(1).`as`(SubscriptionProgress::numOfReadArticle.name) | ||
) | ||
.from(SUBSCRIPTION) | ||
.where(SUBSCRIPTION.MEMBER_ID.eq(query.memberId)) | ||
.and(SUBSCRIPTION.DELETED_AT.isNull) |
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.
구독이 끝이난 경우에는 밀린문제가 어떻게 처리할지 PM이랑 이야기 해봐야 할 것 같아요
* 위에서 조회한 워크부에 속한 아티클 개수에 대해 article_id 들을 조회함 | ||
*/ | ||
val sentArticleIds = subscriptionProgresses.flatMap { subscriptionProgress -> | ||
articleDao.selectArticleIdByWorkbookIdLimitDay( |
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.
여기 workbookId와 day로 해당하는 워크북에 속한 아티클 아이디들을 조회하는 것이니까
selectArticleIds
ByWorkbookIdLimitDay로 하는 것은 어때요?
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.
좋습니다~
articleDao.selectArticleIdByWorkbookIdLimitDay( | ||
SelectAritlceIdByWorkbookIdAndDayQuery( | ||
subscriptionProgress.workbookId, | ||
subscriptionProgress.numOfReadArticle |
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.
여기 변수명 day로 통일하는 것 어떤가요?
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.
오 좋네요
@@ -0,0 +1,64 @@ | |||
package com.few.api.domain.problem.usecase |
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.
BrowseUndoneProblemsUseCase
는 problem이 맞을까요? 아님 member로 가는게 맞을까요?
제출하지 않은 문제 기록이니까 뭔가 member에 있는것도 고려할 수 있을 것 같아서요
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가 사용되는 그 resource 차원에서 볼때, problem에 더 가까울거 같습니다. 멤버를 기준으로 조회할 뿐 그 조회하려는 resource는 문제이기 때문
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가 ~/problems/unsubmitte
로 되어 있는데
만약 해당 구현이 member로 간다면 ~/members/problems
로 될 것 같아요
RESTful API 네이밍을 할때 명사를 사용하라는 추천이 있잖아요.
그 말을 고려하면 member로 괜찮지 않을까 생각해서 코멘트 남겼어요.
근데 problem에 있어도 괜찮을 것 같습니다!
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 처럼 맨 뒤에만 위치하도록 해야 함
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.
아 잠만 근데 저것보다 쿼리 파람으로 두는게 잴 적절하긴 하겠네요…
|
||
val unsubmittedProblemIds = allProblemIdsToBeSolved.filter { it !in submittedProblemIds } | ||
|
||
return BrowseProblemsUseCaseOut(unsubmittedProblemIds) |
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.
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.
이 부분은 확인못했네요... 0fba25e 커밋에서 반영했습니다
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.
변경내용 리뷰 부탁드립니다
/** | ||
* 위에서 구한 아티클에 속한 모든 problem_id, article_id 조합을 조회함 | ||
*/ | ||
val allProblemIdsAndArticleIdsToBeSolved = problemDao.selectProblemIdByArticleIds( | ||
SelectProblemIdByArticleIdsQuery(sentArticleIds) | ||
) | ||
|
||
/** | ||
* 위에서 구한 문제들에 대해 풀이 이력이 존재하는 problem_id만 추출 후 | ||
* 유저가 풀어야 할 전체 problem_id에 대해 여집합 연산 | ||
*/ | ||
val allProblemIdsToBeSolved = allProblemIdsAndArticleIdsToBeSolved.map { it.problemId } | ||
val submittedProblemIds = submitHistoryDao.selectProblemIdByProblemIds( | ||
SelectSubmittedProblemIdsQuery(useCaseIn.memberId, allProblemIdsToBeSolved) | ||
).problemIds | ||
|
||
val unsubmittedProblemIdAndArticleIds: Map<Long, List<Long>> = allProblemIdsAndArticleIdsToBeSolved | ||
.filter { it.problemId !in submittedProblemIds } | ||
.groupBy { it.articleId } | ||
.mapValues { entry -> entry.value.map { it.problemId } } | ||
|
||
/** | ||
* 결과를 article_id를 기준으로 랜덤화한 뒤 problem_id를 순차적으로 리턴함 | ||
*/ | ||
val randomArticleIds = unsubmittedProblemIdAndArticleIds.keys.shuffled() | ||
val problemIdsRandomizedByArticleId = mutableListOf<Long>() | ||
|
||
randomArticleIds.forEach { articleId -> | ||
unsubmittedProblemIdAndArticleIds[articleId]?.let { problemIds -> | ||
problemIdsRandomizedByArticleId.addAll(problemIds) | ||
} | ||
} | ||
|
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.
articleId를 기준으로 랜덤화한 뒤 problemId를 뽑아냅니다
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.
네 이해했슴다~
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.
확인했고 위의 코멘트에 추가한 댓글에 대한 의견만 공유하고 머지하면 될 것 같아요!
@@ -0,0 +1,64 @@ | |||
package com.few.api.domain.problem.usecase |
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가 ~/problems/unsubmitte
로 되어 있는데
만약 해당 구현이 member로 간다면 ~/members/problems
로 될 것 같아요
RESTful API 네이밍을 할때 명사를 사용하라는 추천이 있잖아요.
그 말을 고려하면 member로 괜찮지 않을까 생각해서 코멘트 남겼어요.
근데 problem에 있어도 괜찮을 것 같습니다!
/** | ||
* 결과를 article_id를 기준으로 랜덤화한 뒤 problem_id를 순차적으로 리턴함 | ||
*/ | ||
val randomArticleIds = unsubmittedProblemIdAndArticleIds.keys.shuffled() |
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.
지피티가 알려줌ㅋㅋ 내부적으론 어떤 성능 이슈나 그런게 있을지는 모르겠는데 파악해볼게요
/** | ||
* 위에서 구한 아티클에 속한 모든 problem_id, article_id 조합을 조회함 | ||
*/ | ||
val allProblemIdsAndArticleIdsToBeSolved = problemDao.selectProblemIdByArticleIds( | ||
SelectProblemIdByArticleIdsQuery(sentArticleIds) | ||
) | ||
|
||
/** | ||
* 위에서 구한 문제들에 대해 풀이 이력이 존재하는 problem_id만 추출 후 | ||
* 유저가 풀어야 할 전체 problem_id에 대해 여집합 연산 | ||
*/ | ||
val allProblemIdsToBeSolved = allProblemIdsAndArticleIdsToBeSolved.map { it.problemId } | ||
val submittedProblemIds = submitHistoryDao.selectProblemIdByProblemIds( | ||
SelectSubmittedProblemIdsQuery(useCaseIn.memberId, allProblemIdsToBeSolved) | ||
).problemIds | ||
|
||
val unsubmittedProblemIdAndArticleIds: Map<Long, List<Long>> = allProblemIdsAndArticleIdsToBeSolved | ||
.filter { it.problemId !in submittedProblemIds } | ||
.groupBy { it.articleId } | ||
.mapValues { entry -> entry.value.map { it.problemId } } | ||
|
||
/** | ||
* 결과를 article_id를 기준으로 랜덤화한 뒤 problem_id를 순차적으로 리턴함 | ||
*/ | ||
val randomArticleIds = unsubmittedProblemIdAndArticleIds.keys.shuffled() | ||
val problemIdsRandomizedByArticleId = mutableListOf<Long>() | ||
|
||
randomArticleIds.forEach { articleId -> | ||
unsubmittedProblemIdAndArticleIds[articleId]?.let { problemIds -> | ||
problemIdsRandomizedByArticleId.addAll(problemIds) | ||
} | ||
} | ||
|
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.
네 이해했슴다~
🎫 연관 이슈
resolved #430
💁♂️ PR 내용
[GET] /api/v1/problems/unsubmitted
)articleId
필드 응답 추가🙏 작업
아래는 밀린 문제 조회시 제가 생각한 플로우 (논의 필요)
[GET] /api/v1/problems/unsubmitted
[GET] /api/v1/problems/{problemId}
[POST] /api/v1/problems/{problemId}
🙈 PR 참고 사항
📸 스크린샷
밀린 문제 ID 조회 API 추가
구독중인 워크북이 없을 경우
정상 케이스
* 밀린 문제 없으면 Empty List, size = 0 응답됨문제 단건 조회시(기존API) articleId 필드 응답 추가
🤖 테스트 체크리스트