diff --git a/src/adclaw/app/routers/campaigns.py b/src/adclaw/app/routers/campaigns.py index e23ca67..a1ca961 100644 --- a/src/adclaw/app/routers/campaigns.py +++ b/src/adclaw/app/routers/campaigns.py @@ -21,7 +21,7 @@ from ...campaigns.state import ( next_status, ) from ...campaigns.adopt import adopt_campaign, discover_campaigns -from ...campaigns.sync import SyncConfigurationError, sync_campaign +from ...campaigns.sync import SyncConfigurationError, sync_campaign, unsync_campaign from ...campaigns.validation import validate_campaign from ...meta.client import MetaClient, MetaError, access_token_from_env from ._operator import UNAUTHENTICATED, require_operator @@ -287,7 +287,26 @@ async def update_campaign( @router.delete("/{campaign_id}", status_code=http_status.HTTP_204_NO_CONTENT) async def delete_campaign(campaign_id: str) -> None: - await _load_or_404(campaign_id) + """Delete the campaign here and, if MaskanX created it, on Meta too. + + Meta is deleted first. If that fails the local row is kept and the + caller gets a 502: a campaign forgotten here but left live on Meta + would keep spending with nothing in MaskanX recording that it exists. + + An imported campaign is only forgotten locally — see `unsync_campaign`. + """ + campaign = await _load_or_404(campaign_id) + if campaign.origin != "imported" and campaign.meta_campaign_id: + try: + await unsync_campaign(get_meta_client(), campaign) + except MetaError as exc: + raise HTTPException( + status_code=http_status.HTTP_502_BAD_GATEWAY, + detail=( + f"Could not delete this campaign on Meta, so it was kept " + f"here as well: {exc}. Delete it in Ads Manager, or retry." + ), + ) from exc await get_repository().delete_campaign(campaign_id) diff --git a/src/adclaw/campaigns/sync.py b/src/adclaw/campaigns/sync.py index a0737f9..25678bd 100644 --- a/src/adclaw/campaigns/sync.py +++ b/src/adclaw/campaigns/sync.py @@ -148,3 +148,20 @@ async def sync_campaign(repo, meta, campaign: CampaignSpec, actor: str) -> Campa campaign.sync_error = None campaign.advanced["last_synced_at"] = datetime.now(timezone.utc).isoformat() return await _persist("campaign.sync") + + +async def unsync_campaign(meta, campaign: CampaignSpec) -> bool: + """Delete this campaign's objects on Meta. Returns whether it deleted. + + Deleting a campaign cascades to its ad sets and ads, so the campaign id + is all that is needed. The creative is deliberately left behind: it is + an account-level asset that other ads may reference. + + An imported campaign is never deleted on Meta. MaskanX did not author + it, and forgetting the local record must not destroy work the operator + did in Ads Manager. + """ + if campaign.origin == "imported" or not campaign.meta_campaign_id: + return False + await meta.delete_object(campaign.meta_campaign_id) + return True diff --git a/src/adclaw/meta/client.py b/src/adclaw/meta/client.py index 3d3c48b..28f8981 100644 --- a/src/adclaw/meta/client.py +++ b/src/adclaw/meta/client.py @@ -165,6 +165,23 @@ class MetaClient: ) return result + async def _delete(self, path: str) -> dict[str, Any]: + """DELETE from Graph, with the same body-based error detection. + + Graph reports failures with HTTP 200 and an `"error"` key, so this + mirrors `_get`/`_post` rather than trusting the status code. + """ + payload = {"access_token": self._token} + result = await self._transport("DELETE", f"{GRAPH_BASE_URL}{path}", payload) + error = result.get("error") if isinstance(result, dict) else None + if error: + raise MetaError( + error.get("message") or "Meta request failed.", + code=error.get("code"), + subcode=error.get("error_subcode"), + ) + return result + async def get_ad_account(self, ad_account_id: str) -> dict[str, Any]: """Return billing and configuration fields for an ad account.""" return await self._get( @@ -358,6 +375,24 @@ class MetaClient: """ await self._post(f"/{object_id}", {"status": status}) + async def delete_object(self, object_id: str) -> None: + """Delete a campaign, ad set or ad on Meta. + + Deleting a campaign cascades to its ad sets and ads, so callers + holding the whole chain only need to delete the campaign. + + Meta answers a delete of an already-absent object with code 100 + ("Unsupported get request" / object does not exist). That is the + state the caller wanted, so it is swallowed and the delete is + idempotent — retrying a partially-failed cleanup is safe. + """ + try: + await self._delete(f"/{object_id}") + except MetaError as exc: + if exc.code == 100: + return + raise + async def list_campaigns(self, ad_account_id: str) -> list[dict[str, Any]]: """Return this ad account's campaigns.""" result = await self._get( diff --git a/tests/test_campaign_delete.py b/tests/test_campaign_delete.py new file mode 100644 index 0000000..3e8d401 --- /dev/null +++ b/tests/test_campaign_delete.py @@ -0,0 +1,164 @@ +# -*- coding: utf-8 -*- +"""Deleting a campaign must not leave objects spending on Meta. + +The dangerous case is a campaign forgotten in MaskanX but still live on +Meta: it keeps drawing budget with nothing left here to show it exists. +So Meta is deleted first, and a failure there keeps the local row. + +The opposite mistake matters too: deleting an imported campaign here must +not destroy work the operator did in Ads Manager. +""" +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from adclaw.app.routers import campaigns as campaigns_router +from adclaw.campaigns.models import CampaignSpec +from adclaw.campaigns.sync import unsync_campaign +from adclaw.meta.client import MetaClient, MetaError + + +class FakeRepo: + def __init__(self): + self.items: dict[str, CampaignSpec] = {} + self.deleted: list[str] = [] + + async def get_campaign(self, campaign_id): + return self.items.get(campaign_id) + + async def delete_campaign(self, campaign_id): + self.deleted.append(campaign_id) + self.items.pop(campaign_id, None) + + +class FakeMeta: + def __init__(self, error: MetaError | None = None): + self.error = error + self.deleted: list[str] = [] + + async def delete_object(self, object_id): + if self.error: + raise self.error + self.deleted.append(object_id) + + +def _campaign(**overrides) -> CampaignSpec: + data = { + "id": "camp_1", + "name": "Q3 lead gen", + "status": "live", + "origin": "maskanx", + "ad_account_id": "act_1", + "meta_campaign_id": "meta_camp_1", + "budget": {"daily_budget": 10000}, + } + data.update(overrides) + return CampaignSpec(**data) + + +@pytest.fixture() +def env(monkeypatch): + repo, meta = FakeRepo(), FakeMeta() + monkeypatch.setattr(campaigns_router, "get_repository", lambda: repo) + monkeypatch.setattr(campaigns_router, "get_meta_client", lambda: meta) + app = FastAPI() + app.include_router(campaigns_router.router, prefix="/api") + return TestClient(app), repo, meta + + +def test_delete_removes_the_campaign_on_meta_as_well(env): + client, repo, meta = env + repo.items["camp_1"] = _campaign() + + response = client.delete("/api/campaigns/camp_1") + + assert response.status_code == 204 + assert meta.deleted == ["meta_camp_1"] + assert repo.deleted == ["camp_1"] + + +def test_delete_keeps_the_local_row_when_meta_delete_fails(env): + """Forgetting it here while it still spends there is the worst outcome.""" + client, repo, meta = env + meta.error = MetaError("Permission denied", code=200) + repo.items["camp_1"] = _campaign() + + response = client.delete("/api/campaigns/camp_1") + + assert response.status_code == 502 + assert "Ads Manager" in response.json()["detail"] + assert repo.deleted == [] + assert "camp_1" in repo.items + + +def test_delete_of_an_imported_campaign_does_not_touch_meta(env): + """It was authored in Ads Manager; MaskanX only forgets the import.""" + client, repo, meta = env + repo.items["camp_1"] = _campaign(origin="imported") + + response = client.delete("/api/campaigns/camp_1") + + assert response.status_code == 204 + assert meta.deleted == [] + assert repo.deleted == ["camp_1"] + + +def test_delete_of_an_unsynced_campaign_does_not_call_meta(env): + client, repo, meta = env + repo.items["camp_1"] = _campaign(status="draft", meta_campaign_id=None) + + response = client.delete("/api/campaigns/camp_1") + + assert response.status_code == 204 + assert meta.deleted == [] + assert repo.deleted == ["camp_1"] + + +def test_delete_of_a_missing_campaign_is_404(env): + client, repo, meta = env + + response = client.delete("/api/campaigns/nope") + + assert response.status_code == 404 + assert meta.deleted == [] + + +async def test_unsync_reports_whether_it_deleted(): + meta = FakeMeta() + + assert await unsync_campaign(meta, _campaign()) is True + assert await unsync_campaign(meta, _campaign(origin="imported")) is False + assert await unsync_campaign(meta, _campaign(meta_campaign_id=None)) is False + assert meta.deleted == ["meta_camp_1"] + + +# --- client transport --- + + +async def test_delete_object_issues_a_graph_delete(): + calls = [] + + async def transport(method, url, params): + calls.append((method, url)) + return {"success": True} + + await MetaClient("tok", transport=transport).delete_object("meta_camp_1") + + assert calls == [("DELETE", "https://graph.facebook.com/v23.0/meta_camp_1")] + + +async def test_delete_object_treats_an_absent_object_as_deleted(): + """Retrying a partially-failed cleanup must not fail on the done part.""" + + async def transport(method, url, params): + return {"error": {"message": "Unsupported get request.", "code": 100}} + + await MetaClient("tok", transport=transport).delete_object("gone") + + +async def test_delete_object_raises_on_any_other_meta_error(): + async def transport(method, url, params): + return {"error": {"message": "Permission denied", "code": 200}} + + with pytest.raises(MetaError): + await MetaClient("tok", transport=transport).delete_object("meta_camp_1")