Skip to content

feat(conversion-action): CRUD tools (split 2/4 of #34)#51

Open
illia-sapryga wants to merge 1 commit into
kLOsk:mainfrom
illia-sapryga:feat/conversion-action-crud
Open

feat(conversion-action): CRUD tools (split 2/4 of #34)#51
illia-sapryga wants to merge 1 commit into
kLOsk:mainfrom
illia-sapryga:feat/conversion-action-crud

Conversation

@illia-sapryga

Copy link
Copy Markdown
Contributor

Split 2/4 of #34 — conversion-action CRUD

Second of the four-way split, rebuilt fresh on main, registering only its own three tools (no uploads — those are split (c) — no assets/RSA).

Tools

  • draft_create_conversion_action
  • draft_update_conversion_action
  • draft_remove_conversion_action

Wrappers resolve config via current_config(); impls take config first; the three ops are wired into the _execute_plan dispatch with lazy imports (matching main's style). New src/adloop/ads/conversion_actions.py holds only the CRUD functions — split (c) will add the upload functions to it.

Your fix: default_value>0 no longer silently flips the flag

A positive default_value paired with always_use_default_value=False used to silently force the flag to True — turning a legal "tag value with fallback" config into "always override." It now emits a preview warning and leaves the flag exactly as the caller set it:

"default_value is set but always_use_default_value is False: Google will treat default_value as a FALLBACK, used only when the tag/import provides no value. If you want default_value to override every conversion's value, set always_use_default_value=True explicitly."

Applied to both create and update. Tests assert the warning fires and the flag is not force-set; default_value=0 / explicit True produce no warning.

Preserved (your "reviewed clean" items)

  • include_in_conversions_metric is immutable on create — the create op leaves the proto field unset (regression test asserts not HasField(...)); a test confirms it IS mutable on update.
  • view_through_lookback_window is settable only for AD_CALL.
  • Auto-managed types (SMART_CAMPAIGN_*, GOOGLE_HOSTED) are warned.

On your GAQL-escaping note

The apostrophe-doubling bug is in the by-name conversion-action lookup, which only exists in the upload resolvers — so the backslash-escaping fix lands in split (c) alongside that code, rather than as a dead helper here. Flagging so you know it isn't dropped.

Tests & hygiene

  • 450 passing, warning-free.
  • PII scrubbed — generic names (Example Co), no real client data.

Next: (c) conversion uploads (folds #43 + the GAQL fix), then (d) RSA update (folds #42).

https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant