Repository navigation
feat(advertising): add campaign bid controls - #11621
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
NubsCarson
left a comment
There was a problem hiding this comment.
[cloud-audit] COMMENT — money controls hold; two non-blocking drift caveats.
Verified (money focus):
- Spend gated by balance: bid controls add no new spend path. Budget is still charged via
creditsService.deductCreditsbeforeprovider.createCampaign(packages/cloud/shared/src/lib/services/advertising/index.ts:522-562 on develop), andbudgetAmountremainsz.number().positive()(schemas.ts:78). There is no numeric bid amount in this PR —bidStrategy/optimizationGoalare closed enums (CampaignBidStrategySchema = z.enum(["cpm","cpc","cpa"]), schemas.ts), so no negative/overflow bid is possible. - TikTok rejection refunds cleanly:
validateTikTokCampaignBidControlsreturnssuccess:falsefrom insideprovider.createCampaign, which lands in the existing full-refund branch (refundCredits(... createCampaign + budgetCredits ...), index.ts:583-585). No stranded credits. - Authorization: POST derives org from the authed user (
organizationId: user.organization_id, campaigns/route.ts:80), never the body; PATCH/GET on[id]/route.ts:24and serviceupdateCampaign(index.ts:676) both enforcecampaign.organization_id !== organizationIdfail-closed. - Approval gate (#11516) intact: Meta ad set and Google campaign are still created
status: "PAUSED"(meta.ts, google.ts:362), so bid controls cannot trigger spend before approval; the content-safety gate now also sees the bid text (index.ts:55-56).
Caveats (non-blocking):
- MED — PATCHed bid controls never reach the platform. The service persists
bid_strategy/optimization_goalinto metadata on update (index.ts:738-750), butmetaAdsProvider.updateCampaignonly sendsname(meta.ts:369-371), Google's update ignores bid fields, and the TikTok validator is wired only into create (tiktok.ts:234). A user changing cpm to cpa gets an API success while the live campaign keeps billing the old way — stored state misrepresents real spend behavior. Suggest: either propagate on update or reject bid-control updates like TikTok create does. - LOW — Google create now always injects a bidding strategy. With no controls selected,
mapBidControlsToGoogleCampaigndefaults to{ manualCpm: {} }(google.ts:92-111), which Google rejects on SEARCH-channel campaigns (the default for traffic/awareness). Fail-closed with full refund, so no money loss, but search campaign creation may now error. - LOW — stale-snapshot metadata merge in updateCampaign can clobber a concurrent sync's
external_ad_set_ids/last_sync_at(index.ts:732-744);external_ad_set_idsfeeds the delete path.
Tests cover schema rejection, provider mapping, persistence, and promotion pass-through (9 passing) — solid for the create path; the update-path drift above is untested because the behavior is missing.
[cloud-audit]
|
Lane verification (Fable-5): pushed b91a64439e adding 🤖 Generated with Claude Code |
|
Review (worktree-verified on the merged tree, PR + develop tip): correct create-path implementation; one real defect found and fixed in What holds up
Defect found + fixed (pushed as
|
|
Reviewed and pushed a fix commit: Finding fixed:
Validation run after the fix:
|
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…ampaign approval gate The updateCampaign persistence test mocked adAccountsRepository.findById without a status field. After develop merged #11619 (updateCampaign now throws unless account.status === "active"), the merged tree fails this test with 'Ad account is not active (status: undefined)'. Mark the mock account active so the test exercises the metadata merge, not the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…losed No ad-platform adapter applies bid-control changes to a live campaign (Meta bid controls live on the ad set created with the campaign; Google/TikTok updateCampaign only push name/budget/dates). The previous update path persisted bid_strategy/optimization_goal into local campaign metadata anyway, so a PATCH would silently report a strategy the platform never received — local/remote drift hidden as success. updateCampaign now throws before any credit movement or platform call when bidStrategy/optimizationGoal are present, keeping the typed schema fields so clients get an explicit error instead of a zod-stripped silent ignore. Test replaced accordingly and asserts no deduct/refund/provider/ repo-update side effects fire (mutation-checked red without the guard). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
15e46a8 to
5f1e80c
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Adds `linkedin` to AdPlatform across schemas, DB typing, credit markup, the provider registry, and app-promotion validation, plus a real LinkedIn Marketing API provider (versioned REST gateway): - adAccounts search finder for account discovery/validation - campaign group + paused campaign creation with objective, budget, geo-targeting (urn:li:geo pass-through, worldwide default, loud failure on free-text locations), and #11621 bid-control mapping to costType/optimizationTargetType per the documented combinations - Rest.li PARTIAL_UPDATE for update/pause/activate and PENDING_DELETION deletes - Images/Videos API media upload (initializeUpload -> PUT -> finalizeUpload) owned by the account's organization reference - inline dark-post creative creation (creatives?action=createInline) - adAnalytics analytics-finder metrics mapping - OAuth2 refresh_token grant support Unit tests use fixtures lifted from the Microsoft Learn LinkedIn Marketing API reference pages and drive advertisingService with the real provider to prove the #11619 approval gate, #11621 bid metadata, and the fail-closed refund path apply to LinkedIn automatically. A credential-gated linkedin.real.test.ts live lane loud-skips without LINKEDIN_ADS_ACCESS_TOKEN. Closes #11663. Refs #11361. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Summary
cpm/cpc/cpa) and optimization goals (reach/clicks/conversions) to advertising create/update schemas, DTOs, and campaign metadata.Fixes #11596.
Verification
bun installbun test packages/cloud/shared/src/lib/services/__tests__/ad-campaign-bid-strategy.test.ts- 9 passedbunx biome check packages/cloud/shared/src/lib/services/__tests__/ad-campaign-bid-strategy.test.ts packages/cloud/shared/src/lib/services/app-promotion.ts packages/ui/src/cloud-ui/components/promotion/promote-app-dialog.tsx packages/cloud/shared/src/lib/services/advertising/providers/meta.ts packages/cloud/shared/src/lib/services/advertising/providers/google.ts packages/cloud/shared/src/lib/services/advertising/providers/tiktok.ts packages/cloud/shared/src/lib/services/advertising/index.ts packages/cloud/shared/src/lib/services/advertising/types.ts packages/cloud/shared/src/lib/services/advertising/schemas.ts packages/cloud/shared/src/db/schemas/ad-campaigns.ts packages/cloud/api/v1/advertising/campaigns/route.ts packages/cloud/api/v1/advertising/campaigns/[id]/route.tsbun run --cwd packages/cloud/shared typecheckbun run --cwd packages/cloud/api typecheckbun run --cwd packages/ui typecheckbun run --cwd packages/app audit:app- 348 captures passed; touched/appsroute verdicts weregoodfor mobile portrait, mobile landscape, desktop landscape, and iPad portrait. The command exited 1 on unrelated minimalism ratchet failures inplugin-inbox-gui @ mobile-landscapeandplugin-screenshare-gui @ mobile-portrait.Evidence
.github/issue-evidence/11596-ad-bid-strategy.md