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