draft_update_conversion_action: add category, refuse include_in_conversions_metric

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")

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,
     )
 
 

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"]