Fix audit findings: budget editing, dead code, logging, multi-currency
- 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 <noreply@anthropic.com>
This commit is contained in:
parent
312594f3d2
commit
8ef3bb2965
9 changed files with 181 additions and 64 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -48,9 +48,6 @@ class TOTPSetupResponse(BaseModel):
|
|||
backup_codes: list[str]
|
||||
|
||||
|
||||
class TOTPVerifyRequest(BaseModel):
|
||||
code: str
|
||||
|
||||
|
||||
class TOTPEnableRequest(BaseModel):
|
||||
secret: str
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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 []
|
||||
|
|
|
|||
|
|
@ -60,6 +60,21 @@ export async function createBudget(data: BudgetCreate): Promise<Budget> {
|
|||
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<Budget> {
|
||||
const r = await api.put(`/budgets/${id}`, data);
|
||||
return r.data;
|
||||
}
|
||||
|
||||
export async function deleteBudget(id: string): Promise<void> {
|
||||
await api.delete(`/budgets/${id}`);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<BudgetCreate>({
|
||||
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<K extends keyof BudgetCreate>(key: K, value: BudgetCreate[K]) {
|
||||
|
|
@ -46,7 +48,7 @@ export default function BudgetFormModal({ categories, onClose, onSubmit, isLoadi
|
|||
<div className="fixed inset-0 z-50 flex items-center justify-center bg-black/50 backdrop-blur-sm p-4">
|
||||
<div className="bg-card border border-border rounded-2xl w-full max-w-md shadow-xl">
|
||||
<div className="flex items-center justify-between p-5 border-b border-border">
|
||||
<h2 className="font-semibold text-lg">New Budget</h2>
|
||||
<h2 className="font-semibold text-lg">{isEdit ? "Edit Budget" : "New Budget"}</h2>
|
||||
<button onClick={onClose} className="p-1.5 rounded-lg hover:bg-secondary text-muted-foreground">
|
||||
<X className="w-4 h-4" />
|
||||
</button>
|
||||
|
|
@ -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}
|
||||
>
|
||||
<option value="">Select category...</option>
|
||||
{expenseCategories.map((c) => (
|
||||
<option key={c.id} value={c.id}>{c.name}</option>
|
||||
))}
|
||||
</select>
|
||||
{isEdit && <p className="text-xs text-muted-foreground mt-1">Category cannot be changed after creation.</p>}
|
||||
</div>
|
||||
|
||||
<div className="grid grid-cols-2 gap-3">
|
||||
|
|
@ -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")}
|
||||
</button>
|
||||
</div>
|
||||
</form>
|
||||
|
|
|
|||
|
|
@ -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<Budget | null>(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<typeof updateBudget>[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"
|
||||
)}
|
||||
>
|
||||
<div className="absolute top-3 right-3 flex gap-1 opacity-0 group-hover:opacity-100 transition-all">
|
||||
<button
|
||||
onClick={() => {
|
||||
const b = budgets.find((b) => b.id === item.budget_id);
|
||||
if (b) setEditing(b);
|
||||
}}
|
||||
className="p-1 rounded text-muted-foreground hover:text-foreground hover:bg-secondary"
|
||||
title="Edit budget"
|
||||
>
|
||||
<Pencil className="w-3.5 h-3.5" />
|
||||
</button>
|
||||
<button
|
||||
onClick={() => deleteMutation.mutate(item.budget_id)}
|
||||
className="absolute top-3 right-3 p-1 rounded text-muted-foreground hover:text-destructive opacity-0 group-hover:opacity-100 transition-all"
|
||||
className="p-1 rounded text-muted-foreground hover:text-destructive hover:bg-destructive/10"
|
||||
title="Delete budget"
|
||||
>
|
||||
<Trash2 className="w-3.5 h-3.5" />
|
||||
</button>
|
||||
</div>
|
||||
|
||||
<div className="flex items-start gap-4">
|
||||
<RadialGauge percent={Number(item.percent_used)} />
|
||||
|
|
@ -192,6 +215,16 @@ export default function BudgetPage() {
|
|||
isLoading={createMutation.isPending}
|
||||
/>
|
||||
)}
|
||||
|
||||
{editing && (
|
||||
<BudgetFormModal
|
||||
categories={categories}
|
||||
budget={editing}
|
||||
onClose={() => setEditing(null)}
|
||||
onSubmit={(data) => updateMutation.mutate({ id: editing.id, data })}
|
||||
isLoading={updateMutation.isPending}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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() {
|
|||
</div>
|
||||
)}
|
||||
|
||||
{/* Transaction history */}
|
||||
{holding && (
|
||||
<div className="bg-card border border-border rounded-xl overflow-hidden">
|
||||
<div className="px-4 py-3 border-b border-border">
|
||||
<p className="text-sm font-medium">Transaction History</p>
|
||||
</div>
|
||||
{transactions.length === 0 ? (
|
||||
<p className="text-sm text-muted-foreground text-center py-8">No transactions recorded</p>
|
||||
) : (
|
||||
<table className="w-full text-sm">
|
||||
<thead>
|
||||
<tr className="text-xs text-muted-foreground uppercase tracking-wider border-b border-border">
|
||||
<th className="text-left px-4 py-2">Date</th>
|
||||
<th className="text-left px-4 py-2">Type</th>
|
||||
<th className="text-right px-4 py-2">Quantity</th>
|
||||
<th className="text-right px-4 py-2">Price</th>
|
||||
<th className="text-right px-4 py-2 hidden sm:table-cell">Fees</th>
|
||||
<th className="text-right px-4 py-2">Total</th>
|
||||
</tr>
|
||||
</thead>
|
||||
<tbody>
|
||||
{[...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 (
|
||||
<tr key={t.id} className="border-b border-border/50 last:border-0 hover:bg-secondary/20 transition-colors">
|
||||
<td className="px-4 py-2.5 tabular-nums text-muted-foreground">{t.date}</td>
|
||||
<td className="px-4 py-2.5">
|
||||
<span className={cn(
|
||||
"inline-flex items-center px-2 py-0.5 rounded text-xs font-medium",
|
||||
isBuy ? "bg-success/15 text-success" :
|
||||
isSell ? "bg-destructive/15 text-destructive" :
|
||||
"bg-secondary text-muted-foreground"
|
||||
)}>
|
||||
{t.type.replace("_", " ")}
|
||||
</span>
|
||||
</td>
|
||||
<td className="px-4 py-2.5 text-right tabular-nums">{Number(t.quantity).toLocaleString()}</td>
|
||||
<td className="px-4 py-2.5 text-right tabular-nums">{formatCurrency(t.price, t.currency)}</td>
|
||||
<td className="px-4 py-2.5 text-right tabular-nums hidden sm:table-cell text-muted-foreground">
|
||||
{t.fees > 0 ? formatCurrency(t.fees, t.currency) : "—"}
|
||||
</td>
|
||||
<td className={cn("px-4 py-2.5 text-right tabular-nums font-medium",
|
||||
isBuy ? "text-destructive" : isSell ? "text-success" : ""
|
||||
)}>
|
||||
{isSell ? "+" : isBuy ? "-" : ""}{formatCurrency(t.total_amount, t.currency)}
|
||||
</td>
|
||||
</tr>
|
||||
);
|
||||
})}
|
||||
</tbody>
|
||||
</table>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Candlestick chart */}
|
||||
<div className="bg-card border border-border rounded-xl p-4">
|
||||
<p className="text-sm font-medium mb-3">Price History (1 Year)</p>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue