diff --git a/.claude/rules/adloop.md b/.claude/rules/adloop.md index f4becb7..3db768a 100644 --- a/.claude/rules/adloop.md +++ b/.claude/rules/adloop.md @@ -73,6 +73,7 @@ These tools call both APIs internally and return unified results with computed ` | Tool | What It Does | Validation | |------|-------------|------------| | `draft_campaign` | Create full campaign structure (budget + campaign + ad group + keywords + geo/language targeting) | `campaign_name`, `daily_budget`, `bidding_strategy`, `geo_target_ids` (REQUIRED), `language_ids` (REQUIRED), keywords validated | +| `draft_ad_group` | Create a new ad group within an existing campaign (does NOT publish) | `campaign_id` (REQUIRED), `ad_group_name` (REQUIRED), `keywords` (optional list of {text, match_type}), `cpc_bid_micros` (optional) | | `update_campaign` | Modify existing campaign settings — bid strategy, budget, geo targets, language targets | `campaign_id` (REQUIRED), plus any of: `bidding_strategy`, `daily_budget`, `geo_target_ids`, `language_ids` | | `draft_responsive_search_ad` | Create RSA preview (does NOT publish) | 3-15 headlines (≤30 chars), 2-4 descriptions (≤90 chars), final_url required, path1/path2 (≤15 chars each) | | `draft_sitelinks` | Create sitelink extensions for a campaign (does NOT publish) | `campaign_id`, `sitelinks` list of {link_text ≤25 chars, final_url, description1 ≤35 chars, description2 ≤35 chars} | @@ -204,9 +205,22 @@ Most websites (especially in the EU) use a GDPR cookie consent banner. This has 7. After campaign creation, remind the user to: - Add ads via `draft_responsive_search_ad` (with display paths set) - Add sitelinks via `draft_sitelinks` (at least 4 recommended) + - If the user needs multiple ad groups (e.g., different keyword themes), use `draft_ad_group` to add additional ad groups after the initial campaign is created and confirmed - Enable the campaign via `enable_entity` only after ads and sitelinks are in place 8. Wait for explicit user approval before calling `confirm_and_apply` +### When user wants to add an ad group to an existing campaign + +1. Call `get_campaign_performance` to identify the target campaign and verify it exists +2. **Pre-write checks (CRITICAL):** + - Check the campaign's bidding strategy — if MANUAL_CPC, only use EXACT or PHRASE match keywords + - Check if conversion tracking is active (zero conversions + high spend = problem to fix first) + - Check existing ad groups via `run_gaql`: `SELECT ad_group.id, ad_group.name FROM ad_group WHERE campaign.id = {campaign_id}` — avoid duplicate ad group names +3. Call `draft_ad_group` with `campaign_id`, `ad_group_name`, and optional `keywords` +4. Present the complete preview to the user +5. Wait for explicit user approval before calling `confirm_and_apply` +6. After the ad group is created, remind the user to add RSAs via `draft_responsive_search_ad` using the new `ad_group_id` from the result — an ad group without ads won't serve + ### When user wants to change campaign settings (bid strategy, targeting, budget) 1. Call `get_campaign_performance` to identify the campaign and its current settings diff --git a/.cursor/rules/adloop.mdc b/.cursor/rules/adloop.mdc index 41944f3..0334493 100644 --- a/.cursor/rules/adloop.mdc +++ b/.cursor/rules/adloop.mdc @@ -78,6 +78,7 @@ These tools call both APIs internally and return unified results with computed ` | Tool | What It Does | Validation | |------|-------------|------------| | `draft_campaign` | Create full campaign structure (budget + campaign + ad group + keywords + geo/language targeting) | `campaign_name`, `daily_budget`, `bidding_strategy`, `geo_target_ids` (REQUIRED), `language_ids` (REQUIRED), keywords validated | +| `draft_ad_group` | Create a new ad group within an existing campaign (does NOT publish) | `campaign_id` (REQUIRED), `ad_group_name` (REQUIRED), `keywords` (optional list of {text, match_type}), `cpc_bid_micros` (optional) | | `update_campaign` | Modify existing campaign settings — bid strategy, budget, geo targets, language targets | `campaign_id` (REQUIRED), plus any of: `bidding_strategy`, `daily_budget`, `geo_target_ids`, `language_ids` | | `draft_responsive_search_ad` | Create RSA preview (does NOT publish) | 3-15 headlines (≤30 chars), 2-4 descriptions (≤90 chars), final_url required, path1/path2 (≤15 chars each) | | `draft_sitelinks` | Create sitelink extensions for a campaign (does NOT publish) | `campaign_id`, `sitelinks` list of {link_text ≤25 chars, final_url, description1 ≤35 chars, description2 ≤35 chars} | @@ -209,9 +210,22 @@ Most websites (especially in the EU) use a GDPR cookie consent banner. This has 7. After campaign creation, remind the user to: - Add ads via `draft_responsive_search_ad` (with display paths set) - Add sitelinks via `draft_sitelinks` (at least 4 recommended) + - If the user needs multiple ad groups (e.g., different keyword themes), use `draft_ad_group` to add additional ad groups after the initial campaign is created and confirmed - Enable the campaign via `enable_entity` only after ads and sitelinks are in place 8. Wait for explicit user approval before calling `confirm_and_apply` +### When user wants to add an ad group to an existing campaign + +1. Call `get_campaign_performance` to identify the target campaign and verify it exists +2. **Pre-write checks (CRITICAL):** + - Check the campaign's bidding strategy — if MANUAL_CPC, only use EXACT or PHRASE match keywords + - Check if conversion tracking is active (zero conversions + high spend = problem to fix first) + - Check existing ad groups via `run_gaql`: `SELECT ad_group.id, ad_group.name FROM ad_group WHERE campaign.id = {campaign_id}` — avoid duplicate ad group names +3. Call `draft_ad_group` with `campaign_id`, `ad_group_name`, and optional `keywords` +4. Present the complete preview to the user +5. Wait for explicit user approval before calling `confirm_and_apply` +6. After the ad group is created, remind the user to add RSAs via `draft_responsive_search_ad` using the new `ad_group_id` from the result — an ad group without ads won't serve + ### When user wants to change campaign settings (bid strategy, targeting, budget) 1. Call `get_campaign_performance` to identify the campaign and its current settings diff --git a/CLAUDE.md b/CLAUDE.md index 61c27fe..2e041e0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,7 @@ python scripts/sync-rules.py # Sync rules: .cursor/rules/ -> .claude/rules/ ``` src/adloop/ ├── __init__.py # Entry point — routes 'adloop init' vs MCP server -├── server.py # FastMCP server — 26 tool registrations +├── server.py # FastMCP server — 29 tool registrations ├── config.py # Config loader (~/.adloop/config.yaml) ├── auth.py # OAuth 2.0 + service account + token refresh ├── cli.py # Interactive setup wizard @@ -34,7 +34,7 @@ All tool usage rules, safety protocols, orchestration patterns, GAQL reference, **Read and follow `.claude/rules/adloop.md` for all AdLoop MCP tool orchestration.** -That file is the complete guide for combining AdLoop's 26 tools. It covers: +That file is the complete guide for combining AdLoop's 29 tools. It covers: - Tool inventory with parameters and when to use each - 8 safety rules (budget caps, dry-run defaults, Broad Match prevention, pre-write validation) - 12 orchestration patterns (performance review, ad creation, tracking diagnosis, etc.) diff --git a/src/adloop/ads/write.py b/src/adloop/ads/write.py index d7646d0..f60af5c 100644 --- a/src/adloop/ads/write.py +++ b/src/adloop/ads/write.py @@ -372,6 +372,66 @@ def draft_campaign( return preview +def draft_ad_group( + config: AdLoopConfig, + *, + customer_id: str = "", + campaign_id: str = "", + ad_group_name: str = "", + keywords: list[dict] | None = None, + cpc_bid_micros: int = 0, +) -> dict: + """Draft a new ad group within an existing campaign — returns preview. + + Creates: AdGroup (ENABLED, SEARCH_STANDARD) + optional Keywords. + Ads are NOT included — use draft_responsive_search_ad separately + after the ad group is created. + + cpc_bid_micros: Optional ad-group-level CPC bid in micros. Only relevant + for campaigns using MANUAL_CPC bidding. + """ + from adloop.safety.guards import SafetyViolation, check_blocked_operation + from adloop.safety.preview import ChangePlan, store_plan + + try: + check_blocked_operation("create_ad_group", config.safety) + except SafetyViolation as e: + return {"error": str(e)} + + errors = _validate_ad_group( + campaign_id=campaign_id, + ad_group_name=ad_group_name, + keywords=keywords, + cpc_bid_micros=cpc_bid_micros, + ) + if errors: + return {"error": "Validation failed", "details": errors} + + keywords = keywords or [] + preflight_errors, warnings = _preflight_ad_group_checks( + config, customer_id, campaign_id, ad_group_name, keywords, cpc_bid_micros + ) + if preflight_errors: + return {"error": "Pre-flight check failed", "details": preflight_errors} + + plan = ChangePlan( + operation="create_ad_group", + entity_type="ad_group", + customer_id=customer_id, + changes={ + "campaign_id": campaign_id, + "ad_group_name": ad_group_name, + "keywords": keywords, + "cpc_bid_micros": cpc_bid_micros, + }, + ) + store_plan(plan) + preview = plan.to_preview() + if warnings: + preview["warnings"] = warnings + return preview + + def update_campaign( config: AdLoopConfig, *, @@ -698,7 +758,7 @@ def _check_broad_match_safety( ) -> list[str]: """Warn if BROAD match keywords are being added to a non-Smart Bidding campaign.""" has_broad = any( - kw.get("match_type", "").upper() == "BROAD" for kw in keywords + (kw.get("match_type") or "").upper() == "BROAD" for kw in keywords ) if not has_broad: return [] @@ -825,7 +885,7 @@ def _validate_campaign( if keywords: has_broad = any( - kw.get("match_type", "").upper() == "BROAD" for kw in keywords + (kw.get("match_type") or "").upper() == "BROAD" for kw in keywords ) if has_broad and bs not in _SMART_BIDDING_STRATEGIES: errors.append( @@ -837,7 +897,7 @@ def _validate_campaign( for i, kw in enumerate(keywords): if not kw.get("text"): errors.append(f"Keyword {i + 1} has no text") - mt = kw.get("match_type", "").upper() + mt = (kw.get("match_type") or "").upper() if mt not in _VALID_MATCH_TYPES: errors.append( f"Keyword {i + 1} has invalid match_type '{mt}' " @@ -869,7 +929,7 @@ def _validate_keywords(ad_group_id: str, keywords: list[dict]) -> list[str]: for i, kw in enumerate(keywords): if not kw.get("text"): errors.append(f"Keyword {i + 1} has no text") - mt = kw.get("match_type", "").upper() + mt = (kw.get("match_type") or "").upper() if mt not in _VALID_MATCH_TYPES: errors.append( f"Keyword {i + 1} has invalid match_type '{mt}' " @@ -878,6 +938,135 @@ def _validate_keywords(ad_group_id: str, keywords: list[dict]) -> list[str]: return errors +def _validate_ad_group( + *, + campaign_id: str, + ad_group_name: str, + keywords: list[dict] | None, + cpc_bid_micros: int, +) -> list[str]: + """Validate inputs for draft_ad_group.""" + errors = [] + if not campaign_id: + errors.append("campaign_id is required") + if not ad_group_name or not ad_group_name.strip(): + errors.append("ad_group_name is required") + if cpc_bid_micros < 0: + errors.append("cpc_bid_micros must be >= 0") + if keywords: + for i, kw in enumerate(keywords): + if not kw.get("text"): + errors.append(f"Keyword {i + 1} has no text") + mt = (kw.get("match_type") or "").upper() + if mt not in _VALID_MATCH_TYPES: + errors.append( + f"Keyword {i + 1} has invalid match_type '{mt}' " + "(must be EXACT, PHRASE, or BROAD)" + ) + return errors + + +def _preflight_ad_group_checks( + config: AdLoopConfig, + customer_id: str, + campaign_id: str, + ad_group_name: str, + keywords: list[dict], + cpc_bid_micros: int, +) -> tuple[list[str], list[str]]: + """Run pre-flight checks before creating an ad group. + + Returns (errors, warnings). Errors block the draft; warnings are informational. + + Checks performed: + 1. Campaign must be a SEARCH campaign (error if not). + 2. Warn if cpc_bid_micros is set but campaign uses Smart Bidding (ignored). + 3. Warn if BROAD match keywords + non-Smart Bidding campaign. + 4. Warn if an ad group with the same name already exists in the campaign. + """ + errors: list[str] = [] + warnings: list[str] = [] + + try: + from adloop.ads.gaql import execute_query + + # Query 1: campaign info (type, bidding, name) + campaign_query = f""" + SELECT campaign.advertising_channel_type, + campaign.bidding_strategy_type, + campaign.name + FROM campaign + WHERE campaign.id = {campaign_id} + """ + rows = execute_query(config, customer_id, campaign_query) + if not rows: + errors.append( + f"Campaign {campaign_id} not found. Verify the campaign ID " + "using get_campaign_performance." + ) + return errors, warnings + + row = rows[0] + channel_type = row.get("campaign.advertising_channel_type", "") + bidding = row.get("campaign.bidding_strategy_type", "") + campaign_name = row.get("campaign.name", "") + + # Check 1: campaign type must be SEARCH + if channel_type and channel_type != "SEARCH": + errors.append( + f"Campaign '{campaign_name}' is a {channel_type} campaign. " + "draft_ad_group only supports SEARCH campaigns." + ) + + # Check 2: cpc_bid_micros on Smart Bidding is ignored + if cpc_bid_micros and bidding in _SMART_BIDDING_STRATEGIES: + warnings.append( + f"Campaign '{campaign_name}' uses {bidding} (Smart Bidding). " + "The cpc_bid_micros value will be ignored — Smart Bidding " + "sets bids automatically." + ) + + # Check 3: BROAD match + non-Smart Bidding + has_broad = any( + (kw.get("match_type") or "").upper() == "BROAD" for kw in keywords + ) + if has_broad and bidding not in _SMART_BIDDING_STRATEGIES: + warnings.append( + f"DANGEROUS: Adding BROAD match keywords to campaign " + f"'{campaign_name}' which uses {bidding} bidding. " + f"Broad Match without Smart Bidding (tCPA/tROAS/Maximize " + f"Conversions) leads to irrelevant matches and wasted budget. " + f"Use PHRASE or EXACT match instead, or switch the campaign " + f"to Smart Bidding first." + ) + + # Check 4: existing ad groups (duplicate name check) + ag_query = f""" + SELECT ad_group.name + FROM ad_group + WHERE campaign.id = {campaign_id} + """ + ag_rows = execute_query(config, customer_id, ag_query) + existing_names = {r.get("ad_group.name", "") for r in ag_rows} + if ad_group_name in existing_names: + warnings.append( + f"An ad group named '{ad_group_name}' already exists in " + f"campaign '{campaign_name}'. This will create a duplicate. " + f"Consider using a different name to avoid confusion." + ) + + except Exception as exc: + # Surface preflight failures as warnings so users know checks + # were skipped, rather than silently producing a clean preview. + warnings.append( + f"Preflight checks could not complete ({exc}). " + "The draft will proceed, but some validations were skipped. " + "Full validation happens at confirm_and_apply time." + ) + + return errors, warnings + + def _draft_status_change( config: AdLoopConfig, operation: str, @@ -929,6 +1118,7 @@ def _execute_plan(config: AdLoopConfig, plan: object) -> dict: dispatch = { "create_campaign": _apply_create_campaign, + "create_ad_group": _apply_create_ad_group, "update_campaign": _apply_update_campaign, "create_responsive_search_ad": _apply_create_rsa, "add_keywords": _apply_add_keywords, @@ -1095,6 +1285,54 @@ def _apply_create_campaign(client: object, cid: str, changes: dict) -> dict: return results +def _apply_create_ad_group(client: object, cid: str, changes: dict) -> dict: + """Create ad group + optional keywords in an existing campaign atomically.""" + service = client.get_service("GoogleAdsService") + campaign_service = client.get_service("CampaignService") + ad_group_service = client.get_service("AdGroupService") + + operations: list = [] + + # 1. AdGroup (temp ID: -1, references existing campaign) + ag_op = client.get_type("MutateOperation") + ad_group = ag_op.ad_group_operation.create + ad_group.resource_name = ad_group_service.ad_group_path(cid, "-1") + ad_group.name = changes["ad_group_name"] + ad_group.campaign = campaign_service.campaign_path(cid, changes["campaign_id"]) + ad_group.status = client.enums.AdGroupStatusEnum.ENABLED + ad_group.type_ = client.enums.AdGroupTypeEnum.SEARCH_STANDARD + if changes.get("cpc_bid_micros"): + ad_group.cpc_bid_micros = changes["cpc_bid_micros"] + operations.append(ag_op) + + # 2. Keywords (reference ad_group -1) + kw_list = changes.get("keywords") or [] + for kw in kw_list: + kw_op = client.get_type("MutateOperation") + criterion = kw_op.ad_group_criterion_operation.create + criterion.ad_group = ad_group_service.ad_group_path(cid, "-1") + criterion.keyword.text = kw["text"] + criterion.keyword.match_type = getattr( + client.enums.KeywordMatchTypeEnum, kw["match_type"].upper() + ) + operations.append(kw_op) + + response = service.mutate(customer_id=cid, mutate_operations=operations) + + results: dict = {} + for i, resp in enumerate(response.mutate_operation_responses): + resp_type = resp.WhichOneof("response") + if resp_type: + inner = getattr(resp, resp_type) + resource = getattr(inner, "resource_name", str(inner)) + if i == 0: + results["ad_group"] = resource + else: + results.setdefault("keywords", []).append(resource) + + return results + + def _apply_update_campaign(client: object, cid: str, changes: dict) -> dict: """Update an existing campaign's settings.""" from google.protobuf import field_mask_pb2 diff --git a/src/adloop/server.py b/src/adloop/server.py index a4e118c..7f0f223 100644 --- a/src/adloop/server.py +++ b/src/adloop/server.py @@ -507,6 +507,39 @@ def draft_campaign( ) +@mcp.tool(annotations=_WRITE) +@_safe +def draft_ad_group( + campaign_id: str, + ad_group_name: str, + keywords: list[dict] | None = None, + customer_id: str = "", + cpc_bid_micros: int = 0, +) -> dict: + """Draft a new ad group within an existing campaign — returns a PREVIEW, does NOT create. + + Creates an ad group (ENABLED, type SEARCH_STANDARD) in the specified campaign. + Optionally includes keywords in the same atomic operation. + + campaign_id: The campaign to add the ad group to (get from get_campaign_performance). + ad_group_name: Name for the new ad group. + keywords: Optional list of {"text": "keyword", "match_type": "EXACT|PHRASE|BROAD"}. + cpc_bid_micros: Optional ad group CPC bid in micros (only for MANUAL_CPC campaigns). + + Call confirm_and_apply with the returned plan_id to execute. + """ + from adloop.ads.write import draft_ad_group as _impl + + return _impl( + _config, + customer_id=customer_id or _config.ads.customer_id, + campaign_id=campaign_id, + ad_group_name=ad_group_name, + keywords=keywords, + cpc_bid_micros=cpc_bid_micros, + ) + + @mcp.tool(annotations=_WRITE) @_safe def update_campaign( diff --git a/tests/test_draft_ad_group.py b/tests/test_draft_ad_group.py new file mode 100644 index 0000000..941c025 --- /dev/null +++ b/tests/test_draft_ad_group.py @@ -0,0 +1,360 @@ +"""Tests for draft_ad_group validation and plan creation.""" + +from unittest.mock import patch + +import pytest + +from adloop.ads.write import ( + _preflight_ad_group_checks, + _validate_ad_group, + draft_ad_group, +) +from adloop.config import AdLoopConfig, AdsConfig, SafetyConfig +from adloop.safety.preview import get_plan, remove_plan + + +@pytest.fixture +def config(): + return AdLoopConfig( + ads=AdsConfig(customer_id="1234567890", developer_token="test"), + safety=SafetyConfig(max_daily_budget=50.0, require_dry_run=True), + ) + + +class TestValidateAdGroup: + def test_valid_inputs(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test Ad Group", + keywords=None, + cpc_bid_micros=0, + ) + assert errors == [] + + def test_missing_campaign_id(self): + errors = _validate_ad_group( + campaign_id="", + ad_group_name="Test", + keywords=None, + cpc_bid_micros=0, + ) + assert any("campaign_id" in e for e in errors) + + def test_missing_ad_group_name(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="", + keywords=None, + cpc_bid_micros=0, + ) + assert any("ad_group_name" in e for e in errors) + + def test_whitespace_ad_group_name(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name=" ", + keywords=None, + cpc_bid_micros=0, + ) + assert any("ad_group_name" in e for e in errors) + + def test_negative_cpc_bid(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test", + keywords=None, + cpc_bid_micros=-100, + ) + assert any("cpc_bid_micros" in e for e in errors) + + def test_valid_with_keywords(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test", + keywords=[ + {"text": "buy shoes", "match_type": "EXACT"}, + {"text": "running shoes", "match_type": "PHRASE"}, + ], + cpc_bid_micros=0, + ) + assert errors == [] + + def test_keyword_missing_text(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test", + keywords=[{"text": "", "match_type": "EXACT"}], + cpc_bid_micros=0, + ) + assert any("no text" in e for e in errors) + + def test_keyword_invalid_match_type(self): + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test", + keywords=[{"text": "shoes", "match_type": "INVALID"}], + cpc_bid_micros=0, + ) + assert any("invalid match_type" in e for e in errors) + + def test_keyword_null_match_type(self): + """MCP/JSON callers can send null for match_type.""" + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test", + keywords=[{"text": "shoes", "match_type": None}], + cpc_bid_micros=0, + ) + assert any("invalid match_type" in e for e in errors) + + def test_keyword_missing_match_type_key(self): + """Keywords without a match_type key should get a validation error.""" + errors = _validate_ad_group( + campaign_id="123", + ad_group_name="Test", + keywords=[{"text": "shoes"}], + cpc_bid_micros=0, + ) + assert any("invalid match_type" in e for e in errors) + + +class TestPreflightAdGroupChecks: + """Tests for _preflight_ad_group_checks using mocked GAQL queries.""" + + def _mock_execute(self, campaign_rows, ad_group_rows=None): + """Return a side_effect function that returns different results per query.""" + ad_group_rows = ad_group_rows or [] + + def side_effect(config, customer_id, query): + if "campaign.advertising_channel_type" in query: + return campaign_rows + if "ad_group.name" in query: + return ad_group_rows + return [] + + return side_effect + + @patch("adloop.ads.gaql.execute_query") + def test_search_campaign_no_issues(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "MAXIMIZE_CONVERSIONS", + "campaign.name": "My Campaign"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], 0 + ) + assert errors == [] + assert warnings == [] + + @patch("adloop.ads.gaql.execute_query") + def test_display_campaign_rejected(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "DISPLAY", + "campaign.bidding_strategy_type": "MAXIMIZE_CONVERSIONS", + "campaign.name": "Display Campaign"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], 0 + ) + assert any("DISPLAY" in e for e in errors) + assert any("only supports SEARCH" in e for e in errors) + + @patch("adloop.ads.gaql.execute_query") + def test_shopping_campaign_rejected(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SHOPPING", + "campaign.bidding_strategy_type": "MAXIMIZE_CONVERSIONS", + "campaign.name": "Shopping Campaign"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], 0 + ) + assert any("SHOPPING" in e for e in errors) + + @patch("adloop.ads.gaql.execute_query") + def test_campaign_not_found(self, mock_query, config): + mock_query.side_effect = self._mock_execute([]) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], 0 + ) + assert any("not found" in e for e in errors) + + @patch("adloop.ads.gaql.execute_query") + def test_duplicate_ad_group_name_warns(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "MAXIMIZE_CONVERSIONS", + "campaign.name": "My Campaign"}], + [{"ad_group.name": "Existing Group"}, {"ad_group.name": "Another"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "Existing Group", [], 0 + ) + assert errors == [] + assert any("already exists" in w for w in warnings) + + @patch("adloop.ads.gaql.execute_query") + def test_no_duplicate_name_no_warning(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "MAXIMIZE_CONVERSIONS", + "campaign.name": "My Campaign"}], + [{"ad_group.name": "Other Group"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], 0 + ) + assert errors == [] + assert not any("already exists" in w for w in warnings) + + @patch("adloop.ads.gaql.execute_query") + def test_cpc_bid_on_smart_bidding_warns(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "MAXIMIZE_CONVERSIONS", + "campaign.name": "Smart Campaign"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], cpc_bid_micros=500000 + ) + assert errors == [] + assert any("ignored" in w for w in warnings) + + @patch("adloop.ads.gaql.execute_query") + def test_cpc_bid_on_manual_cpc_no_warning(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "MANUAL_CPC", + "campaign.name": "Manual Campaign"}], + ) + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], cpc_bid_micros=500000 + ) + assert errors == [] + assert not any("ignored" in w for w in warnings) + + @patch("adloop.ads.gaql.execute_query") + def test_broad_match_non_smart_bidding_warns(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "MANUAL_CPC", + "campaign.name": "Manual Campaign"}], + ) + keywords = [{"text": "shoes", "match_type": "BROAD"}] + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", keywords, 0 + ) + assert errors == [] + assert any("DANGEROUS" in w and "BROAD" in w for w in warnings) + + @patch("adloop.ads.gaql.execute_query") + def test_broad_match_smart_bidding_no_warning(self, mock_query, config): + mock_query.side_effect = self._mock_execute( + [{"campaign.advertising_channel_type": "SEARCH", + "campaign.bidding_strategy_type": "TARGET_CPA", + "campaign.name": "Smart Campaign"}], + ) + keywords = [{"text": "shoes", "match_type": "BROAD"}] + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", keywords, 0 + ) + assert errors == [] + assert not any("BROAD" in w for w in warnings) + + @patch("adloop.ads.gaql.execute_query") + def test_api_failure_surfaces_warning(self, mock_query, config): + """If API calls fail, preflight should warn but not block the draft.""" + mock_query.side_effect = Exception("API unavailable") + errors, warnings = _preflight_ad_group_checks( + config, "1234567890", "999", "New Group", [], 0 + ) + assert errors == [] + assert len(warnings) == 1 + assert "Preflight checks could not complete" in warnings[0] + assert "API unavailable" in warnings[0] + + +class TestDraftAdGroup: + def test_returns_preview_with_plan_id(self, config): + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="999", + ad_group_name="My Ad Group", + ) + assert "plan_id" in result + assert result["operation"] == "create_ad_group" + assert result["changes"]["campaign_id"] == "999" + assert result["changes"]["ad_group_name"] == "My Ad Group" + + # Clean up stored plan + remove_plan(result["plan_id"]) + + def test_stores_plan(self, config): + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="999", + ad_group_name="My Ad Group", + ) + plan = get_plan(result["plan_id"]) + assert plan is not None + assert plan.operation == "create_ad_group" + assert plan.entity_type == "ad_group" + + remove_plan(result["plan_id"]) + + def test_includes_keywords_in_plan(self, config): + keywords = [{"text": "buy shoes", "match_type": "EXACT"}] + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="999", + ad_group_name="Shoes Group", + keywords=keywords, + ) + assert result["changes"]["keywords"] == keywords + + remove_plan(result["plan_id"]) + + def test_includes_cpc_bid(self, config): + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="999", + ad_group_name="Test", + cpc_bid_micros=500000, + ) + assert result["changes"]["cpc_bid_micros"] == 500000 + + remove_plan(result["plan_id"]) + + def test_validation_error_missing_campaign_id(self, config): + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="", + ad_group_name="Test", + ) + assert "error" in result + + def test_validation_error_missing_name(self, config): + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="999", + ad_group_name="", + ) + assert "error" in result + + def test_blocked_operation(self, config): + config.safety.blocked_operations = ["create_ad_group"] + result = draft_ad_group( + config, + customer_id="1234567890", + campaign_id="999", + ad_group_name="Test", + ) + assert "error" in result + config.safety.blocked_operations = []