From 949d2df37faece4ed50ea5fd5602e7cdd5b65c46 Mon Sep 17 00:00:00 2001
From: technosexual <92812073+atljh@users.noreply.github.com>
Date: Sun, 30 Aug 2026 02:42:37 +0300
Subject: [PATCH] fix: enforce transfer invariants when updating a transaction
 (#3)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The transfer rules — destination required, destination different from the
source account, no category, and no destination on a non-transfer — were
enforced by TransactionBase's validator on create, but the PATCH route only
re-checked category-vs-type.

So `PATCH /api/transactions/{id} {"type": "transfer"}` on an ordinary
expense committed a transfer row with transfer_account_id = NULL, and only
then failed serializing the response (TransactionRead inherits the same
validator). The caller got a 500 and would reasonably assume nothing was
saved — but from that point on every `GET /api/transactions` failed the same
validation, so the transactions list, the app's main screen, returned 500
permanently, with no way left in the UI to delete the offending row.

The rules now live in one place, transfer_rule_violation(), used by both the
create schema and the update route, so the two paths can't drift apart
again. The update path validates the row as it would look after the patch
rather than only the fields sent, since switching type alone invalidates
fields that were left untouched.

The frontend already sends a complete payload on edit (explicit nulls for
category_id / transfer_account_id) and does these same checks client-side,
so this only closes the hole for direct API calls — no UI change needed.


Claude-Session: https://claude.ai/code/session_01UmuMDFrwdvQkce885VaaTh

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
---
 backend/app/api/routes/transactions.py | 14 ++++++
 backend/app/schemas/transaction.py     | 45 ++++++++++++++---
 backend/tests/test_transactions.py     | 67 ++++++++++++++++++++++++++
 3 files changed, 118 insertions(+), 8 deletions(-)

diff --git a/backend/app/api/routes/transactions.py b/backend/app/api/routes/transactions.py
index fbbb2c3..d8c2990 100644
--- a/backend/app/api/routes/transactions.py
+++ b/backend/app/api/routes/transactions.py
@@ -18,6 +18,7 @@ from app.schemas.transaction import (
     TransactionPage,
     TransactionRead,
     TransactionUpdate,
+    transfer_rule_violation,
 )
 
 router = APIRouter(prefix="/transactions", tags=["transactions"])
@@ -193,9 +194,22 @@ async def update_transaction(
     if transaction is None:
         raise HTTPException(status_code=404, detail="Transaction not found")
     updates = payload.model_dump(exclude_unset=True, exclude={"tag_ids"})
+    # Checks run against the row as it would look after the patch, not just
+    # the fields sent: switching type alone can invalidate fields left
+    # untouched.
     effective_type = updates.get("type", transaction.type)
     effective_category_id = updates.get("category_id", transaction.category_id)
+    effective_account_id = updates.get("account_id", transaction.account_id)
+    effective_transfer_account_id = updates.get("transfer_account_id", transaction.transfer_account_id)
     await _ensure_category_matches_type(session, effective_category_id, effective_type)
+    violation = transfer_rule_violation(
+        type=effective_type,
+        account_id=effective_account_id,
+        transfer_account_id=effective_transfer_account_id,
+        category_id=effective_category_id,
+    )
+    if violation:
+        raise HTTPException(status_code=400, detail=violation)
     for field, value in updates.items():
         setattr(transaction, field, value)
     if payload.tag_ids is not None:
diff --git a/backend/app/schemas/transaction.py b/backend/app/schemas/transaction.py
index 0c29c9c..9194fee 100644
--- a/backend/app/schemas/transaction.py
+++ b/backend/app/schemas/transaction.py
@@ -10,6 +10,35 @@ from app.schemas.category import CategoryRead
 from app.schemas.tag import TagRead
 
 
+def transfer_rule_violation(
+    *,
+    type: TransactionType,
+    account_id: int,
+    transfer_account_id: int | None,
+    category_id: int | None,
+) -> str | None:
+    """The transfer invariants in one place, returning the problem as text or
+    None if the combination is valid.
+
+    Both halves of the write path go through this: the create schema below
+    (as a pydantic validator) and the PATCH route (see
+    routes/transactions.py). Enforcing it on create only used to let an edit
+    produce a row that create would have rejected — most damagingly a
+    transfer with no transfer_account_id, which TransactionRead itself can't
+    serialize, so the row broke every later read of the transactions list.
+    """
+    if type == TransactionType.TRANSFER:
+        if not transfer_account_id:
+            return "transfer_account_id is required for transfer transactions"
+        if transfer_account_id == account_id:
+            return "transfer_account_id must differ from account_id"
+        if category_id:
+            return "category_id is not valid for transfer transactions"
+    elif transfer_account_id:
+        return "transfer_account_id is only valid for transfer transactions"
+    return None
+
+
 class TransactionBase(BaseModel):
     account_id: int
     category_id: int | None = None
@@ -30,14 +59,14 @@ class TransactionBase(BaseModel):
 
     @model_validator(mode="after")
     def _validate_type_specific_fields(self) -> "TransactionBase":
-        if self.type == TransactionType.TRANSFER and not self.transfer_account_id:
-            raise ValueError("transfer_account_id is required for transfer transactions")
-        if self.type == TransactionType.TRANSFER and self.transfer_account_id == self.account_id:
-            raise ValueError("transfer_account_id must differ from account_id")
-        if self.type != TransactionType.TRANSFER and self.transfer_account_id:
-            raise ValueError("transfer_account_id is only valid for transfer transactions")
-        if self.type == TransactionType.TRANSFER and self.category_id:
-            raise ValueError("category_id is not valid for transfer transactions")
+        violation = transfer_rule_violation(
+            type=self.type,
+            account_id=self.account_id,
+            transfer_account_id=self.transfer_account_id,
+            category_id=self.category_id,
+        )
+        if violation:
+            raise ValueError(violation)
         return self
 
 
diff --git a/backend/tests/test_transactions.py b/backend/tests/test_transactions.py
index 5290a7e..445cf0b 100644
--- a/backend/tests/test_transactions.py
+++ b/backend/tests/test_transactions.py
@@ -311,3 +311,70 @@ async def test_years_endpoint_spans_earliest_transaction_through_current_year(
 async def test_years_endpoint_with_no_transactions_returns_only_current_year(client: AsyncClient):
     resp = await client.get("/transactions/years")
     assert resp.json() == [date.today().year]
+
+
+async def test_update_cannot_turn_a_transaction_into_a_transfer_without_a_destination(
+    client: AsyncClient, account_id, categories
+):
+    """The create path rejects a transfer with no transfer_account_id; the
+    update path has to reject it too, or the row ends up moving money out of
+    an account and into nowhere — account_service subtracts the amount from
+    account_id but only credits transfer_account_id when it isn't NULL, so
+    the balance (and Net Worth's cash line, which walks the same rows) just
+    loses it."""
+    created = await client.post(
+        "/transactions", json=_txn(account_id, amount="40.00", category_id=categories["Groceries"]["id"])
+    )
+    txn_id = created.json()["id"]
+    balance_before = money((await client.get("/accounts")).json()[0]["balance"])
+
+    resp = await client.patch(f"/transactions/{txn_id}", json={"type": "transfer", "category_id": None})
+
+    assert resp.status_code == 400
+    assert money((await client.get("/accounts")).json()[0]["balance"]) == balance_before
+
+
+async def test_update_rejects_a_transfer_pointing_at_its_own_account(client: AsyncClient, account_id):
+    """Same rule as on create: a transfer to itself is a no-op row that still
+    shows up in the ledger as a transfer."""
+    other = (await client.post("/accounts", json={"name": "Savings", "type": "savings"})).json()
+    created = await client.post(
+        "/transactions", json=_txn(account_id, type="transfer", transfer_account_id=other["id"], category_id=None)
+    )
+    txn_id = created.json()["id"]
+
+    resp = await client.patch(f"/transactions/{txn_id}", json={"transfer_account_id": account_id})
+
+    assert resp.status_code == 400
+
+
+async def test_update_rejects_a_transfer_destination_on_a_non_transfer(
+    client: AsyncClient, account_id, categories
+):
+    """transfer_account_id on an expense is meaningless — balances only read
+    it for TRANSFER rows — and create already refuses it."""
+    other = (await client.post("/accounts", json={"name": "Savings", "type": "savings"})).json()
+    created = await client.post(
+        "/transactions", json=_txn(account_id, category_id=categories["Groceries"]["id"])
+    )
+
+    resp = await client.patch(
+        f"/transactions/{created.json()['id']}", json={"transfer_account_id": other["id"]}
+    )
+
+    assert resp.status_code == 400
+
+
+async def test_update_rejects_a_category_on_a_transfer(client: AsyncClient, account_id, categories):
+    """A categorized transfer would be counted as spending by the reports that
+    join on category — create rejects it, update must as well."""
+    other = (await client.post("/accounts", json={"name": "Savings", "type": "savings"})).json()
+    created = await client.post(
+        "/transactions", json=_txn(account_id, type="transfer", transfer_account_id=other["id"], category_id=None)
+    )
+
+    resp = await client.patch(
+        f"/transactions/{created.json()['id']}", json={"category_id": categories["Groceries"]["id"]}
+    )
+
+    assert resp.status_code == 400