Feat/#125 타임라인 내 성과 상태 판별 구현#136
Conversation
Walkthrough타임라인 생성/수정 시 현재·비교 기간의 MetricSumProjection 집계값을 조회해 클릭/전환/노출/ROAS 비율을 비교하고, 평균 비율 기준으로 PerformanceStatus를 계산하여 타임라인에 반영하도록 구현했다. Changes타임라인 성과 상태 판별 로직 구현
Sequence DiagramsequenceDiagram
participant Client
participant TimelineServiceImpl
participant MetricFactRepository
participant TimelineUtil
participant Database
Client->>TimelineServiceImpl: createTimeline/updateTimeline 요청
TimelineServiceImpl->>MetricFactRepository: past 기간 집계 조회 (MetricSumProjection)
MetricFactRepository->>Database: JPQL 집계 쿼리 실행
Database-->>MetricFactRepository: pastFacts
TimelineServiceImpl->>MetricFactRepository: current 기간 집계 조회 (MetricSumProjection)
MetricFactRepository->>Database: JPQL 집계 쿼리 실행
Database-->>MetricFactRepository: currentFacts
TimelineServiceImpl->>TimelineUtil: calculatePerformanceStatus(timeline, currentFacts, pastFacts)
TimelineUtil-->>TimelineServiceImpl: PerformanceStatus
TimelineServiceImpl->>TimelineServiceImpl: timeline.setPerformanceStatus(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java (1)
73-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick win비교 데이터 존재 검증 조건과 합계 조회 조건이 달라 오판정될 수 있습니다.
비교 데이터 존재 여부는 상태 조건 없이 조회하고, 실제 성과 합계는
OrgStatus.ACTIVE+Status.ON_GOING으로만 조회하고 있습니다. 이 불일치 때문에 “비교 데이터 있음”으로 통과했는데 계산 시에는 사실상 데이터가 없는 케이스가 발생할 수 있습니다. 두 쿼리의 조건을 동일하게 맞춰주세요.🔧 제안 수정 예시
- boolean hasComparisonData = metricFactRepository.existsByTimeBucketBetweenAndOrg( + boolean hasComparisonData = metricFactRepository.existsByTimeBucketBetweenAndOrgIdAndStatus( comparisonDates.start().atStartOfDay(), comparisonDates.end().atTime(LocalTime.MAX), - orgId + orgId, + OrgStatus.ACTIVE, + Status.ON_GOING );Also applies to: 83-98, 142-146, 151-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java` around lines 73 - 77, The existence check using metricFactRepository.existsByTimeBucketBetweenAndOrg is missing the same status filters used when computing sums, causing mismatches; update the existsBy... calls to include the same OrgStatus.ACTIVE and Status.ON_GOING predicates (e.g., use or add an existsByTimeBucketBetweenAndOrgAndOrgStatusAndStatus-style method or include the same spec/criteria used by the sum methods) so the “hasComparisonData” and other existence checks match the sum queries exactly; apply this same change to the other occurrences noted (the other existsBy... checks around the ranges referenced: lines corresponding to the 83-98, 142-146, and 151-166 blocks) so all existence checks mirror the sum query filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java`:
- Around line 73-77: The existence check using
metricFactRepository.existsByTimeBucketBetweenAndOrg is missing the same status
filters used when computing sums, causing mismatches; update the existsBy...
calls to include the same OrgStatus.ACTIVE and Status.ON_GOING predicates (e.g.,
use or add an existsByTimeBucketBetweenAndOrgAndOrgStatusAndStatus-style method
or include the same spec/criteria used by the sum methods) so the
“hasComparisonData” and other existence checks match the sum queries exactly;
apply this same change to the other occurrences noted (the other existsBy...
checks around the ranges referenced: lines corresponding to the 83-98, 142-146,
and 151-166 blocks) so all existence checks mirror the sum query filters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80cf9388-4139-44ee-8cd1-edabaeabb1f4
📒 Files selected for processing (2)
src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/util/TimelineUtil.java
There was a problem hiding this comment.
P4: 고생하셨습니다! 초기 설정으로 10프로면 무난하고 좋은 것 같습니다!
궁금한 점이 있는데 과거 데이터도 조회할 때 광고 현재 상태가 ON_GOING인 데이터만 받아오고 있는데 만약 1년전 데이터 등과 비교할 때도 동일하게 현재까지도 ON_GOING인 데이터만 가져오는 것이 괜찮을까요? 과거 데이터는 status 관련 없이 받아와서 현재 진행중인 광고 데이터와 비교하는 건 어떤가요?
지금도 진행 중인 광고 데이터만 비교하고 싶다는 의도면 괜찮은 것 같습니다!
ojy0903
left a comment
ojy0903
left a comment
There was a problem hiding this comment.
P4: 고생하셨어요! MetricSumProjection 사용해서 깔끔하게 구현된거 같습니다!!
제가 보기에도 비교 기준 10% 정도가 적당한거 같고, 민규님이 리뷰에서 말해주신 과거 데이터를 status 관련은 제 생각에는... 일단 지금대로 과거 지표 데이터도 ON_GOING 으로 조회하는거는 어떨까요? 과거 값을 Status 상관 없이 집계하면 지금 중단된 광고의 과거 지표가 포함되면서 전반적으로 지표 비교가 UNDERPERFORM 으로 나올 수 있을 거 같아서요...
민규님이랑 의견이 다르니... 최종 결정은 지민님에게 맡기겠습니다... 🥲
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/MetricFactRepository.java`:
- Around line 63-81: The query in MetricFactRepository's
findMetricsSumByOrgIdAndDateRange filters p.organization.status but not the
campaign status; if requirements are to exclude completed campaigns add a
campaign status filter (e.g., join or reference the AdCampaign status via
m.adCampaign.status) to the WHERE clause and add a corresponding parameter
(e.g., `@Param`("campaignStatus") OrgStatus campaignStatus) or hard-code the
allowed statuses (e.g., ON_GOING) in the predicate; update the method signature
and query accordingly to reference m.adCampaign.status when filtering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d94d547-e328-4239-9059-337634eca012
📒 Files selected for processing (2)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/repository/MetricFactRepository.javasrc/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/whereyouad/WhereYouAd/domains/timeline/domain/service/TimelineServiceImpl.java
넵 피드백 감사합니다! 고민해봤을 때 과거에 집행했지만 종료된 데이터까지 현재와 비교하지 않으면 비교할 과거 데이터의 양이 많지 않을 수도 있고, 또 타임라인을 만드는 유저는 결국 캠페인 중단 여부로 생긴 광고 성과나 과거와 달라진 액션으로 생긴 광고 성과 등을 비교하려는 의도가 있을 듯 하여, 과거 데이터는 Status 상관 없이 집계하도록 하겠습니다..! |
3 participants
📌 관련 이슈
🚀 개요
Timeline 생성 및 수정 시 사용자가 선택한 지표값들에 대해 비교 기준 기간과 비교하여, 성과 상태를 평균 이상/이하인지 판별하는 로직을 구현하였습니다.
📄 작업 내용
📸 스크린샷 / 테스트 결과 (선택)
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
성과 상태 (PerformanceStatus) 판단 시 사용자가 선택한 지표들(ex. 클릭수, ROAS...)에서 전체 평균을 내서 기준 기간(ex. 지난주, 지난달 등..)과 비교해 10% 이상, 이하로 차이가 나는지 확인하도록 하였는데 이 로직이 괜찮을지 확인해주시면 감사하겠습니다!
Summary by CodeRabbit