Conversation
- RecommendationService (9 deps): CRUD + 조회 - ScheduledRecommendationService: 배치 오케스트레이션 - RecommendationEmailService: 이메일 발송 - RecommendationCreator: 공통 생성 로직 (package-private) - MissionCyclePolicy: 미션 사이클 정책 (package-private) 레거시 정리: TeamRecommendation, RecommendationStatus 등 5개 삭제 스케줄러 이동: infrastructure/scheduler/ → recommendation/scheduler/
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
컨트롤러 src/main/java/com/ryu/studyhelper/recommendation/RecommendationController.java |
수동 추천 엔드포인트의 count 파라미터 제거, 반환 DTO를 TeamRecommendationDetailResponse → RecommendationDetailResponse로 변경 및 관련 import/주석·로그 조정. |
핵심 서비스 재배치 src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationService.java, .../ScheduledRecommendationService.java, .../RecommendationEmailService.java, .../RecommendationCreator.java, .../MissionCyclePolicy.java |
RecommendationService를 오케스트레이터로 재구성; ScheduledRecommendationService, RecommendationEmailService, RecommendationCreator, MissionCyclePolicy 신규 추가 및 책임 분리. |
레거시 엔티티/리포지토리 삭제 src/main/java/com/ryu/studyhelper/recommendation/domain/team/TeamRecommendation.java, .../TeamRecommendationProblem.java, .../RecommendationStatus.java, src/main/java/com/ryu/studyhelper/recommendation/repository/TeamRecommendationRepository.java, .../TeamRecommendationProblemRepository.java |
레거시 TeamRecommendation 관련 엔티티, enum 및 관련 리포지토리 전부 제거. |
DTO 변경 src/main/java/com/ryu/studyhelper/recommendation/dto/response/RecommendationDetailResponse.java, .../TeamRecommendationDetailResponse.java |
새 RecommendationDetailResponse 레코드 추가 및 기존 TeamRecommendationDetailResponse 제거. |
스케줄러·패키지 이동 src/main/java/com/ryu/studyhelper/recommendation/scheduler/ProblemRecommendationScheduler.java, .../EmailSendScheduler.java, .../mailbuilder/RecommendationMailBuilder.java |
스케줄러 패키지로 이동, EmailSendScheduler/ProblemRecommendationScheduler가 각각 RecommendationEmailService/ScheduledRecommendationService 호출로 변경; RecommendationMailBuilder 패키지명 변경. |
도메인 정리 src/main/java/com/ryu/studyhelper/team/domain/Team.java |
teamRecommendations 연관 필드 및 import 제거. |
테스트 변경/추가 src/test/java/.../RecommendationServiceTest.java, .../ScheduledRecommendationServiceTest.java, .../RecommendationCreatorTest.java, .../RecommendationEmailServiceTest.java |
RecommendationService 테스트 재작업(creator/email 서비스 중심), ScheduledRecommendationService·RecommendationCreator·RecommendationEmailService용 신규 테스트 추가 및 패키지/시나리오 조정. |
Sequence Diagram(s)
sequenceDiagram
rect rgba(200,230,255,0.5)
actor Client
participant Controller as RecommendationController
participant Service as RecommendationService
participant Creator as RecommendationCreator
participant EmailService as RecommendationEmailService
participant DB as Repositories
end
Client->>Controller: POST /teams/{id}/recommendations (manual)
Controller->>Service: createManualRecommendation(teamId)
Service->>Service: validateNoRecommendationToday(teamId)
Service->>Creator: create(team, MANUAL)
Creator->>DB: save Recommendation, RecommendationProblem, MemberRecommendation
Creator-->>Service: created Recommendation + problems + memberRecommendations
Service->>EmailService: send(list<MemberRecommendation>)
EmailService->>DB: update MemberRecommendation status (SENT/FAILED)
EmailService-->>Service: send 결과 집계
Service-->>Controller: RecommendationDetailResponse
Controller-->>Client: 200 OK
sequenceDiagram
rect rgba(200,230,255,0.5)
actor Scheduler
participant ProblemScheduler as ProblemRecommendationScheduler
participant ScheduledService as ScheduledRecommendationService
participant Creator as RecommendationCreator
participant DB as Repositories
end
Scheduler->>ProblemScheduler: scheduled trigger (06:00)
ProblemScheduler->>ScheduledService: prepareDailyRecommendations()
ScheduledService->>DB: getActiveTeams(date)
loop per active team
ScheduledService->>DB: exists recommendation in mission cycle?
alt not exists
ScheduledService->>Creator: create(team, SCHEDULED)
Creator->>DB: persist recommendation entities
Creator-->>ScheduledService: success/failure
end
end
ScheduledService-->>ProblemScheduler: complete
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45분
Possibly related PRs
- feat : 수동 문제 추천 기능 #63: Recommendation 도메인 리팩터링(서비스 분리·미션 사이클·DTO 변경)과 많은 코드 수준 중복이 있어 강한 관련성 있음.
- 44 refactor schema feat 빅이슈 recommendation 스키마 재설계 개인 이력 보존 및 도메인 분리 #53: TeamRecommendation 제거 및 Recommendation 계층 재구성과 직접적으로 겹치는 리팩터링을 수행함.
- refactor(mail): 레거시 TeamRecommendation 이메일 발송 코드 제거 #113: 메일 전송 경로(메일 빌더/발송) 리팩터링 및 RecommendationEmailService와 연관된 변경 사항이 겹침.
Poem
🐰
옛 골격 벗겨 새 길 닦았네,
Creator가 씨 뿌리고 Mail이 편지하네,
여섯 시에 팀들 모여 문제 줄 세우고,
응답은 DTO에 빛나네,
당근 한 입에 배포 완료—깡충! 🥕✨
🚥 Pre-merge checks | ✅ 4 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Merge Conflict Detection |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main objective: splitting the monolithic RecommendationService into 5 focused services and removing legacy files. |
| Linked Issues check | ✅ Passed | The PR fully addresses issue #152: RecommendationService split into 5 services (RecommendationService, ScheduledRecommendationService, RecommendationEmailService, RecommendationCreator, MissionCyclePolicy), legacy entities/repositories removed, package structure reorganized, and tests updated accordingly. |
| Out of Scope Changes check | ✅ Passed | All changes directly align with the refactoring objectives: service splitting, legacy cleanup, file reorganization, and test updates. No unrelated changes detected. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
152-refactor-recommendationservice-책임-분리-레거시-정리
⚔️ Resolve merge conflicts (beta)
- Auto-commit resolved conflicts to branch
152-refactor-recommendationservice-책임-분리-레거시-정리 - Create stacked PR with resolved conflicts
- Post resolved changes as copyable diffs in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/ryu/studyhelper/recommendation/service/RecommendationServiceTest.java (1)
265-293:⚠️ Potential issue | 🟡 Minor
EmailSendableTimeValidation테스트가 코드베이스에 존재하지 않는 로직을 테스트합니다.이 테스트는 이메일 발송 가능 시간 로직(9시 이후 또는 1시 이전)을 검증하지만, 실제 구현에서는
EmailSendScheduler와RecommendationEmailService모두 단순히 매일 9시에 스케줄된 배치 작업으로 이메일을 발송합니다. 시간 검증 로직이 없으므로 이 테스트는 orphaned 상태입니다. 해당 테스트를 제거하거나, 실제 스케줄 동작(9시 배치 실행)을 검증하는 테스트로 리팩토링해 주세요.
🤖 Fix all issues with AI agents
In
`@src/main/java/com/ryu/studyhelper/recommendation/dto/response/RecommendationDetailResponse.java`:
- Around line 41-65: The static factory hardcodes status="SENT" and
sentAt=LocalDateTime.now() which misrepresents the domain and breaks test
determinism; update RecommendationDetailResponse.from to read the real values
from the domain (use recommendation.getStatus() and recommendation.getSentAt()
if available) and stop calling LocalDateTime.now() directly—instead add a Clock
parameter to the method signature (or accept a sentAt value) and use
LocalDateTime.now(clock) so callers can supply a testable clock or the actual
recommendation.sentAt as fallback.
In
`@src/main/java/com/ryu/studyhelper/recommendation/scheduler/EmailSendScheduler.java`:
- Around line 36-39: The catch block in EmailSendScheduler has inconsistent
indentation before the log.error call; remove the extra leading whitespace so
the log.error line aligns with the surrounding block, keeping the existing
parameters (endTime, startTime, e) unchanged; locate the catch(Exception e)
block in EmailSendScheduler and adjust the indentation of the log.error(...)
statement so it matches the closing brace and the long endTime calculation uses
the same indentation level as the declaration of endTime.
In
`@src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationCreator.java`:
- Around line 41-50: The create(Team team, RecommendationType type) method in
RecommendationCreator participates in the caller's transaction which causes
ScheduledRecommendationService.prepareDailyRecommendations() to mark the entire
transaction rollback-only when an exception occurs for one team; change
create(...) to run in its own transaction by annotating the method with
`@Transactional`(propagation = Propagation.REQUIRES_NEW) (and add the necessary
import for org.springframework.transaction.annotation.Propagation and
org.springframework.transaction.annotation.Transactional) so each team's
Recommendation, RecommendationProblem, and MemberRecommendation persist/rollback
independently.
In
`@src/main/java/com/ryu/studyhelper/recommendation/service/ScheduledRecommendationService.java`:
- Around line 22-26: The class-level `@Transactional` on
ScheduledRecommendationService causes the entire prepareDailyRecommendations()
run to share one transaction so a RuntimeException in one team marks the
transaction rollback-only and undoes other teams' work; fix by making team
processing transactional and isolated: either add `@Transactional`(propagation =
Propagation.REQUIRES_NEW) to RecommendationCreator.create() so each team's
create() runs in its own transaction, or remove the class-level `@Transactional`
from ScheduledRecommendationService and apply `@Transactional` only to the
specific methods that must be transactional (e.g., wrap per-team processing or
the methods that commit changes) so team-level try-catch actually prevents
rollbacks of other teams.
In
`@src/test/java/com/ryu/studyhelper/recommendation/service/ScheduledRecommendationServiceTest.java`:
- Around line 98-123: The test manualAt0AM_notFoundBySchedulerAt6AM currently
duplicates the first test; instead prepare a Recommendation entity with
createdAt = LocalDateTime.parse("2025-01-15T00:00:00") and stub
recommendationRepository.findFirstByTeamIdAndCreatedAtBetweenOrderById so that a
call with the midnight range (00:00) would return that recommendation, while the
scheduler's actual query at 06:00
(LocalDateTime.parse("2025-01-15T06:00:00")..06:00) returns Optional.empty;
update the mocks in ScheduledRecommendationServiceTest and keep the verify call
to ensure findFirstByTeamIdAndCreatedAtBetweenOrderById was invoked with the
06:00 range, proving the 00:00 manual recommendation is excluded from the 06:00
mission cycle.
🧹 Nitpick comments (11)
src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationCreator.java (1)
60-76: 개별save()호출 대신saveAll()사용을 고려해 보세요.
RecommendationProblem과MemberRecommendation을 반복문 안에서 개별save()로 저장하고 있습니다.saveAll()을 사용하면 배치 처리로 DB 라운드트립을 줄일 수 있습니다.♻️ saveAll() 적용 예시
private List<Problem> createRecommendationProblems(Recommendation recommendation, Team team) { List<Problem> problems = recommendProblemsForTeam(team); + List<RecommendationProblem> rps = new java.util.ArrayList<>(); for (Problem problem : problems) { RecommendationProblem rp = RecommendationProblem.create(problem); recommendation.addProblem(rp); - recommendationProblemRepository.save(rp); + rps.add(rp); } + recommendationProblemRepository.saveAll(rps); return problems; } private void createMemberRecommendations(Recommendation recommendation, Team team) { List<Member> teamMembers = teamMemberRepository.findMembersByTeamId(team.getId()); + List<MemberRecommendation> mrs = new java.util.ArrayList<>(); for (Member member : teamMembers) { MemberRecommendation mr = MemberRecommendation.create(member, recommendation, team); - memberRecommendationRepository.save(mr); + mrs.add(mr); } + memberRecommendationRepository.saveAll(mrs); }src/main/java/com/ryu/studyhelper/recommendation/service/ScheduledRecommendationService.java (1)
69-76:findAll()+ lazy loading으로 인한 N+1 쿼리 가능성이 있습니다.
teamRepository.findAll()로 전체 팀을 로드한 후team.getTeamMembers().isEmpty()에서 각 팀마다 lazy loading이 발생할 수 있습니다. 배치 작업(새벽 6시)이므로 즉시 문제가 되진 않지만, 팀 수가 늘어나면 성능에 영향을 줄 수 있습니다.src/main/java/com/ryu/studyhelper/recommendation/scheduler/ProblemRecommendationScheduler.java (1)
41-59: 프로덕션 코드에 테스트/디버그용 메서드가 남아있습니다.
testPrepareRecommendations()는 주석 처리된@Scheduled와 함께 디버그 용도로 추가된 메서드입니다.prepareDailyRecommendations()와 거의 동일한 코드가 중복되며, 프로덕션에 배포할 필요가 없는 코드입니다.로컬 테스트가 필요하다면 통합 테스트나 프로파일 기반 설정으로 분리하는 것이 좋습니다.
♻️ 테스트 메서드 제거
- - // 테스트용 첫 실행후 1초뒤에 딱 한번 문제 추천 배치 작업 시작 -// `@Scheduled`(initialDelay = 1000, fixedDelay = Long.MAX_VALUE) - public void testPrepareRecommendations() { - log.info("=== [테스트] 문제 추천 배치 작업 시작 ==="); - - long startTime = System.currentTimeMillis(); - - try { - scheduledRecommendationService.prepareDailyRecommendations(); - - long endTime = System.currentTimeMillis(); - log.info("=== [테스트] 문제 추천 배치 작업 완료 === (소요시간: {}ms)", endTime - startTime); - - } catch (Exception e) { - long endTime = System.currentTimeMillis(); - log.error("=== [테스트] 문제 추천 배치 작업 실패 === (소요시간: {}ms)", endTime - startTime, e); - } - } }src/test/java/com/ryu/studyhelper/recommendation/service/ScheduledRecommendationServiceTest.java (1)
156-176:teamMembers.add(new Object())— 타입 안전성이 떨어지는 리플렉션 사용입니다.
List<TeamMember>에new Object()를 추가하고 있습니다. 현재는isEmpty()체크만 사용하므로 동작하지만, 향후getActiveTeams필터링 로직이 변경되어 리스트 원소에 접근하면ClassCastException이 발생합니다. mockTeamMember를 사용하는 것이 더 안전합니다.♻️ 타입 안전한 TeamMember 사용
- `@SuppressWarnings`("unchecked") - java.util.List<Object> teamMembers = (java.util.List<Object>) teamMembersField.get(team); - teamMembers.add(new Object()); + `@SuppressWarnings`("unchecked") + java.util.List<com.ryu.studyhelper.team.domain.TeamMember> teamMembers = + (java.util.List<com.ryu.studyhelper.team.domain.TeamMember>) teamMembersField.get(team); + teamMembers.add(org.mockito.Mockito.mock(com.ryu.studyhelper.team.domain.TeamMember.class));src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationService.java (2)
61-78: 트랜잭션 내에서 이메일 발송 시 DB 커넥션이 오래 점유될 수 있습니다.
createManualRecommendation이 클래스 레벨@Transactional내에서 실행되므로,recommendationEmailService.send()(SMTP 호출)가 완료될 때까지 DB 커넥션이 유지됩니다. 팀원이 많거나 SMTP 서버가 느릴 경우 커넥션 풀 고갈 위험이 있습니다.이메일 발송을 트랜잭션 밖으로 분리하거나,
@Transactional(propagation = REQUIRES_NEW)를RecommendationEmailService.send()에 적용하거나,@Async로 비동기 처리하는 것을 권장합니다.♻️ 트랜잭션과 이메일 발송 분리 예시
추천 생성과 이메일 발송을 분리하는 방법:
+ `@Transactional` + public Recommendation createManualRecommendationInternal(Long teamId) { + Team team = teamRepository.findById(teamId) + .orElseThrow(() -> new CustomException(CustomResponseStatus.TEAM_NOT_FOUND)); + validateNoRecommendationToday(teamId); + return recommendationCreator.create(team, RecommendationType.MANUAL); + } + public RecommendationDetailResponse createManualRecommendation(Long teamId) { - Team team = teamRepository.findById(teamId) - .orElseThrow(() -> new CustomException(CustomResponseStatus.TEAM_NOT_FOUND)); - - validateNoRecommendationToday(teamId); - - Recommendation recommendation = recommendationCreator.create(team, RecommendationType.MANUAL); - - // 즉시 이메일 발송 + Recommendation recommendation = createManualRecommendationInternal(teamId); + Team team = teamRepository.findById(teamId).orElseThrow(); + + // 트랜잭션 커밋 후 이메일 발송 List<MemberRecommendation> memberRecommendations = memberRecommendationRepository.findByRecommendationId(recommendation.getId()); recommendationEmailService.send(memberRecommendations);
117-135:getMyTodayProblems에서 팀별 N+1 쿼리 패턴이 존재합니다.각 팀에 대해
findTodayRecommendation을 호출하면서 추천 조회 + 문제 조회 + 태그 조회가 반복됩니다. 사용자가 속한 팀 수가 많아지면 쿼리 수가 선형적으로 증가합니다.현재 일반적인 사용 패턴(소수 팀)에서는 허용 범위이나, 향후 성능 이슈 발생 시 배치 쿼리로의 최적화를 고려해 주세요.
src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationEmailService.java (2)
21-25:sendAll()이 단일 트랜잭션에서 전체 배치를 처리합니다.클래스 레벨
@Transactional로 인해sendAll()의 모든 이메일 발송과 상태 업데이트가 하나의 트랜잭션에서 수행됩니다. 대량 이메일 발송 시 다음 문제가 발생할 수 있습니다:
- 프로세스 중단 시 모든 상태 업데이트(
markEmailAsSent/Failed)가 롤백됨- 장시간 DB 커넥션 점유
배치 처리 시 건별 또는 청크 단위 트랜잭션 분리를 고려해 주세요.
♻️ 청크 단위 트랜잭션 분리 예시
sendEmail을 별도 트랜잭션으로 분리하는 접근:+ `@Transactional`(propagation = org.springframework.transaction.annotation.Propagation.REQUIRES_NEW) private boolean sendEmail(MemberRecommendation mr) {또는
sendAll()에서@Transactional을 제거하고,sendEmail을 self-injection 등으로 별도 트랜잭션 메서드로 추출하는 방법도 있습니다.
62-66:send()에서 발송 결과 로깅이 누락되어 있습니다.
sendAll()과 달리send()에서는 성공/실패 건수를 추적하거나 로깅하지 않습니다. 수동 추천 이메일 발송 시에도 결과 요약 로그가 있으면 운영 모니터링에 도움이 됩니다.♻️ 결과 로깅 추가 예시
public void send(List<MemberRecommendation> memberRecommendations) { + int successCount = 0; + int failCount = 0; for (MemberRecommendation mr : memberRecommendations) { - sendEmail(mr); + if (sendEmail(mr)) { + successCount++; + } else { + failCount++; + } } + log.info("수동 추천 이메일 발송 완료 - 대상: {}개, 성공: {}개, 실패: {}개", + memberRecommendations.size(), successCount, failCount); }src/main/java/com/ryu/studyhelper/recommendation/scheduler/EmailSendScheduler.java (1)
42-60:testsendPendingEmails메서드: 네이밍 위반, 코드 중복, 주석 처리된 어노테이션.몇 가지 문제가 있습니다:
- 네이밍:
testsendPendingEmails→testSendPendingEmails(lowerCamelCase 위반)- 코드 중복:
sendPendingEmails()와 동일한 로직이 그대로 복사되어 있습니다.- 주석 처리된
@Scheduled: 프로덕션 코드에 테스트용 주석 코드가 남아 있습니다.테스트용 메서드가 정말 필요하다면 프로파일 조건(
@Profile("local"))을 사용하고, 로직 중복은 기존sendPendingEmails()를 호출하는 방식으로 해결해 주세요. 불필요하다면 제거를 권장합니다.♻️ 제거 또는 리팩토링 제안
옵션 1: 제거 (권장)
- // 테스트용 첫 실행후 10초뒤에 딱 한번 문제 추천 배치 작업 시작 -// `@Scheduled`(initialDelay = 10000, fixedDelay = Long.MAX_VALUE) - public void testsendPendingEmails() { - log.info("=== 이메일 발송 배치 작업 시작 ==="); - - long startTime = System.currentTimeMillis(); - - try { - recommendationEmailService.sendAll(); - - long endTime = System.currentTimeMillis(); - log.info("=== 이메일 발송 배치 작업 완료 === (소요시간: {}ms)", endTime - startTime); - - } catch (Exception e) { - long endTime = System.currentTimeMillis(); - log.error("=== 이메일 발송 배치 작업 실패 === (소요시간: {}ms)", endTime - startTime, e); - } - }옵션 2: 프로파일 기반으로 유지 시
+ `@Profile`("local") + `@Scheduled`(initialDelay = 10000, fixedDelay = Long.MAX_VALUE) -// `@Scheduled`(initialDelay = 10000, fixedDelay = Long.MAX_VALUE) - public void testsendPendingEmails() { - ...중복 코드... - } + public void testSendPendingEmails() { + sendPendingEmails(); + }src/test/java/com/ryu/studyhelper/recommendation/service/RecommendationServiceTest.java (2)
464-506: 리플렉션 기반 헬퍼 메서드가 내부/외부 클래스에 중복되어 있습니다.
createTeamWithIdAndName이 내부 클래스GetMyTodayProblemsValidation(Line 486)과 유사한 패턴의createTeamWithId가 외부 클래스(Line 511)에 각각 존재합니다.setRecommendationId도 내부 클래스에만 있고,createMemberWithId도 내부 클래스에만 있습니다.이 헬퍼들을 외부 클래스 레벨로 통합하면 중복이 줄고 유지보수가 편해집니다.
Also applies to: 509-543
141-142: Mock 반환값에LocalDateTime.now()사용이 시스템 시계에 의존합니다.
createRecommendationWithCreatedAt(TEAM_ID, LocalDateTime.now())에서LocalDateTime.now()는 시스템 시계를 사용합니다. 테스트의 다른 부분에서는fixedClock을 사용하고 있으므로, 여기서도 고정된 시간을 사용하는 것이 일관적입니다. 현재는 Mock 반환값이므로 실제 테스트 결과에 영향은 없지만, 가독성과 일관성을 위해 명시적인 시간을 사용하는 것을 권장합니다.♻️ 고정 시간 사용 예시
when(recommendationCreator.create(any(), any())) - .thenReturn(createRecommendationWithCreatedAt(TEAM_ID, LocalDateTime.now())); + .thenReturn(createRecommendationWithCreatedAt(TEAM_ID, LocalDateTime.parse(dateTime)));
src/main/java/com/ryu/studyhelper/recommendation/dto/response/RecommendationDetailResponse.java
Show resolved
Hide resolved
src/main/java/com/ryu/studyhelper/recommendation/scheduler/EmailSendScheduler.java
Show resolved
Hide resolved
| @Service | ||
| @RequiredArgsConstructor | ||
| @Transactional | ||
| @Slf4j | ||
| public class ScheduledRecommendationService { |
There was a problem hiding this comment.
클래스 레벨 @Transactional과 팀별 try-catch가 충돌합니다.
@Transactional이 클래스 레벨에 적용되어 prepareDailyRecommendations() 전체가 하나의 트랜잭션으로 실행됩니다. 특정 팀에서 RuntimeException이 발생하면 catch로 잡더라도 트랜잭션이 rollback-only로 마킹되어, 이미 성공한 다른 팀의 데이터도 롤백됩니다.
팀별 독립 처리를 보장하려면 RecommendationCreator.create()에 @Transactional(propagation = Propagation.REQUIRES_NEW)를 적용하거나, 클래스 레벨 @Transactional을 제거하고 개별 메서드에만 적용하는 방법을 고려해 주세요.
🔧 수정 방안 1: RecommendationCreator에 REQUIRES_NEW 적용
RecommendationCreator.java에서:
+ `@Transactional`(propagation = Propagation.REQUIRES_NEW)
Recommendation create(Team team, RecommendationType type) {ScheduledRecommendationService.java에서:
`@Service`
`@RequiredArgsConstructor`
-@Transactional
`@Slf4j`
public class ScheduledRecommendationService {🤖 Prompt for AI Agents
In
`@src/main/java/com/ryu/studyhelper/recommendation/service/ScheduledRecommendationService.java`
around lines 22 - 26, The class-level `@Transactional` on
ScheduledRecommendationService causes the entire prepareDailyRecommendations()
run to share one transaction so a RuntimeException in one team marks the
transaction rollback-only and undoes other teams' work; fix by making team
processing transactional and isolated: either add `@Transactional`(propagation =
Propagation.REQUIRES_NEW) to RecommendationCreator.create() so each team's
create() runs in its own transaction, or remove the class-level `@Transactional`
from ScheduledRecommendationService and apply `@Transactional` only to the
specific methods that must be transactional (e.g., wrap per-team processing or
the methods that commit changes) so team-level try-catch actually prevents
rollbacks of other teams.
...test/java/com/ryu/studyhelper/recommendation/service/ScheduledRecommendationServiceTest.java
Show resolved
Hide resolved
- 죽은 테스트 EmailSendableTimeValidation 제거 - 내부 helper를 클래스 레벨로 승격 (createTeamMember, setRecommendationId 등) - reflection helper를 setFieldValue로 통합 - MEMBER_ID 클래스 상수로 승격, 중복 TEAM_ID_1 제거
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/ryu/studyhelper/recommendation/scheduler/EmailSendScheduler.java`:
- Around line 41-60: Remove the duplicate test/debug method
testsendPendingEmails(): delete the entire testsendPendingEmails() method
(including the commented `@Scheduled`) and rely on the existing
sendPendingEmails() implementation; if a one-off test run is needed, implement
it in unit/integration tests or temporarily invoke sendPendingEmails() from test
code instead of leaving a debug method in production, and if you intend to keep
any helper, rename it to follow lowerCamelCase (e.g., testSendPendingEmails) but
preferably remove it to satisfy DRY and naming guidelines.
src/main/java/com/ryu/studyhelper/recommendation/scheduler/EmailSendScheduler.java
Outdated
Show resolved
Hide resolved
- 스케줄러 테스트용 dead method 제거
- RecommendationCreator 예외를 CustomException으로 통일
- deprecated GET /team/{teamId}/today-problem 엔드포인트 제거
- 미사용 getTodayRecommendation 서비스 메서드 제거
- RecommendationCreatorTest, RecommendationEmailServiceTest 추가
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationCreator.java`:
- Around line 43-52: 현재 create(Team team, RecommendationType type)가 먼저
createRecommendation(...)로 DB에 Recommendation을 저장한 뒤
createRecommendationProblems(...)에서 recommendProblemsForTeam(...)을 통해 핸들 검증을 수행해
예외가 발생할 수 있습니다; 이 순서를 바꿔 create() 시작부에서 recommendProblemsForTeam(...)이나 새 검증
메서드(예: validateTeamHasAuthenticatedHandles(team) 또는
ensureValidatedHandles(team))를 호출해 인증된 핸들 유무를 확인하고 예외가 있을 경우 즉시 반환/예외 처리하여 DB에
Recommendation을 저장(createRecommendation(...))하거나
createMemberRecommendations(...)를 호출하기 전에 불필요한 쓰기를 방지하세요. 이 변경은
createRecommendationProblems(...), recommendProblemsForTeam(...) 호출 시점과
createRecommendation(...) 호출 순서를 뒤바꾸는 방식으로 적용하세요.
🧹 Nitpick comments (5)
src/main/java/com/ryu/studyhelper/common/enums/CustomResponseStatus.java (1)
93-94:NO_VERIFIED_HANDLE의 섹션 배치가 의미적으로 불일치합니다.
NO_VERIFIED_HANDLE은HttpStatus.BAD_REQUEST (400)을 사용하는 클라이언트 요청 검증 오류이지만,6000: Server_Error섹션에 배치되어 있습니다.5000: NOT_MATCH섹션의 다른BAD_REQUEST상태들(예:INVALID_PROBLEM_LEVEL_RANGE,PROBLEM_NOT_SOLVED_YET)과 함께 배치하는 것이 코드 그룹의 의미와 일치합니다.또한, Sentry 설정(
SentryConfig.java)에서httpStatus < 500인CustomException은 Sentry로 전송하지 않으므로 동작에는 영향이 없지만, 향후 코드 범위 기반 로직이 추가될 경우 혼란을 줄 수 있습니다.src/test/java/com/ryu/studyhelper/recommendation/service/RecommendationEmailServiceTest.java (2)
136-151:send()테스트에서 상태 변경 및 저장 검증이 누락되었습니다.
sendsToAllMembers테스트는mailSender.send()호출만 검증하고 있습니다.sendsPendingEmails테스트처럼EmailSendStatus.SENT상태 변경과memberRecommendationRepository.save()호출도 검증하면 테스트 신뢰도가 높아집니다.♻️ 제안
// then verify(mailSender, times(2)).send(any(MailMessage.class)); + verify(memberRecommendationRepository, times(2)).save(any()); + assertThat(mr1.getEmailSendStatus()).isEqualTo(EmailSendStatus.SENT); + assertThat(mr2.getEmailSendStatus()).isEqualTo(EmailSendStatus.SENT);
156-184: 이메일 없는 회원 테스트에서save()호출 검증을 추가하는 것을 권장합니다.프로덕션 코드(
sendEmail)에서 이메일이 없으면markEmailAsFailed()후memberRecommendationRepository.save(mr)를 호출합니다. 상태 검증은 되어 있지만, FAILED 상태가 실제로 DB에 영속되는지(save호출) 검증하면 더 완전한 테스트가 됩니다.♻️ 예시 (nullEmail_marksFailed)
// then assertThat(mr.getEmailSendStatus()).isEqualTo(EmailSendStatus.FAILED); verify(mailSender, never()).send(any()); + verify(memberRecommendationRepository).save(mr);src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationService.java (1)
61-78:createManualRecommendation메서드에서RecommendationCreator의 트랜잭션 전파 방식에 따른 데이터 조회 확인이 필요합니다.Line 71에서
memberRecommendationRepository.findByRecommendationId()로 방금 생성된MemberRecommendation들을 조회합니다. 현재는 같은 트랜잭션 내에서 동작하므로 문제없지만, 이전 리뷰에서 제안된 대로RecommendationCreator.create()에REQUIRES_NEW를 적용하면 별도 트랜잭션에서 커밋된 데이터를 조회하게 됩니다.
REQUIRES_NEW적용 시에도 커밋 후 조회이므로 정상 동작하겠지만,create()메서드가 이미 생성한MemberRecommendation목록을 직접 반환하도록 변경하면 불필요한 DB 조회를 제거할 수 있습니다.♻️ create()에서 MemberRecommendation 목록을 반환하는 방안
RecommendationCreator.create()가Recommendation뿐만 아니라 생성된MemberRecommendation목록도 함께 반환하면(예: record나 Pair),findByRecommendationId쿼리를 제거할 수 있습니다.// RecommendationCreator.java -Recommendation create(Team team, RecommendationType type) { +CreationResult create(Team team, RecommendationType type) { Recommendation recommendation = createRecommendation(team, type); List<Problem> problems = createRecommendationProblems(recommendation, team); - createMemberRecommendations(recommendation, team); + List<MemberRecommendation> memberRecommendations = createMemberRecommendations(recommendation, team); ... - return recommendation; + return new CreationResult(recommendation, memberRecommendations); }// RecommendationService.java -Recommendation recommendation = recommendationCreator.create(team, RecommendationType.MANUAL); -List<MemberRecommendation> memberRecommendations = - memberRecommendationRepository.findByRecommendationId(recommendation.getId()); +var result = recommendationCreator.create(team, RecommendationType.MANUAL); +recommendationEmailService.send(result.memberRecommendations());src/main/java/com/ryu/studyhelper/recommendation/RecommendationController.java (1)
29-29:@Validated가 더 이상 필요하지 않을 수 있습니다.
count파라미터의@Min/@Max검증이 제거되면서 현재 이 컨트롤러에서@Validated가 활성화하는 검증 대상이 없습니다. 향후 검증이 추가될 가능성을 고려하여 유지해도 무방하지만, 현재로서는 불필요합니다.
| Recommendation create(Team team, RecommendationType type) { | ||
| Recommendation recommendation = createRecommendation(team, type); | ||
| List<Problem> problems = createRecommendationProblems(recommendation, team); | ||
| createMemberRecommendations(recommendation, team); | ||
|
|
||
| log.info("팀 '{}' 추천 생성 완료 - 타입: {}, 문제: {}개", | ||
| team.getName(), type, problems.size()); | ||
|
|
||
| return recommendation; | ||
| } |
There was a problem hiding this comment.
Recommendation 저장이 핸들 검증보다 먼저 수행됩니다.
create() 메서드 실행 순서:
createRecommendation()→ DB에Recommendation저장 (Line 44)createRecommendationProblems()→recommendProblemsForTeam()→ 핸들 검증 (Line 82-84)
인증된 핸들이 없으면 Recommendation이 이미 저장된 후에 예외가 발생합니다. 트랜잭션 롤백으로 정리되겠지만, 검증 로직을 create() 시작 부분으로 이동하면 불필요한 DB 쓰기를 방지하고 의도가 명확해집니다.
🛡️ 제안: 검증을 먼저 수행
Recommendation create(Team team, RecommendationType type) {
+ validateHandlesExist(team);
Recommendation recommendation = createRecommendation(team, type);
List<Problem> problems = createRecommendationProblems(recommendation, team);
createMemberRecommendations(recommendation, team);
...
}
+private void validateHandlesExist(Team team) {
+ List<String> handles = teamMemberRepository.findHandlesByTeamId(team.getId());
+ if (handles.isEmpty()) {
+ log.warn("팀 '{}'에 인증된 핸들이 없습니다", team.getName());
+ throw new CustomException(CustomResponseStatus.NO_VERIFIED_HANDLE);
+ }
+}🤖 Prompt for AI Agents
In
`@src/main/java/com/ryu/studyhelper/recommendation/service/RecommendationCreator.java`
around lines 43 - 52, 현재 create(Team team, RecommendationType type)가 먼저
createRecommendation(...)로 DB에 Recommendation을 저장한 뒤
createRecommendationProblems(...)에서 recommendProblemsForTeam(...)을 통해 핸들 검증을 수행해
예외가 발생할 수 있습니다; 이 순서를 바꿔 create() 시작부에서 recommendProblemsForTeam(...)이나 새 검증
메서드(예: validateTeamHasAuthenticatedHandles(team) 또는
ensureValidatedHandles(team))를 호출해 인증된 핸들 유무를 확인하고 예외가 있을 경우 즉시 반환/예외 처리하여 DB에
Recommendation을 저장(createRecommendation(...))하거나
createMemberRecommendations(...)를 호출하기 전에 불필요한 쓰기를 방지하세요. 이 변경은
createRecommendationProblems(...), recommendProblemsForTeam(...) 호출 시점과
createRecommendation(...) 호출 순서를 뒤바꾸는 방식으로 적용하세요.
1 participant
레거시 정리: TeamRecommendation, RecommendationStatus 등 5개 삭제
스케줄러 이동: infrastructure/scheduler/ → recommendation/scheduler/
관련 이슈
변경 내용
변경 유형
테스트
스크린샷 (UI 변경 시)
참고사항
Summary by CodeRabbit
릴리스 노트