diff --git a/src/adloop/ads/conversion_actions.py b/src/adloop/ads/conversion_actions.py index 1638399..b6d73e7 100644 --- a/src/adloop/ads/conversion_actions.py +++ b/src/adloop/ads/conversion_actions.py @@ -204,8 +204,9 @@ def draft_create_conversion_action( False = Secondary (records but doesn't affect bidding). include_in_conversions_metric: True (default) = appears in the "Conversions" column; False = "All conversions" only. NOTE: this is - IMMUTABLE on create — Google derives it from the category and rejects - any value set in the create mutate. To change it, use + IMMUTABLE — Google derives it from primary_for_goal and the category's + goal and rejects any value set directly. To change whether an action + counts, use primary_for_goal or category in draft_update_conversion_action after the create succeeds. click_through_window_days / view_through_window_days: attribution windows. 30/1 is the typical lead-gen pair. @@ -305,13 +306,18 @@ def draft_update_conversion_action( click_through_window_days: int = 0, view_through_window_days: int = 0, attribution_model: str = "", + category: str = "", ) -> dict: """Draft a partial UPDATE of an existing ConversionAction — returns PREVIEW. Only the parameters you pass non-empty/non-default will be sent to the API. Use this to rename, demote a Primary to Secondary, change value, - adjust the call-duration threshold, or change attribution settings. - include_in_conversions_metric IS mutable here (unlike on create). + adjust the call-duration threshold, change attribution settings, or move + the action to another goal (category; the preview shows the current one). + + include_in_conversions_metric is refused: Google answers IMMUTABLE_FIELD + at apply, and validate_only does not catch it. It derives the value from + primary_for_goal and whether the goal of the action's category is biddable. conversion_action_id: numeric ID. Find via: SELECT conversion_action.id, conversion_action.name FROM conversion_action @@ -341,9 +347,44 @@ def draft_update_conversion_action( view_through_window_days=view_through_window_days, attribution_model=attribution_model, ) + if include_in_conversions_metric is not None: + errors.append( + "include_in_conversions_metric cannot be changed: Google answers " + "IMMUTABLE_FIELD at apply, and a dry run does not catch it. Whether an " + "action counts in Conversions follows primary_for_goal and whether the " + "goal of its category is biddable: set primary_for_goal, or move it to " + "a biddable goal with category." + ) + category = category.strip().upper() + if category and category not in _VALID_CATEGORIES: + errors.append( + f"category '{category}' invalid; valid: {sorted(_VALID_CATEGORIES)}" + ) + if category and not str(conversion_action_id).strip().isdigit(): + errors.append( + f"conversion_action_id must be numeric, got '{conversion_action_id}'" + ) if errors: return {"error": "Validation failed", "details": errors} + warnings: list[str] = [] + current_category = "" + if category: + current = _current_conversion_action(config, customer_id, conversion_action_id) + if current is None: + return {"error": f"Conversion action {conversion_action_id} was not found in this account"} + if current["status"] == "REMOVED": + return {"error": f"Conversion action {conversion_action_id} is removed"} + if current["category"] == category: + return {"error": f"Conversion action {conversion_action_id} is already in category {category}"} + current_category = current["category"] + warnings.append( + f"Moves '{current['name']}' from the {current_category} goal to the {category} " + "goal. It counts in Conversions only if it is primary and the new goal is " + "biddable, so campaigns bidding on these goals can start or stop optimizing " + "toward it, which restarts Smart Bidding's learning period." + ) + # Track which fields the caller actually wants to update so we build # the right field_mask at apply time. changes: dict = {"conversion_action_id": str(conversion_action_id)} @@ -361,10 +402,9 @@ def draft_update_conversion_action( changes["counting_type"] = counting_type if phone_call_duration_seconds: changes["phone_call_duration_seconds"] = int(phone_call_duration_seconds) - if include_in_conversions_metric is not None: - changes["include_in_conversions_metric"] = bool( - include_in_conversions_metric - ) + if category: + changes["current_category"] = current_category + changes["category"] = category if click_through_window_days: changes["click_through_window_days"] = int(click_through_window_days) if view_through_window_days: @@ -375,7 +415,6 @@ def draft_update_conversion_action( if len(changes) == 1: # only conversion_action_id return {"error": "No fields to update"} - warnings: list[str] = [] # Same fallback-vs-override nuance as create: on update, a positive # default_value with always_use_default_value explicitly set to False is # legal (fallback). Warn rather than silently overriding intent. @@ -404,6 +443,24 @@ def draft_update_conversion_action( return preview +def _current_conversion_action( + config: AdLoopConfig, customer_id: str, conversion_action_id: str +) -> dict | None: + """Name, category and status of a conversion action, or None if not found.""" + from adloop.ads.gaql import execute_query + + rows = execute_query(config, customer_id, ( + "SELECT conversion_action.name, conversion_action.category, conversion_action.status " + f"FROM conversion_action WHERE conversion_action.id = {str(conversion_action_id).strip()} LIMIT 1")) + if not rows: + return None + return { + "name": rows[0].get("conversion_action.name") or "", + "category": str(rows[0].get("conversion_action.category") or ""), + "status": str(rows[0].get("conversion_action.status") or ""), + } + + def draft_remove_conversion_action( config: AdLoopConfig, *, @@ -472,8 +529,8 @@ def _apply_create_conversion_action(client: object, cid: str, changes: dict) -> ca.primary_for_goal = changes["primary_for_goal"] # NOTE: include_in_conversions_metric is IMMUTABLE on create — Google # derives it from the conversion category and rejects any value set in - # the create mutate (IMMUTABLE_FIELD). To change it, use - # draft_update_conversion_action after the create succeeds. + # the create mutate (IMMUTABLE_FIELD). draft_update_conversion_action + # changes what it derives from: primary_for_goal and category. if changes.get("phone_call_duration_seconds"): ca.phone_call_duration_seconds = changes["phone_call_duration_seconds"] if changes.get("click_through_window_days"): @@ -505,6 +562,13 @@ def _apply_update_conversion_action(client: object, cid: str, changes: dict) -> cid, changes["conversion_action_id"] ) + if "include_in_conversions_metric" in changes: + # A plan drafted before the draft refused it; Google would reject it. + raise ValueError( + "include_in_conversions_metric cannot be changed; nothing was sent. " + "Draft again with primary_for_goal or category." + ) + paths: list[str] = [] if "name" in changes: @@ -530,9 +594,11 @@ def _apply_update_conversion_action(client: object, cid: str, changes: dict) -> if "phone_call_duration_seconds" in changes: ca.phone_call_duration_seconds = changes["phone_call_duration_seconds"] paths.append("phone_call_duration_seconds") - if "include_in_conversions_metric" in changes: - ca.include_in_conversions_metric = changes["include_in_conversions_metric"] - paths.append("include_in_conversions_metric") + if "category" in changes: + ca.category = getattr( + client.enums.ConversionActionCategoryEnum, changes["category"] + ) + paths.append("category") if "click_through_window_days" in changes: ca.click_through_lookback_window_days = changes["click_through_window_days"] paths.append("click_through_lookback_window_days") diff --git a/src/adloop/server.py b/src/adloop/server.py index 9d759aa..e2bfde7 100644 --- a/src/adloop/server.py +++ b/src/adloop/server.py @@ -6820,9 +6820,8 @@ def draft_create_conversion_action( category). 0 leaves Google's default. primary_for_goal: True (default) makes it a Primary action that Smart Bidding optimizes toward; False makes it Secondary (recorded only). - include_in_conversions_metric: IMMUTABLE on create — Google derives it - from the category, so this value is not sent; change it later via - draft_update_conversion_action. + include_in_conversions_metric: IMMUTABLE — Google derives it from + primary_for_goal and the category's goal, so this value is not sent. click_through_window_days: Click-through attribution window in days, 1-90. 0 leaves Google's default. view_through_window_days: View-through attribution window in days, @@ -6871,12 +6870,14 @@ def draft_update_conversion_action( view_through_window_days: int = 0, attribution_model: str = "", customer_id: str = "", + category: str = "", ) -> dict: """Draft a partial UPDATE of an existing ConversionAction — returns PREVIEW. Only parameters passed non-empty/non-default are sent to the API. Covers renaming, demoting a Primary to Secondary, changing value, - adjusting the call-duration threshold, or changing attribution. + adjusting the call-duration threshold, changing attribution, or moving + the action to another goal with category. SMART_CAMPAIGN_* and GOOGLE_HOSTED types reject mutations with MUTATE_NOT_ALLOWED at apply time. Returns a preview with a plan_id (plus warnings where relevant); nothing changes until confirm_and_apply applies @@ -6901,9 +6902,10 @@ def draft_update_conversion_action( unchanged. phone_call_duration_seconds: Minimum call length in seconds for a call to count. 0 leaves it unchanged. - include_in_conversions_metric: True shows the action in the - "Conversions" column; False in "All conversions" only. Mutable here - (unlike on create). Omit to leave unchanged. + include_in_conversions_metric: Refused when passed: Google answers + IMMUTABLE_FIELD at apply, and a dry run does not catch it. It derives + the value from primary_for_goal and whether the goal of the action's + category is biddable; use those instead. click_through_window_days: Click-through attribution window in days, 1-90. 0 leaves it unchanged. view_through_window_days: View-through attribution window in days, @@ -6912,6 +6914,9 @@ def draft_update_conversion_action( GOOGLE_SEARCH_ATTRIBUTION_DATA_DRIVEN. Empty leaves it unchanged. customer_id: Google Ads customer ID (digits, dashes allowed). Defaults to the configured account. + category: ConversionActionCategory to move the action to, e.g. + PHONE_CALL_LEAD; that decides which goal it belongs to. The preview + shows the current one (current_category). Empty leaves it unchanged. """ from adloop.ads.conversion_actions import ( draft_update_conversion_action as _impl, @@ -6932,6 +6937,7 @@ def draft_update_conversion_action( click_through_window_days=click_through_window_days, view_through_window_days=view_through_window_days, attribution_model=attribution_model, + category=category, ) diff --git a/tests/test_conversion_actions.py b/tests/test_conversion_actions.py index 138c665..0cb87f6 100644 --- a/tests/test_conversion_actions.py +++ b/tests/test_conversion_actions.py @@ -309,17 +309,20 @@ def test_phone_duration_persisted(self, config): plan = _stored_plan(result) assert plan.changes["phone_call_duration_seconds"] == 90 - def test_include_in_conversions_metric_is_mutable_on_update(self, config): - """Unlike create (where it's IMMUTABLE), the update path accepts - include_in_conversions_metric and passes it through.""" + @pytest.mark.parametrize("value", [True, False]) + def test_include_in_conversions_metric_is_refused(self, config, value): + """Google answers IMMUTABLE_FIELD at apply; validate_only misses it.""" result = conversion_actions.draft_update_conversion_action( config, customer_id="1", conversion_action_id="6797442210", - include_in_conversions_metric=False, + name="Also a real change", + include_in_conversions_metric=value, ) - plan = _stored_plan(result) - assert plan.changes["include_in_conversions_metric"] is False + assert result["error"] == "Validation failed" + details = " ".join(result["details"]) + assert "IMMUTABLE_FIELD" in details + assert "primary_for_goal" in details and "category" in details def test_fallback_flag_warns_on_update(self, config): """Fix #2 also applies on update: positive default_value with the @@ -507,25 +510,20 @@ def test_update_only_phone_duration(self): mask_paths = list(op.update_mask.paths) assert mask_paths == ["phone_call_duration_seconds"] - def test_update_include_in_conversions_metric_fieldmask(self): - ca_svc = _FakeConversionActionService( - [_FakeResult("customers/1/conversionActions/6797442210")] - ) + def test_a_stored_plan_with_include_in_conversions_metric_sends_nothing(self): + ca_svc = _FakeConversionActionService([]) client = _FakeClient({"ConversionActionService": ca_svc}) - conversion_actions._apply_update_conversion_action( - client, - "1", - { - "conversion_action_id": "6797442210", - "include_in_conversions_metric": False, - }, - ) - - op = ca_svc.operations[0] - ca = op.update - assert ca.include_in_conversions_metric is False - assert list(op.update_mask.paths) == ["include_in_conversions_metric"] + with pytest.raises(ValueError, match="cannot be changed"): + conversion_actions._apply_update_conversion_action( + client, + "1", + { + "conversion_action_id": "6797442210", + "include_in_conversions_metric": False, + }, + ) + assert ca_svc.operations == [] class TestApplyRemoveConversionAction: @@ -4899,3 +4897,91 @@ def test_enhanced_conversions_draft_from_an_upload(self, config, store, monkeypa assert result["status"] == "PENDING_CONFIRMATION" assert store.list("", kind="csv") == [] assert "jane@example.com" not in str(_stored_plan(result).apply_only_payload) + + +# --------------------------------------------------------------------------- +# draft_update_conversion_action: category +# --------------------------------------------------------------------------- + + +class _RecordingService: + def __init__(self, sent: list): + self._sent = sent + + def conversion_action_path(self, customer_id: str, ca_id: str) -> str: + return f"customers/{customer_id}/conversionActions/{ca_id}" + + def mutate_conversion_actions(self, request=None, **_kwargs): + self._sent.append(request) + return SimpleNamespace(results=[], partial_failure_error=None) + + +class TestUpdateCategory: + @pytest.fixture + def current(self, monkeypatch): + row = { + "conversion_action.name": "Calls", + "conversion_action.category": "DEFAULT", + "conversion_action.status": "ENABLED", + } + monkeypatch.setattr("adloop.ads.gaql.execute_query", lambda *_a, **_k: [row]) + return row + + def _draft(self, config, category="phone_call_lead", conversion_action_id="6797442210"): + return conversion_actions.draft_update_conversion_action( + config, customer_id="1", conversion_action_id=conversion_action_id, category=category + ) + + def test_preview_shows_the_current_category_and_warns(self, config, current): + preview = self._draft(config) + assert preview["changes"]["current_category"] == "DEFAULT" + assert preview["changes"]["category"] == "PHONE_CALL_LEAD" + assert "'Calls' from the DEFAULT goal to the PHONE_CALL_LEAD goal" in preview["warnings"][0] + + def test_dry_run_sends_only_the_category_validate_only(self, config, current, monkeypatch): + sent: list = [] + client = GoogleAdsClient(credentials=None, developer_token="t", use_proto_plus=True, + version=GOOGLE_ADS_API_VERSION) + fake = SimpleNamespace(enums=client.enums, get_type=client.get_type, + get_service=lambda _name, *_a, **_k: _RecordingService(sent)) + monkeypatch.setattr("adloop.ads.client.get_ads_client", lambda _cfg: fake) + monkeypatch.setattr( + write, "_validate_with_google", + lambda cfg, plan: write._execute_plan(cfg, plan, validate_only=True), + ) + + result = write.confirm_and_apply(config, plan_id=self._draft(config)["plan_id"], dry_run=True) + + assert result["status"] == "DRY_RUN_SUCCESS", result + [request] = sent + assert request.validate_only is True + [operation] = request.operations + assert operation.update.category == client.enums.ConversionActionCategoryEnum.PHONE_CALL_LEAD + assert list(operation.update_mask.paths) == ["category"] + + def test_invalid_category_is_refused_before_any_read(self, config): + # no_live_gaql from conftest fails this test if the draft queries anything. + result = self._draft(config, category="NOT_A_CATEGORY") + assert any("category 'NOT_A_CATEGORY' invalid" in d for d in result["details"]) + + def test_non_numeric_id_is_refused_before_any_read(self, config): + result = self._draft(config, conversion_action_id="1 OR 1=1") + assert any("must be numeric" in d for d in result["details"]) + + def test_the_same_category_is_nothing_to_change(self, config, current): + assert "already in category" in self._draft(config, category="DEFAULT")["error"] + + def test_unknown_and_removed_actions_are_refused(self, config, current, monkeypatch): + current["conversion_action.status"] = "REMOVED" + assert "is removed" in self._draft(config)["error"] + monkeypatch.setattr("adloop.ads.gaql.execute_query", lambda *_a, **_k: []) + assert "was not found" in self._draft(config)["error"] + + @pytest.mark.asyncio + async def test_the_mcp_tool_takes_category(self): + from adloop.server import mcp + + tools = {tool.name: tool for tool in await mcp.list_tools()} + properties = tools["draft_update_conversion_action"].parameters["properties"] + assert properties["category"].get("description") + assert "IMMUTABLE_FIELD" in properties["include_in_conversions_metric"]["description"]