feat(campaigns): delete on Meta when deleting a MaskanX campaign
Deleting a campaign only removed the local row. A live campaign deleted in MaskanX kept spending on Meta with nothing left here recording that it existed. Meta is now deleted first, and a failure there keeps the local row and answers 502, since forgetting it here while it still spends there is the worse of the two outcomes. An imported campaign is never deleted on Meta. MaskanX did not author it, and forgetting the import must not destroy work done in Ads Manager. Deleting a campaign cascades to its ad sets and ads, so only the campaign id is sent. The creative is left behind deliberately: it is an account-level asset that other ads may reference. Meta reports a delete of an absent object as code 100, which delete_object swallows, so retrying a partially-failed cleanup is safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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")
|
||||
Reference in New Issue
Block a user