diff --git a/HISTORY.md b/HISTORY.md index d1b99cb..8f8d213 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -11,6 +11,7 @@ ### 🐛 Tracker & bill-modal hardening +- **[Bill modal] Correctness + error/toast + validator cleanup** — several small fixes in `BillModal`: `handleBlur` used positional guessing that defaulted every unmapped field to `interestRate`'s value (latent, masked by inline validators) — now takes the field value explicitly; the three copy-pasted money validators collapsed into one shared `validateNonNegativeMoney(val, label)` in `client/lib/money.js` (the expected-amount message also went from "positive number" to "non-negative", since 0 is allowed); the save action's duplicate due-day/interest-rate re-validation (which re-checked with toasts what `validateForm` already field-validated) was removed; the save/deactivate/verify-autopay `toast.error(err.message)` calls got fallbacks so a missing message never shows "undefined"; and the save toasts now name the bill ("Rent added" / "Rent updated"). Tests: `client/lib/money.test.js` covers the shared validator. (Tracker BM1) - **[Tracker] Route error handling + autopay write atomicity** — `routes/tracker.js` had no try/catch and returned a plain `{error}` shape unlike the rest of the API; its three GET handlers now wrap in try/catch and return `standardizeError` (`{error, message, field, code}`), including the invalid-month validation path. Separately, `applyAutopaySuggestions` (which runs on a `GET /tracker`) inserted the auto-mark-paid payment and applied the balance delta as two un-transactional steps — a mid-way failure could leave a payment without its balance adjustment. Wrapped the INSERT + `applyBalanceDelta` in a single `db.transaction`. Covered by new cases in `tests/trackerService.test.js` (auto-mark creates one payment + drops the balance, idempotent on re-load; route returns the standardized error). (Tracker T2) - **[Tracker] Sidebar overdue badge (and drift/bills) went stale after row actions** — the app never called `queryClient.invalidateQueries`; a Tracker mutation only `refetch()`'d the single tracker query passed down as `refresh`. So after paying/skipping/editing a bill, the sidebar **overdue badge** (`['overdue-count']`, 2-minute staleTime) kept its old number for up to two minutes — you could clear your last overdue bill and still see "3" — and the drift report / bills list didn't update either. Added a `useInvalidateTrackerData()` hook (invalidates `tracker` + `overdue-count` + `drift-report` + `bills`) and wired it in place of the bare `refetch` handed to the rows, `BillModal.onSave`, bank-sync, reorder, and the payment/late-attribution handlers, so the whole shell updates live. (Tracker X3) - **[Notifications] "Reminder days before" was a dead setting — the notifier ignored it** — every bill has a `reminder_days_before` column (default 3) and the bill modal exposed a "Reminder Days" control for subscriptions, but `services/notificationService.js` used a hard-coded schedule (early reminder always at exactly 3 days out) and never read the column. A user who set "remind me 7 days before" still only got the fixed 3-day/1-day/today reminders. The early reminder now fires at the bill's own `reminder_days_before` lead (only when ≥ 2 days, so it never collides with the 1-day/same-day reminders), and the email subject + body say "due in N days" using that value. The lead-time selection was pulled into a pure, exported `reminderTypeFor(bill, diffDays)` so it's unit-tested directly (`tests/notificationLeadTime.test.js`) — default 3 stays backwards-compatible. The **"Reminder Days Before" control now shows for every bill** (not just subscriptions), and saving a non-subscription bill no longer clobbers the column back to 3. (Tracker BM3) diff --git a/client/components/BillModal.jsx b/client/components/BillModal.jsx index d64864b..d3377a7 100644 --- a/client/components/BillModal.jsx +++ b/client/components/BillModal.jsx @@ -1,6 +1,6 @@ import { useActionState, useEffect, useState } from 'react'; import { ChevronDown, Copy, Layers, Link2, Link2Off, Loader2, Pencil, Plus, RefreshCw, Trash2 } from 'lucide-react'; -import { formatCentsUSD } from '@/lib/money'; +import { formatCentsUSD, validateNonNegativeMoney } from '@/lib/money'; import { toast } from 'sonner'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; @@ -248,12 +248,10 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa return ''; }; - const validateExpectedAmount = (val) => { - if (val === '' || val === null) return ''; - const num = parseFloat(val); - if (isNaN(num) || num < 0) return 'Amount must be a positive number'; - return ''; - }; + // Money fields share one non-negative validator (blank allowed, 0 allowed). + const validateExpectedAmount = (val) => validateNonNegativeMoney(val, 'Amount'); + const validateCurrentBalance = (val) => validateNonNegativeMoney(val, 'Balance'); + const validateMinimumPayment = (val) => validateNonNegativeMoney(val, 'Min payment'); const validateInterestRate = (val) => { if (val === '' || val === null) return ''; @@ -263,20 +261,6 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa return ''; }; - const validateCurrentBalance = (val) => { - if (val === '' || val === null) return ''; - const num = parseFloat(val); - if (isNaN(num) || num < 0) return 'Balance must be a non-negative number'; - return ''; - }; - - const validateMinimumPayment = (val) => { - if (val === '' || val === null) return ''; - const num = parseFloat(val); - if (isNaN(num) || num < 0) return 'Min payment must be a non-negative number'; - return ''; - }; - const validateForm = () => { const newErrors = { name: validateName(name), @@ -290,13 +274,11 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa return Object.values(newErrors).every(err => err === ''); }; - const handleBlur = (field, validator) => { - setErrors(prev => ({ ...prev, [field]: validator( - field === 'name' ? name : - field === 'dueDay' ? dueDay : - field === 'expectedAmount' ? expectedAmount : - interestRate - )})); + // Value passed explicitly so this never falls through to the wrong field's + // state (the old positional guessing defaulted every unmapped field to + // interestRate). + const handleBlur = (field, value, validator) => { + setErrors(prev => ({ ...prev, [field]: validator(value) })); }; const handleCategoryChange = (val) => { @@ -325,7 +307,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setLocalVerifiedAt(new Date(res.autopay_verified_at)); toast.success('Autopay marked as verified.'); } catch (err) { - toast.error(err.message); + toast.error(err.message || 'Failed to verify autopay.'); } } @@ -530,18 +512,11 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa return; } + // validateForm() already enforced due-day (1–31) and interest-rate (0–100) + // ranges and blocked the save with field errors, so these are just parses. const parsedDueDay = Number(dueDay); - if (!Number.isInteger(parsedDueDay) || parsedDueDay < 1 || parsedDueDay > 31) { - toast.error('Due day must be a whole number from 1 to 31.'); - return; - } - const trimmedInterestRate = interestRate.trim(); const parsedInterestRate = trimmedInterestRate === '' ? null : Number(trimmedInterestRate); - if (parsedInterestRate !== null && (!Number.isFinite(parsedInterestRate) || parsedInterestRate < 0 || parsedInterestRate > 100)) { - toast.error('Interest rate must be blank or a number from 0 to 100.'); - return; - } const data = { source_bill_id: sourceBill?.source_bill_id, @@ -582,10 +557,10 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa } else { savedBill = await api.createBill(data); } - toast.success('Bill added'); + toast.success(`${data.name} added`); } else { savedBill = await api.updateBill(bill.id, data); - toast.success('Bill updated'); + toast.success(`${data.name} updated`); } if (saveTemplate) { const safeTemplateName = templateName.trim() || data.name; @@ -595,7 +570,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa onSave(savedBill); setDialogOpen(false); } catch (err) { - toast.error(err.message); + toast.error(err.message || 'Failed to save bill.'); } }, null); @@ -609,7 +584,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa onSave?.(); setDialogOpen(false); } catch (err) { - toast.error(err.message); + toast.error(err.message || 'Failed to update bill.'); } } @@ -638,7 +613,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setName(e.target.value); setTimeout(() => setErrors(prev => ({ ...prev, name: validateName(e.target.value) })), 300); }} - onBlur={() => handleBlur('name', validateName)} + onBlur={() => handleBlur('name', name, validateName)} required /> {errors.name && ( @@ -673,7 +648,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setDueDay(e.target.value); setTimeout(() => setErrors(prev => ({ ...prev, dueDay: validateDueDay(e.target.value) })), 300); }} - onBlur={() => handleBlur('dueDay', validateDueDay)} + onBlur={() => handleBlur('dueDay', dueDay, validateDueDay)} /> {errors.dueDay && ( {errors.dueDay} @@ -694,7 +669,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setExpected(e.target.value); setTimeout(() => setErrors(prev => ({ ...prev, expectedAmount: validateExpectedAmount(e.target.value) })), 300); }} - onBlur={() => handleBlur('expectedAmount', validateExpectedAmount)} + onBlur={() => handleBlur('expectedAmount', expectedAmount, validateExpectedAmount)} /> {errors.expectedAmount && ( {errors.expectedAmount} @@ -867,7 +842,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setInterestRate(e.target.value); setTimeout(() => setErrors(prev => ({ ...prev, interestRate: validateInterestRate(e.target.value) })), 300); }} - onBlur={() => handleBlur('interestRate', validateInterestRate)} + onBlur={() => handleBlur('interestRate', interestRate, validateInterestRate)} /> {errors.interestRate && ( {errors.interestRate} @@ -886,7 +861,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setCurrentBalance(e.target.value); setTimeout(() => setErrors(prev => ({ ...prev, currentBalance: validateCurrentBalance(e.target.value) })), 300); }} - onBlur={() => setErrors(prev => ({ ...prev, currentBalance: validateCurrentBalance(currentBalance) }))} + onBlur={() => handleBlur('currentBalance', currentBalance, validateCurrentBalance)} /> {errors.currentBalance && ( {errors.currentBalance} @@ -905,7 +880,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa setMinimumPayment(e.target.value); setTimeout(() => setErrors(prev => ({ ...prev, minimumPayment: validateMinimumPayment(e.target.value) })), 300); }} - onBlur={() => setErrors(prev => ({ ...prev, minimumPayment: validateMinimumPayment(minimumPayment) }))} + onBlur={() => handleBlur('minimumPayment', minimumPayment, validateMinimumPayment)} /> {errors.minimumPayment && ( {errors.minimumPayment} diff --git a/client/lib/money.js b/client/lib/money.js index 5e01599..7f824e3 100644 --- a/client/lib/money.js +++ b/client/lib/money.js @@ -60,3 +60,18 @@ export function formatCentsUSD(cents, { signed = false, dash = false, currency = if (signed) return (c < 0 ? '-' : '+') + body; return (c < 0 ? '-' : '') + body; } + +/** + * Shared form validator for a non-negative money field (dollars). Blank is + * allowed (returns ''); otherwise the value must parse to a number ≥ 0. Returns + * '' when valid, or an error string labelled for the field. Zero is allowed — + * these are non-negative, not strictly-positive, amounts. + * @param {number|string|null} val + * @param {string} [label] — field name for the message (e.g. "Amount", "Balance") + */ +export function validateNonNegativeMoney(val, label = 'Amount') { + if (val === '' || val === null || val === undefined) return ''; + const num = parseFloat(val); + if (isNaN(num) || num < 0) return `${label} must be a non-negative number`; + return ''; +} diff --git a/client/lib/money.test.js b/client/lib/money.test.js index 2605308..c156cc8 100644 --- a/client/lib/money.test.js +++ b/client/lib/money.test.js @@ -1,5 +1,21 @@ import { describe, it, expect } from 'vitest'; -import { formatUSD, formatUSDWhole, formatCentsUSD } from './money'; +import { formatUSD, formatUSDWhole, formatCentsUSD, validateNonNegativeMoney } from './money'; + +describe('validateNonNegativeMoney', () => { + it('allows blank and zero (non-negative, not strictly positive)', () => { + expect(validateNonNegativeMoney('')).toBe(''); + expect(validateNonNegativeMoney(null)).toBe(''); + expect(validateNonNegativeMoney(undefined)).toBe(''); + expect(validateNonNegativeMoney('0')).toBe(''); + expect(validateNonNegativeMoney('12.50')).toBe(''); + }); + + it('rejects negatives and non-numeric with a labelled message', () => { + expect(validateNonNegativeMoney('-1', 'Amount')).toBe('Amount must be a non-negative number'); + expect(validateNonNegativeMoney('abc', 'Balance')).toBe('Balance must be a non-negative number'); + expect(validateNonNegativeMoney('-0.01', 'Min payment')).toBe('Min payment must be a non-negative number'); + }); +}); describe('formatUSD (dollars in)', () => { it('formats to two decimals with grouping', () => {