From 8ef3bb29650f2d7f5a558a81934ec59e244a9ca4 Mon Sep 17 00:00:00 2001 From: megaproxy Date: Thu, 23 Apr 2026 10:54:32 +0000 Subject: [PATCH] Fix audit findings: budget editing, dead code, logging, multi-currency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add budget editing: updateBudget() API, edit button on budget cards, BudgetFormModal adapted for create/update (category locked on edit) - Remove permanently-broken POST /auth/totp/verify stub and its unused TOTPVerifyRequest schema - Wire getHoldingTransactions() to AssetDetail page — transaction history table now shows above the candlestick chart, sorted newest-first - Fix multi-currency net worth in account_service: account balances are now converted to base_currency via ExchangeRate table before summing - Replace silent bare pass exception handlers with logger.warning() in transactions.py (OCR/AI pipeline) and price_feed_service.py (search) — ValueError in date/number regex parsing left silent (control flow) Co-Authored-By: Claude Sonnet 4.6 --- backend/app/api/v1/auth.py | 16 +---- backend/app/api/v1/transactions.py | 25 ++++--- backend/app/schemas/auth.py | 3 - backend/app/services/account_service.py | 21 +++++- backend/app/services/price_feed_service.py | 4 +- frontend/src/api/budgets.ts | 15 +++++ .../src/pages/budgets/BudgetFormModal.tsx | 30 +++++---- frontend/src/pages/budgets/BudgetPage.tsx | 67 ++++++++++++++----- .../src/pages/investments/AssetDetail.tsx | 64 +++++++++++++++++- 9 files changed, 181 insertions(+), 64 deletions(-) diff --git a/backend/app/api/v1/auth.py b/backend/app/api/v1/auth.py index c6bd973..439810c 100644 --- a/backend/app/api/v1/auth.py +++ b/backend/app/api/v1/auth.py @@ -19,7 +19,7 @@ from app.schemas.auth import ( TOTPChallengeResponse, TOTPLoginRequest, TOTPSetupResponse, - TOTPVerifyRequest, + TokenResponse, ) from app.services.auth_service import ( @@ -297,20 +297,6 @@ async def totp_setup( return TOTPSetupResponse(secret=secret, qr_code_png_b64=qr_b64, backup_codes=backup_codes) -@router.post("/totp/verify", status_code=200) -async def totp_verify( - body: TOTPVerifyRequest, - request: Request, - db: AsyncSession = Depends(get_db), - user=Depends(get_current_user), -): - # Secret must be passed back from setup — here we expect it stored temporarily in body - # In practice the client stores it until verification; it's never persisted until verified - # This endpoint receives the secret + verification code - # For simplicity we accept: {"secret": "...", "code": "..."} - # Redefine body inline: - raise HTTPException(status_code=400, detail="Use /totp/enable endpoint with secret and code") - @router.post("/totp/enable", status_code=200) async def totp_enable( diff --git a/backend/app/api/v1/transactions.py b/backend/app/api/v1/transactions.py index 5a931b4..5929479 100644 --- a/backend/app/api/v1/transactions.py +++ b/backend/app/api/v1/transactions.py @@ -1,11 +1,14 @@ import csv import io +import logging import mimetypes import os import uuid from pathlib import Path from typing import Annotated +logger = logging.getLogger(__name__) + from fastapi import APIRouter, Depends, File, Form, HTTPException, Query, UploadFile from fastapi.responses import FileResponse from sqlalchemy.ext.asyncio import AsyncSession @@ -265,8 +268,8 @@ async def delete_attachment( path = Path(settings.upload_dir) / str(user.id) / ref["stored_name"] try: path.unlink(missing_ok=True) - except OSError: - pass + except OSError as e: + logger.warning("Could not delete attachment file %s: %s", path, e) new_refs = [r for r in refs if r["id"] != attachment_id] await db.execute( @@ -307,8 +310,8 @@ def _extract_ocr_text(file_bytes: bytes, mime_type: str) -> str: text = "\n".join(pages_text).strip() if text: return text - except Exception: - pass + except Exception as e: + logger.warning("pdfplumber text extraction failed: %s", e) # Scanned PDF — convert first page to image then OCR try: from pdf2image import convert_from_bytes @@ -316,8 +319,8 @@ def _extract_ocr_text(file_bytes: bytes, mime_type: str) -> str: images = convert_from_bytes(file_bytes, first_page=1, last_page=1, dpi=200) if images: return pytesseract.image_to_string(images[0]) - except Exception: - pass + except Exception as e: + logger.warning("pdf2image/tesseract OCR failed: %s", e) return "" else: import io @@ -326,7 +329,8 @@ def _extract_ocr_text(file_bytes: bytes, mime_type: str) -> str: try: img = Image.open(io.BytesIO(file_bytes)) return pytesseract.image_to_string(img) - except Exception: + except Exception as e: + logger.warning("Image OCR failed: %s", e) return "" @@ -488,11 +492,10 @@ async def _call_ai_parse(file_bytes: bytes, mime_type: str, user_row) -> dict: "ocr_text": ocr_text, } except json.JSONDecodeError: - # AI returned something non-JSON — fall through to rules, keep raw for debug - pass + logger.warning("AI returned non-JSON response, falling back to rule-based parser") - except (httpx.HTTPStatusError, httpx.RequestError): - pass # fall through to rule-based + except (httpx.HTTPStatusError, httpx.RequestError) as e: + logger.warning("AI API request failed (%s), falling back to rule-based parser", type(e).__name__) # Step 3: rule-based fallback (also used when AI is not configured) if ocr_text.strip(): diff --git a/backend/app/schemas/auth.py b/backend/app/schemas/auth.py index 0380f25..3ca03db 100644 --- a/backend/app/schemas/auth.py +++ b/backend/app/schemas/auth.py @@ -48,9 +48,6 @@ class TOTPSetupResponse(BaseModel): backup_codes: list[str] -class TOTPVerifyRequest(BaseModel): - code: str - class TOTPEnableRequest(BaseModel): secret: str diff --git a/backend/app/services/account_service.py b/backend/app/services/account_service.py index d2e4f63..23089e9 100644 --- a/backend/app/services/account_service.py +++ b/backend/app/services/account_service.py @@ -9,6 +9,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.core.security import encrypt_field, decrypt_field from app.db.models.account import Account +from app.db.models.currency import ExchangeRate from app.db.models.transaction import Transaction from app.schemas.account import AccountCreate, AccountUpdate @@ -168,6 +169,19 @@ async def recalculate_balance(db: AsyncSession, account_id: uuid.UUID) -> None: await db.flush() +async def _fx_rate(db: AsyncSession, from_currency: str, to_currency: str) -> Decimal: + if from_currency == to_currency: + return Decimal("1") + result = await db.execute( + select(ExchangeRate) + .where(ExchangeRate.base_currency == from_currency, ExchangeRate.quote_currency == to_currency) + .order_by(ExchangeRate.fetched_at.desc()) + .limit(1) + ) + er = result.scalar_one_or_none() + return er.rate if er else Decimal("1") + + async def get_net_worth(db: AsyncSession, user_id: uuid.UUID, base_currency: str) -> dict: accounts = await db.execute( select(Account).where( @@ -180,8 +194,11 @@ async def get_net_worth(db: AsyncSession, user_id: uuid.UUID, base_currency: str total_liabilities = Decimal("0") for account in accounts.scalars(): - # TODO Phase 3: convert to base_currency via FX rates - bal = account.current_balance + bal = account.current_balance or Decimal("0") + acct_currency = account.currency or base_currency + if acct_currency != base_currency: + rate = await _fx_rate(db, acct_currency, base_currency) + bal = bal * rate if account.type in LIABILITY_TYPES: total_liabilities += abs(bal) else: diff --git a/backend/app/services/price_feed_service.py b/backend/app/services/price_feed_service.py index 161aa13..3841f9a 100644 --- a/backend/app/services/price_feed_service.py +++ b/backend/app/services/price_feed_service.py @@ -118,6 +118,6 @@ def search_yahoo(query: str) -> list[dict]: "data_source_id": None, }) return out - except Exception: - pass + except Exception as exc: + logger.warning("yahoo_search_failed", query=query, error=str(exc)) return [] diff --git a/frontend/src/api/budgets.ts b/frontend/src/api/budgets.ts index 25fc508..bc10ebf 100644 --- a/frontend/src/api/budgets.ts +++ b/frontend/src/api/budgets.ts @@ -60,6 +60,21 @@ export async function createBudget(data: BudgetCreate): Promise { return r.data; } +export interface BudgetUpdate { + name?: string; + amount?: number; + period?: "weekly" | "monthly" | "quarterly" | "yearly"; + end_date?: string | null; + rollover?: boolean; + alert_threshold?: number; + is_active?: boolean; +} + +export async function updateBudget(id: string, data: BudgetUpdate): Promise { + const r = await api.put(`/budgets/${id}`, data); + return r.data; +} + export async function deleteBudget(id: string): Promise { await api.delete(`/budgets/${id}`); } diff --git a/frontend/src/pages/budgets/BudgetFormModal.tsx b/frontend/src/pages/budgets/BudgetFormModal.tsx index 9997860..f8171f5 100644 --- a/frontend/src/pages/budgets/BudgetFormModal.tsx +++ b/frontend/src/pages/budgets/BudgetFormModal.tsx @@ -1,6 +1,6 @@ import { useState } from "react"; import { X } from "lucide-react"; -import { BudgetCreate } from "@/api/budgets"; +import type { Budget, BudgetCreate } from "@/api/budgets"; import { format } from "date-fns"; interface Category { @@ -14,22 +14,24 @@ interface Props { onClose: () => void; onSubmit: (data: BudgetCreate) => void; isLoading: boolean; + budget?: Budget; } -export default function BudgetFormModal({ categories, onClose, onSubmit, isLoading }: Props) { +export default function BudgetFormModal({ categories, onClose, onSubmit, isLoading, budget }: Props) { const today = format(new Date(), "yyyy-MM-dd"); const [form, setForm] = useState({ - category_id: "", - name: "", - amount: 0, - currency: "GBP", - period: "monthly", - start_date: today, - end_date: null, - rollover: false, - alert_threshold: 80, + category_id: budget?.category_id ?? "", + name: budget?.name ?? "", + amount: budget?.amount ?? 0, + currency: budget?.currency ?? "GBP", + period: (budget?.period as BudgetCreate["period"]) ?? "monthly", + start_date: budget?.start_date ?? today, + end_date: budget?.end_date ?? null, + rollover: budget?.rollover ?? false, + alert_threshold: budget?.alert_threshold ?? 80, }); + const isEdit = !!budget; const expenseCategories = categories.filter((c) => c.type === "expense" || c.type === "system"); function set(key: K, value: BudgetCreate[K]) { @@ -46,7 +48,7 @@ export default function BudgetFormModal({ categories, onClose, onSubmit, isLoadi
-

New Budget

+

{isEdit ? "Edit Budget" : "New Budget"}

@@ -71,12 +73,14 @@ export default function BudgetFormModal({ categories, onClose, onSubmit, isLoadi onChange={(e) => set("category_id", e.target.value)} className="w-full rounded-md border border-input bg-background px-3 py-2 text-sm focus:outline-none focus:ring-2 focus:ring-ring" required + disabled={isEdit} > {expenseCategories.map((c) => ( ))} + {isEdit &&

Category cannot be changed after creation.

}
@@ -154,7 +158,7 @@ export default function BudgetFormModal({ categories, onClose, onSubmit, isLoadi disabled={isLoading} className="flex-1 py-2.5 rounded-lg bg-primary text-primary-foreground text-sm font-medium hover:bg-primary/90 disabled:opacity-50 transition-colors" > - {isLoading ? "Creating..." : "Create Budget"} + {isLoading ? (isEdit ? "Saving..." : "Creating...") : (isEdit ? "Save changes" : "Create Budget")}
diff --git a/frontend/src/pages/budgets/BudgetPage.tsx b/frontend/src/pages/budgets/BudgetPage.tsx index 7a25553..3bf6d33 100644 --- a/frontend/src/pages/budgets/BudgetPage.tsx +++ b/frontend/src/pages/budgets/BudgetPage.tsx @@ -1,10 +1,10 @@ import { useState } from "react"; import { useQuery, useMutation, useQueryClient } from "@tanstack/react-query"; -import { getBudgetSummary, createBudget, deleteBudget } from "@/api/budgets"; +import { getBudgets, getBudgetSummary, createBudget, updateBudget, deleteBudget, type Budget } from "@/api/budgets"; import { getCategories } from "@/api/transactions"; import { formatCurrency } from "@/utils/currency"; import { cn } from "@/utils/cn"; -import { Plus, Trash2, AlertTriangle, CheckCircle } from "lucide-react"; +import { Plus, Trash2, Pencil, AlertTriangle, CheckCircle } from "lucide-react"; import BudgetFormModal from "./BudgetFormModal"; function RadialGauge({ percent, size = 80 }: { percent: number; size?: number }) { @@ -55,6 +55,7 @@ function RadialGauge({ percent, size = 80 }: { percent: number; size?: number }) export default function BudgetPage() { const qc = useQueryClient(); const [showForm, setShowForm] = useState(false); + const [editing, setEditing] = useState(null); const { data: summary = [], isLoading } = useQuery({ queryKey: ["budget-summary"], @@ -62,23 +63,32 @@ export default function BudgetPage() { refetchInterval: 60_000, }); + const { data: budgets = [] } = useQuery({ + queryKey: ["budgets"], + queryFn: () => getBudgets(false), + }); + const { data: categories = [] } = useQuery({ queryKey: ["categories"], queryFn: getCategories }); + const invalidate = () => { + qc.invalidateQueries({ queryKey: ["budget-summary"] }); + qc.invalidateQueries({ queryKey: ["budgets"] }); + }; + const createMutation = useMutation({ mutationFn: createBudget, - onSuccess: () => { - qc.invalidateQueries({ queryKey: ["budget-summary"] }); - qc.invalidateQueries({ queryKey: ["budgets"] }); - setShowForm(false); - }, + onSuccess: () => { invalidate(); setShowForm(false); }, + }); + + const updateMutation = useMutation({ + mutationFn: ({ id, data }: { id: string; data: Parameters[1] }) => + updateBudget(id, data), + onSuccess: () => { invalidate(); setEditing(null); }, }); const deleteMutation = useMutation({ mutationFn: deleteBudget, - onSuccess: () => { - qc.invalidateQueries({ queryKey: ["budget-summary"] }); - qc.invalidateQueries({ queryKey: ["budgets"] }); - }, + onSuccess: invalidate, }); const overBudget = summary.filter((s) => s.is_over_budget).length; @@ -133,12 +143,25 @@ export default function BudgetPage() { : "border-border" )} > - +
+ + +
@@ -192,6 +215,16 @@ export default function BudgetPage() { isLoading={createMutation.isPending} /> )} + + {editing && ( + setEditing(null)} + onSubmit={(data) => updateMutation.mutate({ id: editing.id, data })} + isLoading={updateMutation.isPending} + /> + )}
); } diff --git a/frontend/src/pages/investments/AssetDetail.tsx b/frontend/src/pages/investments/AssetDetail.tsx index 20cf6c7..9898267 100644 --- a/frontend/src/pages/investments/AssetDetail.tsx +++ b/frontend/src/pages/investments/AssetDetail.tsx @@ -1,6 +1,6 @@ import { useParams, Link } from "react-router-dom"; import { useQuery } from "@tanstack/react-query"; -import { getPriceHistory, getPortfolio } from "@/api/investments"; +import { getPriceHistory, getPortfolio, getHoldingTransactions } from "@/api/investments"; import { formatCurrency } from "@/utils/currency"; import { useUiStore } from "@/store/uiStore"; import { cn } from "@/utils/cn"; @@ -20,6 +20,12 @@ export default function AssetDetail() { enabled: !!assetId, }); + const { data: transactions = [] } = useQuery({ + queryKey: ["holding-transactions", holding?.id], + queryFn: () => getHoldingTransactions(holding!.id), + enabled: !!holding?.id, + }); + const dates = prices.map(p => p.date); const opens = prices.map(p => p.open ?? p.close); const highs = prices.map(p => p.high ?? p.close); @@ -73,6 +79,62 @@ export default function AssetDetail() {
)} + {/* Transaction history */} + {holding && ( +
+
+

Transaction History

+
+ {transactions.length === 0 ? ( +

No transactions recorded

+ ) : ( + + + + + + + + + + + + + {[...transactions].sort((a, b) => b.date.localeCompare(a.date)).map((t) => { + const isBuy = t.type === "buy" || t.type === "transfer_in"; + const isSell = t.type === "sell" || t.type === "transfer_out"; + return ( + + + + + + + + + ); + })} + +
DateTypeQuantityPriceFeesTotal
{t.date} + + {t.type.replace("_", " ")} + + {Number(t.quantity).toLocaleString()}{formatCurrency(t.price, t.currency)} + {t.fees > 0 ? formatCurrency(t.fees, t.currency) : "—"} + + {isSell ? "+" : isBuy ? "-" : ""}{formatCurrency(t.total_amount, t.currency)} +
+ )} +
+ )} + {/* Candlestick chart */}

Price History (1 Year)