Conversation
|
Important Review skippedToo many files! This PR contains 171 files, which is 21 over the limit of 150. 📒 Files selected for processing (171)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
soo0711
reviewed
Feb 22, 2026
Comment on lines
+45
to
+46
| var penaltyCount: Int = 0, | ||
| var warningCount: Int = 0, // todo: 경고시 자동 페널티 기능도 제거 |
Collaborator
There was a problem hiding this comment.
자동페널티 기능은 Leets가 적용하던 기능이기 때문에 제거하는건가용?
Contributor
Author
There was a problem hiding this comment.
넵넵 맞습니당 해당 기능은 리츠의 요구사항이었으니 우선은 제거했서용
Comment on lines
+6
to
+7
| fun getByCardinalNumber(cardinalNumber: Int): Cardinal | ||
|
|
Collaborator
There was a problem hiding this comment.
port 인터페이스에서는 JPA 네이밍과의 일관성을 위해 get보다 find를 사용하는 것이 어떨까요?!!
Contributor
Author
There was a problem hiding this comment.
타 도메인들에서는 대부분 Reader에서 list를 받고 있어서 이런 차이가 없었는데, 유저 도메인의 경우는 개별 조회가 들어가다 보니 get: 없으면 예외, find: 그냥 반환 이렇게 구분을 해주는건 어떨까요??
Comment on lines
13
to
18
| fun getCurrentCardinal(user: User): Cardinal = | ||
| userCardinalReader | ||
| .findAllByUser(user) | ||
| .maxByOrNull { it.cardinal.cardinalNumber } | ||
| ?.cardinal | ||
| ?: throw CardinalNotFoundException() |
Collaborator
There was a problem hiding this comment.
현재 구현은 전체 조회 후 max를 계산하고 있는데 최신 기수 조회라는 의도를 고려하면 DB에서 정렬 후 1건만 조회하는 방식으로도 구현 가능할 것 같습니다! 해당 방식은 어떨까요?
Contributor
Author
There was a problem hiding this comment.
좋습니당! 쿼리 복잡도랑 성능 한 번 확인해서 리팩토링 해볼게욥
Contributor
Author
There was a problem hiding this comment.
쿼리가 그렇게 복잡하게 나오지 않고, 단건 조회부터 대량 반복 조회까지 모두 DB 정렬이 빨라서 그렇게 리팩토링 했습니다!
Comment on lines
126
to
132
| @DeleteMapping | ||
| @Operation(summary = "동아리 탈퇴") | ||
| fun leave( | ||
| @Parameter(hidden = true) @CurrentUser userId: Long, | ||
| ): CommonResponse<Void> { | ||
| adminUserUseCase.leave(userId) | ||
| return CommonResponse.success(UserResponseCode.USER_LEAVE_SUCCESS) |
Collaborator
There was a problem hiding this comment.
탈퇴는 사용자 본인의 행위로 보이는데 AdminUserUseCase에서 처리하기보다 User 쪽 UseCase로 이동하는 것이 더 자연스럽지 않을까염?
Contributor
Author
There was a problem hiding this comment.
우선 AuthUserUseCase로 이전하고, 리팩토링 할 때 유스케이스 하나 더 분리해서 옮겨둘게요!
Comment on lines
+31
to
+32
| @Service | ||
| class AuthUserUseCase( |
Collaborator
There was a problem hiding this comment.
현재 UseCase가 인증, 회원 생성, 프로필 수정 등 여러 책임을 함께 가지고 있어서 추후 리팩토링 시점에 역할을 분리해도 좋을 것 같습니다!!
Comment on lines
93
to
100
| it("회원 추방 시 상태를 BANNED로 변경한다") { | ||
| val user = UserTestFixture.createActiveUser1(1L) | ||
| every { userReader.findAllByIds(listOf(1L)) } returns listOf(user) | ||
|
|
||
| useCase.ban(UserIdsRequest(listOf(1L))) | ||
|
|
||
| user.isInactive() shouldBe true | ||
| } |
Comment on lines
119
to
120
| verify(exactly = 1) { attendanceSaveService.init(user, meetings) } | ||
| verify(exactly = 1) { userCardinalRepository.save(any()) } |
Collaborator
There was a problem hiding this comment.
현재는 save(any())로 호출 여부만 확인하고 있어서 기대한 user와 cardinal로 저장됐는지도 검증하면 테스트가 더 명확해질 것 같습니당
Comment on lines
162
to
164
| verify(exactly = 1) { cardinalRepository.save(any()) } | ||
| verify(exactly = 1) { attendanceSaveService.init(user, meetings) } | ||
| verify(exactly = 1) { userCardinalRepository.save(any()) } |
Collaborator
There was a problem hiding this comment.
여기도! 기대한 기수로 저장되었는지까지 검증하면 좋을 것 같아요 !!
Comment on lines
12
to
19
| user.accept() | ||
| user.isInactive() shouldBe false | ||
|
|
||
| user.ban() | ||
| user.isInactive() shouldBe true | ||
|
|
||
| user.leave() | ||
| user.isInactive() shouldBe true |
Collaborator
There was a problem hiding this comment.
여기도 true말고 정확한 상태를 검증하는 방향으로 바꾸는 게 어떨까요?!
Comment on lines
22
to
30
| "attendance 카운터 및 출석률 계산" { | ||
| val user = User(name = "test", email = "test@test.com", studentId = "20200001") | ||
| user.attend() | ||
| user.attend() | ||
| user.absent() | ||
|
|
||
| user.attendanceCount shouldBe 2 | ||
| user.absenceCount shouldBe 1 | ||
| user.attendanceRate shouldBe 66 |
Collaborator
There was a problem hiding this comment.
66이라는 값이 조금 뜬금없이 느껴질 수 있을 것 같아서 계산식을 명시해 검증하면 더 좋을 것 같습니당
Collaborator
|
아! 스케줄쪽 수정은 UseCaseImpl로 마이그레이션하면서 다 삭제해서 큰 충돌은 아닐 것 같습니당 |
soo0711
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Summary
유저 도메인 마이그레이션을 진행했습니다.
📝 Changes
What
Why
How
📸 Screenshots / Logs
💡 Reviewer 참고사항
✅ Checklist