Feat/#141 예산 수정 - 네이버 광고 예산 수정 API 구현#142
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough캠페인·광고그룹 예산(및 광고그룹 입찰가) 수정 기능을 추가했습니다: DTO·에러코드 → Feign 클라이언트 → 서비스 권한·검증·PUT 호출 → 컨트롤러 엔드포인트 → Swagger 문서 → 엔티티/컨버터 매핑 순으로 구현됨. Changes네이버 광고 예산 수정 기능
Sequence DiagramsequenceDiagram
participant Client
participant NaverAdApiController
participant NaverAdApiService
participant OrgMemberRepository
participant NaverClient
Client->>NaverAdApiController: PUT /campaigns/{campaignId}/budget
NaverAdApiController->>NaverAdApiService: updateCampaignBudget(userId, connectionId, campaignId, request)
NaverAdApiService->>OrgMemberRepository: findOrgMemberByConnectionOrg(...)
OrgMemberRepository-->>NaverAdApiService: OrgMember (role)
NaverAdApiService->>NaverAdApiService: validate ADMIN, validate budget/bid rules
NaverAdApiService->>NaverClient: updateCampaignBudget(headers, campaignId, fields, body)
NaverClient-->>NaverAdApiService: CampaignResponse
NaverAdApiService-->>NaverAdApiController: CampaignResponse
NaverAdApiController-->>Client: DataResponse<CampaignResponse>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/NaverAdApiController.java (1)
144-166: 💤 Low value엔드포인트 구조는 기존 패턴과 잘 맞습니다 👍
@AuthenticationPrincipal(expression = "userId")로 인증 사용자 추출 → 서비스 위임 →DataResponse래핑까지 컨트롤러는 얇게 유지되어 좋습니다. 한 가지만, 예산/입찰가의 "10의 배수" 검증이 지금은 서비스 안의 수동 if문으로 흩어져 있는데, 향후 DTO에 Bean Validation(예: 커스텀 제약 또는@Positive)을 붙이고 컨트롤러에서@Valid로 받으면 검증 위치가 한곳으로 모여 더 선언적이고 일관됩니다. 지금 당장 막는 이슈는 아니라 참고만 해주세요.🤖 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/advertisement/presentation/NaverAdApiController.java` around lines 144 - 166, Move the "10의 배수" and other budget validation into declarative Bean Validation: add appropriate validation annotations to NaverDTO.UpdateCampaignBudgetRequest and NaverDTO.UpdateAdGroupBudgetRequest (e.g., `@Positive` for >0 and a custom constraint annotation like `@MultipleOf`(value=10) implemented via ConstraintValidator to enforce multiples of 10), then annotate the controller parameters in updateCampaignBudget and updateAdGroupBudget with `@Valid` so Spring validates before calling naverAdApiService; keep the service logic focused on business rules only and remove the duplicated manual if-checks from the service methods.src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java (1)
180-188: ⚡ Quick win조직원/ADMIN 권한 검증 로직이 두 메서드에 그대로 복붙되어 있어요 (DRY)
180-188 블록과 214-222 블록이 connectionId 조회 → orgId 추출 → ADMIN 검증까지 사실상 동일합니다. 한쪽 정책이 바뀌면 양쪽을 모두 고쳐야 하므로, 공통 private 헬퍼로 추출하는 걸 권장합니다.
♻️ 권한 검증 헬퍼 추출 예시
private void validateAdmin(Long userId, Long connectionId, NaverAdErrorCode notFoundCode) { PlatformConnection connection = connectionRepository.findWithAccountAndOrgById(connectionId) .orElseThrow(() -> new AdvertisementHandler(notFoundCode)); Long orgId = connection.getPlatformAccount().getOrganization().getId(); OrgMember orgMember = orgMemberRepository.findByUserIdAndOrgId(userId, orgId) .orElseThrow(() -> new OrgHandler(OrgErrorCode.ORG_MEMBER_NOT_FOUND)); if (orgMember.getRole() != OrgRole.ADMIN) { throw new OrgHandler(OrgErrorCode.ORG_MEMBER_FORBIDDEN); } }As per coding guidelines, "SOLID 원칙, 의존성 주입(DI)" 점검 항목에 따라 중복 제거를 제안합니다.
🤖 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/advertisement/domain/service/NaverAdApiService.java` around lines 180 - 188, Extract the duplicated ADMIN permission checks into a single private helper (e.g., validateAdmin) and replace both inline blocks with calls to it; the helper should use connectionRepository.findWithAccountAndOrgById(connectionId) and throw new AdvertisementHandler(...) when not found, derive orgId from connection.getPlatformAccount().getOrganization().getId(), check orgMemberRepository.findByUserIdAndOrgId(userId, orgId) (throwing new OrgHandler(OrgErrorCode.ORG_MEMBER_NOT_FOUND) if absent) and finally verify orgMember.getRole() == OrgRole.ADMIN (throw new OrgHandler(OrgErrorCode.ORG_MEMBER_FORBIDDEN) otherwise), so callers simply call validateAdmin(userId, connectionId, NaverAdErrorCode.NAVER_CAMPAIGN_BUDGET_UPDATE_FAILED) or appropriate notFoundCode.
🤖 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/domain/service/NaverAdApiService.java`:
- Around line 176-178: Both updateCampaignBudget and updateAdGroupBudget
currently keep a `@Transactional` open while making slow external Naver PUT calls;
split the short DB/permission checks into dedicated methods annotated
`@Transactional`(readOnly = true) (e.g., validateCampaignOwnership,
validateAdGroupOwnership) that perform only the necessary JPA reads, remove
`@Transactional` from updateCampaignBudget and updateAdGroupBudget so the external
HTTP calls run outside any transaction/DB connection, and ensure any required
entities/DTOs from the validation methods are returned for use by the
non-transactional methods.
- Around line 181-182: The repository call
connectionRepository.findWithAccountAndOrgById currently throws an
AdvertisementHandler with NaverAdErrorCode.NAVER_CAMPAIGN_BUDGET_UPDATE_FAILED
(500) when empty; change this to throw a new/not-found-specific
AdvertisementHandler error (add or use a NaverAdErrorCode like
NAVER_CONNECTION_NOT_FOUND mapped to 404) so missing PlatformConnection returns
404, and make the identical change where the same pattern is used in the
ad-group block that calls findWithAccountAndOrgById (around the other
occurrence). Also update the Swagger response documentation sections that
describe these endpoints (the docs around the current 196-202 and 213-217
regions) to include the 404 response and reference the new error code.
- Around line 191-193: The current validation in NaverAdApiService only checks
request.dailyBudget() % 10 == 0 which lets negative multiples pass; update the
validation to require a positive 10-multiple (e.g., if (request.dailyBudget() ==
null || request.dailyBudget() <= 0 || request.dailyBudget() % 10 != 0) throw new
AdvertisementHandler(NaverAdErrorCode.NAVER_INVALID_BUDGET_VALUE)); apply the
same positive 10-multiple check to the bidAmt validation referenced around the
bidAmt handling (lines ~225-230) so negative values are rejected before calling
the Naver API.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java`:
- Around line 180-188: Extract the duplicated ADMIN permission checks into a
single private helper (e.g., validateAdmin) and replace both inline blocks with
calls to it; the helper should use
connectionRepository.findWithAccountAndOrgById(connectionId) and throw new
AdvertisementHandler(...) when not found, derive orgId from
connection.getPlatformAccount().getOrganization().getId(), check
orgMemberRepository.findByUserIdAndOrgId(userId, orgId) (throwing new
OrgHandler(OrgErrorCode.ORG_MEMBER_NOT_FOUND) if absent) and finally verify
orgMember.getRole() == OrgRole.ADMIN (throw new
OrgHandler(OrgErrorCode.ORG_MEMBER_FORBIDDEN) otherwise), so callers simply call
validateAdmin(userId, connectionId,
NaverAdErrorCode.NAVER_CAMPAIGN_BUDGET_UPDATE_FAILED) or appropriate
notFoundCode.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/NaverAdApiController.java`:
- Around line 144-166: Move the "10의 배수" and other budget validation into
declarative Bean Validation: add appropriate validation annotations to
NaverDTO.UpdateCampaignBudgetRequest and NaverDTO.UpdateAdGroupBudgetRequest
(e.g., `@Positive` for >0 and a custom constraint annotation like
`@MultipleOf`(value=10) implemented via ConstraintValidator to enforce multiples
of 10), then annotate the controller parameters in updateCampaignBudget and
updateAdGroupBudget with `@Valid` so Spring validates before calling
naverAdApiService; keep the service logic focused on business rules only and
remove the duplicated manual if-checks from the service methods.
🪄 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: 552ea33e-8f31-46ac-a28a-6977c0c8fac5
📒 Files selected for processing (6)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/exception/code/NaverAdErrorCode.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/NaverAdApiController.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/docs/NaverAdApiControllerDocs.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/client/NaverClient.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/dto/NaverDTO.java
ojy0903
left a comment
ojy0903
left a comment
There was a problem hiding this comment.
P4: 고생하셨습니다! SQLD 시험 때문에 좀 늦게 확인했네요...ㅠㅠ
AdGroup 쪽에 입찰가 필드 추가하는건 구글은 광고 그룹에도 예산이 있는지 잘 모르겠지만... 일단 Meta 에서는 활용 가능할 거 같아서 추가해도 문제되진 않을 것 같습니다.
그리고 추가한다면 AdGroup 쪽에 Long bidAmount, Long dailyBudget 이런 식으로 추가하고, 플랫폼 API 에서 저희 서비스 AdGroup 엔티티로 받아올 때 단위만 잘 변환시켜주면 될 것 같습니다...!!
좋습니다! 수정하려면 기존 값이 얼마인지 보여주는 것이 괜찮을 것 같아서 저장하는 것이 좋을 것 같긴한데 그러면 스케줄러 쪽에도 저 필드를 동기화 시켜주는 로직을 추가해야 해서 지민님 의견도 듣고 추가해보겠습니다! |
There was a problem hiding this comment.
P4: 고생하셨습니다! 리뷰가 늦었네요 ㅠㅠ 죄송합니다.. API로 예산 수정하는 로직을 깔끔하게 구현해주셔서 좋네요!
다만 구글은 광고 그룹 단위로 예산을 수정할 수 없고(예산 수정은 캠페인 단위로만 수정 가능) 입찰가만 조정이 가능해서, 만약 AdGroup에 예산과 입찰가 필드를 추가한다면 구글은 광고그룹 예산을 null로 두면 될 것 같네요! (만약 구글만 null로 두는 게 별로라면 세 플랫폼 모두 광고 그룹에서는 입찰가만 수정할 수 있게 입찰가만 필드로 추가하는 방법도 있을 것 같네요...)
예산 수정 시점도 타임라인 그래프에서 보여줘야하니 기록할 수 있는 필드를 추가하는 아이디어 좋다고 생각합니다!
좋습니다! 그러면 필드는 예산, 입찰가 2개로 추가하고, 예산 수정 시점은 입찰가 수정 시점으로 하겠습니다! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/entity/AdCampaign.java`:
- Around line 97-100: The updateBudget method in AdCampaign currently ignores
null inputs (if (budget != null) this.budget = budget;), so calls from
NaverAdApiService.updateCampaignBudget passing dailyBudget = null won't clear
the stored budget; change AdCampaign.updateBudget to always assign the incoming
value (this.budget = budget) so a null value clears the local field (and ensure
any DB mapping allows null if necessary).
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdGroup.java`:
- Around line 65-68: The updateBudget method currently treats budget==null as
"no change", but callers sometimes mean "clear the budget"
(useDailyBudget=false); change the signature of AdGroup.updateBudget to include
a boolean flag (e.g., boolean useDailyBudget) and implement logic: if
useDailyBudget is false set this.budget = null (clearing saved budget) and set
this.bidAmount only if bidAmount != null; if useDailyBudget is true then apply
the existing behavior (set this.budget = budget when budget != null and
this.bidAmount = bidAmount when bidAmount != null). Update call sites such as
NaverAdApiService.updateAdGroupBudget and NaverConverter.updateAdGroup to pass
the new useDailyBudget flag accordingly.
🪄 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: 561d6efc-9c03-4b5c-8bf0-311efd125e60
📒 Files selected for processing (6)
src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/exception/code/NaverAdErrorCode.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdCampaign.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/persistence/entity/AdGroup.javasrc/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/docs/NaverAdApiControllerDocs.javasrc/main/java/com/whereyouad/WhereYouAd/infrastructure/client/naver/converter/NaverConverter.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/domain/service/NaverAdApiService.java
- src/main/java/com/whereyouad/WhereYouAd/domains/advertisement/presentation/docs/NaverAdApiControllerDocs.java
| public void updateBudget(Long budget, Long bidAmount) { | ||
| if (budget != null) this.budget = budget; | ||
| if (bidAmount != null) this.bidAmount = bidAmount; | ||
| } |
There was a problem hiding this comment.
budget은 null을 “미수정”이 아니라 “해제”로 해석해야 하는 케이스가 있습니다.
Line 66 조건식 때문에 useDailyBudget=false 흐름에서 전달되는 budget=null이 반영되지 않아, 광고그룹 예산이 DB에 잔존할 수 있습니다. NaverAdApiService.updateAdGroupBudget와 NaverConverter.updateAdGroup 모두 해당 경로를 타고 들어옵니다.
🔧 제안 수정(의도 전달을 위해 플래그 포함)
- public void updateBudget(Long budget, Long bidAmount) {
- if (budget != null) this.budget = budget;
- if (bidAmount != null) this.bidAmount = bidAmount;
- }
+ public void updateBudget(Boolean useDailyBudget, Long budget, Long bidAmount) {
+ if (Boolean.FALSE.equals(useDailyBudget)) {
+ this.budget = null;
+ } else if (budget != null) {
+ this.budget = budget;
+ }
+ if (bidAmount != null) {
+ this.bidAmount = bidAmount;
+ }
+ }(호출부에서 useDailyBudget 전달도 함께 맞춰주세요.)
🤖 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/advertisement/persistence/entity/AdGroup.java`
around lines 65 - 68, The updateBudget method currently treats budget==null as
"no change", but callers sometimes mean "clear the budget"
(useDailyBudget=false); change the signature of AdGroup.updateBudget to include
a boolean flag (e.g., boolean useDailyBudget) and implement logic: if
useDailyBudget is false set this.budget = null (clearing saved budget) and set
this.bidAmount only if bidAmount != null; if useDailyBudget is true then apply
the existing behavior (set this.budget = budget when budget != null and
this.bidAmount = bidAmount when bidAmount != null). Update call sites such as
NaverAdApiService.updateAdGroupBudget and NaverConverter.updateAdGroup to pass
the new useDailyBudget flag accordingly.
3 participants
📌 관련 이슈
🚀 개요
네이버 광고 예산 수정 API를 이용하여 캠페인 or 광고 그룹에 할당된 예산을 수정합니다.
📄 작업 내용
📸 스크린샷 / 테스트 결과 (선택)
하루 예산 (dailyBudget)
하루 동안 광고에 쓸 수 있는 최대 금액. 이 금액을 다 쓰면 그날 광고가 자동으로 멈춤.
기본 입찰가 (bidAmt)
광고 클릭 1번에 최대 얼마까지 지불할 의향이 있는지 설정하는 금액. 경쟁 광고주보다 입찰가가 높을수록 더 좋은 위치에 광고가 노출됨.
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
(예: 이 로직이 최선일까요? P2)
(예: 예외 처리 누락 여부 확인 부탁드립니다. P1)
Ad-Group쪽에는 광고 예산을 저장하는 필드가 없는 걸로 아는데 추가하는 편이 괜찮을까요? 그리고 추가한다면 하루 예산이랑 기본 입찰가 2개다 추가하는 것이 괜찮을까요..? 제 생각에는 둘 다 수정가능한 값이라 둘 다 저장하는 것이 괜찮을 것 같습니다..! 그리고 저희 서비스 내부에서 수정한 값에 대해서 budget_update_at같은 필드를 또 추가해서 예산 수정 시점을 반환하는 것도 괜찮을 것 같습니다..!
Summary by CodeRabbit
릴리스 노트
New Features
Security / Access
Documentation