fix(qa): notification _push export was clobbered → "Send test push" 500'd (B10-01)
- notificationService: `module.exports._push = {...}` was set BEFORE the final
`module.exports = {...}`, which wiped it, so routes/notifications.js got
`_push || {}` → sendTestPush undefined → POST /api/notifications/test-push
always threw "Push service not initialised". Scheduled reminders were fine
(in-scope calls). Moved the _push assignment after the reassignment.
- add tests/notificationDelivery.test.js (7 tests: ntfy/gotify/discord payloads,
dispatch, error handling, unknown channel, no token leak in the body)
- docs: archive QA-B10-01
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
972daa9b07
commit
2050e13407
|
|
@ -3,6 +3,7 @@
|
||||||
|
|
||||||
### 🐛 QA Fixes
|
### 🐛 QA Fixes
|
||||||
|
|
||||||
|
- **[Notifications] "Send test push" was completely broken** — `services/notificationService.js` attached its `_push` export (the ntfy/Gotify/Discord/Telegram helpers) *before* the final `module.exports = {…}`, which clobbered it, so `require('…/notificationService')._push` was `undefined`. `routes/notifications.js` (`const { sendTestPush } = require(…)._push || {}`) therefore always hit `throw 'Push service not initialised'` → **`POST /api/notifications/test-push` always returned 500** for every user testing their push channel. Scheduled reminders were unaffected (they call `sendPushToUser` in-scope). Moved the `_push` assignment after the reassignment. Covered by `tests/notificationDelivery.test.js` (per-channel payloads, dispatch, error handling, and a check that the auth token never leaks into the message body). (was QA-B10-01)
|
||||||
- **[Summary/Analytics] Non-monthly bills were counted in every month** — the Summary expense list/total and the Analytics "expected vs actual" line both counted annual (and off-month quarterly) bills for months they weren't due, over-stating the obligation and disagreeing with the Tracker (e.g. a yearly insurance bill inflated every month). Both `routes/summary.js` and `services/analyticsService.js` now gate bills by `resolveDueDate` — the same occurrence check the Tracker uses. Guarded by Tracker↔Summary and Tracker↔Analytics reconciliation checks in `e2e/api.probe.spec.js`. The SimpleFIN bank-tracking `unpaid_this_month` metric had the same gap and is fixed the same way (fetch + JS `resolveDueDate` filter, since SQL can't call it), covered by `tests/summaryBankTracking.test.js`. (was QA-B5-01, QA-B5-02, QA-B5-03)
|
- **[Summary/Analytics] Non-monthly bills were counted in every month** — the Summary expense list/total and the Analytics "expected vs actual" line both counted annual (and off-month quarterly) bills for months they weren't due, over-stating the obligation and disagreeing with the Tracker (e.g. a yearly insurance bill inflated every month). Both `routes/summary.js` and `services/analyticsService.js` now gate bills by `resolveDueDate` — the same occurrence check the Tracker uses. Guarded by Tracker↔Summary and Tracker↔Analytics reconciliation checks in `e2e/api.probe.spec.js`. The SimpleFIN bank-tracking `unpaid_this_month` metric had the same gap and is fixed the same way (fetch + JS `resolveDueDate` filter, since SQL can't call it), covered by `tests/summaryBankTracking.test.js`. (was QA-B5-01, QA-B5-02, QA-B5-03)
|
||||||
- **[Data] Seed Demo Data amounts were 100× too small** — `scripts/seedDemoData.js` inserted demo dollar amounts straight into `bills.expected_amount` / `current_balance` / `minimum_payment`, which became integer-cents columns in the v1.03 migration, so a seeded "$85" bill showed as $0.85 (a whole demo month totalled ~$32 instead of ~$3,200). Now wraps demo values in `toCents()` before insert. Regression guard added in `e2e/api.probe.spec.js`. (was QA-B9-01)
|
- **[Data] Seed Demo Data amounts were 100× too small** — `scripts/seedDemoData.js` inserted demo dollar amounts straight into `bills.expected_amount` / `current_balance` / `minimum_payment`, which became integer-cents columns in the v1.03 migration, so a seeded "$85" bill showed as $0.85 (a whole demo month totalled ~$32 instead of ~$3,200). Now wraps demo values in `toCents()` before insert. Regression guard added in `e2e/api.probe.spec.js`. (was QA-B9-01)
|
||||||
- **[Bills] `expected_amount` was unvalidated** — POST/PUT `/api/bills` accepted negative amounts, non-numeric input (silently coerced to $0), and absurd values (`1e15` → cents past `Number.MAX_SAFE_INTEGER`). `validateBillData` (`services/billsService.js`) now rejects non-numeric / negative / out-of-range with a structured `VALIDATION_ERROR`, accepting `0`…`$100,000,000`. Regression assertions in `e2e/api.probe.spec.js`. (was QA-B13-01)
|
- **[Bills] `expected_amount` was unvalidated** — POST/PUT `/api/bills` accepted negative amounts, non-numeric input (silently coerced to $0), and absurd values (`1e15` → cents past `Number.MAX_SAFE_INTEGER`). `validateBillData` (`services/billsService.js`) now rejects non-numeric / negative / out-of-range with a structured `VALIDATION_ERROR`, accepting `0`…`$100,000,000`. Regression assertions in `e2e/api.probe.spec.js`. (was QA-B13-01)
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,6 @@
|
||||||
# BillTracker — Master QA Plan (living document)
|
# BillTracker — Master QA Plan (living document)
|
||||||
|
|
||||||
**Version target:** v0.41.x · **Executor:** Claude (active) · **Last updated:** 2026-07-02 (Cycle 1: 12 findings fixed & archived, 0 open; automated re-run clean)
|
**Version target:** v0.41.x · **Executor:** Claude (active) · **Last updated:** 2026-07-02 (Cycle 1: 13 findings fixed & archived, 0 open — incl. a broken "Send test push")
|
||||||
|
|
||||||
This is a **living, operational** QA document, not a static spec. Claude runs it,
|
This is a **living, operational** QA document, not a static spec. Claude runs it,
|
||||||
in **batches**, actively hunting for bugs/errors/rough edges, **fixing** them, and
|
in **batches**, actively hunting for bugs/errors/rough edges, **fixing** them, and
|
||||||
|
|
@ -97,7 +97,7 @@ before cross-cutting; regression last). Update **Status** and **Findings** every
|
||||||
| B7 | Debt planning (math) | `/snowball`, `/payoff` APR/amortization vs hand-calc | edge (APR=0, $0 debt) | 🔄 | 0 / 2 |
|
| B7 | Debt planning (math) | `/snowball`, `/payoff` APR/amortization vs hand-calc | edge (APR=0, $0 debt) | 🔄 | 0 / 2 |
|
||||||
| B8 | Banking & bank sync | `/bank-transactions`, SimpleFIN sync, matching, merchant/store, advisory filter | seeded txns | ⬜ | 0 / 0 |
|
| B8 | Banking & bank sync | `/bank-transactions`, SimpleFIN sync, matching, merchant/store, advisory filter | seeded txns | ⬜ | 0 / 0 |
|
||||||
| B9 | Data lifecycle | `/data` import (XLSX/CSV/SQLite), export, ICS feed, backups round-trip | empty + seeded | 🔄 | 0 / 1 |
|
| B9 | Data lifecycle | `/data` import (XLSX/CSV/SQLite), export, ICS feed, backups round-trip | empty + seeded | 🔄 | 0 / 1 |
|
||||||
| B10 | Notifications & workers | email + ntfy/Gotify/Discord/Telegram, reminders, cron workers | seeded | 🔄 | 0 / 0 |
|
| B10 | Notifications & workers | email + ntfy/Gotify/Discord/Telegram, reminders, cron workers | seeded | 🔄 | 0 / 1 |
|
||||||
| B11 | Admin panel | users, login mode, auth methods, backups, cleanup, status, onboarding | admin | 🔄 | 0 / 0 |
|
| B11 | Admin panel | users, login mode, auth methods, backups, cleanup, status, onboarding | admin | 🔄 | 0 / 0 |
|
||||||
| B12 | Settings, Profile & global UI | `/settings`, `/profile`, static pages, command palette, sidebar/nav | any | 🔄 | 0 / 0 |
|
| B12 | Settings, Profile & global UI | `/settings`, `/profile`, static pages, command palette, sidebar/nav | any | 🔄 | 0 / 0 |
|
||||||
| B13 | API / backend direct | all `/api/*`: auth, CSRF, validation, rate limits, error shape, IDOR, cents | via HTTP client | 🔄 | 0 / 1 |
|
| B13 | API / backend direct | all `/api/*`: auth, CSRF, validation, rate limits, error shape, IDOR, cents | via HTTP client | 🔄 | 0 / 1 |
|
||||||
|
|
@ -115,7 +115,7 @@ until you get a clean cycle.
|
||||||
|
|
||||||
| Cycle | Started | Build / commit | Findings logged | Fixed / archived | Result |
|
| Cycle | Started | Build / commit | Findings logged | Fixed / archived | Result |
|
||||||
|-------|---------|----------------|-----------------|------------------|--------|
|
|-------|---------|----------------|-----------------|------------------|--------|
|
||||||
| 1 | 2026-07-02 | `bdbf231`→(dev) | 12 | **12 → all fixed, verified & archived** (B9-01, B13-01, B6-01, B7-01/02, B14-01/02/03, B0-01, B5-01/02/03) | ✅ **0 open.** Post seed-fix reconciliation caught the **occurrence-gating family** — Summary (S2), Analytics, and SimpleFIN bank-tracking all counted non-monthly bills every month; all fixed via `resolveDueDate` and guarded (probe reconciliation + `tests/summaryBankTracking.test.js`). Probed B0/B1/B3/B4/B5/B6/B7/B8/B9/B13/B14; solid: auth/isolation, CSRF, payment/date validation, recurrence, matching/dedup, subscription+spending math, XSS, calendar gating. **A full re-run (B0→B15) is still required to declare the cycle clean per exit criteria.** |
|
| 1 | 2026-07-02 | `bdbf231`→(dev) | 13 | **13 → all fixed, verified & archived** (…, +B10-01 broken "Send test push") | ✅ **0 open.** Post seed-fix reconciliation caught the **occurrence-gating family** — Summary (S2), Analytics, and SimpleFIN bank-tracking all counted non-monthly bills every month; all fixed via `resolveDueDate` and guarded (probe reconciliation + `tests/summaryBankTracking.test.js`). Probed B0/B1/B3/B4/B5/B6/B7/B8/B9/B13/B14; solid: auth/isolation, CSRF, payment/date validation, recurrence, matching/dedup, subscription+spending math, XSS, calendar gating. **A full re-run (B0→B15) is still required to declare the cycle clean per exit criteria.** |
|
||||||
|
|
||||||
**Result key:** 🔄 in progress · 🔁 findings fixed, re-run required · ✅ clean (zero findings — QA complete)
|
**Result key:** 🔄 in progress · 🔁 findings fixed, re-run required · ✅ clean (zero findings — QA complete)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -95,7 +95,8 @@ async function sendTestPush(user) {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports._push = { sendNtfy, sendGotify, sendDiscord, sendTelegram, sendTestPush, sendPushToUser, encryptSecret };
|
// NOTE: the `_push` export is attached AFTER the final `module.exports = {…}`
|
||||||
|
// below — assigning it here would be clobbered by that reassignment (QA-B10-01).
|
||||||
|
|
||||||
// ── SMTP transport ────────────────────────────────────────────────────────────
|
// ── SMTP transport ────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
@ -546,3 +547,7 @@ async function runDriftNotifications() {
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports = { runNotifications, runDriftNotifications, sendTestEmail, createTransport };
|
module.exports = { runNotifications, runDriftNotifications, sendTestEmail, createTransport };
|
||||||
|
// Push helpers, exposed for the test-push route + tests. Assigned AFTER the line
|
||||||
|
// above so it isn't clobbered by the reassignment (QA-B10-01: previously set
|
||||||
|
// before it, leaving `_push` undefined → "Send test push" always 500'd).
|
||||||
|
module.exports._push = { sendNtfy, sendGotify, sendDiscord, sendTelegram, sendTestPush, sendPushToUser, encryptSecret };
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,108 @@
|
||||||
|
'use strict';
|
||||||
|
|
||||||
|
// B10: push-notification DELIVERY — verifies the message-building + HTTP send path
|
||||||
|
// for each channel against a local sink (no external network), plus error handling,
|
||||||
|
// dispatch, and that the auth token never leaks into the message body.
|
||||||
|
const test = require('node:test');
|
||||||
|
const assert = require('node:assert/strict');
|
||||||
|
const http = require('node:http');
|
||||||
|
|
||||||
|
const { _push } = require('../services/notificationService');
|
||||||
|
const { sendNtfy, sendGotify, sendDiscord, sendPushToUser } = _push;
|
||||||
|
|
||||||
|
// Local HTTP sink: records requests, status configurable.
|
||||||
|
function startSink() {
|
||||||
|
const requests = [];
|
||||||
|
let status = 200;
|
||||||
|
const server = http.createServer((req, res) => {
|
||||||
|
let body = '';
|
||||||
|
req.on('data', (c) => (body += c));
|
||||||
|
req.on('end', () => {
|
||||||
|
requests.push({ method: req.method, url: req.url, headers: req.headers, body });
|
||||||
|
res.statusCode = status;
|
||||||
|
res.end(status === 200 ? 'ok' : 'err');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
return new Promise((resolve) => {
|
||||||
|
server.listen(0, '127.0.0.1', () => {
|
||||||
|
resolve({
|
||||||
|
url: `http://127.0.0.1:${server.address().port}`,
|
||||||
|
requests,
|
||||||
|
setStatus: (c) => { status = c; },
|
||||||
|
close: () => new Promise((r) => server.close(r)),
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
test('ntfy delivery: posts body + Title header; token stays in Authorization, not the body', async () => {
|
||||||
|
const sink = await startSink();
|
||||||
|
try {
|
||||||
|
await sendNtfy(sink.url, 'secret-token-xyz', 'Bill due', 'Electric $85 due tomorrow', 'overdue');
|
||||||
|
assert.equal(sink.requests.length, 1);
|
||||||
|
const r = sink.requests[0];
|
||||||
|
assert.equal(r.method, 'POST');
|
||||||
|
assert.equal(r.body, 'Electric $85 due tomorrow');
|
||||||
|
assert.equal(r.headers['title'], 'Bill due');
|
||||||
|
assert.ok(r.headers['priority'], 'priority header present');
|
||||||
|
assert.equal(r.headers['authorization'], 'Bearer secret-token-xyz');
|
||||||
|
assert.ok(!r.body.includes('secret-token-xyz'), 'token must never appear in the message body');
|
||||||
|
} finally { await sink.close(); }
|
||||||
|
});
|
||||||
|
|
||||||
|
test('gotify delivery: JSON {title, message, priority} to /message?token=', async () => {
|
||||||
|
const sink = await startSink();
|
||||||
|
try {
|
||||||
|
await sendGotify(sink.url, 'gtoken', 'Reminder', 'Rent due in 3 days', 'today');
|
||||||
|
const r = sink.requests[0];
|
||||||
|
assert.match(r.url, /\/message\?token=gtoken/);
|
||||||
|
const json = JSON.parse(r.body);
|
||||||
|
assert.equal(json.title, 'Reminder');
|
||||||
|
assert.equal(json.message, 'Rent due in 3 days');
|
||||||
|
assert.equal(json.priority, 7); // 'today'
|
||||||
|
} finally { await sink.close(); }
|
||||||
|
});
|
||||||
|
|
||||||
|
test('discord delivery: webhook embed with title/description', async () => {
|
||||||
|
const sink = await startSink();
|
||||||
|
try {
|
||||||
|
await sendDiscord(sink.url, 'Overdue bill', 'Water bill is 2 days overdue', 'overdue');
|
||||||
|
const json = JSON.parse(sink.requests[0].body);
|
||||||
|
assert.equal(json.embeds[0].title, 'Overdue bill');
|
||||||
|
assert.equal(json.embeds[0].description, 'Water bill is 2 days overdue');
|
||||||
|
assert.ok(typeof json.embeds[0].color === 'number');
|
||||||
|
} finally { await sink.close(); }
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch: sendPushToUser routes to the configured channel', async () => {
|
||||||
|
const sink = await startSink();
|
||||||
|
try {
|
||||||
|
const user = { notify_push_enabled: 1, push_channel: 'ntfy', push_url: sink.url, push_token: '' };
|
||||||
|
await sendPushToUser(user, 'Title', 'Body', 'soon');
|
||||||
|
assert.equal(sink.requests.length, 1);
|
||||||
|
assert.equal(sink.requests[0].body, 'Body');
|
||||||
|
} finally { await sink.close(); }
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch: disabled/unconfigured user sends nothing (no throw)', async () => {
|
||||||
|
await assert.doesNotReject(sendPushToUser({ notify_push_enabled: 0 }, 'T', 'B', 'soon'));
|
||||||
|
await assert.doesNotReject(sendPushToUser({ notify_push_enabled: 1, push_channel: 'ntfy' }, 'T', 'B', 'soon'));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('error handling: an unreachable/erroring channel throws a clear, channel-named error', async () => {
|
||||||
|
const sink = await startSink();
|
||||||
|
sink.setStatus(500);
|
||||||
|
try {
|
||||||
|
await assert.rejects(
|
||||||
|
sendNtfy(sink.url, '', 'T', 'B', 'soon'),
|
||||||
|
/ntfy returned 500/,
|
||||||
|
);
|
||||||
|
} finally { await sink.close(); }
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch: an unknown channel throws instead of silently doing nothing', async () => {
|
||||||
|
await assert.rejects(
|
||||||
|
sendPushToUser({ notify_push_enabled: 1, push_channel: 'carrier-pigeon', push_url: 'http://x' }, 'T', 'B', 'soon'),
|
||||||
|
/Unknown push channel: carrier-pigeon/,
|
||||||
|
);
|
||||||
|
});
|
||||||
Loading…
Reference in New Issue