From 55d5af849b32b67483cf37dd36293f3430e1a753 Mon Sep 17 00:00:00 2001 From: AFFAANh Date: Tue, 4 Aug 2026 10:55:46 +0530 Subject: [PATCH 1/2] feat(campaigns): report advertising spend and cost per lead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MaskanX runs the campaigns; this stores what they cost and what they produced so the question "what did this campaign cost us per lead" is answerable next to the leads themselves. POST /integrations/campaigns is an upsert keyed on (provider, external_id), not an idempotent create like /leads. A lead is an event that happened once; a campaign's figures change every time they are read, and MaskanX re-pushes the same campaign as its spend grows. An Idempotency-Key here would pin the CRM to the first numbers it ever saw. Money is stored as integers in minor currency units, matching what MaskanX sends and what Meta uses. A Numeric would add a second convention and a rounding step between systems that currently agree exactly. Ad attribution is promoted out of crm_leads.attributes into indexed columns, so counting leads per campaign is a join rather than a JSON scan — which also keeps it working on both SQLite and PostgreSQL. Meta's lead count and the CRM's own are both kept. They routinely differ, since Meta attributes late and leads can be entered by hand, and the gap is worth seeing rather than hiding behind one number. Co-Authored-By: Claude Opus 5 --- .../b1c4d7e29a03_add_crm_campaigns.py | 94 +++++++++ app/api/campaigns.py | 72 +++++++ app/api/integrations.py | 86 ++++++++ app/api/router.py | 12 +- app/models.py | 70 +++++++ app/schemas.py | 61 ++++++ tests/test_campaigns.py | 192 ++++++++++++++++++ 7 files changed, 586 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/b1c4d7e29a03_add_crm_campaigns.py create mode 100644 app/api/campaigns.py create mode 100644 tests/test_campaigns.py diff --git a/alembic/versions/b1c4d7e29a03_add_crm_campaigns.py b/alembic/versions/b1c4d7e29a03_add_crm_campaigns.py new file mode 100644 index 0000000..f9bebef --- /dev/null +++ b/alembic/versions/b1c4d7e29a03_add_crm_campaigns.py @@ -0,0 +1,94 @@ +"""add crm_campaigns + +Mirrors advertising campaigns pushed from MaskanX so the CRM can report +spend and cost per lead alongside the leads those campaigns produced. + +Money columns are integers in minor currency units (paise, cents), matching +what MaskanX sends and what Meta's budget fields use. A Numeric here would +add a second convention and a rounding step between two systems that +currently agree exactly. + +Revision ID: b1c4d7e29a03 +Revises: eca4681c9f66 +Create Date: 2026-08-03 +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +revision: str = 'b1c4d7e29a03' +down_revision: Union[str, None] = 'eca4681c9f66' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.create_table( + 'crm_campaigns', + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('tenant_id', sa.String(length=36), nullable=False), + sa.Column('provider', sa.String(length=80), nullable=False), + sa.Column('external_id', sa.String(length=255), nullable=False), + sa.Column('name', sa.String(length=240), nullable=False), + sa.Column('status', sa.String(length=40), nullable=False), + sa.Column('objective', sa.String(length=80), nullable=True), + sa.Column('channel', sa.String(length=80), nullable=True), + sa.Column('currency', sa.String(length=8), nullable=True), + sa.Column('daily_budget', sa.Integer(), nullable=True), + sa.Column('spend', sa.Integer(), nullable=False), + sa.Column('impressions', sa.Integer(), nullable=False), + sa.Column('clicks', sa.Integer(), nullable=False), + sa.Column('leads', sa.Integer(), nullable=False), + sa.Column('cost_per_lead', sa.Integer(), nullable=True), + sa.Column('metrics_from', sa.String(length=20), nullable=True), + sa.Column('metrics_to', sa.String(length=20), nullable=True), + sa.Column('synced_at', sa.DateTime(timezone=True), nullable=False), + sa.Column('metadata_json', sa.JSON(), nullable=False), + sa.Column('created_at', sa.DateTime(timezone=True), nullable=False), + sa.Column('updated_at', sa.DateTime(timezone=True), nullable=False), + sa.ForeignKeyConstraint(['tenant_id'], ['crm_tenants.id'], ondelete='CASCADE'), + sa.PrimaryKeyConstraint('id'), + # MaskanX re-pushes the same campaign as its figures change, so the + # provider's own id is what identifies it, not our surrogate key. + sa.UniqueConstraint( + 'tenant_id', 'provider', 'external_id', + name='uq_crm_campaigns_provider_external', + ), + ) + op.create_index( + 'ix_crm_campaigns_tenant_status', + 'crm_campaigns', + ['tenant_id', 'status'], + ) + + # Ad attribution promoted out of crm_leads.attributes into real columns, + # so counting leads per campaign is an indexed join rather than a JSON + # scan. Nullable: most leads do not come from an ad. + op.add_column( + 'crm_leads', + sa.Column('campaign_external_id', sa.String(length=255), nullable=True), + ) + op.add_column( + 'crm_leads', + sa.Column('adset_external_id', sa.String(length=255), nullable=True), + ) + op.add_column( + 'crm_leads', + sa.Column('ad_external_id', sa.String(length=255), nullable=True), + ) + op.create_index( + 'ix_crm_leads_campaign_external_id', + 'crm_leads', + ['campaign_external_id'], + ) + + +def downgrade() -> None: + op.drop_index('ix_crm_leads_campaign_external_id', table_name='crm_leads') + op.drop_column('crm_leads', 'ad_external_id') + op.drop_column('crm_leads', 'adset_external_id') + op.drop_column('crm_leads', 'campaign_external_id') + op.drop_index('ix_crm_campaigns_tenant_status', table_name='crm_campaigns') + op.drop_table('crm_campaigns') diff --git a/app/api/campaigns.py b/app/api/campaigns.py new file mode 100644 index 0000000..845df3b --- /dev/null +++ b/app/api/campaigns.py @@ -0,0 +1,72 @@ +"""Advertising campaigns, as mirrored from MaskanX. + +Read-only. The CRM does not run campaigns — MaskanX does, and pushes their +figures here through `POST /integrations/campaigns`. This exists so that +"what did this campaign cost us per lead" can be answered next to the leads +themselves, without leaving the CRM. + +Each campaign carries two lead counts: `leads` as Meta attributes them, and +`crm_leads` as this database actually holds them. They routinely differ — +Meta attributes late, and a lead can be entered here by hand — and the gap +is worth seeing rather than hiding behind one number. +""" + +from fastapi import APIRouter +from sqlalchemy import func, select + +from app.core.security import CurrentUser, Database +from app.models import Campaign, Lead +from app.schemas import CampaignOut + +router = APIRouter(prefix="/campaigns", tags=["Campaigns"]) + + +@router.get("", response_model=list[CampaignOut]) +def list_campaigns(user: CurrentUser, db: Database) -> list[CampaignOut]: + campaigns = list( + db.scalars( + select(Campaign) + .where(Campaign.tenant_id == user.tenant_id) + .order_by(Campaign.spend.desc()), + ), + ) + + # One grouped count rather than a query per campaign: a workspace with + # a hundred campaigns should still be one round trip. + counts = dict( + db.execute( + select( + Lead.campaign_external_id, + func.count(Lead.id), + ) + .where( + Lead.tenant_id == user.tenant_id, + Lead.campaign_external_id.is_not(None), + ) + .group_by(Lead.campaign_external_id), + ).all(), + ) + + return [ + CampaignOut( + id=campaign.id, + provider=campaign.provider, + external_id=campaign.external_id, + name=campaign.name, + status=campaign.status, + objective=campaign.objective, + channel=campaign.channel, + currency=campaign.currency, + daily_budget=campaign.daily_budget, + spend=campaign.spend, + impressions=campaign.impressions, + clicks=campaign.clicks, + leads=campaign.leads, + cost_per_lead=campaign.cost_per_lead, + metrics_from=campaign.metrics_from, + metrics_to=campaign.metrics_to, + synced_at=campaign.synced_at, + crm_leads=counts.get(campaign.external_id, 0), + ) + for campaign in campaigns + ] diff --git a/app/api/integrations.py b/app/api/integrations.py index cfd9eb6..6b704f2 100644 --- a/app/api/integrations.py +++ b/app/api/integrations.py @@ -10,6 +10,7 @@ from sqlalchemy.orm import selectinload from app.core.config import get_settings from app.core.security import Admin, Database, hash_service_key from app.models import ( + Campaign, Contact, ExternalLink, IdempotencyRecord, @@ -21,6 +22,8 @@ from app.models import ( Tenant, ) from app.schemas import ( + IntegrationCampaignRequest, + IntegrationCampaignResponse, IntegrationCredentialOut, IntegrationKeyCreate, IntegrationKeyResponse, @@ -155,6 +158,21 @@ def _authenticate_integration(db: Database, raw_key: str | None) -> IntegrationC return credential +def _attribution(campaign: dict, key: str) -> str | None: + """Read one ad id out of a lead's campaign block. + + `campaign` is free-form JSON from the sender, so a value may be absent, + null, or a number. Anything not usable as an id becomes None rather + than being coerced — a lead attributed to the string "None" would be + worse than one attributed to nothing. + """ + value = campaign.get(key) + if value is None: + return None + text = str(value).strip() + return text or None + + @router.get("/status", response_model=IntegrationStatusResponse) def integration_status( db: Database, @@ -333,6 +351,12 @@ def ingest_lead( stage_id=first_stage.id, source_id=source.id, position=next_lead_position(db, credential.tenant_id, first_stage.id), + # Kept in `attributes` as well, so nothing the sender included is + # lost, but the three ids that identify the ad are promoted to + # columns — that is what makes "leads for this campaign" a join. + campaign_external_id=_attribution(payload.campaign, "campaign_id"), + adset_external_id=_attribution(payload.campaign, "adset_id"), + ad_external_id=_attribution(payload.campaign, "ad_id"), attributes={"campaign": payload.campaign, **payload.metadata}, ) db.add(lead) @@ -378,3 +402,65 @@ def ingest_lead( ) db.commit() return result + + +@router.post("/campaigns", response_model=IntegrationCampaignResponse) +def ingest_campaign( + payload: IntegrationCampaignRequest, + db: Database, + integration_key: Annotated[str | None, Header(alias="X-Integration-Key")] = None, +) -> IntegrationCampaignResponse: + """Create or update one advertising campaign's figures. + + Deliberately an upsert keyed on `(provider, external_id)` rather than an + idempotent create like `/leads`. A lead is an event that happened once; + a campaign's figures change every time they are read, and MaskanX pushes + the same campaign repeatedly as its spend grows. An Idempotency-Key here + would either reject the update or return a stale answer. + """ + credential = _authenticate_integration(db, integration_key) + + campaign = db.scalar( + select(Campaign).where( + Campaign.tenant_id == credential.tenant_id, + Campaign.provider == payload.provider, + Campaign.external_id == payload.external_id, + ), + ) + created = campaign is None + if campaign is None: + campaign = Campaign( + tenant_id=credential.tenant_id, + provider=payload.provider, + external_id=payload.external_id, + ) + db.add(campaign) + + campaign.name = payload.name + campaign.status = payload.status + campaign.objective = payload.objective + campaign.channel = payload.channel + campaign.currency = payload.currency + campaign.daily_budget = payload.daily_budget + campaign.spend = payload.spend + campaign.impressions = payload.impressions + campaign.clicks = payload.clicks + campaign.leads = payload.leads + campaign.cost_per_lead = payload.cost_per_lead + campaign.metrics_from = payload.metrics_from + campaign.metrics_to = payload.metrics_to + campaign.metadata_json = payload.metadata + campaign.synced_at = datetime.now(UTC) + + add_event( + db, + tenant_id=credential.tenant_id, + topic="campaign.synced", + payload={ + "provider": payload.provider, + "external_id": payload.external_id, + "spend": payload.spend, + }, + ) + db.commit() + return IntegrationCampaignResponse(campaign_id=campaign.id, created=created) diff --git a/app/api/router.py b/app/api/router.py index ae6dc62..5a786bf 100644 --- a/app/api/router.py +++ b/app/api/router.py @@ -1,6 +1,15 @@ from fastapi import APIRouter -from app.api import activities, auth, catalog, contacts, dashboard, integrations, leads +from app.api import ( + activities, + auth, + campaigns, + catalog, + contacts, + dashboard, + integrations, + leads, +) api_router = APIRouter(prefix="/api/v1") api_router.include_router(auth.router) @@ -9,5 +18,6 @@ api_router.include_router(contacts.router) api_router.include_router(leads.router) api_router.include_router(activities.router) api_router.include_router(catalog.router) +api_router.include_router(campaigns.router) api_router.include_router(integrations.router) diff --git a/app/models.py b/app/models.py index 1a89599..7be9565 100644 --- a/app/models.py +++ b/app/models.py @@ -279,6 +279,14 @@ class Lead(Base, TimestampMixin): type_id: Mapped[str | None] = mapped_column( ForeignKey("crm_lead_types.id", ondelete="SET NULL"), ) + # Ad attribution, promoted out of `attributes` into real columns. + # Meta stamps every lead-ad submission with the campaign, ad set and ad + # that produced it, so "spend and leads for this ad" is a join rather + # than a JSON scan — which also keeps it working on SQLite and + # PostgreSQL alike. Null for leads that did not come from an ad. + campaign_external_id: Mapped[str | None] = mapped_column(String(255), index=True) + adset_external_id: Mapped[str | None] = mapped_column(String(255)) + ad_external_id: Mapped[str | None] = mapped_column(String(255)) attributes: Mapped[dict[str, Any]] = mapped_column(JSON, default=dict, nullable=False) contact: Mapped[Contact | None] = relationship() @@ -558,3 +566,65 @@ class IdempotencyRecord(Base): nullable=False, ) + + +class Campaign(Base, TimestampMixin): + """An advertising campaign mirrored from MaskanX. + + The CRM does not run campaigns; it reports on them next to the leads + they produced. MaskanX pushes each campaign's figures here so that + "what did this campaign cost us per lead" is answerable without leaving + the CRM. + + Money is stored in **minor** currency units (paise, cents), matching + what MaskanX sends and what Meta's own budget fields use. Storing a + decimal here would introduce a second convention and a rounding step + between two systems that currently agree exactly. + """ + + __tablename__ = "crm_campaigns" + __table_args__ = ( + UniqueConstraint( + "tenant_id", + "provider", + "external_id", + name="uq_crm_campaigns_provider_external", + ), + Index("ix_crm_campaigns_tenant_status", "tenant_id", "status"), + ) + + id: Mapped[str] = mapped_column(String(36), primary_key=True, default=new_id) + tenant_id: Mapped[str] = mapped_column( + ForeignKey("crm_tenants.id", ondelete="CASCADE"), + nullable=False, + ) + # "maskanx" for now; kept general so a second ad platform does not need + # a second table. + provider: Mapped[str] = mapped_column(String(80), nullable=False) + external_id: Mapped[str] = mapped_column(String(255), nullable=False) + name: Mapped[str] = mapped_column(String(240), nullable=False) + status: Mapped[str] = mapped_column(String(40), nullable=False, default="unknown") + objective: Mapped[str | None] = mapped_column(String(80), nullable=True) + channel: Mapped[str | None] = mapped_column(String(80), nullable=True) + currency: Mapped[str | None] = mapped_column(String(8), nullable=True) + + daily_budget: Mapped[int | None] = mapped_column(Integer, nullable=True) + spend: Mapped[int] = mapped_column(Integer, nullable=False, default=0) + impressions: Mapped[int] = mapped_column(Integer, nullable=False, default=0) + clicks: Mapped[int] = mapped_column(Integer, nullable=False, default=0) + # Leads as Meta counts them. The CRM's own lead count can differ — a + # lead may arrive here that Meta has not attributed yet, or vice versa — + # so both are kept rather than one being derived from the other. + leads: Mapped[int] = mapped_column(Integer, nullable=False, default=0) + cost_per_lead: Mapped[int | None] = mapped_column(Integer, nullable=True) + + metrics_from: Mapped[str | None] = mapped_column(String(20), nullable=True) + metrics_to: Mapped[str | None] = mapped_column(String(20), nullable=True) + synced_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), + default=now_utc, + nullable=False, + ) + metadata_json: Mapped[dict[str, Any]] = mapped_column( + JSON, default=dict, nullable=False, + ) diff --git a/app/schemas.py b/app/schemas.py index 9ba1ad4..f2b1b9e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -409,3 +409,64 @@ class IntegrationStatusResponse(ApiModel): credential_id: str credential_name: str key_prefix: str + + +class IntegrationCampaignRequest(ApiModel): + """One advertising campaign's current figures, pushed from MaskanX. + + Money is in **minor** currency units (paise, cents), matching what Meta + and MaskanX both use. Sending a decimal would introduce a second + convention and a rounding step between systems that agree exactly. + """ + + provider: str = Field(default="maskanx", min_length=2, max_length=80) + external_id: str = Field(min_length=1, max_length=255) + name: str = Field(min_length=1, max_length=240) + status: str = Field(default="unknown", max_length=40) + objective: str | None = Field(default=None, max_length=80) + channel: str | None = Field(default=None, max_length=80) + currency: str | None = Field(default=None, max_length=8) + + daily_budget: int | None = Field(default=None, ge=0) + spend: int = Field(default=0, ge=0) + impressions: int = Field(default=0, ge=0) + clicks: int = Field(default=0, ge=0) + leads: int = Field(default=0, ge=0) + # Sent rather than derived so the CRM shows exactly the figure MaskanX + # shows. Recomputing it here from spend and leads would drift whenever + # the two systems rounded differently. + cost_per_lead: int | None = Field(default=None, ge=0) + + metrics_from: str | None = Field(default=None, max_length=20) + metrics_to: str | None = Field(default=None, max_length=20) + metadata: dict[str, Any] = Field(default_factory=dict) + + +class IntegrationCampaignResponse(ApiModel): + campaign_id: str + created: bool + + +class CampaignOut(ApiModel): + id: str + provider: str + external_id: str + name: str + status: str + objective: str | None + channel: str | None + currency: str | None + daily_budget: int | None + spend: int + impressions: int + clicks: int + leads: int + cost_per_lead: int | None + metrics_from: str | None + metrics_to: str | None + synced_at: datetime + # Leads the CRM itself holds for this campaign. Kept alongside `leads` + # (Meta's count) rather than replacing it: a lead can exist here that + # Meta has not attributed yet, and the gap between the two is worth + # seeing rather than hiding. + crm_leads: int = 0 diff --git a/tests/test_campaigns.py b/tests/test_campaigns.py new file mode 100644 index 0000000..29bb6b0 --- /dev/null +++ b/tests/test_campaigns.py @@ -0,0 +1,192 @@ +"""Advertising campaigns mirrored from MaskanX. + +The CRM does not run campaigns; MaskanX pushes their figures here so that +spend and cost per lead can be read next to the leads they produced. + +The behaviour that matters is that pushing the same campaign again updates +it rather than duplicating it — MaskanX re-pushes every few minutes as +spend grows — and that lead attribution actually links the two. +""" +from fastapi.testclient import TestClient +import pytest + + +@pytest.fixture() +def service_key(client: TestClient, auth_headers: dict[str, str]) -> str: + created = client.post( + "/api/v1/integrations/credentials", + headers=auth_headers, + json={"name": "MaskanX"}, + ) + assert created.status_code == 201, created.text + return created.json()["key"] + + +def _campaign(**overrides) -> dict: + payload = { + "provider": "maskanx", + "external_id": "camp_1", + "name": "Q3 lead gen", + "status": "live", + "objective": "OUTCOME_LEADS", + "channel": "facebook", + "currency": "INR", + "daily_budget": 50000, + "spend": 46000, + "impressions": 12000, + "clicks": 300, + "leads": 4, + "cost_per_lead": 11500, + "metrics_from": "2026-07-05", + "metrics_to": "2026-08-03", + } + payload.update(overrides) + return payload + + +def _push(client, service_key, **overrides): + return client.post( + "/api/v1/integrations/campaigns", + headers={"X-Integration-Key": service_key}, + json=_campaign(**overrides), + ) + + +def test_a_campaign_can_be_pushed_and_read_back( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + pushed = _push(client, service_key) + assert pushed.status_code == 200, pushed.text + assert pushed.json()["created"] is True + + listed = client.get("/api/v1/campaigns", headers=auth_headers) + assert listed.status_code == 200, listed.text + body = listed.json() + assert len(body) == 1 + assert body[0]["name"] == "Q3 lead gen" + assert body[0]["spend"] == 46000 + assert body[0]["cost_per_lead"] == 11500 + + +def test_pushing_the_same_campaign_again_updates_it( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + """MaskanX re-pushes every few minutes as spend grows.""" + _push(client, service_key, spend=46000) + again = _push(client, service_key, spend=90000, leads=9, cost_per_lead=10000) + + assert again.json()["created"] is False + + body = client.get("/api/v1/campaigns", headers=auth_headers).json() + assert len(body) == 1 + assert body[0]["spend"] == 90000 + assert body[0]["leads"] == 9 + + +def test_two_campaigns_are_kept_apart( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + _push(client, service_key, external_id="camp_1", name="First") + _push(client, service_key, external_id="camp_2", name="Second") + + body = client.get("/api/v1/campaigns", headers=auth_headers).json() + + assert {row["name"] for row in body} == {"First", "Second"} + + +def test_campaigns_are_ordered_by_spend( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + _push(client, service_key, external_id="small", name="Small", spend=1000) + _push(client, service_key, external_id="big", name="Big", spend=99000) + + body = client.get("/api/v1/campaigns", headers=auth_headers).json() + + assert [row["name"] for row in body] == ["Big", "Small"] + + +def test_pushing_a_campaign_needs_an_integration_key(client: TestClient) -> None: + response = client.post("/api/v1/integrations/campaigns", json=_campaign()) + + assert response.status_code == 401 + + +def test_reading_campaigns_needs_a_logged_in_user(client: TestClient) -> None: + assert client.get("/api/v1/campaigns").status_code == 401 + + +def test_negative_spend_is_rejected(client: TestClient, service_key: str) -> None: + """Money that went backwards is a bug upstream, not a figure to store.""" + assert _push(client, service_key, spend=-1).status_code == 422 + + +# --- attribution --- + + +def _lead(client, service_key, external_id, campaign_id): + return client.post( + "/api/v1/integrations/leads", + headers={ + "X-Integration-Key": service_key, + "Idempotency-Key": f"key-{external_id}", + }, + json={ + "provider": "maskanx", + "external_id": external_id, + "first_name": "Asha", + "last_name": "Menon", + "email": f"{external_id}@example.com", + "campaign": { + "campaign_id": campaign_id, + "adset_id": "meta_set_1", + "ad_id": "meta_ad_1", + }, + }, + ) + + +def test_leads_are_counted_against_the_campaign_that_produced_them( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + _push(client, service_key, external_id="camp_1", leads=9) + assert _lead(client, service_key, "lead_1", "camp_1").status_code == 200 + assert _lead(client, service_key, "lead_2", "camp_1").status_code == 200 + + body = client.get("/api/v1/campaigns", headers=auth_headers).json() + + # Meta's count and the CRM's own count are kept separately: they + # routinely differ, and the gap is worth seeing. + assert body[0]["leads"] == 9 + assert body[0]["crm_leads"] == 2 + + +def test_a_lead_from_another_campaign_is_not_counted( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + _push(client, service_key, external_id="camp_1") + _lead(client, service_key, "lead_1", "camp_2") + + body = client.get("/api/v1/campaigns", headers=auth_headers).json() + + assert body[0]["crm_leads"] == 0 + + +def test_a_lead_with_no_campaign_does_not_break_the_count( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + """Most leads do not come from an ad.""" + _push(client, service_key, external_id="camp_1") + response = client.post( + "/api/v1/integrations/leads", + headers={"X-Integration-Key": service_key, "Idempotency-Key": "manual-1"}, + json={ + "provider": "maskanx", + "external_id": "walk_in_1", + "first_name": "Walk", + "last_name": "In", + }, + ) + assert response.status_code == 200 + + body = client.get("/api/v1/campaigns", headers=auth_headers).json() + assert body[0]["crm_leads"] == 0 From 9d6aa64ad6075a55f503d11b3e4dc3e5fc25eb21 Mon Sep 17 00:00:00 2001 From: AFFAANh Date: Tue, 4 Aug 2026 11:07:50 +0530 Subject: [PATCH 2/2] feat(properties): add custom fields without a migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Modelled on HubSpot's custom properties: the definition lives in a table, the values live in each record's existing JSON attributes. Adding a field is an INSERT, so it can happen mid-conversation through MCP and the next record can use it immediately. The cost of that is that the database enforces nothing about the values, so data_type is enforced in application code and every write path has to come through it — otherwise the type on a definition is decoration. Values are coerced rather than merely checked, because callers are agents and HTTP clients and "42" is a number expressed loosely; what is rejected is genuinely ambiguous, like "quite large" for a number. A value whose property has no definition is rejected rather than stored. A typo sitting in the database looks exactly like data, and a registry whose set of fields is not actually the set of fields is worse than none. Workspaces with nothing defined keep the old free-form behaviour, so this does not break attributes already in use. name and data_type are not updatable: renaming orphans every value stored under the old key, and retyping leaves values that no longer satisfy the type. Deleting a definition leaves existing values alone rather than rewriting every record, so undoing a mistaken delete is just recreating the property. Two ways in, sharing one validator: a logged-in person, and an integration key for MaskanX. An agent gets no shortcut around the rules a person is held to. Co-Authored-By: Claude Opus 5 --- .../c2e5f8a41b76_add_custom_properties.py | 62 +++ app/api/contacts.py | 12 +- app/api/properties.py | 205 ++++++++++ app/api/router.py | 3 + app/models.py | 51 +++ app/properties.py | 173 ++++++++ app/schemas.py | 39 ++ app/services.py | 49 +++ tests/test_properties.py | 372 ++++++++++++++++++ 9 files changed, 965 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/c2e5f8a41b76_add_custom_properties.py create mode 100644 app/api/properties.py create mode 100644 app/properties.py create mode 100644 tests/test_properties.py diff --git a/alembic/versions/c2e5f8a41b76_add_custom_properties.py b/alembic/versions/c2e5f8a41b76_add_custom_properties.py new file mode 100644 index 0000000..b52a4f9 --- /dev/null +++ b/alembic/versions/c2e5f8a41b76_add_custom_properties.py @@ -0,0 +1,62 @@ +"""add crm_custom_properties + +Fields an operator (or an agent, through MCP) adds without a migration. + +The definition lives here; the values live in each record's existing +`attributes` JSON. That split is the whole point: adding a field is an +INSERT, so it can happen mid-conversation and be usable immediately. + +Revision ID: c2e5f8a41b76 +Revises: b1c4d7e29a03 +Create Date: 2026-08-04 +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +revision: str = 'c2e5f8a41b76' +down_revision: Union[str, None] = 'b1c4d7e29a03' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.create_table( + 'crm_custom_properties', + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('tenant_id', sa.String(length=36), nullable=False), + sa.Column('object_type', sa.String(length=40), nullable=False), + sa.Column('name', sa.String(length=80), nullable=False), + sa.Column('label', sa.String(length=160), nullable=False), + sa.Column('description', sa.Text(), nullable=True), + sa.Column('data_type', sa.String(length=24), nullable=False), + sa.Column('options', sa.JSON(), nullable=False), + sa.Column('is_required', sa.Boolean(), nullable=False), + sa.Column('created_by', sa.String(length=120), nullable=True), + sa.Column('created_at', sa.DateTime(timezone=True), nullable=False), + sa.Column('updated_at', sa.DateTime(timezone=True), nullable=False), + sa.ForeignKeyConstraint(['tenant_id'], ['crm_tenants.id'], ondelete='CASCADE'), + sa.PrimaryKeyConstraint('id'), + # `name` is the key written into each record's attributes, so it has + # to be unique per object type — two definitions sharing a key would + # fight over the same stored value. + sa.UniqueConstraint( + 'tenant_id', 'object_type', 'name', + name='uq_crm_custom_properties_object_name', + ), + ) + op.create_index( + 'ix_crm_custom_properties_tenant_object', + 'crm_custom_properties', + ['tenant_id', 'object_type'], + ) + + +def downgrade() -> None: + op.drop_index( + 'ix_crm_custom_properties_tenant_object', + table_name='crm_custom_properties', + ) + op.drop_table('crm_custom_properties') diff --git a/app/api/contacts.py b/app/api/contacts.py index b468770..d54dc89 100644 --- a/app/api/contacts.py +++ b/app/api/contacts.py @@ -18,6 +18,7 @@ from app.services import ( add_audit, add_event, apply_updates, + validated_attributes, contact_to_out, model_or_404, verify_optional_reference, @@ -83,7 +84,11 @@ def create_contact(payload: ContactCreate, user: Writer, db: Database) -> Contac user.tenant_id, ) verify_optional_reference(db, User, payload.owner_id, user.tenant_id) - contact = Contact(tenant_id=user.tenant_id, **payload.model_dump()) + values = payload.model_dump() + values["attributes"] = validated_attributes( + db, user.tenant_id, "contact", {}, values.get("attributes"), + ) + contact = Contact(tenant_id=user.tenant_id, **values) db.add(contact) try: db.flush() @@ -145,6 +150,11 @@ def update_contact( user.tenant_id, ) verify_optional_reference(db, User, values.get("owner_id"), user.tenant_id) + if "attributes" in values: + values["attributes"] = validated_attributes( + db, user.tenant_id, "contact", contact.attributes, + values["attributes"], + ) apply_updates(contact, values) add_audit( db, diff --git a/app/api/properties.py b/app/api/properties.py new file mode 100644 index 0000000..3373491 --- /dev/null +++ b/app/api/properties.py @@ -0,0 +1,205 @@ +"""Custom property definitions — fields added without a migration. + +Two ways in, on purpose: + +- `/properties` for a logged-in person, from the CRM's own settings screen. +- `/integrations/properties` for MaskanX, authenticated by an integration + key. That is what an agent reaches through MCP when a conversation asks + for a new field. + +Both go through the same validation. An agent gets no shortcut around the +rules a person is held to — if anything it needs them more, since it will +happily invent a property name from a half-sentence. +""" + +from typing import Annotated + +from fastapi import APIRouter, Header, HTTPException, status +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError + +from app.core.security import CurrentUser, Database +from app.models import CustomProperty +from app.properties import PropertyError, validate_definition +from app.schemas import ( + CustomPropertyCreate, + CustomPropertyOut, + CustomPropertyUpdate, +) + +router = APIRouter(prefix="/properties", tags=["Properties"]) +integration_router = APIRouter(prefix="/integrations", tags=["Integrations"]) + + +def _list_for(db, tenant_id: str, object_type: str | None) -> list[CustomProperty]: + query = select(CustomProperty).where(CustomProperty.tenant_id == tenant_id) + if object_type: + query = query.where(CustomProperty.object_type == object_type) + return list(db.scalars(query.order_by(CustomProperty.label))) + + +def _create( + db, + tenant_id: str, + payload: CustomPropertyCreate, + created_by: str, +) -> CustomProperty: + try: + validate_definition( + name=payload.name, + object_type=payload.object_type, + data_type=payload.data_type, + options=payload.options, + ) + except PropertyError as exc: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=str(exc), + ) from exc + + prop = CustomProperty( + tenant_id=tenant_id, + object_type=payload.object_type, + name=payload.name, + label=payload.label, + description=payload.description, + data_type=payload.data_type, + options=payload.options, + is_required=payload.is_required, + created_by=created_by, + ) + db.add(prop) + try: + db.commit() + except IntegrityError as exc: + db.rollback() + # Told plainly rather than as a constraint name: an agent that + # re-runs a request needs to know the field already exists, not + # that a unique index fired. + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + f"A property named {payload.name!r} already exists on " + f"{payload.object_type}." + ), + ) from exc + return prop + + +@router.get("", response_model=list[CustomPropertyOut]) +def list_properties( + user: CurrentUser, + db: Database, + object_type: str | None = None, +) -> list[CustomProperty]: + return _list_for(db, user.tenant_id, object_type) + + +@router.post("", response_model=CustomPropertyOut, status_code=status.HTTP_201_CREATED) +def create_property( + payload: CustomPropertyCreate, + user: CurrentUser, + db: Database, +) -> CustomProperty: + return _create(db, user.tenant_id, payload, created_by=user.email) + + +@router.patch("/{property_id}", response_model=CustomPropertyOut) +def update_property( + property_id: str, + payload: CustomPropertyUpdate, + user: CurrentUser, + db: Database, +) -> CustomProperty: + """Update a property's presentation, never its identity or type. + + `name` and `data_type` are not updatable — see `CustomPropertyUpdate`. + Narrowing an enumeration is allowed even though records may already + hold a value that is no longer offered: those values stay readable and + simply cannot be chosen again, which is the behaviour an operator + retiring an option expects. + """ + prop = db.scalar( + select(CustomProperty).where( + CustomProperty.id == property_id, + CustomProperty.tenant_id == user.tenant_id, + ), + ) + if prop is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) + + if payload.options is not None and prop.data_type != "enumeration": + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Options only apply to an enumeration, not to a {prop.data_type}.", + ) + if payload.options is not None and not payload.options: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="An enumeration needs at least one option.", + ) + + for field, value in payload.model_dump(exclude_unset=True).items(): + if value is not None: + setattr(prop, field, value) + db.commit() + return prop + + +@router.delete("/{property_id}", status_code=status.HTTP_204_NO_CONTENT) +def delete_property(property_id: str, user: CurrentUser, db: Database) -> None: + """Remove a definition. + + Values already written into records are left where they are. Rewriting + every record to strip a key would be a large, silent, irreversible + write in response to a small request; leaving them means undoing a + mistaken delete is just recreating the property. + """ + prop = db.scalar( + select(CustomProperty).where( + CustomProperty.id == property_id, + CustomProperty.tenant_id == user.tenant_id, + ), + ) + if prop is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) + db.delete(prop) + db.commit() + + +# --- integration key access, for MaskanX and its MCP tools --- + + +@integration_router.get("/properties", response_model=list[CustomPropertyOut]) +def list_properties_via_integration( + db: Database, + object_type: str | None = None, + integration_key: Annotated[str | None, Header(alias="X-Integration-Key")] = None, +) -> list[CustomProperty]: + from app.api.integrations import _authenticate_integration + + credential = _authenticate_integration(db, integration_key) + properties = _list_for(db, credential.tenant_id, object_type) + db.commit() + return properties + + +@integration_router.post( + "/properties", + response_model=CustomPropertyOut, + status_code=status.HTTP_201_CREATED, +) +def create_property_via_integration( + payload: CustomPropertyCreate, + db: Database, + integration_key: Annotated[str | None, Header(alias="X-Integration-Key")] = None, +) -> CustomProperty: + from app.api.integrations import _authenticate_integration + + credential = _authenticate_integration(db, integration_key) + return _create( + db, + credential.tenant_id, + payload, + created_by=f"integration:{credential.name}", + ) diff --git a/app/api/router.py b/app/api/router.py index 5a786bf..e031d9e 100644 --- a/app/api/router.py +++ b/app/api/router.py @@ -8,6 +8,7 @@ from app.api import ( contacts, dashboard, integrations, + properties, leads, ) @@ -19,5 +20,7 @@ api_router.include_router(leads.router) api_router.include_router(activities.router) api_router.include_router(catalog.router) api_router.include_router(campaigns.router) +api_router.include_router(properties.router) +api_router.include_router(properties.integration_router) api_router.include_router(integrations.router) diff --git a/app/models.py b/app/models.py index 7be9565..a302c2d 100644 --- a/app/models.py +++ b/app/models.py @@ -628,3 +628,54 @@ class Campaign(Base, TimestampMixin): metadata_json: Mapped[dict[str, Any]] = mapped_column( JSON, default=dict, nullable=False, ) + + +class CustomProperty(Base, TimestampMixin): + """A field an operator added, without a schema migration. + + Modelled on how HubSpot treats properties: the definition lives in a + table, and the values live in the record's existing `attributes` JSON. + That is the whole point — adding a field is an INSERT, not a migration, + so an agent can create one mid-conversation and the next record can use + it immediately. + + The cost of that choice is that values are not typed by the database, + so `data_type` is enforced in application code on write. Anything that + writes a custom property value has to go through that validation; a + direct UPDATE to `attributes` bypasses it. + + `name` is the stable key stored inside `attributes`; `label` is what a + person reads. They are kept separate for the same reason HubSpot keeps + them separate: renaming a label must not orphan every value already + written under the old key. + """ + + __tablename__ = "crm_custom_properties" + __table_args__ = ( + UniqueConstraint( + "tenant_id", + "object_type", + "name", + name="uq_crm_custom_properties_object_name", + ), + Index("ix_crm_custom_properties_tenant_object", "tenant_id", "object_type"), + ) + + id: Mapped[str] = mapped_column(String(36), primary_key=True, default=new_id) + tenant_id: Mapped[str] = mapped_column( + ForeignKey("crm_tenants.id", ondelete="CASCADE"), + nullable=False, + ) + # Which kind of record carries it: contact, lead, organization, campaign. + object_type: Mapped[str] = mapped_column(String(40), nullable=False) + name: Mapped[str] = mapped_column(String(80), nullable=False) + label: Mapped[str] = mapped_column(String(160), nullable=False) + description: Mapped[str | None] = mapped_column(Text) + data_type: Mapped[str] = mapped_column(String(24), nullable=False, default="string") + # Allowed values for an enumeration. Empty for every other type. + options: Mapped[list[str]] = mapped_column(JSON, default=list, nullable=False) + is_required: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False) + # Where it came from. An agent-created field is marked so a human can + # tell at a glance which fields they defined and which one a + # conversation produced. + created_by: Mapped[str | None] = mapped_column(String(120)) diff --git a/app/properties.py b/app/properties.py new file mode 100644 index 0000000..904af82 --- /dev/null +++ b/app/properties.py @@ -0,0 +1,173 @@ +"""Validation for custom property definitions and their values. + +Custom property values live in a JSON column, so the database enforces +nothing about them. Everything that writes one has to come through here, or +the `data_type` on the definition is decoration. + +Pure functions: no session, no I/O, so the rules can be tested directly. +""" + +from __future__ import annotations + +from datetime import date, datetime +from typing import Any + +# A property name becomes a key inside a record's `attributes` JSON and is +# referenced by agents and API callers, so it is restricted to something +# that survives being a JSON key, a query parameter and a column header. +NAME_PATTERN = r"^[a-z][a-z0-9_]{1,63}$" + +DATA_TYPES = ("string", "number", "boolean", "date", "enumeration") + +OBJECT_TYPES = ("contact", "lead", "organization", "campaign") + + +class PropertyError(ValueError): + """Raised when a definition or a value is not usable.""" + + +def validate_definition( + *, + name: str, + object_type: str, + data_type: str, + options: list[str] | None, +) -> None: + """Check a property definition before it is stored. + + Rejects an enumeration with no options: a field whose only valid values + are none of them cannot ever be filled in, and an agent creating one by + mistake would produce a form nobody can submit. + """ + import re + + if object_type not in OBJECT_TYPES: + raise PropertyError( + f"Unknown object type {object_type!r}. " + f"Available: {', '.join(OBJECT_TYPES)}.", + ) + if data_type not in DATA_TYPES: + raise PropertyError( + f"Unknown data type {data_type!r}. Available: {', '.join(DATA_TYPES)}.", + ) + if not re.match(NAME_PATTERN, name or ""): + raise PropertyError( + f"{name!r} is not a usable property name. Use lowercase letters, " + f"digits and underscores, starting with a letter, e.g. " + f"'budget_range'.", + ) + if data_type == "enumeration" and not options: + raise PropertyError( + "An enumeration needs at least one option; otherwise no value " + "could ever be valid for it.", + ) + if data_type != "enumeration" and options: + raise PropertyError( + f"Options only apply to an enumeration, not to a {data_type}.", + ) + + +def coerce_value(definition: Any, value: Any) -> Any: + """Return `value` in the shape its definition calls for. + + Coerces rather than merely checking, because callers are agents and + HTTP clients: a number arriving as the string "42" is a well-formed + intention expressed loosely, and rejecting it would be pedantry. What + is rejected is anything genuinely ambiguous — "quite large" is not a + number by any reading. + + `None` clears the value and is always allowed here; whether a property + may be empty is `is_required`'s business, checked separately, because + the two questions have different answers on a partial update. + """ + if value is None: + return None + + data_type = definition.data_type + + if data_type == "string": + return str(value) + + if data_type == "number": + try: + number = float(value) + except (TypeError, ValueError) as exc: + raise PropertyError( + f"{definition.label!r} expects a number, not {value!r}.", + ) from exc + # Keep whole numbers whole: 3.0 stored for a count reads oddly and + # round-trips into JSON as 3.0 forever. + return int(number) if number.is_integer() else number + + if data_type == "boolean": + if isinstance(value, bool): + return value + text = str(value).strip().lower() + if text in ("true", "yes", "1"): + return True + if text in ("false", "no", "0"): + return False + raise PropertyError( + f"{definition.label!r} expects true or false, not {value!r}.", + ) + + if data_type == "date": + if isinstance(value, (date, datetime)): + return value.isoformat() + try: + return date.fromisoformat(str(value)[:10]).isoformat() + except ValueError as exc: + raise PropertyError( + f"{definition.label!r} expects a date as YYYY-MM-DD, " + f"not {value!r}.", + ) from exc + + if data_type == "enumeration": + text = str(value) + if text not in (definition.options or []): + raise PropertyError( + f"{text!r} is not one of the allowed values for " + f"{definition.label!r}: " + f"{', '.join(definition.options or []) or 'none'}.", + ) + return text + + raise PropertyError(f"Unknown data type {data_type!r}.") + + +def apply_values( + definitions: list[Any], + current: dict[str, Any], + incoming: dict[str, Any], +) -> dict[str, Any]: + """Merge validated custom property values into a record's attributes. + + Only keys that have a definition are touched. An unknown key is + rejected rather than stored: silently accepting one would let a typo + ("budjet_range") sit in the database looking like data, and the whole + reason for a registry is that the set of fields is knowable. + + Required properties are enforced only against what is being written, + not against the merged result — a partial update that does not mention + a required field is not an attempt to clear it. + """ + by_name = {definition.name: definition for definition in definitions} + + unknown = sorted(set(incoming) - set(by_name)) + if unknown: + known = ", ".join(sorted(by_name)) or "none" + raise PropertyError( + f"No such property: {', '.join(unknown)}. Defined here: {known}.", + ) + + merged = dict(current) + for name, raw in incoming.items(): + definition = by_name[name] + coerced = coerce_value(definition, raw) + if coerced is None and definition.is_required: + raise PropertyError(f"{definition.label!r} is required and cannot be empty.") + if coerced is None: + merged.pop(name, None) + else: + merged[name] = coerced + return merged diff --git a/app/schemas.py b/app/schemas.py index f2b1b9e..f428869 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -470,3 +470,42 @@ class CampaignOut(ApiModel): # Meta has not attributed yet, and the gap between the two is worth # seeing rather than hiding. crm_leads: int = 0 + + +class CustomPropertyCreate(ApiModel): + object_type: str = Field(min_length=2, max_length=40) + name: str = Field(min_length=2, max_length=80) + label: str = Field(min_length=1, max_length=160) + description: str | None = None + data_type: str = Field(default="string", max_length=24) + options: list[str] = Field(default_factory=list) + is_required: bool = False + + +class CustomPropertyUpdate(ApiModel): + """A partial update. + + `name`, `object_type` and `data_type` are absent on purpose. They are + the identity and meaning of the field: changing a name orphans every + value already stored under it, and changing a type leaves stored values + that no longer satisfy it. Delete and recreate instead — deliberately + more effort, because it loses data. + """ + + label: str | None = Field(default=None, min_length=1, max_length=160) + description: str | None = None + options: list[str] | None = None + is_required: bool | None = None + + +class CustomPropertyOut(ApiModel): + id: str + object_type: str + name: str + label: str + description: str | None + data_type: str + options: list[str] + is_required: bool + created_by: str | None + created_at: datetime diff --git a/app/services.py b/app/services.py index 2a42c37..9d1c4c9 100644 --- a/app/services.py +++ b/app/services.py @@ -247,3 +247,52 @@ def next_lead_position(db: Session, tenant_id: str, stage_id: str) -> int: def decimal_or_zero(value: Decimal | None) -> Decimal: return value or Decimal("0") + + +def validated_attributes( + db: Session, + tenant_id: str, + object_type: str, + current: dict[str, Any], + incoming: dict[str, Any] | None, +) -> dict[str, Any]: + """Merge incoming custom property values after validating them. + + Custom property values live in a JSON column, so the database enforces + nothing about them — every write path has to come through here or the + `data_type` on a definition is decoration. + + Values whose property has no definition are rejected rather than + stored. Silently accepting one would let a typo sit in the database + looking like data, and a registry whose set of fields is not actually + the set of fields is worse than none. + """ + from sqlalchemy import select + + from app.models import CustomProperty + from app.properties import PropertyError, apply_values + + if incoming is None: + return current + + definitions = list( + db.scalars( + select(CustomProperty).where( + CustomProperty.tenant_id == tenant_id, + CustomProperty.object_type == object_type, + ), + ), + ) + # Nothing defined yet: keep the previous behaviour of storing whatever + # arrives, so adding this validation does not break workspaces that + # already use `attributes` as a free-form bag. + if not definitions: + return incoming + + try: + return apply_values(definitions, current, incoming) + except PropertyError as exc: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=str(exc), + ) from exc diff --git a/tests/test_properties.py b/tests/test_properties.py new file mode 100644 index 0000000..4c3e56a --- /dev/null +++ b/tests/test_properties.py @@ -0,0 +1,372 @@ +"""Custom properties: fields added without a migration. + +The definitions live in a table and the values in each record's JSON +`attributes`, so nothing in the database enforces a type. These tests are +mostly about the validation that stands in for that, and about the two ways +in — a logged-in person, and an integration key, which is what an agent +reaches through MCP. +""" +import pytest +from fastapi.testclient import TestClient + +from app.properties import PropertyError, coerce_value, validate_definition + + +class _Definition: + """Stands in for a stored definition in the pure-function tests.""" + + def __init__(self, data_type, options=None, label="Field", is_required=False): + self.data_type = data_type + self.options = options or [] + self.label = label + self.is_required = is_required + self.name = "field" + + +# --- defining --- + + +def test_a_name_must_be_a_usable_key(): + """It becomes a JSON key and a query parameter, not just a label.""" + for bad in ("Budget Range", "1st_field", "budget-range", "", "A"): + with pytest.raises(PropertyError): + validate_definition( + name=bad, object_type="contact", data_type="string", options=None, + ) + + +def test_a_good_name_is_accepted(): + validate_definition( + name="budget_range", object_type="contact", data_type="string", options=None, + ) + + +def test_an_enumeration_with_no_options_is_refused(): + """No value could ever be valid for it.""" + with pytest.raises(PropertyError, match="at least one option"): + validate_definition( + name="stage", object_type="lead", data_type="enumeration", options=[], + ) + + +def test_options_on_a_non_enumeration_are_refused(): + with pytest.raises(PropertyError, match="only apply to an enumeration"): + validate_definition( + name="notes", object_type="lead", data_type="string", options=["a"], + ) + + +def test_an_unknown_object_type_lists_the_real_ones(): + with pytest.raises(PropertyError, match="contact"): + validate_definition( + name="x", object_type="spaceship", data_type="string", options=None, + ) + + +# --- values --- + + +def test_a_number_arriving_as_a_string_is_accepted(): + """Callers are agents and HTTP clients; "42" is a number loosely typed.""" + assert coerce_value(_Definition("number"), "42") == 42 + + +def test_a_whole_number_stays_whole(): + assert coerce_value(_Definition("number"), "3.0") == 3 + + +def test_a_decimal_survives(): + assert coerce_value(_Definition("number"), "3.5") == 3.5 + + +def test_something_genuinely_unnumeric_is_refused(): + with pytest.raises(PropertyError, match="expects a number"): + coerce_value(_Definition("number"), "quite large") + + +@pytest.mark.parametrize("value", ["true", "Yes", "1", True]) +def test_truthy_spellings_of_boolean(value): + assert coerce_value(_Definition("boolean"), value) is True + + +@pytest.mark.parametrize("value", ["false", "No", "0", False]) +def test_falsy_spellings_of_boolean(value): + assert coerce_value(_Definition("boolean"), value) is False + + +def test_an_ambiguous_boolean_is_refused(): + with pytest.raises(PropertyError, match="true or false"): + coerce_value(_Definition("boolean"), "maybe") + + +def test_a_date_is_normalised_to_iso(): + assert coerce_value(_Definition("date"), "2026-08-04") == "2026-08-04" + + +def test_a_datetime_string_keeps_only_the_date(): + assert coerce_value(_Definition("date"), "2026-08-04T10:30:00Z") == "2026-08-04" + + +def test_an_unparseable_date_says_what_it_wanted(): + with pytest.raises(PropertyError, match="YYYY-MM-DD"): + coerce_value(_Definition("date"), "next tuesday") + + +def test_an_enumeration_value_must_be_offered(): + definition = _Definition("enumeration", options=["hot", "warm"], label="Temp") + + assert coerce_value(definition, "hot") == "hot" + with pytest.raises(PropertyError, match="hot, warm"): + coerce_value(definition, "lukewarm") + + +def test_none_always_clears_a_value(): + """Whether it may be empty is is_required's business, checked elsewhere.""" + assert coerce_value(_Definition("number", is_required=True), None) is None + + +# --- the API --- + + +@pytest.fixture() +def service_key(client: TestClient, auth_headers: dict[str, str]) -> str: + created = client.post( + "/api/v1/integrations/credentials", + headers=auth_headers, + json={"name": "MaskanX"}, + ) + return created.json()["key"] + + +def _definition(**overrides) -> dict: + payload = { + "object_type": "contact", + "name": "budget_range", + "label": "Budget range", + "data_type": "enumeration", + "options": ["50-70 lakh", "70-90 lakh", "1 crore+"], + } + payload.update(overrides) + return payload + + +def test_a_property_can_be_created_and_listed( + client: TestClient, auth_headers: dict, +) -> None: + created = client.post( + "/api/v1/properties", headers=auth_headers, json=_definition(), + ) + assert created.status_code == 201, created.text + + listed = client.get("/api/v1/properties", headers=auth_headers).json() + assert [p["name"] for p in listed] == ["budget_range"] + assert listed[0]["options"] == ["50-70 lakh", "70-90 lakh", "1 crore+"] + + +def test_properties_can_be_filtered_by_object_type( + client: TestClient, auth_headers: dict, +) -> None: + client.post("/api/v1/properties", headers=auth_headers, json=_definition()) + client.post( + "/api/v1/properties", + headers=auth_headers, + json=_definition( + object_type="lead", name="site_visit_done", + label="Site visit done", data_type="boolean", options=[], + ), + ) + + contacts = client.get( + "/api/v1/properties?object_type=contact", headers=auth_headers, + ).json() + + assert [p["name"] for p in contacts] == ["budget_range"] + + +def test_the_same_property_twice_is_a_readable_conflict( + client: TestClient, auth_headers: dict, +) -> None: + """An agent re-running a request needs to know it already exists.""" + client.post("/api/v1/properties", headers=auth_headers, json=_definition()) + again = client.post( + "/api/v1/properties", headers=auth_headers, json=_definition(), + ) + + assert again.status_code == 409 + assert "already exists" in again.json()["error"]["message"] + + +def test_an_invalid_definition_is_refused_with_a_reason( + client: TestClient, auth_headers: dict, +) -> None: + response = client.post( + "/api/v1/properties", + headers=auth_headers, + json=_definition(name="Budget Range"), + ) + + assert response.status_code == 422 + assert "budget_range" in response.json()["error"]["message"] + + +def test_a_label_can_be_changed_without_orphaning_values( + client: TestClient, auth_headers: dict, +) -> None: + created = client.post( + "/api/v1/properties", headers=auth_headers, json=_definition(), + ).json() + + updated = client.patch( + f"/api/v1/properties/{created['id']}", + headers=auth_headers, + json={"label": "Buying budget"}, + ) + + assert updated.status_code == 200 + assert updated.json()["label"] == "Buying budget" + # The key values are stored under is untouched. + assert updated.json()["name"] == "budget_range" + + +def test_a_property_can_be_deleted(client: TestClient, auth_headers: dict) -> None: + created = client.post( + "/api/v1/properties", headers=auth_headers, json=_definition(), + ).json() + + assert ( + client.delete( + f"/api/v1/properties/{created['id']}", headers=auth_headers, + ).status_code + == 204 + ) + assert client.get("/api/v1/properties", headers=auth_headers).json() == [] + + +def test_reading_properties_needs_a_login(client: TestClient) -> None: + assert client.get("/api/v1/properties").status_code == 401 + + +# --- through an integration key, which is what MCP uses --- + + +def test_an_integration_can_create_a_property( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + created = client.post( + "/api/v1/integrations/properties", + headers={"X-Integration-Key": service_key}, + json=_definition(), + ) + + assert created.status_code == 201, created.text + # Marked as machine-made, so a human can tell which fields a + # conversation produced. + assert created.json()["created_by"].startswith("integration:") + + +def test_an_integration_gets_no_shortcut_around_validation( + client: TestClient, service_key: str, +) -> None: + response = client.post( + "/api/v1/integrations/properties", + headers={"X-Integration-Key": service_key}, + json=_definition(data_type="enumeration", options=[]), + ) + + assert response.status_code == 422 + + +def test_an_integration_property_is_visible_to_people( + client: TestClient, auth_headers: dict, service_key: str, +) -> None: + """A field created in chat has to show up in the CRM's own screens.""" + client.post( + "/api/v1/integrations/properties", + headers={"X-Integration-Key": service_key}, + json=_definition(), + ) + + listed = client.get("/api/v1/properties", headers=auth_headers).json() + + assert [p["name"] for p in listed] == ["budget_range"] + + +def test_creating_a_property_needs_a_valid_key(client: TestClient) -> None: + response = client.post("/api/v1/integrations/properties", json=_definition()) + + assert response.status_code == 401 + + +# --- values on records --- + + +def _contact(client, auth_headers, attributes): + return client.post( + "/api/v1/contacts", + headers=auth_headers, + json={ + "first_name": "Asha", + "last_name": "Menon", + "primary_email": "asha@example.com", + "attributes": attributes, + }, + ) + + +def test_a_value_is_validated_against_its_definition( + client: TestClient, auth_headers: dict, +) -> None: + client.post("/api/v1/properties", headers=auth_headers, json=_definition()) + + good = _contact(client, auth_headers, {"budget_range": "70-90 lakh"}) + assert good.status_code == 201, good.text + assert good.json()["attributes"]["budget_range"] == "70-90 lakh" + + +def test_a_value_outside_the_enumeration_is_refused( + client: TestClient, auth_headers: dict, +) -> None: + client.post("/api/v1/properties", headers=auth_headers, json=_definition()) + + response = _contact(client, auth_headers, {"budget_range": "a few rupees"}) + + assert response.status_code == 422 + assert "70-90 lakh" in response.json()["error"]["message"] + + +def test_an_undefined_property_is_refused_rather_than_stored( + client: TestClient, auth_headers: dict, +) -> None: + """A typo left in the database looks exactly like data.""" + client.post("/api/v1/properties", headers=auth_headers, json=_definition()) + + response = _contact(client, auth_headers, {"budjet_range": "70-90 lakh"}) + + assert response.status_code == 422 + assert "budjet_range" in response.json()["error"]["message"] + + +def test_attributes_stay_free_form_when_nothing_is_defined( + client: TestClient, auth_headers: dict, +) -> None: + """Adding this validation must not break workspaces already using them.""" + response = _contact(client, auth_headers, {"anything": "at all"}) + + assert response.status_code == 201 + assert response.json()["attributes"] == {"anything": "at all"} + + +def test_a_number_property_is_coerced_on_the_way_in( + client: TestClient, auth_headers: dict, +) -> None: + client.post( + "/api/v1/properties", + headers=auth_headers, + json=_definition( + name="bedrooms", label="Bedrooms", data_type="number", options=[], + ), + ) + + response = _contact(client, auth_headers, {"bedrooms": "3"}) + + assert response.json()["attributes"]["bedrooms"] == 3