epic-2/story-1: Track Conversions from a Visitor Context - #29
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the foundation for conversion tracking (Story 2.1), introducing the track_conversion method on the visitor context, indexing goals by key in the configuration snapshot for O(1) lookups, and defining the core conversion models (ConversionStatus, ConversionEvent, and ConversionResult). The review feedback recommends adding **kwargs to track_conversion and create_conversion to ensure true forward-compatibility with future stories, and suggests using None instead of an empty string to represent missing goal IDs in ConversionEvent to align with standard Pythonic practices.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def track_conversion(self, goal_key: str) -> ConversionResult: | ||
| """Track a goal conversion for this visitor (Story 2.1). | ||
|
|
||
| Resolves ``goal_key`` against the current immutable snapshot and creates | ||
| an in-process conversion event associated with this visitor and the | ||
| resolved goal identity. Returns a typed | ||
| :class:`~convert_sdk.domain.results.ConversionResult`: | ||
|
|
||
| * ``status == ConversionStatus.QUEUED`` — the goal resolved and an event | ||
| was created (``result.event`` carries the | ||
| :class:`~convert_sdk.domain.results.ConversionEvent`). | ||
| * ``status == ConversionStatus.GOAL_NOT_FOUND`` — the goal key is absent | ||
| from the loaded config. This is a diagnosable NON-EXCEPTION outcome | ||
| (FR50), distinguishable from success via ``status`` alone — callers | ||
| never need ``try``/``except`` to tell the two apart. | ||
|
|
||
| Performs no network I/O and no payload serialization; event delivery, | ||
| payload shaping, batching, deduplication, and flush land in later Epic 2 | ||
| stories. The Story 2.1 surface is goal-key only; richer conversion | ||
| attributes (e.g. revenue) arrive in Story 2.2 as additional keyword | ||
| arguments, keeping this signature forward-compatible. | ||
| """ | ||
| return create_conversion( | ||
| self._snapshot, | ||
| visitor_id=self._state.visitor_id, | ||
| goal_key=goal_key, | ||
| ) |
There was a problem hiding this comment.
The docstring and PR description mention that the signature of track_conversion is designed to be forward-compatible with Story 2.2's conversion_data= kwargs. However, the current signature def track_conversion(self, goal_key: str) does not accept arbitrary keyword arguments. Any caller attempting to pass keyword arguments (e.g., conversion_data) will encounter a TypeError. To make the signature truly forward-compatible, we should add **kwargs: Any to both track_conversion and create_conversion.
def track_conversion(self, goal_key: str, **kwargs: Any) -> ConversionResult:
"""Track a goal conversion for this visitor (Story 2.1).
Resolves ``goal_key`` against the current immutable snapshot and creates
an in-process conversion event associated with this visitor and the
resolved goal identity. Returns a typed
:class:`~convert_sdk.domain.results.ConversionResult`:
* ``status == ConversionStatus.QUEUED`` — the goal resolved and an event
was created (``result.event`` carries the
:class:`~convert_sdk.domain.results.ConversionEvent`).
* ``status == ConversionStatus.GOAL_NOT_FOUND`` — the goal key is absent
from the loaded config. This is a diagnosable NON-EXCEPTION outcome
(FR50), distinguishable from success via ``status`` alone — callers
never need ``try``/``except`` to tell the two apart.
Performs no network I/O and no payload serialization; event delivery,
payload shaping, batching, deduplication, and flush land in later Epic 2
stories. The Story 2.1 surface is goal-key only; richer conversion
attributes (e.g. revenue) arrive in Story 2.2 as additional keyword
arguments, keeping this signature forward-compatible.
"""
return create_conversion(
self._snapshot,
visitor_id=self._state.visitor_id,
goal_key=goal_key,
**kwargs,
)| def create_conversion( | ||
| snapshot: "ConfigSnapshot", | ||
| *, | ||
| visitor_id: str, | ||
| goal_key: str, | ||
| ) -> ConversionResult: |
There was a problem hiding this comment.
To support the forward-compatible signature of Context.track_conversion, create_conversion should also accept arbitrary keyword arguments (**kwargs: Any). This ensures that when additional attributes (like revenue) are introduced in Story 2.2, the underlying creation service can receive them without breaking the current signature. Note: You will need to import Any from typing in this file.
def create_conversion(
snapshot: "ConfigSnapshot",
*,
visitor_id: str,
goal_key: str,
**kwargs: Any,
) -> ConversionResult:| """ | ||
|
|
||
| visitor_id: str | ||
| goal_id: str |
There was a problem hiding this comment.
In Python, it is idiomatic to represent the absence of an optional value using None rather than an empty string "". Since ConversionResult.goal_id is already typed as Optional[str], we should also type ConversionEvent.goal_id as Optional[str] to maintain consistency and avoid using empty strings as a sentinel for missing IDs.
| goal_id: str | |
| goal_id: Optional[str] |
References
- Representing missing optional values as None is standard Pythonic practice (PEP 8 / idiomatic Python), rather than using empty strings as a sentinel. (link)
| raw_goal_id = goal.get("id") | ||
| goal_id = str(raw_goal_id) if raw_goal_id is not None else "" |
There was a problem hiding this comment.
Aligning with the updated Optional[str] type for ConversionEvent.goal_id, we should use None instead of "" when raw_goal_id is missing. This avoids using empty strings as a sentinel for missing IDs and keeps the representation of missing IDs consistent across both ConversionEvent and ConversionResult.
| raw_goal_id = goal.get("id") | |
| goal_id = str(raw_goal_id) if raw_goal_id is not None else "" | |
| raw_goal_id = goal.get("id") | |
| goal_id = str(raw_goal_id) if raw_goal_id is not None else None |
References
- Representing missing optional values as None is standard Pythonic practice (PEP 8 / idiomatic Python), rather than using empty strings as a sentinel. (link)
Beads: ai-driven-product-dev-1bqe Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/Event — implementation (GREEN) Beads: ai-driven-product-dev-1bqe Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…plementation (GREEN) Beads: ai-driven-product-dev-z46t Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lementation (GREEN) Beads: ai-driven-product-dev-ck5j Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Beads: ai-driven-product-dev-ck5j Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9ec000b to
3962b4e
Compare
e5af437 to
4c2b98b
Compare
|
Superseded — all commits already in main (bc76b64). Closing without merge as part of post-sprint cleanup. |
Summary
First tracking-slice story (Epic 2), implemented fresh per the audit-corrected spec (findings F-038, F-051, F-052, F-055; original Dev Agent Record referred to the superseded PR 17 lineage):
Context.track_conversion(goal_key)— public MVP tracking call, forward-compatible with Story 2.2'sconversion_data=kwargs. ExportsConversionResult+ConversionStatusfrom the package root.ConfigSnapshotstoredgoalsbut had no precomputed index/accessor (Story 1.2 scope gap on this lineage) — added minimal_goals_by_keyindex +get_goal_by_key(), mirroring the existingget_feature_by_keypattern.ConversionResult(status=GOAL_NOT_FOUND, event=None)— distinguishable from a successful enqueue via.statuswithout try/except (1-4/1-6 typed-result precedent).tracking/package created as__init__.py+conversions.py(deliberate scope-narrowing —tracker.py/queue.py/deduplication.py/payloads.py/flush.pydeferred to later Epic 2 stories). No network delivery, no payload JSON, no batching in this story.Tests: 218 → 230 passing (+12,
tests/test_conversion_tracking.py). No regressions.Traceability
sprint/2026-04-06-convert-python-sdk— stacked on epic-1/story-3: Create and Reuse Visitor Contexts (gap-fill) #28 (story 1-3) → epic-1/story-6: Deliver Quickstart and First-Run Examples #27 (1-6) → epic-1/story-4: Run Local Experience Evaluations #26 (1-4) → epic-1/story-2: Support sdkKey and Direct-Config Initialization #25 (1-2) → Epic 1 Story 1: Scaffold the publishable SDK foundation #24 (1-1)ai-driven-product-dev-mq1w; tasks-1bqe(goal index + domain models),-z46t(conversions service),-ck5j(public surface) — all closed_bmad-output/implementation-artifacts/2026-04-06-convert-python-sdk/2-1-track-conversions-from-a-visitor-context.mdNotes for reviewer
ai-driven-product-dev/work/2026-06-07-track-conversions-from-a-visitor-context/readiness-assessment.md.str(None)coercion of missing goal id — fixed; round 2 clean).🤖 Generated with Claude Code