From 3cdd7d9eabd9bd1ccba3896f4a1413c580beb04c Mon Sep 17 00:00:00 2001 From: Carlos Escalante Date: Wed, 10 Jun 2026 14:45:46 -0600 Subject: [PATCH] Constraint and query pass: FK delete rules, grouped queries, tiebreakers - FK rules via migration 7884505b16b3 (verified on dev with a rolled- back probe): transactions/recurring items SET NULL on category delete (this also fixes the 500 that DELETE /categories raised for in-use categories), water readings CASCADE with their receipt. Models declare ondelete to stay in sync. (ARCH-09, BE-11) - compute_actuals_by_source: 3 grouped queries replace 10 single- aggregate round-trips; behavior pinned by the existing cycle suite. (BE-21) - compute_cc_by_category prefetches category names (ARCH-15); agent pension latest-per-fund resolved in SQL (BE-04); municipal water consumption batched into one grouped query (BE-05). - Deterministic pagination: date DESC, id DESC on all transaction listings. (ARCH-12) - New agent-tool tests (session binding, JSON-safe output, batched queries): 64 tests green. Co-Authored-By: Claude Fable 5 --- .../versions/7884505b16b3_fk_delete_rules.py | 61 +++++++ backend/app/agent/tools.py | 43 +++-- backend/app/api/v1/endpoints/salarios.py | 2 +- backend/app/api/v1/endpoints/transactions.py | 4 +- backend/app/models/models.py | 12 +- backend/app/services/budget_projection.py | 171 ++++++------------ backend/tests/test_agent_tools.py | 151 ++++++++++++++++ 7 files changed, 316 insertions(+), 128 deletions(-) create mode 100644 backend/alembic/versions/7884505b16b3_fk_delete_rules.py create mode 100644 backend/tests/test_agent_tools.py diff --git a/backend/alembic/versions/7884505b16b3_fk_delete_rules.py b/backend/alembic/versions/7884505b16b3_fk_delete_rules.py new file mode 100644 index 0000000..7307141 --- /dev/null +++ b/backend/alembic/versions/7884505b16b3_fk_delete_rules.py @@ -0,0 +1,61 @@ +"""fk delete rules + +Revision ID: 7884505b16b3 +Revises: 7f567c8bafdb +Create Date: 2026-06-10 13:50:39.153585 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +import sqlmodel + + +# revision identifiers, used by Alembic. +revision: str = '7884505b16b3' +down_revision: Union[str, Sequence[str], None] = '7f567c8bafdb' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Add delete rules to the three FKs (review ARCH-09/BE-11). + + Transactions and recurring items outlive their category (SET NULL); + water readings die with their receipt (CASCADE). Constraint names match + both pre-Alembic databases and fresh baseline builds (Postgres default + naming). + """ + op.drop_constraint("transaction_category_id_fkey", "transaction", type_="foreignkey") + op.create_foreign_key( + "transaction_category_id_fkey", "transaction", "category", + ["category_id"], ["id"], ondelete="SET NULL", + ) + op.drop_constraint("recurringitem_category_id_fkey", "recurringitem", type_="foreignkey") + op.create_foreign_key( + "recurringitem_category_id_fkey", "recurringitem", "category", + ["category_id"], ["id"], ondelete="SET NULL", + ) + op.drop_constraint("watermeterreading_receipt_id_fkey", "watermeterreading", type_="foreignkey") + op.create_foreign_key( + "watermeterreading_receipt_id_fkey", "watermeterreading", "municipalreceipt", + ["receipt_id"], ["id"], ondelete="CASCADE", + ) + +def downgrade() -> None: + op.drop_constraint("watermeterreading_receipt_id_fkey", "watermeterreading", type_="foreignkey") + op.create_foreign_key( + "watermeterreading_receipt_id_fkey", "watermeterreading", "municipalreceipt", + ["receipt_id"], ["id"], + ) + op.drop_constraint("recurringitem_category_id_fkey", "recurringitem", type_="foreignkey") + op.create_foreign_key( + "recurringitem_category_id_fkey", "recurringitem", "category", + ["category_id"], ["id"], + ) + op.drop_constraint("transaction_category_id_fkey", "transaction", type_="foreignkey") + op.create_foreign_key( + "transaction_category_id_fkey", "transaction", "category", + ["category_id"], ["id"], + ) diff --git a/backend/app/agent/tools.py b/backend/app/agent/tools.py index 11c535d..d66910d 100644 --- a/backend/app/agent/tools.py +++ b/backend/app/agent/tools.py @@ -266,16 +266,27 @@ def get_pension_snapshots( ) -> list[dict]: """Pension fund snapshots. Each snapshot covers a period with balances, contributions, returns, fees and the ending balance (saldo_final).""" - q = select(PensionSnapshot).order_by(col(PensionSnapshot.period_end).desc()) + if latest_only: + # Latest snapshot per fund resolved in SQL instead of scanning all rows + latest = ( + select( + PensionSnapshot.fund.label("fund"), + func.max(PensionSnapshot.period_end).label("period_end"), + ) + .group_by(PensionSnapshot.fund) + .subquery() + ) + q = select(PensionSnapshot).join( + latest, + (PensionSnapshot.fund == latest.c.fund) + & (PensionSnapshot.period_end == latest.c.period_end), + ) + else: + q = select(PensionSnapshot) + q = q.order_by(col(PensionSnapshot.period_end).desc()) if fund: q = q.where(PensionSnapshot.fund == fund) rows = _s().exec(q).all() - if latest_only: - seen: dict[str, PensionSnapshot] = {} - for r in rows: - if r.fund.value not in seen: - seen[r.fund.value] = r - rows = list(seen.values()) return [ { "fund": r.fund.value, @@ -322,11 +333,21 @@ def get_municipal_receipts( q = q.where(MunicipalReceipt.account == account) q = q.limit(limit) rows = _s().exec(q).all() + # One grouped query for all receipts instead of one per receipt + consumption: dict[int, float] = {} + ids = [r.id for r in rows if r.id is not None] + if ids: + grouped = _s().exec( + select( + WaterMeterReading.receipt_id, + func.sum(WaterMeterReading.consumption_m3), + ) + .where(col(WaterMeterReading.receipt_id).in_(ids)) + .group_by(WaterMeterReading.receipt_id) + ).all() + consumption = {rid: float(total) for rid, total in grouped} out: list[dict] = [] for r in rows: - readings = _s().exec( - select(WaterMeterReading).where(WaterMeterReading.receipt_id == r.id) - ).all() out.append( { "id": r.id, @@ -338,7 +359,7 @@ def get_municipal_receipts( "interests": float(r.interests), "iva": float(r.iva), "total": float(r.total), - "water_consumption_m3": float(sum(w.consumption_m3 for w in readings)), + "water_consumption_m3": consumption.get(r.id, 0.0), } ) return out diff --git a/backend/app/api/v1/endpoints/salarios.py b/backend/app/api/v1/endpoints/salarios.py index c9c0ba4..d83753e 100644 --- a/backend/app/api/v1/endpoints/salarios.py +++ b/backend/app/api/v1/endpoints/salarios.py @@ -31,7 +31,7 @@ def list_salarios( query = ( select(Transaction) .where(col(Transaction.transaction_type).in_(SALARIO_TYPES)) - .order_by(col(Transaction.date).desc()) + .order_by(col(Transaction.date).desc(), col(Transaction.id).desc()) .offset(offset) .limit(limit) ) diff --git a/backend/app/api/v1/endpoints/transactions.py b/backend/app/api/v1/endpoints/transactions.py index 6d2fd35..ee8dba1 100644 --- a/backend/app/api/v1/endpoints/transactions.py +++ b/backend/app/api/v1/endpoints/transactions.py @@ -93,7 +93,7 @@ def list_transactions( Transaction.date >= datetime.fromisoformat(start_date), Transaction.date < datetime.fromisoformat(end_date), ) - query = query.order_by(col(Transaction.date).desc()).offset(offset).limit(limit) + query = query.order_by(col(Transaction.date).desc(), col(Transaction.id).desc()).offset(offset).limit(limit) return session.exec(query).all() @@ -166,7 +166,7 @@ def recent_transactions( query = ( select(Transaction) .where(Transaction.source == TransactionSource.CREDIT_CARD) - .order_by(col(Transaction.date).desc()) + .order_by(col(Transaction.date).desc(), col(Transaction.id).desc()) .limit(limit) ) return session.exec(query).all() diff --git a/backend/app/models/models.py b/backend/app/models/models.py index 048e8bb..1990e8f 100644 --- a/backend/app/models/models.py +++ b/backend/app/models/models.py @@ -157,7 +157,9 @@ class TransactionBase(SQLModel): source: TransactionSource = TransactionSource.CREDIT_CARD bank: Bank = Bank.BAC notes: Optional[str] = None - category_id: Optional[int] = Field(default=None, foreign_key="category.id") + category_id: Optional[int] = Field( + default=None, foreign_key="category.id", ondelete="SET NULL" + ) deferred_to_next_cycle: bool = Field( default=False, sa_column_kwargs={"server_default": "false"} ) @@ -273,7 +275,9 @@ class RecurringItemBase(SQLModel): default=None, sa_column=Column(JSON, nullable=True), ) - category_id: Optional[int] = Field(default=None, foreign_key="category.id") + category_id: Optional[int] = Field( + default=None, foreign_key="category.id", ondelete="SET NULL" + ) is_active: bool = True notes: Optional[str] = None @@ -470,7 +474,9 @@ class WaterMeterReadingBase(SQLModel): alcant_sanitario: Money = Field(default=Decimal("0"), max_digits=15, decimal_places=2) iva: Money = Field(default=Decimal("0"), max_digits=15, decimal_places=2) is_historical: bool = False - receipt_id: Optional[int] = Field(default=None, foreign_key="municipalreceipt.id") + receipt_id: Optional[int] = Field( + default=None, foreign_key="municipalreceipt.id", ondelete="CASCADE" + ) class WaterMeterReading(WaterMeterReadingBase, table=True): diff --git a/backend/app/services/budget_projection.py b/backend/app/services/budget_projection.py index 829295b..4685c83 100644 --- a/backend/app/services/budget_projection.py +++ b/backend/app/services/budget_projection.py @@ -124,115 +124,65 @@ def compute_actuals_by_source( amount_crc = get_converted_amount_expr(session) + def _grouped(*where) -> dict[tuple, tuple[float, int]]: + """{(source, type): (total_crc, count)} for matching transactions.""" + rows = session.exec( + select( + Transaction.source, + Transaction.transaction_type, + func.coalesce(func.sum(amount_crc), 0), + func.count(), + ) + .where(*where) + .group_by(Transaction.source, Transaction.transaction_type) + ).all() + return {(s, t): (float(total), cnt) for s, t, total, cnt in rows} + + # Three grouped queries replace the previous ten single-aggregate ones. + cc_now = _grouped( + Transaction.date >= cc_start, + Transaction.date < cc_end, + Transaction.source == TransactionSource.CREDIT_CARD, + Transaction.deferred_to_next_cycle == False, # noqa: E712 + ) + cc_deferred = _grouped( + Transaction.date >= prev_start, + Transaction.date < prev_end, + Transaction.source == TransactionSource.CREDIT_CARD, + Transaction.deferred_to_next_cycle == True, # noqa: E712 + ) + non_cc = _grouped( + Transaction.date >= cal_start, + Transaction.date < cal_end, + Transaction.source != TransactionSource.CREDIT_CARD, + ) + results = {} for source in TransactionSource: if source == TransactionSource.CREDIT_CARD: - start, end = cc_start, cc_end - # Normal transactions in this cycle (not deferred) - compra_normal = session.exec( - select(func.coalesce(func.sum(amount_crc), 0)).where( - Transaction.date >= start, - Transaction.date < end, - Transaction.source == source, - Transaction.transaction_type == TransactionType.COMPRA, - Transaction.deferred_to_next_cycle == False, # noqa: E712 - ) - ).one() - # Deferred from previous cycle - compra_deferred = session.exec( - select(func.coalesce(func.sum(amount_crc), 0)).where( - Transaction.date >= prev_start, - Transaction.date < prev_end, - Transaction.source == source, - Transaction.transaction_type == TransactionType.COMPRA, - Transaction.deferred_to_next_cycle == True, # noqa: E712 - ) - ).one() - compra = float(compra_normal) + float(compra_deferred) - - dev_normal = session.exec( - select(func.coalesce(func.sum(amount_crc), 0)).where( - Transaction.date >= start, - Transaction.date < end, - Transaction.source == source, - Transaction.transaction_type == TransactionType.DEVOLUCION, - Transaction.deferred_to_next_cycle == False, # noqa: E712 - ) - ).one() - dev_deferred = session.exec( - select(func.coalesce(func.sum(amount_crc), 0)).where( - Transaction.date >= prev_start, - Transaction.date < prev_end, - Transaction.source == source, - Transaction.transaction_type == TransactionType.DEVOLUCION, - Transaction.deferred_to_next_cycle == True, # noqa: E712 - ) - ).one() - devolucion = float(dev_normal) + float(dev_deferred) - - count_normal = session.exec( - select(func.count()).where( - Transaction.date >= start, - Transaction.date < end, - Transaction.source == source, - col(Transaction.transaction_type).notin_(INCOME_TYPES), - Transaction.deferred_to_next_cycle == False, # noqa: E712 - ) - ).one() - count_deferred = session.exec( - select(func.count()).where( - Transaction.date >= prev_start, - Transaction.date < prev_end, - Transaction.source == source, - col(Transaction.transaction_type).notin_(INCOME_TYPES), - Transaction.deferred_to_next_cycle == True, # noqa: E712 - ) - ).one() - count = count_normal + count_deferred - - results[source.value] = { - "source": source.value, - "total_compra": compra, - "total_devolucion": devolucion, - "net": compra - devolucion, - "count": count, - } + buckets = (cc_now, cc_deferred) else: - # Cash / Transfer: calendar month, no deferred logic - compra = session.exec( - select(func.coalesce(func.sum(amount_crc), 0)).where( - Transaction.date >= cal_start, - Transaction.date < cal_end, - Transaction.source == source, - Transaction.transaction_type == TransactionType.COMPRA, - ) - ).one() - devolucion = session.exec( - select(func.coalesce(func.sum(amount_crc), 0)).where( - Transaction.date >= cal_start, - Transaction.date < cal_end, - Transaction.source == source, - Transaction.transaction_type == TransactionType.DEVOLUCION, - ) - ).one() - count = session.exec( - select(func.count()).where( - Transaction.date >= cal_start, - Transaction.date < cal_end, - Transaction.source == source, - col(Transaction.transaction_type).notin_(INCOME_TYPES), - ) - ).one() - - compra_val = float(compra) - devolucion_val = float(devolucion) - results[source.value] = { - "source": source.value, - "total_compra": compra_val, - "total_devolucion": devolucion_val, - "net": compra_val - devolucion_val, - "count": count, - } + buckets = (non_cc,) + compra = sum( + b.get((source, TransactionType.COMPRA), (0.0, 0))[0] for b in buckets + ) + devolucion = sum( + b.get((source, TransactionType.DEVOLUCION), (0.0, 0))[0] + for b in buckets + ) + count = sum( + cnt + for b in buckets + for (s, t), (_total, cnt) in b.items() + if s == source and t not in INCOME_TYPES + ) + results[source.value] = { + "source": source.value, + "total_compra": compra, + "total_devolucion": devolucion, + "net": compra - devolucion, + "count": count, + } return results @@ -380,18 +330,17 @@ def compute_cc_by_category( ).all() ) - # Resolve category names + # Resolve category names in one query instead of one per category from app.models.models import Category + names = { + c.id: c.name for c in session.exec(select(Category)).all() + } result = [] for cat_id, amount in totals.items(): if amount <= 0: continue - if cat_id is not None: - cat = session.get(Category, cat_id) - name = cat.name if cat else "Sin categoría" - else: - name = "Sin categoría" + name = names.get(cat_id, "Sin categoría") if cat_id is not None else "Sin categoría" result.append({"category_name": name, "amount": round(amount, 2)}) return sorted(result, key=lambda x: x["amount"], reverse=True) diff --git a/backend/tests/test_agent_tools.py b/backend/tests/test_agent_tools.py new file mode 100644 index 0000000..3233f8d --- /dev/null +++ b/backend/tests/test_agent_tools.py @@ -0,0 +1,151 @@ +"""Agent tool coverage: session binding, JSON-safe output (no Decimal leaks), +and the batched queries introduced in Phase 2.""" + +import json +from datetime import date, datetime +from decimal import Decimal + +import pytest + +from app.agent import tools +from app.agent.tools import reset_session, set_session +from app.models.models import ( + Account, + AccountType, + Bank, + Currency, + MunicipalReceipt, + PensionSnapshot, + Transaction, + WaterMeterReading, +) + + +@pytest.fixture() +def bound_session(session): + token = set_session(session) + yield session + reset_session(token) + + +def make_snapshot(fund: Bank, start: date, end: date, saldo: str) -> PensionSnapshot: + zero = Decimal("0") + return PensionSnapshot( + fund=fund, + contract_number="C-1", + period_start=start, + period_end=end, + saldo_anterior=zero, + aportes=zero, + rendimientos=zero, + retiros=zero, + traslados=zero, + comision=zero, + correccion=zero, + bonificacion=zero, + saldo_final=Decimal(saldo), + source_filename="t.pdf", + ) + + +def make_receipt(period: str) -> MunicipalReceipt: + zero = Decimal("0") + return MunicipalReceipt( + receipt_date=date(2026, 4, 1), + due_date=date(2026, 4, 15), + period=period, + account="A1", + finca="F1", + holder_name="x", + holder_cedula="x", + holder_address="x", + subtotal=Decimal("100"), + interests=zero, + iva=Decimal("13"), + total=Decimal("113"), + source_filename="r.pdf", + ) + + +def test_unbound_session_raises(session): + with pytest.raises(LookupError): + tools.get_accounts() + + +def test_accounts_output_is_json_safe(bound_session): + bound_session.add( + Account( + bank=Bank.BAC, + currency=Currency.CRC, + label="Main", + balance=Decimal("1234.56"), + account_type=AccountType.BANK, + ) + ) + bound_session.commit() + out = tools.get_accounts() + json.dumps(out) # would raise on Decimal leakage + assert out[0]["balance"] == 1234.56 + + +def test_pension_latest_only_per_fund(bound_session): + bound_session.add( + make_snapshot(Bank.FCL, date(2026, 1, 1), date(2026, 1, 31), "100") + ) + bound_session.add( + make_snapshot(Bank.FCL, date(2026, 2, 1), date(2026, 2, 28), "200") + ) + bound_session.add( + make_snapshot(Bank.ROP, date(2026, 1, 1), date(2026, 1, 31), "900") + ) + bound_session.commit() + + out = tools.get_pension_snapshots(latest_only=True) + json.dumps(out) + assert len(out) == 2 + by_fund = {o["fund"]: o for o in out} + assert by_fund["FCL"]["saldo_final"] == 200.0 # latest period wins + assert by_fund["ROP"]["saldo_final"] == 900.0 + + full = tools.get_pension_snapshots(latest_only=False) + assert len(full) == 3 + + +def test_municipal_receipts_batched_consumption(bound_session): + r1, r2 = make_receipt("2026-03"), make_receipt("2026-04") + r2.account = "A2" + bound_session.add(r1) + bound_session.add(r2) + bound_session.commit() + bound_session.refresh(r1) + bound_session.refresh(r2) + for rid, m3 in ((r1.id, "10.5"), (r1.id, "4.5"), (r2.id, "7")): + bound_session.add( + WaterMeterReading( + meter_id="7335", + period=f"p{m3}", + consumption_m3=Decimal(m3), + receipt_id=rid, + ) + ) + bound_session.commit() + + out = tools.get_municipal_receipts() + json.dumps(out) + by_account = {o["account"]: o for o in out} + assert by_account["A1"]["water_consumption_m3"] == 15.0 + assert by_account["A2"]["water_consumption_m3"] == 7.0 + + +def test_recent_transactions_json_safe(bound_session): + bound_session.add( + Transaction( + amount=Decimal("42.42"), + merchant="M", + date=datetime(2026, 4, 1), + ) + ) + bound_session.commit() + out = tools.get_recent_transactions() + json.dumps(out) + assert out[0]["amount"] == 42.42