Repository navigation
feat(segments): P1/4 — the segment table, create and read a static list - #1345
manas-narra wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThis change adds CRM segment storage and service operations, then exposes merchant-scoped endpoints to create static lists, list segments, and retrieve a segment by UUID. ChangesCRM Segments
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CRMRootRouter
participant SegmentRouter
participant SegmentService
participant SegmentAccessor
participant crm_connection
CRMRootRouter->>SegmentRouter: Mount router at /segments
SegmentRouter->>SegmentService: Create static list for merchant
SegmentService->>SegmentAccessor: Insert normalized name and caller email
SegmentAccessor->>crm_connection: Execute insert query
crm_connection-->>SegmentAccessor: Return inserted row
SegmentAccessor-->>SegmentRouter: Return decoded Segment
Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 18.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 10 files. (2 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main changes: the segment table and create/read support for static lists.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 18.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 10 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the segment rows,
And trims the names before it goes.
The lists are stored, the pages flow,
Merchant-scoped results now show.
It thumps once softly: “All set!”
Comment @coderabbitai help to get the list of available commands.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary — PR #1345 (segments P1/4)
Files reviewed: 12 · New issues raised: 1 MINOR · 0 MAJOR · 0 CRITICAL
What was verified (not assumed)
- SQL safety: every builder in
db/queries/segment.pyparameterizes all values via$1/$2/$3withmerchant_idat$1on every read and write; the f-strings interpolate only module constants (SEGMENT_TABLE,_SEGMENT_COLUMNS), never request data. Matches the repo-wide builder pattern. - Auth & tenancy: all three
/segmentsroutes declaremerchant_scope(...)(verified againstapp/crm/auth.py— fail-closed 403/400, request-sourced merchant_id). Every query self-scopes onmerchant_id = $1, so another merchant's segment id reads as None → 404. The route-walk test pins the door structurally. - Validation:
SegmentCreateis a Pydantic model; blank-after-trim names are refused app-side (422) and DB-side (CHECK), and never reach the database. - Migration 084: a NEW sequential file (append-only rule satisfied);
crm_touch_updated_at()exists since 048;gen_random_uuid()has broad precedent; no GRANT/REVOKE block matches sibling non-append-only tables (064). - Layering: queries → accessor → decoder respected;
crm_connection()single-statement usage is the documented sanctioned pattern;UniqueViolationis exported from the db door;jsonb_valueis the total shared primitive; noSegmentname collision.
The one finding
💡 MINOR (084, line 35): the unique index is on raw name, so the whitespace-insensitive uniqueness the module promises is enforced only by the app-level trim. Upcoming writers (P2 member writer, P3 CSV import) could bypass it. Suggested fix: index on (merchant_id, btrim(name)) so the database enforces the invariant for every writer.
Verdict
Approve. No blocking criteria met. The MINOR is worth addressing before P2/P3 land writers into this table, but it does not block this phase.
|
|
||
| -- T15 col 3: the name is unique per merchant — merchant_id first (tenancy law). | ||
| CREATE UNIQUE INDEX crm_segment_merchant_name_uq | ||
| ON crm_segment (merchant_id, name); |
There was a problem hiding this comment.
💡 MINOR: The whitespace-insensitive uniqueness this module promises is enforced only by the app-level trim — this index is on the raw name, so segments.segment_name()'s claim that "Diwali VIPs" and "Diwali VIPs " are "one name to the unique index" holds only for writers that trim first. The P2 member writer and P3 CSV import are both upcoming writers into this table; any one of them that skips the trim creates two rows a merchant cannot tell apart, and nothing at the DB level refuses it. Enforce the invariant in the database so every writer inherits it:
CREATE UNIQUE INDEX crm_segment_merchant_name_uq
ON crm_segment (merchant_id, btrim(name));btrim is IMMUTABLE so the expression index is valid, it pairs naturally with the btrim(name) <> '' CHECK above, and the app path is unchanged: inserts still surface UniqueViolation, which create_static_list already maps to SegmentNameTaken.
There was a problem hiding this comment.
Fixed in 6f4e964, a CHECK instead of the expression index: name <> '' AND name !~ '^\s|\s$'. The table now refuses a blank or untrimmed name from any writer, so the plain (merchant_id, name) unique index already treats "Diwali VIPs" and "Diwali VIPs " as one name. Why the CHECK: it keeps the stored name clean for display, it's a format CHECK (CLAUDE.md), and \s also catches tabs and newlines, which btrim(name) alone doesn't. Every name the app writes passes it (segment_name() strips more). Checked in Postgres: "Diwali VIPs ", " Diwali", "Diwali\t", "Diwali\n", "" and " " are refused; "Diwali VIPs" and "Diwali – VIPs" pass. Thanks!
07301fc to
6f4e964
Compare
6f4e964 to
114c074
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary — PR #1345 (segments P1/4)
Files reviewed: 12 · New issues this run: 0 🔒 CRITICAL · 0
Decision: APPROVE — no blocking criteria met.
What was verified
- SQL safety — every builder in
db/queries/segment.pyuses$1/$2/$3positional placeholders withmerchant_idat$1on every read and write (tenancy law); the f-strings interpolate only module constants (SEGMENT_TABLE,_SEGMENT_COLUMNS), never request data. No injection surface. - Auth & tenancy — all three
/segmentsroutes declaremerchant_scope(...), which I confirmed internally performsget_current_user_with_rbac(JWT) plus the fail-closedassert_merchant_accesscheck, so the GET routes without an explicit auth dependency are still authenticated and tenant-scoped. Cross-merchant reads answer 404/None, and the(merchant_id, id)tenant-pin index is in place for the futurecrm_broadcastFK. - Migration 084 — a new sequential file (append-only rule respected; no existing migration touched).
crm_touch_updated_at()exists since 048; the table is not append-only, so no REVOKE/refusal trigger is required by the template. The earlier whitespace-uniqueness thread (Tara-ag) was resolved by the author'sname <> '' AND name !~ '^\s|\s$'CHECK — not re-raised. - Layering — queries → accessor → decoder respected;
crm_connection()is the sanctioned single-statement path;jsonb_valueis the shared defensive decoder;UniqueViolationis imported through the module's db door. - Validation —
SegmentCreateis a Pydantic model consistent with the module'sWorkflowCreateprecedent (merchant_scopereadsmerchant_idfrom the raw body on POST); name trimming/blank-refusal is tested and the DB CHECK backstops it for future writers. - Tests — 32 tests cover builders, the tenancy-door route walk, cross-merchant isolation, and every documented status code.
The one new comment
- 💡 MINOR (
app/crm/outreach/segments.py): the page and itstotalcome from two separate statements, while the repo's own precedent (enrollment.py) carriescount(*) OVER ()so a page and its total come from one statement. Non-blocking; a concrete one-query suggestion is in the comment.
Nice work — the canon self-defense tests that read the migration SQL back are a particularly good guard against drift in P2–P4.
| """One page of the merchant's segments, newest first, and the total.""" | ||
| return SegmentPage( | ||
| items=await segment_accessor.list_segments(merchant_id, limit, offset), |
There was a problem hiding this comment.
💡 MINOR: The page and its total come from two separate statements here, but this repo's own precedent for paged lists is one statement: app/crm/outreach/db/queries/enrollment.py's list query carries count(*) OVER () on every row precisely so "a page and its '1–10 of 46' come from one statement", with a fallback count read only when the page comes back empty. Two statements cost an extra round trip on every page turn and can disagree under a concurrent write — the page renders rows from one snapshot while total reflects another, so a merchant can see 3 items beside "total: 4".
Suggestion — carry the total on the page query and keep the fallback for the empty page, mirroring the enrollment accessor:
# db/queries/segment.py
def list_segments_query(
merchant_id: str, limit: int, offset: int
) -> Tuple[str, List[Any]]:
query = f"""
SELECT {_SEGMENT_COLUMNS}, (count(*) OVER ())::int AS total
FROM {SEGMENT_TABLE}
WHERE merchant_id = $1
ORDER BY created_at DESC, id DESC
LIMIT $2 OFFSET $3
"""
return query, [merchant_id, limit, offset]# db/accessors/segment.py
async def list_segments(
merchant_id: str, limit: int, offset: int
) -> Tuple[List[Segment], int]:
query, values = list_segments_query(merchant_id, limit, offset)
async with crm_connection() as conn:
rows = await conn.fetch(query, *values)
if rows:
return [decode_segment(r) for r in rows], int(rows[0]["total"])
# Empty page: count(*) OVER () has no row to ride on — read the total
# separately, the same fallback the enrollment accessor established.
return [], await count_segments(merchant_id)and segments.list_segments passes the pair straight into SegmentPage.
Migration 085 creates crm_segment (canon T15), owned by outreach: name
unique per merchant and stored trimmed (a CHECK refuses a blank or
untrimmed name, so the plain unique index holds for every writer), definition NULL for a static list (always a JSON
object when set), member_count + computed_at stamped 0/now at birth, and
the (merchant_id, id) pin target a broadcast will FK. The kind is derived
from definition on read, never stored.
POST /segments creates an empty static list (409 name taken, 422 blank),
GET /segments pages a merchant's segments with the total in the body,
GET /segments/{id} reads one (404 unknown or foreign). Every route
declares merchant_scope.
Members (T21) arrive in P2, CSV import in P3, dynamic segments in P4.
114c074 to
414221b
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary — APPROVED
Scope: 12 changed files (+731/−2): migration 085, the queries/accessor/decoder trio, segments.py logic, Segment/SegmentPage schemas, the /segments routes, router mount, owner-map + docs updates, and 373 lines of tests.
New issues this run: 0 🔒 CRITICAL · 0
What was verified
- SQL safety: all four builders in
db/queries/segment.pyare fully parameterized ($1–$3); f-strings interpolate only module constants (SEGMENT_TABLE,_SEGMENT_COLUMNS), never request/lead data.merchant_idis$1on every read and write — tenancy law holds. - Auth & tenancy: all three
/segmentsroutes declaremerchant_scope(...)(checked againstapp/crm/auth.py— JWT viaget_current_user_with_rbac+ fail-closedassert_merchant_access, marked for the route-walk test). Cross-tenant reads answer 404/None; the 403 path is tested. No client-supplied ID is trusted without the scope check. - Migration hygiene: 085 is a new sequential file — no existing migration touched. The name CHECK was verified at the raw-byte level:
'\\s'is a single backslash in the file, so the regex behaves correctly understandard_conforming_strings=on, and the test asserts that exact text.crm_touch_updated_at()exists (048); the three indexes match the query shapes (unique name, tenant pin, newest-first list). - Layering & conventions: queries → accessor → decoder respected;
assert row is not Nonematches the workflow/enrollment accessor precedent;jsonb_valuedecode is total;kindderived fromdefinition, never stored. - Validation & errors: Pydantic body with bounded
limit/offset;UniqueViolation → 409, blank-nameValueError → 422, unknown/foreign id→ 404all mapped and tested.
Pre-existing threads (not re-raised)
- 085 whitespace-insensitive uniqueness (Tara-ag) — answered by the author with the format CHECK; verified above.
segments.pypage+total from two statements vs thecount(*) OVER ()enrollment precedent — open MINOR for the author to consider; excluded from this decision per duplicate-avoidance.
Next steps
None blocking. P2 (members + writer), P3/P4 (dynamic segments) as planned — the tenant-pin index is already in place for crm_broadcast.segment_id.
3 participants
Why
Phase 2 of the CRM (task map W7) needs audiences: a broadcast admits a segment into a workflow, and
crm_broadcast.segment_idis NOT NULL (canon T17 col 6). Nothing exists for segments yet. This is phase 1 of 4: the segment row itself, created and read.A segment is one of two kinds (canon/05-audiences.md:3-4):
definitionis NULL.definitionholds the rule.This PR creates static lists only. Members arrive in P2; dynamic segments in P3 (the rule format, its check on save, create) and P4 (the compiler, evaluate and count). Uploading a CSV is a console feature: the console sends the file's rows in batches of 1,000 to P2's add-members route.
What
Migration 085 —
crm_segment(canon T15), owned by outreachid,merchant_idname(merchant_id, name)); a CHECK refuses a blank or untrimmed name (name <> '' AND name !~ '^\s|\s$'), so the plain unique index is whitespace-insensitive for every writer, not only the ones that trimdefinitionmember_count,computed_at0, stamped nowcreated_by,created_at,updated_atupdated_atvia the shared touch triggerAlso a unique
(merchant_id, id)— the pin target for the tenant-pinned FKcrm_broadcastwill use (the 049/058/073 precedent), so a broadcast can never freeze another merchant's segment — and(merchant_id, created_at DESC, id DESC)for the list read. The kind (static / dynamic) is derived fromdefinitionon read, never stored (law 10).Routes (
/segments, every route declaresmerchant_scope)POST /segments{merchant_id, name}GET /segments?merchant_id=&limit=&offset=totalin the bodyGET /segments/{id}?merchant_id=Layout — the module skeleton as outreach already has it:
outreach/segments.py(logic),db/queries|accessors|decoders/segment.py(one table, one file),Segment/SegmentPageinschemas.py,segment_routerinoutreach/api.pymounted at/segments,crm_segmentinTABLE_OWNERS, ownership note indocs/crm/migrations.md.Example
Not in this PR
crm_segment_member(T21) and adding members throughresolve()— P2, together with the writer that fills it.outreach/schemas.pyis past 1,000 lines; the per-family split the corpus names is owed as its own structure PR so it does not collide with open branches.Tests
tests/crm/test_segments.py(32): the table's owner and 085's self-defense (name unique per merchant with merchant first, blank name, object-only definition, tenant pin, no stored kind) · every builder putsmerchant_idat$1and parameterizes · a new list is born0/ stamped / no definition · name trimming and blank refusal (blank never reaches the database) · duplicate name →SegmentNameTaken· the same name across two merchants · another merchant's id reads as None · the kind derived fromdefinition· every/segmentsroute declares the tenancy door · the root router mounts/segments· 201 / 409 / 422 / 403 / 404 / 422 and the paged list. Two mutation checks (drop the trim; drop the refusal mapping) turn the suite red.Gates
black / isort / autoflake / pyrefly (0 errors) / migration guard (84, no gaps) / CRM boundary guard: pass.
tests/crm: 1581 passed, 15 skipped.After review
name, so "Diwali VIPs" and "Diwali VIPs " were one name only for writers that trim first. The name CHECK now refuses a blank or untrimmed name (name <> '' AND name !~ '^\s|\s$'—\scovers tabs and newlines, whichbtrimalone does not), so the plain unique index holds for every writer. Chosen over an expression index onbtrim(name)because it keeps the stored name clean for display and is a format CHECK (CLAUDE.md: CHECKs on format are required). Every name the app stores passes it:segment_name()strips a superset.Summary by CodeRabbit