fix(bill-modal): correctness + toast fallbacks + validator consolidation (BM1)
- handleBlur now takes the field value explicitly (was positional guessing that fell through to interestRate for unmapped fields). - Three copy-pasted money validators -> one shared validateNonNegativeMoney in client/lib/money.js; expected-amount copy 'positive' -> 'non-negative' (0 ok). - Removed the save action's duplicate due-day/interest-rate re-validation (validateForm already covers it); kept the parses. - Added err.message fallbacks to save/deactivate/verify-autopay toasts. - Save toasts now name the bill. Test: client/lib/money.test.js. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null
parent
e9c5e4d1d3
commit
d92cc38116
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 && (
|
||||
<span className="text-[10px] text-red-500 font-medium">{errors.dueDay}</span>
|
||||
|
|
@ -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 && (
|
||||
<span className="text-[10px] text-red-500 font-medium">{errors.expectedAmount}</span>
|
||||
|
|
@ -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 && (
|
||||
<span className="text-[10px] text-red-500 font-medium">{errors.interestRate}</span>
|
||||
|
|
@ -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 && (
|
||||
<span className="text-[10px] text-red-500 font-medium">{errors.currentBalance}</span>
|
||||
|
|
@ -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 && (
|
||||
<span className="text-[10px] text-red-500 font-medium">{errors.minimumPayment}</span>
|
||||
|
|
|
|||
|
|
@ -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 '';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue