feat(conversions): offline conversion uploads + #43 fold (split 3/4 of #34) - #52
feat(conversions): offline conversion uploads + #43 fold (split 3/4 of #34)#52illia-sapryga wants to merge 3 commits into
Conversation
Split (b) of kLOsk#34, rebuilt fresh on main — create/update/remove conversion actions only (uploads are split c). Registers only its own three tools. - current_config() resolution; the three ops wired into the _execute_plan dispatch, matching main's lazy-import style. - Preserves the immutable/invalid-field handling the maintainer flagged as correct: include_in_conversions_metric is immutable on create; the view-through window is AD_CALL-only; auto-managed types are warned. - Fix: a positive default_value with always_use_default_value=False no longer silently flips the flag to True. It now emits a preview warning and leaves the flag exactly as the caller set it (fallback vs override is the caller's decision, not a silent override). The GAQL apostrophe-escaping fix lands in split (c), where the only by-name conversion-action lookup (the upload resolvers) actually lives. Claude-Session: https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s
Adds two write tools to the conversion_actions module and wires them into the safety flow: * draft_upload_call_conversions — UploadCallConversions. caller_id (E.164) is required raw by Google for call-to-click matching and cannot be hashed; it is stored in the plan (apply needs it) but REDACTED in the preview and audit log. * draft_upload_enhanced_conversions_for_leads — UploadClickConversions with user_identifiers. Email/phone/name are normalized then SHA-256 hashed AT PREVIEW TIME; only hashes land in the plan and the audit log never sees raw PII. Security + correctness properties: - Apply builds upload protos from plan.changes["rows"] (frozen at preview time) — the CSV is never re-read at apply. What you previewed is what uploads, and no raw PII is re-read. - success_count counts only result rows with a populated conversion_action (Google echoes caller_id / user_identifiers back on FAILED rows, so the old identifier-based heuristic always reported ~100%). Applied to BOTH call and EC-for-leads paths. - order_id dedup key: optional "Order ID" CSV column -> ClickConversion .order_id; dedup_warnings[] surface missing/partial coverage. - consent param -> ClickConversion/CallConversion.consent (ad_user_data + ad_personalization mapped to ConsentStatus; default UNSPECIFIED). EEA GDPR requirement. - _normalize_phone_e164: strip exactly one domestic trunk 0, keep Italy's +39 leading 0, map leading "00" -> "+". - _gaql_escape: backslash escaping (GAQL, not SQL doubled-quotes) for conversion-action name lookups; handles O'Brien-style names. Audit path: confirm_and_apply now routes plan.changes through _redact_changes_for_audit before every log_mutation call (dry-run, error, success) and in the dry-run response payload. Claude-Session: https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s
Covers the two upload tools end to end with fake PII only (user@example.com, +15555550142, Test User, ORD-001), asserting on the pinned SHA-256 hex of those known fakes: - PII is normalized THEN hashed; parser leaks no raw-PII keys. - _normalize_phone_e164 trunk-zero / Italy / "00" cases. - _gaql_escape backslash escaping, used by both name resolvers. - Call preview redacts caller_id in sample rows but keeps it raw in the frozen plan rows; no csv_path persisted. - EC plan.changes contains only hashes — no raw email/phone/name. - Apply builds protos from plan rows (no CSV), sets order_id + consent. - success_count keys off conversion_action (not echoed identifiers) for both call and EC paths (0% / partial / full match). - _redact_changes_for_audit redacts caller_id and drops the EC hash blob without mutating the original changes dict. - Both tools registered; upload ops wired into _execute_plan dispatch. Claude-Session: https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s
bbd7f07 to
19b24c9ComparekLOsk
commented
Sep 8, 2026
Reviewed at last — sorry for the delay, and this is the most carefully built of the four. Verified against the diff:
Holding this one briefly, for a reason on my side rather than yours. AdLoop Cloud deploys by cloning So merging this obliges a DPA update and a 30-day notice to customers before it can ship hosted. That is entirely our problem, caused by our deployment shape, not by anything in your code. It happens that we owe customers a terms notice anyway, so the fix is to fold the data-category change into that same notice rather than send a second one. Concretely: #51 is merged, #52 stays open for about a week while that goes out, then it merges unchanged. Nothing for you to do, and nothing here needs rewriting. Worth saying plainly: the fact that this is the PR with a legal consequence attached is a compliment to it. You built the hashing, the redaction and the consent handling carefully enough that the remaining question is about where our database sits, not about whether the data is handled properly. |
Split 3/4 of #34 — conversion uploads (folds in #43)
Third of the split. Stacks on #51 (it extends the same
conversion_actions.pymodule), so please merge after #51. Registers only its own two tools.Tools
draft_upload_call_conversions→UploadCallConversionsdraft_upload_enhanced_conversions_for_leads→UploadClickConversionswithuser_identifiersYour six requirements — each implemented and verified
strip().lower(), phone → E.164) then SHA-256-hashes each field. The raw values are locals used only to compute the hash — the row that enters the plan holds only*_sha256keys.plan.changes/ audit log. EC rows are hash-only. Call rows must keep the raw E.164caller_id(Google requires it unhashed for call matching), so it lives in the plan for apply but is redacted at every audit boundary:_redact_changes_for_auditis applied at all threelog_mutationcall sites (dry-run/error/success) and in the dry-run response — callcaller_id→+155***0142, and the EC hash blob is dropped from the audit trail entirely (only non-PII counters remain)._apply_*start fromchanges["rows"]; the CSV path is never stored in the plan.consent={"ad_user_data","ad_personalization"}validated and mapped toConsentStatusEnumonClickConversion.consent/CallConversion.consent; absent → UNSPECIFIED._normalize_phone_e164fixed — strips exactly one domestic trunk zero (not all), keeps Italy's leading zero (via the+-prefixed verbatim path), and maps00…→+…._gaql_escapeapplied in both_resolve_conversion_action_idsand_resolve_upload_clicks_action(soO'Brien Leaddoesn't break the query). This is the fix you flagged in your feat(ads): asset extension tools + RSA update + conversion-action mgmt #34 notes; it lands here where the only by-name lookup lives.#43 folded in (per your instruction)
ClickConversion.order_id, backwards-compatible).conversion_actionpopulated — neutralizes thecaller_id/user_identifiersecho-back that made the base always report ~100%. Applied to both call and EC paths.rows_with_order_idandsample_rows[].order_id.Things I want your call on (flagging honestly)
caller_idas a per-row failure. For EC (hashed), a bare national number with no country code would hash to a value Google can't match — silently. The normalizer is conservative (only adds+when it can infer one) and documented; recommend the EC CSV require international/E.164 phones. Happy to add a per-row warning/skip if you'd prefer._normalize_phone_e164and_gaql_escapenow exist in bothwrite.py(from split (a)) andconversion_actions.py(here). They have different signatures — worth consolidating to one shared helper when the splits land.caller_id. Audit redaction covers the log, but the persistent plan store (hostedset_plan_store) would hold raw phone between draft and apply. If that store persists to disk/DB, it's a raw-PII-at-rest surface the audit redaction doesn't cover — flagging for the hosted deployment.Tests & hygiene
user@example.com,+15555550142,Test User.Next: (d) RSA update, folding #42 with the corrected learning-reset warning.
https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s