fix(ec-upload): add order_id dedup key + correct success_count#43
Open
illia-sapryga wants to merge 6 commits into
Open
fix(ec-upload): add order_id dedup key + correct success_count#43illia-sapryga wants to merge 6 commits into
illia-sapryga wants to merge 6 commits into
Conversation
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.
…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.
Two related issues in the Enhanced Conversions for Leads upload path: 1. **No dedup key on ClickConversion.** Google dedupes EC uploads by (conversion_action, order_id). The current parser/apply never set order_id, so any re-upload of the same source row double-counts every row that actually matched. Add an optional `Order ID` CSV column that the parser reads and the apply passes through to ClickConversion.order_id when present. Backwards-compatible: existing CSVs without the column continue to parse fine; rows without an order_id are still sent (just without dedup protection on re-upload). 2. **success_count silently overcounted.** The previous heuristic counted any response row whose `user_identifiers` were truthy as a success. But the Google Ads API echoes user_identifiers back on FAILED rows too (it's just returning what you sent), so the check returned True for 100% of rows regardless of actual match outcome. Fix: count only rows where the response's `conversion_action` resource_name is populated — that's the field Google leaves empty when matching fails. Surface dedup hygiene in the draft preview: - `rows_with_order_id` count - `dedup_warnings[]` flags fully-missing or partially-missing coverage - `sample_rows[].order_id` shows the values being sent Tests: +12 cases covering parse with/without Order ID, partial coverage, preview surfacing, proto propagation, and realistic success_count under 0%/partial/full match. Full suite: 529 passed.
…en files The repo root accumulates per-client work (audits, generated reports, Urable/Square tokens, etc.) and personal ops scripts that should never ship. Add explicit rules so these stay local and a stray `git add -A` can't accidentally leak credentials.
illia-sapryga
force-pushed
the
feat/ec-upload-order-id
branch
from
May 29, 2026 06:20
5cc1186 to
44a8557
Compare
6 tasks
Owner
|
Reviewed as part of the full stack — all three fixes here are correct and valuable: the Per the split plan in #34, these fold into split PR (c) — conversion uploads with SHA-256 enforcement — rather than merging from this branch. Please carry the |
3 tasks
This was referenced Jul 16, 2026
Contributor
Author
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
2 participants
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.
Summary
Two related issues in the Enhanced Conversions for Leads upload path (added in #34):
ClickConversion— Google dedupes EC uploads by(conversion_action, order_id). The current parser/apply never setorder_id, so any re-upload of the same source row double-counts every row that matched the first time.success_countsilently overcounted — the previous heuristic counted any response row whoseuser_identifierswere truthy as a success, but the Google Ads API echoesuser_identifiersback on FAILED rows too. So the count always returned 100% regardless of actual match outcome.Why this matters
Real-world example from BBP: 115 rows uploaded yesterday, script reported "success_count: 115". The actual matched count visible in the conversion action's stats is 4 — a ~3% real match rate. The 96% gap was invisible until now.
What's in the change
Parser — accept an optional
Order IDCSV column:order_idis read from each roworder_idis empty string (proto default — Google treats it as unset)Apply — propagate
order_idtoClickConversion.order_id:response.results[i].conversion_action— the field Google leaves empty when matching failsDraft preview — surface dedup hygiene:
rows_with_order_idcountdedup_warnings[]flags fully-missing or partially-missing coveragesample_rows[].order_idshows the values being sent so users can sanity-check before applyingTest plan
Order IDwhen presentrows_with_order_idcountorder_idpresent (would double-count)order_idtoClickConversionprotoorder_idunset when row lacks itsuccess_countreports 0 / partial / full correctly based on response shapetest_conversion_actions.pycasesDepends on
Sits on top of #34 (which introduced
_apply_upload_enhanced_conversions_for_leads). The diff againstmainreflects all of #34's work; once #34 merges, this PR rebases to a focused +289/-4.