feat(ads): asset extension tools + RSA update + conversion-action mgmt#34
feat(ads): asset extension tools + RSA update + conversion-action mgmt#34illia-sapryga wants to merge 7 commits into
Conversation
The TestModulesUseDynamicEnums class was importing adloop.ads.conversion_actions and asserting on adloop.ads.write._VALID_PROMOTION_OCCASIONS — modules / constants that don't exist in this PR. They live in the follow-up PR (kLOsk#34, feat/asset-and-conversion-tools) and should be tested there. Branch A is intentionally minimal: the helper + helper-only unit tests. CI now passes — 12/12 tests.
* feat(ads): dynamic Google Ads enum introspection helper
Adds adloop.ads.enums.enum_names() — pulls valid enum member names
straight from the google-ads SDK at the API version we're pinned to
(see adloop.ads.client.GOOGLE_ADS_API_VERSION).
Drops the need to hand-maintain parallel lists like:
_VALID_CONVERSION_ACTION_TYPES = {"AD_CALL", "WEBSITE_CALL", ...}
— which otherwise drift every time the SDK or API version updates.
The helper is module-cached (functools.lru_cache) so the no-auth
GoogleAdsClient used for introspection is built once per process,
and every enum_names() call after the first is essentially free.
UNSPECIFIED + UNKNOWN sentinels are dropped by default since they
are protobuf zero-values that should never appear in user input.
Pure addition. No existing validators changed in this PR; downstream
PRs (conversion-action tools, in-place asset updates, promotion
helper refactors) will switch their hardcoded enum sets to enum_names()
calls in their own commits.
12 unit tests cover:
- enum_names returns a frozenset
- UNSPECIFIED + UNKNOWN are excluded by default and includable on opt-in
- Critical members (AD_CALL, WEBSITE_CALL, GOOGLE_SEARCH_ATTRIBUTION_DATA_DRIVEN,
USE_RESOURCE_LEVEL_CALL_CONVERSION_ACTION, BLACK_FRIDAY) are present
- ConversionActionCountingTypeEnum returns exactly {ONE_PER_CLICK, MANY_PER_CLICK}
- Unknown enum names raise AttributeError
- LRU cache returns the same frozenset instance on repeat calls
- The introspection client is memoized
* test: remove cross-module enum tests from test_ads_enums.py
The TestModulesUseDynamicEnums class was importing adloop.ads.conversion_actions
and asserting on adloop.ads.write._VALID_PROMOTION_OCCASIONS — modules /
constants that don't exist in this PR. They live in the follow-up PR
(#34, feat/asset-and-conversion-tools) and should be tested there.
Branch A is intentionally minimal: the helper + helper-only unit tests.
CI now passes — 12/12 tests.
Adds the complete asset-extension and conversion-action management
surface that AdLoop was missing. Three logical chunks bundled here
because they share `ads/write.py`'s dispatch + apply infrastructure
and would conflict if split into separate PRs.
- `draft_call_asset` — campaign or customer scope, E.164 normalization,
ad-schedule restriction, optional conversion-action override
- `draft_location_asset` — Google Business Profile-backed AssetSet
(LOCATION_SYNC), with label/listing filters
- `draft_image_assets` — campaign image extensions from local files
with MIME + dimension validation
- `draft_callouts`, `draft_structured_snippets`, `draft_sitelinks` —
refactored to support BOTH campaign-scope AND customer-scope (the
account-level CustomerAsset path that propagates to every eligible
campaign automatically)
- `add_ad_schedule` — Mon-Sat 8am-9pm-style scheduling via
AdScheduleInfo CampaignCriterion
- `add_geo_exclusions` — negative geo CampaignCriterion records to
shrink a broad include list
- `_apply_assets()` shared helper routing a populate fn through either
CampaignAsset or CustomerAsset linkage based on scope
- Phone-number E.164 normalization with US/CA + EU trunk-prefix handling
- Update existing RSAs without delete-then-recreate
- Partial update via FieldMask — only the fields the caller passes
are modified
- Headlines/descriptions accept either bare strings (unpinned) or
{"text": "...", "pinned_field": "HEADLINE_1"} dicts (pinned)
- `draft_create_conversion_action` — AD_CALL / WEBSITE_CALL / WEBPAGE
/ GA4_CUSTOM with value, threshold, attribution model, counting type
- `draft_update_conversion_action` — partial update with FieldMask;
rename / promote-demote / set value / change duration threshold
- `draft_remove_conversion_action` — irreversible removal (warns that
SMART_CAMPAIGN_* and GOOGLE_HOSTED types reject mutation)
The 3 conversion-action tools live in their own module
`adloop/ads/conversion_actions.py` and route through dispatch via
`_apply_*_conversion_action_route` shims kept in `ads/write.py`.
- `link_asset_to_customer` — promote existing Asset rows from
campaign-scope to account-level (CustomerAsset)
- `update_call_asset` / `update_sitelink` / `update_callout` —
in-place asset updates with FieldMask
- `draft_promotion` / `update_promotion` — PromotionAsset create
+ swap (PromotionAsset is immutable; update is implemented as
create-new-link-old-unlink)
- Promotion module uses `enum_names("PromotionExtensionOccasionEnum")`
+ `enum_names("PromotionExtensionDiscountModifierEnum")` from
the dynamic-enums helper
- Conversion-actions module uses `enum_names("...")` for all 4
Google Ads enums it validates against — drops 4 hardcoded enum
sets that were drifting from the SDK
- Auto-cleanup script `scripts/cleanup_sitelink_links.py` for
duplicate sitelink CampaignAsset detection
- `tests/test_ads_extensions.py` — comprehensive validation +
apply-handler tests for every new function (uses fake services
mirroring the google-ads SDK protos; no network)
- `tests/test_conversion_actions.py` — 29 tests
- `tests/test_update_rsa.py` — RSA update integration tests
- All 430 tests pass
Add two new MCP tools for pushing offline conversion data directly to Google Ads, complementing the existing conversion-action management: - draft_upload_call_conversions: uploads call conversions (Caller ID + Call Start Time) to UPLOAD_CALLS-typed actions via ConversionUploadService.UploadCallConversions - draft_upload_enhanced_conversions_for_leads: uploads hashed PII to UPLOAD_CLICKS-typed actions via UploadClickConversions with user_identifiers populated. Works retroactively — no "action must exist before the conversion" constraint that UPLOAD_CALLS has. Both tools follow the AdLoop draft → confirm_and_apply pattern with plan storage, dry-run, audit logging, and per-row error reporting via partial_failure mode. Adds 25 new unit tests covering CSV parsing, action-name resolution, payload shape, partial-failure handling, and type-mismatch rejection (UPLOAD_CALLS vs UPLOAD_CLICKS). Also removes scripts/cleanup_sitelink_links.py, which was scoped to a specific client cleanup and shouldn't ship in the public tool.
bacb626 to
7666a41
Compare
|
Hey @kLOsk, just rebased this onto main and resolved the conflicts (12 commits behind → caught up). Also extended the PR with two new conversion-upload tools — If you'd prefer those upload tools split into a separate PR for easier review, happy to break them out. Otherwise, anything else you'd want before merging? |
…es on create Two issues surfaced when creating ConversionActions via the Google Ads API that were not covered by validation: 1. include_in_conversions_metric is IMMUTABLE on create — Google derives it from the conversion category and rejects any value supplied in the create mutate with IMMUTABLE_FIELD. The apply function previously set it unconditionally, blocking every AD_CALL / PHONE_CALL_LEAD create with the default True value. Removed the line; callers who need to change it use draft_update_conversion_action after create. 2. default_value > 0 paired with always_use_default_value=False is rejected as INVALID_VALUE — it reads as "you set a value but told me not to use it". The draft tool defaults always_use_default_value to False, so the typical "set a default $250 lead value" call kept failing with no useful error. Added an auto-correct in the draft function: when default_value > 0 and the caller didn't explicitly opt out, flip always_use_default_value to True. Tests added cover both regressions plus a sanity test that callers who already set the flag correctly still get their value through, and that default_value=0 keeps the flag False (for snippet-value callers). All 227 tests pass.
draft_call_asset previously supported only customer or campaign scope.
This blocks the multi-service / multi-ad-group pattern where each ad
group inside one campaign needs its own tracked phone number wired to
its own service-specific call conversion action (e.g. Tint ad group
fires "Tint Call from Ad" worth $400, PPF ad group fires "PPF Call
from Ad" worth $2,000).
Added an ad_group_id parameter to both the MCP wrapper in server.py
and the impl in ads/write.py, with most-specific-wins scope precedence
(ad_group > campaign > customer). When ad_group_id is provided, the
apply function creates an AdGroupAsset link instead of a CampaignAsset
or CustomerAsset link.
Tests added:
- draft test: ad_group_id selects ad_group scope and populates entity
- draft test: ad_group_id wins over campaign_id (precedence rule)
- apply test: creates an AdGroupAsset link with correct ad_group path
and conversion action wiring
- apply test: missing ad_group_id raises a clear error
Also fixed a latent bug in _FakeGoogleAdsService.ad_group_path: it
inherited from _FakePathService which uses a single prefix
("campaigns") for all resource path helpers, so ad_group_path returned
"customers/.../campaigns/{id}" instead of "customers/.../adGroups/{id}".
Override returns the correct prefix; only affects the new ad-group
scope test, no existing tests touched ad_group_path.
NOTE: when adding new params to MCP tools you must update BOTH the
impl in ads/*.py AND the wrapper in server.py — the wrapper's signature
is what FastMCP uses to build the JSON schema exposed to clients. The
first patch attempt only updated the impl and broke at call time with
unexpected_keyword_argument.
All 227 tests pass.
…rice asset tool Extend callout, structured-snippet, and promotion assets to support ad-group-level linking (AdGroupAsset), and add a new draft_price_asset tool for price extensions. - _apply_assets gains an ad_group scope branch; all asset types that route through it (callouts, snippets, promotions) inherit it. Most- specific-wins precedence (ad_group > campaign > customer) via new _asset_scope helper. Verified against Google Ads API: CALLOUT, STRUCTURED_SNIPPET, PROMOTION all support AdGroupAsset. - draft_callouts / draft_structured_snippets / draft_promotion gain an ad_group_id param (impl + server.py MCP wrappers). - New draft_price_asset: PriceAsset with 3-8 offerings, FROM/UP_TO/ AVERAGE qualifier, per-offering header(<=25)/description(<=25)/price/ final_url/unit. Validates count, dupes, char limits, price>0, URL reachability, and enum membership. Same draft->preview->apply flow, wired into dispatch, ad-group/campaign/customer scope. Tests: +25 (ad-group scope for callouts/snippets/promotions; full draft + apply + dispatch coverage for price assets). 349 targeted tests pass.
CampaignSharedSetService.mutate_campaign_shared_sets() does not accept a flattened partial_failure kwarg (unlike GoogleAdsService.mutate) — it raised 'unexpected keyword argument partial_failure' at apply time on both attach and detach. Build a MutateCampaignSharedSetsRequest with customer_id/operations/partial_failure set on it and pass request=. The existing tests masked this: their fake _mutate accepted (customer_id, operations, partial_failure=...) — a signature the real SDK rejects. Rewrote all 4 fakes to take request= and read fields off the request object, so they now assert the real API contract and would catch a regression. 291 write+extension tests pass.
StructuredSnippetAsset header+values are immutable once created, so update is a swap (mirrors update_promotion): create new snippet asset, link at the same scope, unlink the old link. - Generalizes the swap to ad-group/campaign/customer scope via a new _find_asset_link helper (extends _find_promotion_link, which only handled campaign/customer) — reusable for any future asset swap. - draft_update via update_structured_snippet (impl) + server.py MCP wrapper; wired into the apply dispatch. - Tests: +13 (draft validation/scope precedence; apply across customer and ad-group scope incl. old-link unlink + not-found skip; dispatch wiring). New _FakeAdGroupAssetService. 300 write+extension tests pass.
|
There's a lot of genuinely good work in here, but it can't be reviewed or merged in this shape: the branch's server.py registers ~40 tools whose modules aren't in the PR (GTM, ServiceTitan), which would |
|
Addendum after a deeper pass over the full stack (this + #42/#43) — a few items to fold into the four-way split from my last comment. The split proposal stands; these are additions, not changes. Two things that affect how you rebuild the branches:
Smaller correctness notes for the resubmissions:
For balance: the deep pass confirmed the parts I praised before hold up — the safety-model discipline across all ~17 real tools is flawless, the commit messages documenting API landmines (IMMUTABLE_FIELD, the user_identifiers echo-back trap, order_id semantics) are genuinely valuable, and the AD_IMAGE→marketing-image field-type fix is a real bug find. This work is worth the cleanup. |
|
Thanks for the thorough review — the four-way split makes sense, and the "only its own registrations, rebuilt on main" framing is the right call. Starting through it in your order. Split 1/4 is up: #50 — asset extensions + the per-ad-group multi-scope refactor.
Next up, in your sequence: (b) conversion-action CRUD (with the GAQL backslash-escaping fix and the Appreciate the detailed notes — they made the rebuild straightforward. |
|
All four splits are up, in your order — each rebuilt fresh on
Suggested review/merge order: (a) → (b) → (c) → (d). #42 and #43 are superseded by (d)/(c) — I'll close them once you've confirmed, unless you'd like them kept for reference. Two things worth your eye specifically: (1) in (c), the EC path is a rewrite rather than a port — raw PII in, hash at preview, apply from frozen rows — to satisfy "hashing enforced + PII out of the plan/audit + no CSV re-read"; and (2) the raw Thanks again for the thorough review notes — they're what made a clean rebuild possible. |
2 participants
Summary
Three logical chunks bundled here because they share
ads/write.py's dispatch + apply infrastructure and would conflict at the file level if split into separate PRs.update_responsive_search_ad)Depends on #33 (dynamic Google Ads enum introspection) — please merge that first.
What's added
1. Asset extension tools
draft_call_asset+19164609257).draft_location_assetLOCATION_SYNCAssetSet, with label/listing filtersdraft_image_assetsdraft_calloutsCustomerAssetpath that propagates to every eligible campaign automatically)draft_sitelinksdraft_structured_snippetsadd_ad_scheduleAdScheduleInfo CampaignCriterionadd_geo_exclusionsCampaignCriterionrecords to shrink a broad include listlink_asset_to_customerCustomerAsset) — for re-using shared logos / sitelinks across campaignsupdate_call_asset/update_sitelink/update_calloutFieldMask— only the fields the caller passes are modifieddraft_promotion/update_promotionPromotionAssetcreate + swap (PromotionAsset is immutable, so 'update' is implemented as create-new-link-old-unlink atomically)_apply_assets()CampaignAssetorCustomerAssetlinkage based on scope_normalize_phone_e164()2. RSA in-place update
update_responsive_search_adFieldMask— only the fields the caller passes are modified. Headlines/descriptions accept either bare strings (unpinned) or{\"text\": ..., \"pinned_field\": \"HEADLINE_1\"}dicts (pinned).3. Conversion-action management (3 new tools)
draft_create_conversion_actiondraft_update_conversion_actionFieldMask— rename / promote-to-Primary / demote-to-Secondary / set value / change duration threshold / change attributiondraft_remove_conversion_actionSMART_CAMPAIGN_*andGOOGLE_HOSTEDtypes reject mutation withMUTATE_NOT_ALLOWED(Google manages those).The 3 conversion-action tools live in their own module
adloop/ads/conversion_actions.pyand route through_execute_plan's dispatch via lazy-import_apply_*_conversion_action_routeshims.Refactors (uses #33's
enum_names())The
_VALID_TYPES,_VALID_CATEGORIES,_VALID_COUNTING_TYPES,_VALID_ATTRIBUTION_MODELSsets inconversion_actions.pyare pulled dynamically from the SDK viaenum_names(). Same for_VALID_PROMOTION_OCCASIONS,_VALID_DISCOUNT_MODIFIERS,_VALID_CALL_REPORTING_STATESinads/write.py.The hardcoded versions of those sets had drifted from the SDK. The
ConversionActionTypeEnumset was missing 29 of 40 valid values;PromotionExtensionOccasionEnumwas missing 2.Validated end-to-end
This batch was used to build out a real Google Ads account end-to-end (BGI Tint, customer 8202753856):
Calls from Ads (≥90s)AD_CALL @ $250,Website Call (GFN ≥90s)WEBSITE_CALL @ $250 with 90s threshold) and bound them to a custom conversion goalUSE_RESOURCE_LEVEL_CALL_CONVERSION_ACTIONTest plan
uv run pytest→ 430/430 teststests/test_ads_extensions.py— comprehensive validation + apply-handler tests for every new function (uses fake services mirroring the google-ads SDK protos; no network)tests/test_conversion_actions.py— 29 tests covering validation, partial update mask, FieldMask correctness, MCP registrationtests/test_update_rsa.py— RSA update integration tests_apply_*handlers verified to emit the rightFieldMaskpaths via fake service inspectionKnown follow-ups (out of scope for this PR)
ads/write.py(_VALID_BIDDING_STRATEGIES,_VALID_CHANNEL_TYPES,_VALID_DAYS_OF_WEEK,_VALID_HEADLINE_PINS, etc.) could be migrated toenum_names()in a separate refactor PR.update_call_assetallows full-list replacement ofad_schedule_targetsbut no append-mode — append would be useful for some workflows.