Feat/#141 예산 수정 - 네이버 광고 예산 수정 API 구현#142
Open
kingmingyu wants to merge 11 commits into
Open
Conversation
3 tasks
|
Warning Review limit reached
More reviews will be available in 13 minutes and 6 seconds. 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 (3)
Walkthrough캠페인 및 광고그룹 예산을 수정하는 기능을 구현함. 요청 DTO 정의 → Feign 클라이언트 메서드 추가 → 조직 권한 검증과 입력값 검증을 포함한 서비스 로직 → HTTP 엔드포인트 → Swagger 문서화 순서로 진행됨. Changes네이버 광고 예산 수정 기능
Sequence DiagramsequenceDiagram
participant Client
participant NaverAdApiController
participant NaverAdApiService
participant OrgMemberRepository
participant NaverClient
participant NaverAPI
Client->>NaverAdApiController: PUT /campaigns/{campaignId}/budget
NaverAdApiController->>NaverAdApiService: updateCampaignBudget(userId, connectionId, campaignId, request)
NaverAdApiService->>OrgMemberRepository: 조직원 조회
OrgMemberRepository-->>NaverAdApiService: OrgMember
NaverAdApiService->>NaverAdApiService: ADMIN 권한 검증
NaverAdApiService->>NaverAdApiService: dailyBudget % 10 == 0 검증
NaverAdApiService->>NaverClient: updateCampaignBudget(headers, campaignId, fields, body)
NaverClient->>NaverAPI: PUT /ncc/campaigns/{campaignId}
NaverAPI-->>NaverClient: CampaignResponse
NaverClient-->>NaverAdApiService: CampaignResponse
NaverAdApiService-->>NaverAdApiController: CampaignResponse
NaverAdApiController-->>Client: DataResponse<CampaignResponse>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
📝 리뷰 포인트이 PR은 엔드-투-엔드 기능 구현으로, 데이터 계약부터 API 노출까지 일관성 있게 구성되어 있습니다. 다음 항목들을 꼼꼼히 확인해주세요: 1️⃣ 권한 검증 로직
2️⃣ 입력값 검증
3️⃣ Naver API 호출 구성
4️⃣ DTO 매핑
5️⃣ API 문서화
6️⃣ 예외 처리
잘 짜인 점: 🎉
🚥 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
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
1 participant
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.
📌 관련 이슈
🚀 개요
네이버 광고 예산 수정 API를 이용하여 캠페인 or 광고 그룹에 할당된 예산을 수정합니다.
📄 작업 내용
📸 스크린샷 / 테스트 결과 (선택)
하루 예산 (dailyBudget)
하루 동안 광고에 쓸 수 있는 최대 금액. 이 금액을 다 쓰면 그날 광고가 자동으로 멈춤.
기본 입찰가 (bidAmt)
광고 클릭 1번에 최대 얼마까지 지불할 의향이 있는지 설정하는 금액. 경쟁 광고주보다 입찰가가 높을수록 더 좋은 위치에 광고가 노출됨.
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
(예: 이 로직이 최선일까요? P2)
(예: 예외 처리 누락 여부 확인 부탁드립니다. P1)
Ad-Group쪽에는 광고 예산을 저장하는 필드가 없는 걸로 아는데 추가하는 편이 괜찮을까요? 그리고 추가한다면 하루 예산이랑 기본 입찰가 2개다 추가하는 것이 괜찮을까요..? 제 생각에는 둘 다 수정가능한 값이라 둘 다 저장하는 것이 괜찮을 것 같습니다..! 그리고 저희 서비스 내부에서 수정한 값에 대해서 budget_update_at같은 필드를 또 추가해서 예산 수정 시점을 반환하는 것도 괜찮을 것 같습니다..!
Summary by CodeRabbit
릴리스 노트
New Features
Documentation