From 29f3d881f79b25f7811e5d1e67f93c9ee4747782 Mon Sep 17 00:00:00 2001 From: Christian Staudt Date: Sun, 28 Jun 2026 14:45:52 +0200 Subject: [PATCH] fix(contracts): make pricing type an explicit discriminator A contract's pricing was inferred from which of two independent nullable columns (rate, fixed_price) happened to be set, so nothing prevented both from being populated. The document-import path built Contract rows directly (bypassing ContractsIntent validation) and let an LLM-extracted contract carry both a rate and a fixed price into the database. Make `type` (time_based | fixed_price) the single source of truth: - Add ContractType enum + non-null Contract.type column; is_fixed_price now derives from type. validate_pricing() requires the matching value column and clears the other, so an ambiguous row cannot be persisted. - Migration backfills type from existing data (fixed_price wins on any legacy both-set row) and nulls the off-column to clean up contradictions. - Importer derives type when absent and validates on create/update, with enum-string coercion. ContractsIntent delegates to validate_pricing(). - LLM extraction emits type; both contract editors (manual + document import) use an exclusive Time-Based/Fixed-Price toggle. Adds regression coverage for the reported both-set case. Co-authored-by: Cursor --- tuttle/app/contracts/intent.py | 9 +- tuttle/app/imports/intent.py | 27 ++++ tuttle/llm.py | 3 + ...9065_add_type_discriminator_to_contract.py | 115 ++++++++++++++++++ tuttle/model.py | 45 +++++-- tuttle/time.py | 13 ++ tuttle_tests/test_document_import.py | 94 ++++++++++++++ tuttle_tests/test_invoice.py | 3 +- ui/src/components/business/ContractsView.tsx | 18 ++- .../components/import/DocumentImportView.tsx | 38 +++++- 10 files changed, 342 insertions(+), 23 deletions(-) create mode 100644 tuttle/migrations/versions/8de1ce679065_add_type_discriminator_to_contract.py diff --git a/tuttle/app/contracts/intent.py b/tuttle/app/contracts/intent.py index ae141f97..5771b796 100644 --- a/tuttle/app/contracts/intent.py +++ b/tuttle/app/contracts/intent.py @@ -47,12 +47,13 @@ def get_default_currency(self) -> IntentResult: def _validated_save(self, contract: Contract) -> IntentResult: is_updating = contract.id is not None - has_rate = contract.rate is not None and contract.rate > 0 - has_fixed = contract.fixed_price is not None and contract.fixed_price > 0 - if not has_rate and not has_fixed: + try: + contract.validate_pricing() + except ValueError as e: return IntentResult( was_intent_successful=False, - error_msg="A contract needs either a rate or a fixed price.", + error_msg=str(e), + log_message=f"ContractsIntent._validated_save: {e}", ) try: contract.VAT_rate = normalize_vat_rate(contract.VAT_rate) diff --git a/tuttle/app/imports/intent.py b/tuttle/app/imports/intent.py index 075fb02d..49fd6d6d 100644 --- a/tuttle/app/imports/intent.py +++ b/tuttle/app/imports/intent.py @@ -2,6 +2,7 @@ import base64 import datetime +import enum from decimal import Decimal from pathlib import Path from typing import Any, Dict, List, Optional @@ -13,6 +14,7 @@ from ..core.intent_result import IntentResult from ...data_dir import get_data_dir from ...model import Address, Contact, Client, Contract, Project, Invoice, InvoiceItem +from ...time import ContractType # Fields that are internal to the import workflow, not part of the model @@ -289,6 +291,8 @@ def _save_entity( fields = _model_fields(item, model_cls) for k, v in fields.items(): setattr(entity, k, v) + if isinstance(entity, Contract): + _finalize_contract(entity, fields) summary["updated"].append(label) else: summary["linked"].append(label) @@ -313,12 +317,32 @@ def _save_entity( clean = _model_fields(fields, model_cls) entity = model_cls(**clean, **nested_objects) + if isinstance(entity, Contract): + _finalize_contract(entity, clean) session.add(entity) session.flush() _bind_ref(ref, entity.id, ref_to_id) summary["created"].append(label) +def _finalize_contract(entity: Contract, provided: dict) -> None: + """Pin a contract's pricing ``type`` and enforce the type invariant. + + If the import payload did not specify ``type`` explicitly, derive it + from whichever value column is populated. Then ``validate_pricing`` + becomes the single guard: it requires the value column for the chosen + type and clears the other, so an ambiguous contract can never be + committed regardless of what the LLM extracted. + """ + if "type" not in provided: + entity.type = ( + ContractType.fixed_price + if entity.fixed_price and not entity.rate + else ContractType.time_based + ) + entity.validate_pricing() + + def _coerce_value(value: Any, annotation: Any) -> Any: """Coerce a JSON value to the expected Python type.""" if value is None or value == "": @@ -338,6 +362,9 @@ def _coerce_value(value: Any, annotation: Any) -> Any: ): if isinstance(value, (int, float, str)): return Decimal(str(value)) + if isinstance(annotation, type) and issubclass(annotation, enum.Enum): + if isinstance(value, str): + return annotation(value) return value diff --git a/tuttle/llm.py b/tuttle/llm.py index 9c7e8c92..ab4af56d 100644 --- a/tuttle/llm.py +++ b/tuttle/llm.py @@ -197,6 +197,7 @@ class _ContactExtract(_ContactScalarExtract): # type: ignore[valid-type] Contract, include=[ "title", + "type", "rate", "fixed_price", "currency", @@ -450,6 +451,7 @@ def _map_contracts(result: ContractExtractionResult) -> List[Dict[str, Any]]: c = item.contract unit = getattr(c, "unit", None) billing_cycle = getattr(c, "billing_cycle", None) + ctype = getattr(c, "type", None) rate = getattr(c, "rate", None) vat = getattr(c, "VAT_rate", None) vat_normalized: Optional[float] = None @@ -463,6 +465,7 @@ def _map_contracts(result: ContractExtractionResult) -> List[Dict[str, Any]]: results.append( { "title": getattr(c, "title", "") or "", + "type": ctype.value if ctype else None, "rate": float(rate) if rate is not None else None, "fixed_price": float(fixed_price) if fixed_price is not None else None, "currency": getattr(c, "currency", "") or "", diff --git a/tuttle/migrations/versions/8de1ce679065_add_type_discriminator_to_contract.py b/tuttle/migrations/versions/8de1ce679065_add_type_discriminator_to_contract.py new file mode 100644 index 00000000..176f5535 --- /dev/null +++ b/tuttle/migrations/versions/8de1ce679065_add_type_discriminator_to_contract.py @@ -0,0 +1,115 @@ +"""add type discriminator to contract + +Revision ID: 8de1ce679065 +Revises: 34dd17917a18 +Create Date: 2026-06-28 14:38:25.581818 + +====================================================================== +FROZEN HISTORICAL SNAPSHOT — NOT THE SCHEMA SOURCE OF TRUTH. + +The source of truth is tuttle/model.py. This file captures the schema +DELTA from the previous revision to this point in history. It is +APPEND-ONLY: once committed, never edit it. To change the schema, edit +tuttle/model.py and run `just migrate ""` to ADD a new revision. + +Reading this file to learn the current schema is a MISTAKE — it is a +point-in-time snapshot. Read tuttle/model.py instead. +====================================================================== + +MANDATORY REVIEW CHECKLIST before committing this file: + +1. RENAMES — autogenerate emits drop_column + add_column for renames, + which DESTROYS DATA. If you intended a rename, replace the pair with + op.alter_column(, , new_column_name=). + +2. NO MODEL IMPORTS — never `from tuttle.model import ...` here. + Model classes drift over time; this script must be pinned to the + schema at this point in history. For data transformations, declare + a local sa.table(...) snapshot with only the columns this revision + touches. + +3. BATCH MODE — render_as_batch=True rebuilds tables for SQLite. After + a batch op on a table with foreign keys, verify integrity inside the + migration: op.execute("PRAGMA foreign_key_check"). + +See tuttle/migrations/README.md. +---------------------------------------------------------------------- +""" +# pyright: reportAttributeAccessIssue=false +# sqlmodel.sql.sqltypes is a submodule resolved at runtime; basedpyright +# does not statically expose `sql` as an attribute of `sqlmodel`. +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +import sqlmodel +import sqlmodel.sql.sqltypes # noqa: F401 — ensures runtime resolution of AutoString + + +revision: str = "8de1ce679065" +down_revision: Union[str, Sequence[str], None] = "34dd17917a18" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema. + + Adds the ``type`` discriminator (defaults all rows to ``time_based``), + then backfills it from existing data and removes any ambiguity: + + - A contract with a positive ``fixed_price`` becomes ``fixed_price``. + (If a legacy row had BOTH a rate and a fixed price — the bug this + revision closes — fixed_price wins, as the stronger commitment.) + - The value column that does not match the chosen type is nulled, so + no row carries both ``rate`` and ``fixed_price`` afterwards. + """ + with op.batch_alter_table("contract", schema=None) as batch_op: + batch_op.add_column( + sa.Column( + "type", + sa.Enum("time_based", "fixed_price", name="contracttype"), + server_default="time_based", + nullable=False, + ) + ) + + contract = sa.table( + "contract", + sa.column("id", sa.Integer), + sa.column("type", sa.String), + sa.column("rate", sa.Numeric), + sa.column("fixed_price", sa.Numeric), + ) + op.execute( + contract.update() + .where(contract.c.fixed_price.isnot(None)) + .where(contract.c.fixed_price > 0) + .values(type="fixed_price") + ) + op.execute( + contract.update().where(contract.c.type == "fixed_price").values(rate=None) + ) + op.execute( + contract.update() + .where(contract.c.type == "time_based") + .values(fixed_price=None) + ) + op.execute("PRAGMA foreign_key_check") + + +def downgrade() -> None: + """Downgrades are not supported. + + Tuttle is a single-user desktop app. Rolling back schema is destructive + (data in dropped columns is lost) and offers nothing over restoring a + timestamped backup from ensure_schema()'s pre-upgrade snapshot. + + If you need to iterate on a migration during development: + 1. Delete this revision file (versions/8de1ce679065_*.py) + 2. Run `just reset` to wipe ~/.tuttle + 3. Edit model.py, run `just migrate` again + """ + raise NotImplementedError( + "Downgrades are not supported. Restore from a .bak- snapshot instead." + ) diff --git a/tuttle/model.py b/tuttle/model.py index 787fb5db..2ad0698c 100644 --- a/tuttle/model.py +++ b/tuttle/model.py @@ -39,7 +39,7 @@ from .app.core.formatting import fmt_currency from .data_dir import get_data_dir from .dev import deprecated -from .time import Cycle, TimeUnit +from .time import ContractType, Cycle, TimeUnit DocumentType = Literal["invoice", "reminder"] @@ -461,14 +461,24 @@ class Contract(RpcMixin, SQLModel, table=True): foreign_key="client.id", ondelete="RESTRICT", ) + type: ContractType = Field( + description="Whether the contract is time-based (rate per unit) or fixed-price. " + "The authoritative discriminator: a contract is exactly one type.", + sa_column=sqlalchemy.Column( + sqlalchemy.Enum(ContractType), + nullable=False, + server_default=ContractType.time_based.value, + ), + default=ContractType.time_based, + ) rate: Optional[condecimal(decimal_places=2)] = Field( default=None, - description="Rate of remuneration per billing unit (for time-based contracts).", + description="Rate of remuneration per billing unit. Set iff type is time_based.", ) fixed_price: Optional[Decimal] = Field( default=None, sa_column=sqlalchemy.Column(sqlalchemy.Numeric(12, 2), nullable=True), - description="Total agreed price (for fixed-price contracts).", + description="Total agreed price. Set iff type is fixed_price.", ) is_completed: bool = Field( default=False, description="flag marking if contract has been completed" @@ -510,7 +520,7 @@ class Contract(RpcMixin, SQLModel, table=True): @property def is_fixed_price(self) -> bool: - return self.fixed_price is not None + return self.type == ContractType.fixed_price @property def unit_abbrev(self) -> str: @@ -567,10 +577,29 @@ def get_status(self, default: str = "All") -> str: # default return default - # NOTE: pydantic-v1-style @validator decorators do not run on - # SQLModel(table=True) classes. VAT_rate normalisation lives in - # ``ContractsIntent._validated_save`` (canonical write path) and - # ``normalize_vat_rate`` is used on every ingress (LLM mapping, + def validate_pricing(self) -> None: + """Enforce that the contract's pricing matches its ``type``. + + ``type`` is the single source of truth. This requires the value + column for that type and **clears the other column**, so an + ambiguous row (both ``rate`` and ``fixed_price`` set) can never be + persisted. Raises ``ValueError`` if the required value is missing. + + Called on every write path (intent save + document-import commit) + because pydantic-v1 ``@validator`` hooks do not run on + ``SQLModel(table=True)`` classes. + """ + if self.type == ContractType.fixed_price: + if not (self.fixed_price is not None and self.fixed_price > 0): + raise ValueError("A fixed-price contract needs a fixed price.") + self.rate = None + else: + if not (self.rate is not None and self.rate > 0): + raise ValueError("A time-based contract needs a rate.") + self.fixed_price = None + + # NOTE: VAT_rate normalisation lives in ``ContractsIntent._validated_save`` + # and ``normalize_vat_rate`` is used on every ingress (LLM mapping, # manual scripts). diff --git a/tuttle/time.py b/tuttle/time.py index f172f4fc..1879e4d2 100644 --- a/tuttle/time.py +++ b/tuttle/time.py @@ -2,6 +2,19 @@ import datetime +class ContractType(enum.Enum): + """How a contract is priced. The single discriminator that decides + whether a contract is time-based (a ``rate`` per unit) or fixed-price + (a single ``fixed_price``). A contract is always exactly one of these. + """ + + time_based = "time_based" + fixed_price = "fixed_price" + + def __str__(self): + return str(self.value) + + class Cycle(enum.Enum): hourly = "hourly" daily = "daily" diff --git a/tuttle_tests/test_document_import.py b/tuttle_tests/test_document_import.py index bac536a7..920843dd 100644 --- a/tuttle_tests/test_document_import.py +++ b/tuttle_tests/test_document_import.py @@ -15,6 +15,7 @@ ) from tuttle.app.imports.intent import ImportsIntent, _model_fields from tuttle.app.core.abstractions import SQLModelDataSourceMixin +from tuttle.time import ContractType @pytest.fixture @@ -297,6 +298,99 @@ def test_validation_skips_existing_entities(self, in_memory_db): assert result.was_intent_successful +class TestContractPricingInvariant: + """A contract is time-based XOR fixed-price — never both, never neither. + + Regression guard for the document-import path, which builds Contract + rows directly (bypassing ContractsIntent) and let an LLM-extracted + contract carry BOTH a rate and a fixed price into the database. + """ + + def _base_contract(self, **overrides): + data = { + "ref": "contract_1", + "title": "Ambiguous Contract", + "currency": "EUR", + "unit": "day", + "billing_cycle": "monthly", + "volume": 10, + "start_date": "2026-06-10", + "end_date": "2026-07-10", + "VAT_rate": 0.19, + "term_of_payment": 14, + } + data.update(overrides) + return data + + def test_both_rate_and_fixed_price_is_normalised(self, in_memory_db): + """The exact reported bug: an extracted contract with both a fixed + price and a day rate must persist as a single, unambiguous type.""" + intent = ImportsIntent() + data = { + "contacts": [], + "clients": [], + "contracts": [ + self._base_contract(type="fixed_price", fixed_price=2000.0, rate=1000.0) + ], + "projects": [], + "invoices": [], + } + + result = intent.commit_import(data) + assert result.was_intent_successful + + with Session(in_memory_db) as session: + c = session.exec(select(Contract)).one() + assert c.is_fixed_price + assert c.fixed_price == Decimal("2000") + # The contradictory rate must have been cleared. + assert c.rate is None + + def test_type_derived_when_absent(self, in_memory_db): + """If the LLM omits ``type``, it is derived from the value columns.""" + intent = ImportsIntent() + data = { + "contacts": [], + "clients": [], + "contracts": [self._base_contract(fixed_price=5000.0)], + "projects": [], + "invoices": [], + } + + result = intent.commit_import(data) + assert result.was_intent_successful + + with Session(in_memory_db) as session: + c = session.exec(select(Contract)).one() + assert c.is_fixed_price + assert c.rate is None + + def test_validate_pricing_clears_off_column(self): + """The model method is the single source of truth and self-heals.""" + c = Contract( + title="X", + currency="EUR", + start_date=datetime.date(2026, 1, 1), + type=ContractType.fixed_price, + fixed_price=Decimal("2000"), + rate=Decimal("1000"), + ) + c.validate_pricing() + assert c.rate is None + assert c.fixed_price == Decimal("2000") + + def test_validate_pricing_requires_a_value(self): + """A fixed-price contract with no fixed price is rejected.""" + c = Contract( + title="X", + currency="EUR", + start_date=datetime.date(2026, 1, 1), + type=ContractType.fixed_price, + ) + with pytest.raises(ValueError): + c.validate_pricing() + + class TestLlmInvoiceSchema: def test_extraction_schema_includes_invoices(self): """Verify the extraction schema has an invoices field.""" diff --git a/tuttle_tests/test_invoice.py b/tuttle_tests/test_invoice.py index 9831f645..a92f5cee 100644 --- a/tuttle_tests/test_invoice.py +++ b/tuttle_tests/test_invoice.py @@ -18,7 +18,7 @@ InvoiceNote, Project, ) -from tuttle.time import Cycle, TimeUnit +from tuttle.time import ContractType, Cycle, TimeUnit from tuttle.calendar import get_month_start_end @@ -233,6 +233,7 @@ def _build_fixed_price_project(fixed_price, tag: str) -> Project: title="Fixed Price Contract", client=client, start_date=datetime.date(2022, 1, 1), + type=ContractType.fixed_price, fixed_price=fixed_price, currency="EUR", VAT_rate=Decimal("0.19"), diff --git a/ui/src/components/business/ContractsView.tsx b/ui/src/components/business/ContractsView.tsx index b3cbcd8a..ec16b08b 100644 --- a/ui/src/components/business/ContractsView.tsx +++ b/ui/src/components/business/ContractsView.tsx @@ -77,8 +77,9 @@ export function ContractsView() { const contract: Record = { title: data.title, client_id: data.clientId, - fixed_price: data.fixedPrice || null, - rate: data.rate || null, + type: data.type, + fixed_price: data.type === "fixed_price" ? (data.fixedPrice || null) : null, + rate: data.type === "time_based" ? (data.rate || null) : null, currency: data.currency, unit: data.unit, billing_cycle: data.billingCycle, @@ -397,6 +398,7 @@ function RelatedCard({ icon, count, label, onClick }: { icon: React.ReactNode; c interface ContractFormData { title: string; clientId: number | null; + type: PricingMode; fixedPrice: number | null; rate: number | null; currency: string; @@ -423,11 +425,15 @@ function ContractForm({ contract, clients, defaultCurrency, onSave, onCancel, er }) { const cl = contract ? subEntity(contract, "client") : null; const initFixed = contract ? (num(contract, "fixed_price") || null) : null; - const [pricingMode, setPricingMode] = useState(initFixed ? "fixed_price" : "time_based"); + const initType: PricingMode = contract + ? ((str(contract, "type") as PricingMode) || (initFixed ? "fixed_price" : "time_based")) + : "time_based"; + const [pricingMode, setPricingMode] = useState(initType); const [form, setForm] = useState(() => { if (contract) return { title: str(contract, "title"), clientId: cl?.id ?? null, + type: initType, fixedPrice: initFixed, rate: num(contract, "rate") || null, currency: str(contract, "currency") || defaultCurrency, @@ -446,7 +452,7 @@ function ContractForm({ contract, clients, defaultCurrency, onSave, onCancel, er unitsPerWorkday: num(contract, "units_per_workday") || 8, }; return { - title: "", clientId: null, fixedPrice: null, rate: null, currency: defaultCurrency, + title: "", clientId: null, type: "time_based", fixedPrice: null, rate: null, currency: defaultCurrency, unit: "hour", billingCycle: "monthly", volume: null, vatRate: 0.19, signatureDate: "", startDate: "", endDate: "", termOfPayment: 31, unitsPerWorkday: 8, }; @@ -467,9 +473,9 @@ function ContractForm({ contract, clients, defaultCurrency, onSave, onCancel, er setPricingMode(mode); setValidationError(null); if (mode === "fixed_price") { - setForm((prev) => ({ ...prev, rate: null })); + setForm((prev) => ({ ...prev, type: mode, rate: null })); } else { - setForm((prev) => ({ ...prev, fixedPrice: null })); + setForm((prev) => ({ ...prev, type: mode, fixedPrice: null })); } } diff --git a/ui/src/components/import/DocumentImportView.tsx b/ui/src/components/import/DocumentImportView.tsx index 457703e7..3cdf1da5 100644 --- a/ui/src/components/import/DocumentImportView.tsx +++ b/ui/src/components/import/DocumentImportView.tsx @@ -1064,15 +1064,45 @@ function ContractCard({ item, onUpdate, existing, clients, requiredFields, enumF const req = (f: string) => requiredFields.includes(f); + // ``type`` is the single source of truth for pricing. Derive a sensible + // initial value when the LLM did not set it explicitly, then switching + // type clears the other value column so we never commit both. + const ctype: string = d.type || (d.fixed_price && !d.rate ? "fixed_price" : "time_based"); + function switchType(next: string) { + onUpdate({ + ...item, + data: { + ...d, + type: next, + rate: next === "time_based" ? d.rate : null, + fixed_price: next === "fixed_price" ? d.fixed_price : null, + }, + }); + } + const isFixed = ctype === "fixed_price"; + return (
+
+ {["time_based", "fixed_price"].map((mode) => ( + + ))} +
set("title", v)} required={req("title")} /> - set("rate", v ? parseFloat(v) : null)} required={req("rate")} /> + {isFixed ? ( + set("fixed_price", v ? parseFloat(v) : null)} /> + ) : ( + set("rate", v ? parseFloat(v) : null)} /> + )} set("currency", v)} required={req("currency")} /> - set("unit", v)} required={req("unit")} options={enumFields.unit} /> - set("billing_cycle", v)} required={req("billing_cycle")} options={enumFields.billing_cycle} /> - set("volume", v ? parseInt(v) : null)} required={req("volume")} /> + {!isFixed && set("unit", v)} required={req("unit")} options={enumFields.unit} />} + {!isFixed && set("billing_cycle", v)} required={req("billing_cycle")} options={enumFields.billing_cycle} />} + {!isFixed && set("volume", v ? parseInt(v) : null)} required={req("volume")} />}
set("signature_date", v)} type="date" required={req("signature_date")} />