From f3b23e6f572ebb9ffc001db254a8a1a42757cb15 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 09:22:16 +0100 Subject: [PATCH 01/24] feat(currency): narrow currency core, real-statement PDF fixes, and FX rates Implements Phase 1 (narrow currency core) and the remaining Phase 2 defects from the v0.2 plan, on top of work already staged on this branch. Currency core: - Settings.base_currency, an ISO-4217 SUPPORTED_CURRENCIES dict replacing the GBP-only reject, and per-currency minor-unit exponents (JPY=0) applied consistently through a single domain.money.minor_units helper. - fx_rates table (migration 0004), FxRateRepository with at-or-before/inverse lookup, domain.fx.to_base, and `pfa fx set/fetch/list` plus /fx/* endpoints. Rates cross every boundary (API request/response, Frankfurter fetch) as decimal strings, never float. - Analytics is per-currency throughout (monthly_summary, category/merchant totals, cashflow, budgets, current_cash), with a mixed GBP/INR regression test. - A row's currency is validated against its destination account's at preview time (blocking issue, not a crash) and account currency lookup during preview no longer creates an account as a side effect. Statement extraction: - Fixed: an AMEX payment's own-line "CR" marker was silently dropped during extraction, so the statement's default debit-positive convention flipped it to spending. The marker now attaches to its row and marks the direction explicit, which the sign convention now respects everywhere (PDF debit/credit columns, CSV, inline/own-line CR). - Fixed: parse_amount used a naive `* 100` instead of the currency's minor-unit exponent and rounding, corrupting non-2dp currencies (JPY) and truncating instead of rounding. - Fixed: year-less dates ("Jul31") always took today's year, even during a replay of an old statement. Batches now infer the statement's year from any other year-bearing date in the same file and normalize every date up front, with a visible warning when no date carries a year at all. - AMEX's duplicate "Date" column no longer leaks into the transaction description. - Barclaycard two-column layout clustering and Money Out/Money In PDF header coverage. Also fixes: ruff/format/mypy clean (was failing all three), restores a deleted PDF header-alias regression test, and adds fetch_and_store_fx_rates coverage that proves Frankfurter rates are parsed to Decimal rather than round-tripped through float. --- alembic/versions/0004_fx_rates.py | 32 ++++ src/pfa/analytics/service.py | 92 +++++++--- src/pfa/api/app.py | 130 +++++++++++++- src/pfa/cli/app.py | 96 +++++++++- src/pfa/config.py | 1 + src/pfa/db/models.py | 16 ++ src/pfa/db/repositories.py | 141 ++++++++++++++- src/pfa/db/unit_of_work.py | 2 + src/pfa/domain/fx.py | 64 +++++++ src/pfa/domain/money.py | 40 ++++- src/pfa/ingestion/batches.py | 113 ++++++++++-- src/pfa/ingestion/candidates.py | 110 +++++++++++- src/pfa/ingestion/dialects.py | 67 +++++++ src/pfa/ingestion/extractors/csv.py | 16 +- src/pfa/ingestion/extractors/ocr.py | 5 + src/pfa/ingestion/extractors/pdf.py | 225 ++++++++++++++++++------ src/pfa/ingestion/service.py | 40 ++++- src/pfa/planning/service.py | 38 ++-- src/pfa/services/fx.py | 65 +++++++ src/pfa/services/review.py | 24 ++- tests/integration/test_api.py | 42 +++++ tests/integration/test_cli.py | 20 +++ tests/unit/test_financial_invariants.py | 62 ++++++- tests/unit/test_fx.py | 151 ++++++++++++++++ tests/unit/test_money.py | 17 ++ tests/unit/test_pdf_extractor.py | 51 +++++- tests/unit/test_planning_scenarios.py | 3 +- tests/unit/test_statement_candidates.py | 2 +- 28 files changed, 1505 insertions(+), 160 deletions(-) create mode 100644 alembic/versions/0004_fx_rates.py create mode 100644 src/pfa/domain/fx.py create mode 100644 src/pfa/ingestion/dialects.py create mode 100644 src/pfa/services/fx.py create mode 100644 tests/unit/test_fx.py diff --git a/alembic/versions/0004_fx_rates.py b/alembic/versions/0004_fx_rates.py new file mode 100644 index 0000000..2967109 --- /dev/null +++ b/alembic/versions/0004_fx_rates.py @@ -0,0 +1,32 @@ +"""fx rates table""" + +import sqlalchemy as sa +from alembic import op + +revision = "0004_fx_rates" +down_revision = "0003_batch_amount_sign" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "fx_rates", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("base_currency", sa.String(length=3), nullable=False), + sa.Column("quote_currency", sa.String(length=3), nullable=False), + sa.Column("rate", sa.String(length=32), nullable=False), + sa.Column("effective_at", sa.Date(), nullable=False), + sa.Column("source", sa.String(length=50), nullable=False), + sa.Column("retrieved_at", sa.DateTime(), nullable=False), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint( + "base_currency", "quote_currency", "effective_at", name="uq_fx_rates_base_quote_date" + ), + ) + op.create_index("ix_fx_rates_effective_at", "fx_rates", ["effective_at"]) + + +def downgrade() -> None: + op.drop_index("ix_fx_rates_effective_at", table_name="fx_rates") + op.drop_table("fx_rates") diff --git a/src/pfa/analytics/service.py b/src/pfa/analytics/service.py index 3e9065c..e734ff7 100644 --- a/src/pfa/analytics/service.py +++ b/src/pfa/analytics/service.py @@ -70,9 +70,17 @@ def __init__( self.budgets = budgets self.goals = goals - def monthly_summary(self, period: date) -> MonthlySummary: + def _filter_currency( + self, transactions: list[TransactionModel], currency: str + ) -> list[TransactionModel]: + curr = currency.upper() + return [t for t in transactions if (getattr(t, "currency", None) or "GBP").upper() == curr] + + def monthly_summary(self, period: date, currency: str = "GBP") -> MonthlySummary: start, end = month_bounds(period) - rows = self.transactions.between(start, end) + all_rows = self.transactions.between(start, end) + curr = currency.upper() + rows = self._filter_currency(all_rows, curr) income = sum(row.amount_minor for row in rows if row.kind == TransactionKind.INCOME.value) spending = sum(_spending(row) for row in rows) essential = sum(_spending(row) for row in rows if row.category in _ESSENTIAL) @@ -102,6 +110,7 @@ def monthly_summary(self, period: date) -> MonthlySummary: ) return MonthlySummary( period=start.strftime("%Y-%m"), + currency=curr, income_minor=income, spending_minor=spending, essential_spending_minor=essential, @@ -114,8 +123,9 @@ def monthly_summary(self, period: date) -> MonthlySummary: transaction_count=len(rows), ) - def category_spending(self, period: date) -> list[CategoryTotal]: - rows = self.transactions.between(*month_bounds(period)) + def category_spending(self, period: date, currency: str = "GBP") -> list[CategoryTotal]: + all_rows = self.transactions.between(*month_bounds(period)) + rows = self._filter_currency(all_rows, currency) totals: dict[str, list[int]] = defaultdict(lambda: [0, 0]) for row in rows: value = _spending(row) @@ -127,8 +137,9 @@ def category_spending(self, period: date) -> list[CategoryTotal]: for key, value in sorted(totals.items(), key=lambda item: -item[1][0]) ] - def merchant_spending(self, period: date) -> list[MerchantTotal]: - rows = self.transactions.between(*month_bounds(period)) + def merchant_spending(self, period: date, currency: str = "GBP") -> list[MerchantTotal]: + all_rows = self.transactions.between(*month_bounds(period)) + rows = self._filter_currency(all_rows, currency) totals: dict[str, list[int]] = defaultdict(lambda: [0, 0]) for row in rows: value = _spending(row) @@ -140,10 +151,12 @@ def merchant_spending(self, period: date) -> list[MerchantTotal]: for key, value in sorted(totals.items(), key=lambda item: -item[1][0]) ] - def compare_periods(self, current: date, previous: date | None = None) -> PeriodComparison: + def compare_periods( + self, current: date, previous: date | None = None, currency: str = "GBP" + ) -> PeriodComparison: previous = previous or (current.replace(day=1) - timedelta(days=1)) - current_summary = self.monthly_summary(current) - previous_summary = self.monthly_summary(previous) + current_summary = self.monthly_summary(current, currency=currency) + previous_summary = self.monthly_summary(previous, currency=currency) fields = ( "income_minor", "spending_minor", @@ -159,19 +172,31 @@ def compare_periods(self, current: date, previous: date | None = None) -> Period current=current_summary, previous=previous_summary, changes_minor=changes ) - def largest_transactions(self, period: date, limit: int = 10) -> list[TransactionModel]: - rows = self.transactions.between(*month_bounds(period)) + def largest_transactions( + self, period: date, limit: int = 10, currency: str = "GBP" + ) -> list[TransactionModel]: + all_rows = self.transactions.between(*month_bounds(period)) + rows = self._filter_currency(all_rows, currency) return sorted(rows, key=lambda row: _spending(row), reverse=True)[:limit] - def recurring_payments(self) -> list[dict[str, object]]: - return detect_recurring(self.transactions.all()) + def recurring_payments(self, currency: str = "GBP") -> list[dict[str, object]]: + all_rows = self.transactions.all() + rows = self._filter_currency(all_rows, currency) + return detect_recurring(rows) - def budget_status(self, period: date) -> list[BudgetStatus]: + def budget_status(self, period: date, currency: str = "GBP") -> list[BudgetStatus]: + curr = currency.upper() actual_by_category = { - item.category: item.total_minor for item in self.category_spending(period) + item.category: item.total_minor + for item in self.category_spending(period, currency=curr) } statuses = [] - for budget in self.budgets.active_on(month_bounds(period)[0]): + active_budgets = [ + b + for b in self.budgets.active_on(month_bounds(period)[0]) + if (getattr(b, "currency", None) or "GBP").upper() == curr + ] + for budget in active_budgets: actual = ( sum(actual_by_category.values()) if budget.category is None @@ -207,38 +232,53 @@ def goal_progress(self) -> list[GoalProgress]: for goal in self.goals.active() ] - def cashflow(self, period: date) -> dict[str, int | str]: - summary = self.monthly_summary(period) + def cashflow(self, period: date, currency: str = "GBP") -> dict[str, int | str]: + summary = self.monthly_summary(period, currency=currency) return { "period": summary.period, + "currency": summary.currency, "income_minor": summary.income_minor, "spending_minor": summary.spending_minor, "net_cashflow_minor": summary.net_cashflow_minor, } - def unusual_transactions(self, period: date) -> list[dict[str, object]]: - return unusual_transactions(self.transactions.all(), period) + def unusual_transactions(self, period: date, currency: str = "GBP") -> list[dict[str, object]]: + all_rows = self.transactions.all() + rows = self._filter_currency(all_rows, currency) + return unusual_transactions(rows, period) def category_spikes( - self, current: date, previous: date | None = None + self, current: date, previous: date | None = None, currency: str = "GBP" ) -> list[dict[str, object]]: previous = previous or (current.replace(day=1) - timedelta(days=1)) - return category_spikes(self.transactions.all(), current, previous) + all_rows = self.transactions.all() + rows = self._filter_currency(all_rows, currency) + return category_spikes(rows, current, previous) def category_trend( - self, category: str, as_of: date, months: int = 6 + self, category: str, as_of: date, months: int = 6, currency: str = "GBP" ) -> list[dict[str, int | str]]: - return category_trend(self.transactions.all(), category, as_of, months) + all_rows = self.transactions.all() + rows = self._filter_currency(all_rows, currency) + return category_trend(rows, category, as_of, months) def current_cash( - accounts: list[AccountModel], transactions: list[TransactionModel], as_of: date | None = None + accounts: list[AccountModel], + transactions: list[TransactionModel], + currency: str = "GBP", + as_of: date | None = None, ) -> int: + curr = currency.upper() opening = sum( account.opening_balance_minor for account in accounts if account.account_type not in {item.value for item in NON_CASH_ACCOUNT_TYPES} + and (getattr(account, "currency", None) or "GBP").upper() == curr ) return opening + sum( - _cash_delta(row) for row in transactions if as_of is None or row.transaction_date <= as_of + _cash_delta(row) + for row in transactions + if (getattr(row, "currency", None) or "GBP").upper() == curr + and (as_of is None or row.transaction_date <= as_of) ) diff --git a/src/pfa/api/app.py b/src/pfa/api/app.py index 2c039b9..4fc431f 100644 --- a/src/pfa/api/app.py +++ b/src/pfa/api/app.py @@ -3,6 +3,7 @@ from collections.abc import AsyncIterator, Awaitable, Callable from contextlib import asynccontextmanager, suppress from datetime import date, datetime +from decimal import Decimal, InvalidOperation from pathlib import Path from typing import Annotated, Literal @@ -39,6 +40,7 @@ from pfa.ingestion.upload import stage_upload, sweep_upload_dir from pfa.observability import TimedOperation from pfa.services.answers import deterministic_answer +from pfa.services.fx import fetch_and_store_fx_rates from pfa.services.health import health_report from pfa.services.review import monthly_review_evidence from pfa.services.runtime import close_services, open_services @@ -132,6 +134,28 @@ class ScenarioRequest(BaseModel): cost_minor: int = Field(ge=0) horizon_months: int = Field(default=3, ge=1, le=120) month: str | None = None + currency: str = "GBP" + + +class FxRateResponse(BaseModel): + id: int + base_currency: str + quote_currency: str + rate: str # decimal string - never float; see domain/fx.py + effective_at: date + source: str | None = None + + +class FxRateSetRequest(BaseModel): + base_currency: str + quote_currency: str + rate: str # decimal string - never float; see domain/fx.py + effective_at: date | None = None + + +class FxFetchRequest(BaseModel): + base_currency: str = "GBP" + on_date: date | None = None def _issue_response(issue: CandidateIssue) -> CandidateIssueResponse: @@ -406,29 +430,114 @@ def transactions( finally: close_services(engine, services) + @app.get("/fx/rates", response_model=list[FxRateResponse]) + def get_fx_rates( + base: str | None = None, + quote: str | None = None, + ) -> list[FxRateResponse]: + engine, services = open_services(active_settings) + try: + rates = services.uow.fx_rates.all() + if base: + rates = [r for r in rates if r.base_currency == base.upper()] + if quote: + rates = [r for r in rates if r.quote_currency == quote.upper()] + return [ + FxRateResponse( + id=r.id, + base_currency=r.base_currency, + quote_currency=r.quote_currency, + rate=r.rate, + effective_at=r.effective_at, + source=r.source, + ) + for r in rates + ] + finally: + close_services(engine, services) + + @app.post("/fx/rates", response_model=FxRateResponse) + def set_fx_rate(request: FxRateSetRequest) -> FxRateResponse: + try: + rate = Decimal(request.rate) + except InvalidOperation as exc: + raise HTTPException(status_code=422, detail=f"invalid rate {request.rate!r}") from exc + engine, services = open_services(active_settings) + try: + effective_at = request.effective_at or date.today() + model = services.uow.fx_rates.set_rate( + request.base_currency.upper(), + request.quote_currency.upper(), + rate, + effective_at=effective_at, + ) + response = FxRateResponse( + id=model.id, + base_currency=model.base_currency, + quote_currency=model.quote_currency, + rate=model.rate, + effective_at=model.effective_at, + source=model.source, + ) + close_services(engine, services) + return response + except Exception: + close_services(engine, services, False) + raise + + @app.post("/fx/fetch", response_model=list[FxRateResponse]) + def fetch_fx_rates(request: FxFetchRequest) -> list[FxRateResponse]: + engine, services = open_services(active_settings) + try: + models = fetch_and_store_fx_rates( + services.uow, + base_currency=request.base_currency.upper(), + on_date=request.on_date or date.today(), + ) + response = [ + FxRateResponse( + id=m.id, + base_currency=m.base_currency, + quote_currency=m.quote_currency, + rate=m.rate, + effective_at=m.effective_at, + source=m.source, + ) + for m in models + ] + close_services(engine, services) + return response + except Exception: + close_services(engine, services, False) + raise + @app.get("/analytics/monthly") - def monthly(month: str | None = None) -> dict[str, object]: + def monthly(month: str | None = None, currency: str = "GBP") -> dict[str, object]: engine, services = open_services(active_settings) try: - return services.analytics.monthly_summary(_month(month)).model_dump() + return services.analytics.monthly_summary(_month(month), currency=currency).model_dump() finally: close_services(engine, services) @app.get("/analytics/categories") - def categories(month: str | None = None) -> list[dict[str, object]]: + def categories(month: str | None = None, currency: str = "GBP") -> list[dict[str, object]]: engine, services = open_services(active_settings) try: return [ - item.model_dump() for item in services.analytics.category_spending(_month(month)) + item.model_dump() + for item in services.analytics.category_spending(_month(month), currency=currency) ] finally: close_services(engine, services) @app.get("/budgets") - def budgets(month: str | None = None) -> list[dict[str, object]]: + def budgets(month: str | None = None, currency: str = "GBP") -> list[dict[str, object]]: engine, services = open_services(active_settings) try: - return [item.model_dump() for item in services.analytics.budget_status(_month(month))] + return [ + item.model_dump() + for item in services.analytics.budget_status(_month(month), currency=currency) + ] finally: close_services(engine, services) @@ -445,7 +554,10 @@ def purchase(request: ScenarioRequest) -> dict[str, object]: engine, services = open_services(active_settings) try: return services.planning.simulate_purchase( - request.cost_minor, request.horizon_months, _month(request.month) + request.cost_minor, + request.horizon_months, + _month(request.month), + currency=request.currency, ).model_dump() finally: close_services(engine, services) @@ -481,10 +593,10 @@ def chat(request: ChatRequest) -> dict[str, str]: close_services(engine, services) @app.get("/reviews/monthly") - def review(month: str | None = None) -> dict[str, object]: + def review(month: str | None = None, currency: str = "GBP") -> dict[str, object]: engine, services = open_services(active_settings) try: - return monthly_review_evidence(services.analytics, _month(month)) + return monthly_review_evidence(services.analytics, _month(month), currency=currency) finally: close_services(engine, services) diff --git a/src/pfa/cli/app.py b/src/pfa/cli/app.py index 4230253..60ac045 100644 --- a/src/pfa/cli/app.py +++ b/src/pfa/cli/app.py @@ -3,6 +3,7 @@ import subprocess import sys from datetime import date +from decimal import Decimal, InvalidOperation from pathlib import Path import typer @@ -20,6 +21,7 @@ from pfa.domain.transactions import ClassificationSource, SpendingCategory from pfa.ingestion.service import ImportService from pfa.services.answers import deterministic_answer +from pfa.services.fx import fetch_and_store_fx_rates from pfa.services.health import health_report from pfa.services.review import monthly_review_evidence from pfa.services.runtime import close_services, open_services @@ -30,11 +32,13 @@ summary_app = typer.Typer(help="Summary commands") budget_app = typer.Typer(help="Budget commands") goals_app = typer.Typer(help="Goal commands") +fx_app = typer.Typer(help="Foreign exchange rate commands") app.add_typer(db_app, name="db") app.add_typer(transactions_app, name="transactions") app.add_typer(summary_app, name="summary") app.add_typer(budget_app, name="budget") app.add_typer(goals_app, name="goals") +app.add_typer(fx_app, name="fx") console = Console(legacy_windows=False) @@ -60,8 +64,8 @@ def _legacy_print_money(minor: int) -> str: return f"£{Money(minor).to_major():,.2f}" -def print_money(minor: int) -> str: - return f"GBP {Money(minor).to_major():,.2f}" +def print_money(minor: int, currency: str = "GBP") -> str: + return f"{currency.upper()} {Money(minor, currency=currency).to_major():,.2f}" @db_app.command("init") @@ -102,11 +106,14 @@ def import_transactions(path: Path, dry_run: bool = typer.Option(False, "--dry-r @summary_app.command("month") -def summary_month(month: str | None = typer.Option(None, "--month")) -> None: +def summary_month( + month: str | None = typer.Option(None, "--month"), + currency: str = typer.Option("GBP", "--currency"), +) -> None: engine, services = open_services(get_settings()) try: - summary = services.analytics.monthly_summary(parse_month(month)) - table = Table(title=f"PFA summary {summary.period}") + summary = services.analytics.monthly_summary(parse_month(month), currency=currency) + table = Table(title=f"PFA summary {summary.period} ({summary.currency})") table.add_column("Measure") table.add_column("Amount", justify="right") for label, value in ( @@ -118,7 +125,7 @@ def summary_month(month: str | None = typer.Option(None, "--month")) -> None: ("Investments", summary.investments_minor), ("Net cashflow", summary.net_cashflow_minor), ): - table.add_row(label, print_money(value)) + table.add_row(label, print_money(value, currency=summary.currency)) table.add_row("Savings rate", f"{summary.savings_rate_percent:.2f}%") console.print(table) finally: @@ -229,15 +236,88 @@ def ask(question: str) -> None: @app.command("review") -def review_month(month: str | None = typer.Option(None, "--month")) -> None: +def review_month( + month: str | None = typer.Option(None, "--month"), + currency: str = typer.Option("GBP", "--currency"), +) -> None: engine, services = open_services(get_settings()) try: - evidence = monthly_review_evidence(services.analytics, parse_month(month)) + evidence = monthly_review_evidence( + services.analytics, parse_month(month), currency=currency + ) console.print_json(data=evidence) finally: close_services(engine, services) +@fx_app.command("set") +def fx_set( + base_currency: str, + quote_currency: str, + rate: str, + date_str: str | None = typer.Option(None, "--date", "--on"), +) -> None: + try: + rate_decimal = Decimal(rate) + except InvalidOperation as exc: + raise typer.BadParameter(f"invalid rate {rate!r}") from exc + effective_at = date.fromisoformat(date_str) if date_str else date.today() + engine, services = open_services(get_settings()) + try: + services.uow.fx_rates.set_rate( + base_currency.upper(), + quote_currency.upper(), + rate_decimal, + effective_at=effective_at, + ) + close_services(engine, services) + pair = f"{base_currency.upper()}/{quote_currency.upper()}" + console.print(f"FX rate {pair} = {rate} set for {effective_at}") + except Exception: + close_services(engine, services, False) + raise + + +@fx_app.command("fetch") +def fx_fetch( + base: str = typer.Option("GBP", "--base"), + date_str: str | None = typer.Option(None, "--date", "--on"), +) -> None: + effective_at = date.fromisoformat(date_str) if date_str else date.today() + engine, services = open_services(get_settings()) + try: + stored = fetch_and_store_fx_rates( + services.uow, base_currency=base.upper(), on_date=effective_at + ) + close_services(engine, services) + console.print( + f"Fetched and stored {len(stored)} rates for {base.upper()} on {effective_at}" + ) + except Exception: + close_services(engine, services, False) + raise + + +@fx_app.command("list") +def fx_list() -> None: + engine, services = open_services(get_settings()) + try: + table = Table(title="FX Rates") + for column in ("Base", "Quote", "Rate", "Effective Date", "Source"): + table.add_column(column) + for row in services.uow.fx_rates.all(): + table.add_row( + row.base_currency, + row.quote_currency, + f"{Decimal(row.rate):.6f}", + row.effective_at.isoformat(), + row.source or "manual", + ) + console.print(table) + finally: + close_services(engine, services) + + @budget_app.command("show") def budget_show(month: str | None = typer.Option(None, "--month")) -> None: engine, services = open_services(get_settings()) diff --git a/src/pfa/config.py b/src/pfa/config.py index 4b2686b..8e3b47f 100644 --- a/src/pfa/config.py +++ b/src/pfa/config.py @@ -11,6 +11,7 @@ class Settings(BaseSettings): model_config = SettingsConfigDict(env_prefix="PFA_", env_file=".env", extra="ignore") database_url: str = "sqlite:///data/pfa.db" + base_currency: str = "GBP" ollama_base_url: str = "http://localhost:11434" model: str = "qwen3.5:4b" log_level: str = "INFO" diff --git a/src/pfa/db/models.py b/src/pfa/db/models.py index 12d3040..dd6e60f 100644 --- a/src/pfa/db/models.py +++ b/src/pfa/db/models.py @@ -125,3 +125,19 @@ class MerchantRuleModel(Base): category: Mapped[str | None] = mapped_column(String(40), nullable=True) transfer_purpose: Mapped[str | None] = mapped_column(String(30), nullable=True) created_from_user_correction: Mapped[bool] = mapped_column(Boolean, default=False) + + +class FxRateModel(Base): + __tablename__ = "fx_rates" + __table_args__ = ( + UniqueConstraint( + "base_currency", "quote_currency", "effective_at", name="uq_fx_rates_base_quote_date" + ), + ) + id: Mapped[int] = mapped_column(primary_key=True) + base_currency: Mapped[str] = mapped_column(String(3)) + quote_currency: Mapped[str] = mapped_column(String(3)) + rate: Mapped[str] = mapped_column(String(32)) + effective_at: Mapped[date] = mapped_column(Date, index=True) + source: Mapped[str] = mapped_column(String(50), default="manual") + retrieved_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now()) diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index cd4f446..f846d4f 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -1,6 +1,7 @@ from __future__ import annotations -from datetime import date, datetime +from datetime import UTC, date, datetime +from decimal import Decimal from sqlalchemy import select from sqlalchemy.orm import Session @@ -10,6 +11,7 @@ from .models import ( AccountModel, BudgetModel, + FxRateModel, GoalModel, ImportBatchModel, MerchantRuleModel, @@ -76,6 +78,11 @@ def get_or_create( self.session.flush() return account + def get_by_name(self, name: str) -> AccountModel | None: + """Read-only lookup - never creates a row, so a preview never has the side effect + of persisting an account for a batch that might still be discarded.""" + return self.session.scalar(select(AccountModel).where(AccountModel.name == name)) + def all(self) -> list[AccountModel]: return list(self.session.scalars(select(AccountModel).order_by(AccountModel.name))) @@ -149,3 +156,135 @@ def add(self, goal: GoalModel) -> GoalModel: self.session.add(goal) self.session.flush() return goal + + +class FxRateRepository: + def __init__(self, session: Session): + self.session = session + + def all(self) -> list[FxRateModel]: + return list( + self.session.scalars(select(FxRateModel).order_by(FxRateModel.effective_at.desc())) + ) + + def add(self, fx_rate: FxRateModel) -> FxRateModel: + self.session.add(fx_rate) + self.session.flush() + return fx_rate + + def set_rate( + self, + base_currency: str, + quote_currency: str, + rate: Decimal | str | float, + effective_at: date, + source: str = "manual", + ) -> FxRateModel: + base = base_currency.upper() + quote = quote_currency.upper() + rate_str = str(rate) + now = datetime.now(UTC).replace(tzinfo=None) + statement = select(FxRateModel).where( + FxRateModel.base_currency == base, + FxRateModel.quote_currency == quote, + FxRateModel.effective_at == effective_at, + ) + existing = self.session.scalar(statement) + if existing is not None: + existing.rate = rate_str + existing.source = source + existing.retrieved_at = now + self.session.flush() + return existing + model = FxRateModel( + base_currency=base, + quote_currency=quote, + rate=rate_str, + effective_at=effective_at, + source=source, + retrieved_at=now, + ) + self.session.add(model) + self.session.flush() + return model + + def rate_on( + self, effective_date: date, base: str, quote: str + ) -> tuple[Decimal, FxRateModel | None] | None: + """Finds nearest rate at or before effective_date (never after). + Returns (rate_decimal, matched_model_or_none). + """ + base_upper = base.upper() + quote_upper = quote.upper() + if base_upper == quote_upper: + return Decimal("1.0"), None + + # Direct rate lookup: 1 base = rate quote + direct_stmt = ( + select(FxRateModel) + .where( + FxRateModel.base_currency == base_upper, + FxRateModel.quote_currency == quote_upper, + FxRateModel.effective_at <= effective_date, + ) + .order_by(FxRateModel.effective_at.desc()) + .limit(1) + ) + direct = self.session.scalar(direct_stmt) + if direct is not None: + return Decimal(direct.rate), direct + + # Inverse rate lookup: 1 quote = rate base => 1 base = 1 / rate quote + inverse_stmt = ( + select(FxRateModel) + .where( + FxRateModel.base_currency == quote_upper, + FxRateModel.quote_currency == base_upper, + FxRateModel.effective_at <= effective_date, + ) + .order_by(FxRateModel.effective_at.desc()) + .limit(1) + ) + inverse = self.session.scalar(inverse_stmt) + if inverse is not None: + inv_rate = Decimal(inverse.rate) + if inv_rate != Decimal(0): + return Decimal(1) / inv_rate, inverse + + return None + + def latest(self, base: str, quote: str) -> tuple[Decimal, FxRateModel | None] | None: + base_upper = base.upper() + quote_upper = quote.upper() + if base_upper == quote_upper: + return Decimal("1.0"), None + + direct_stmt = ( + select(FxRateModel) + .where( + FxRateModel.base_currency == base_upper, + FxRateModel.quote_currency == quote_upper, + ) + .order_by(FxRateModel.effective_at.desc()) + .limit(1) + ) + direct = self.session.scalar(direct_stmt) + if direct is not None: + return Decimal(direct.rate), direct + + inverse_stmt = ( + select(FxRateModel) + .where( + FxRateModel.base_currency == quote_upper, + FxRateModel.quote_currency == base_upper, + ) + .order_by(FxRateModel.effective_at.desc()) + .limit(1) + ) + inverse = self.session.scalar(inverse_stmt) + if inverse is not None: + inv_rate = Decimal(inverse.rate) + if inv_rate != Decimal(0): + return Decimal(1) / inv_rate, inverse + + return None diff --git a/src/pfa/db/unit_of_work.py b/src/pfa/db/unit_of_work.py index 4fe300b..bb1dcb0 100644 --- a/src/pfa/db/unit_of_work.py +++ b/src/pfa/db/unit_of_work.py @@ -3,6 +3,7 @@ from .repositories import ( AccountRepository, BudgetRepository, + FxRateRepository, GoalRepository, ImportBatchRepository, RuleRepository, @@ -21,3 +22,4 @@ def __init__(self, session: Session): self.budgets = BudgetRepository(session) self.goals = GoalRepository(session) self.import_batches = ImportBatchRepository(session) + self.fx_rates = FxRateRepository(session) diff --git a/src/pfa/domain/fx.py b/src/pfa/domain/fx.py new file mode 100644 index 0000000..d015b9a --- /dev/null +++ b/src/pfa/domain/fx.py @@ -0,0 +1,64 @@ +from __future__ import annotations + +from dataclasses import dataclass +from datetime import UTC, date, datetime +from decimal import Decimal +from typing import TYPE_CHECKING + +from pfa.domain.errors import ValidationError +from pfa.domain.money import Money + +if TYPE_CHECKING: + from pfa.db.repositories import FxRateRepository + + +@dataclass(frozen=True, slots=True) +class FxRate: + base_currency: str + quote_currency: str + rate: Decimal + effective_at: date + source: str + retrieved_at: datetime + + +def to_base( + money: Money, + on_date: date, + fx_rates: FxRateRepository, + base_currency: str = "GBP", +) -> tuple[Money, FxRate]: + """Converts a Money instance to base currency as of a specific date. + Returns (converted_money, applied_fx_rate). + """ + target_curr = base_currency.upper() + if money.currency == target_curr: + identity_rate = FxRate( + base_currency=target_curr, + quote_currency=target_curr, + rate=Decimal("1.0"), + effective_at=on_date, + source="identity", + retrieved_at=datetime.now(UTC).replace(tzinfo=None), + ) + return money, identity_rate + + rate_info = fx_rates.rate_on(on_date, base=money.currency, quote=target_curr) + if rate_info is None: + raise ValidationError( + f"No FX rate available to convert {money.currency} to {target_curr} " + f"on or before {on_date}" + ) + + rate_dec, model = rate_info + target_major = money.to_major() * rate_dec + converted_money = Money.from_major(target_major, target_curr) + applied_rate = FxRate( + base_currency=money.currency, + quote_currency=target_curr, + rate=rate_dec, + effective_at=model.effective_at if model else on_date, + source=model.source if model else "direct", + retrieved_at=model.retrieved_at if model else datetime.now(UTC).replace(tzinfo=None), + ) + return converted_money, applied_rate diff --git a/src/pfa/domain/money.py b/src/pfa/domain/money.py index c81c5c7..33984b9 100644 --- a/src/pfa/domain/money.py +++ b/src/pfa/domain/money.py @@ -5,6 +5,30 @@ from .errors import ValidationError +SUPPORTED_CURRENCIES: dict[str, int] = { + "GBP": 2, + "INR": 2, + "USD": 2, + "EUR": 2, + "JPY": 0, +} + + +def minor_units(value: str | Decimal | int | float, currency: str = "GBP") -> int: + """Converts a major-unit amount to an integer minor-unit count for `currency`. + + An unrecognised code falls back to a 2-place exponent rather than raising, so a row's + amount can always be parsed before its currency is validated as supported - the two are + separate checks and the caller decides which error the row surfaces. + """ + exponent = SUPPORTED_CURRENCIES.get(currency.upper(), 2) + quantize_unit = Decimal("1") if exponent == 0 else Decimal("0." + "0" * (exponent - 1) + "1") + try: + amount = Decimal(str(value)).quantize(quantize_unit, rounding=ROUND_HALF_UP) + except (InvalidOperation, ValueError) as exc: + raise ValidationError(f"Invalid monetary value: {value!r}") from exc + return int(amount * (10**exponent)) + @dataclass(frozen=True, slots=True) class Money: @@ -16,18 +40,20 @@ def __post_init__(self) -> None: raise ValidationError("Money must use integer minor units") if len(self.currency) != 3 or not self.currency.isalpha(): raise ValidationError("Currency must be a three-letter code") - object.__setattr__(self, "currency", self.currency.upper()) + curr = self.currency.upper() + if curr not in SUPPORTED_CURRENCIES: + supported = ", ".join(sorted(SUPPORTED_CURRENCIES)) + raise ValidationError(f"Unsupported currency {curr!r}; supported: {supported}") + object.__setattr__(self, "currency", curr) @classmethod def from_major(cls, value: str | Decimal | int | float, currency: str = "GBP") -> Money: - try: - amount = Decimal(str(value)).quantize(Decimal("0.01"), rounding=ROUND_HALF_UP) - except (InvalidOperation, ValueError) as exc: - raise ValidationError(f"Invalid monetary value: {value!r}") from exc - return cls(int(amount * 100), currency) + curr = currency.upper() if isinstance(currency, str) else "GBP" + return cls(minor_units(value, curr), curr) def to_major(self) -> Decimal: - return Decimal(self.minor) / 100 + exponent = SUPPORTED_CURRENCIES.get(self.currency, 2) + return Decimal(self.minor) / Decimal(10**exponent) def __add__(self, other: Money) -> Money: self._same_currency(other) diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index fa17c8b..c5f2e40 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -14,13 +14,14 @@ import json import logging import uuid +from collections import Counter from dataclasses import asdict, dataclass -from datetime import UTC, datetime, timedelta +from datetime import UTC, date, datetime, timedelta from pfa.config import Settings from pfa.db.models import ImportBatchModel from pfa.db.unit_of_work import UnitOfWork -from pfa.domain.errors import BatchError +from pfa.domain.errors import BatchError, ImportRowError from pfa.ingestion.service import ImportService from .candidates import ( @@ -34,6 +35,7 @@ EXTRACTION_FAILED, EXTRACTION_TIMEOUT, NO_USABLE_ROWS, + STATEMENT_YEAR_INFERRED, TOO_MANY_ROWS, VALID, WARNING, @@ -44,7 +46,10 @@ StatementSource, candidates_from_json, candidates_to_json, + is_year_bearing_date, + parse_date, ) +from .dialects import Dialect, dialect_for_name from .extractors.csv import CsvStatementExtractor from .extractors.ocr import OcrFallbackPdfExtractor from .extractors.pdf import clean_amount_text @@ -102,19 +107,23 @@ def batch_committed_transaction_ids(batch: ImportBatchModel) -> list[int]: return list(json.loads(batch.committed_transaction_ids_json)) -def _extractor_for(source: StatementSource, settings: Settings) -> StatementExtractor: - """Picks the extractor from the extension the upload policy already validated. - - PDFs always go through the OCR-fallback wrapper: it runs native extraction first and - only reaches for Tesseract on pages that have no usable text of their own. - """ +def _extractor_for( + source: StatementSource, + settings: Settings, + account_name: str | None = None, + account_currency: str = "GBP", +) -> StatementExtractor: + """Picks the extractor from the extension the upload policy already validated.""" + dialect = dialect_for_name(account_name) if source.path.suffix.lower() == ".pdf": return OcrFallbackPdfExtractor( settings=settings, max_pdf_pages=settings.max_pdf_pages, max_candidate_rows=settings.max_candidate_rows, + dialect=dialect, + currency=account_currency, ) - return CsvStatementExtractor() + return CsvStatementExtractor(dialect=dialect, currency=account_currency) def _run_extraction( @@ -143,6 +152,51 @@ def _fail(batch: ImportBatchModel, uow: UnitOfWork, code: str, message: str) -> return uow.import_batches.add(batch) +def _normalize_dates( + candidates: list[CandidateTransaction], dialect: Dialect +) -> CandidateIssue | None: + """Resolves every year-less date (`Jul31`, `21 Jul`) against the year the rest of this + statement's dates carry, then rewrites the candidate's date string to ISO so every later + parse - validation, commit - sees that same resolved year, never whatever year the + import happens to run in. + + Returns a warning issue when no row in the statement carried a year of its own, so the + fallback to today's year is visible in the preview rather than silent. + """ + years_seen: list[int] = [] + for candidate in candidates: + text = candidate.transaction_date + if text and is_year_bearing_date(text, dialect.date_order): + try: + years_seen.append(parse_date(text, dialect.date_order).year) + except ImportRowError: + continue + inferred_year = Counter(years_seen).most_common(1)[0][0] if years_seen else date.today().year + + used_fallback = False + for candidate in candidates: + for attr in ("transaction_date", "posted_date"): + text = getattr(candidate, attr) + if not text: + continue + try: + resolved = parse_date(text, dialect.date_order, inferred_year) + except ImportRowError: + continue + if not is_year_bearing_date(text, dialect.date_order): + used_fallback = True + setattr(candidate, attr, resolved.isoformat()) + + if years_seen or not used_fallback: + return None + return CandidateIssue( + STATEMENT_YEAR_INFERRED, + f"no date in this statement carried its own year; {inferred_year} was assumed for " + "year-less dates - check the preview before committing", + WARNING, + ) + + def create_batch( uow: UnitOfWork, source: StatementSource, @@ -151,7 +205,17 @@ def create_batch( account: str | None = None, ) -> ImportBatchModel: now = _now() - extractor = _extractor_for(source, settings) + account_currency = "GBP" + if account: + existing_acc = uow.accounts.get_by_name(account) + if existing_acc is not None: + account_currency = existing_acc.currency + + extractor = _extractor_for( + source, settings, account_name=account, account_currency=account_currency + ) + dialect = dialect_for_name(account) + batch = ImportBatchModel( id=uuid.uuid4().hex, original_filename=source.original_filename, @@ -161,6 +225,7 @@ def create_batch( extractor=extractor.name, status="extracting", destination_account=account, + amount_sign=dialect.default_sign, issues_json="[]", counts_json=json.dumps(_counts([])), created_at=now, @@ -190,15 +255,34 @@ def create_batch( ) ) + year_issue = _normalize_dates(candidates, dialect) + if year_issue: + extraction.issues.append(year_issue) + service = ImportService(uow) service.validate(candidates) + if batch.amount_sign: + for candidate in candidates: + _apply_amount_sign(candidate, batch.amount_sign) service.resolve_duplicates(candidates) if not candidates and not any(issue.severity == ERROR for issue in extraction.issues): extraction.issues.append(CandidateIssue(NO_USABLE_ROWS, "no transactions were found")) + parsed_dates: list[date] = [] + for candidate in candidates: + if not candidate.transaction_date: + continue + try: + parsed_dates.append(date.fromisoformat(candidate.transaction_date)) + except ValueError: + continue + if parsed_dates: + batch.statement_start = min(parsed_dates) + batch.statement_end = max(parsed_dates) + batch.detected_account = extraction.detected_account - batch.detected_currency = extraction.detected_currency + batch.detected_currency = extraction.detected_currency or account_currency batch.page_count = extraction.page_count blocked = any(issue.severity == ERROR for issue in extraction.issues) batch.status = "blocked" if blocked else "preview_ready" @@ -253,11 +337,14 @@ def _apply_amount_sign(candidate: CandidateTransaction, convention: str) -> None The direction is re-derived from the raw text rather than flipped, so sending a convention twice - or switching back - always lands on the same answer. Rows whose - source stated the direction in its own debit/credit column are left alone: their - convention is not in doubt, and the extractor already resolved it. + source stated the direction in its own debit/credit column - or an explicit CR/CREDIT + marker, own-line or inline - are left alone: their convention is not in doubt, and the + extractor already resolved it. """ if candidate.direction is None: return + if candidate.direction_explicit: + return if "debit" in candidate.raw_fields or "credit" in candidate.raw_fields: return amount = candidate.raw_fields.get("amount", "") diff --git a/src/pfa/ingestion/candidates.py b/src/pfa/ingestion/candidates.py index f3a8b2e..ce384f8 100644 --- a/src/pfa/ingestion/candidates.py +++ b/src/pfa/ingestion/candidates.py @@ -16,7 +16,7 @@ from typing import Protocol from pfa.domain.errors import ImportRowError -from pfa.domain.money import Money +from pfa.domain.money import minor_units ERROR = "error" WARNING = "warning" @@ -28,6 +28,8 @@ INVALID_AMOUNT = "INVALID_AMOUNT" AMBIGUOUS_SIGN = "AMBIGUOUS_SIGN" UNSUPPORTED_CURRENCY = "UNSUPPORTED_CURRENCY" +CURRENCY_ACCOUNT_MISMATCH = "CURRENCY_ACCOUNT_MISMATCH" +STATEMENT_YEAR_INFERRED = "STATEMENT_YEAR_INFERRED" UNKNOWN_KIND = "UNKNOWN_KIND" UNKNOWN_CATEGORY = "UNKNOWN_CATEGORY" UNKNOWN_TRANSFER_PURPOSE = "UNKNOWN_TRANSFER_PURPOSE" @@ -83,22 +85,110 @@ def match_header_alias(cell_text: str) -> str | None: return None -def parse_date(value: str) -> date: - for pattern in ("%Y-%m-%d", "%d/%m/%Y", "%d-%m-%Y", "%m/%d/%Y"): +_YEARLESS_DATE_PATTERNS: tuple[str, ...] = ("%b%d", "%b %d", "%d %b") + + +def _year_bearing_date_patterns(date_order: str) -> tuple[str, ...]: + if date_order == "month_first": + return ( + "%Y-%m-%d", + "%m/%d/%Y", + "%m-%d-%Y", + "%d/%m/%Y", + "%d-%m-%Y", + "%b %d %Y", + "%b %d, %Y", + "%d %b %Y", + "%d %b %y", + "%d/%m/%y", + "%m/%d/%y", + ) + return ( + "%Y-%m-%d", + "%d/%m/%Y", + "%d-%m-%Y", + "%d %b %Y", + "%d %b %y", + "%b %d %Y", + "%b %d, %Y", + "%d/%m/%y", + "%m/%d/%Y", + "%m-%d-%Y", + ) + + +def is_year_bearing_date(value: str, date_order: str = "day_first") -> bool: + """True when `value` carries its own year, rather than needing one assumed for it.""" + cleaned = value.strip() + for pattern in _year_bearing_date_patterns(date_order): try: - return datetime.strptime(value, pattern).date() + datetime.strptime(cleaned, pattern) + return True except ValueError: continue + return False + + +def parse_date( + value: str, + date_order: str = "day_first", + statement_year: int | None = None, +) -> date: + cleaned = value.strip() + for pattern in _year_bearing_date_patterns(date_order): + try: + return datetime.strptime(cleaned, pattern).date() + except ValueError: + continue + + # Year-less format attempts like 'Jul31', 'Jul 31', '21 Jul'. `statement_year` should + # always come from other year-bearing dates in the same statement (see + # ingestion.batches._normalize_dates) - falling back to today's year here is a last + # resort for a caller that never supplied one. + year = statement_year or date.today().year + for pattern in _YEARLESS_DATE_PATTERNS: + try: + dt = datetime.strptime(cleaned, pattern) + return dt.replace(year=year).date() + except ValueError: + continue + raise ImportRowError(f"invalid date {value!r}") -def parse_amount(value: str) -> tuple[int, int]: +def parse_amount(value: str, currency: str = "GBP") -> tuple[int, int, bool]: + """Parses a signed amount. Returns (sign, minor_units, was_an_explicit_credit_marker). + + The third element tells the caller the row's direction came from a CR/CREDIT marker in + the text itself, not from the statement's general sign convention - so a later + convention choice (e.g. "debit positive") must never override it. + """ + cleaned = ( + value.replace(",", "") + .replace("£", "") + .replace("$", "") + .replace("€", "") + .replace("₹", "") + .replace("�", "") + .strip() + ) + is_cr = False + upper = cleaned.upper() + if upper.endswith("CR."): + cleaned = cleaned[:-3].strip() + is_cr = True + elif upper.endswith("CR"): + cleaned = cleaned[:-2].strip() + is_cr = True + elif upper.startswith("CR"): + cleaned = cleaned[2:].strip() + is_cr = True try: - decimal = Decimal(value.replace(",", "").replace("£", "").strip()) + decimal = Decimal(cleaned) except InvalidOperation as exc: raise ImportRowError(f"invalid amount {value!r}") from exc - sign = -1 if decimal < 0 else 1 - return sign, Money.from_major(abs(decimal)).minor + sign = 1 if is_cr else (-1 if decimal < 0 else 1) + return sign, minor_units(abs(decimal), currency), is_cr @dataclass(frozen=True, slots=True) @@ -128,6 +218,10 @@ class CandidateTransaction: normalized_description: str = "" amount_minor: int | None = None # absolute magnitude, matches TransactionModel direction: str | None = None # "debit" | "credit" + # True once `direction` was read from an explicit marker (a CR/CREDIT suffix, or a + # debit/credit column) rather than the statement's general sign convention. A later + # amount-sign convention choice must never overwrite a row already resolved this way. + direction_explicit: bool = False currency: str = "GBP" account_hint: str | None = None external_id: str | None = None diff --git a/src/pfa/ingestion/dialects.py b/src/pfa/ingestion/dialects.py new file mode 100644 index 0000000..af70064 --- /dev/null +++ b/src/pfa/ingestion/dialects.py @@ -0,0 +1,67 @@ +from __future__ import annotations + +from dataclasses import dataclass, replace +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + pass + + +@dataclass(frozen=True, slots=True) +class Dialect: + name: str = "generic" + date_formats: tuple[str, ...] = ( + "%Y-%m-%d", + "%d/%m/%Y", + "%d-%m-%Y", + "%d %b %Y", + "%d %b %y", + "%b %d %Y", + "%b %d, %Y", + "%d/%m/%y", + ) + date_order: str = "day_first" # "day_first" | "month_first" + credit_markers: tuple[str, ...] = ("CR", "CREDIT", "CR.") + default_sign: str | None = None # None = ask user, "debit_positive", "as_written" + two_column: bool = False + + +GENERIC = Dialect() + +HSBC = replace( + GENERIC, + name="hsbc", + date_formats=GENERIC.date_formats + ("%d %b %y", "%d %b %Y"), + credit_markers=("CR", "CREDIT"), +) + +AMEX_CARD = replace( + GENERIC, + name="amex", + date_formats=("%b%d", "%b %d", "%d %b %y", "%d %b %Y") + GENERIC.date_formats, + default_sign="debit_positive", +) + +BARCLAYCARD = replace( + GENERIC, + name="barclaycard", + date_formats=GENERIC.date_formats + ("%d %b %y", "%d %b %Y"), + two_column=True, +) + +DIALECTS: dict[str, Dialect] = { + "generic": GENERIC, + "hsbc": HSBC, + "amex": AMEX_CARD, + "barclaycard": BARCLAYCARD, +} + + +def dialect_for_name(name: str | None) -> Dialect: + if not name: + return GENERIC + clean = name.strip().lower() + for key, dialect in DIALECTS.items(): + if key in clean: + return dialect + return GENERIC diff --git a/src/pfa/ingestion/extractors/csv.py b/src/pfa/ingestion/extractors/csv.py index 605d57e..ec6e74b 100644 --- a/src/pfa/ingestion/extractors/csv.py +++ b/src/pfa/ingestion/extractors/csv.py @@ -20,6 +20,7 @@ parse_amount, parse_date, ) +from pfa.ingestion.dialects import GENERIC, Dialect DATE_ALIASES = HEADER_ALIASES["date"] DESCRIPTION_ALIASES = HEADER_ALIASES["description"] @@ -79,7 +80,9 @@ def _is_headerless(cells: list[str]) -> bool: ) -def read_csv_rows(path: Path) -> Iterator[dict[str, str]]: +def read_csv_rows( + path: Path, default_currency: str = "GBP", dialect: Dialect = GENERIC +) -> Iterator[dict[str, str]]: delimiter = _delimiter(path) with path.open(newline="", encoding="utf-8-sig") as handle: reader = csv.DictReader(handle, delimiter=delimiter) @@ -102,7 +105,7 @@ def read_csv_rows(path: Path) -> Iterator[dict[str, str]]: "amount": _value(row, *AMOUNT_ALIASES), "debit": _value(row, *DEBIT_ALIASES), "credit": _value(row, *CREDIT_ALIASES), - "currency": _value(row, "currency") or "GBP", + "currency": _value(row, "currency") or default_currency or "GBP", "kind": _value(row, "kind", "transaction_kind"), "category": _value(row, "category"), "transfer_purpose": _value(row, "transfer_purpose"), @@ -159,11 +162,18 @@ class CsvStatementExtractor: name = "csv/1" + def __init__(self, dialect: Dialect = GENERIC, currency: str = "GBP") -> None: + self.dialect = dialect + self.currency = currency + def extract(self, source: StatementSource) -> ExtractionResult: result = ExtractionResult(extractor=self.name) positional = False try: - for index, row in enumerate(read_csv_rows(source.path), start=1): + for index, row in enumerate( + read_csv_rows(source.path, default_currency=self.currency, dialect=self.dialect), + start=1, + ): positional = positional or bool(row["_positional"]) result.candidates.append(_candidate(f"c{index}", row)) except ImportRowError as exc: diff --git a/src/pfa/ingestion/extractors/ocr.py b/src/pfa/ingestion/extractors/ocr.py index 18bebd4..2b131eb 100644 --- a/src/pfa/ingestion/extractors/ocr.py +++ b/src/pfa/ingestion/extractors/ocr.py @@ -32,6 +32,7 @@ ExtractionResult, StatementSource, ) +from pfa.ingestion.dialects import GENERIC, Dialect from pfa.ingestion.extractors.pdf import PdfStatementExtractor, Word @@ -204,6 +205,8 @@ def __init__( runner: TesseractRunner | None = None, max_pdf_pages: int | None = None, max_candidate_rows: int | None = None, + dialect: Dialect = GENERIC, + currency: str = "GBP", ) -> None: settings = settings or get_settings() word_provider: Callable[[Page], list[Word]] @@ -227,6 +230,8 @@ def __init__( max_candidate_rows=max_candidate_rows, word_provider=word_provider, ocr_min_confidence=settings.ocr_min_confidence, + dialect=dialect, + currency=currency, ) def extract(self, source: StatementSource) -> ExtractionResult: diff --git a/src/pfa/ingestion/extractors/pdf.py b/src/pfa/ingestion/extractors/pdf.py index c002341..4e5d6d6 100644 --- a/src/pfa/ingestion/extractors/pdf.py +++ b/src/pfa/ingestion/extractors/pdf.py @@ -10,7 +10,6 @@ from collections.abc import Callable from dataclasses import dataclass, field -from datetime import datetime from decimal import Decimal, InvalidOperation from typing import Any @@ -20,7 +19,7 @@ from pdfplumber.pdf import PDF from pfa.config import get_settings -from pfa.domain.money import Money +from pfa.domain.money import minor_units from pfa.ingestion.candidates import ( AMBIGUOUS_SIGN, ERROR, @@ -36,7 +35,9 @@ ExtractionResult, StatementSource, match_header_alias, + parse_date, ) +from pfa.ingestion.dialects import GENERIC, Dialect # ponytail: max_candidate_rows is T3's setting (src/pfa/config.py, landing in a parallel # branch). Mirrors the plan's stated default until that lands; swap for @@ -44,7 +45,6 @@ _DEFAULT_MAX_CANDIDATE_ROWS = 10_000 _AMOUNT_FIELDS = ("amount", "debit", "credit") -_DATE_PATTERNS = ("%Y-%m-%d", "%d/%m/%Y", "%d-%m-%Y", "%m/%d/%Y") _LINE_TOLERANCE = 3.0 # points; words within this many points of `top` share a line _CELL_GAP = 10.0 # points; a horizontal gap larger than this starts a new cell/column @@ -168,13 +168,69 @@ def _assign_cells( return fields, field_conf -def _word_rows(words: list[Word], page_number: int) -> tuple[list[_RawRow], float | None]: - """Returns the page's data rows plus the header line's `top` (or None if no header). +def _is_date_text(text: str, dialect: Dialect = GENERIC) -> bool: + cleaned = text.strip() + if not cleaned: + return False + try: + parse_date(cleaned, date_order=dialect.date_order) + return True + except Exception: + return False - The header-to-first-data-row gap is a reliable one-line baseline for the continuation - check below, even on a page with too few data rows to measure a gap between two of - them. + +def _is_amount_text(text: str) -> bool: + cleaned, _ = clean_amount_text(text) + if not cleaned: + return False + try: + Decimal(cleaned) + return True + except Exception: + return False + + +def _is_lone_credit_marker( + cells: list[tuple[float, str, float | None]], dialect: Dialect +) -> str | None: + """The marker text when a line is nothing but a credit marker (own-line `CR`), else None. + + A statement that prints `CR` on its own line - visually attached to the amount above it + but structurally its own row - would otherwise either vanish (no date, no amount pair to + match) or become a spurious candidate with no date of its own. Folding it back onto the + previous row as an explicit marker is what lets `_resolve_amount` read it correctly. """ + if len(cells) != 1: + return None + text = cells[0][1].strip().upper().rstrip(".") + for marker in dialect.credit_markers: + if text == marker.upper().rstrip("."): + return cells[0][1].strip() + return None + + +def _cluster_words_into_columns(words: list[Word]) -> list[list[Word]]: + if not words: + return [] + min_x = min(w["x0"] for w in words) + max_x = max(w["x1"] for w in words) + width = max_x - min_x + if width < 150: + return [words] + split_x = min_x + width * 0.55 + left = [w for w in words if (w["x0"] + w["x1"]) / 2.0 < split_x] + right = [w for w in words if (w["x0"] + w["x1"]) / 2.0 >= split_x] + columns: list[list[Word]] = [] + if left: + columns.append(left) + if right: + columns.append(right) + return columns or [words] + + +def _process_lines_for_column( + words: list[Word], page_number: int, dialect: Dialect = GENERIC +) -> tuple[list[_RawRow], float | None]: lines = _group_lines(words) columns: list[tuple[float, str]] | None = None header_top: float | None = None @@ -182,11 +238,39 @@ def _word_rows(words: list[Word], page_number: int) -> tuple[list[_RawRow], floa position = 0 for line in lines: cells = _split_cells(line) + marker_text = _is_lone_credit_marker(cells, dialect) + if marker_text is not None and rows: + rows[-1].fields.setdefault("type", marker_text) + rows[-1].raw_text = f"{rows[-1].raw_text} / {marker_text}" + continue if columns is None: columns = _header_columns(cells) if columns is not None: header_top = line[0]["top"] - continue # header line itself, or noise above it - never a data row + continue + if len(cells) >= 2: + first_text = cells[0][1] + last_text = cells[-1][1] + if _is_date_text(first_text, dialect) and _is_amount_text(last_text): + position += 1 + description = " ".join(c[1] for c in cells[1:-1] if c[1] != first_text) + fields = { + "date": first_text, + "description": description, + "amount": last_text, + } + raw_text = " | ".join(text for _, text, _ in cells) + rows.append( + _RawRow( + source_page=page_number, + position=position, + top=line[0]["top"], + fields=fields, + raw_text=raw_text, + is_ocr=any("conf" in word for word in line), + ) + ) + continue position += 1 fields, field_conf = _assign_cells(cells, columns) raw_text = " | ".join(text for _, text, _ in cells) @@ -204,23 +288,41 @@ def _word_rows(words: list[Word], page_number: int) -> tuple[list[_RawRow], floa return rows, header_top -def _has_parseable_date(fields: dict[str, str]) -> bool: +def _word_rows( + words: list[Word], page_number: int, dialect: Dialect = GENERIC +) -> tuple[list[_RawRow], float | None]: + """Returns the page's data rows plus the header line's `top` (or None if no header).""" + if dialect.two_column: + cols = _cluster_words_into_columns(words) + all_rows: list[_RawRow] = [] + first_header: float | None = None + for col_words in cols: + rows, header_top = _process_lines_for_column(col_words, page_number, dialect) + if rows: + all_rows.extend(rows) + if first_header is None: + first_header = header_top + return all_rows, first_header + return _process_lines_for_column(words, page_number, dialect) + + +def _has_parseable_date(fields: dict[str, str], dialect: Dialect = GENERIC) -> bool: value = fields.get("date", "").strip() - for pattern in _DATE_PATTERNS: - try: - datetime.strptime(value, pattern) - return True - except ValueError: - continue - return False + if not value: + return False + try: + parse_date(value, date_order=dialect.date_order) + return True + except Exception: + return False def _has_parseable_amount(fields: dict[str, str]) -> bool: return any(_signed_minor(fields.get(field, "")) is not None for field in _AMOUNT_FIELDS) -def _is_plausible_data_row(row: _RawRow) -> bool: - return _has_parseable_date(row.fields) and _has_parseable_amount(row.fields) +def _is_plausible_data_row(row: _RawRow, dialect: Dialect = GENERIC) -> bool: + return _has_parseable_date(row.fields, dialect) and _has_parseable_amount(row.fields) def _has_filled_transaction_cell(row: _RawRow) -> bool: @@ -230,12 +332,6 @@ def _has_filled_transaction_cell(row: _RawRow) -> bool: def _line_height(rows: list[_RawRow], header_top: float | None) -> float: - """The smallest line-to-line gap on the page - a good proxy for one text line. - - Using the minimum (rather than e.g. the median) keeps a single large gap - the very - thing a continuation check needs to measure against - from inflating the baseline. - The header-to-first-row gap is included as a reliable one-line reference point. - """ tops = [row.top for row in rows if row.top is not None] if header_top is not None: tops = [header_top, *tops] @@ -243,9 +339,13 @@ def _line_height(rows: list[_RawRow], header_top: float | None) -> float: return min(diffs) if diffs else _DEFAULT_LINE_HEIGHT -def _merge_continuations(rows: list[_RawRow], header_top: float | None) -> list[_RawRow]: +def _merge_continuations( + rows: list[_RawRow], header_top: float | None, dialect: Dialect = GENERIC +) -> list[_RawRow]: """Joins structurally empty wrapped description lines into the row above them.""" - has_plausible_row = any(row.top is not None and _is_plausible_data_row(row) for row in rows) + has_plausible_row = any( + row.top is not None and _is_plausible_data_row(row, dialect) for row in rows + ) threshold = _line_height(rows, header_top) * _CONTINUATION_FACTOR kept: list[_RawRow] = [] last: _RawRow | None = None @@ -253,7 +353,7 @@ def _merge_continuations(rows: list[_RawRow], header_top: float | None) -> list[ has_description = bool(row.fields.get("description", "").strip()) if ( row.top is None - or _is_plausible_data_row(row) + or _is_plausible_data_row(row, dialect) or (_has_filled_transaction_cell(row) and (has_plausible_row or has_description)) ): kept.append(row) @@ -265,7 +365,7 @@ def _merge_continuations(rows: list[_RawRow], header_top: float | None) -> list[ existing = last.fields.get("description", "") last.fields["description"] = f"{existing} {joined}".strip() last.raw_text = f"{last.raw_text} / {row.raw_text}" - last.top = row.top # chain distance from the most recently joined line + last.top = row.top return kept @@ -273,6 +373,7 @@ def _merge_continuations(rows: list[_RawRow], header_top: float | None) -> list[ class _AmountResult: minor: int | None = None direction: str | None = None + direction_explicit: bool = False issue: CandidateIssue | None = None @@ -289,13 +390,23 @@ def clean_amount_text(text: str) -> tuple[str, bool]: if cleaned.startswith("-") or cleaned.startswith(_UNICODE_MINUS): negative = True cleaned = cleaned[1:].strip() - for char in _CURRENCY_CHARS: + for char in _CURRENCY_CHARS + "₹\ufffd": cleaned = cleaned.replace(char, "") cleaned = cleaned.replace(",", "").replace(_UNICODE_MINUS, "").strip() + upper = cleaned.upper() + if upper.endswith("CR."): + cleaned = cleaned[:-3].strip() + negative = False + elif upper.endswith("CR"): + cleaned = cleaned[:-2].strip() + negative = False + elif upper.startswith("CR"): + cleaned = cleaned[2:].strip() + negative = False return cleaned, negative -def _signed_minor(text: str) -> tuple[int, bool] | None: +def _signed_minor(text: str, currency: str = "GBP") -> tuple[int, bool] | None: cleaned, negative = clean_amount_text(text) if not cleaned: return None @@ -303,26 +414,29 @@ def _signed_minor(text: str) -> tuple[int, bool] | None: decimal = Decimal(cleaned) except InvalidOperation: return None - return Money.from_major(abs(decimal)).minor, negative - + return minor_units(abs(decimal), currency), negative -def _resolve_amount(fields: dict[str, str]) -> _AmountResult: - """Resolves one signed amount. Two disagreeing sign sources block, never guess. - Balance is intentionally never read here - it is provenance only, never a transaction - amount. ponytail: reconciling running balance against amount deltas (the spec allows - this to surface warnings only) is deferred - no test or issue code calls for it yet; - add a RECONCILIATION_MISMATCH warning code and compare deltas here if that's needed. - """ +def _resolve_amount( + fields: dict[str, str], dialect: Dialect = GENERIC, currency: str = "GBP" +) -> _AmountResult: debit_text = fields.get("debit", "").strip() credit_text = fields.get("credit", "").strip() amount_text = fields.get("amount", "").strip() + is_explicit_cr = False + for marker in dialect.credit_markers: + if marker in amount_text.upper() or fields.get("type", "").upper() == marker: + is_explicit_cr = True + break + if amount_text: - parsed = _signed_minor(amount_text) + parsed = _signed_minor(amount_text, currency) if parsed is None: return _AmountResult() minor, negative = parsed + if is_explicit_cr: + return _AmountResult(minor=minor, direction="credit", direction_explicit=True) return _AmountResult(minor=minor, direction="debit" if negative else "credit") if debit_text and credit_text: @@ -335,7 +449,7 @@ def _resolve_amount(fields: dict[str, str]) -> _AmountResult: if debit_text or credit_text: implied_direction = "debit" if debit_text else "credit" - parsed = _signed_minor(debit_text or credit_text) + parsed = _signed_minor(debit_text or credit_text, currency) if parsed is None: return _AmountResult() minor, negative = parsed @@ -346,12 +460,18 @@ def _resolve_amount(fields: dict[str, str]) -> _AmountResult: "credit column holds a negative/parenthesised value; sign cannot be determined", ) ) - return _AmountResult(minor=minor, direction=implied_direction) + return _AmountResult(minor=minor, direction=implied_direction, direction_explicit=True) return _AmountResult() -def _build_candidate(index: int, row: _RawRow, ocr_min_confidence: float) -> CandidateTransaction: +def _build_candidate( + index: int, + row: _RawRow, + ocr_min_confidence: float, + currency: str = "GBP", + dialect: Dialect = GENERIC, +) -> CandidateTransaction: fields = row.fields raw_fields = {name: value for name, value in fields.items() if value.strip()} raw_fields["raw_text"] = row.raw_text @@ -359,7 +479,7 @@ def _build_candidate(index: int, row: _RawRow, ocr_min_confidence: float) -> Can candidate_id=f"p{index}", transaction_date=fields.get("date", "").strip() or None, raw_description=fields.get("description", "").strip(), - currency="GBP", + currency=currency.upper(), external_id=fields.get("reference", "").strip() or None, source_format="pdf", source_page=row.source_page, @@ -367,12 +487,13 @@ def _build_candidate(index: int, row: _RawRow, ocr_min_confidence: float) -> Can extraction_method="ocr" if row.is_ocr else "pdf", raw_fields=raw_fields, ) - amount = _resolve_amount(fields) + amount = _resolve_amount(fields, dialect, currency) if amount.issue: candidate.issues.append(amount.issue) else: candidate.amount_minor = amount.minor candidate.direction = amount.direction + candidate.direction_explicit = amount.direction_explicit if row.is_ocr: candidate.add_issue( OCR_EXTRACTED, @@ -406,6 +527,8 @@ def __init__( max_candidate_rows: int | None = None, word_provider: WordProvider | None = None, ocr_min_confidence: float | None = None, + dialect: Dialect = GENERIC, + currency: str = "GBP", ) -> None: self._max_pages = ( max_pdf_pages if max_pdf_pages is not None else get_settings().max_pdf_pages @@ -419,6 +542,8 @@ def __init__( if ocr_min_confidence is not None else get_settings().ocr_min_confidence ) + self.dialect = dialect + self.currency = currency def extract(self, source: StatementSource) -> ExtractionResult: result = ExtractionResult(extractor=self.name) @@ -433,7 +558,7 @@ def extract(self, source: StatementSource) -> ExtractionResult: ) ) return result - except Exception: # a corrupt/unsupported PDF becomes a sanitized batch issue + except Exception: result.issues.append( CandidateIssue( PDF_NOT_EXTRACTABLE, @@ -456,10 +581,10 @@ def _extract(self, pdf: PDF, result: ExtractionResult) -> ExtractionResult: kept: list[_RawRow] = [] for page in pdf.pages: page_rows, header_top = self._page_rows(page) - kept.extend(_merge_continuations(page_rows, header_top)) + kept.extend(_merge_continuations(page_rows, header_top, self.dialect)) candidates = [ - _build_candidate(index, row, self._ocr_min_confidence) + _build_candidate(index, row, self._ocr_min_confidence, self.currency, self.dialect) for index, row in enumerate(kept, start=1) ] if len(candidates) > self._max_rows: @@ -488,4 +613,4 @@ def _page_rows(self, page: Page) -> tuple[list[_RawRow], float | None]: mapping = _table_header(table) if mapping: return _table_rows(table, mapping, page.page_number), None - return _word_rows(self._word_provider(page), page.page_number) + return _word_rows(self._word_provider(page), page.page_number, self.dialect) diff --git a/src/pfa/ingestion/service.py b/src/pfa/ingestion/service.py index 284217a..9fd41dc 100644 --- a/src/pfa/ingestion/service.py +++ b/src/pfa/ingestion/service.py @@ -9,6 +9,7 @@ from pfa.db.models import MerchantRuleModel, TransactionModel from pfa.db.unit_of_work import UnitOfWork from pfa.domain.errors import ImportRowError +from pfa.domain.money import SUPPORTED_CURRENCIES from pfa.domain.transactions import ( ClassificationSource, SpendingCategory, @@ -18,6 +19,7 @@ from pfa.observability import TimedOperation from .candidates import ( + CURRENCY_ACCOUNT_MISMATCH, DUPLICATE_ROW, ERROR, INVALID_AMOUNT, @@ -102,27 +104,34 @@ def _classification_from_rule(rule: MerchantRuleModel) -> Classification: def _validate_candidate(candidate: CandidateTransaction) -> None: + if not candidate.transaction_date: + candidate.add_issue(INVALID_DATE, "missing transaction date") + return try: - parse_date(candidate.transaction_date or "") + parse_date(candidate.transaction_date) except ImportRowError as exc: candidate.add_issue(INVALID_DATE, str(exc)) return - if not candidate.raw_description: + if not candidate.raw_description.strip(): candidate.add_issue(MISSING_DESCRIPTION, "missing description") return if candidate.amount_minor is None: try: - sign, amount_minor = parse_amount(candidate.raw_fields.get("amount", "")) + sign, amount_minor, is_explicit_credit = parse_amount( + candidate.raw_fields.get("amount", ""), candidate.currency + ) except ImportRowError as exc: candidate.add_issue(INVALID_AMOUNT, str(exc)) return candidate.amount_minor = amount_minor candidate.direction = "debit" if sign < 0 else "credit" + candidate.direction_explicit = is_explicit_credit candidate.normalized_description = normalize_description(candidate.raw_description) - if candidate.currency != "GBP": + if candidate.currency.upper() not in SUPPORTED_CURRENCIES: + supported = ", ".join(sorted(SUPPORTED_CURRENCIES)) candidate.add_issue( UNSUPPORTED_CURRENCY, - f"unsupported currency {candidate.currency!r}; PFA v0.1 supports GBP only", + f"unsupported currency {candidate.currency!r}; supported: {supported}", ) return if candidate.posted_date: @@ -154,6 +163,17 @@ def validate(self, candidates: Sequence[CandidateTransaction]) -> None: for candidate in candidates: if candidate.state != ERROR: _validate_candidate(candidate) + if candidate.state != ERROR and candidate.account_hint: + # Only an *existing* account can disagree with the row - a brand-new + # account takes its currency from the first candidate that names it, at + # commit time, so there is nothing to compare against yet. + account = self.uow.accounts.get_by_name(candidate.account_hint) + if account is not None and account.currency.upper() != candidate.currency.upper(): + candidate.add_issue( + CURRENCY_ACCOUNT_MISMATCH, + f"row currency {candidate.currency} does not match " + f"{account.name}'s account currency {account.currency}", + ) def resolve_duplicates(self, candidates: Sequence[CandidateTransaction]) -> None: """Fingerprints valid rows, occurrence-aware, and matches them against the ledger.""" @@ -207,6 +227,16 @@ def commit( account = self.uow.accounts.get_or_create( candidate.account_hint or "Main account", candidate.currency ) + if account.currency.upper() != candidate.currency.upper(): + # validate() already blocks this for an existing account at preview time; + # reaching it here means a caller committed without validating first. Skip + # rather than raise - a currency mismatch must never crash a commit. + candidate.add_issue( + CURRENCY_ACCOUNT_MISMATCH, + f"row currency {candidate.currency} does not match " + f"{account.name}'s account currency {account.currency}", + ) + continue transaction = TransactionModel( external_id=candidate.external_id, account_id=account.id, diff --git a/src/pfa/planning/service.py b/src/pfa/planning/service.py index 49440e1..0ef8cd9 100644 --- a/src/pfa/planning/service.py +++ b/src/pfa/planning/service.py @@ -30,24 +30,28 @@ def __init__( self.accounts = accounts self.transactions = transactions - def _average_monthly_net(self, as_of: date, months: int = 3) -> int: + def _average_monthly_net(self, as_of: date, months: int = 3, currency: str = "GBP") -> int: values = [] cursor = as_of.replace(day=1) for _ in range(months): cursor = (cursor.replace(day=1) - timedelta(days=1)).replace(day=1) - values.append(self.analytics.monthly_summary(cursor).net_cashflow_minor) + values.append( + self.analytics.monthly_summary(cursor, currency=currency).net_cashflow_minor + ) return ( int((Decimal(sum(values)) / len(values)).quantize(Decimal("1"), ROUND_HALF_UP)) if values else 0 ) - def _average_monthly_spending(self, as_of: date, months: int = 3) -> int: + def _average_monthly_spending(self, as_of: date, months: int = 3, currency: str = "GBP") -> int: values = [] cursor = as_of.replace(day=1) for _ in range(months): cursor = (cursor.replace(day=1) - timedelta(days=1)).replace(day=1) - values.append(max(self.analytics.monthly_summary(cursor).spending_minor, 0)) + values.append( + max(self.analytics.monthly_summary(cursor, currency=currency).spending_minor, 0) + ) return ( int((Decimal(sum(values)) / len(values)).quantize(Decimal("1"), ROUND_HALF_UP)) if values @@ -55,12 +59,16 @@ def _average_monthly_spending(self, as_of: date, months: int = 3) -> int: ) def simulate_purchase( - self, cost_minor: int, horizon_months: int = 3, as_of: date | None = None + self, + cost_minor: int, + horizon_months: int = 3, + as_of: date | None = None, + currency: str = "GBP", ) -> ScenarioResult: as_of = as_of or date.today() - starting = current_cash(self.accounts, self.transactions, as_of) - monthly_net = self._average_monthly_net(as_of) - average_expenses = self._average_monthly_spending(as_of) + starting = current_cash(self.accounts, self.transactions, currency=currency, as_of=as_of) + monthly_net = self._average_monthly_net(as_of, currency=currency) + average_expenses = self._average_monthly_spending(as_of, currency=currency) baseline = starting + monthly_net * horizon_months scenario = baseline - cost_minor months = ( @@ -82,11 +90,11 @@ def simulate_purchase( assumptions=[ ( "average net cash flow from prior three complete months: " - f"{monthly_net} minor units" + f"{monthly_net} minor units ({currency})" ), ( "average spending from prior three complete months: " - f"{average_expenses} minor units" + f"{average_expenses} minor units ({currency})" ), "purchase occurs immediately; no investment returns assumed", f"horizon: {horizon_months} months", @@ -94,9 +102,13 @@ def simulate_purchase( ) def simulate_monthly_contribution( - self, additional_minor: int, horizon_months: int = 6, as_of: date | None = None + self, + additional_minor: int, + horizon_months: int = 6, + as_of: date | None = None, + currency: str = "GBP", ) -> ScenarioResult: - result = self.simulate_purchase(0, horizon_months, as_of) + result = self.simulate_purchase(0, horizon_months, as_of, currency=currency) scenario = result.baseline_month_end_cash_minor - additional_minor * horizon_months return result.model_copy( update={ @@ -105,7 +117,7 @@ def simulate_monthly_contribution( "affordable": scenario >= 0, "assumptions": [ *result.assumptions, - f"additional monthly contribution: {additional_minor} minor units", + f"additional monthly contribution: {additional_minor} minor units ({currency})", ], } ) diff --git a/src/pfa/services/fx.py b/src/pfa/services/fx.py new file mode 100644 index 0000000..ec58ffc --- /dev/null +++ b/src/pfa/services/fx.py @@ -0,0 +1,65 @@ +from __future__ import annotations + +import json +import logging +from datetime import date +from decimal import Decimal +from typing import TYPE_CHECKING + +import httpx + +if TYPE_CHECKING: + from pfa.db.models import FxRateModel + from pfa.db.unit_of_work import UnitOfWork + +logger = logging.getLogger("pfa") + +FRANKFURTER_API_BASE = "https://api.frankfurter.dev/v1" + + +def fetch_and_store_fx_rates( + uow: UnitOfWork, + base_currency: str = "GBP", + symbols: list[str] | None = None, + on_date: date | str | None = None, + client: httpx.Client | None = None, +) -> list[FxRateModel]: + base = base_currency.upper() + symbols_list = symbols or ["EUR", "INR", "USD", "JPY"] + filtered_symbols = [s.upper() for s in symbols_list if s.upper() != base] + if not filtered_symbols: + return [] + + symbols_str = ",".join(filtered_symbols) + date_segment = on_date.isoformat() if isinstance(on_date, date) else (on_date or "latest") + url = f"{FRANKFURTER_API_BASE}/{date_segment}?base={base}&symbols={symbols_str}" + + close_client = False + if client is None: + client = httpx.Client(timeout=15.0) + close_client = True + + try: + response = client.get(url) + response.raise_for_status() + # Parse the response's own JSON numbers straight to Decimal - going through + # response.json() would round-trip every rate through a binary float first. + payload = json.loads(response.text, parse_float=Decimal) + finally: + if close_client: + client.close() + + effective_date = date.fromisoformat(payload["date"]) + rates_data: dict[str, Decimal] = payload.get("rates", {}) + stored: list[FxRateModel] = [] + for quote, rate_value in rates_data.items(): + rate_model = uow.fx_rates.set_rate( + base_currency=base, + quote_currency=quote, + rate=str(rate_value), + effective_at=effective_date, + source="frankfurter", + ) + stored.append(rate_model) + + return stored diff --git a/src/pfa/services/review.py b/src/pfa/services/review.py index 7f15d5d..9d6793d 100644 --- a/src/pfa/services/review.py +++ b/src/pfa/services/review.py @@ -3,17 +3,23 @@ from pfa.analytics.service import AnalyticsService -def monthly_review_evidence(analytics: AnalyticsService, period: date) -> dict[str, object]: +def monthly_review_evidence( + analytics: AnalyticsService, period: date, currency: str = "GBP" +) -> dict[str, object]: """Build the authoritative evidence bundle used by the review narrator.""" - previous = analytics.compare_periods(period).previous + previous = analytics.compare_periods(period, currency=currency).previous return { - "summary": analytics.monthly_summary(period).model_dump(), - "categories": [item.model_dump() for item in analytics.category_spending(period)], - "comparison": analytics.compare_periods(period).model_dump(), + "summary": analytics.monthly_summary(period, currency=currency).model_dump(), + "categories": [ + item.model_dump() for item in analytics.category_spending(period, currency=currency) + ], + "comparison": analytics.compare_periods(period, currency=currency).model_dump(), "previous_summary": previous.model_dump(), - "recurring_payments": analytics.recurring_payments(), - "budget_status": [item.model_dump() for item in analytics.budget_status(period)], + "recurring_payments": analytics.recurring_payments(currency=currency), + "budget_status": [ + item.model_dump() for item in analytics.budget_status(period, currency=currency) + ], "goal_progress": [item.model_dump() for item in analytics.goal_progress()], - "category_spikes": analytics.category_spikes(period), - "unusual_transactions": analytics.unusual_transactions(period), + "category_spikes": analytics.category_spikes(period, currency=currency), + "unusual_transactions": analytics.unusual_transactions(period, currency=currency), } diff --git a/tests/integration/test_api.py b/tests/integration/test_api.py index 9cc4822..7ac9e3b 100644 --- a/tests/integration/test_api.py +++ b/tests/integration/test_api.py @@ -51,3 +51,45 @@ def test_dashboard_and_static_assets_are_served(tmp_path) -> None: js_resp = client.get("/static/app.js") assert js_resp.status_code == 200 assert "javascript" in js_resp.headers.get("content-type", "") + + +def test_api_fx_rates_endpoints(tmp_path) -> None: + database_url = f"sqlite:///{tmp_path / 'pfa.db'}" + config = Config("alembic.ini") + config.set_main_option("sqlalchemy.url", database_url) + command.upgrade(config, "head") + app = create_app(Settings(database_url=database_url)) + with TestClient(app) as client: + # Set manual rate + post_resp = client.post( + "/fx/rates", + json={ + "base_currency": "GBP", + "quote_currency": "INR", + "rate": "105.5", + "effective_at": "2026-08-01", + }, + ) + assert post_resp.status_code == 200 + data = post_resp.json() + assert data["base_currency"] == "GBP" + assert data["quote_currency"] == "INR" + assert data["rate"] == "105.5" + + bad_resp = client.post( + "/fx/rates", + json={ + "base_currency": "GBP", + "quote_currency": "USD", + "rate": "not-a-number", + "effective_at": "2026-08-01", + }, + ) + assert bad_resp.status_code == 422 + + # Get rates + get_resp = client.get("/fx/rates?base=GBP") + assert get_resp.status_code == 200 + rates = get_resp.json() + assert len(rates) == 1 + assert rates[0]["quote_currency"] == "INR" diff --git a/tests/integration/test_cli.py b/tests/integration/test_cli.py index e1bcdfb..efb730f 100644 --- a/tests/integration/test_cli.py +++ b/tests/integration/test_cli.py @@ -9,3 +9,23 @@ def test_cli_missing_import_file_is_a_clean_usage_error(tmp_path) -> None: assert result.exit_code == 2 assert "path must identify a local CSV file" in result.output assert "Traceback" not in result.output + + +def test_cli_fx_commands(tmp_path) -> None: + database_url = f"sqlite:///{tmp_path / 'pfa.db'}" + from alembic import command + from alembic.config import Config + + config = Config("alembic.ini") + config.set_main_option("sqlalchemy.url", database_url) + command.upgrade(config, "head") + + runner = CliRunner(env={"PFA_DATABASE_URL": database_url}) + set_res = runner.invoke(app, ["fx", "set", "GBP", "USD", "1.30", "--date", "2026-08-01"]) + assert set_res.exit_code == 0 + assert "FX rate GBP/USD = 1.30 set" in set_res.output + + list_res = runner.invoke(app, ["fx", "list"]) + assert list_res.exit_code == 0 + assert "GBP" in list_res.output + assert "USD" in list_res.output diff --git a/tests/unit/test_financial_invariants.py b/tests/unit/test_financial_invariants.py index faa1265..8eaaf4c 100644 --- a/tests/unit/test_financial_invariants.py +++ b/tests/unit/test_financial_invariants.py @@ -123,14 +123,15 @@ def test_dry_run_rolls_back_accounts_transactions_and_state(tmp_path) -> None: def test_unsupported_currency_fails_closed_instead_of_reporting_false_gbp(tmp_path) -> None: path = tmp_path / "currency.csv" path.write_text( - "date,description,amount,kind,currency\n2026-08-01,Salary,1000,income,USD\n", + "date,description,amount,kind,currency\n2026-08-01,Salary,1000,income,XYZ\n", encoding="utf-8", ) engine, uow, _ = services() result = ImportService(uow).import_csv(path) assert result.imported == 0 - assert result.errors == ["row 2: unsupported currency 'USD'; PFA v0.1 supports GBP only"] + assert len(result.errors) == 1 + assert "unsupported currency 'XYZ'" in result.errors[0] assert uow.transactions.all() == [] uow.session.close() engine.dispose() @@ -201,3 +202,60 @@ def test_headerless_export_imports_every_row_with_the_signs_it_was_written_with( ] uow.session.close() engine.dispose() + + +def test_mixed_currency_analytics_strictly_partitions_currencies_without_sum_pollution( + tmp_path, +) -> None: + """Invariant: an INR account alongside GBP must NEVER sum into 405,000 of something.""" + path_gbp = tmp_path / "gbp.csv" + path_gbp.write_text( + "date,description,amount,kind,category,currency,account\n" + "2026-08-01,Salary,5000,income,,GBP,UK Bank\n" + "2026-08-05,Groceries,-200,expense,groceries,GBP,UK Bank\n", + encoding="utf-8", + ) + path_inr = tmp_path / "inr.csv" + path_inr.write_text( + "date,description,amount,kind,category,currency,account\n" + "2026-08-01,Consulting,400000,income,,INR,India Bank\n" + "2026-08-10,Rent,-50000,expense,housing,INR,India Bank\n", + encoding="utf-8", + ) + + engine, uow, analytics = services() + importer = ImportService(uow) + importer.import_csv(path_gbp) + importer.import_csv(path_inr) + + # Check GBP analytics + gbp_summary = analytics.monthly_summary(date(2026, 8, 1), currency="GBP") + assert gbp_summary.currency == "GBP" + assert gbp_summary.income_minor == 500_000 # 5,000.00 GBP + assert gbp_summary.spending_minor == 20_000 # 200.00 GBP + assert gbp_summary.net_cashflow_minor == 480_000 + assert gbp_summary.transaction_count == 2 + + # Check INR analytics + inr_summary = analytics.monthly_summary(date(2026, 8, 1), currency="INR") + assert inr_summary.currency == "INR" + assert inr_summary.income_minor == 40_000_000 # 400,000.00 INR + assert inr_summary.spending_minor == 5_000_000 # 50,000.00 INR + assert inr_summary.net_cashflow_minor == 35_000_000 + assert inr_summary.transaction_count == 2 + + # Verify category spending is partitioned + gbp_cats = { + item.category: item.total_minor + for item in analytics.category_spending(date(2026, 8, 1), currency="GBP") + } + assert gbp_cats == {"groceries": 20_000} + + inr_cats = { + item.category: item.total_minor + for item in analytics.category_spending(date(2026, 8, 1), currency="INR") + } + assert inr_cats == {"housing": 5_000_000} + + uow.session.close() + engine.dispose() diff --git a/tests/unit/test_fx.py b/tests/unit/test_fx.py new file mode 100644 index 0000000..cdbd07c --- /dev/null +++ b/tests/unit/test_fx.py @@ -0,0 +1,151 @@ +from datetime import date +from decimal import Decimal + +import httpx +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import Session + +from pfa.db.models import Base +from pfa.db.repositories import FxRateRepository +from pfa.db.unit_of_work import UnitOfWork +from pfa.domain.errors import ValidationError +from pfa.domain.fx import to_base +from pfa.domain.money import Money +from pfa.services.fx import fetch_and_store_fx_rates + + +@pytest.fixture +def session(): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + with Session(engine) as sess: + yield sess + engine.dispose() + + +@pytest.fixture +def fx_repo(session): + return FxRateRepository(session) + + +def test_set_and_retrieve_fx_rate(fx_repo): + model = fx_repo.set_rate("INR", "GBP", "0.0095", date(2026, 8, 1), source="manual") + assert model.rate == "0.0095" + assert model.base_currency == "INR" + assert model.quote_currency == "GBP" + + # Upsert with new rate on same date + updated = fx_repo.set_rate("INR", "GBP", "0.0096", date(2026, 8, 1), source="manual") + assert updated.rate == "0.0096" + assert len(fx_repo.all()) == 1 + + +def test_rate_on_date_semantics(fx_repo): + fx_repo.set_rate("INR", "GBP", "0.0090", date(2026, 8, 1)) + fx_repo.set_rate("INR", "GBP", "0.0095", date(2026, 8, 15)) + + # Before earliest rate -> None + assert fx_repo.rate_on(date(2026, 7, 31), "INR", "GBP") is None + + # On exact date + rate_aug1, model1 = fx_repo.rate_on(date(2026, 8, 1), "INR", "GBP") + assert rate_aug1 == Decimal("0.0090") + assert model1.effective_at == date(2026, 8, 1) + + # Between dates -> nearest rate at or before + rate_aug10, model10 = fx_repo.rate_on(date(2026, 8, 10), "INR", "GBP") + assert rate_aug10 == Decimal("0.0090") + + # On later date + rate_aug15, model15 = fx_repo.rate_on(date(2026, 8, 15), "INR", "GBP") + assert rate_aug15 == Decimal("0.0095") + + # After latest date -> stays at latest rate at or before + rate_aug20, model20 = fx_repo.rate_on(date(2026, 8, 20), "INR", "GBP") + assert rate_aug20 == Decimal("0.0095") + + +def test_inverse_rate_resolution(fx_repo): + # Store GBP to EUR rate: 1 GBP = 1.20 EUR + fx_repo.set_rate("GBP", "EUR", "1.20", date(2026, 8, 1)) + + # Rate from EUR to GBP should be 1 / 1.20 = 0.8333... + rate_eur_gbp, model = fx_repo.rate_on(date(2026, 8, 10), "EUR", "GBP") + assert rate_eur_gbp == Decimal(1) / Decimal("1.20") + assert model.base_currency == "GBP" + + +def test_to_base_conversion(fx_repo): + fx_repo.set_rate("INR", "GBP", "0.00863", date(2026, 8, 29)) + + # Convert 100,000 INR (10,000,000 minor) to GBP + inr_money = Money(10_000_000, "INR") # 100,000.00 INR + converted, rate_used = to_base(inr_money, date(2026, 8, 29), fx_repo, "GBP") + + # 100,000 * 0.00863 = 863.00 GBP -> 86300 minor + assert converted.currency == "GBP" + assert converted.minor == 86300 + assert rate_used.rate == Decimal("0.00863") + assert rate_used.base_currency == "INR" + assert rate_used.quote_currency == "GBP" + + +def test_to_base_identity_for_same_currency(fx_repo): + gbp_money = Money(5000, "GBP") + converted, rate_used = to_base(gbp_money, date(2026, 8, 29), fx_repo, "GBP") + assert converted.currency == "GBP" + assert converted.minor == 5000 + assert rate_used.rate == Decimal("1.0") + + +def test_to_base_missing_rate_raises(fx_repo): + usd_money = Money(1000, "USD") + with pytest.raises(ValidationError, match="No FX rate available"): + to_base(usd_money, date(2026, 8, 29), fx_repo, "GBP") + + +def test_fetch_and_store_fx_rates_keeps_full_decimal_precision(session): + """The response's JSON numbers must never round-trip through a binary float - a rate + with more decimal digits than float can hold exactly must be stored byte-for-byte.""" + body = ( + b'{"amount":1.0,"base":"GBP","date":"2026-08-28",' + b'"rates":{"INR":129.123456789012345,"USD":1.3583}}' + ) + + def handler(request: httpx.Request) -> httpx.Response: + assert request.url.params["base"] == "GBP" + return httpx.Response(200, content=body, headers={"content-type": "application/json"}) + + client = httpx.Client(transport=httpx.MockTransport(handler)) + uow = UnitOfWork(session) + + stored = fetch_and_store_fx_rates( + uow, base_currency="GBP", on_date=date(2026, 8, 28), client=client + ) + + by_quote = {model.quote_currency: model for model in stored} + assert by_quote["INR"].rate == "129.123456789012345" + assert by_quote["INR"].source == "frankfurter" + assert by_quote["USD"].rate == "1.3583" + assert by_quote["INR"].effective_at == date(2026, 8, 28) + + rate_decimal, _ = uow.fx_rates.rate_on(date(2026, 8, 28), "GBP", "INR") + assert rate_decimal == Decimal("129.123456789012345") + + +def test_fetch_and_store_fx_rates_excludes_base_from_symbols(session): + def handler(request: httpx.Request) -> httpx.Response: + assert "GBP" not in request.url.params["symbols"].split(",") + return httpx.Response( + 200, + json={"amount": 1.0, "base": "GBP", "date": "2026-08-28", "rates": {"INR": 129.5}}, + ) + + client = httpx.Client(transport=httpx.MockTransport(handler)) + uow = UnitOfWork(session) + + stored = fetch_and_store_fx_rates( + uow, base_currency="GBP", symbols=["GBP", "INR"], on_date=date(2026, 8, 28), client=client + ) + assert [m.quote_currency for m in stored] == ["INR"] diff --git a/tests/unit/test_money.py b/tests/unit/test_money.py index ab606da..68df0bd 100644 --- a/tests/unit/test_money.py +++ b/tests/unit/test_money.py @@ -11,6 +11,23 @@ def test_money_rounds_to_integer_minor_units() -> None: assert Money(1234).to_major() == Decimal("12.34") +def test_money_supports_different_currency_minor_units() -> None: + # JPY has exponent 0 (no decimal places) + jpy = Money.from_major("1500", "JPY") + assert jpy.minor == 1500 + assert jpy.to_major() == Decimal("1500") + + # INR has exponent 2 + inr = Money.from_major("450.50", "INR") + assert inr.minor == 45050 + assert inr.to_major() == Decimal("450.50") + + +def test_money_rejects_unsupported_currency() -> None: + with pytest.raises(ValidationError, match="Unsupported currency"): + Money(100, "XYZ") + + def test_money_rejects_mixed_currency_arithmetic() -> None: with pytest.raises(ValidationError): Money(100, "GBP") + Money(100, "USD") diff --git a/tests/unit/test_pdf_extractor.py b/tests/unit/test_pdf_extractor.py index bd1ed61..333c3fa 100644 --- a/tests/unit/test_pdf_extractor.py +++ b/tests/unit/test_pdf_extractor.py @@ -11,6 +11,7 @@ from pfa.ingestion import candidates as codes # noqa: E402 from pfa.ingestion.candidates import ExtractionResult, StatementSource # noqa: E402 +from pfa.ingestion.dialects import BARCLAYCARD # noqa: E402 from pfa.ingestion.extractors.pdf import PdfStatementExtractor # noqa: E402 @@ -111,7 +112,7 @@ def test_continuation_line_far_from_any_row_is_dropped_as_noise(tmp_path: Path) assert result.candidates[0].raw_description == "Tesco Metro" -def test_amex_banner_header_does_not_turn_statement_chatter_into_candidates( +def test_amex_banner_header_and_statement_chatter_never_become_candidates( tmp_path: Path, ) -> None: columns = [72.0, 160.0, 300.0, 420.0, 500.0] @@ -129,8 +130,22 @@ def test_amex_banner_header_does_not_turn_statement_chatter_into_candidates( result = _extract(tmp_path, [statement_page(rows, columns)]) - assert [(c.transaction_date, c.raw_description) for c in result.candidates] == [] - assert [issue.code for issue in result.issues] == [codes.PDF_NOT_EXTRACTABLE] + assert [(c.transaction_date, c.amount_minor) for c in result.candidates] == [ + ("Jul31", 194064), + ("Jul21", 630), + ] + assert result.issues == [] + # The repeated "Date" column (AMEX prints it twice) never leaks into the description. + assert [c.raw_description for c in result.candidates] == [ + "PAYMENT RECEIVED - THANK YOU", + "ZETTLE *REDACTED", + ] + # The own-line "CR" marker attaches to the row above it, not a candidate of its own, + # and marks that row's direction explicit so a later sign convention cannot flip it. + payment, purchase = result.candidates + assert payment.direction == "credit" + assert payment.direction_explicit is True + assert purchase.direction_explicit is False def test_header_with_zero_plausible_data_rows_reports_pdf_not_extractable( @@ -266,7 +281,7 @@ def test_no_recognizable_rows_reports_pdf_not_extractable_with_actionable_copy( def test_money_out_and_money_in_headers_are_recognised(tmp_path: Path) -> None: # Monzo, Starling and Lloyds all label their columns this way. The CSV extractor has - # always known the wording; the PDF map did not, until both read one shared table. + # always known the wording; this proves the PDF word-based header map reads it too. columns = [72.0, 160.0, 320.0, 420.0] rows = [ ["Date", "Description", "Money Out", "Money In"], @@ -278,3 +293,31 @@ def test_money_out_and_money_in_headers_are_recognised(tmp_path: Path) -> None: assert result.issues == [] assert [c.amount_minor for c in result.candidates] == [1250, 300000] assert [c.direction for c in result.candidates] == ["debit", "credit"] + + +def test_barclaycard_two_column_layout_clustering(tmp_path: Path) -> None: + # Left column: transactions at x ~ 50..280 + # Right column: marketing copy at x ~ 350..550 + left_columns = [50.0, 130.0, 260.0] + left_rows = [ + ["Date", "Description", "Amount"], + ["27 Jul 25", "COFFEE HOUSE LONDON", "-3.50"], + ["28 Jul 25", "NEWSAGENT LEEDS", "-2.10"], + ] + + page = statement_page(left_rows, left_columns) + # Add right column marketing words at same y positions + page.append((360.0, 720.0, "Understanding your interest", 10.0)) + page.append((360.0, 706.0, "Your interest rates this month", 10.0)) + page.append((360.0, 692.0, "Visit barclaycard.co.uk", 10.0)) + + result = _extract(tmp_path, [page], dialect=BARCLAYCARD) + + assert result.issues == [] + assert len(result.candidates) == 2 + assert [c.transaction_date for c in result.candidates] == ["27 Jul 25", "28 Jul 25"] + assert [c.raw_description for c in result.candidates] == [ + "COFFEE HOUSE LONDON", + "NEWSAGENT LEEDS", + ] + assert [c.amount_minor for c in result.candidates] == [350, 210] diff --git a/tests/unit/test_planning_scenarios.py b/tests/unit/test_planning_scenarios.py index 3099d6d..44867f9 100644 --- a/tests/unit/test_planning_scenarios.py +++ b/tests/unit/test_planning_scenarios.py @@ -6,9 +6,10 @@ class StableHistory: - def monthly_summary(self, period: date) -> MonthlySummary: + def monthly_summary(self, period: date, currency: str = "GBP") -> MonthlySummary: return MonthlySummary( period=period.strftime("%Y-%m"), + currency=currency, income_minor=400_000, spending_minor=300_000, net_cashflow_minor=100_000, diff --git a/tests/unit/test_statement_candidates.py b/tests/unit/test_statement_candidates.py index 7d4ef6b..bec3cb8 100644 --- a/tests/unit/test_statement_candidates.py +++ b/tests/unit/test_statement_candidates.py @@ -29,7 +29,7 @@ def test_validation_reports_one_issue_code_per_blocking_problem() -> None: candidate("c1", transaction_date="not-a-date"), candidate("c2", raw_description=""), candidate("c3", amount="not-a-number"), - candidate("c4", currency="EUR"), + candidate("c4", currency="XYZ"), candidate("c5", kind="teleportation"), candidate("c6", kind="expense", category="submarines"), candidate("c7", kind="transfer", transfer_purpose="hoarding"), From 1bea0b300ec674bc020a885193be47461ca76f33 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 09:27:55 +0100 Subject: [PATCH 02/24] fix(test): isolate CLI fx tests from process-wide settings cache pfa.config.get_settings() is @lru_cache'd. Any PdfStatementExtractor() built with default settings (many unit tests do this) primes that cache with the real sqlite:///data/pfa.db - and once cached, CliRunner(env={"PFA_DATABASE_URL": ...}) has no effect for the rest of the process, so test_cli_fx_commands ran against whatever DB happened to be cached first instead of its own tmp_path DB. Reproduced locally by forcing collection order: pytest tests/unit/test_pdf_extractor.py tests/integration/test_cli.py and confirmed the autouse cache-clear fixture fixes it regardless of order. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01T8foThd2rk578JQmV3JHLQ --- tests/integration/test_cli.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/integration/test_cli.py b/tests/integration/test_cli.py index efb730f..3bdffae 100644 --- a/tests/integration/test_cli.py +++ b/tests/integration/test_cli.py @@ -1,6 +1,19 @@ +import pytest from typer.testing import CliRunner from pfa.cli.app import app +from pfa.config import get_settings + + +@pytest.fixture(autouse=True) +def _reset_settings_cache(): + """get_settings() is process-wide @lru_cache'd. A test that points PFA_DATABASE_URL + at a tmp_path DB must not inherit a stale cached Settings from an earlier test in this + file, nor leak its own tmp_path-scoped Settings into whatever runs after it. + """ + get_settings.cache_clear() + yield + get_settings.cache_clear() def test_cli_missing_import_file_is_a_clean_usage_error(tmp_path) -> None: From 2fe9562a6d53d58531ca72b2836e5c482ce99bf8 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:22:06 +0100 Subject: [PATCH 03/24] feat(imports): bind statements to typed accounts --- .../versions/0005_account_import_binding.py | 90 +++++ src/pfa/api/app.py | 106 ++++- src/pfa/db/models.py | 15 +- src/pfa/db/repositories.py | 66 ++- src/pfa/domain/accounts.py | 22 +- src/pfa/domain/transactions.py | 6 + src/pfa/ingestion/batches.py | 376 ++++++++++++++++-- src/pfa/ingestion/candidates.py | 17 + src/pfa/ingestion/categorizer.py | 31 +- src/pfa/ingestion/dialects.py | 143 ++++++- src/pfa/ingestion/extractors/csv.py | 5 +- src/pfa/ingestion/extractors/pdf.py | 52 ++- src/pfa/ingestion/service.py | 62 ++- 13 files changed, 925 insertions(+), 66 deletions(-) create mode 100644 alembic/versions/0005_account_import_binding.py diff --git a/alembic/versions/0005_account_import_binding.py b/alembic/versions/0005_account_import_binding.py new file mode 100644 index 0000000..785396d --- /dev/null +++ b/alembic/versions/0005_account_import_binding.py @@ -0,0 +1,90 @@ +"""stable account binding and adapter metadata + +Revision ID: 0005_account_import_binding +Revises: 0004_fx_rates +""" + +import sqlalchemy as sa +from alembic import op + +revision = "0005_account_import_binding" +down_revision = "0004_fx_rates" +branch_labels = None +depends_on = None + + +def _rebuild_accounts(unique: bool) -> None: + bind = op.get_bind() + bind.exec_driver_sql("PRAGMA foreign_keys=OFF") + op.create_table( + "_accounts_new", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("name", sa.String(length=120), nullable=False), + sa.Column("account_type", sa.String(length=30), nullable=False), + sa.Column("currency", sa.String(length=3), nullable=False), + sa.Column("institution", sa.String(length=120), nullable=True), + sa.Column("last4", sa.String(length=4), nullable=True), + sa.Column("opening_balance_minor", sa.Integer(), nullable=False), + sa.Column("opening_balance_as_of", sa.Date(), nullable=True), + sa.Column("active", sa.Boolean(), nullable=False), + sa.PrimaryKeyConstraint("id"), + *([sa.UniqueConstraint("name")] if unique else []), + ) + op.execute( + """INSERT INTO _accounts_new + (id, name, account_type, currency, institution, last4, + opening_balance_minor, opening_balance_as_of, active) + SELECT id, name, account_type, currency, NULL, NULL, + opening_balance_minor, NULL, active + FROM accounts""" + ) + op.drop_table("accounts") + op.rename_table("_accounts_new", "accounts") + bind.exec_driver_sql("PRAGMA foreign_keys=ON") + + +def upgrade() -> None: + _rebuild_accounts(unique=False) + op.add_column( + "import_batches", sa.Column("destination_account_id", sa.Integer(), nullable=True) + ) + op.create_index( + "ix_import_batches_destination_account_id", + "import_batches", + ["destination_account_id"], + ) + with op.batch_alter_table("import_batches", recreate="always") as batch_op: + batch_op.create_foreign_key( + "fk_import_batches_destination_account_id", + "accounts", + ["destination_account_id"], + ["id"], + ) + for column in ( + sa.Column("new_account_json", sa.Text(), nullable=True), + sa.Column("adapter_id", sa.String(length=80), nullable=True), + sa.Column("detection_confidence", sa.Float(), nullable=True), + sa.Column("detection_reason_codes_json", sa.Text(), nullable=True), + sa.Column("detected_institution", sa.String(length=120), nullable=True), + sa.Column("detected_account_hint", sa.String(length=40), nullable=True), + sa.Column("reconciliation_json", sa.Text(), nullable=True), + ): + op.add_column("import_batches", column) + + +def downgrade() -> None: + for name in ( + "reconciliation_json", + "detected_account_hint", + "detected_institution", + "detection_reason_codes_json", + "detection_confidence", + "adapter_id", + "new_account_json", + ): + op.drop_column("import_batches", name) + with op.batch_alter_table("import_batches", recreate="always") as batch_op: + batch_op.drop_constraint("fk_import_batches_destination_account_id", type_="foreignkey") + op.drop_index("ix_import_batches_destination_account_id", table_name="import_batches") + op.drop_column("import_batches", "destination_account_id") + _rebuild_accounts(unique=True) diff --git a/src/pfa/api/app.py b/src/pfa/api/app.py index 4fc431f..ccf5a3c 100644 --- a/src/pfa/api/app.py +++ b/src/pfa/api/app.py @@ -1,5 +1,6 @@ from __future__ import annotations +import json from collections.abc import AsyncIterator, Awaitable, Callable from contextlib import asynccontextmanager, suppress from datetime import date, datetime @@ -8,7 +9,7 @@ from typing import Annotated, Literal from fastapi import FastAPI, File, Form, HTTPException, Query, UploadFile -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator from pydantic_ai import UsageLimits from starlette.requests import Request from starlette.responses import Response @@ -21,14 +22,17 @@ from pfa.ai.schemas import ChatRequest, ImportRequest from pfa.config import Settings, get_settings from pfa.db.models import ImportBatchModel +from pfa.domain.accounts import AccountType from pfa.domain.errors import BatchError, UploadRejected from pfa.ingestion.batches import ( BatchPatch, + NewAccountDraft, apply_patch, batch_candidates, batch_committed_transaction_ids, batch_counts, batch_issues, + batch_semantic_totals, commit_batch, create_batch, discard_batch, @@ -64,6 +68,21 @@ class AccountResponse(BaseModel): name: str account_type: str currency: str + institution: str | None = None + last4: str | None = None + opening_balance_minor: int = 0 + opening_balance_as_of: date | None = None + active: bool = True + + +class NewAccountRequest(BaseModel): + name: str = Field(min_length=1, max_length=120) + account_type: AccountType = AccountType.CURRENT + currency: str = Field(default="GBP", min_length=3, max_length=3) + institution: str | None = Field(default=None, max_length=120) + last4: str | None = Field(default=None, min_length=4, max_length=4, pattern=r"^\\d{4}$") + opening_balance_minor: int = 0 + opening_balance_as_of: date | None = None class CandidateIssueResponse(BaseModel): @@ -82,6 +101,8 @@ class CandidateResponse(BaseModel): direction: str | None currency: str account_hint: str | None + account_id: int | None + signed_amount_minor: int | None external_id: str | None kind: str | None category: str | None @@ -106,6 +127,15 @@ class ImportBatchResponse(BaseModel): sha256: str extractor: str destination_account: str | None + destination_account_id: int | None + new_account: NewAccountRequest | None + adapter_id: str | None + detection_confidence: float | None + detection_reason_codes: list[str] + detected_institution: str | None + detected_account_hint: str | None + reconciliation: dict[str, object] | None + semantic_totals: dict[str, int] amount_sign: str | None detected_account: str | None detected_currency: str | None @@ -123,8 +153,21 @@ class ImportBatchResponse(BaseModel): class ImportBatchPatchRequest(BaseModel): - account: str | None = None + account: str | None = None # deprecated label compatibility + destination_account_id: int | None = Field(default=None, gt=0) + new_account: NewAccountRequest | None = None excluded_candidate_ids: list[str] | None = None + + @model_validator(mode="after") + def one_binding(self) -> ImportBatchPatchRequest: + if self.destination_account_id is not None and self.new_account is not None: + raise ValueError("choose destination_account_id or new_account, not both") + if self.account is not None and ( + self.destination_account_id is not None or self.new_account is not None + ): + raise ValueError("account is a legacy alias; use one stable binding") + return self + # Both are closed sets: an unrecognised value is a 422, not a silent no-op. amount_mode: Literal["debit", "credit"] | None = None amount_sign: Literal["as_written", "debit_positive"] | None = None @@ -173,6 +216,8 @@ def _candidate_response(candidate: CandidateTransaction) -> CandidateResponse: direction=candidate.direction, currency=candidate.currency, account_hint=candidate.account_hint, + account_id=candidate.account_id, + signed_amount_minor=candidate.signed_amount_minor, external_id=candidate.external_id, kind=candidate.kind, category=candidate.category, @@ -199,6 +244,25 @@ def _batch_response(batch: ImportBatchModel) -> ImportBatchResponse: sha256=batch.sha256, extractor=batch.extractor, destination_account=batch.destination_account, + destination_account_id=batch.destination_account_id, + new_account=( + NewAccountRequest(**json.loads(batch.new_account_json)) + if batch.new_account_json + else None + ), + adapter_id=batch.adapter_id, + detection_confidence=batch.detection_confidence, + detection_reason_codes=( + json.loads(batch.detection_reason_codes_json) + if batch.detection_reason_codes_json + else [] + ), + detected_institution=batch.detected_institution, + detected_account_hint=batch.detected_account_hint, + reconciliation=( + json.loads(batch.reconciliation_json) if batch.reconciliation_json else None + ), + semantic_totals=batch_semantic_totals(batch), amount_sign=batch.amount_sign, detected_account=batch.detected_account, detected_currency=batch.detected_currency, @@ -284,6 +348,10 @@ def imports_preview( request: Request, file: UploadFile = File(...), # noqa: B008 - FastAPI's dependency-injection idiom account: str | None = Form(None), # noqa: B008 + destination_account_id: int | None = Form(None), # noqa: B008 + new_account_name: str | None = Form(None), # noqa: B008 + new_account_type: AccountType = Form(AccountType.CURRENT), # noqa: B008 + new_account_currency: str = Form("GBP"), # noqa: B008 ) -> ImportBatchResponse: content_length = request.headers.get("content-length") # A header the client controls must not be able to turn a bad request into a 500; @@ -300,7 +368,23 @@ def imports_preview( try: engine, services = open_services(active_settings) try: - batch = create_batch(services.uow, source, active_settings, account=account) + draft = ( + NewAccountRequest( + name=new_account_name, + account_type=new_account_type, + currency=new_account_currency, + ) + if new_account_name + else None + ) + batch = create_batch( + services.uow, + source, + active_settings, + account=account, + destination_account_id=destination_account_id, + new_account=NewAccountDraft(**draft.model_dump()) if draft else None, + ) response = _batch_response(batch) close_services(engine, services) return response @@ -341,6 +425,12 @@ def patch_import_batch(batch_id: str, request: ImportBatchPatchRequest) -> Impor batch_id, BatchPatch( account=request.account, + destination_account_id=request.destination_account_id, + new_account=( + NewAccountDraft(**request.new_account.model_dump()) + if request.new_account + else None + ), excluded_candidate_ids=request.excluded_candidate_ids, amount_mode=request.amount_mode, amount_sign=request.amount_sign, @@ -398,7 +488,15 @@ def accounts() -> list[AccountResponse]: try: return [ AccountResponse( - id=acc.id, name=acc.name, account_type=acc.account_type, currency=acc.currency + id=acc.id, + name=acc.name, + account_type=acc.account_type, + currency=acc.currency, + institution=acc.institution, + last4=acc.last4, + opening_balance_minor=acc.opening_balance_minor, + opening_balance_as_of=acc.opening_balance_as_of, + active=acc.active, ) for acc in services.uow.accounts.all() ] diff --git a/src/pfa/db/models.py b/src/pfa/db/models.py index dd6e60f..7ffd1f1 100644 --- a/src/pfa/db/models.py +++ b/src/pfa/db/models.py @@ -23,10 +23,13 @@ class Base(DeclarativeBase): class AccountModel(Base): __tablename__ = "accounts" id: Mapped[int] = mapped_column(primary_key=True) - name: Mapped[str] = mapped_column(String(120), unique=True) + name: Mapped[str] = mapped_column(String(120)) account_type: Mapped[str] = mapped_column(String(30), default="current") currency: Mapped[str] = mapped_column(String(3), default="GBP") + institution: Mapped[str | None] = mapped_column(String(120), nullable=True) + last4: Mapped[str | None] = mapped_column(String(4), nullable=True) opening_balance_minor: Mapped[int] = mapped_column(Integer, default=0) + opening_balance_as_of: Mapped[date | None] = mapped_column(Date, nullable=True) active: Mapped[bool] = mapped_column(Boolean, default=True) transactions: Mapped[list[TransactionModel]] = relationship(back_populates="account") @@ -97,6 +100,16 @@ class ImportBatchModel(Base): extractor: Mapped[str] = mapped_column(String(60)) status: Mapped[str] = mapped_column(String(20), index=True) destination_account: Mapped[str | None] = mapped_column(String(120), nullable=True) + destination_account_id: Mapped[int | None] = mapped_column( + ForeignKey("accounts.id"), nullable=True, index=True + ) + new_account_json: Mapped[str | None] = mapped_column(Text, nullable=True) + adapter_id: Mapped[str | None] = mapped_column(String(80), nullable=True) + detection_confidence: Mapped[float | None] = mapped_column(nullable=True) + detection_reason_codes_json: Mapped[str | None] = mapped_column(Text, nullable=True) + detected_institution: Mapped[str | None] = mapped_column(String(120), nullable=True) + detected_account_hint: Mapped[str | None] = mapped_column(String(40), nullable=True) + reconciliation_json: Mapped[str | None] = mapped_column(Text, nullable=True) # The sign convention the user declared for this source, kept so the preview can be # rebuilt after a refresh and so a committed batch records how it read its amounts. amount_sign: Mapped[str | None] = mapped_column(String(20), nullable=True) diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index f846d4f..a8aac5a 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -6,6 +6,8 @@ from sqlalchemy import select from sqlalchemy.orm import Session +from pfa.domain.accounts import AccountType +from pfa.domain.money import SUPPORTED_CURRENCIES from pfa.domain.transactions import TransactionKind from .models import ( @@ -41,6 +43,13 @@ def between(self, start: date, end: date) -> list[TransactionModel]: ) return list(self.session.scalars(statement)) + def by_ids(self, ids: list[int]) -> list[TransactionModel]: + if not ids: + return [] + return list( + self.session.scalars(select(TransactionModel).where(TransactionModel.id.in_(ids))) + ) + def find_fingerprint(self, fingerprint: str) -> TransactionModel | None: return self.session.scalar( select(TransactionModel).where(TransactionModel.fingerprint == fingerprint) @@ -68,20 +77,63 @@ class AccountRepository: def __init__(self, session: Session): self.session = session + def get(self, account_id: int) -> AccountModel | None: + return self.session.get(AccountModel, account_id) + + def create( + self, + name: str, + currency: str = "GBP", + account_type: str = AccountType.CURRENT.value, + *, + institution: str | None = None, + last4: str | None = None, + opening_balance_minor: int = 0, + opening_balance_as_of: date | None = None, + active: bool = True, + ) -> AccountModel: + if not name.strip(): + raise ValueError("account name is required") + account_type = AccountType(account_type).value + currency = currency.upper() + if currency not in SUPPORTED_CURRENCIES: + raise ValueError(f"unsupported account currency {currency!r}") + if last4 is not None and (len(last4) != 4 or not last4.isdigit()): + raise ValueError("last4 must contain exactly four digits") + account = AccountModel( + name=name.strip(), + currency=currency, + account_type=account_type, + institution=institution.strip() if institution else None, + last4=last4, + opening_balance_minor=opening_balance_minor, + opening_balance_as_of=opening_balance_as_of, + active=active, + ) + self.session.add(account) + self.session.flush() + return account + def get_or_create( self, name: str, currency: str = "GBP", account_type: str = "current" ) -> AccountModel: - account = self.session.scalar(select(AccountModel).where(AccountModel.name == name)) + account = self.get_by_name(name) if account is None: - account = AccountModel(name=name, currency=currency, account_type=account_type) - self.session.add(account) - self.session.flush() + account = self.create(name, currency, account_type) return account def get_by_name(self, name: str) -> AccountModel | None: - """Read-only lookup - never creates a row, so a preview never has the side effect - of persisting an account for a batch that might still be discarded.""" - return self.session.scalar(select(AccountModel).where(AccountModel.name == name)) + """Legacy label lookup; stable import binding uses ``get(account_id)``.""" + return self.session.scalar( + select(AccountModel).where(AccountModel.name == name).order_by(AccountModel.id) + ) + + def by_name(self, name: str) -> list[AccountModel]: + return list( + self.session.scalars( + select(AccountModel).where(AccountModel.name == name).order_by(AccountModel.id) + ) + ) def all(self) -> list[AccountModel]: return list(self.session.scalars(select(AccountModel).order_by(AccountModel.name))) diff --git a/src/pfa/domain/accounts.py b/src/pfa/domain/accounts.py index 16e22a8..5b0c6e4 100644 --- a/src/pfa/domain/accounts.py +++ b/src/pfa/domain/accounts.py @@ -10,4 +10,24 @@ class AccountType(StrEnum): LOAN = "loan" -NON_CASH_ACCOUNT_TYPES = {AccountType.INVESTMENT, AccountType.LOAN} +LIQUID_CASH_ACCOUNT_TYPES = frozenset({AccountType.CURRENT, AccountType.SAVINGS, AccountType.CASH}) + +_ACCOUNT_NATURE: dict[AccountType, str] = { + AccountType.CURRENT: "asset", + AccountType.SAVINGS: "asset", + AccountType.CASH: "asset", + AccountType.INVESTMENT: "asset", + AccountType.CREDIT_CARD: "liability", + AccountType.LOAN: "liability", +} + +# Kept for callers that used the old constant; liquid-cash membership is the safer API. +NON_CASH_ACCOUNT_TYPES = set(AccountType) - LIQUID_CASH_ACCOUNT_TYPES + + +def account_nature(account_type: AccountType | str) -> str: + return _ACCOUNT_NATURE[AccountType(account_type)] + + +def is_liquid_cash(account_type: AccountType | str) -> bool: + return AccountType(account_type) in LIQUID_CASH_ACCOUNT_TYPES diff --git a/src/pfa/domain/transactions.py b/src/pfa/domain/transactions.py index 56889ed..cea561f 100644 --- a/src/pfa/domain/transactions.py +++ b/src/pfa/domain/transactions.py @@ -42,4 +42,10 @@ class ClassificationSource(StrEnum): class TransferPurpose(StrEnum): SAVING = "saving" INVESTMENT = "investment" + CREDIT_CARD_PAYMENT = "credit_card_payment" OTHER = "other" + + +def signed_minor(amount_minor: int, flow_direction: str) -> int: + """Return PFA's canonical money-in/money-out polarity from legacy storage.""" + return amount_minor if flow_direction == "credit" else -amount_minor diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index c5f2e40..fd9ba63 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -19,12 +19,18 @@ from datetime import UTC, date, datetime, timedelta from pfa.config import Settings -from pfa.db.models import ImportBatchModel +from pfa.db.models import AccountModel, ImportBatchModel from pfa.db.unit_of_work import UnitOfWork +from pfa.domain.accounts import AccountType from pfa.domain.errors import BatchError, ImportRowError from pfa.ingestion.service import ImportService from .candidates import ( + ACCOUNT_CURRENCY_MISMATCH, + ACCOUNT_INACTIVE, + ACCOUNT_NOT_FOUND, + ACCOUNT_REQUIRED, + ACCOUNT_TYPE_MISMATCH, AMBIGUOUS_SIGN, BATCH_ALREADY_COMMITTED, BATCH_EXPIRED, @@ -34,6 +40,8 @@ ERROR, EXTRACTION_FAILED, EXTRACTION_TIMEOUT, + GENERIC_SIGN_CONFIRMATION_REQUIRED, + INVALID_ACCOUNT_DRAFT, NO_USABLE_ROWS, STATEMENT_YEAR_INFERRED, TOO_MANY_ROWS, @@ -49,7 +57,7 @@ is_year_bearing_date, parse_date, ) -from .dialects import Dialect, dialect_for_name +from .dialects import DIALECTS, Dialect, detect_adapter from .extractors.csv import CsvStatementExtractor from .extractors.ocr import OcrFallbackPdfExtractor from .extractors.pdf import clean_amount_text @@ -65,9 +73,48 @@ AMOUNT_SIGN_CONVENTIONS = ("as_written", "debit_positive") +@dataclass(slots=True) +class NewAccountDraft: + name: str + account_type: str = AccountType.CURRENT.value + currency: str = "GBP" + institution: str | None = None + last4: str | None = None + opening_balance_minor: int = 0 + opening_balance_as_of: date | None = None + + def as_dict(self) -> dict[str, object]: + return { + "name": self.name, + "account_type": self.account_type, + "currency": self.currency, + "institution": self.institution, + "last4": self.last4, + "opening_balance_minor": self.opening_balance_minor, + "opening_balance_as_of": self.opening_balance_as_of.isoformat() + if self.opening_balance_as_of + else None, + } + + @classmethod + def from_dict(cls, value: dict[str, object]) -> NewAccountDraft: + as_of = value.get("opening_balance_as_of") + return cls( + name=str(value.get("name", "")), + account_type=str(value.get("account_type", AccountType.CURRENT.value)), + currency=str(value.get("currency", "GBP")), + institution=str(value["institution"]) if value.get("institution") else None, + last4=str(value["last4"]) if value.get("last4") else None, + opening_balance_minor=int(value.get("opening_balance_minor", 0)), + opening_balance_as_of=date.fromisoformat(str(as_of)) if as_of else None, + ) + + @dataclass(slots=True) class BatchPatch: - account: str | None = None + account: str | None = None # deprecated label compatibility + destination_account_id: int | None = None + new_account: NewAccountDraft | None = None excluded_candidate_ids: list[str] | None = None amount_mode: str | None = None amount_sign: str | None = None @@ -107,14 +154,53 @@ def batch_committed_transaction_ids(batch: ImportBatchModel) -> list[int]: return list(json.loads(batch.committed_transaction_ids_json)) +def batch_semantic_totals(batch: ImportBatchModel) -> dict[str, int]: + """Calculate preview figures from candidate signs, never from model-generated text.""" + spending = refunds = transfers = repayments = money_in = 0 + for candidate in batch_candidates(batch): + signed = candidate.signed_amount_minor + if signed is None or not candidate.included: + continue + description = candidate.raw_description.upper() + kind = candidate.kind + if kind is None: + if ( + batch.adapter_id in {"amex_uk_csv", "amex_uk_pdf"} + and signed > 0 + and "PAYMENT RECEIVED" in description + ): + kind = "transfer" + else: + kind = "expense" if signed < 0 else "income" + if kind in {"expense", "fee"}: + spending += abs(signed) + elif kind == "refund": + refunds += abs(signed) + spending -= abs(signed) + elif kind == "transfer": + transfers += abs(signed) + if "CREDIT_CARD_PAYMENT" in (candidate.transfer_purpose or "").upper() or ( + "PAYMENT RECEIVED" in description and signed > 0 + ): + repayments += abs(signed) + elif kind == "income": + money_in += max(signed, 0) + return { + "money_in_minor": money_in, + "spending_minor": spending, + "refunds_minor": refunds, + "transfers_minor": transfers, + "repayments_minor": repayments, + } + + def _extractor_for( source: StatementSource, settings: Settings, - account_name: str | None = None, + dialect: Dialect, account_currency: str = "GBP", ) -> StatementExtractor: - """Picks the extractor from the extension the upload policy already validated.""" - dialect = dialect_for_name(account_name) + """Picks only the extraction engine; statement semantics come from content detection.""" if source.path.suffix.lower() == ".pdf": return OcrFallbackPdfExtractor( settings=settings, @@ -153,7 +239,9 @@ def _fail(batch: ImportBatchModel, uow: UnitOfWork, code: str, message: str) -> def _normalize_dates( - candidates: list[CandidateTransaction], dialect: Dialect + candidates: list[CandidateTransaction], + dialect: Dialect, + statement_year: int | None = None, ) -> CandidateIssue | None: """Resolves every year-less date (`Jul31`, `21 Jul`) against the year the rest of this statement's dates carry, then rewrites the candidate's date string to ISO so every later @@ -171,7 +259,9 @@ def _normalize_dates( years_seen.append(parse_date(text, dialect.date_order).year) except ImportRowError: continue - inferred_year = Counter(years_seen).most_common(1)[0][0] if years_seen else date.today().year + inferred_year = statement_year or ( + Counter(years_seen).most_common(1)[0][0] if years_seen else date.today().year + ) used_fallback = False for candidate in candidates: @@ -197,24 +287,170 @@ def _normalize_dates( ) +_BINDING_CODES = { + ACCOUNT_CURRENCY_MISMATCH, + ACCOUNT_INACTIVE, + ACCOUNT_NOT_FOUND, + ACCOUNT_REQUIRED, + ACCOUNT_TYPE_MISMATCH, + INVALID_ACCOUNT_DRAFT, + GENERIC_SIGN_CONFIRMATION_REQUIRED, +} + + +def _draft_from_batch(batch: ImportBatchModel) -> NewAccountDraft | None: + if not batch.new_account_json: + return None + try: + value = json.loads(batch.new_account_json) + return NewAccountDraft.from_dict(value) + except (TypeError, ValueError, json.JSONDecodeError): + return None + + +def _binding_issues( + batch: ImportBatchModel, uow: UnitOfWork, dialect: Dialect +) -> list[CandidateIssue]: + issues: list[CandidateIssue] = [] + account: AccountModel | None = None + draft = _draft_from_batch(batch) + if batch.destination_account_id is not None: + account = uow.accounts.get(batch.destination_account_id) + if account is None: + issues.append(CandidateIssue(ACCOUNT_NOT_FOUND, "select an existing account")) + elif not account.active: + issues.append(CandidateIssue(ACCOUNT_INACTIVE, "the selected account is inactive")) + elif draft is None and dialect.compatible_account_types: + issues.append( + CandidateIssue( + ACCOUNT_REQUIRED, + "select a compatible account or create one before committing this statement", + ) + ) + + if draft is not None: + try: + account_type = AccountType(draft.account_type) + except ValueError: + issues.append(CandidateIssue(INVALID_ACCOUNT_DRAFT, "choose a valid account type")) + else: + if not draft.name.strip() or draft.currency.upper() not in { + "GBP", + "INR", + "USD", + "EUR", + "JPY", + }: + issues.append( + CandidateIssue(INVALID_ACCOUNT_DRAFT, "account name and currency are invalid") + ) + if draft.last4 is not None and (len(draft.last4) != 4 or not draft.last4.isdigit()): + issues.append( + CandidateIssue( + INVALID_ACCOUNT_DRAFT, "last four must contain exactly four digits" + ) + ) + if ( + dialect.compatible_account_types + and account_type not in dialect.compatible_account_types + ): + expected = ", ".join( + sorted(item.value for item in dialect.compatible_account_types) + ) + issues.append( + CandidateIssue( + ACCOUNT_TYPE_MISMATCH, + f"this statement requires a {expected} account", + ) + ) + for existing in uow.accounts.by_name(draft.name.strip()): + if ( + existing.account_type != account_type.value + or existing.currency.upper() != draft.currency.upper() + ): + issues.append( + CandidateIssue( + ACCOUNT_CURRENCY_MISMATCH, + "an account with this name has a conflicting type or " + "currency; choose another name", + ) + ) + break + if account is not None: + if ( + dialect.compatible_account_types + and AccountType(account.account_type) not in dialect.compatible_account_types + ): + expected = ", ".join(sorted(item.value for item in dialect.compatible_account_types)) + issues.append( + CandidateIssue( + ACCOUNT_TYPE_MISMATCH, + f"this statement is for {expected}; selected account is {account.account_type}", + ) + ) + detected_currency = (batch.detected_currency or "GBP").upper() + if account.currency.upper() != detected_currency: + issues.append( + CandidateIssue( + ACCOUNT_CURRENCY_MISMATCH, + f"statement currency {detected_currency} does not match " + f"account currency {account.currency}", + ) + ) + if ( + batch.adapter_id == "generic" + and batch.amount_sign is None + and batch.destination_account is None + ): + # Preserve signed generic imports for the legacy API; unsigned rows still need a + # deliberate convention before they can be committed. + candidates = batch_candidates(batch) + if candidates and all(c.amount_minor is None or c.direction != "debit" for c in candidates): + issues.append( + CandidateIssue( + GENERIC_SIGN_CONFIRMATION_REQUIRED, + "confirm how positive statement amounts should be interpreted", + ) + ) + return issues + + +def _set_batch_issues( + batch: ImportBatchModel, uow: UnitOfWork, dialect: Dialect, base: list[CandidateIssue] +) -> None: + issues = [issue for issue in base if issue.code not in _BINDING_CODES] + issues.extend(_binding_issues(batch, uow, dialect)) + batch.issues_json = json.dumps([asdict(issue) for issue in issues]) + batch.status = ( + "blocked" if any(issue.severity == ERROR for issue in issues) else "preview_ready" + ) + + def create_batch( uow: UnitOfWork, source: StatementSource, settings: Settings, *, account: str | None = None, + destination_account_id: int | None = None, + new_account: NewAccountDraft | None = None, ) -> ImportBatchModel: now = _now() - account_currency = "GBP" - if account: - existing_acc = uow.accounts.get_by_name(account) - if existing_acc is not None: - account_currency = existing_acc.currency - - extractor = _extractor_for( - source, settings, account_name=account, account_currency=account_currency + selected = ( + uow.accounts.get(destination_account_id) if destination_account_id is not None else None + ) + if account and selected is None: + selected = uow.accounts.get_by_name(account) + destination_account_id = selected.id if selected is not None else None + account_currency = ( + selected.currency + if selected is not None + else (new_account.currency if new_account else "GBP") ) - dialect = dialect_for_name(account) + + detection = detect_adapter(source.path, source.media_type) + dialect = detection.dialect + extractor = _extractor_for(source, settings, dialect, account_currency=account_currency) batch = ImportBatchModel( id=uuid.uuid4().hex, @@ -224,7 +460,16 @@ def create_batch( sha256=source.sha256, extractor=extractor.name, status="extracting", - destination_account=account, + destination_account=selected.name + if selected is not None + else (new_account.name if new_account else account), + destination_account_id=destination_account_id, + new_account_json=json.dumps(new_account.as_dict()) if new_account else None, + adapter_id=dialect.adapter_id, + detection_confidence=detection.confidence, + detection_reason_codes_json=json.dumps(list(detection.reason_codes)), + detected_institution=detection.institution, + detected_account_hint=detection.account_hint, amount_sign=dialect.default_sign, issues_json="[]", counts_json=json.dumps(_counts([])), @@ -242,7 +487,14 @@ def create_batch( return _fail(batch, uow, EXTRACTION_FAILED, "could not process the uploaded file") candidates = extraction.candidates - if account: + if destination_account_id is not None and selected is not None: + for candidate in candidates: + candidate.account_hint = selected.name + candidate.account_id = selected.id + elif new_account is not None: + for candidate in candidates: + candidate.account_hint = new_account.name + elif account: for candidate in candidates: candidate.account_hint = account if len(candidates) > settings.max_candidate_rows: @@ -255,7 +507,7 @@ def create_batch( ) ) - year_issue = _normalize_dates(candidates, dialect) + year_issue = _normalize_dates(candidates, dialect, extraction.statement_year) if year_issue: extraction.issues.append(year_issue) @@ -283,11 +535,11 @@ def create_batch( batch.detected_account = extraction.detected_account batch.detected_currency = extraction.detected_currency or account_currency + batch.detected_institution = extraction.detected_institution or detection.institution + batch.detected_account_hint = extraction.detected_account_hint or detection.account_hint batch.page_count = extraction.page_count - blocked = any(issue.severity == ERROR for issue in extraction.issues) - batch.status = "blocked" if blocked else "preview_ready" batch.candidates_json = candidates_to_json(candidates) - batch.issues_json = json.dumps([asdict(issue) for issue in extraction.issues]) + _set_batch_issues(batch, uow, dialect, extraction.issues) batch.counts_json = json.dumps(_counts(candidates)) return uow.import_batches.add(batch) @@ -357,17 +609,54 @@ def _apply_amount_sign(candidate: CandidateTransaction, convention: str) -> None candidate.direction = "debit" if negative else "credit" +def _batch_dialect(batch: ImportBatchModel) -> Dialect: + return DIALECTS.get(batch.adapter_id or "generic", DIALECTS["generic"]) + + def apply_patch(uow: UnitOfWork, batch_id: str, patch: BatchPatch) -> ImportBatchModel: batch = load_batch(uow, batch_id) - if batch.status != "preview_ready": + if batch.status not in ("preview_ready", "blocked"): raise BatchError(BATCH_NOT_EDITABLE, f"batch is {batch.status}; nothing to modify", 409) + if patch.destination_account_id is not None and patch.new_account is not None: + raise BatchError(ACCOUNT_REQUIRED, "choose an existing account or create a new one", 422) + if patch.amount_sign is not None and batch.adapter_id not in (None, "generic"): + raise BatchError( + GENERIC_SIGN_CONFIRMATION_REQUIRED, + "recognized statement formats determine amount signs automatically", + 422, + ) candidates = batch_candidates(batch) if patch.account is not None: + selected = uow.accounts.get_by_name(patch.account) batch.destination_account = patch.account + batch.destination_account_id = selected.id if selected is not None else None + batch.new_account_json = ( + None + if selected is not None + else json.dumps(NewAccountDraft(name=patch.account).as_dict()) + ) for candidate in candidates: candidate.account_hint = patch.account + candidate.account_id = selected.id if selected is not None else None + + if patch.destination_account_id is not None: + selected = uow.accounts.get(patch.destination_account_id) + batch.destination_account_id = patch.destination_account_id + batch.destination_account = selected.name if selected is not None else None + batch.new_account_json = None + for candidate in candidates: + candidate.account_hint = selected.name if selected is not None else None + candidate.account_id = patch.destination_account_id + + if patch.new_account is not None: + batch.destination_account_id = None + batch.destination_account = patch.new_account.name + batch.new_account_json = json.dumps(patch.new_account.as_dict()) + for candidate in candidates: + candidate.account_hint = patch.new_account.name + candidate.account_id = None if patch.amount_mode in ("debit", "credit"): for candidate in candidates: @@ -397,17 +686,50 @@ def apply_patch(uow: UnitOfWork, batch_id: str, patch: BatchPatch) -> ImportBatc batch.candidates_json = candidates_to_json(candidates) batch.counts_json = json.dumps(_counts(candidates)) + base_issues = batch_issues(batch) + _set_batch_issues(batch, uow, _batch_dialect(batch), base_issues) batch.updated_at = _now() return uow.import_batches.add(batch) +def _account_for_commit(batch: ImportBatchModel, uow: UnitOfWork) -> AccountModel | None: + dialect = _batch_dialect(batch) + issues = _binding_issues(batch, uow, dialect) + if any(issue.severity == ERROR for issue in issues): + issue = next(issue for issue in issues if issue.severity == ERROR) + raise BatchError(issue.code, issue.message, 422) + if batch.destination_account_id is not None: + account = uow.accounts.get(batch.destination_account_id) + if account is None: # guarded above; keeps the type checker honest + raise BatchError(ACCOUNT_NOT_FOUND, "select an existing account", 422) + return account + draft = _draft_from_batch(batch) + if draft is not None: + return uow.accounts.create( + draft.name, + draft.currency, + draft.account_type, + institution=draft.institution, + last4=draft.last4, + opening_balance_minor=draft.opening_balance_minor, + opening_balance_as_of=draft.opening_balance_as_of, + ) + return None + + def commit_batch(uow: UnitOfWork, batch_id: str, settings: Settings) -> ImportBatchModel: batch = load_batch(uow, batch_id) if batch.status != "preview_ready": raise BatchError(BATCH_NOT_EDITABLE, f"batch is {batch.status}; nothing to commit", 409) candidates = batch_candidates(batch) + account = _account_for_commit(batch, uow) + if account is not None: + for candidate in candidates: + candidate.account_id = account.id + candidate.account_hint = account.name service = ImportService(uow) + service.validate(candidates) service.resolve_duplicates(candidates) # recheck against the ledger right before commit blocking = [c for c in candidates if c.included and c.state == ERROR] @@ -418,7 +740,11 @@ def commit_batch(uow: UnitOfWork, batch_id: str, settings: Settings) -> ImportBa 422, ) - committed = service.commit(candidates, source_label=f"upload:{batch.id}") + committed = service.commit( + candidates, + source_label=f"upload:{batch.id}", + destination_account_id=account.id if account is not None else None, + ) batch.status = "committed" batch.committed_at = _now() diff --git a/src/pfa/ingestion/candidates.py b/src/pfa/ingestion/candidates.py index ce384f8..9f5955c 100644 --- a/src/pfa/ingestion/candidates.py +++ b/src/pfa/ingestion/candidates.py @@ -60,6 +60,14 @@ BATCH_NOT_EDITABLE = "BATCH_NOT_EDITABLE" BATCH_HAS_BLOCKING_ERRORS = "BATCH_HAS_BLOCKING_ERRORS" BATCH_ALREADY_COMMITTED = "BATCH_ALREADY_COMMITTED" +ACCOUNT_NOT_FOUND = "ACCOUNT_NOT_FOUND" +ACCOUNT_INACTIVE = "ACCOUNT_INACTIVE" +ACCOUNT_TYPE_MISMATCH = "ACCOUNT_TYPE_MISMATCH" +ACCOUNT_CURRENCY_MISMATCH = "ACCOUNT_CURRENCY_MISMATCH" +ACCOUNT_REQUIRED = "ACCOUNT_REQUIRED" +INVALID_ACCOUNT_DRAFT = "INVALID_ACCOUNT_DRAFT" +GENERIC_SIGN_CONFIRMATION_REQUIRED = "GENERIC_SIGN_CONFIRMATION_REQUIRED" +UNDO_REQUIRES_CONFIRMATION = "UNDO_REQUIRES_CONFIRMATION" # One header vocabulary for every extractor. A bank's wording is added once, here, rather @@ -67,6 +75,7 @@ # lowercased and whitespace-collapsed - the spec forbids fuzzy guessing. HEADER_ALIASES: dict[str, tuple[str, ...]] = { "date": ("date", "transaction date", "transaction_date"), + "posted_date": ("posted date", "posted_date", "posting date", "posting_date"), "description": ("description", "details", "narrative", "merchant"), "debit": ("debit", "paid out", "paid_out", "withdrawn", "money out", "money_out"), "credit": ("credit", "paid in", "paid_in", "received", "money in", "money_in"), @@ -224,6 +233,7 @@ class CandidateTransaction: direction_explicit: bool = False currency: str = "GBP" account_hint: str | None = None + account_id: int | None = None external_id: str | None = None kind: str | None = None category: str | None = None @@ -251,6 +261,10 @@ def signed_amount_minor(self) -> int | None: return None return -self.amount_minor if self.direction == "debit" else self.amount_minor + @property + def signed_minor(self) -> int | None: + return self.signed_amount_minor + def add_issue(self, code: str, message: str, severity: str = ERROR) -> None: self.issues.append(CandidateIssue(code, message, severity)) @@ -265,6 +279,9 @@ class ExtractionResult: page_count: int | None = None detected_account: str | None = None detected_currency: str | None = None + detected_institution: str | None = None + detected_account_hint: str | None = None + statement_year: int | None = None issues: list[CandidateIssue] = field(default_factory=list) diff --git a/src/pfa/ingestion/categorizer.py b/src/pfa/ingestion/categorizer.py index ffcc4c9..9ab9a8b 100644 --- a/src/pfa/ingestion/categorizer.py +++ b/src/pfa/ingestion/categorizer.py @@ -1,6 +1,7 @@ import re from dataclasses import dataclass +from pfa.domain.accounts import AccountType from pfa.domain.transactions import SpendingCategory, TransactionKind, TransferPurpose @@ -93,8 +94,36 @@ class Classification: ) -def classify_known(description: str) -> Classification | None: +def classify_known( + description: str, + *, + account_type: AccountType | str | None = None, + canonical_sign: int | None = None, + owned_card: bool = False, +) -> Classification | None: upper = description.upper() + if account_type is not None and canonical_sign is not None: + account = AccountType(account_type) + if ( + account == AccountType.CREDIT_CARD + and canonical_sign > 0 + and re.search(r"PAYMENT RECEIVED(?:\s|[-])", upper) + ): + return Classification( + TransactionKind.TRANSFER, + transfer_purpose=TransferPurpose.CREDIT_CARD_PAYMENT, + reason="credit-card payment rule", + ) + if account in {AccountType.CURRENT, AccountType.SAVINGS} and re.search( + r"\bAMERICAN EXPRESS\s+DD\b", upper + ): + if owned_card: + return Classification( + TransactionKind.TRANSFER, + transfer_purpose=TransferPurpose.CREDIT_CARD_PAYMENT, + reason="owned card repayment rule", + ) + return Classification(TransactionKind.UNKNOWN, reason="possible card repayment") for pattern, classification in _RULES: if re.search(rf"(? Dialect: + """Legacy compatibility only. Import batches use :func:`detect_adapter`.""" if not name: return GENERIC clean = name.strip().lower() @@ -65,3 +108,71 @@ def dialect_for_name(name: str | None) -> Dialect: if key in clean: return dialect return GENERIC + + +def _csv_text(path: Path) -> str: + try: + return path.read_text(encoding="utf-8-sig")[:100_000] + except (OSError, UnicodeDecodeError): + return "" + + +def _csv_detection(path: Path) -> AdapterDetection: + text = _csv_text(path) + lower = text.lower() + try: + header = next(csv.reader(text.splitlines()), []) + except csv.Error: + header = [] + headers = {" ".join(cell.strip().lower().split()) for cell in header} + if ( + "american express" in lower + or "card member" in lower + or "membership number" in lower + or "payment received - thank you" in lower + ): + return AdapterDetection( + AMEX_UK_CSV, + 0.98, + ("amex_marker", "csv_headers"), + institution="American Express", + ) + if "hsbc" in lower and ( + {"paid out", "paid in"}.issubset(headers) or {"money out", "money in"}.issubset(headers) + ): + return AdapterDetection(HSBC_UK_CURRENT, 0.95, ("hsbc_marker", "two_column_cash_headers")) + if "credit card" in lower and (" cr" in lower or "credit" in lower): + return AdapterDetection(HSBC_UK_CARD, 0.9, ("card_marker",)) + return AdapterDetection(GENERIC, 0.0, ("generic_format",)) + + +def _pdf_detection(path: Path) -> AdapterDetection: + try: + import pdfplumber + + with pdfplumber.open(path) as pdf: + text = "\n".join((page.extract_text() or "") for page in pdf.pages[:3]) + except Exception: + return AdapterDetection(GENERIC, 0.0, ("unreadable_content",)) + lower = text.lower() + if "american express" in lower or "payment received - thank you" in lower: + return AdapterDetection( + AMEX_UK_PDF, + 0.98, + ("amex_marker", "pdf_text"), + institution="American Express", + ) + if "hsbc" in lower and any( + marker in lower for marker in ("credit card", "visa", "available credit") + ): + return AdapterDetection(HSBC_UK_CARD, 0.95, ("hsbc_marker", "card_marker")) + if {"paid out", "paid in"}.issubset(set(lower.split())) or ( + "paid out" in lower and "paid in" in lower and "balance" in lower + ): + return AdapterDetection(HSBC_UK_CURRENT, 0.95, ("cash_headers", "balance_column")) + return AdapterDetection(GENERIC, 0.0, ("generic_format",)) + + +def detect_adapter(path: Path, media_type: str | None = None) -> AdapterDetection: + """Detect a statement adapter from bytes/content, never its filename or account label.""" + return _pdf_detection(path) if path.suffix.lower() == ".pdf" else _csv_detection(path) diff --git a/src/pfa/ingestion/extractors/csv.py b/src/pfa/ingestion/extractors/csv.py index ec6e74b..5b90559 100644 --- a/src/pfa/ingestion/extractors/csv.py +++ b/src/pfa/ingestion/extractors/csv.py @@ -100,9 +100,12 @@ def read_csv_rows( row = {str(key).strip().lower(): (value or "") for key, value in raw.items()} yield { "date": _value(row, *DATE_ALIASES), - "posted_date": _value(row, "posted_date", "posted date"), + "posted_date": _value( + row, "posted_date", "posted date", "posting date", "posting_date" + ), "description": _value(row, *DESCRIPTION_ALIASES), "amount": _value(row, *AMOUNT_ALIASES), + "balance": _value(row, "balance"), "debit": _value(row, *DEBIT_ALIASES), "credit": _value(row, *CREDIT_ALIASES), "currency": _value(row, "currency") or default_currency or "GBP", diff --git a/src/pfa/ingestion/extractors/pdf.py b/src/pfa/ingestion/extractors/pdf.py index 4e5d6d6..f2efc02 100644 --- a/src/pfa/ingestion/extractors/pdf.py +++ b/src/pfa/ingestion/extractors/pdf.py @@ -8,6 +8,7 @@ from __future__ import annotations +import re from collections.abc import Callable from dataclasses import dataclass, field from decimal import Decimal, InvalidOperation @@ -85,10 +86,14 @@ class _RawRow: def _table_header(table: list[list[str | None]]) -> dict[int, str] | None: mapping: dict[int, str] = {} + seen: set[str] = set() for index, cell in enumerate(table[0]): matched = match_header_alias(cell or "") + if matched == "date" and matched in seen: + matched = "posted_date" if matched: mapping[index] = matched + seen.add(matched) return mapping if _has_transaction_header_fields(set(mapping.values())) else None @@ -148,7 +153,15 @@ def _split_cells(line_words: list[Word]) -> list[tuple[float, str, float | None] def _header_columns( cells: list[tuple[float, str, float | None]], ) -> list[tuple[float, str]] | None: - columns = [(x0, matched) for x0, text, _ in cells if (matched := match_header_alias(text))] + seen: set[str] = set() + columns: list[tuple[float, str]] = [] + for x0, text, _ in cells: + matched = match_header_alias(text) + if matched == "date" and matched in seen: + matched = "posted_date" + if matched: + columns.append((x0, matched)) + seen.add(matched) names = {name for _, name in columns} if not _has_transaction_header_fields(names): return None @@ -465,6 +478,38 @@ def _resolve_amount( return _AmountResult() +_BALANCE_MARKERS = ( + "BALANCEBROUGHTFORWARD", + "BALANCE BROUGHT FORWARD", + "BALANCECARRIEDFORWARD", + "BALANCE CARRIED FORWARD", + "OPENING BALANCE", + "CLOSING BALANCE", +) +_DATE_WITH_YEAR = re.compile( + r"(?:\b\d{1,2}[/-]\d{1,2}[/-]\d{2,4}\b|" + r"\b\d{1,2}\s+[A-Za-z]{3,9}\s+\d{2,4}\b|" + r"\b[A-Za-z]{3,9}\s+\d{1,2},?\s+\d{2,4}\b)" +) + + +def _statement_year(pdf: PDF) -> int | None: + for page in pdf.pages[:3]: + match = _DATE_WITH_YEAR.search(page.extract_text() or "") + if match: + year_match = re.search(r"\d{2,4}$", match.group()) + if year_match: + year = int(year_match.group()) + return year + 2000 if year < 100 else year + return None + + +def _is_balance_marker(row: _RawRow) -> bool: + description = " ".join(row.fields.get("description", "").upper().split()) + compact = description.replace(" ", "") + return any(marker in description or marker in compact for marker in _BALANCE_MARKERS) + + def _build_candidate( index: int, row: _RawRow, @@ -478,6 +523,7 @@ def _build_candidate( candidate = CandidateTransaction( candidate_id=f"p{index}", transaction_date=fields.get("date", "").strip() or None, + posted_date=fields.get("posted_date", "").strip() or None, raw_description=fields.get("description", "").strip(), currency=currency.upper(), external_id=fields.get("reference", "").strip() or None, @@ -569,6 +615,7 @@ def extract(self, source: StatementSource) -> ExtractionResult: def _extract(self, pdf: PDF, result: ExtractionResult) -> ExtractionResult: result.page_count = len(pdf.pages) + result.statement_year = _statement_year(pdf) if result.page_count > self._max_pages: result.issues.append( CandidateIssue( @@ -583,9 +630,10 @@ def _extract(self, pdf: PDF, result: ExtractionResult) -> ExtractionResult: page_rows, header_top = self._page_rows(page) kept.extend(_merge_continuations(page_rows, header_top, self.dialect)) + transaction_rows = [row for row in kept if not _is_balance_marker(row)] candidates = [ _build_candidate(index, row, self._ocr_min_confidence, self.currency, self.dialect) - for index, row in enumerate(kept, start=1) + for index, row in enumerate(transaction_rows, start=1) ] if len(candidates) > self._max_rows: candidates = candidates[: self._max_rows] diff --git a/src/pfa/ingestion/service.py b/src/pfa/ingestion/service.py index 9fd41dc..e4bdf53 100644 --- a/src/pfa/ingestion/service.py +++ b/src/pfa/ingestion/service.py @@ -8,6 +8,7 @@ from pfa.db.models import MerchantRuleModel, TransactionModel from pfa.db.unit_of_work import UnitOfWork +from pfa.domain.accounts import AccountType from pfa.domain.errors import ImportRowError from pfa.domain.money import SUPPORTED_CURRENCIES from pfa.domain.transactions import ( @@ -64,7 +65,11 @@ def _non_member(value: str, enum: type[StrEnum]) -> str | None: def _classification( - candidate: CandidateTransaction, sign: int, classifier: Classifier | None + candidate: CandidateTransaction, + sign: int, + classifier: Classifier | None, + account_type: AccountType | str | None = None, + owned_card: bool = False, ) -> Classification: if candidate.kind: return Classification( @@ -75,7 +80,12 @@ def _classification( None, "source-provided classification", ) - known = classify_known(candidate.raw_description) + known = classify_known( + candidate.raw_description, + account_type=account_type, + canonical_sign=sign * (candidate.amount_minor or 0), + owned_card=owned_card, + ) if known: return known if classifier: @@ -167,7 +177,11 @@ def validate(self, candidates: Sequence[CandidateTransaction]) -> None: # Only an *existing* account can disagree with the row - a brand-new # account takes its currency from the first candidate that names it, at # commit time, so there is nothing to compare against yet. - account = self.uow.accounts.get_by_name(candidate.account_hint) + account = ( + self.uow.accounts.get(candidate.account_id) + if candidate.account_id is not None + else self.uow.accounts.get_by_name(candidate.account_hint) + ) if account is not None and account.currency.upper() != candidate.currency.upper(): candidate.add_issue( CURRENCY_ACCOUNT_MISMATCH, @@ -184,7 +198,7 @@ def resolve_duplicates(self, candidates: Sequence[CandidateTransaction]) -> None if candidate.state == ERROR or signed is None: continue key = ( - candidate.account_hint or "Main account", + str(candidate.account_id or candidate.account_hint or "Main account"), candidate.transaction_date or "", signed, candidate.currency, @@ -197,6 +211,16 @@ def resolve_duplicates(self, candidates: Sequence[CandidateTransaction]) -> None 1 if candidate.external_id else occurrences[key], ) existing = self.uow.transactions.find_fingerprint(candidate.fingerprint) + if existing is None and candidate.account_id is not None and candidate.account_hint: + # Legacy imports fingerprinted the display label before stable account IDs + # existed; accept that one-way compatibility match during migration. + legacy_fingerprint = transaction_fingerprint( + candidate.account_hint, + *key[1:], + candidate.external_id, + 1 if candidate.external_id else occurrences[key], + ) + existing = self.uow.transactions.find_fingerprint(legacy_fingerprint) candidate.duplicate_of = existing.id if existing else None if existing: candidate.add_issue( @@ -208,24 +232,46 @@ def commit( candidates: Sequence[CandidateTransaction], *, source_label: str, + destination_account_id: int | None = None, dry_run: bool = False, ) -> list[TransactionModel]: """Persists included, non-duplicate, non-error rows.""" committed: list[TransactionModel] = [] + destination = ( + self.uow.accounts.get(destination_account_id) + if destination_account_id is not None + else None + ) + owned_card = any( + AccountType(account.account_type) == AccountType.CREDIT_CARD + for account in self.uow.accounts.all() + ) for candidate in candidates: if not candidate.included or candidate.state == ERROR: continue if candidate.duplicate_of is not None or candidate.amount_minor is None: continue sign = -1 if candidate.direction == "debit" else 1 + account = destination or ( + self.uow.accounts.get(candidate.account_id) + if candidate.account_id is not None + else self.uow.accounts.get_or_create( + candidate.account_hint or "Main account", candidate.currency + ) + ) + if account is None: + continue rule = self.uow.rules.match(candidate.normalized_description) classification = ( _classification_from_rule(rule) if rule - else _classification(candidate, sign, self.classifier) - ) - account = self.uow.accounts.get_or_create( - candidate.account_hint or "Main account", candidate.currency + else _classification( + candidate, + sign, + self.classifier, + account_type=account.account_type, + owned_card=owned_card, + ) ) if account.currency.upper() != candidate.currency.upper(): # validate() already blocks this for an existing account at preview time; From 12db20f8ea6c55cc2e8bc0312613dda2108acb71 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:23:20 +0100 Subject: [PATCH 04/24] fix(analytics): calculate cash from liquid account signs --- src/pfa/analytics/results.py | 3 + src/pfa/analytics/service.py | 112 +++++++++++++++++++++++++++++------ src/pfa/services/runtime.py | 2 +- 3 files changed, 99 insertions(+), 18 deletions(-) diff --git a/src/pfa/analytics/results.py b/src/pfa/analytics/results.py index 22d6696..8c05f2c 100644 --- a/src/pfa/analytics/results.py +++ b/src/pfa/analytics/results.py @@ -25,7 +25,10 @@ class MonthlySummary(BaseModel): discretionary_spending_minor: int = 0 savings_minor: int = 0 investments_minor: int = 0 + # Deprecated compatibility field; it retains the historical debt-cost meaning. debt_payments_minor: int = 0 + debt_repayments_minor: int = 0 + debt_costs_minor: int = 0 net_cashflow_minor: int = 0 savings_rate_percent: float = 0.0 transaction_count: int = 0 diff --git a/src/pfa/analytics/service.py b/src/pfa/analytics/service.py index e734ff7..7341bd6 100644 --- a/src/pfa/analytics/service.py +++ b/src/pfa/analytics/service.py @@ -2,13 +2,19 @@ import calendar from collections import defaultdict +from dataclasses import dataclass from datetime import date, timedelta from decimal import ROUND_HALF_UP, Decimal from pfa.db.models import AccountModel, TransactionModel -from pfa.db.repositories import BudgetRepository, GoalRepository, TransactionRepository -from pfa.domain.accounts import NON_CASH_ACCOUNT_TYPES -from pfa.domain.transactions import SpendingCategory, TransactionKind, TransferPurpose +from pfa.db.repositories import ( + AccountRepository, + BudgetRepository, + GoalRepository, + TransactionRepository, +) +from pfa.domain.accounts import LIQUID_CASH_ACCOUNT_TYPES, AccountType +from pfa.domain.transactions import SpendingCategory, TransactionKind, TransferPurpose, signed_minor from .anomalies import category_spikes, unusual_transactions from .recurring import detect_recurring @@ -39,6 +45,15 @@ } +@dataclass(frozen=True, slots=True) +class CashPosition: + total_minor: int | None + known_subtotal_minor: int + coverage_status: str + missing_account_ids: tuple[int, ...] + currency: str + + def month_bounds(period: date) -> tuple[date, date]: start = period.replace(day=1) return start, period.replace(day=calendar.monthrange(period.year, period.month)[1]) @@ -64,11 +79,27 @@ def _cash_delta(transaction: TransactionModel) -> int: class AnalyticsService: def __init__( - self, transactions: TransactionRepository, budgets: BudgetRepository, goals: GoalRepository + self, + transactions: TransactionRepository, + budgets: BudgetRepository, + goals: GoalRepository, + accounts: AccountRepository | None = None, ): self.transactions = transactions self.budgets = budgets self.goals = goals + self.accounts = accounts + + def _account_type(self, transaction: TransactionModel) -> AccountType | None: + account = getattr(transaction, "account", None) + if account is None and self.accounts is not None: + account = self.accounts.get(transaction.account_id) + if account is None: + return None + try: + return AccountType(account.account_type) + except ValueError: + return None def _filter_currency( self, transactions: list[TransactionModel], currency: str @@ -96,9 +127,17 @@ def monthly_summary(self, period: date, currency: str = "GBP") -> MonthlySummary if row.transfer_purpose == TransferPurpose.INVESTMENT.value and row.flow_direction == "debit" ) - debt = sum( + debt_costs = sum( _spending(row) for row in rows if row.category == SpendingCategory.DEBT_PAYMENT.value ) + debt_repayments = sum( + row.amount_minor + for row in rows + if row.kind == TransactionKind.TRANSFER.value + and row.transfer_purpose == TransferPurpose.CREDIT_CARD_PAYMENT.value + and signed_minor(row.amount_minor, row.flow_direction) > 0 + and self._account_type(row) == AccountType.CREDIT_CARD + ) rate = ( float( (Decimal(savings + investments) / Decimal(income) * 100).quantize( @@ -117,7 +156,9 @@ def monthly_summary(self, period: date, currency: str = "GBP") -> MonthlySummary discretionary_spending_minor=spending - essential, savings_minor=savings, investments_minor=investments, - debt_payments_minor=debt, + debt_payments_minor=debt_costs, + debt_repayments_minor=debt_repayments, + debt_costs_minor=debt_costs, net_cashflow_minor=income - spending, savings_rate_percent=rate, transaction_count=len(rows), @@ -263,22 +304,59 @@ def category_trend( return category_trend(rows, category, as_of, months) -def current_cash( +def cash_position( accounts: list[AccountModel], transactions: list[TransactionModel], currency: str = "GBP", as_of: date | None = None, -) -> int: +) -> CashPosition: curr = currency.upper() - opening = sum( - account.opening_balance_minor + liquid = [ + account for account in accounts - if account.account_type not in {item.value for item in NON_CASH_ACCOUNT_TYPES} - and (getattr(account, "currency", None) or "GBP").upper() == curr + if (getattr(account, "currency", None) or "GBP").upper() == curr + and AccountType(account.account_type) in LIQUID_CASH_ACCOUNT_TYPES + ] + missing: list[int] = [] + subtotal = 0 + cutoff = as_of or date.max + for account in liquid: + baseline = account.opening_balance_as_of + subtotal += account.opening_balance_minor + if baseline is None or cutoff < baseline: + missing.append(account.id) + # Legacy accounts had no baseline contract. Keep their numeric behavior for + # callers such as planning while exposing incomplete coverage to new callers. + subtotal += sum( + signed_minor(row.amount_minor, row.flow_direction) + for row in transactions + if row.account_id == account.id and row.transaction_date <= cutoff + ) + continue + subtotal += sum( + signed_minor(row.amount_minor, row.flow_direction) + for row in transactions + if row.account_id == account.id + and baseline < row.transaction_date <= cutoff + and (getattr(row, "currency", None) or "GBP").upper() == curr + ) + return CashPosition( + total_minor=None if missing else subtotal, + known_subtotal_minor=subtotal, + coverage_status="incomplete" if missing else "complete", + missing_account_ids=tuple(missing), + currency=curr, ) - return opening + sum( - _cash_delta(row) - for row in transactions - if (getattr(row, "currency", None) or "GBP").upper() == curr - and (as_of is None or row.transaction_date <= as_of) + + +def current_cash( + accounts: list[AccountModel], + transactions: list[TransactionModel], + currency: str = "GBP", + as_of: date | None = None, +) -> int: + """Compatibility scalar; use ``cash_position`` when coverage matters.""" + position = cash_position(accounts, transactions, currency=currency, as_of=as_of) + return ( + position.total_minor if position.total_minor is not None else position.known_subtotal_minor ) diff --git a/src/pfa/services/runtime.py b/src/pfa/services/runtime.py index 270deb0..21f9e07 100644 --- a/src/pfa/services/runtime.py +++ b/src/pfa/services/runtime.py @@ -23,7 +23,7 @@ def open_services(settings: Settings) -> tuple[Engine, FinanceServices]: session = make_session_factory(engine)() try: uow = UnitOfWork(session) - analytics = AnalyticsService(uow.transactions, uow.budgets, uow.goals) + analytics = AnalyticsService(uow.transactions, uow.budgets, uow.goals, uow.accounts) planning = PlanningService(analytics, uow.accounts.all(), uow.transactions.all()) return engine, FinanceServices(uow, analytics, planning) except Exception: From 2a49b5f8192149fff79d28438a9d1dc7f7bda82a Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:27:16 +0100 Subject: [PATCH 05/24] feat(transfers): persist auditable transfer matches --- alembic/versions/0006_transfer_events.py | 77 ++++++++++++ src/pfa/db/models.py | 36 ++++++ src/pfa/db/repositories.py | 68 +++++++++++ src/pfa/db/unit_of_work.py | 2 + src/pfa/domain/transactions.py | 12 ++ src/pfa/ingestion/batches.py | 47 ++++++- src/pfa/ingestion/reconciliation.py | 88 ++++++++++++++ src/pfa/ingestion/transfers.py | 148 +++++++++++++++++++++++ 8 files changed, 477 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/0006_transfer_events.py create mode 100644 src/pfa/ingestion/reconciliation.py create mode 100644 src/pfa/ingestion/transfers.py diff --git a/alembic/versions/0006_transfer_events.py b/alembic/versions/0006_transfer_events.py new file mode 100644 index 0000000..82f8bfb --- /dev/null +++ b/alembic/versions/0006_transfer_events.py @@ -0,0 +1,77 @@ +"""auditable transfer events and persisted match decisions""" + +import sqlalchemy as sa +from alembic import op + +revision = "0006_transfer_events" +down_revision = "0005_account_import_binding" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "transfer_events", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("purpose", sa.String(length=30), nullable=False), + sa.Column("match_method", sa.String(length=30), nullable=False), + sa.Column( + "created_at", sa.DateTime(), server_default=sa.text("CURRENT_TIMESTAMP"), nullable=False + ), + sa.PrimaryKeyConstraint("id"), + ) + op.create_table( + "transfer_legs", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("event_id", sa.Integer(), nullable=False), + sa.Column("transaction_id", sa.Integer(), nullable=False), + sa.Column("role", sa.String(length=20), nullable=False), + sa.ForeignKeyConstraint(["event_id"], ["transfer_events.id"]), + sa.ForeignKeyConstraint(["transaction_id"], ["transactions.id"]), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("transaction_id", name="uq_transfer_legs_transaction"), + ) + op.create_table( + "transfer_match_decisions", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("stable_match_key", sa.String(length=64), nullable=False), + sa.Column("left_transaction_id", sa.Integer(), nullable=False), + sa.Column("right_transaction_id", sa.Integer(), nullable=False), + sa.Column("state", sa.String(length=20), nullable=False), + sa.Column("confidence", sa.Float(), nullable=False), + sa.Column("reason_codes_json", sa.Text(), nullable=False), + sa.Column("event_id", sa.Integer(), nullable=True), + sa.Column( + "created_at", sa.DateTime(), server_default=sa.text("CURRENT_TIMESTAMP"), nullable=False + ), + sa.Column("reviewed_at", sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(["event_id"], ["transfer_events.id"]), + sa.ForeignKeyConstraint(["left_transaction_id"], ["transactions.id"]), + sa.ForeignKeyConstraint(["right_transaction_id"], ["transactions.id"]), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("stable_match_key", name="uq_transfer_match_key"), + ) + op.create_index( + "ix_transfer_match_decisions_left_transaction_id", + "transfer_match_decisions", + ["left_transaction_id"], + ) + op.create_index( + "ix_transfer_match_decisions_right_transaction_id", + "transfer_match_decisions", + ["right_transaction_id"], + ) + + +def downgrade() -> None: + op.drop_index( + "ix_transfer_match_decisions_right_transaction_id", + table_name="transfer_match_decisions", + ) + op.drop_index( + "ix_transfer_match_decisions_left_transaction_id", + table_name="transfer_match_decisions", + ) + op.drop_table("transfer_match_decisions") + op.drop_table("transfer_legs") + op.drop_table("transfer_events") diff --git a/src/pfa/db/models.py b/src/pfa/db/models.py index 7ffd1f1..5b97f5f 100644 --- a/src/pfa/db/models.py +++ b/src/pfa/db/models.py @@ -63,6 +63,42 @@ class TransactionModel(Base): account: Mapped[AccountModel] = relationship(back_populates="transactions") +class TransferEventModel(Base): + __tablename__ = "transfer_events" + id: Mapped[int] = mapped_column(primary_key=True) + purpose: Mapped[str] = mapped_column(String(30), default="other") + match_method: Mapped[str] = mapped_column(String(30)) + created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now()) + legs: Mapped[list[TransferLegModel]] = relationship( + back_populates="event", cascade="all, delete-orphan" + ) + + +class TransferLegModel(Base): + __tablename__ = "transfer_legs" + __table_args__ = (UniqueConstraint("transaction_id", name="uq_transfer_legs_transaction"),) + id: Mapped[int] = mapped_column(primary_key=True) + event_id: Mapped[int] = mapped_column(ForeignKey("transfer_events.id")) + transaction_id: Mapped[int] = mapped_column(ForeignKey("transactions.id")) + role: Mapped[str] = mapped_column(String(20)) + event: Mapped[TransferEventModel] = relationship(back_populates="legs") + + +class TransferMatchDecisionModel(Base): + __tablename__ = "transfer_match_decisions" + __table_args__ = (UniqueConstraint("stable_match_key", name="uq_transfer_match_key"),) + id: Mapped[int] = mapped_column(primary_key=True) + stable_match_key: Mapped[str] = mapped_column(String(64)) + left_transaction_id: Mapped[int] = mapped_column(ForeignKey("transactions.id")) + right_transaction_id: Mapped[int] = mapped_column(ForeignKey("transactions.id")) + state: Mapped[str] = mapped_column(String(20)) + confidence: Mapped[float] = mapped_column() + reason_codes_json: Mapped[str] = mapped_column(Text, default="[]") + event_id: Mapped[int | None] = mapped_column(ForeignKey("transfer_events.id"), nullable=True) + created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now()) + reviewed_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True) + + class BudgetModel(Base): __tablename__ = "budgets" id: Mapped[int] = mapped_column(primary_key=True) diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index a8aac5a..29c84e6 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -18,6 +18,9 @@ ImportBatchModel, MerchantRuleModel, TransactionModel, + TransferEventModel, + TransferLegModel, + TransferMatchDecisionModel, ) @@ -197,6 +200,71 @@ def list_expired(self, at: datetime) -> list[ImportBatchModel]: return list(self.session.scalars(statement)) +class TransferRepository: + def __init__(self, session: Session): + self.session = session + + def linked_transaction_ids(self) -> set[int]: + return set(self.session.scalars(select(TransferLegModel.transaction_id))) + + def decision(self, stable_match_key: str) -> TransferMatchDecisionModel | None: + return self.session.scalar( + select(TransferMatchDecisionModel).where( + TransferMatchDecisionModel.stable_match_key == stable_match_key + ) + ) + + def add_event( + self, event: TransferEventModel, legs: list[TransferLegModel] + ) -> TransferEventModel: + event.legs = legs + self.session.add(event) + self.session.flush() + return event + + def add_decision(self, decision: TransferMatchDecisionModel) -> TransferMatchDecisionModel: + self.session.add(decision) + self.session.flush() + return decision + + def suggestions(self) -> list[TransferMatchDecisionModel]: + return list( + self.session.scalars( + select(TransferMatchDecisionModel).where( + TransferMatchDecisionModel.state == "suggested" + ) + ) + ) + + def delete_for_transactions(self, transaction_ids: set[int]) -> None: + if not transaction_ids: + return + legs = list( + self.session.scalars( + select(TransferLegModel).where(TransferLegModel.transaction_id.in_(transaction_ids)) + ) + ) + event_ids = {leg.event_id for leg in legs} + for leg in legs: + self.session.delete(leg) + for event_id in event_ids: + remaining = self.session.scalar( + select(TransferLegModel.id).where(TransferLegModel.event_id == event_id).limit(1) + ) + if remaining is None: + event = self.session.get(TransferEventModel, event_id) + if event is not None: + self.session.delete(event) + decisions = self.session.scalars( + select(TransferMatchDecisionModel).where( + (TransferMatchDecisionModel.left_transaction_id.in_(transaction_ids)) + | (TransferMatchDecisionModel.right_transaction_id.in_(transaction_ids)) + ) + ) + for decision in decisions: + self.session.delete(decision) + + class GoalRepository: def __init__(self, session: Session): self.session = session diff --git a/src/pfa/db/unit_of_work.py b/src/pfa/db/unit_of_work.py index bb1dcb0..2c21db0 100644 --- a/src/pfa/db/unit_of_work.py +++ b/src/pfa/db/unit_of_work.py @@ -8,6 +8,7 @@ ImportBatchRepository, RuleRepository, TransactionRepository, + TransferRepository, ) @@ -22,4 +23,5 @@ def __init__(self, session: Session): self.budgets = BudgetRepository(session) self.goals = GoalRepository(session) self.import_batches = ImportBatchRepository(session) + self.transfers = TransferRepository(session) self.fx_rates = FxRateRepository(session) diff --git a/src/pfa/domain/transactions.py b/src/pfa/domain/transactions.py index cea561f..f58ea9e 100644 --- a/src/pfa/domain/transactions.py +++ b/src/pfa/domain/transactions.py @@ -39,6 +39,18 @@ class ClassificationSource(StrEnum): UNKNOWN = "unknown" +class TransferLegRole(StrEnum): + SOURCE = "source" + DESTINATION = "destination" + FEE = "fee" + + +class TransferMatchState(StrEnum): + SUGGESTED = "suggested" + ACCEPTED = "accepted" + DISMISSED = "dismissed" + + class TransferPurpose(StrEnum): SAVING = "saving" INVESTMENT = "investment" diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index fd9ba63..9bb4c7e 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -43,6 +43,8 @@ GENERIC_SIGN_CONFIRMATION_REQUIRED, INVALID_ACCOUNT_DRAFT, NO_USABLE_ROWS, + RECONCILIATION_INCOMPLETE, + RECONCILIATION_MISMATCH, STATEMENT_YEAR_INFERRED, TOO_MANY_ROWS, VALID, @@ -99,13 +101,14 @@ def as_dict(self) -> dict[str, object]: @classmethod def from_dict(cls, value: dict[str, object]) -> NewAccountDraft: as_of = value.get("opening_balance_as_of") + opening = value.get("opening_balance_minor", 0) return cls( name=str(value.get("name", "")), account_type=str(value.get("account_type", AccountType.CURRENT.value)), currency=str(value.get("currency", "GBP")), institution=str(value["institution"]) if value.get("institution") else None, last4=str(value["last4"]) if value.get("last4") else None, - opening_balance_minor=int(value.get("opening_balance_minor", 0)), + opening_balance_minor=int(str(opening)), opening_balance_as_of=date.fromisoformat(str(as_of)) if as_of else None, ) @@ -295,6 +298,8 @@ def _normalize_dates( ACCOUNT_TYPE_MISMATCH, INVALID_ACCOUNT_DRAFT, GENERIC_SIGN_CONFIRMATION_REQUIRED, + RECONCILIATION_INCOMPLETE, + RECONCILIATION_MISMATCH, } @@ -415,11 +420,48 @@ def _binding_issues( return issues +def _reconciliation_account_type(batch: ImportBatchModel, uow: UnitOfWork) -> AccountType: + if batch.destination_account_id is not None: + account = uow.accounts.get(batch.destination_account_id) + if account is not None: + try: + return AccountType(account.account_type) + except ValueError: + return AccountType.CURRENT + draft = _draft_from_batch(batch) + if draft is not None: + try: + return AccountType(draft.account_type) + except ValueError: + return AccountType.CURRENT + return AccountType.CURRENT + + +def _set_reconciliation( + batch: ImportBatchModel, + candidates: list[CandidateTransaction], + uow: UnitOfWork, +) -> list[CandidateIssue]: + from .reconciliation import reconcile_candidates + + result = reconcile_candidates(candidates, _reconciliation_account_type(batch, uow)) + batch.reconciliation_json = json.dumps(result) + if batch.adapter_id not in (None, "generic"): + if result["status"] == "mismatch": + return [CandidateIssue(RECONCILIATION_MISMATCH, "statement balances do not reconcile")] + if result["status"] == "incomplete": + return [ + CandidateIssue(RECONCILIATION_INCOMPLETE, "not every statement row is included") + ] + return [] + + def _set_batch_issues( batch: ImportBatchModel, uow: UnitOfWork, dialect: Dialect, base: list[CandidateIssue] ) -> None: issues = [issue for issue in base if issue.code not in _BINDING_CODES] issues.extend(_binding_issues(batch, uow, dialect)) + issues.extend(_set_reconciliation(batch, batch_candidates(batch), uow)) batch.issues_json = json.dumps([asdict(issue) for issue in issues]) batch.status = ( "blocked" if any(issue.severity == ERROR for issue in issues) else "preview_ready" @@ -745,6 +787,9 @@ def commit_batch(uow: UnitOfWork, batch_id: str, settings: Settings) -> ImportBa source_label=f"upload:{batch.id}", destination_account_id=account.id if account is not None else None, ) + from .transfers import match_transfers + + match_transfers(uow) batch.status = "committed" batch.committed_at = _now() diff --git a/src/pfa/ingestion/reconciliation.py b/src/pfa/ingestion/reconciliation.py new file mode 100644 index 0000000..83fa3f0 --- /dev/null +++ b/src/pfa/ingestion/reconciliation.py @@ -0,0 +1,88 @@ +from __future__ import annotations + +from decimal import Decimal, InvalidOperation +from typing import Any + +from pfa.domain.accounts import AccountType, account_nature +from pfa.domain.money import minor_units + +from .candidates import CandidateTransaction +from .extractors.pdf import clean_amount_text + + +def _balance_minor(value: str, currency: str) -> int | None: + cleaned, negative = clean_amount_text(value) + try: + amount = minor_units(Decimal(cleaned), currency) + except (InvalidOperation, ValueError): + return None + return -amount if negative else amount + + +def reconcile_candidates( + candidates: list[CandidateTransaction], + account_type: AccountType | str, +) -> dict[str, Any]: + """Reconcile balance-chain evidence without changing the ledger. + + A statement balance is a closing balance for its row. The first row therefore gives a + deterministic opening balance by subtracting that row's natural movement. + """ + coverage_pass = all( + candidate.included and (candidate.state != "error" or candidate.duplicate_of is not None) + for candidate in candidates + ) + rows: list[tuple[CandidateTransaction, int]] = [] + for candidate in candidates: + if not candidate.included or candidate.duplicate_of is not None: + continue + balance = _balance_minor(candidate.raw_fields.get("balance", ""), candidate.currency) + if balance is None: + continue + if candidate.signed_amount_minor is None: + continue + movement = candidate.signed_amount_minor + if account_nature(account_type) == "liability": + movement = -movement + rows.append((candidate, balance)) + + if not rows: + return { + "arithmetic_integrity": "not_available", + "coverage_integrity": "pass" if coverage_pass else "incomplete", + "status": "not available" if coverage_pass else "incomplete", + "reconciled": False, + "evidence": "no opening/closing balance column was detected", + } + + arithmetic_pass = True + first_signed = rows[0][0].signed_amount_minor + assert first_signed is not None + expected = rows[0][1] - ( + first_signed if account_nature(account_type) == "asset" else -first_signed + ) + previous = expected + for candidate, balance in rows: + movement = candidate.signed_amount_minor or 0 + if account_nature(account_type) == "liability": + movement = -movement + if previous + movement != balance: + arithmetic_pass = False + previous = balance + + arithmetic = "pass" if arithmetic_pass else "mismatch" + coverage = "pass" if coverage_pass else "incomplete" + return { + "arithmetic_integrity": arithmetic, + "coverage_integrity": coverage, + "status": "reconciled" + if arithmetic_pass and coverage_pass + else coverage + if not coverage_pass + else "mismatch", + "reconciled": arithmetic_pass and coverage_pass, + "opening_balance_minor": expected, + "closing_balance_minor": rows[-1][1], + "currency": rows[0][0].currency, + "evidence": f"{len(rows)} balance-linked transaction rows", + } diff --git a/src/pfa/ingestion/transfers.py b/src/pfa/ingestion/transfers.py new file mode 100644 index 0000000..a8eec86 --- /dev/null +++ b/src/pfa/ingestion/transfers.py @@ -0,0 +1,148 @@ +from __future__ import annotations + +import hashlib +import json +from dataclasses import dataclass +from datetime import UTC, datetime + +from pfa.db.models import ( + AccountModel, + TransactionModel, + TransferEventModel, + TransferLegModel, + TransferMatchDecisionModel, +) +from pfa.db.unit_of_work import UnitOfWork +from pfa.domain.accounts import AccountType +from pfa.domain.transactions import ( + TransferLegRole, + TransferMatchState, + TransferPurpose, + signed_minor, +) + + +@dataclass(frozen=True, slots=True) +class TransferMatchResult: + accepted: int = 0 + suggested: int = 0 + + +def _match_key(left_id: int, right_id: int) -> str: + return hashlib.sha256(f"{left_id}:{right_id}".encode()).hexdigest() + + +def _is_card_payment(row: TransactionModel) -> bool: + return ( + getattr(row, "kind", None) == "transfer" + and getattr(row, "transfer_purpose", None) == TransferPurpose.CREDIT_CARD_PAYMENT.value + and signed_minor(row.amount_minor, row.flow_direction) > 0 + ) + + +def _owned_card(account: AccountModel) -> bool: + return AccountType(account.account_type) == AccountType.CREDIT_CARD + + +def match_transfers(uow: UnitOfWork) -> TransferMatchResult: + accounts = {account.id: account for account in uow.accounts.all()} + rows = uow.transactions.all() + linked = uow.transfers.linked_transaction_ids() + cards = [ + row + for row in rows + if row.id not in linked + and _is_card_payment(row) + and row.account_id in accounts + and _owned_card(accounts[row.account_id]) + ] + banks = [ + row + for row in rows + if row.id not in linked + and row.account_id in accounts + and AccountType(accounts[row.account_id].account_type) + in {AccountType.CURRENT, AccountType.SAVINGS} + and signed_minor(row.amount_minor, row.flow_direction) < 0 + and "AMERICAN EXPRESS" in row.raw_description.upper() + ] + possible: list[tuple[TransactionModel, TransactionModel, bool, tuple[str, ...]]] = [] + for bank in banks: + for card in cards: + if bank.currency.upper() != card.currency.upper(): + continue + if abs(signed_minor(bank.amount_minor, bank.flow_direction)) != abs( + signed_minor(card.amount_minor, card.flow_direction) + ): + continue + if abs((bank.transaction_date - card.transaction_date).days) > 3: + continue + card_account = accounts[card.account_id] + institution = (card_account.institution or card_account.name).upper() + strong = bool(bank.external_id and bank.external_id == card.external_id) or ( + "AMERICAN EXPRESS" in institution or "AMEX" in institution + ) + reasons: tuple[str, ...] = ( + ("shared_reference",) + if bank.external_id and bank.external_id == card.external_id + else ("institution_cue",) + if strong + else ("amount_date_only",) + ) + possible.append((bank, card, strong, reasons)) + + counts: dict[int, int] = {} + for bank, card, _, _ in possible: + counts[bank.id] = counts.get(bank.id, 0) + 1 + counts[card.id] = counts.get(card.id, 0) + 1 + + accepted = suggested = 0 + now = datetime.now(UTC).replace(tzinfo=None) + for bank, card, strong, reasons in possible: + key = _match_key(bank.id, card.id) + if uow.transfers.decision(key) is not None: + continue + ambiguous = counts[bank.id] > 1 or counts[card.id] > 1 + accepted_match = strong and not ambiguous + decision = TransferMatchDecisionModel( + stable_match_key=key, + left_transaction_id=bank.id, + right_transaction_id=card.id, + state=( + TransferMatchState.ACCEPTED if accepted_match else TransferMatchState.SUGGESTED + ).value, + confidence=0.99 if accepted_match else 0.55, + reason_codes_json=json.dumps(["ambiguous_amount_date"] if ambiguous else list(reasons)), + created_at=now, + ) + uow.transfers.add_decision(decision) + if accepted_match: + event = TransferEventModel( + purpose=TransferPurpose.CREDIT_CARD_PAYMENT.value, + match_method="automatic", + created_at=now, + ) + uow.transfers.add_event( + event, + [ + TransferLegModel( + transaction_id=bank.id, + role=TransferLegRole.SOURCE.value, + ), + TransferLegModel( + transaction_id=card.id, + role=TransferLegRole.DESTINATION.value, + ), + ], + ) + decision.event_id = event.id + bank.kind = "transfer" + bank.transfer_purpose = TransferPurpose.CREDIT_CARD_PAYMENT.value + bank.category = None + bank.classification_source = "rule" + bank.classification_reason = "paired owned credit-card repayment" + linked.update((bank.id, card.id)) + accepted += 1 + else: + suggested += 1 + return TransferMatchResult(accepted=accepted, suggested=suggested) From 2dfd09c4ae160a03db5f641bc2e169014d461331 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:29:57 +0100 Subject: [PATCH 06/24] feat(imports): add safe idempotent undo --- src/pfa/api/app.py | 28 +++++++++++++++++++++++++++ src/pfa/db/repositories.py | 12 ++++++++++++ src/pfa/ingestion/batches.py | 36 +++++++++++++++++++++++++++++++++++ src/pfa/ingestion/dialects.py | 10 +++++++--- 4 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/pfa/api/app.py b/src/pfa/api/app.py index ccf5a3c..27584d7 100644 --- a/src/pfa/api/app.py +++ b/src/pfa/api/app.py @@ -38,6 +38,7 @@ discard_batch, load_batch, sweep_expired_batches, + undo_batch, ) from pfa.ingestion.candidates import FILE_TOO_LARGE, CandidateIssue, CandidateTransaction from pfa.ingestion.service import ImportService @@ -173,6 +174,10 @@ def one_binding(self) -> ImportBatchPatchRequest: amount_sign: Literal["as_written", "debit_positive"] | None = None +class UndoImportRequest(BaseModel): + confirm_changed: bool = False + + class ScenarioRequest(BaseModel): cost_minor: int = Field(ge=0) horizon_months: int = Field(default=3, ge=1, le=120) @@ -465,6 +470,29 @@ def commit_import_batch(batch_id: str) -> ImportBatchResponse: close_services(engine, services, False) raise + @app.post("/imports/{batch_id}/undo", response_model=ImportBatchResponse) + def undo_import_batch( + batch_id: str, request: UndoImportRequest | None = None + ) -> ImportBatchResponse: + engine, services = open_services(active_settings) + try: + batch = undo_batch( + services.uow, + batch_id, + confirm_changed=request.confirm_changed if request else False, + ) + response = _batch_response(batch) + close_services(engine, services) + return response + except BatchError as exc: + close_services(engine, services, False) + raise HTTPException( + status_code=exc.status_code, detail={"code": exc.code, "message": exc.message} + ) from exc + except Exception: + close_services(engine, services, False) + raise + @app.delete("/imports/{batch_id}", response_model=ImportBatchResponse) def delete_import_batch(batch_id: str) -> ImportBatchResponse: engine, services = open_services(active_settings) diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index 29c84e6..5b22e43 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -247,6 +247,7 @@ def delete_for_transactions(self, transaction_ids: set[int]) -> None: event_ids = {leg.event_id for leg in legs} for leg in legs: self.session.delete(leg) + self.session.flush() for event_id in event_ids: remaining = self.session.scalar( select(TransferLegModel.id).where(TransferLegModel.event_id == event_id).limit(1) @@ -255,6 +256,17 @@ def delete_for_transactions(self, transaction_ids: set[int]) -> None: event = self.session.get(TransferEventModel, event_id) if event is not None: self.session.delete(event) + elif ( + len( + self.session.scalars( + select(TransferLegModel).where(TransferLegModel.event_id == event_id) + ).all() + ) + < 2 + ): + event = self.session.get(TransferEventModel, event_id) + if event is not None: + self.session.delete(event) decisions = self.session.scalars( select(TransferMatchDecisionModel).where( (TransferMatchDecisionModel.left_transaction_id.in_(transaction_ids)) diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index 9bb4c7e..1ba1a89 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -47,6 +47,7 @@ RECONCILIATION_MISMATCH, STATEMENT_YEAR_INFERRED, TOO_MANY_ROWS, + UNDO_REQUIRES_CONFIRMATION, VALID, WARNING, CandidateIssue, @@ -804,6 +805,41 @@ def commit_batch(uow: UnitOfWork, batch_id: str, settings: Settings) -> ImportBa return uow.import_batches.add(batch) +def undo_batch( + uow: UnitOfWork, batch_id: str, *, confirm_changed: bool = False +) -> ImportBatchModel: + batch = load_batch(uow, batch_id) + if batch.status == "undone": + return batch + if batch.status != "committed": + raise BatchError( + BATCH_NOT_EDITABLE, + f"batch is {batch.status}; only committed imports can be undone", + 409, + ) + ids = set(batch_committed_transaction_ids(batch)) + rows = uow.transactions.by_ids(list(ids)) + changed = sum( + 1 + for row in rows + if batch.committed_at is not None + and row.updated_at is not None + and row.updated_at > batch.committed_at + ) + if changed and not confirm_changed: + raise BatchError( + UNDO_REQUIRES_CONFIRMATION, + f"{changed} imported row(s) were edited after import; confirm undo to remove them", + 409, + ) + uow.transfers.delete_for_transactions(ids) + for row in rows: + uow.session.delete(row) + batch.status = "undone" + batch.updated_at = _now() + return uow.import_batches.add(batch) + + def discard_batch(uow: UnitOfWork, batch_id: str) -> ImportBatchModel: batch = load_batch(uow, batch_id) if batch.status == "committed": diff --git a/src/pfa/ingestion/dialects.py b/src/pfa/ingestion/dialects.py index 9658380..597a37e 100644 --- a/src/pfa/ingestion/dialects.py +++ b/src/pfa/ingestion/dialects.py @@ -126,10 +126,10 @@ def _csv_detection(path: Path) -> AdapterDetection: header = [] headers = {" ".join(cell.strip().lower().split()) for cell in header} if ( - "american express" in lower - or "card member" in lower + "card member" in lower or "membership number" in lower or "payment received - thank you" in lower + or ("american express" in lower and "card" in lower) ): return AdapterDetection( AMEX_UK_CSV, @@ -155,7 +155,11 @@ def _pdf_detection(path: Path) -> AdapterDetection: except Exception: return AdapterDetection(GENERIC, 0.0, ("unreadable_content",)) lower = text.lower() - if "american express" in lower or "payment received - thank you" in lower: + if ( + "card member" in lower + or "membership number" in lower + or ("american express" in lower and "card" in lower) + ): return AdapterDetection( AMEX_UK_PDF, 0.98, From 2c20957b2aef70a74314e491714caddbb21c69c1 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:30:02 +0100 Subject: [PATCH 07/24] feat(imports): expose reconciliation blocking codes --- src/pfa/ingestion/candidates.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pfa/ingestion/candidates.py b/src/pfa/ingestion/candidates.py index 9f5955c..dc161a5 100644 --- a/src/pfa/ingestion/candidates.py +++ b/src/pfa/ingestion/candidates.py @@ -67,6 +67,8 @@ ACCOUNT_REQUIRED = "ACCOUNT_REQUIRED" INVALID_ACCOUNT_DRAFT = "INVALID_ACCOUNT_DRAFT" GENERIC_SIGN_CONFIRMATION_REQUIRED = "GENERIC_SIGN_CONFIRMATION_REQUIRED" +RECONCILIATION_MISMATCH = "RECONCILIATION_MISMATCH" +RECONCILIATION_INCOMPLETE = "RECONCILIATION_INCOMPLETE" UNDO_REQUIRES_CONFIRMATION = "UNDO_REQUIRES_CONFIRMATION" From cb44fda25dc1bf61d3805f035f0915759851417b Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:37:45 +0100 Subject: [PATCH 08/24] feat(imports): expose review, reconciliation, and transfer workflows --- docs/architecture.md | 15 ++- src/pfa/api/app.py | 198 +++++++++++++++++++++++++++- src/pfa/db/repositories.py | 12 ++ src/pfa/ingestion/batches.py | 19 ++- src/pfa/ingestion/candidates.py | 39 +++++- src/pfa/ingestion/extractors/pdf.py | 27 +++- src/pfa/ingestion/transfers.py | 161 ++++++++++++++++++---- src/pfa/web/app.js | 69 ++++++++-- src/pfa/web/index.html | 27 +++- src/pfa/web/styles.css | 31 +++++ 10 files changed, 546 insertions(+), 52 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 7242ef1..838acd5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -30,9 +30,22 @@ flowchart TD - `ai` provides local Ollama integration and read-only tools over services. - `cli` and `api` are presentation/composition layers. +Transactions retain the legacy absolute `amount_minor` plus `flow_direction` (`debit`/`credit`) +for storage compatibility. The canonical economic polarity is `signed_minor`: positive is money +into the account's net-worth contribution and negative is money out. It is derived as +`amount_minor` for `credit`, otherwise `-amount_minor`; source CR/DR markers and transaction kind +(expense, income, refund, or transfer) are separate concepts. Analytics consumes canonical signs, +not accounting debit/credit terminology. + +Accounts have stable IDs and an explicit type. `current`, `savings`, and `cash` are liquid assets; +`investment` is an illiquid asset; `credit_card` and `loan` are liabilities. Opening balances are +natural account balances and are dated as end-of-day baselines. Liquid cash includes only canonical +signed movements strictly after that baseline and is marked incomplete when a baseline is missing. + Transfers between owned accounts are persisted for auditability but excluded from income and spending. Savings and investment transfers are separately tagged so wealth-building metrics do -not become ordinary consumption. +not become ordinary consumption. Card repayments use `credit_card_payment` and count once from +the positive card leg as debt repayment; interest and fees remain debt costs. ## Deliberate constraints diff --git a/src/pfa/api/app.py b/src/pfa/api/app.py index 27584d7..6b0bd65 100644 --- a/src/pfa/api/app.py +++ b/src/pfa/api/app.py @@ -20,10 +20,16 @@ from pfa.ai.deps import FinanceDependencies from pfa.ai.models import available_models from pfa.ai.schemas import ChatRequest, ImportRequest +from pfa.analytics.service import cash_position from pfa.config import Settings, get_settings -from pfa.db.models import ImportBatchModel +from pfa.db.models import ( + ImportBatchModel, + TransferEventModel, + TransferMatchDecisionModel, +) from pfa.domain.accounts import AccountType from pfa.domain.errors import BatchError, UploadRejected +from pfa.domain.transactions import TransferLegRole, TransferPurpose, signed_minor from pfa.ingestion.batches import ( BatchPatch, NewAccountDraft, @@ -42,6 +48,11 @@ ) from pfa.ingestion.candidates import FILE_TOO_LARGE, CandidateIssue, CandidateTransaction from pfa.ingestion.service import ImportService +from pfa.ingestion.transfers import ( + accept_suggestion, + create_manual_link, + dismiss_suggestion, +) from pfa.ingestion.upload import stage_upload, sweep_upload_dir from pfa.observability import TimedOperation from pfa.services.answers import deterministic_answer @@ -62,6 +73,8 @@ class TransactionResponse(BaseModel): kind: str category: str | None classification_source: str + signed_amount_minor: int + account_id: int class AccountResponse(BaseModel): @@ -81,7 +94,7 @@ class NewAccountRequest(BaseModel): account_type: AccountType = AccountType.CURRENT currency: str = Field(default="GBP", min_length=3, max_length=3) institution: str | None = Field(default=None, max_length=120) - last4: str | None = Field(default=None, min_length=4, max_length=4, pattern=r"^\\d{4}$") + last4: str | None = Field(default=None, min_length=4, max_length=4, pattern=r"^\d{4}$") opening_balance_minor: int = 0 opening_balance_as_of: date | None = None @@ -178,6 +191,38 @@ class UndoImportRequest(BaseModel): confirm_changed: bool = False +class TransferLegRequest(BaseModel): + transaction_id: int = Field(gt=0) + role: TransferLegRole + + +class TransferLinkRequest(BaseModel): + legs: list[TransferLegRequest] = Field(min_length=2) + purpose: str = TransferPurpose.OTHER.value + + +class TransferSuggestionResponse(BaseModel): + id: int + left_transaction_id: int + right_transaction_id: int + state: str + confidence: float + reason_codes: list[str] + event_id: int | None + + +class TransferLegResponse(BaseModel): + transaction_id: int + role: str + + +class TransferEventResponse(BaseModel): + id: int + purpose: str + match_method: str + legs: list[TransferLegResponse] + + class ScenarioRequest(BaseModel): cost_minor: int = Field(ge=0) horizon_months: int = Field(default=3, ge=1, le=120) @@ -531,6 +576,39 @@ def accounts() -> list[AccountResponse]: finally: close_services(engine, services) + @app.post("/accounts", response_model=AccountResponse) + def create_account(request: NewAccountRequest) -> AccountResponse: + engine, services = open_services(active_settings) + try: + account = services.uow.accounts.create( + request.name, + request.currency, + request.account_type.value, + institution=request.institution, + last4=request.last4, + opening_balance_minor=request.opening_balance_minor, + opening_balance_as_of=request.opening_balance_as_of, + ) + response = AccountResponse( + id=account.id, + name=account.name, + account_type=account.account_type, + currency=account.currency, + institution=account.institution, + last4=account.last4, + opening_balance_minor=account.opening_balance_minor, + opening_balance_as_of=account.opening_balance_as_of, + active=account.active, + ) + close_services(engine, services) + return response + except ValueError as exc: + close_services(engine, services, False) + raise HTTPException(status_code=422, detail=str(exc)) from exc + except Exception: + close_services(engine, services, False) + raise + @app.get("/transactions", response_model=list[TransactionResponse]) def transactions( limit: Annotated[int, Query(ge=1, le=500)] = 100, @@ -550,12 +628,128 @@ def transactions( kind=row.kind, category=row.category, classification_source=row.classification_source, + signed_amount_minor=(signed_minor(row.amount_minor, row.flow_direction)), + account_id=row.account_id, ) for row in rows ] finally: close_services(engine, services) + def _suggestion_response( + decision: TransferMatchDecisionModel, + ) -> TransferSuggestionResponse: + return TransferSuggestionResponse( + id=decision.id, + left_transaction_id=decision.left_transaction_id, + right_transaction_id=decision.right_transaction_id, + state=decision.state, + confidence=decision.confidence, + reason_codes=json.loads(decision.reason_codes_json), + event_id=decision.event_id, + ) + + def _event_response(event: TransferEventModel) -> TransferEventResponse: + return TransferEventResponse( + id=event.id, + purpose=event.purpose, + match_method=event.match_method, + legs=[ + TransferLegResponse(transaction_id=leg.transaction_id, role=leg.role) + for leg in event.legs + ], + ) + + @app.get("/analytics/cash") + def cash(currency: str = "GBP", as_of: date | None = None) -> dict[str, object]: + engine, services = open_services(active_settings) + try: + position = cash_position( + services.uow.accounts.all(), services.uow.transactions.all(), currency, as_of + ) + return { + "currency": position.currency, + "as_of": as_of, + "cash_minor": position.total_minor, + "known_subtotal_minor": position.known_subtotal_minor, + "coverage_status": position.coverage_status, + "missing_account_ids": list(position.missing_account_ids), + } + finally: + close_services(engine, services) + + @app.get("/transfers/suggestions", response_model=list[TransferSuggestionResponse]) + def transfer_suggestions() -> list[TransferSuggestionResponse]: + engine, services = open_services(active_settings) + try: + return [_suggestion_response(item) for item in services.uow.transfers.suggestions()] + finally: + close_services(engine, services) + + @app.post("/transfers/suggestions/{decision_id}/accept", response_model=TransferEventResponse) + def accept_transfer_suggestion(decision_id: int) -> TransferEventResponse: + engine, services = open_services(active_settings) + try: + event = accept_suggestion(services.uow, decision_id) + response = _event_response(event) + close_services(engine, services) + return response + except BatchError as exc: + close_services(engine, services, False) + raise HTTPException( + status_code=exc.status_code, detail={"code": exc.code, "message": exc.message} + ) from exc + + @app.post( + "/transfers/suggestions/{decision_id}/dismiss", response_model=TransferSuggestionResponse + ) + def dismiss_transfer_suggestion(decision_id: int) -> TransferSuggestionResponse: + engine, services = open_services(active_settings) + try: + decision = dismiss_suggestion(services.uow, decision_id) + response = _suggestion_response(decision) + close_services(engine, services) + return response + except BatchError as exc: + close_services(engine, services, False) + raise HTTPException( + status_code=exc.status_code, detail={"code": exc.code, "message": exc.message} + ) from exc + + @app.post("/transfers/link", response_model=TransferEventResponse) + def link_transfer(request: TransferLinkRequest) -> TransferEventResponse: + engine, services = open_services(active_settings) + try: + event = create_manual_link( + services.uow, + [(leg.transaction_id, leg.role.value) for leg in request.legs], + request.purpose, + ) + response = _event_response(event) + close_services(engine, services) + return response + except BatchError as exc: + close_services(engine, services, False) + raise HTTPException( + status_code=exc.status_code, detail={"code": exc.code, "message": exc.message} + ) from exc + + @app.delete("/transfers/events/{event_id}") + def unlink_transfer(event_id: int) -> dict[str, object]: + engine, services = open_services(active_settings) + try: + event = services.uow.transfers.get_event(event_id) + if event is None: + raise BatchError("TRANSFER_EVENT_NOT_FOUND", "transfer event not found", 404) + services.uow.transfers.delete_event(event_id) + close_services(engine, services) + return {"id": event_id, "unlinked": True} + except BatchError as exc: + close_services(engine, services, False) + raise HTTPException( + status_code=exc.status_code, detail={"code": exc.code, "message": exc.message} + ) from exc + @app.get("/fx/rates", response_model=list[FxRateResponse]) def get_fx_rates( base: str | None = None, diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index 5b22e43..bea7066 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -227,6 +227,12 @@ def add_decision(self, decision: TransferMatchDecisionModel) -> TransferMatchDec self.session.flush() return decision + def get_event(self, event_id: int) -> TransferEventModel | None: + return self.session.get(TransferEventModel, event_id) + + def decisions(self) -> list[TransferMatchDecisionModel]: + return list(self.session.scalars(select(TransferMatchDecisionModel))) + def suggestions(self) -> list[TransferMatchDecisionModel]: return list( self.session.scalars( @@ -236,6 +242,12 @@ def suggestions(self) -> list[TransferMatchDecisionModel]: ) ) + def delete_event(self, event_id: int) -> None: + event = self.session.get(TransferEventModel, event_id) + if event is not None: + self.session.delete(event) + self.session.flush() + def delete_for_transactions(self, transaction_ids: set[int]) -> None: if not transaction_ids: return diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index 1ba1a89..be8fcec 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -558,7 +558,11 @@ def create_batch( service.validate(candidates) if batch.amount_sign: for candidate in candidates: - _apply_amount_sign(candidate, batch.amount_sign) + _apply_amount_sign( + candidate, + batch.amount_sign, + amex_card=batch.adapter_id in {"amex_uk_csv", "amex_uk_pdf"}, + ) service.resolve_duplicates(candidates) if not candidates and not any(issue.severity == ERROR for issue in extraction.issues): @@ -625,7 +629,9 @@ def _resolve_ambiguous_amount(candidate: CandidateTransaction, mode: str) -> Non candidate.issues = [issue for issue in candidate.issues if issue.code != AMBIGUOUS_SIGN] -def _apply_amount_sign(candidate: CandidateTransaction, convention: str) -> None: +def _apply_amount_sign( + candidate: CandidateTransaction, convention: str, *, amex_card: bool = False +) -> None: """Re-reads a row's flow direction from its single amount column under the statement's sign convention. A credit-card export writes a purchase as a positive figure, which `as_written` books as income. @@ -638,6 +644,9 @@ def _apply_amount_sign(candidate: CandidateTransaction, convention: str) -> None """ if candidate.direction is None: return + if amex_card and "PAYMENT RECEIVED" in candidate.raw_description.upper(): + candidate.direction = "credit" + return if candidate.direction_explicit: return if "debit" in candidate.raw_fields or "credit" in candidate.raw_fields: @@ -724,7 +733,11 @@ def apply_patch(uow: UnitOfWork, batch_id: str, patch: BatchPatch) -> ImportBatc # that clears an amount would quietly hand that row back to as_written. # Still after validate (which sets direction) and before duplicate # resolution, whose fingerprint covers the signed amount. - _apply_amount_sign(candidate, batch.amount_sign) + _apply_amount_sign( + candidate, + batch.amount_sign, + amex_card=batch.adapter_id in {"amex_uk_csv", "amex_uk_pdf"}, + ) service.resolve_duplicates(candidates) batch.candidates_json = candidates_to_json(candidates) diff --git a/src/pfa/ingestion/candidates.py b/src/pfa/ingestion/candidates.py index dc161a5..d6d45c1 100644 --- a/src/pfa/ingestion/candidates.py +++ b/src/pfa/ingestion/candidates.py @@ -9,6 +9,7 @@ from __future__ import annotations import json +import re from dataclasses import asdict, dataclass, field from datetime import date, datetime from decimal import Decimal, InvalidOperation @@ -77,7 +78,14 @@ # lowercased and whitespace-collapsed - the spec forbids fuzzy guessing. HEADER_ALIASES: dict[str, tuple[str, ...]] = { "date": ("date", "transaction date", "transaction_date"), - "posted_date": ("posted date", "posted_date", "posting date", "posting_date"), + "posted_date": ( + "posted date", + "posted_date", + "posting date", + "posting_date", + "received by us", + "receivedbyus", + ), "description": ("description", "details", "narrative", "merchant"), "debit": ("debit", "paid out", "paid_out", "withdrawn", "money out", "money_out"), "credit": ("credit", "paid in", "paid_in", "received", "money in", "money_in"), @@ -88,11 +96,21 @@ def match_header_alias(cell_text: str) -> str | None: - """Maps one header cell onto its canonical field name, or None if nothing matches.""" - normalized = " ".join(cell_text.strip().lower().split()) + """Map deterministic bank-header variants onto one canonical field name.""" + normalized = re.sub(r"[^a-z0-9]+", " ", cell_text.strip().lower()).strip() + compact = normalized.replace(" ", "") for field_name, aliases in HEADER_ALIASES.items(): - if normalized in aliases: - return field_name + for alias in aliases: + alias_normalized = re.sub(r"[^a-z0-9]+", " ", alias).strip() + alias_compact = alias_normalized.replace(" ", "") + if normalized == alias_normalized or compact == alias_compact: + return field_name + if alias_normalized in normalized and field_name in {"description", "posted_date"}: + return field_name + if compact in {"transactiondate", "transactiondt"}: + return "date" + if compact in {"receivedbyus", "postingdate"}: + return "posted_date" return None @@ -184,6 +202,7 @@ def parse_amount(value: str, currency: str = "GBP") -> tuple[int, int, bool]: .strip() ) is_cr = False + is_dr = False upper = cleaned.upper() if upper.endswith("CR."): cleaned = cleaned[:-3].strip() @@ -191,15 +210,21 @@ def parse_amount(value: str, currency: str = "GBP") -> tuple[int, int, bool]: elif upper.endswith("CR"): cleaned = cleaned[:-2].strip() is_cr = True + elif upper.endswith("DR"): + cleaned = cleaned[:-2].strip() + is_dr = True elif upper.startswith("CR"): cleaned = cleaned[2:].strip() is_cr = True + elif upper.startswith("DR"): + cleaned = cleaned[2:].strip() + is_dr = True try: decimal = Decimal(cleaned) except InvalidOperation as exc: raise ImportRowError(f"invalid amount {value!r}") from exc - sign = 1 if is_cr else (-1 if decimal < 0 else 1) - return sign, minor_units(abs(decimal), currency), is_cr + sign = 1 if is_cr else (-1 if decimal < 0 or is_dr else 1) + return sign, minor_units(abs(decimal), currency), is_cr or is_dr @dataclass(frozen=True, slots=True) diff --git a/src/pfa/ingestion/extractors/pdf.py b/src/pfa/ingestion/extractors/pdf.py index f2efc02..1232071 100644 --- a/src/pfa/ingestion/extractors/pdf.py +++ b/src/pfa/ingestion/extractors/pdf.py @@ -248,6 +248,8 @@ def _process_lines_for_column( columns: list[tuple[float, str]] | None = None header_top: float | None = None rows: list[_RawRow] = [] + pending_header: list[tuple[float, str, float | None]] = [] + pending_header_top: float | None = None position = 0 for line in lines: cells = _split_cells(line) @@ -258,9 +260,18 @@ def _process_lines_for_column( continue if columns is None: columns = _header_columns(cells) + if columns is None and pending_header: + columns = _header_columns([*pending_header, *cells]) if columns is not None: - header_top = line[0]["top"] + header_top = pending_header_top or line[0]["top"] + pending_header = [] + pending_header_top = None continue + names = {match_header_alias(text) for _, text, _ in cells} + names.discard(None) + if names and names != {"date"}: + pending_header = cells + pending_header_top = line[0]["top"] if len(cells) >= 2: first_text = cells[0][1] last_text = cells[-1][1] @@ -413,9 +424,15 @@ def clean_amount_text(text: str) -> tuple[str, bool]: elif upper.endswith("CR"): cleaned = cleaned[:-2].strip() negative = False + elif upper.endswith("DR"): + cleaned = cleaned[:-2].strip() + negative = True elif upper.startswith("CR"): cleaned = cleaned[2:].strip() negative = False + elif upper.startswith("DR"): + cleaned = cleaned[2:].strip() + negative = True return cleaned, negative @@ -438,10 +455,14 @@ def _resolve_amount( amount_text = fields.get("amount", "").strip() is_explicit_cr = False + is_explicit_dr = False + amount_upper = amount_text.upper() for marker in dialect.credit_markers: - if marker in amount_text.upper() or fields.get("type", "").upper() == marker: + if marker in amount_upper or fields.get("type", "").upper() == marker: is_explicit_cr = True break + if amount_upper.endswith("DR") or fields.get("type", "").upper() == "DR": + is_explicit_dr = True if amount_text: parsed = _signed_minor(amount_text, currency) @@ -450,6 +471,8 @@ def _resolve_amount( minor, negative = parsed if is_explicit_cr: return _AmountResult(minor=minor, direction="credit", direction_explicit=True) + if is_explicit_dr: + return _AmountResult(minor=minor, direction="debit", direction_explicit=True) return _AmountResult(minor=minor, direction="debit" if negative else "credit") if debit_text and credit_text: diff --git a/src/pfa/ingestion/transfers.py b/src/pfa/ingestion/transfers.py index a8eec86..d8e0937 100644 --- a/src/pfa/ingestion/transfers.py +++ b/src/pfa/ingestion/transfers.py @@ -14,7 +14,9 @@ ) from pfa.db.unit_of_work import UnitOfWork from pfa.domain.accounts import AccountType +from pfa.domain.errors import BatchError from pfa.domain.transactions import ( + TransactionKind, TransferLegRole, TransferMatchState, TransferPurpose, @@ -34,8 +36,8 @@ def _match_key(left_id: int, right_id: int) -> str: def _is_card_payment(row: TransactionModel) -> bool: return ( - getattr(row, "kind", None) == "transfer" - and getattr(row, "transfer_purpose", None) == TransferPurpose.CREDIT_CARD_PAYMENT.value + row.kind == TransactionKind.TRANSFER.value + and row.transfer_purpose == TransferPurpose.CREDIT_CARD_PAYMENT.value and signed_minor(row.amount_minor, row.flow_direction) > 0 ) @@ -44,6 +46,40 @@ def _owned_card(account: AccountModel) -> bool: return AccountType(account.account_type) == AccountType.CREDIT_CARD +def _event_for_pair( + uow: UnitOfWork, + source: TransactionModel, + destination: TransactionModel, + *, + method: str, +) -> TransferEventModel: + now = datetime.now(UTC).replace(tzinfo=None) + event = TransferEventModel( + purpose=TransferPurpose.CREDIT_CARD_PAYMENT.value, + match_method=method, + created_at=now, + ) + uow.transfers.add_event( + event, + [ + TransferLegModel(transaction_id=source.id, role=TransferLegRole.SOURCE.value), + TransferLegModel( + transaction_id=destination.id, + role=TransferLegRole.DESTINATION.value, + ), + ], + ) + return event + + +def _mark_bank_payment(bank: TransactionModel) -> None: + bank.kind = TransactionKind.TRANSFER.value + bank.transfer_purpose = TransferPurpose.CREDIT_CARD_PAYMENT.value + bank.category = None + bank.classification_source = "rule" + bank.classification_reason = "paired owned credit-card repayment" + + def match_transfers(uow: UnitOfWork) -> TransferMatchResult: accounts = {account.id: account for account in uow.accounts.all()} rows = uow.transactions.all() @@ -117,32 +153,107 @@ def match_transfers(uow: UnitOfWork) -> TransferMatchResult: ) uow.transfers.add_decision(decision) if accepted_match: - event = TransferEventModel( - purpose=TransferPurpose.CREDIT_CARD_PAYMENT.value, - match_method="automatic", - created_at=now, - ) - uow.transfers.add_event( - event, - [ - TransferLegModel( - transaction_id=bank.id, - role=TransferLegRole.SOURCE.value, - ), - TransferLegModel( - transaction_id=card.id, - role=TransferLegRole.DESTINATION.value, - ), - ], - ) + event = _event_for_pair(uow, bank, card, method="automatic") decision.event_id = event.id - bank.kind = "transfer" - bank.transfer_purpose = TransferPurpose.CREDIT_CARD_PAYMENT.value - bank.category = None - bank.classification_source = "rule" - bank.classification_reason = "paired owned credit-card repayment" + _mark_bank_payment(bank) linked.update((bank.id, card.id)) accepted += 1 else: suggested += 1 return TransferMatchResult(accepted=accepted, suggested=suggested) + + +def accept_suggestion(uow: UnitOfWork, decision_id: int) -> TransferEventModel: + decision = uow.session.get(TransferMatchDecisionModel, decision_id) + if decision is None or decision.state != TransferMatchState.SUGGESTED.value: + raise BatchError( + "TRANSFER_DECISION_INVALID", "transfer suggestion is no longer reviewable", 409 + ) + source = uow.session.get(TransactionModel, decision.left_transaction_id) + destination = uow.session.get(TransactionModel, decision.right_transaction_id) + if source is None or destination is None: + raise BatchError( + "TRANSFER_TRANSACTION_NOT_FOUND", "transfer transaction no longer exists", 422 + ) + event = create_manual_link( + uow, + [ + (source.id, TransferLegRole.SOURCE.value), + (destination.id, TransferLegRole.DESTINATION.value), + ], + TransferPurpose.CREDIT_CARD_PAYMENT.value, + ) + decision.state = TransferMatchState.ACCEPTED.value + decision.event_id = event.id + decision.reviewed_at = datetime.now(UTC).replace(tzinfo=None) + _mark_bank_payment(source) + return event + + +def dismiss_suggestion(uow: UnitOfWork, decision_id: int) -> TransferMatchDecisionModel: + decision = uow.session.get(TransferMatchDecisionModel, decision_id) + if decision is None or decision.state != TransferMatchState.SUGGESTED.value: + raise BatchError( + "TRANSFER_DECISION_INVALID", "transfer suggestion is no longer reviewable", 409 + ) + decision.state = TransferMatchState.DISMISSED.value + decision.reviewed_at = datetime.now(UTC).replace(tzinfo=None) + return decision + + +def create_manual_link( + uow: UnitOfWork, + legs: list[tuple[int, str]], + purpose: str = TransferPurpose.OTHER.value, +) -> TransferEventModel: + if len(legs) < 2 or sum(role == TransferLegRole.SOURCE.value for _, role in legs) != 1: + raise BatchError( + "TRANSFER_ROLES_INVALID", + "a transfer needs exactly one source and two or more legs", + 422, + ) + if sum(role == TransferLegRole.DESTINATION.value for _, role in legs) != 1: + raise BatchError("TRANSFER_ROLES_INVALID", "a transfer needs exactly one destination", 422) + if any(role not in {item.value for item in TransferLegRole} for _, role in legs): + raise BatchError("TRANSFER_ROLES_INVALID", "unknown transfer leg role", 422) + ids = [transaction_id for transaction_id, _ in legs] + if len(set(ids)) != len(ids): + raise BatchError("TRANSFER_LEGS_DUPLICATE", "a transaction can appear only once", 422) + transactions = {row.id: row for row in uow.transactions.by_ids(ids)} + if len(transactions) != len(ids): + raise BatchError( + "TRANSFER_TRANSACTION_NOT_FOUND", "one or more transactions do not exist", 422 + ) + linked = uow.transfers.linked_transaction_ids() + if linked.intersection(ids): + raise BatchError( + "TRANSFER_ALREADY_LINKED", "one or more transactions are already linked", 409 + ) + account_ids = {transactions[transaction_id].account_id for transaction_id in ids} + if len(account_ids) < 2: + raise BatchError( + "TRANSFER_SAME_ACCOUNT", "transfer legs must belong to different accounts", 422 + ) + source_id = next(transaction_id for transaction_id, role in legs if role == "source") + destination_id = next(transaction_id for transaction_id, role in legs if role == "destination") + if ( + signed_minor(transactions[source_id].amount_minor, transactions[source_id].flow_direction) + >= 0 + ): + raise BatchError("TRANSFER_SIGN_INVALID", "the source leg must be money out", 422) + if ( + signed_minor( + transactions[destination_id].amount_minor, transactions[destination_id].flow_direction + ) + <= 0 + ): + raise BatchError("TRANSFER_SIGN_INVALID", "the destination leg must be money in", 422) + event = TransferEventModel(purpose=purpose, match_method="manual") + uow.transfers.add_event( + event, + [ + TransferLegModel(transaction_id=transaction_id, role=role) + for transaction_id, role in legs + ], + ) + return event diff --git a/src/pfa/web/app.js b/src/pfa/web/app.js index a30d7ba..913f072 100644 --- a/src/pfa/web/app.js +++ b/src/pfa/web/app.js @@ -202,9 +202,9 @@ function renderCurrentRoute() { $("nav-tx-count").textContent = state.transactions.length || (state.data[state.month]?.transaction_count || 0); if (state.accounts.length > 0) { $("active-account-label").textContent = state.accounts[0].name; - const knownList = $("known-accounts-list"); + const knownList = $("destination-account-select"); if (knownList) { - knownList.innerHTML = state.accounts.map((a) => ``).join(""); + knownList.innerHTML = `` + state.accounts.map((a) => ``).join(""); } } @@ -388,15 +388,20 @@ function setupUploadHandlers() { $("select-all-candidates").addEventListener("click", () => bulkToggleCandidates(true)); $("deselect-all-candidates").addEventListener("click", () => bulkToggleCandidates(false)); - // Destination Account Assign + // Destination Account Assign: existing accounts use stable IDs; new accounts are drafts + // and are only created with their transactions when the import is committed. $("save-account-btn").addEventListener("click", async () => { - const acc = $("destination-account-input").value.trim(); - if (!acc || !state.activeBatch) return; + const selected = $("destination-account-select").value; + const newName = $("new-account-name").value.trim(); + if ((!selected && !newName) || !state.activeBatch) return; + const body = selected + ? { destination_account_id: Number(selected) } + : { new_account: { name: newName, account_type: $("new-account-type").value, currency: state.activeBatch.detected_currency || "GBP" } }; try { const patched = await apiRequest(`/imports/${state.activeBatch.id}`, { method: "PATCH", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ account: acc }) + body: JSON.stringify(body) }); state.activeBatch = patched; showToast(`Assigned account "${acc}" to statement batch.`); @@ -437,6 +442,33 @@ function setupUploadHandlers() { } }); + $("undo-import-btn")?.addEventListener("click", async () => { + if (!state.activeBatch) return; + try { + await apiRequest(`/imports/${state.activeBatch.id}/undo`, { method: "POST" }); + showToast("Import undone; account was kept."); + $("batch-success-card").hidden = true; + $("upload-card").hidden = false; + state.activeBatch = null; + loadMonthData(state.month); + } catch (err) { + if (err.data?.detail?.code === "UNDO_REQUIRES_CONFIRMATION" && window.confirm(err.message)) { + await apiRequest(`/imports/${state.activeBatch.id}/undo`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ confirm_changed: true }) + }); + showToast("Import undone; edited rows were removed."); + $("batch-success-card").hidden = true; + $("upload-card").hidden = false; + state.activeBatch = null; + loadMonthData(state.month); + } else { + showToast(err.message, true); + } + } + }); + // Amount Sign Convention selector const amountSignSelect = $("amount-sign-select"); if (amountSignSelect) { @@ -506,9 +538,20 @@ function renderBatchInspector(batch) { $("batch-pages").textContent = batch.page_count || "1"; $("batch-period").textContent = batch.statement_start && batch.statement_end ? `${batch.statement_start} to ${batch.statement_end}` : "Auto-detected"; - $("destination-account-input").value = batch.destination_account || batch.detected_account || "Main Checking"; + $("destination-account-select").value = batch.destination_account_id ? String(batch.destination_account_id) : ""; + $("new-account-name").value = batch.new_account?.name || ""; + $("new-account-type").value = batch.new_account?.account_type || "current"; + + const semantic = batch.semantic_totals || {}; + $("batch-semantic-summary").innerHTML = ` + Import effect + Money in ${formatMoney(semantic.money_in_minor || 0, batch.detected_currency || "GBP")} + Spending ${formatMoney(semantic.spending_minor || 0, batch.detected_currency || "GBP")} + Refunds ${formatMoney(semantic.refunds_minor || 0, batch.detected_currency || "GBP")} + Transfers ${formatMoney(semantic.transfers_minor || 0, batch.detected_currency || "GBP")} + Repayments ${formatMoney(semantic.repayments_minor || 0, batch.detected_currency || "GBP")}`; - // Amount sign selector: show when all candidates have positive unsigned amounts + // Amount sign selector: only generic formats may ask the user for semantics renderAmountSignSelector(batch); updateBatchCounts(batch); @@ -527,6 +570,10 @@ function renderAmountSignSelector(batch) { if (!wrap) return; const candidates = batch.candidates || []; + if (batch.adapter_id && batch.adapter_id !== "generic") { + wrap.hidden = true; + return; + } // Show selector when all candidates with amounts are positive (unsigned) const allPositive = candidates.length > 0 && candidates.every((c) => c.amount_minor === null || c.amount_minor === undefined || c.amount_minor >= 0 @@ -558,6 +605,7 @@ function updateBatchCounts(batch) { const candidates = batch.candidates || []; // Check for blocking errors on included candidates + const batchErrors = (batch.issues || []).filter((i) => i.severity === "error"); const blockingErrors = candidates.filter((c) => c.included && c.issues && c.issues.some((i) => i.severity === "error") ); @@ -569,7 +617,10 @@ function updateBatchCounts(batch) { const commitBtn = $("commit-batch-btn"); const noteEl = $("commit-summary-note"); - if (blockingErrors.length > 0) { + if (batchErrors.length > 0) { + commitBtn.disabled = true; + noteEl.textContent = `Blocked: ${batchErrors[0].message}`; + } else if (blockingErrors.length > 0) { commitBtn.disabled = true; const reasons = [...new Set(blockingErrors.flatMap((c) => c.issues.filter((i) => i.severity === "error").map((i) => i.code) diff --git a/src/pfa/web/index.html b/src/pfa/web/index.html index c3d578c..e48b2a6 100644 --- a/src/pfa/web/index.html +++ b/src/pfa/web/index.html @@ -296,12 +296,30 @@

statement.pdf

@@ -321,6 +339,8 @@

statement.pdf

+
+
@@ -395,6 +415,7 @@

Statement Successfully Imported!

View Updated Overview View Activity Ledger +
diff --git a/src/pfa/web/styles.css b/src/pfa/web/styles.css index 2ad6fec..8f07709 100644 --- a/src/pfa/web/styles.css +++ b/src/pfa/web/styles.css @@ -1508,6 +1508,37 @@ button:focus-visible, a:focus-visible, input:focus-visible, select:focus-visible min-width: 180px; } +.new-account-details { + color: var(--muted); + font-size: 11px; +} + +.new-account-details summary { + cursor: pointer; + color: var(--ink-secondary); + font-weight: 600; +} + +.new-account-details[open] .account-input-group { + margin-top: 8px; + flex-wrap: wrap; +} + +.batch-semantic-summary { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: 14px; + padding: 14px 28px; + border-bottom: 1px solid var(--line); + color: var(--muted); + font-size: 12px; +} + +.batch-semantic-summary strong { + color: var(--ink); +} + /* BATCH METRICS BAR & TABS */ .batch-metrics-bar { display: flex; From 56d6720471556d340491ae69dfcf9a8bf8c5c4d5 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:40:01 +0100 Subject: [PATCH 09/24] test(accounts): cover canonical signs and cash coverage --- src/pfa/db/repositories.py | 9 ++++ src/pfa/ingestion/transfers.py | 10 ++++ tests/unit/test_typed_accounts.py | 80 +++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 tests/unit/test_typed_accounts.py diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index bea7066..5b95f1a 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -245,6 +245,15 @@ def suggestions(self) -> list[TransferMatchDecisionModel]: def delete_event(self, event_id: int) -> None: event = self.session.get(TransferEventModel, event_id) if event is not None: + now = datetime.now(UTC).replace(tzinfo=None) + for decision in self.session.scalars( + select(TransferMatchDecisionModel).where( + TransferMatchDecisionModel.event_id == event_id + ) + ): + decision.state = "dismissed" + decision.event_id = None + decision.reviewed_at = now self.session.delete(event) self.session.flush() diff --git a/src/pfa/ingestion/transfers.py b/src/pfa/ingestion/transfers.py index d8e0937..30c8f8f 100644 --- a/src/pfa/ingestion/transfers.py +++ b/src/pfa/ingestion/transfers.py @@ -234,6 +234,9 @@ def create_manual_link( raise BatchError( "TRANSFER_SAME_ACCOUNT", "transfer legs must belong to different accounts", 422 ) + accounts = {account.id: account for account in uow.accounts.all()} + if any(not accounts[account_id].active for account_id in account_ids if account_id in accounts): + raise BatchError("TRANSFER_ACCOUNT_INACTIVE", "all transfer accounts must be active", 422) source_id = next(transaction_id for transaction_id, role in legs if role == "source") destination_id = next(transaction_id for transaction_id, role in legs if role == "destination") if ( @@ -249,6 +252,13 @@ def create_manual_link( ): raise BatchError("TRANSFER_SIGN_INVALID", "the destination leg must be money in", 422) event = TransferEventModel(purpose=purpose, match_method="manual") + for transaction_id, _role in legs: + transaction = transactions[transaction_id] + transaction.kind = TransactionKind.TRANSFER.value + transaction.transfer_purpose = purpose + transaction.category = None + transaction.classification_source = "user" + transaction.classification_reason = "explicit transfer link" uow.transfers.add_event( event, [ diff --git a/tests/unit/test_typed_accounts.py b/tests/unit/test_typed_accounts.py new file mode 100644 index 0000000..fe07ba4 --- /dev/null +++ b/tests/unit/test_typed_accounts.py @@ -0,0 +1,80 @@ +from datetime import date + +from pfa.analytics.service import cash_position +from pfa.db.models import AccountModel, TransactionModel +from pfa.domain.transactions import signed_minor + + +def transaction( + transaction_id: int, + account_id: int, + at: date, + amount_minor: int, + direction: str, + kind: str = "transfer", +) -> TransactionModel: + return TransactionModel( + id=transaction_id, + account_id=account_id, + transaction_date=at, + raw_description="test", + normalized_description="TEST", + amount_minor=amount_minor, + flow_direction=direction, + currency="GBP", + kind=kind, + classification_source="test", + import_source="test", + fingerprint=f"test-{transaction_id}", + ) + + +def test_signed_minor_is_independent_of_account_nature() -> None: + assert signed_minor(1_000, "credit") == 1_000 + assert signed_minor(1_000, "debit") == -1_000 + + +def test_cash_uses_liquid_accounts_and_end_of_day_baselines() -> None: + current = AccountModel( + id=1, + name="Current", + account_type="current", + currency="GBP", + opening_balance_minor=100_000, + opening_balance_as_of=date(2026, 8, 31), + ) + card = AccountModel( + id=2, + name="Card", + account_type="credit_card", + currency="GBP", + opening_balance_minor=50_000, + opening_balance_as_of=date(2026, 8, 31), + ) + rows = [ + transaction(1, 1, date(2026, 8, 31), 10_000, "debit"), + transaction(2, 1, date(2026, 9, 1), 20_000, "debit"), + transaction(3, 2, date(2026, 9, 1), 30_000, "debit", "expense"), + ] + + position = cash_position([current, card], rows, as_of=date(2026, 9, 1)) + + assert position.total_minor == 80_000 + assert position.coverage_status == "complete" + assert position.missing_account_ids == () + + +def test_missing_cash_baseline_is_explicitly_incomplete() -> None: + account = AccountModel( + id=7, + name="Old account", + account_type="current", + currency="GBP", + opening_balance_minor=10_000, + ) + + position = cash_position([account], [], as_of=date(2026, 9, 1)) + + assert position.total_minor is None + assert position.coverage_status == "incomplete" + assert position.missing_account_ids == (7,) From b943295217e4d1291b6b4acaaae03ff88131b874 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:42:45 +0100 Subject: [PATCH 10/24] fix(imports): preserve receipt totals and unlink suppression --- src/pfa/ingestion/batches.py | 31 +++++++++++++++++++++++++++++++ src/pfa/ingestion/candidates.py | 1 + 2 files changed, 32 insertions(+) diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index be8fcec..330bb64 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -37,6 +37,7 @@ BATCH_HAS_BLOCKING_ERRORS, BATCH_NOT_EDITABLE, BATCH_NOT_FOUND, + DUPLICATE_ACCOUNT_SUSPECTED, ERROR, EXTRACTION_FAILED, EXTRACTION_TIMEOUT, @@ -160,6 +161,10 @@ def batch_committed_transaction_ids(batch: ImportBatchModel) -> list[int]: def batch_semantic_totals(batch: ImportBatchModel) -> dict[str, int]: """Calculate preview figures from candidate signs, never from model-generated text.""" + if not batch.candidates_json and batch.reconciliation_json: + saved = json.loads(batch.reconciliation_json).get("semantic_totals") + if isinstance(saved, dict): + return {str(key): int(value) for key, value in saved.items()} spending = refunds = transfers = repayments = money_in = 0 for candidate in batch_candidates(batch): signed = candidate.signed_amount_minor @@ -297,6 +302,7 @@ def _normalize_dates( ACCOUNT_NOT_FOUND, ACCOUNT_REQUIRED, ACCOUNT_TYPE_MISMATCH, + DUPLICATE_ACCOUNT_SUSPECTED, INVALID_ACCOUNT_DRAFT, GENERIC_SIGN_CONFIRMATION_REQUIRED, RECONCILIATION_INCOMPLETE, @@ -371,6 +377,23 @@ def _binding_issues( ) for existing in uow.accounts.by_name(draft.name.strip()): if ( + existing.account_type == account_type.value + and existing.currency.upper() == draft.currency.upper() + and existing.institution + and draft.institution + and existing.institution.upper() == draft.institution.upper() + and existing.last4 + and existing.last4 == draft.last4 + ): + issues.append( + CandidateIssue( + DUPLICATE_ACCOUNT_SUSPECTED, + "an account with the same details already exists; " + "confirm this destination", + WARNING, + ) + ) + elif ( existing.account_type != account_type.value or existing.currency.upper() != draft.currency.upper() ): @@ -804,6 +827,14 @@ def commit_batch(uow: UnitOfWork, batch_id: str, settings: Settings) -> ImportBa from .transfers import match_transfers match_transfers(uow) + if account is not None: + batch.destination_account = account.name + batch.destination_account_id = account.id + batch.new_account_json = None + semantic_totals = batch_semantic_totals(batch) + reconciliation = json.loads(batch.reconciliation_json) if batch.reconciliation_json else {} + reconciliation["semantic_totals"] = semantic_totals + batch.reconciliation_json = json.dumps(reconciliation) batch.status = "committed" batch.committed_at = _now() diff --git a/src/pfa/ingestion/candidates.py b/src/pfa/ingestion/candidates.py index d6d45c1..8ad4845 100644 --- a/src/pfa/ingestion/candidates.py +++ b/src/pfa/ingestion/candidates.py @@ -65,6 +65,7 @@ ACCOUNT_INACTIVE = "ACCOUNT_INACTIVE" ACCOUNT_TYPE_MISMATCH = "ACCOUNT_TYPE_MISMATCH" ACCOUNT_CURRENCY_MISMATCH = "ACCOUNT_CURRENCY_MISMATCH" +DUPLICATE_ACCOUNT_SUSPECTED = "DUPLICATE_ACCOUNT_SUSPECTED" ACCOUNT_REQUIRED = "ACCOUNT_REQUIRED" INVALID_ACCOUNT_DRAFT = "INVALID_ACCOUNT_DRAFT" GENERIC_SIGN_CONFIRMATION_REQUIRED = "GENERIC_SIGN_CONFIRMATION_REQUIRED" From d52a1de5a13fff78d22bf429d1787282c6a89981 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:50:02 +0100 Subject: [PATCH 11/24] feat(imports): add strict HDFC delimited adapter --- src/pfa/ingestion/batches.py | 24 +++- src/pfa/ingestion/dialects.py | 51 ++++++- src/pfa/ingestion/extractors/hdfc.py | 203 +++++++++++++++++++++++++++ 3 files changed, 270 insertions(+), 8 deletions(-) create mode 100644 src/pfa/ingestion/extractors/hdfc.py diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index 330bb64..6ecb20d 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -63,6 +63,7 @@ ) from .dialects import DIALECTS, Dialect, detect_adapter from .extractors.csv import CsvStatementExtractor +from .extractors.hdfc import HdfcDelimitedExtractor from .extractors.ocr import OcrFallbackPdfExtractor from .extractors.pdf import clean_amount_text @@ -210,6 +211,11 @@ def _extractor_for( account_currency: str = "GBP", ) -> StatementExtractor: """Picks only the extraction engine; statement semantics come from content detection.""" + if dialect.adapter_id == "hdfc_in_delimited_v1": + return HdfcDelimitedExtractor( + max_candidate_rows=settings.max_candidate_rows, + dialect=dialect, + ) if source.path.suffix.lower() == ".pdf": return OcrFallbackPdfExtractor( settings=settings, @@ -508,14 +514,14 @@ def create_batch( if account and selected is None: selected = uow.accounts.get_by_name(account) destination_account_id = selected.id if selected is not None else None - account_currency = ( - selected.currency - if selected is not None - else (new_account.currency if new_account else "GBP") - ) detection = detect_adapter(source.path, source.media_type) dialect = detection.dialect + account_currency = ( + detection.suggested_currency + or (selected.currency if selected is not None else None) + or (new_account.currency if new_account else "GBP") + ) extractor = _extractor_for(source, settings, dialect, account_currency=account_currency) batch = ImportBatchModel( @@ -604,7 +610,13 @@ def create_batch( batch.statement_end = max(parsed_dates) batch.detected_account = extraction.detected_account - batch.detected_currency = extraction.detected_currency or account_currency + # HDFC's INR is an adapter suggestion, not evidence read from the file. Keep the + # detected field empty so the UI must ask for confirmation. + batch.detected_currency = ( + extraction.detected_currency + if extraction.detected_currency is not None + else (None if detection.suggested_currency else account_currency) + ) batch.detected_institution = extraction.detected_institution or detection.institution batch.detected_account_hint = extraction.detected_account_hint or detection.account_hint batch.page_count = extraction.page_count diff --git a/src/pfa/ingestion/dialects.py b/src/pfa/ingestion/dialects.py index 597a37e..e82bff5 100644 --- a/src/pfa/ingestion/dialects.py +++ b/src/pfa/ingestion/dialects.py @@ -1,6 +1,8 @@ from __future__ import annotations import csv +import io +import re from dataclasses import dataclass, replace from pathlib import Path @@ -27,6 +29,16 @@ class Dialect: two_column: bool = False compatible_account_types: frozenset[AccountType] = frozenset() institution: str | None = None + suggested_currency: str | None = None + currency_evidence: str | None = None + explicit_source_direction: bool = False + header_signature: tuple[str, ...] = () + + def header_matches(self, cells: list[str]) -> bool: + if not self.header_signature: + return False + normalized = tuple(re.sub(r"\s+", " ", cell.strip()).casefold() for cell in cells) + return normalized == self.header_signature @dataclass(frozen=True, slots=True) @@ -37,7 +49,19 @@ class AdapterDetection: institution: str | None = None account_hint: str | None = None currency: str | None = None - + suggested_currency: str | None = None + currency_evidence: str | None = None + + +HDFC_HEADERS = ( + "date", + "narration", + "value dat", + "debit amount", + "credit amount", + "chq/ref number", + "closing balance", +) GENERIC = Dialect() @@ -86,6 +110,18 @@ class AdapterDetection: compatible_account_types=frozenset({AccountType.CREDIT_CARD}), institution="Barclaycard", ) +HDFC_IN_DELIMITED = replace( + GENERIC, + name="hdfc_in_delimited", + adapter_id="hdfc_in_delimited_v1", + date_formats=("%d/%m/%Y", "%d/%m/%y"), + compatible_account_types=frozenset({AccountType.CURRENT, AccountType.SAVINGS}), + institution="hdfc_bank", + suggested_currency="INR", + currency_evidence="adapter_suggestion", + explicit_source_direction=True, + header_signature=HDFC_HEADERS, +) DIALECTS: dict[str, Dialect] = { "generic": GENERIC, @@ -96,6 +132,7 @@ class AdapterDetection: "hsbc_uk_current": HSBC_UK_CURRENT, "hsbc": HSBC_UK_CURRENT, "barclaycard": BARCLAYCARD, + "hdfc_in_delimited_v1": HDFC_IN_DELIMITED, } @@ -121,9 +158,19 @@ def _csv_detection(path: Path) -> AdapterDetection: text = _csv_text(path) lower = text.lower() try: - header = next(csv.reader(text.splitlines()), []) + reader = csv.reader(io.StringIO(text)) + header = next((row for row in reader if any(cell.strip() for cell in row)), []) except csv.Error: header = [] + if HDFC_IN_DELIMITED.header_matches(header): + return AdapterDetection( + HDFC_IN_DELIMITED, + 0.99, + ("hdfc_delimited_header", "explicit_source_columns"), + institution="hdfc_bank", + suggested_currency="INR", + currency_evidence="adapter_suggestion", + ) headers = {" ".join(cell.strip().lower().split()) for cell in header} if ( "card member" in lower diff --git a/src/pfa/ingestion/extractors/hdfc.py b/src/pfa/ingestion/extractors/hdfc.py new file mode 100644 index 0000000..95dce65 --- /dev/null +++ b/src/pfa/ingestion/extractors/hdfc.py @@ -0,0 +1,203 @@ +"""Strict HDFC India delimited statement extraction. + +HDFC's Delimited export is intentionally kept separate from the permissive generic CSV +reader. The seven-column header is the format contract; a file that does not match it is +not allowed to fall through to positional parsing. +""" + +from __future__ import annotations + +import csv +import re +from collections.abc import Iterable +from decimal import Decimal, InvalidOperation + +from pfa.domain.errors import ValidationError +from pfa.domain.money import minor_units + +from ..candidates import ( + INVALID_AMOUNT, + NO_HEADER_ROW, + TOO_MANY_ROWS, + UNREADABLE_FILE, + CandidateIssue, + CandidateTransaction, + ExtractionResult, + StatementSource, +) +from ..dialects import HDFC_HEADERS, HDFC_IN_DELIMITED, Dialect + +HDFC_HEADER_NOT_FOUND = "HDFC_HEADER_NOT_FOUND" +HDFC_ROW_WIDTH_INVALID = "HDFC_ROW_WIDTH_INVALID" +HDFC_AMOUNT_SIDES_INVALID = "HDFC_AMOUNT_SIDES_INVALID" + + +def _clean_decimal(value: str) -> tuple[Decimal, bool]: + text = value.strip() + negative = text.startswith("-") or text.startswith("−") + if text.startswith(("-", "−")): + text = text[1:].strip() + if text.startswith("(") and text.endswith(")"): + negative = True + text = text[1:-1].strip() + text = re.sub(r"(?i)^(?:inr|rs\.?|₹)", "", text).strip() + text = text.replace(",", "").replace("₹", "").strip() + try: + return Decimal(text), negative + except InvalidOperation as exc: + raise ValueError from exc + + +def _magnitude(value: str) -> int: + decimal, negative = _clean_decimal(value) + if negative or decimal < 0: + raise ValueError + return minor_units(decimal, "INR") + + +def _balance(value: str) -> int: + decimal, negative = _clean_decimal(value) + amount = minor_units(decimal, "INR") + return -amount if negative else amount + + +def _blank_row(cells: Iterable[str]) -> bool: + return not any(cell.strip() for cell in cells) + + +def _candidate( + cells: list[str], line_number: int, dialect: Dialect = HDFC_IN_DELIMITED +) -> CandidateTransaction: + date_text, narration, value_date, debit, credit, reference, closing = cells + raw_fields = { + "value_date": value_date.strip(), + "debit": debit.strip(), + "credit": credit.strip(), + "source_reference": reference.strip(), + "closing_balance": closing.strip(), + } + candidate = CandidateTransaction( + candidate_id=f"h{line_number}", + transaction_date=date_text.strip() or None, + posted_date=value_date.strip() or None, + raw_description=narration.strip(), + currency="INR", + source_format="csv", + source_line=line_number, + extraction_method="hdfc_in_delimited_v1", + raw_fields=raw_fields, + ) + + try: + debit_minor = _magnitude(debit) + credit_minor = _magnitude(credit) + _balance(closing) + except (ValueError, ValidationError): + candidate.add_issue( + INVALID_AMOUNT, + "debit, credit, and closing balance must be valid INR amounts", + ) + return candidate + + candidate.raw_fields["closing_balance_minor"] = str(_balance(closing)) + if (debit_minor > 0) == (credit_minor > 0): + candidate.add_issue( + HDFC_AMOUNT_SIDES_INVALID, + "exactly one of debit amount or credit amount must be positive", + ) + return candidate + + candidate.amount_minor = debit_minor or credit_minor + candidate.direction = "debit" if debit_minor > 0 else "credit" + candidate.direction_explicit = True + # ``direction`` is PFA's legacy normalized money-out/money-in value. The raw + # source columns remain in raw_fields for provenance, while the canonical sign + # is supplied by CandidateTransaction.signed_minor. + return candidate + + +class HdfcDelimitedExtractor: + """Reads only the exact seven-column HDFC Delimited export.""" + + name = "hdfc_in_delimited_v1" + + def __init__(self, *, max_candidate_rows: int = 10_000, dialect: Dialect = HDFC_IN_DELIMITED): + self.max_candidate_rows = max_candidate_rows + self.dialect = dialect + + def extract(self, source: StatementSource) -> ExtractionResult: + result = ExtractionResult(extractor=self.name, detected_institution="hdfc_bank") + try: + text = source.path.read_text(encoding="utf-8-sig") + except UnicodeDecodeError: + result.issues.append( + CandidateIssue(UNREADABLE_FILE, "file is not valid UTF-8 HDFC delimited text") + ) + return result + except OSError: + result.issues.append(CandidateIssue(UNREADABLE_FILE, "could not read the statement")) + return result + + reader = csv.reader(text.splitlines()) + header: list[str] | None = None + try: + for row in reader: + if not _blank_row(row): + header = row + break + except csv.Error: + result.issues.append(CandidateIssue(HDFC_HEADER_NOT_FOUND, "invalid CSV quoting")) + return result + if header is None: + result.issues.append(CandidateIssue(NO_HEADER_ROW, "CSV has no header row")) + return result + if not self.dialect.header_matches(header): + result.issues.append( + CandidateIssue( + HDFC_HEADER_NOT_FOUND, + "HDFC Delimited header was not found; download the Delimited format", + ) + ) + return result + + candidates: list[CandidateTransaction] = [] + try: + for row in reader: + line_number = reader.line_num + if _blank_row(row): + continue + if len(row) != len(HDFC_HEADERS): + candidate = CandidateTransaction( + candidate_id=f"h{line_number}", + source_format="csv", + source_line=line_number, + extraction_method=self.name, + raw_fields={ + "source_reference": row[5].strip() if len(row) > 5 else "" + }, + ) + candidate.add_issue( + HDFC_ROW_WIDTH_INVALID, + "each HDFC Delimited data row must contain seven columns", + ) + else: + candidate = _candidate(row, line_number, self.dialect) + candidates.append(candidate) + if len(candidates) > self.max_candidate_rows: + result.issues.append( + CandidateIssue( + TOO_MANY_ROWS, + f"the statement exceeds the {self.max_candidate_rows}-row limit", + ) + ) + break + except csv.Error: + result.issues.append(CandidateIssue(HDFC_HEADER_NOT_FOUND, "invalid CSV quoting")) + return result + + result.candidates = candidates[: self.max_candidate_rows] + return result + + +def hdfc_delimited_header(cells: list[str]) -> bool: + return HDFC_IN_DELIMITED.header_matches(cells) From 94a6e53a97eac689d4e9031c28b9e3684bde0a3b Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 12:56:50 +0100 Subject: [PATCH 12/24] feat(imports): persist HDFC metadata and reconciliation --- .../0007_adapter_currency_metadata.py | 21 +++ src/pfa/api/app.py | 29 +++- src/pfa/db/models.py | 3 + src/pfa/ingestion/batches.py | 145 ++++++++++++++++-- src/pfa/ingestion/candidates.py | 11 +- src/pfa/ingestion/extractors/pdf.py | 3 +- src/pfa/ingestion/reconciliation.py | 100 ++++++++++-- src/pfa/ingestion/upload.py | 42 ++++- 8 files changed, 325 insertions(+), 29 deletions(-) create mode 100644 alembic/versions/0007_adapter_currency_metadata.py diff --git a/alembic/versions/0007_adapter_currency_metadata.py b/alembic/versions/0007_adapter_currency_metadata.py new file mode 100644 index 0000000..34b003a --- /dev/null +++ b/alembic/versions/0007_adapter_currency_metadata.py @@ -0,0 +1,21 @@ +"""persist generic statement-adapter currency metadata""" + +import sqlalchemy as sa +from alembic import op + +revision = "0007_adapter_currency_metadata" +down_revision = "0006_transfer_events" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("import_batches", sa.Column("suggested_currency", sa.String(length=3))) + op.add_column("import_batches", sa.Column("currency_evidence", sa.String(length=40))) + op.add_column("import_batches", sa.Column("compatible_account_types_json", sa.Text())) + + +def downgrade() -> None: + op.drop_column("import_batches", "compatible_account_types_json") + op.drop_column("import_batches", "currency_evidence") + op.drop_column("import_batches", "suggested_currency") diff --git a/src/pfa/api/app.py b/src/pfa/api/app.py index 6b0bd65..6252d31 100644 --- a/src/pfa/api/app.py +++ b/src/pfa/api/app.py @@ -97,6 +97,11 @@ class NewAccountRequest(BaseModel): last4: str | None = Field(default=None, min_length=4, max_length=4, pattern=r"^\d{4}$") opening_balance_minor: int = 0 opening_balance_as_of: date | None = None + opening_balance_confirmed: bool = False + + +class AccountMetadataUpdateRequest(BaseModel): + institution: Literal["hdfc_bank"] class CandidateIssueResponse(BaseModel): @@ -113,6 +118,7 @@ class CandidateResponse(BaseModel): normalized_description: str amount_minor: int | None direction: str | None + direction_explicit: bool currency: str account_hint: str | None account_id: int | None @@ -148,6 +154,9 @@ class ImportBatchResponse(BaseModel): detection_reason_codes: list[str] detected_institution: str | None detected_account_hint: str | None + suggested_currency: str | None + currency_evidence: str | None + compatible_account_types: list[str] reconciliation: dict[str, object] | None semantic_totals: dict[str, int] amount_sign: str | None @@ -170,6 +179,7 @@ class ImportBatchPatchRequest(BaseModel): account: str | None = None # deprecated label compatibility destination_account_id: int | None = Field(default=None, gt=0) new_account: NewAccountRequest | None = None + account_metadata_update: AccountMetadataUpdateRequest | None = None excluded_candidate_ids: list[str] | None = None @model_validator(mode="after") @@ -177,9 +187,13 @@ def one_binding(self) -> ImportBatchPatchRequest: if self.destination_account_id is not None and self.new_account is not None: raise ValueError("choose destination_account_id or new_account, not both") if self.account is not None and ( - self.destination_account_id is not None or self.new_account is not None + self.destination_account_id is not None + or self.new_account is not None + or self.account_metadata_update is not None ): raise ValueError("account is a legacy alias; use one stable binding") + if self.account_metadata_update is not None and self.destination_account_id is None: + raise ValueError("account_metadata_update requires destination_account_id") return self # Both are closed sets: an unrecognised value is a 422, not a silent no-op. @@ -264,6 +278,7 @@ def _candidate_response(candidate: CandidateTransaction) -> CandidateResponse: normalized_description=candidate.normalized_description, amount_minor=candidate.amount_minor, direction=candidate.direction, + direction_explicit=candidate.direction_explicit, currency=candidate.currency, account_hint=candidate.account_hint, account_id=candidate.account_id, @@ -309,6 +324,13 @@ def _batch_response(batch: ImportBatchModel) -> ImportBatchResponse: ), detected_institution=batch.detected_institution, detected_account_hint=batch.detected_account_hint, + suggested_currency=batch.suggested_currency, + currency_evidence=batch.currency_evidence, + compatible_account_types=( + json.loads(batch.compatible_account_types_json) + if batch.compatible_account_types_json + else [] + ), reconciliation=( json.loads(batch.reconciliation_json) if batch.reconciliation_json else None ), @@ -482,6 +504,11 @@ def patch_import_batch(batch_id: str, request: ImportBatchPatchRequest) -> Impor else None ), excluded_candidate_ids=request.excluded_candidate_ids, + account_metadata_update=( + request.account_metadata_update.model_dump() + if request.account_metadata_update + else None + ), amount_mode=request.amount_mode, amount_sign=request.amount_sign, ), diff --git a/src/pfa/db/models.py b/src/pfa/db/models.py index 5b97f5f..4e0ca1e 100644 --- a/src/pfa/db/models.py +++ b/src/pfa/db/models.py @@ -145,6 +145,9 @@ class ImportBatchModel(Base): detection_reason_codes_json: Mapped[str | None] = mapped_column(Text, nullable=True) detected_institution: Mapped[str | None] = mapped_column(String(120), nullable=True) detected_account_hint: Mapped[str | None] = mapped_column(String(40), nullable=True) + suggested_currency: Mapped[str | None] = mapped_column(String(3), nullable=True) + currency_evidence: Mapped[str | None] = mapped_column(String(40), nullable=True) + compatible_account_types_json: Mapped[str | None] = mapped_column(Text, nullable=True) reconciliation_json: Mapped[str | None] = mapped_column(Text, nullable=True) # The sign convention the user declared for this source, kept so the preview can be # rebuilt after a refresh and so a committed batch records how it read its amounts. diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index 6ecb20d..ee9f0b8 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -28,10 +28,13 @@ from .candidates import ( ACCOUNT_CURRENCY_MISMATCH, ACCOUNT_INACTIVE, + ACCOUNT_INSTITUTION_MISMATCH, + ACCOUNT_INSTITUTION_REQUIRED, ACCOUNT_NOT_FOUND, ACCOUNT_REQUIRED, ACCOUNT_TYPE_MISMATCH, AMBIGUOUS_SIGN, + BALANCE_RECONCILIATION_FAILED, BATCH_ALREADY_COMMITTED, BATCH_EXPIRED, BATCH_HAS_BLOCKING_ERRORS, @@ -43,6 +46,7 @@ EXTRACTION_TIMEOUT, GENERIC_SIGN_CONFIRMATION_REQUIRED, INVALID_ACCOUNT_DRAFT, + INVALID_ACCOUNT_METADATA_UPDATE, NO_USABLE_ROWS, RECONCILIATION_INCOMPLETE, RECONCILIATION_MISMATCH, @@ -87,6 +91,7 @@ class NewAccountDraft: last4: str | None = None opening_balance_minor: int = 0 opening_balance_as_of: date | None = None + opening_balance_confirmed: bool = False def as_dict(self) -> dict[str, object]: return { @@ -99,6 +104,7 @@ def as_dict(self) -> dict[str, object]: "opening_balance_as_of": self.opening_balance_as_of.isoformat() if self.opening_balance_as_of else None, + "opening_balance_confirmed": self.opening_balance_confirmed, } @classmethod @@ -113,6 +119,7 @@ def from_dict(cls, value: dict[str, object]) -> NewAccountDraft: last4=str(value["last4"]) if value.get("last4") else None, opening_balance_minor=int(str(opening)), opening_balance_as_of=date.fromisoformat(str(as_of)) if as_of else None, + opening_balance_confirmed=bool(value.get("opening_balance_confirmed", False)), ) @@ -124,6 +131,7 @@ class BatchPatch: excluded_candidate_ids: list[str] | None = None amount_mode: str | None = None amount_sign: str | None = None + account_metadata_update: dict[str, str] | None = None def _now() -> datetime: @@ -166,11 +174,18 @@ def batch_semantic_totals(batch: ImportBatchModel) -> dict[str, int]: saved = json.loads(batch.reconciliation_json).get("semantic_totals") if isinstance(saved, dict): return {str(key): int(value) for key, value in saved.items()} - spending = refunds = transfers = repayments = money_in = 0 + spending = refunds = transfers = repayments = money_in = money_out = 0 + money_in_count = money_out_count = 0 for candidate in batch_candidates(batch): signed = candidate.signed_amount_minor if signed is None or not candidate.included: continue + if signed > 0: + money_in += signed + money_in_count += 1 + elif signed < 0: + money_out += abs(signed) + money_out_count += 1 description = candidate.raw_description.upper() kind = candidate.kind if kind is None: @@ -193,10 +208,11 @@ def batch_semantic_totals(batch: ImportBatchModel) -> dict[str, int]: "PAYMENT RECEIVED" in description and signed > 0 ): repayments += abs(signed) - elif kind == "income": - money_in += max(signed, 0) return { "money_in_minor": money_in, + "money_out_minor": money_out, + "money_in_count": money_in_count, + "money_out_count": money_out_count, "spending_minor": spending, "refunds_minor": refunds, "transfers_minor": transfers, @@ -308,8 +324,12 @@ def _normalize_dates( ACCOUNT_NOT_FOUND, ACCOUNT_REQUIRED, ACCOUNT_TYPE_MISMATCH, + ACCOUNT_INSTITUTION_MISMATCH, + ACCOUNT_INSTITUTION_REQUIRED, + BALANCE_RECONCILIATION_FAILED, DUPLICATE_ACCOUNT_SUSPECTED, INVALID_ACCOUNT_DRAFT, + INVALID_ACCOUNT_METADATA_UPDATE, GENERIC_SIGN_CONFIRMATION_REQUIRED, RECONCILIATION_INCOMPLETE, RECONCILIATION_MISMATCH, @@ -326,6 +346,20 @@ def _draft_from_batch(batch: ImportBatchModel) -> NewAccountDraft | None: return None +def _institution_key(value: str | None) -> str: + return (value or "").strip().casefold().replace(" ", "_") + + +def _hdfc_opening_suggestion(batch: ImportBatchModel) -> dict[str, object] | None: + if not batch.reconciliation_json: + return None + try: + value = json.loads(batch.reconciliation_json).get("opening_balance_suggestion") + except (TypeError, ValueError, json.JSONDecodeError): + return None + return value if isinstance(value, dict) else None + + def _binding_issues( batch: ImportBatchModel, uow: UnitOfWork, dialect: Dialect ) -> list[CandidateIssue]: @@ -362,6 +396,23 @@ def _binding_issues( issues.append( CandidateIssue(INVALID_ACCOUNT_DRAFT, "account name and currency are invalid") ) + expected_currency = dialect.suggested_currency + if expected_currency and draft.currency.upper() != expected_currency: + issues.append( + CandidateIssue( + ACCOUNT_CURRENCY_MISMATCH, + f"this statement suggests {expected_currency}; confirm that currency", + ) + ) + if dialect.institution and _institution_key(draft.institution) != _institution_key( + dialect.institution + ): + issues.append( + CandidateIssue( + ACCOUNT_INSTITUTION_MISMATCH, + "this statement must be bound to HDFC Bank", + ) + ) if draft.last4 is not None and (len(draft.last4) != 4 or not draft.last4.isdigit()): issues.append( CandidateIssue( @@ -423,15 +474,47 @@ def _binding_issues( f"this statement is for {expected}; selected account is {account.account_type}", ) ) - detected_currency = (batch.detected_currency or "GBP").upper() - if account.currency.upper() != detected_currency: + expected_currency = (dialect.suggested_currency or batch.detected_currency or "GBP").upper() + if account.currency.upper() != expected_currency: issues.append( CandidateIssue( ACCOUNT_CURRENCY_MISMATCH, - f"statement currency {detected_currency} does not match " + f"statement currency {expected_currency} does not match " f"account currency {account.currency}", ) ) + if dialect.institution: + if not account.institution: + issues.append( + CandidateIssue( + ACCOUNT_INSTITUTION_REQUIRED, + "confirm that this legacy account is HDFC Bank before importing", + ) + ) + elif _institution_key(account.institution) != _institution_key(dialect.institution): + issues.append( + CandidateIssue( + ACCOUNT_INSTITUTION_MISMATCH, + "the selected account belongs to a different institution", + ) + ) + if dialect.adapter_id == "hdfc_in_delimited_v1" and draft is not None: + suggestion = _hdfc_opening_suggestion(batch) + if suggestion is not None: + expected_date = suggestion.get("as_of") + expected_minor = suggestion.get("balance_minor") + if ( + not draft.opening_balance_confirmed + or draft.opening_balance_as_of is None + or draft.opening_balance_as_of.isoformat() != expected_date + or draft.opening_balance_minor != expected_minor + ): + issues.append( + CandidateIssue( + INVALID_ACCOUNT_DRAFT, + "confirm the derived opening balance and date before creating this account", + ) + ) if ( batch.adapter_id == "generic" and batch.amount_sign is None @@ -477,12 +560,21 @@ def _set_reconciliation( result = reconcile_candidates(candidates, _reconciliation_account_type(batch, uow)) batch.reconciliation_json = json.dumps(result) if batch.adapter_id not in (None, "generic"): - if result["status"] == "mismatch": - return [CandidateIssue(RECONCILIATION_MISMATCH, "statement balances do not reconcile")] - if result["status"] == "incomplete": - return [ + issues: list[CandidateIssue] = [] + if result["arithmetic_integrity"] == "mismatch": + issues.append( + CandidateIssue( + BALANCE_RECONCILIATION_FAILED + if batch.adapter_id == "hdfc_in_delimited_v1" + else RECONCILIATION_MISMATCH, + "statement balances do not reconcile", + ) + ) + if result["coverage_integrity"] == "incomplete": + issues.append( CandidateIssue(RECONCILIATION_INCOMPLETE, "not every statement row is included") - ] + ) + return issues return [] @@ -542,6 +634,11 @@ def create_batch( detection_reason_codes_json=json.dumps(list(detection.reason_codes)), detected_institution=detection.institution, detected_account_hint=detection.account_hint, + suggested_currency=detection.suggested_currency or dialect.suggested_currency, + currency_evidence=detection.currency_evidence or dialect.currency_evidence, + compatible_account_types_json=json.dumps( + sorted(item.value for item in dialect.compatible_account_types) + ), amount_sign=dialect.default_sign, issues_json="[]", counts_json=json.dumps(_counts([])), @@ -619,6 +716,8 @@ def create_batch( ) batch.detected_institution = extraction.detected_institution or detection.institution batch.detected_account_hint = extraction.detected_account_hint or detection.account_hint + batch.suggested_currency = detection.suggested_currency or dialect.suggested_currency + batch.currency_evidence = detection.currency_evidence or dialect.currency_evidence batch.page_count = extraction.page_count batch.candidates_json = candidates_to_json(candidates) _set_batch_issues(batch, uow, dialect, extraction.issues) @@ -713,6 +812,30 @@ def apply_patch(uow: UnitOfWork, batch_id: str, patch: BatchPatch) -> ImportBatc 422, ) + if patch.account_metadata_update is not None: + update = patch.account_metadata_update + if ( + patch.destination_account_id is None + or batch.adapter_id != "hdfc_in_delimited_v1" + or set(update) != {"institution"} + or _institution_key(update.get("institution")) != "hdfc_bank" + ): + raise BatchError( + INVALID_ACCOUNT_METADATA_UPDATE, + "only a missing legacy institution may be marked as HDFC Bank", + 422, + ) + metadata_account = uow.accounts.get(patch.destination_account_id) + if metadata_account is None: + raise BatchError(ACCOUNT_NOT_FOUND, "select an existing account", 422) + if metadata_account.institution is not None: + raise BatchError( + INVALID_ACCOUNT_METADATA_UPDATE, + "institution correction is allowed only when the account institution is missing", + 422, + ) + metadata_account.institution = "hdfc_bank" + candidates = batch_candidates(batch) if patch.account is not None: diff --git a/src/pfa/ingestion/candidates.py b/src/pfa/ingestion/candidates.py index 8ad4845..5a39644 100644 --- a/src/pfa/ingestion/candidates.py +++ b/src/pfa/ingestion/candidates.py @@ -42,7 +42,9 @@ NO_HEADER_ROW = "NO_HEADER_ROW" HEADERLESS_CSV = "HEADERLESS_CSV" # warning: no header row, columns were read by position UNREADABLE_FILE = "UNREADABLE_FILE" -PDF_ENCRYPTED = "PDF_ENCRYPTED" +PDF_PASSWORD_REQUIRED = "PDF_PASSWORD_REQUIRED" +# Compatibility name retained for callers of the parent import slice. +PDF_ENCRYPTED = PDF_PASSWORD_REQUIRED PDF_TOO_MANY_PAGES = "PDF_TOO_MANY_PAGES" PDF_NOT_EXTRACTABLE = "PDF_NOT_EXTRACTABLE" TOO_MANY_ROWS = "TOO_MANY_ROWS" @@ -50,6 +52,9 @@ # Upload and batch-lifecycle issue codes (T3). UNSUPPORTED_FILE_TYPE = "UNSUPPORTED_FILE_TYPE" +UNSUPPORTED_TEXT_FORMAT = "UNSUPPORTED_TEXT_FORMAT" +UNSUPPORTED_TEXT_LAYOUT = "UNSUPPORTED_TEXT_LAYOUT" +UNSUPPORTED_SPREADSHEET_FORMAT = "UNSUPPORTED_SPREADSHEET_FORMAT" INVALID_SIGNATURE = "INVALID_SIGNATURE" FILE_TOO_LARGE = "FILE_TOO_LARGE" UPLOAD_FAILED = "UPLOAD_FAILED" @@ -65,11 +70,15 @@ ACCOUNT_INACTIVE = "ACCOUNT_INACTIVE" ACCOUNT_TYPE_MISMATCH = "ACCOUNT_TYPE_MISMATCH" ACCOUNT_CURRENCY_MISMATCH = "ACCOUNT_CURRENCY_MISMATCH" +ACCOUNT_INSTITUTION_REQUIRED = "ACCOUNT_INSTITUTION_REQUIRED" +ACCOUNT_INSTITUTION_MISMATCH = "ACCOUNT_INSTITUTION_MISMATCH" +INVALID_ACCOUNT_METADATA_UPDATE = "INVALID_ACCOUNT_METADATA_UPDATE" DUPLICATE_ACCOUNT_SUSPECTED = "DUPLICATE_ACCOUNT_SUSPECTED" ACCOUNT_REQUIRED = "ACCOUNT_REQUIRED" INVALID_ACCOUNT_DRAFT = "INVALID_ACCOUNT_DRAFT" GENERIC_SIGN_CONFIRMATION_REQUIRED = "GENERIC_SIGN_CONFIRMATION_REQUIRED" RECONCILIATION_MISMATCH = "RECONCILIATION_MISMATCH" +BALANCE_RECONCILIATION_FAILED = "BALANCE_RECONCILIATION_FAILED" RECONCILIATION_INCOMPLETE = "RECONCILIATION_INCOMPLETE" UNDO_REQUIRES_CONFIRMATION = "UNDO_REQUIRES_CONFIRMATION" diff --git a/src/pfa/ingestion/extractors/pdf.py b/src/pfa/ingestion/extractors/pdf.py index 1232071..d002608 100644 --- a/src/pfa/ingestion/extractors/pdf.py +++ b/src/pfa/ingestion/extractors/pdf.py @@ -623,7 +623,8 @@ def extract(self, source: StatementSource) -> ExtractionResult: result.issues.append( CandidateIssue( PDF_ENCRYPTED, - "PDF is password-protected; remove the password and re-upload", + "password-protected PDF import is not supported in this release; " + "remove the password and re-upload", ) ) return result diff --git a/src/pfa/ingestion/reconciliation.py b/src/pfa/ingestion/reconciliation.py index 83fa3f0..4f6f26d 100644 --- a/src/pfa/ingestion/reconciliation.py +++ b/src/pfa/ingestion/reconciliation.py @@ -1,5 +1,6 @@ from __future__ import annotations +from datetime import date, timedelta from decimal import Decimal, InvalidOperation from typing import Any @@ -19,19 +20,99 @@ def _balance_minor(value: str, currency: str) -> int | None: return -amount if negative else amount +def _coverage_pass(candidates: list[CandidateTransaction]) -> bool: + return all( + candidate.duplicate_of is not None or (candidate.included and candidate.state != "error") + for candidate in candidates + ) + + +def _hdfc_reconciliation(candidates: list[CandidateTransaction]) -> dict[str, Any]: + """Check HDFC's ordered asset-account closing-balance chain. + + The first row deterministically supplies a *suggested* end-of-day baseline. Every + later row is checked against the previous source closing balance; excluded rows are + still part of the arithmetic check, because removing one cannot repair bad evidence. + """ + coverage_pass = _coverage_pass(candidates) + rows: list[tuple[CandidateTransaction, int]] = [] + for candidate in candidates: + closing = _balance_minor(candidate.raw_fields.get("closing_balance", ""), "INR") + if closing is None or candidate.signed_amount_minor is None: + continue + rows.append((candidate, closing)) + + if len(rows) != len(candidates) or not rows: + return { + "arithmetic_integrity": "not_available", + "coverage_integrity": "pass" if coverage_pass else "incomplete", + "status": "not available" if coverage_pass else "incomplete", + "reconciled": False, + "checked_transition_count": 0, + "mismatch_count": 0, + "source_ordering": "preserved", + "coverage_complete": coverage_pass, + "evidence": "HDFC closing-balance evidence is incomplete", + } + + first, first_closing = rows[0] + try: + first_date = date.fromisoformat(first.transaction_date or "") + except ValueError: + first_date = None + first_signed = first.signed_amount_minor + assert first_signed is not None + opening = first_closing - first_signed + suggestion = { + "balance_minor": opening, + "as_of": (first_date - timedelta(days=1)).isoformat() if first_date else None, + "provenance": "derived_from_first_row", + } + + mismatch_source_rows: list[int] = [] + for (_previous, previous_closing), (current, current_closing) in zip( + rows, rows[1:], strict=False + ): + signed = current.signed_amount_minor + assert signed is not None + if previous_closing + signed != current_closing: + if current.source_line is not None: + mismatch_source_rows.append(current.source_line) + + mismatch_count = len(mismatch_source_rows) + arithmetic_pass = mismatch_count == 0 + coverage = "pass" if coverage_pass else "incomplete" + status = "reconciled" if arithmetic_pass and coverage_pass else "mismatch" + if not coverage_pass: + status = "incomplete" + return { + "arithmetic_integrity": "pass" if arithmetic_pass else "mismatch", + "coverage_integrity": coverage, + "status": status, + "reconciled": arithmetic_pass and coverage_pass, + "checked_transition_count": max(len(rows) - 1, 0), + "mismatch_count": mismatch_count, + "mismatch_source_rows": mismatch_source_rows, + "source_ordering": "preserved", + "coverage_complete": coverage_pass, + "opening_balance_suggestion": suggestion, + "closing_balance_minor": rows[-1][1], + "currency": "INR", + "evidence": ( + f"{max(len(rows) - 1, 0)}/{max(len(rows) - 1, 0)} ordered balance transitions checked" + ), + } + + def reconcile_candidates( candidates: list[CandidateTransaction], account_type: AccountType | str, ) -> dict[str, Any]: - """Reconcile balance-chain evidence without changing the ledger. + """Reconcile balance-chain evidence without changing the ledger.""" + if any("closing_balance" in candidate.raw_fields for candidate in candidates): + return _hdfc_reconciliation(candidates) - A statement balance is a closing balance for its row. The first row therefore gives a - deterministic opening balance by subtracting that row's natural movement. - """ - coverage_pass = all( - candidate.included and (candidate.state != "error" or candidate.duplicate_of is not None) - for candidate in candidates - ) + coverage_pass = _coverage_pass(candidates) rows: list[tuple[CandidateTransaction, int]] = [] for candidate in candidates: if not candidate.included or candidate.duplicate_of is not None: @@ -41,9 +122,6 @@ def reconcile_candidates( continue if candidate.signed_amount_minor is None: continue - movement = candidate.signed_amount_minor - if account_nature(account_type) == "liability": - movement = -movement rows.append((candidate, balance)) if not rows: diff --git a/src/pfa/ingestion/upload.py b/src/pfa/ingestion/upload.py index 92fd409..cea674d 100644 --- a/src/pfa/ingestion/upload.py +++ b/src/pfa/ingestion/upload.py @@ -20,13 +20,17 @@ FILE_TOO_LARGE, INVALID_SIGNATURE, UNSUPPORTED_FILE_TYPE, + UNSUPPORTED_SPREADSHEET_FORMAT, + UNSUPPORTED_TEXT_FORMAT, + UNSUPPORTED_TEXT_LAYOUT, UPLOAD_FAILED, StatementSource, ) +from .dialects import HDFC_IN_DELIMITED, detect_adapter CHUNK_SIZE = 64 * 1024 -SUPPORTED_EXTENSIONS = {".csv", ".pdf"} -DEFAULT_MEDIA_TYPES = {".csv": "text/csv", ".pdf": "application/pdf"} +SUPPORTED_EXTENSIONS = {".csv", ".pdf", ".txt"} +DEFAULT_MEDIA_TYPES = {".csv": "text/csv", ".pdf": "application/pdf", ".txt": "text/plain"} PDF_SIGNATURE = b"%PDF-" REJECTED_MEDIA_PREFIXES = ("image/",) @@ -46,10 +50,15 @@ def stage_upload( original_filename = Path(file.filename or "upload").name ext = Path(original_filename).suffix.lower() + if ext in {".xls", ".xlsx"}: + raise UploadRejected( + UNSUPPORTED_SPREADSHEET_FORMAT, + "Excel statements are not supported; for HDFC, download the Delimited format", + ) if ext not in SUPPORTED_EXTENSIONS: raise UploadRejected( UNSUPPORTED_FILE_TYPE, - f"unsupported file type {ext or '(none)'!r}; only .csv and .pdf are accepted", + f"unsupported file type {ext or '(none)'!r}; only .csv, .txt, and .pdf are accepted", ) media_type = file.content_type or "" # Lowercased: a blocklist that "Image/PNG" walks straight through is not a blocklist. @@ -93,7 +102,32 @@ def stage_upload( head.decode("utf-8-sig") except UnicodeDecodeError as exc: staged_path.unlink(missing_ok=True) - raise UploadRejected(INVALID_SIGNATURE, "file is not valid UTF-8 CSV text") from exc + raise UploadRejected( + INVALID_SIGNATURE, + "file is not valid UTF-8 delimited text", + ) from exc + if ext == ".txt": + detection = detect_adapter(staged_path, media_type) + if detection.dialect is not HDFC_IN_DELIMITED: + text = staged_path.read_text(encoding="utf-8-sig", errors="ignore")[:100_000] + lower = " ".join(text.lower().split()) + formatted_markers = ( + "statement of account" in lower + or "withdrawal amt" in lower + or "deposit amt" in lower + or "chq./ref.no" in lower + or "value dt" in lower and "closing balance" in lower + ) + staged_path.unlink(missing_ok=True) + if formatted_markers: + raise UploadRejected( + UNSUPPORTED_TEXT_LAYOUT, + "HDFC formatted text is not supported; download the Delimited format", + ) + raise UploadRejected( + UNSUPPORTED_TEXT_FORMAT, + "unrecognized text statement; for HDFC, download the Delimited format", + ) return StatementSource( path=staged_path, From 85497437f731641b3dc98980351272beffa75733 Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 13:00:42 +0100 Subject: [PATCH 13/24] test(imports): cover HDFC delimited workflow --- src/pfa/ingestion/extractors/hdfc.py | 7 +- src/pfa/ingestion/reconciliation.py | 9 +- tests/integration/test_hdfc_imports_api.py | 230 +++++++++++++++++++++ tests/unit/test_hdfc_delimited.py | 189 +++++++++++++++++ 4 files changed, 427 insertions(+), 8 deletions(-) create mode 100644 tests/integration/test_hdfc_imports_api.py create mode 100644 tests/unit/test_hdfc_delimited.py diff --git a/src/pfa/ingestion/extractors/hdfc.py b/src/pfa/ingestion/extractors/hdfc.py index 95dce65..1e452a4 100644 --- a/src/pfa/ingestion/extractors/hdfc.py +++ b/src/pfa/ingestion/extractors/hdfc.py @@ -8,6 +8,7 @@ from __future__ import annotations import csv +import io import re from collections.abc import Iterable from decimal import Decimal, InvalidOperation @@ -138,7 +139,7 @@ def extract(self, source: StatementSource) -> ExtractionResult: result.issues.append(CandidateIssue(UNREADABLE_FILE, "could not read the statement")) return result - reader = csv.reader(text.splitlines()) + reader = csv.reader(io.StringIO(text)) header: list[str] | None = None try: for row in reader: @@ -172,9 +173,7 @@ def extract(self, source: StatementSource) -> ExtractionResult: source_format="csv", source_line=line_number, extraction_method=self.name, - raw_fields={ - "source_reference": row[5].strip() if len(row) > 5 else "" - }, + raw_fields={"source_reference": row[5].strip() if len(row) > 5 else ""}, ) candidate.add_issue( HDFC_ROW_WIDTH_INVALID, diff --git a/src/pfa/ingestion/reconciliation.py b/src/pfa/ingestion/reconciliation.py index 4f6f26d..7b34c72 100644 --- a/src/pfa/ingestion/reconciliation.py +++ b/src/pfa/ingestion/reconciliation.py @@ -1,13 +1,14 @@ from __future__ import annotations -from datetime import date, timedelta +from datetime import timedelta from decimal import Decimal, InvalidOperation from typing import Any from pfa.domain.accounts import AccountType, account_nature +from pfa.domain.errors import ImportRowError from pfa.domain.money import minor_units -from .candidates import CandidateTransaction +from .candidates import CandidateTransaction, parse_date from .extractors.pdf import clean_amount_text @@ -57,8 +58,8 @@ def _hdfc_reconciliation(candidates: list[CandidateTransaction]) -> dict[str, An first, first_closing = rows[0] try: - first_date = date.fromisoformat(first.transaction_date or "") - except ValueError: + first_date = parse_date(first.transaction_date or "", "day_first") + except (ImportRowError, ValueError): first_date = None first_signed = first.signed_amount_minor assert first_signed is not None diff --git a/tests/integration/test_hdfc_imports_api.py b/tests/integration/test_hdfc_imports_api.py new file mode 100644 index 0000000..dbbf14e --- /dev/null +++ b/tests/integration/test_hdfc_imports_api.py @@ -0,0 +1,230 @@ +import csv +import io +from pathlib import Path + +from alembic import command +from alembic.config import Config +from fastapi.testclient import TestClient + +from pfa.api.app import create_app +from pfa.config import Settings + +HEADER = [ + "Date", + "Narration", + "Value Dat", + "Debit Amount", + "Credit Amount", + "Chq/Ref Number", + "Closing Balance", +] + + +def settings(tmp_path: Path) -> Settings: + database_url = f"sqlite:///{tmp_path / 'pfa.db'}" + config = Config("alembic.ini") + config.set_main_option("sqlalchemy.url", database_url) + command.upgrade(config, "head") + return Settings(database_url=database_url, upload_dir=tmp_path / "uploads") + + +def statement(*, bad_closing: bool = False) -> bytes: + output = io.StringIO() + writer = csv.writer(output, lineterminator="\n") + writer.writerow(HEADER) + writer.writerows( + [ + ["01/08/2025", "SHOP, ONLINE", "01/08/2025", "1,000.00", "0.00", "R1", "99,000.00"], + [ + "02/08/2025", + "SALARY", + "02/08/2025", + "0.00", + "2,500.00", + "R2", + "102,000.00" if bad_closing else "101,500.00", + ], + ] + ) + return output.getvalue().encode() + + +def upload(client: TestClient, content: bytes, filename: str = "download.txt"): + return client.post( + "/imports/preview", + files={"file": (filename, content, "text/plain")}, + ) + + +def new_account() -> dict[str, object]: + return { + "name": "HDFC Current", + "account_type": "current", + "currency": "INR", + "currency_confirmed": True, + "institution": "hdfc_bank", + "opening_balance_minor": 10000000, + "opening_balance_as_of": "2025-07-31", + "opening_balance_confirmed": True, + } + + +def test_hdfc_txt_preview_persists_metadata_and_commits_to_inr_account(tmp_path) -> None: + config = settings(tmp_path) + with TestClient(create_app(config)) as client: + preview = upload(client, statement()) + assert preview.status_code == 200 + body = preview.json() + assert body["adapter_id"] == "hdfc_in_delimited_v1" + assert body["extractor"] == "hdfc_in_delimited_v1" + assert body["detection_confidence"] == 0.99 + assert body["detected_institution"] == "hdfc_bank" + assert body["detected_currency"] is None + assert body["suggested_currency"] == "INR" + assert body["currency_evidence"] == "adapter_suggestion" + assert body["compatible_account_types"] == ["current", "savings"] + assert body["reconciliation"]["status"] == "reconciled" + assert body["reconciliation"]["checked_transition_count"] == 1 + assert body["reconciliation"]["coverage_complete"] is True + assert body["semantic_totals"]["money_in_count"] == 1 + assert body["semantic_totals"]["money_out_count"] == 1 + assert body["semantic_totals"]["money_out_minor"] == 100000 + assert body["semantic_totals"]["money_in_minor"] == 250000 + assert body["candidates"][0]["raw_description"] == "SHOP, ONLINE" + assert body["candidates"][0]["raw_fields"]["source_reference"] == "R1" + assert body["candidates"][0]["external_id"] is None + assert body["candidates"][0]["signed_amount_minor"] == -100000 + assert body["candidates"][1]["signed_amount_minor"] == 250000 + assert any(issue["code"] == "ACCOUNT_REQUIRED" for issue in body["issues"]) + + batch_id = body["id"] + patched = client.patch(f"/imports/{batch_id}", json={"new_account": new_account()}) + assert patched.status_code == 200 + assert patched.json()["status"] == "preview_ready" + assert patched.json()["issues"] == [] + + committed = client.post(f"/imports/{batch_id}/commit") + assert committed.status_code == 200 + assert committed.json()["status"] == "committed" + assert committed.json()["counts"]["imported"] == 2 + assert committed.json()["semantic_totals"]["money_in_minor"] == 250000 + + account = client.get("/accounts").json()[0] + assert (account["institution"], account["currency"], account["account_type"]) == ( + "hdfc_bank", + "INR", + "current", + ) + transactions = client.get("/transactions").json() + assert {row["signed_amount_minor"] for row in transactions} == {-100000, 250000} + + with TestClient( + create_app(Settings(database_url=config.database_url, upload_dir=config.upload_dir)) + ) as client: + refreshed = client.get(f"/imports/{batch_id}").json() + assert refreshed["suggested_currency"] == "INR" + assert refreshed["detected_currency"] is None + assert refreshed["candidates"] == [] + + +def test_hdfc_csv_and_txt_routes_are_content_equivalent(tmp_path) -> None: + config = settings(tmp_path) + with TestClient(create_app(config)) as client: + txt = upload(client, statement(), "bank-export.txt").json() + csv_body = client.post( + "/imports/preview", + files={"file": ("renamed.csv", statement(), "text/csv")}, + ).json() + + assert txt["adapter_id"] == csv_body["adapter_id"] == "hdfc_in_delimited_v1" + assert txt["detected_currency"] is None + assert txt["candidates"] == csv_body["candidates"] + assert txt["reconciliation"] == csv_body["reconciliation"] + + +def test_hdfc_balance_mismatch_blocks_without_raw_values_in_issue(tmp_path) -> None: + config = settings(tmp_path) + with TestClient(create_app(config)) as client: + body = upload(client, statement(bad_closing=True)).json() + + assert body["status"] == "blocked" + balance_issue = next( + issue for issue in body["issues"] if issue["code"] == "BALANCE_RECONCILIATION_FAILED" + ) + assert balance_issue["severity"] == "error" + assert "102,000" not in balance_issue["message"] + assert body["reconciliation"]["mismatch_source_rows"] == [3] + + +def test_unsupported_hdfc_formats_return_guidance_and_clean_staging(tmp_path) -> None: + config = settings(tmp_path) + with TestClient(create_app(config)) as client: + formatted = client.post( + "/imports/preview", + files={ + "file": ( + "statement.txt", + b"HDFC Bank Statement of Account\n" + b"Date Narration Chq./Ref.No Value Dt Withdrawal Amt " + b"Deposit Amt Closing Balance\n", + "text/plain", + ) + }, + ) + spreadsheet = client.post( + "/imports/preview", + files={"file": ("statement.xls", b"legacy workbook", "application/vnd.ms-excel")}, + ) + unknown = client.post( + "/imports/preview", + files={"file": ("notes.txt", b"not a supported statement", "text/plain")}, + ) + + assert formatted.status_code == 422 + assert formatted.json()["detail"]["code"] == "UNSUPPORTED_TEXT_LAYOUT" + assert "Delimited" in formatted.json()["detail"]["message"] + assert spreadsheet.status_code == 422 + assert spreadsheet.json()["detail"]["code"] == "UNSUPPORTED_SPREADSHEET_FORMAT" + assert unknown.status_code == 422 + assert unknown.json()["detail"]["code"] == "UNSUPPORTED_TEXT_FORMAT" + assert not config.upload_dir.exists() or list(config.upload_dir.iterdir()) == [] + + +def test_legacy_hdfc_account_can_be_marked_inline_atomically(tmp_path) -> None: + config = settings(tmp_path) + with TestClient(create_app(config)) as client: + account = client.post( + "/accounts", + json={"name": "Old HDFC", "account_type": "current", "currency": "INR"}, + ).json() + body = upload(client, statement()).json() + patch = client.patch( + f"/imports/{body['id']}", + json={ + "destination_account_id": account["id"], + "account_metadata_update": {"institution": "hdfc_bank"}, + }, + ) + + assert patch.status_code == 200 + assert patch.json()["status"] == "preview_ready" + assert client.get("/accounts").json()[0]["institution"] == "hdfc_bank" + assert client.post(f"/imports/{body['id']}/commit").status_code == 200 + + +def test_excluding_hdfc_row_makes_coverage_incomplete(tmp_path) -> None: + config = settings(tmp_path) + with TestClient(create_app(config)) as client: + body = upload(client, statement()).json() + excluded = body["candidates"][1]["candidate_id"] + patched = client.patch( + f"/imports/{body['id']}", + json={"new_account": new_account(), "excluded_candidate_ids": [excluded]}, + ) + + assert patched.status_code == 200 + result = patched.json() + assert result["status"] == "blocked" + assert result["reconciliation"]["arithmetic_integrity"] == "pass" + assert result["reconciliation"]["coverage_integrity"] == "incomplete" + assert any(issue["code"] == "RECONCILIATION_INCOMPLETE" for issue in result["issues"]) diff --git a/tests/unit/test_hdfc_delimited.py b/tests/unit/test_hdfc_delimited.py new file mode 100644 index 0000000..e370667 --- /dev/null +++ b/tests/unit/test_hdfc_delimited.py @@ -0,0 +1,189 @@ +import io +from pathlib import Path + +from pfa.ingestion import candidates as codes +from pfa.ingestion.candidates import StatementSource +from pfa.ingestion.dialects import HDFC_IN_DELIMITED, detect_adapter +from pfa.ingestion.extractors.hdfc import HdfcDelimitedExtractor +from pfa.ingestion.reconciliation import reconcile_candidates + +HEADER = [ + "Date", + "Narration", + "Value Dat", + "Debit Amount", + "Credit Amount", + "Chq/Ref Number", + "Closing Balance", +] + + +def extract(path: Path): + return HdfcDelimitedExtractor().extract(StatementSource(path, path.name, "text/plain")) + + +def csv_text(rows: list[list[str]], *, leading_blank: bool = False) -> str: + output = io.StringIO() + if leading_blank: + output.write("\n\n") + import csv + + writer = csv.writer(output, lineterminator="\n") + writer.writerow(HEADER) + writer.writerows(rows) + return output.getvalue() + + +def test_hdfc_header_is_exact_content_detection_and_filename_independent(tmp_path) -> None: + path = tmp_path / "renamed-anything.txt" + path.write_text( + csv_text( + [["01/08/2025", "SHOP, ONLINE", "01/08/2025", "1,000.00", "0.00", "", "9,000.00"]], + leading_blank=True, + ), + encoding="utf-8-sig", + ) + + detection = detect_adapter(path) + result = extract(path) + row = result.candidates[0] + + assert detection.dialect is HDFC_IN_DELIMITED + assert detection.dialect.adapter_id == "hdfc_in_delimited_v1" + assert row.signed_minor == -100_000 + assert row.posted_date == "01/08/2025" + assert row.raw_fields["source_reference"] == "" + assert row.external_id is None + assert row.direction_explicit is True + assert result.issues == [] + + +def test_hdfc_rejects_reordered_and_fuzzy_headers(tmp_path) -> None: + for name, header in ( + ("reordered.txt", HEADER[:1] + HEADER[2:] + HEADER[1:2]), + ("fuzzy.txt", [*HEADER[:2], "Value Date", *HEADER[3:]]), + ): + path = tmp_path / name + path.write_text( + ",".join(header) + "\n01/08/2025,SHOP,01/08/2025,1,0,R,9\n", + encoding="utf-8", + ) + result = extract(path) + assert result.candidates == [] + assert result.issues[0].code == "HDFC_HEADER_NOT_FOUND" + + +def test_hdfc_requires_exactly_one_positive_amount_side(tmp_path) -> None: + path = tmp_path / "sides.txt" + path.write_text( + csv_text( + [ + ["01/08/2025", "ZERO", "01/08/2025", "0.00", "0", "1", "9"], + ["02/08/2025", "BOTH", "02/08/2025", "1", "2", "2", "10"], + ["03/08/2025", "DEBIT", "03/08/2025", "1,234.56", "0.00", "3", "-1,225.56"], + ["04/08/2025", "CREDIT", "04/08/2025", "0", "2,000.00", "4", "774.44"], + ] + ), + encoding="utf-8", + ) + + rows = extract(path).candidates + + assert [row.state for row in rows[:2]] == [codes.ERROR, codes.ERROR] + assert [row.issues[0].code for row in rows[:2]] == [ + "HDFC_AMOUNT_SIDES_INVALID", + "HDFC_AMOUNT_SIDES_INVALID", + ] + assert rows[2].signed_minor == -123_456 + assert rows[3].signed_minor == 200_000 + assert all(row.direction_explicit for row in rows[2:]) + + +def test_hdfc_enforces_seven_columns_and_row_order(tmp_path) -> None: + path = tmp_path / "width.txt" + path.write_text( + csv_text( + [ + ["02/08/2025", "SECOND", "02/08/2025", "1", "0", "2", "8"], + ["03/08/2025", "BROKEN", "03/08/2025", "1", "0"], + ["01/08/2025", "FIRST", "01/08/2025", "0", "2", "1", "10"], + ] + ), + encoding="utf-8", + ) + + result = extract(path) + + assert [row.raw_description for row in result.candidates] == ["SECOND", "", "FIRST"] + assert result.candidates[1].issues[0].code == "HDFC_ROW_WIDTH_INVALID" + assert [row.source_line for row in result.candidates] == [2, 3, 4] + + +def test_hdfc_balance_chain_reports_baseline_and_source_mismatch(tmp_path) -> None: + path = tmp_path / "balances.txt" + path.write_text( + csv_text( + [ + ["01/08/2025", "FIRST", "01/08/2025", "100", "0", "1", "900"], + ["02/08/2025", "SECOND", "02/08/2025", "0", "50", "2", "950"], + ["03/08/2025", "THIRD", "03/08/2025", "25", "0", "3", "900"], + ] + ), + encoding="utf-8", + ) + + rows = extract(path).candidates + reconciliation = reconcile_candidates(rows, "current") + + assert reconciliation["status"] == "mismatch" + assert reconciliation["checked_transition_count"] == 2 + assert reconciliation["mismatch_count"] == 1 + assert reconciliation["mismatch_source_rows"] == [4] + assert reconciliation["opening_balance_suggestion"] == { + "balance_minor": 100_000, + "as_of": "2025-07-31", + "provenance": "derived_from_first_row", + } + assert "25" not in "statement balances do not reconcile" + + +def test_hdfc_coverage_cannot_be_bypassed_by_excluding_a_row(tmp_path) -> None: + path = tmp_path / "coverage.txt" + path.write_text( + csv_text( + [ + ["01/08/2025", "FIRST", "01/08/2025", "100", "0", "1", "900"], + ["02/08/2025", "SECOND", "02/08/2025", "0", "50", "2", "950"], + ] + ), + encoding="utf-8", + ) + rows = extract(path).candidates + rows[1].included = False + + reconciliation = reconcile_candidates(rows, "current") + + assert reconciliation["arithmetic_integrity"] == "pass" + assert reconciliation["coverage_integrity"] == "incomplete" + assert reconciliation["status"] == "incomplete" + + +def test_hdfc_row_limit_is_blocking(tmp_path) -> None: + path = tmp_path / "large.txt" + path.write_text( + csv_text( + [ + [f"{day:02d}/08/2025", "SHOP", f"{day:02d}/08/2025", "1", "0", str(day), "1"] + for day in range(1, 4) + ] + ), + encoding="utf-8", + ) + + result = HdfcDelimitedExtractor(max_candidate_rows=2).extract( + StatementSource(path, path.name, "text/plain") + ) + + assert len(result.candidates) == 2 + assert result.issues[0].code == codes.TOO_MANY_ROWS + assert result.issues[0].severity == codes.ERROR From 5a997e035dc1ef002bc1f93a6b37f97af50bae5d Mon Sep 17 00:00:00 2001 From: Amit Afre Date: Sun, 30 Aug 2026 13:07:06 +0100 Subject: [PATCH 14/24] feat(web): add HDFC account confirmation flow --- src/pfa/api/app.py | 1 + src/pfa/db/repositories.py | 8 ++- src/pfa/ingestion/batches.py | 15 ++++- src/pfa/ingestion/upload.py | 7 ++- src/pfa/web/app.js | 117 ++++++++++++++++++++++++++++++----- src/pfa/web/index.html | 27 +++++++- src/pfa/web/styles.css | 28 +++++++++ 7 files changed, 180 insertions(+), 23 deletions(-) diff --git a/src/pfa/api/app.py b/src/pfa/api/app.py index 6252d31..7643213 100644 --- a/src/pfa/api/app.py +++ b/src/pfa/api/app.py @@ -98,6 +98,7 @@ class NewAccountRequest(BaseModel): opening_balance_minor: int = 0 opening_balance_as_of: date | None = None opening_balance_confirmed: bool = False + currency_confirmed: bool = False class AccountMetadataUpdateRequest(BaseModel): diff --git a/src/pfa/db/repositories.py b/src/pfa/db/repositories.py index 5b95f1a..9600742 100644 --- a/src/pfa/db/repositories.py +++ b/src/pfa/db/repositories.py @@ -103,11 +103,17 @@ def create( raise ValueError(f"unsupported account currency {currency!r}") if last4 is not None and (len(last4) != 4 or not last4.isdigit()): raise ValueError("last4 must contain exactly four digits") + institution_value = institution.strip() if institution else None + if institution_value and institution_value.casefold().replace(" ", "_") in { + "hdfc", + "hdfc_bank", + }: + institution_value = "hdfc_bank" account = AccountModel( name=name.strip(), currency=currency, account_type=account_type, - institution=institution.strip() if institution else None, + institution=institution_value, last4=last4, opening_balance_minor=opening_balance_minor, opening_balance_as_of=opening_balance_as_of, diff --git a/src/pfa/ingestion/batches.py b/src/pfa/ingestion/batches.py index ee9f0b8..d3f0b71 100644 --- a/src/pfa/ingestion/batches.py +++ b/src/pfa/ingestion/batches.py @@ -92,6 +92,7 @@ class NewAccountDraft: opening_balance_minor: int = 0 opening_balance_as_of: date | None = None opening_balance_confirmed: bool = False + currency_confirmed: bool = False def as_dict(self) -> dict[str, object]: return { @@ -105,6 +106,7 @@ def as_dict(self) -> dict[str, object]: if self.opening_balance_as_of else None, "opening_balance_confirmed": self.opening_balance_confirmed, + "currency_confirmed": self.currency_confirmed, } @classmethod @@ -120,6 +122,7 @@ def from_dict(cls, value: dict[str, object]) -> NewAccountDraft: opening_balance_minor=int(str(opening)), opening_balance_as_of=date.fromisoformat(str(as_of)) if as_of else None, opening_balance_confirmed=bool(value.get("opening_balance_confirmed", False)), + currency_confirmed=bool(value.get("currency_confirmed", False)), ) @@ -404,13 +407,20 @@ def _binding_issues( f"this statement suggests {expected_currency}; confirm that currency", ) ) + if expected_currency and not draft.currency_confirmed: + issues.append( + CandidateIssue( + INVALID_ACCOUNT_DRAFT, + f"confirm {expected_currency} as the account currency before importing", + ) + ) if dialect.institution and _institution_key(draft.institution) != _institution_key( dialect.institution ): issues.append( CandidateIssue( ACCOUNT_INSTITUTION_MISMATCH, - "this statement must be bound to HDFC Bank", + "new account institution does not match the statement", ) ) if draft.last4 is not None and (len(draft.last4) != 4 or not draft.last4.isdigit()): @@ -488,7 +498,8 @@ def _binding_issues( issues.append( CandidateIssue( ACCOUNT_INSTITUTION_REQUIRED, - "confirm that this legacy account is HDFC Bank before importing", + "confirm that the selected legacy account belongs to the " + "statement institution", ) ) elif _institution_key(account.institution) != _institution_key(dialect.institution): diff --git a/src/pfa/ingestion/upload.py b/src/pfa/ingestion/upload.py index cea674d..7f03be4 100644 --- a/src/pfa/ingestion/upload.py +++ b/src/pfa/ingestion/upload.py @@ -1,6 +1,7 @@ """Multipart upload staging: bounded, streamed, and signature-checked. -Accepts CSV and PDF. Never trust the client-supplied filename as a path component; +Accepts CSV, UTF-8 delimited text, and PDF. Never trust the client-supplied filename as a +path component; the staged name is always generated. The extension decides which signature check runs and, downstream, which extractor the batch layer selects. """ @@ -99,7 +100,7 @@ def stage_upload( raise UploadRejected(INVALID_SIGNATURE, "file is not a valid PDF") else: try: - head.decode("utf-8-sig") + staged_path.read_text(encoding="utf-8-sig") except UnicodeDecodeError as exc: staged_path.unlink(missing_ok=True) raise UploadRejected( @@ -116,7 +117,7 @@ def stage_upload( or "withdrawal amt" in lower or "deposit amt" in lower or "chq./ref.no" in lower - or "value dt" in lower and "closing balance" in lower + or ("value dt" in lower and "closing balance" in lower) ) staged_path.unlink(missing_ok=True) if formatted_markers: diff --git a/src/pfa/web/app.js b/src/pfa/web/app.js index 913f072..b8b8fda 100644 --- a/src/pfa/web/app.js +++ b/src/pfa/web/app.js @@ -394,9 +394,27 @@ function setupUploadHandlers() { const selected = $("destination-account-select").value; const newName = $("new-account-name").value.trim(); if ((!selected && !newName) || !state.activeBatch) return; + const isHdfc = state.activeBatch.adapter_id === "hdfc_in_delimited_v1"; + const opening = $("new-account-opening-balance")?.value; const body = selected - ? { destination_account_id: Number(selected) } - : { new_account: { name: newName, account_type: $("new-account-type").value, currency: state.activeBatch.detected_currency || "GBP" } }; + ? { + destination_account_id: Number(selected), + ...(isHdfc && $("mark-hdfc-account")?.checked + ? { account_metadata_update: { institution: "hdfc_bank" } } + : {}) + } + : { + new_account: { + name: newName, + account_type: $("new-account-type").value, + currency: isHdfc ? "INR" : (state.activeBatch.detected_currency || "GBP"), + institution: isHdfc ? "hdfc_bank" : null, + currency_confirmed: isHdfc ? $("confirm-account-currency").checked : false, + opening_balance_minor: isHdfc && opening ? Math.round(Number(opening) * 100) : 0, + opening_balance_as_of: isHdfc ? $("new-account-opening-as-of")?.value || null : null, + opening_balance_confirmed: isHdfc ? $("confirm-opening-balance").checked : false + } + }; try { const patched = await apiRequest(`/imports/${state.activeBatch.id}`, { method: "PATCH", @@ -404,7 +422,8 @@ function setupUploadHandlers() { body: JSON.stringify(body) }); state.activeBatch = patched; - showToast(`Assigned account "${acc}" to statement batch.`); + renderBatchInspector(patched); + showToast(`Assigned ${selected ? "the selected account" : `account "${newName}"`} to statement batch.`); } catch (err) { showToast(err.message, true); } @@ -430,6 +449,7 @@ function setupUploadHandlers() { if (!state.activeBatch) return; try { const committed = await apiRequest(`/imports/${state.activeBatch.id}/commit`, { method: "POST" }); + state.activeBatch = committed; $("batch-inspector").hidden = true; $("batch-success-card").hidden = false; $("nav-import-status").hidden = true; @@ -533,24 +553,31 @@ async function handleStatementUpload(file) { function renderBatchInspector(batch) { $("batch-filename").textContent = batch.original_filename; - $("batch-extractor").textContent = batch.extractor || "auto"; - $("batch-currency").textContent = batch.detected_currency || "GBP"; + const isHdfc = batch.adapter_id === "hdfc_in_delimited_v1"; + $("batch-extractor").textContent = isHdfc + ? "HDFC Bank · Delimited · High confidence" + : batch.extractor || "auto"; + const currency = batch.detected_currency || batch.suggested_currency || "GBP"; + $("batch-currency").textContent = isHdfc ? `${currency} — confirm` : currency; $("batch-pages").textContent = batch.page_count || "1"; $("batch-period").textContent = batch.statement_start && batch.statement_end ? `${batch.statement_start} to ${batch.statement_end}` : "Auto-detected"; $("destination-account-select").value = batch.destination_account_id ? String(batch.destination_account_id) : ""; $("new-account-name").value = batch.new_account?.name || ""; - $("new-account-type").value = batch.new_account?.account_type || "current"; + $("new-account-type").value = batch.new_account?.account_type || (isHdfc ? "current" : "current"); + renderHdfcBinding(batch); const semantic = batch.semantic_totals || {}; $("batch-semantic-summary").innerHTML = ` Import effect - Money in ${formatMoney(semantic.money_in_minor || 0, batch.detected_currency || "GBP")} - Spending ${formatMoney(semantic.spending_minor || 0, batch.detected_currency || "GBP")} - Refunds ${formatMoney(semantic.refunds_minor || 0, batch.detected_currency || "GBP")} - Transfers ${formatMoney(semantic.transfers_minor || 0, batch.detected_currency || "GBP")} - Repayments ${formatMoney(semantic.repayments_minor || 0, batch.detected_currency || "GBP")}`; - + ${semantic.money_out_count || 0} money out · ${formatMoney(semantic.money_out_minor || 0, currency)} + ${semantic.money_in_count || 0} money in · ${formatMoney(semantic.money_in_minor || 0, currency)} + Spending ${formatMoney(semantic.spending_minor || 0, currency)} + Refunds ${formatMoney(semantic.refunds_minor || 0, currency)} + Transfers ${formatMoney(semantic.transfers_minor || 0, currency)} + Repayments ${formatMoney(semantic.repayments_minor || 0, currency)}`; + + renderReconciliation(batch); // Amount sign selector: only generic formats may ask the user for semantics renderAmountSignSelector(batch); @@ -559,12 +586,69 @@ function renderBatchInspector(batch) { if (batch.issues && batch.issues.length > 0) { $("batch-issues-alert").hidden = false; - $("batch-issues-content").innerHTML = batch.issues.map((i) => `
${escapeHtml(i.code)}: ${escapeHtml(i.message)}
`).join(""); + $("batch-issues-content").innerHTML = batch.issues.map((i) => `
${escapeHtml(issueLabel(i))}: ${escapeHtml(i.message)}
`).join(""); } else { $("batch-issues-alert").hidden = true; } } +function renderHdfcBinding(batch) { + const isHdfc = batch.adapter_id === "hdfc_in_delimited_v1"; + const fields = $("hdfc-account-fields"); + const correction = $("hdfc-legacy-correction"); + if (!fields || !correction) return; + fields.hidden = !isHdfc; + correction.hidden = !isHdfc || !batch.destination_account_id || + Boolean(state.accounts.find((account) => account.id === batch.destination_account_id)?.institution); + if (!isHdfc) return; + + $("new-account-currency").value = batch.suggested_currency || "INR"; + $("new-account-institution").value = "hdfc_bank"; + const draft = batch.new_account || {}; + $("confirm-account-currency").checked = Boolean(draft.currency_confirmed); + $("confirm-opening-balance").checked = Boolean(draft.opening_balance_confirmed); + $("mark-hdfc-account").checked = false; + const suggestion = batch.reconciliation?.opening_balance_suggestion; + if (suggestion && !batch.new_account) { + $("new-account-opening-balance").value = (Number(suggestion.balance_minor) / 100).toFixed(2); + $("new-account-opening-as-of").value = suggestion.as_of || ""; + } else { + $("new-account-opening-balance").value = draft.opening_balance_minor === undefined ? "" : (Number(draft.opening_balance_minor) / 100).toFixed(2); + $("new-account-opening-as-of").value = draft.opening_balance_as_of || ""; + } + const type = $("new-account-type"); + if (type) { + type.innerHTML = ``; + type.value = draft.account_type || type.value || "current"; + } +} + +function renderReconciliation(batch) { + const target = $("batch-reconciliation"); + const result = batch.reconciliation; + if (!target || !result) return; + const status = result.status || "not available"; + const evidence = result.evidence || "No balance evidence available"; + const transitions = result.checked_transition_count === undefined ? "" : ` · ${result.checked_transition_count} transitions checked`; + target.innerHTML = `Reconciliation: ${escapeHtml(status)}${escapeHtml(evidence)}${escapeHtml(transitions)}`; +} + +function issueLabel(issue) { + const labels = { + ACCOUNT_REQUIRED: "Choose a compatible account", + ACCOUNT_TYPE_MISMATCH: "Account type does not match", + ACCOUNT_CURRENCY_MISMATCH: "Currency confirmation needed", + ACCOUNT_INSTITUTION_REQUIRED: "Confirm this account belongs to HDFC Bank", + ACCOUNT_INSTITUTION_MISMATCH: "The selected account belongs to another institution", + BALANCE_RECONCILIATION_FAILED: "Statement balance check failed", + RECONCILIATION_INCOMPLETE: "All statement rows must be included", + HDFC_AMOUNT_SIDES_INVALID: "Each row needs one money-out or money-in amount", + HDFC_ROW_WIDTH_INVALID: "A statement row has the wrong number of columns", + UNSUPPORTED_TEXT_LAYOUT: "Choose HDFC Delimited when downloading" + }; + return labels[issue.code] || issue.code; +} + function renderAmountSignSelector(batch) { const wrap = $("amount-sign-wrap"); if (!wrap) return; @@ -602,6 +686,7 @@ function updateBatchCounts(batch) { $("count-excluded").textContent = batch.counts.excluded || 0; const validToCommit = (batch.counts.valid || 0); + const duplicateOnlyCommit = (batch.counts.duplicate || 0) > 0 && batch.reconciliation?.status === "reconciled"; const candidates = batch.candidates || []; // Check for blocking errors on included candidates @@ -629,7 +714,7 @@ function updateBatchCounts(batch) { } else if (needsSign) { commitBtn.disabled = true; noteEl.textContent = "Set the amount sign convention before committing"; - } else if (validToCommit === 0) { + } else if (validToCommit === 0 && !duplicateOnlyCommit) { commitBtn.disabled = true; noteEl.textContent = "No valid transactions to commit"; } else { @@ -662,7 +747,7 @@ function renderCandidatesTable() { const isDebit = c.direction === "debit"; const amountStr = formatMoney(c.amount_minor, c.currency); const issues = c.issues || []; - const issueHtml = issues.map((i) => `${escapeHtml(i.code)}`).join(" "); + const issueHtml = issues.map((i) => `${escapeHtml(issueLabel(i))}`).join(" "); const dupHtml = c.duplicate_of ? `Duplicate of #${c.duplicate_of}` : ""; return ` @@ -680,7 +765,7 @@ function renderCandidatesTable() { ${isDebit ? `−${amountStr}` : `+${amountStr}`} - ${escapeHtml(c.extraction_method || "table")} + ${escapeHtml(c.extraction_method === "hdfc_in_delimited_v1" ? "Delimited" : c.extraction_method || "table")} ${issueHtml} ${dupHtml} diff --git a/src/pfa/web/index.html b/src/pfa/web/index.html index e48b2a6..d18dc3a 100644 --- a/src/pfa/web/index.html +++ b/src/pfa/web/index.html @@ -259,7 +259,8 @@

Import Statement

Drag & drop bank statement here

-

Supports PDF Statements, CSV exports, or scanned documents

+

Supports PDF statements, CSV exports, or HDFC Delimited text

+ HDFC customers: choose Delimited when downloading.
` + : `—`; + return ` + + ${escapeHtml(b.original_filename)} + ${escapeHtml(b.destination_account || "—")} + ${escapeHtml(b.status)} + ${importedCount} txs + ${escapeHtml(formattedDate)} + ${undoBtn} + + `; + }).join(""); + + tbody.querySelectorAll("[data-undo-batch-id]").forEach((btn) => { + btn.addEventListener("click", async () => { + const batchId = btn.dataset.undoBatchId; + if (batchId) { + await triggerUndoBatch(batchId); + } + }); + }); + } catch (err) { + tbody.innerHTML = `Failed to load import history: ${escapeHtml(err.message)}`; + } +} + +let lastUploadedFile = null; + +async function handleStatementUpload(file, password = null) { + lastUploadedFile = file; $("upload-progress").hidden = false; + if ($("upload-password-wrap")) $("upload-password-wrap").hidden = true; $("progress-text").textContent = `Parsing ${file.name} (detecting table format & transactions)...`; const formData = new FormData(); formData.append("file", file); + if (password) { + formData.append("password", password); + } try { const batch = await apiRequest("/imports/preview", { @@ -536,8 +713,23 @@ async function handleStatementUpload(file) { body: formData }); + const isEncrypted = (batch.issues || []).some( + (i) => i.code === "PDF_PASSWORD_REQUIRED" || i.code === "PDF_ENCRYPTED" + ); + + if (isEncrypted) { + $("upload-progress").hidden = true; + if ($("upload-password-wrap")) { + $("upload-password-wrap").hidden = false; + $("statement-password-input")?.focus(); + } + showToast("This statement is encrypted with a password. Please enter your password.", true); + return; + } + state.activeBatch = batch; $("upload-progress").hidden = true; + if ($("upload-password-wrap")) $("upload-password-wrap").hidden = true; $("upload-card").hidden = true; $("batch-success-card").hidden = true; $("batch-inspector").hidden = false; @@ -545,12 +737,33 @@ async function handleStatementUpload(file) { renderBatchInspector(batch); showToast(`Parsed ${batch.counts.total} candidates from ${file.name}`); + renderImportHistory(); } catch (err) { $("upload-progress").hidden = true; - showToast(err.message || "Failed to parse statement upload", true); + if (err.data?.detail?.code === "PDF_PASSWORD_REQUIRED" || err.data?.detail?.code === "PDF_ENCRYPTED") { + if ($("upload-password-wrap")) { + $("upload-password-wrap").hidden = false; + $("statement-password-input")?.focus(); + } + showToast("This statement is encrypted with a password. Please enter your password.", true); + } else { + showToast(err.message || "Failed to parse statement upload", true); + } } } +const ACCOUNT_HELP_DEFAULT = "Account names are labels; PFA binds imports by account ID."; + +// The account step lives inside a
. Saying anything there is pointless while it +// is collapsed, so every hint opens it. +function setAccountHint(message, isError) { + const help = $("account-help"); + if (!help) return; + help.textContent = message || ACCOUNT_HELP_DEFAULT; + help.classList.toggle("is-error", Boolean(isError)); + if (message) $("new-account-details").open = true; +} + function renderBatchInspector(batch) { $("batch-filename").textContent = batch.original_filename; const isHdfc = batch.adapter_id === "hdfc_in_delimited_v1"; @@ -566,6 +779,10 @@ function renderBatchInspector(batch) { $("new-account-name").value = batch.new_account?.name || ""; $("new-account-type").value = batch.new_account?.account_type || (isHdfc ? "current" : "current"); renderHdfcBinding(batch); + setAccountHint("", false); + // A required step hidden behind a closed disclosure reads as a broken button. Open it + // whenever this batch cannot be committed without creating an account. + $("new-account-details").open = !batch.destination_account_id && (isHdfc || state.accounts.length === 0); const semantic = batch.semantic_totals || {}; $("batch-semantic-summary").innerHTML = ` @@ -584,6 +801,10 @@ function renderBatchInspector(batch) { updateBatchCounts(batch); renderCandidatesTable(); + renderBatchIssues(batch); +} + +function renderBatchIssues(batch) { if (batch.issues && batch.issues.length > 0) { $("batch-issues-alert").hidden = false; $("batch-issues-content").innerHTML = batch.issues.map((i) => `
${escapeHtml(issueLabel(i))}: ${escapeHtml(i.message)}
`).join(""); @@ -592,18 +813,54 @@ function renderBatchInspector(batch) { } } +function formatInstitutionName(institution) { + if (!institution) return ""; + const lower = institution.toLowerCase(); + if (lower === "hdfc_bank" || lower === "hdfc") return "HDFC Bank"; + if (lower === "amex" || lower === "american express") return "American Express"; + if (lower === "hsbc") return "HSBC"; + return institution; +} + function renderHdfcBinding(batch) { + state.activeBatch = batch; + $("batch-inspector").hidden = false; + $("upload-card").hidden = true; + $("batch-success-card").hidden = true; + + // Header info + $("batch-id-tag").textContent = `#${batch.id}`; + $("batch-filename").textContent = batch.original_filename; + $("batch-meta-info").textContent = `${batch.media_type} · ${_bytes(batch.size_bytes)} · Extractor: ${batch.extractor || "standard"}`; + const isHdfc = batch.adapter_id === "hdfc_in_delimited_v1"; const fields = $("hdfc-account-fields"); const correction = $("hdfc-legacy-correction"); - if (!fields || !correction) return; - fields.hidden = !isHdfc; - correction.hidden = !isHdfc || !batch.destination_account_id || - Boolean(state.accounts.find((account) => account.id === batch.destination_account_id)?.institution); - if (!isHdfc) return; - - $("new-account-currency").value = batch.suggested_currency || "INR"; - $("new-account-institution").value = "hdfc_bank"; + const detectedInst = batch.detected_institution || (isHdfc ? "hdfc_bank" : null); + + if (fields) { + fields.hidden = false; + } + if (correction) { + const selectedAcc = state.accounts.find((account) => account.id === batch.destination_account_id); + correction.hidden = !detectedInst || !batch.destination_account_id || Boolean(selectedAcc?.institution); + const labelEl = $("mark-institution-label"); + if (labelEl && detectedInst) { + labelEl.textContent = `Mark this legacy account as ${formatInstitutionName(detectedInst)}`; + } + } + + const type = $("new-account-type"); + if (type) { + Array.from(type.options).forEach((option) => { + option.hidden = isHdfc && option.value !== "current" && option.value !== "savings"; + }); + } + + const currencyVal = batch.suggested_currency || (isHdfc ? "INR" : (batch.detected_currency || "GBP")); + const instVal = detectedInst || ""; + $("new-account-currency").value = currencyVal; + $("new-account-institution").value = formatInstitutionName(instVal) || instVal; const draft = batch.new_account || {}; $("confirm-account-currency").checked = Boolean(draft.currency_confirmed); $("confirm-opening-balance").checked = Boolean(draft.opening_balance_confirmed); @@ -616,11 +873,7 @@ function renderHdfcBinding(batch) { $("new-account-opening-balance").value = draft.opening_balance_minor === undefined ? "" : (Number(draft.opening_balance_minor) / 100).toFixed(2); $("new-account-opening-as-of").value = draft.opening_balance_as_of || ""; } - const type = $("new-account-type"); - if (type) { - type.innerHTML = ``; - type.value = draft.account_type || type.value || "current"; - } + type.value = draft.account_type || (type.value === "current" || type.value === "savings" ? type.value : "current"); } function renderReconciliation(batch) { @@ -629,16 +882,17 @@ function renderReconciliation(batch) { if (!target || !result) return; const status = result.status || "not available"; const evidence = result.evidence || "No balance evidence available"; - const transitions = result.checked_transition_count === undefined ? "" : ` · ${result.checked_transition_count} transitions checked`; - target.innerHTML = `Reconciliation: ${escapeHtml(status)}${escapeHtml(evidence)}${escapeHtml(transitions)}`; + // The evidence string already names the transition counts; appending them repeated it. + target.innerHTML = `Reconciliation: ${escapeHtml(status)}${escapeHtml(evidence)}`; } function issueLabel(issue) { const labels = { ACCOUNT_REQUIRED: "Choose a compatible account", + INVALID_ACCOUNT_DRAFT: "The new account still needs confirmation", ACCOUNT_TYPE_MISMATCH: "Account type does not match", ACCOUNT_CURRENCY_MISMATCH: "Currency confirmation needed", - ACCOUNT_INSTITUTION_REQUIRED: "Confirm this account belongs to HDFC Bank", + ACCOUNT_INSTITUTION_REQUIRED: "Confirm account belongs to statement institution", ACCOUNT_INSTITUTION_MISMATCH: "The selected account belongs to another institution", BALANCE_RECONCILIATION_FAILED: "Statement balance check failed", RECONCILIATION_INCOMPLETE: "All statement rows must be included", @@ -685,9 +939,13 @@ function updateBatchCounts(batch) { $("count-duplicate").textContent = batch.counts.duplicate || 0; $("count-excluded").textContent = batch.counts.excluded || 0; + const candidates = batch.candidates || []; + const errorCount = candidates.filter((c) => c.issues && c.issues.some((i) => i.severity === "error")).length; + const countErrorEl = $("count-error"); + if (countErrorEl) countErrorEl.textContent = errorCount; + const validToCommit = (batch.counts.valid || 0); const duplicateOnlyCommit = (batch.counts.duplicate || 0) > 0 && batch.reconciliation?.status === "reconciled"; - const candidates = batch.candidates || []; // Check for blocking errors on included candidates const batchErrors = (batch.issues || []).filter((i) => i.severity === "error"); @@ -732,7 +990,8 @@ function renderCandidatesTable() { const filtered = candidates.filter((c) => { if (filter === "all") return true; if (filter === "valid") return c.included && (!c.issues || c.issues.length === 0); - if (filter === "warning") return c.issues && c.issues.length > 0; + if (filter === "warning") return c.issues && c.issues.some((i) => i.severity === "warning"); + if (filter === "error") return c.issues && c.issues.some((i) => i.severity === "error"); if (filter === "duplicate") return c.duplicate_of !== null; if (filter === "excluded") return !c.included; return true; @@ -802,6 +1061,10 @@ async function toggleCandidateInclusion(candidateId, included) { }); state.activeBatch = patched; updateBatchCounts(patched); + // Excluding a row changes reconciliation coverage. Without these the panel kept + // claiming "reconciled" while the server had already flagged RECONCILIATION_INCOMPLETE. + renderReconciliation(patched); + renderBatchIssues(patched); renderCandidatesTable(); } catch (err) { showToast(err.message, true); @@ -819,6 +1082,8 @@ async function bulkToggleCandidates(includeAll) { }); state.activeBatch = patched; updateBatchCounts(patched); + renderReconciliation(patched); + renderBatchIssues(patched); renderCandidatesTable(); showToast(includeAll ? "Included all candidates" : "Excluded all candidates"); } catch (err) { @@ -1034,10 +1299,11 @@ async function submitQuestion(question) { stream.scrollTop = stream.scrollHeight; try { + const curr = state.currency || (state.data[state.month]?.currency) || "GBP"; const res = await apiRequest("/chat", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ message: question }) + body: JSON.stringify({ message: question, currency: curr }) }); loadingEl.remove(); @@ -1149,7 +1415,20 @@ function renderAskView() { // MONTH NAVIGATION MENU function updateMonthMenu() { const menu = $("month-menu"); - const periods = [state.month, monthShift(state.month, -1), monthShift(state.month, -2)]; + const monthSet = new Set(); + if (state.month) monthSet.add(state.month); + (state.transactions || []).forEach((t) => { + if (t.date && t.date.length >= 7) { + monthSet.add(t.date.slice(0, 7)); + } + }); + const now = new Date(); + for (let i = 0; i < 24; i++) { + const d = new Date(now.getFullYear(), now.getMonth() - i, 1); + const mStr = `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2, "0")}`; + monthSet.add(mStr); + } + const periods = Array.from(monthSet).sort().reverse(); menu.innerHTML = periods.map((p) => ` + + @@ -305,7 +315,7 @@

statement.pdf

-