fix(notifications): honor per-bill reminder_days_before + expose for all bills (BM3)

The notifier used a hard-coded 3-day early reminder and never read
reminder_days_before, so the modal's 'Reminder Days' control was a no-op.
The early reminder now fires at the bill's own lead (>= 2 days so it never
collides with the 1-day/same-day reminders); email subject+body say 'due in
N days'. Lead-time selection extracted to a pure exported reminderTypeFor()
for unit testing. The Reminder Days control now shows for every bill and a
non-subscription save no longer clobbers the column to 3.

Test: tests/notificationLeadTime.test.js

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null 2026-07-03 18:16:10 -05:00
parent ad1f5bebf6
commit 10e159352a
4 changed files with 103 additions and 23 deletions

View File

@ -3,6 +3,7 @@
### 🐛 Tracker & bill-modal hardening ### 🐛 Tracker & bill-modal hardening
- **[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)
- **[Bill modal/SimpleFIN] Importing bank payments didn't refresh the payment list or the Tracker** — the two flows in the bill modal that *create* payments — **Sync** (`syncBillSimplefinPayments`) and a **merchant-rule historical import** (`onRulesChanged` → `importHistoricalPayments`) — only reloaded the linked-transactions list, unlike the unmatch handlers which correctly reload payments *and* linked transactions *and* call `onSave`. So after importing, say, 3 payments from bank history, the modal's Payment History stayed stale and the Tracker row behind it kept showing "due/overdue" even though the bill was now covered — until you closed and reopened. Both paths now `await Promise.all([loadPayments(), loadLinkedTransactions()])` then `onSave?.()`, matching the unmatch pattern, so imported payments appear immediately and the Tracker updates live. (The SimpleFIN *search/preview/candidate* flow was already correct.) (Tracker BM4) - **[Bill modal/SimpleFIN] Importing bank payments didn't refresh the payment list or the Tracker** — the two flows in the bill modal that *create* payments — **Sync** (`syncBillSimplefinPayments`) and a **merchant-rule historical import** (`onRulesChanged` → `importHistoricalPayments`) — only reloaded the linked-transactions list, unlike the unmatch handlers which correctly reload payments *and* linked transactions *and* call `onSave`. So after importing, say, 3 payments from bank history, the modal's Payment History stayed stale and the Tracker row behind it kept showing "due/overdue" even though the bill was now covered — until you closed and reopened. Both paths now `await Promise.all([loadPayments(), loadLinkedTransactions()])` then `onSave?.()`, matching the unmatch pattern, so imported payments appear immediately and the Tracker updates live. (The SimpleFIN *search/preview/candidate* flow was already correct.) (Tracker BM4)
- **[Tracker/SimpleFIN] Bank card's "unpaid this month" and "remaining" over-counted off-month bills** — `buildBankTracking` (`services/trackerService.js`) summed `expected_amount` for *all* active unpaid bills via SQL with no occurrence gate, so an annual or off-month quarterly bill inflated `unpaid_this_month` (and therefore the bank `remaining`) even though the Tracker rows beside it correctly excluded it — the same class of bug as QA-B5-02, still live on the bank path. `getTracker` now derives the unpaid total from the already-gated rows (via `resolveDueDate`), netting partial payments, and passes it into `buildBankTracking`. Also made `summary.remaining` / `total_remaining` use the bank card's own remaining when bank tracking is on (they previously used manual starting-amount math even in bank mode, disagreeing with safe-to-spend), and switched a stray `balance / 100` to `fromCents`. New test file `tests/trackerService.test.js` covers the gating fix, summary totals, the bank-mode remaining agreement, cents↔dollars integrity, and `getOverdueCount` gating — the dense Tracker aggregation had no dedicated tests before. (Tracker T1) - **[Tracker/SimpleFIN] Bank card's "unpaid this month" and "remaining" over-counted off-month bills** — `buildBankTracking` (`services/trackerService.js`) summed `expected_amount` for *all* active unpaid bills via SQL with no occurrence gate, so an annual or off-month quarterly bill inflated `unpaid_this_month` (and therefore the bank `remaining`) even though the Tracker rows beside it correctly excluded it — the same class of bug as QA-B5-02, still live on the bank path. `getTracker` now derives the unpaid total from the already-gated rows (via `resolveDueDate`), netting partial payments, and passes it into `buildBankTracking`. Also made `summary.remaining` / `total_remaining` use the bank card's own remaining when bank tracking is on (they previously used manual starting-amount math even in bank mode, disagreeing with safe-to-spend), and switched a stray `balance / 100` to `fromCents`. New test file `tests/trackerService.test.js` covers the gating fix, summary totals, the bank-mode remaining agreement, cents↔dollars integrity, and `getOverdueCount` gating — the dense Tracker aggregation had no dedicated tests before. (Tracker T1)
- **[Payments] Quick-pay could create duplicate payments and double-drop the balance** — `POST /api/payments/quick` (the one-click "pay" behind every Tracker row) had **no duplicate guard** and its INSERT + balance update weren't atomic, unlike `POST /api/payments/bulk`. A double-click, a retry, or two open tabs made a *second* payment for the same bill/date/amount and applied the balance drop twice; a failure between the INSERT and the balance write left a payment with no balance adjustment. Quick-pay now checks the same `bill_id + paid_date + amount` composite key (returning the existing payment idempotently, HTTP 200) and wraps the INSERT + `applyBalanceDelta` in a single `db.transaction`. A different amount on the same day is still a legitimate new payment. Test: `tests/paymentsQuickRoute.test.js`. (Tracker X1) - **[Payments] Quick-pay could create duplicate payments and double-drop the balance** — `POST /api/payments/quick` (the one-click "pay" behind every Tracker row) had **no duplicate guard** and its INSERT + balance update weren't atomic, unlike `POST /api/payments/bulk`. A double-click, a retry, or two open tabs made a *second* payment for the same bill/date/amount and applied the balance drop twice; a failure between the INSERT and the balance write left a payment with no balance adjustment. Quick-pay now checks the same `bill_id + paid_date + amount` composite key (returning the existing payment idempotently, HTTP 200) and wraps the INSERT + `applyBalanceDelta` in a single `db.transaction`. A different amount on the same day is still a legitimate new payment. Test: `tests/paymentsQuickRoute.test.js`. (Tracker X1)

View File

@ -559,7 +559,7 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa
auto_mark_paid: canAutoMarkPaid && autoMarkPaid, auto_mark_paid: canAutoMarkPaid && autoMarkPaid,
is_subscription: isSubscription, is_subscription: isSubscription,
subscription_type: isSubscription ? subscriptionType : null, subscription_type: isSubscription ? subscriptionType : null,
reminder_days_before: isSubscription ? parseInt(reminderDaysBefore || '3', 10) : 3, reminder_days_before: parseInt(reminderDaysBefore || '3', 10),
subscription_source: sourceBill?.subscription_source || 'manual', subscription_source: sourceBill?.subscription_source || 'manual',
subscription_detected_at: sourceBill?.subscription_detected_at, subscription_detected_at: sourceBill?.subscription_detected_at,
has_2fa: has2fa, has_2fa: has2fa,
@ -788,8 +788,8 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa
</label> </label>
{isSubscription && ( {isSubscription && (
<div className="mt-3 grid gap-3 border-t border-border/40 pt-3 sm:grid-cols-2"> <div className="mt-3 border-t border-border/40 pt-3">
<div className="space-y-1.5"> <div className="space-y-1.5 sm:max-w-[50%]">
<Label className="text-xs uppercase tracking-wider text-muted-foreground">Subscription Type</Label> <Label className="text-xs uppercase tracking-wider text-muted-foreground">Subscription Type</Label>
<Select value={subscriptionType} onValueChange={setSubscriptionType}> <Select value={subscriptionType} onValueChange={setSubscriptionType}>
<SelectTrigger className={cn(inp, 'w-full')}> <SelectTrigger className={cn(inp, 'w-full')}>
@ -802,8 +802,16 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa
</SelectContent> </SelectContent>
</Select> </Select>
</div> </div>
<div className="space-y-1.5"> {/* Bank sync button moved to the Transactions tab → Bank Matching Rules section */}
<Label className="text-xs uppercase tracking-wider text-muted-foreground">Reminder Days</Label> </div>
)}
</div>
{/* Reminder lead time applies to every bill (the notifier honors
reminder_days_before for the early "due soon" reminder). */}
<div className="col-span-2 rounded-lg border border-border/60 bg-muted/20 p-3">
<div className="space-y-1.5 sm:max-w-[50%]">
<Label className="text-xs uppercase tracking-wider text-muted-foreground">Reminder Days Before</Label>
<Input <Input
className={cn(inp, 'tracker-number')} className={cn(inp, 'tracker-number')}
type="number" type="number"
@ -812,11 +820,10 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa
value={reminderDaysBefore} value={reminderDaysBefore}
onChange={e => setReminderDaysBefore(e.target.value)} onChange={e => setReminderDaysBefore(e.target.value)}
/> />
<p className="text-[10px] text-muted-foreground/70">0-30 days before renewal.</p> <p className="text-[10px] text-muted-foreground/70">
Get an early reminder this many days before the due date (0-30). Also needs reminders enabled in Settings.
</p>
</div> </div>
{/* Bank sync button moved to the Transactions tab → Bank Matching Rules section */}
</div>
)}
</div> </div>
{/* Debt / Snowball Details — collapsible */} {/* Debt / Snowball Details — collapsible */}

View File

@ -141,8 +141,32 @@ function senderAddress() {
// ── Email templates ─────────────────────────────────────────────────────────── // ── Email templates ───────────────────────────────────────────────────────────
// Per-bill reminder lead time (days before due) for the early "due soon"
// reminder. Every bill has a reminder_days_before column (default 3); honor it
// so a user who asks for "7 days before" actually gets a 7-day reminder.
function leadDaysOf(bill) {
return Number.isInteger(bill?.reminder_days_before) ? bill.reminder_days_before : 3;
}
// Which reminder type (if any) applies to a bill that is `diffDays` calendar
// days from its due date. Pure + exported so the lead-time logic is unit-testable
// without invoking the full runner (which uses the real clock + real senders).
// The early reminder fires at the bill's own lead time, and only when that lead
// is ≥ 2 so it never collides with the 1-day or same-day reminders.
function reminderTypeFor(bill, diffDays) {
const leadDays = leadDaysOf(bill);
if (diffDays >= 2 && diffDays === leadDays) return 'due_3d';
if (diffDays === 1) return 'due_1d';
if (diffDays === 0) return 'due_today';
if (diffDays < 0) return 'overdue';
return null;
}
const TYPE_META = { const TYPE_META = {
due_3d: { subject: (b) => `Reminder: ${b.name} due in 3 days`, urgency: 'upcoming' }, // The `due_3d` key is the early reminder; its lead time is the bill's own
// reminder_days_before (only fires when that is ≥ 2 — a 1-day lead is covered
// by due_1d and a 0-day lead by due_today).
due_3d: { subject: (b) => `Reminder: ${b.name} due in ${leadDaysOf(b)} days`, urgency: 'upcoming' },
due_1d: { subject: (b) => `Reminder: ${b.name} due tomorrow`, urgency: 'soon' }, due_1d: { subject: (b) => `Reminder: ${b.name} due tomorrow`, urgency: 'soon' },
due_today: { subject: (b) => `Due today: ${b.name}`, urgency: 'today' }, due_today: { subject: (b) => `Due today: ${b.name}`, urgency: 'today' },
overdue: { subject: (b) => `Overdue: ${b.name} hasn't been paid`, urgency: 'overdue' }, overdue: { subject: (b) => `Overdue: ${b.name} hasn't been paid`, urgency: 'overdue' },
@ -169,7 +193,7 @@ function buildEmailHtml(bill, type, dueDate) {
// these message strings (previously raw — an XSS vector via the bill name). // these message strings (previously raw — an XSS vector via the bill name).
const name = esc(bill.name); const name = esc(bill.name);
const messages = { const messages = {
due_3d: `<strong>${name}</strong> is due in 3 days.`, due_3d: `<strong>${name}</strong> is due in ${leadDaysOf(bill)} days.`,
due_1d: `<strong>${name}</strong> is due <strong>tomorrow</strong>.`, due_1d: `<strong>${name}</strong> is due <strong>tomorrow</strong>.`,
due_today: `<strong>${name}</strong> is due <strong>today</strong>.`, due_today: `<strong>${name}</strong> is due <strong>today</strong>.`,
overdue: `<strong>${name}</strong> was due on ${fmt(dueDate)} and has not been marked as paid.`, overdue: `<strong>${name}</strong> was due on ${fmt(dueDate)} and has not been marked as paid.`,
@ -359,12 +383,9 @@ async function runNotifications() {
const dueDay = new Date(due.getFullYear(), due.getMonth(), due.getDate()); const dueDay = new Date(due.getFullYear(), due.getMonth(), due.getDate());
const diffDays = Math.round((dueDay - todayDate) / 86400000); const diffDays = Math.round((dueDay - todayDate) / 86400000);
// Determine which type applies today // Determine which type applies today. The early reminder fires at the bill's
let type = null; // own lead time (reminder_days_before, default 3) rather than a fixed 3 days.
if (diffDays === 3) type = 'due_3d'; const type = reminderTypeFor(bill, diffDays);
else if (diffDays === 1) type = 'due_1d';
else if (diffDays === 0) type = 'due_today';
else if (diffDays < 0) type = 'overdue';
if (!type) continue; if (!type) continue;
@ -555,3 +576,6 @@ module.exports = { runNotifications, runDriftNotifications, sendTestEmail, creat
// before it, leaving `_push` undefined → "Send test push" always 500'd). // before it, leaving `_push` undefined → "Send test push" always 500'd).
module.exports._push = { sendNtfy, sendGotify, sendDiscord, sendTelegram, sendTestPush, sendPushToUser, encryptSecret }; module.exports._push = { sendNtfy, sendGotify, sendDiscord, sendTelegram, sendTestPush, sendPushToUser, encryptSecret };
module.exports._email = { buildEmailHtml, esc }; module.exports._email = { buildEmailHtml, esc };
// Reminder lead-time logic, exposed for unit tests (the full runner uses the
// real clock + real senders, so the pure type selection is tested in isolation).
module.exports._reminders = { reminderTypeFor, leadDaysOf, TYPE_META };

View File

@ -0,0 +1,48 @@
'use strict';
// BM3 — the reminder notifier used a hard-coded 3-day early reminder and never
// read the bill's reminder_days_before, so the "Reminder days before" control
// was a no-op. These tests pin the now-honored per-bill lead time (pure logic,
// no clock/senders) and the dynamic email wording.
const test = require('node:test');
const assert = require('node:assert/strict');
const { _reminders, _email } = require('../services/notificationService');
const { reminderTypeFor, leadDaysOf, TYPE_META } = _reminders;
test('leadDaysOf defaults to 3 and honors an integer reminder_days_before', () => {
assert.equal(leadDaysOf({}), 3);
assert.equal(leadDaysOf({ reminder_days_before: null }), 3);
assert.equal(leadDaysOf({ reminder_days_before: 7 }), 7);
assert.equal(leadDaysOf({ reminder_days_before: 0 }), 0);
});
test('a 7-day lead fires the early reminder at 7 days out, NOT at 3', () => {
const bill = { reminder_days_before: 7 };
assert.equal(reminderTypeFor(bill, 7), 'due_3d', 'early reminder at the chosen lead');
assert.equal(reminderTypeFor(bill, 3), null, 'no reminder at the old fixed 3 days');
});
test('default (no reminder_days_before) still fires at 3 days — backwards compatible', () => {
assert.equal(reminderTypeFor({}, 3), 'due_3d');
assert.equal(reminderTypeFor({}, 5), null);
});
test('1-day and same-day reminders are unaffected and never double up with a small lead', () => {
// lead of 1 → the early reminder is suppressed (due_1d covers it)
assert.equal(reminderTypeFor({ reminder_days_before: 1 }, 1), 'due_1d');
// lead of 0 → due_today covers it
assert.equal(reminderTypeFor({ reminder_days_before: 0 }, 0), 'due_today');
// generic day/overdue selection
assert.equal(reminderTypeFor({ reminder_days_before: 5 }, 1), 'due_1d');
assert.equal(reminderTypeFor({ reminder_days_before: 5 }, 0), 'due_today');
assert.equal(reminderTypeFor({ reminder_days_before: 5 }, -2), 'overdue');
assert.equal(reminderTypeFor({ reminder_days_before: 5 }, 4), null);
});
test('email subject + body reflect the actual lead days', () => {
const bill = { name: 'Insurance', expected_amount: 12000, reminder_days_before: 7 };
assert.match(TYPE_META.due_3d.subject(bill), /due in 7 days/);
const html = _email.buildEmailHtml(bill, 'due_3d', '2026-07-10');
assert.match(html, /is due in 7 days/);
});