Repository navigation
feat: publish deal.created from every booking path - #85
Conversation
Only the approval flow and the negotiation service emitted events. A deal booked over REST (quote -> book, from-template, or curated) left GET /events empty, so a client polling the feed never learned a deal existed. Add _emit_deal_created() and call it after each booking path persists its deal. The payload carries source (quote | template | curated), deal_type, status, product_id and quote_id. Fail-open like every non-audit event: a bus failure never fails the booking. Tests authenticate the quote booking call, since IABTechLab#77 requires a verified buyer on POST /api/v1/deals.
…nches _emit_deal_created is fail-closed, not fail-open: deal.created is in AUDIT_EVENT_TYPES, so a bus failure writes the event to the audit fallback file and the booking still succeeds; if that write also fails, the error propagates after the deal is already persisted. No code-path change. Two tests pin the branches over HTTP on the quote path.
bulk_deal_operations (create branch) and migrate_deal persisted a new deal without publishing deal.created, so the event feed missed those bookings. Both now call _emit_deal_created right after storage.set_deal, with source "bulk" and "migration"; the helper docstring lists all five sources. No emission for deprecate or for the old deal in a migration.
Same quote booked twice over HTTP with the same idempotency_key: both responses are 200 with the same deal_id and the bus holds exactly one deal.created. Test-only; the emission already sits after the replay short-circuit.
Each payload-inspecting test now asserts source, deal_type, status, product_id and quote_id (None where the path books without a quote) through one module-level _assert_payload helper shared by the five tests.
The http_client fixture's _FakeStorage has no ``set``, so the booking's audit-class deal.created fell back to data/audit_fallback.jsonl on every run. The fixture now patches get_event_bus to an InMemoryEventBus for every test that books through it, and the fallback path is gitignored next to *.db.
The DEAL_CREATED row said the event came from an accepted proposal; it is published by every booking path, with payload.source naming which one.
490119c to
b0e20f3
Compare
# Conflicts: # CHANGELOG.md
|
In bulk create, deal.created is published before the quote is marked booked. A listener can read an available quote for an already-created deal. Can the emit move after set_quote? |
Bulk create published before marking the quote booked, so a listener could read an available quote for a deal that already existed. Migration had the same shape: it published before deprecating the replaced deal. Both now publish last; the tests record what a listener reads from storage at publish time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # src/ad_seller/services/deal_service.py
|
Good catch, fixed in 4577058. Bulk create now publishes deal.created after the quote is marked booked. Migration had the same ordering: it published before deprecating the replaced deal, so it now publishes last too. The bulk and migration tests now record what a listener reads from storage at publish time (quote |
Kishore-2572
left a comment
Kishore-2572
left a comment
There was a problem hiding this comment.
Both of Aleks's and the author's own fixes verified live, not just read. Reproduced the original defect on the pre-fix tree first:
bulk_deal_operationsgenuinely publisheddeal.createdbeforestorage.set_quote(booked), so a listener at publish time would see anavailablequote for a deal that already existed;migrate_dealhad the identical shape, publishing before the replaced deal was markeddeprecated.Verified the fix (
4577058) closes both, and checked it isn't scoped narrower than it needs to be: traced all 5 deal-creation paths (quote,template,curated,bulk,migration) and every one now emits strictly after its path's last write. Checked every otherstorage.set_deal()call in the file (lazy expiry, bulk cancel, bulk notes-update, standalone deprecate) — all updates to already-existing deals, correctly not re-emitting. No sixth creation path silently skips the event.Negative control, run myself: reverted just the source fix with the test file untouched, reran
test_booking_events.py— exactly the 2 tests coveringbulk/migrationfailed, the other 6 stayed green. Confirms the new tests actually discriminate against the regression.Cross-repo: grepped buyer-agent for any consumption of
deal.created— none. Seller-side-only audit feed, zero cross-repo risk.Full suite green: 1664 passed (0 failed) on the latest rebase onto current
main,ruff checkclean, no conflicts anywhere.
3 participants
Problem
Only the approval flow and the negotiation service publish events. A deal booked over REST left
GET /eventsempty, so a client polling the feed never learned a deal existed. Verified on a running instance: two bookings, event count zero.Change
Add
_emit_deal_created()and call it after each booking path persists its deal: quote booking, from-template, curated, bulk create, and migration. The payload carriessource(one of those five),deal_type,status,product_id, andquote_id.deal.createdis an audit-class event in the event model, so the emission is fail-closed by design: if the bus fails, the event is written to the audit fallback log and the booking still returns 200; if that write also fails, the error propagates after the deal is already persisted. Both branches are pinned by tests. The retry-after-failure ordering, where a failure after the quote is flipped leaves a retry with a 409 and no deal id, applies to any post-persist failure and predates this change; it is tracked as #88.Docs: the event-bus overview now describes the real emitters and the
sourcevalues; changelog entry under Unreleased;data/audit_fallback.jsonlis gitignored. The fallback file is the same class of leftover as the database in #82: the audience-plan tests wrote it through a fake storage that backed the event bus, and now publish to an in-memory bus instead.Verification
tests/unit/test_booking_events.py: one test per booking path (five), bus failure with fallback written and booking at 200, fallback failure propagating after persist, idempotent replay emitting exactly once, and every payload asserted on all five keys. Each behavior test was seen failing before its change.tests/unit/test_audience_plan_validation.pynow publishes to an in-memory bus, so its fake storage no longer trips the audit fallback; a suite run on head leaves the tree clean.POST /api/v1/deals.test_self_asserted_advertiser_identity_is_floored, because the suite leavesad_seller.dbbehind; that is Unit suite leaves ./ad_seller.db behind, so a second consecutive local run can fail #82 and is not touched by this PR.Independent of #84; the
deal_service.pyhunks are disjoint.Not changed
From-template booking has no idempotency lookup, so a retried request creates a second deal and now a second event. Pre-existing; tracked as #87.