From 9199fb2dac2956ae2ae0cb66fbd0d9357ececbd3 Mon Sep 17 00:00:00 2001 From: Aleksander Sekowski Date: Sun, 30 Aug 2026 20:37:55 -0700 Subject: [PATCH] fix: 404 unresolvable deal_id and quote_id on order create (#68, #70) --- src/ad_seller/services/order_service.py | 37 +++++++++++++++++- tests/unit/test_change_request.py | 25 ++++++++++++ tests/unit/test_order_endpoints.py | 51 +++++++++++++++++++++++++ tests/unit/test_service_layer.py | 19 +++++++++ 4 files changed, 131 insertions(+), 1 deletion(-) diff --git a/src/ad_seller/services/order_service.py b/src/ad_seller/services/order_service.py index 9dc85376..745bb121 100644 --- a/src/ad_seller/services/order_service.py +++ b/src/ad_seller/services/order_service.py @@ -33,12 +33,33 @@ async def create_order( quote_id: Optional[str] = None, metadata: Optional[dict] = None, ) -> dict[str, Any]: - """Create a new order and persist its state machine.""" + """Create a new order and persist its state machine. + + ``deal_id`` and ``quote_id`` may be omitted (an order can be drafted + before commercial terms exist). A provided id must already be in + storage; unknown ids 404 with the same structured errors as GET deal + and GET quote. + """ from ..models.order_state_machine import OrderStateMachine from ..storage.factory import get_storage storage = await get_storage() + if deal_id: + deal = await storage.get_deal(deal_id) + if not deal: + raise HTTPException( + status_code=404, + detail={"error": "deal_not_found", "message": f"Deal '{deal_id}' not found."}, + ) + if quote_id: + quote = await storage.get_quote(quote_id) + if not quote: + raise HTTPException( + status_code=404, + detail={"error": "quote_not_found", "message": f"Quote '{quote_id}' not found."}, + ) + order_id = f"ORD-{uuid.uuid4().hex[:12].upper()}" machine = OrderStateMachine(order_id=order_id) @@ -333,6 +354,20 @@ async def create_change_request(request: Any) -> dict[str, Any]: }, ) + # A non-empty deal_id on the order must still resolve. Empty/None is + # allowed (orders may exist before a deal is attached). + deal_id = order.get("deal_id") or "" + if deal_id: + deal = await storage.get_deal(deal_id) + if not deal: + raise HTTPException( + status_code=404, + detail={ + "error": "deal_not_found", + "message": f"Deal '{deal_id}' not found.", + }, + ) + # Build diffs diffs = [ FieldDiff(field=d.field, old_value=d.old_value, new_value=d.new_value) diff --git a/tests/unit/test_change_request.py b/tests/unit/test_change_request.py index ce0a147d..454bf6df 100644 --- a/tests/unit/test_change_request.py +++ b/tests/unit/test_change_request.py @@ -73,6 +73,7 @@ def mock_storage(): ) ] ) + storage.get_deal = AsyncMock(side_effect=lambda did: store.get(f"deal:{did}")) storage._store = store return storage @@ -98,6 +99,7 @@ def _seed_order(mock_storage, order_id="ORD-TEST001", status="booked"): "metadata": {"campaign": "spring-2026"}, "audit_log": {"order_id": order_id, "transitions": []}, } + mock_storage._store["deal:DEMO-ABC123"] = {"deal_id": "DEMO-ABC123"} # ============================================================================= @@ -261,6 +263,29 @@ async def test_validation_failure_on_completed_order(self, client, mock_storage) assert resp.status_code == 422 assert resp.json()["detail"]["error"] == "validation_failed" + async def test_unresolvable_deal_id_does_not_create_pending_cr(self, client, mock_storage): + mock_storage._store["order:ORD-ORPHAN"] = { + "order_id": "ORD-ORPHAN", + "status": "booked", + "deal_id": "DEMO-GONE", + "metadata": {}, + "audit_log": {"order_id": "ORD-ORPHAN", "transitions": []}, + } + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + resp = await client.post( + "/api/v1/change-requests", + json={ + "order_id": "ORD-ORPHAN", + "change_type": "impressions", + "diffs": [{"field": "impressions", "old_value": 5000000, "new_value": 8000000}], + "reason": "Increase campaign reach", + }, + ) + + assert resp.status_code == 404 + assert resp.json()["detail"]["error"] == "deal_not_found" + assert not any(k.startswith("change_request:") for k in mock_storage._store) + # ============================================================================= # GET /api/v1/change-requests diff --git a/tests/unit/test_order_endpoints.py b/tests/unit/test_order_endpoints.py index a9cd90e1..83079bf9 100644 --- a/tests/unit/test_order_endpoints.py +++ b/tests/unit/test_order_endpoints.py @@ -54,10 +54,26 @@ def mock_storage(): and (not filters or not filters.get("status") or v.get("status") == filters["status"]) ] ) + storage.get_deal = AsyncMock(side_effect=lambda did: store.get(f"deal:{did}")) + storage.set_deal = AsyncMock( + side_effect=lambda did, data: store.__setitem__(f"deal:{did}", data) + ) + storage.get_quote = AsyncMock(side_effect=lambda qid: store.get(f"quote:{qid}")) + storage.set_quote = AsyncMock( + side_effect=lambda qid, data, ttl=86400: store.__setitem__(f"quote:{qid}", data) + ) storage._store = store return storage +def _seed_deal(mock_storage, deal_id): + mock_storage._store[f"deal:{deal_id}"] = {"deal_id": deal_id} + + +def _seed_quote(mock_storage, quote_id): + mock_storage._store[f"quote:{quote_id}"] = {"quote_id": quote_id} + + @pytest.fixture def client(mock_storage): app.dependency_overrides[_get_optional_api_key_record] = lambda: None @@ -88,6 +104,8 @@ async def test_create_order_returns_draft(self, client, mock_storage): assert "audit_log" in data async def test_create_order_with_deal_id(self, client, mock_storage): + _seed_deal(mock_storage, "DEMO-ABC123") + _seed_quote(mock_storage, "qt-test456") with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): resp = await client.post( "/api/v1/orders", @@ -104,6 +122,34 @@ async def test_create_order_with_deal_id(self, client, mock_storage): assert data["quote_id"] == "qt-test456" assert data["metadata"]["campaign"] == "spring-2026" + async def test_unknown_deal_id_returns_404(self, client, mock_storage): + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + resp = await client.post("/api/v1/orders", json={"deal_id": "DEMO-MISSING"}) + + assert resp.status_code == 404 + assert resp.json()["detail"]["error"] == "deal_not_found" + assert not any(k.startswith("order:") for k in mock_storage._store) + + async def test_unknown_quote_id_returns_404(self, client, mock_storage): + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + resp = await client.post("/api/v1/orders", json={"quote_id": "qt-missing"}) + + assert resp.status_code == 404 + assert resp.json()["detail"]["error"] == "quote_not_found" + assert not any(k.startswith("order:") for k in mock_storage._store) + + async def test_unknown_quote_id_with_known_deal_returns_404(self, client, mock_storage): + _seed_deal(mock_storage, "DEMO-ABC123") + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + resp = await client.post( + "/api/v1/orders", + json={"deal_id": "DEMO-ABC123", "quote_id": "qt-missing"}, + ) + + assert resp.status_code == 404 + assert resp.json()["detail"]["error"] == "quote_not_found" + assert not any(k.startswith("order:") for k in mock_storage._store) + async def test_order_persisted_to_storage(self, client, mock_storage): with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): resp = await client.post("/api/v1/orders", json={}) @@ -129,6 +175,8 @@ async def test_list_empty(self, client, mock_storage): assert resp.json()["orders"] == [] async def test_list_returns_created_orders(self, client, mock_storage): + _seed_deal(mock_storage, "d1") + _seed_deal(mock_storage, "d2") with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): await client.post("/api/v1/orders", json={"deal_id": "d1"}) await client.post("/api/v1/orders", json={"deal_id": "d2"}) @@ -166,6 +214,7 @@ async def test_list_filter_by_status(self, client, mock_storage): class TestGetOrder: async def test_retrieve_order(self, client, mock_storage): + _seed_deal(mock_storage, "DEMO-X") with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): create_resp = await client.post("/api/v1/orders", json={"deal_id": "DEMO-X"}) order_id = create_resp.json()["order_id"] @@ -317,6 +366,7 @@ async def test_transition_not_found(self, client, mock_storage): assert resp.status_code == 404 async def test_full_lifecycle_via_api(self, client, mock_storage): + _seed_deal(mock_storage, "DEMO-LIFE") with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): # Create r = await client.post("/api/v1/orders", json={"deal_id": "DEMO-LIFE"}) @@ -350,6 +400,7 @@ async def test_full_lifecycle_via_api(self, client, mock_storage): assert r.json()["transition_count"] == 6 async def test_transition_preserves_extra_fields(self, client, mock_storage): + _seed_deal(mock_storage, "DEMO-KEEP") with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): r = await client.post( "/api/v1/orders", diff --git a/tests/unit/test_service_layer.py b/tests/unit/test_service_layer.py index a61dcad0..e6b69c0e 100644 --- a/tests/unit/test_service_layer.py +++ b/tests/unit/test_service_layer.py @@ -314,6 +314,7 @@ def test_deterministic_score_and_labels_are_stable(self): class TestOrderService: async def test_create_and_transition_order_happy_path(self, mock_storage): """Happy: create -> draft, then a valid draft->submitted transition.""" + mock_storage._store["deal:DEMO-1"] = {"deal_id": "DEMO-1"} with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): order = await order_service.create_order(deal_id="DEMO-1", metadata={"k": "v"}) assert order["status"] == "draft" @@ -331,6 +332,24 @@ async def test_create_and_transition_order_happy_path(self, mock_storage): assert stored["deal_id"] == "DEMO-1" assert stored["metadata"] == {"k": "v"} + async def test_create_order_unknown_deal_raises_404(self, mock_storage): + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + with pytest.raises(HTTPException) as exc: + await order_service.create_order(deal_id="DEMO-MISSING") + + assert exc.value.status_code == 404 + assert exc.value.detail["error"] == "deal_not_found" + assert mock_storage._store == {} + + async def test_create_order_unknown_quote_raises_404(self, mock_storage): + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + with pytest.raises(HTTPException) as exc: + await order_service.create_order(quote_id="qt-missing") + + assert exc.value.status_code == 404 + assert exc.value.detail["error"] == "quote_not_found" + assert mock_storage._store == {} + async def test_invalid_transition_returns_409_with_allowed(self, mock_storage): """Edge: draft->completed is rejected with allowed_transitions listed.""" with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage):