diff --git a/HISTORY.md b/HISTORY.md index b58f790..c0b9fd8 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -11,6 +11,7 @@ ### 🔍 Snowball review - **[Debt] Payoff projection no longer fakes a "freedom" date when a debt can't be paid off** — if a debt's minimum payment never overcomes its interest and the rolling snowball can't cover it either, it never reaches $0. The simulation previously excluded that debt from `months_to_freedom` (a `Math.max` over payoff months treated "never" as 0), so it reported the *other* debts' last month as your payoff date — misleadingly optimistic. Now `months_to_freedom`/`payoff_date` are `null` when any active debt never clears, the result is flagged `capped`, and each such debt is marked `never_paid` (`services/snowballService.js`, `services/aprService.js`). (Snowball review #1) +- **[Debt] Snowball plan API: consistent errors, less duplication, no N+1** — the four plan-lifecycle endpoints (pause/resume/complete/abandon) were near-identical copies and returned a plain `{error}` shape unlike the rest of the API; folded them into one `transitionPlan` helper returning `standardizeError` `{message, code}` with proper state guards + ownership scoping. `enrichPlanWithProgress` fetched each snapshot bill in its own query (`GET /plans` = plans × debts queries) and wasn't user-scoped — now one `WHERE id IN (…) AND user_id = ?` batch. Test: `tests/snowballPlanRoute.test.js`. (Snowball review #4, #6, #7) - **[Debt] Added the missing test coverage for all payoff math** — the debt-payoff engine (`calculateSnowball`/`calculateAvalanche`/`calculateMinimumOnly`/`amortizationSchedule`/`monthsToPayoff`/`debtAprSnapshot`) had **zero** automated tests despite being the most math-heavy code in the app. Added `tests/snowballMath.test.js` (12 tests) with hand-calculated examples (0% and 12% APR amortization rows, snowball rolling, avalanche-beats-snowball interest, skip reasons, APR breakdown) and the unpayable-debt edge above. (Snowball review #5) ### 🐛 QA Fixes diff --git a/routes/snowball.js b/routes/snowball.js index 91a69f7..fcd9d99 100644 --- a/routes/snowball.js +++ b/routes/snowball.js @@ -278,8 +278,21 @@ function enrichPlanWithProgress(db, plan) { let snapshot; try { snapshot = JSON.parse(plan.plan_snapshot); } catch { snapshot = null; } - const currentDebts = (snapshot?.debts ?? []).map(d => { - const bill = db.prepare('SELECT current_balance, name, deleted_at FROM bills WHERE id = ?').get(d.bill_id); + // Fetch every snapshot bill in one query (user-scoped), not one-per-debt. + const snapDebts = snapshot?.debts ?? []; + const billIds = [...new Set(snapDebts.map(d => d.bill_id).filter(Boolean))]; + const billsById = {}; + if (billIds.length) { + const placeholders = billIds.map(() => '?').join(','); + for (const b of db.prepare( + `SELECT id, current_balance, name, deleted_at FROM bills WHERE id IN (${placeholders}) AND user_id = ?`, + ).all(...billIds, plan.user_id)) { + billsById[b.id] = b; + } + } + + const currentDebts = snapDebts.map(d => { + const bill = billsById[d.bill_id]; const currentBalance = bill && !bill.deleted_at ? fromCents(bill.current_balance) : null; const startingBalance = d.starting_balance ?? 0; const progressPct = startingBalance > 0 && currentBalance !== null @@ -308,7 +321,7 @@ router.post('/plans', (req, res) => { const debts = getDebtBills(userId, ramseyMode); const activeDebts = debts.filter(b => (b.current_balance ?? 0) > 0); if (activeDebts.length === 0) { - return res.status(400).json({ error: 'No debts with a balance found. Add a balance to at least one bill.' }); + return res.status(400).json(standardizeError('No debts with a balance found. Add a balance to at least one bill.', 'NO_DEBTS')); } // Money fields on `debts` are stored as integer cents; the snowball/APR @@ -367,7 +380,7 @@ router.post('/plans', (req, res) => { res.status(201).json(enrichPlanWithProgress(db, plan)); } catch (err) { console.error('[snowball plans] POST error:', err.message); - res.status(500).json({ error: 'Failed to start plan' }); + res.status(500).json(standardizeError('Failed to start plan', 'PLAN_CREATE_ERROR')); } }); @@ -381,7 +394,7 @@ router.get('/plans', (req, res) => { res.json({ plans: plans.map(p => enrichPlanWithProgress(db, p)) }); } catch (err) { console.error('[snowball plans] GET /plans error:', err.message); - res.status(500).json({ error: 'Failed to load plans' }); + res.status(500).json(standardizeError('Failed to load plans', 'PLAN_LIST_ERROR')); } }); @@ -397,7 +410,7 @@ router.get('/plans/active', (req, res) => { res.json(plan ? enrichPlanWithProgress(db, plan) : null); } catch (err) { console.error('[snowball plans] GET /plans/active error:', err.message); - res.status(500).json({ error: 'Failed to load active plan' }); + res.status(500).json(standardizeError('Failed to load active plan', 'PLAN_ACTIVE_ERROR')); } }); @@ -406,9 +419,9 @@ router.patch('/plans/:id', (req, res) => { try { const db = getDb(); const id = parseInt(req.params.id, 10); - if (!Number.isInteger(id) || id <= 0) return res.status(400).json({ error: 'Invalid id' }); + if (!Number.isInteger(id) || id <= 0) return res.status(400).json(standardizeError('Invalid plan id', 'VALIDATION_ERROR', 'id')); const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ? AND user_id = ?').get(id, req.user.id); - if (!plan) return res.status(404).json({ error: 'Plan not found' }); + if (!plan) return res.status(404).json(standardizeError('Plan not found', 'NOT_FOUND')); const { name, notes } = req.body; const newName = (typeof name === 'string' && name.trim()) ? name.trim().slice(0, 100) : plan.name; @@ -422,96 +435,43 @@ router.patch('/plans/:id', (req, res) => { res.json(enrichPlanWithProgress(db, updated)); } catch (err) { console.error('[snowball plans] PATCH error:', err.message); - res.status(500).json({ error: 'Failed to update plan' }); + res.status(500).json(standardizeError('Failed to update plan', 'PLAN_UPDATE_ERROR')); } }); -// POST /api/snowball/plans/:id/pause -router.post('/plans/:id/pause', (req, res) => { +// Shared status-transition handler for pause / resume / complete / abandon. +// `setSql` is a fixed constant per route (never user input). +function transitionPlan(req, res, { allowedFrom, setSql, action, past }) { try { const db = getDb(); const id = parseInt(req.params.id, 10); - if (!Number.isInteger(id) || id <= 0) return res.status(400).json({ error: 'Invalid id' }); + if (!Number.isInteger(id) || id <= 0) { + return res.status(400).json(standardizeError('Invalid plan id', 'VALIDATION_ERROR', 'id')); + } const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ? AND user_id = ?').get(id, req.user.id); - if (!plan) return res.status(404).json({ error: 'Plan not found' }); - if (plan.status !== 'active') return res.status(400).json({ error: 'Only active plans can be paused' }); - - db.prepare(` - UPDATE snowball_plans SET status = 'paused', paused_at = datetime('now'), updated_at = datetime('now') WHERE id = ? - `).run(id); - + if (!plan) return res.status(404).json(standardizeError('Plan not found', 'NOT_FOUND')); + if (!allowedFrom.includes(plan.status)) { + return res.status(400).json(standardizeError( + `Only ${allowedFrom.join(' or ')} plans can be ${past}`, 'INVALID_PLAN_STATE', + )); + } + db.prepare(`UPDATE snowball_plans SET ${setSql}, updated_at = datetime('now') WHERE id = ?`).run(id); const updated = db.prepare('SELECT * FROM snowball_plans WHERE id = ?').get(id); res.json(enrichPlanWithProgress(db, updated)); } catch (err) { - console.error('[snowball plans] pause error:', err.message); - res.status(500).json({ error: 'Failed to pause plan' }); + console.error(`[snowball plans] ${action} error:`, err.message); + res.status(500).json(standardizeError(`Failed to ${action} plan`, 'PLAN_TRANSITION_ERROR')); } -}); +} -// POST /api/snowball/plans/:id/resume -router.post('/plans/:id/resume', (req, res) => { - try { - const db = getDb(); - const id = parseInt(req.params.id, 10); - if (!Number.isInteger(id) || id <= 0) return res.status(400).json({ error: 'Invalid id' }); - const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ? AND user_id = ?').get(id, req.user.id); - if (!plan) return res.status(404).json({ error: 'Plan not found' }); - if (plan.status !== 'paused') return res.status(400).json({ error: 'Only paused plans can be resumed' }); - - db.prepare(` - UPDATE snowball_plans SET status = 'active', paused_at = NULL, updated_at = datetime('now') WHERE id = ? - `).run(id); - - const updated = db.prepare('SELECT * FROM snowball_plans WHERE id = ?').get(id); - res.json(enrichPlanWithProgress(db, updated)); - } catch (err) { - console.error('[snowball plans] resume error:', err.message); - res.status(500).json({ error: 'Failed to resume plan' }); - } -}); - -// POST /api/snowball/plans/:id/complete -router.post('/plans/:id/complete', (req, res) => { - try { - const db = getDb(); - const id = parseInt(req.params.id, 10); - if (!Number.isInteger(id) || id <= 0) return res.status(400).json({ error: 'Invalid id' }); - const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ? AND user_id = ?').get(id, req.user.id); - if (!plan) return res.status(404).json({ error: 'Plan not found' }); - if (!['active', 'paused'].includes(plan.status)) return res.status(400).json({ error: 'Only active or paused plans can be completed' }); - - db.prepare(` - UPDATE snowball_plans SET status = 'completed', completed_at = datetime('now'), updated_at = datetime('now') WHERE id = ? - `).run(id); - - const updated = db.prepare('SELECT * FROM snowball_plans WHERE id = ?').get(id); - res.json(enrichPlanWithProgress(db, updated)); - } catch (err) { - console.error('[snowball plans] complete error:', err.message); - res.status(500).json({ error: 'Failed to complete plan' }); - } -}); - -// POST /api/snowball/plans/:id/abandon -router.post('/plans/:id/abandon', (req, res) => { - try { - const db = getDb(); - const id = parseInt(req.params.id, 10); - if (!Number.isInteger(id) || id <= 0) return res.status(400).json({ error: 'Invalid id' }); - const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ? AND user_id = ?').get(id, req.user.id); - if (!plan) return res.status(404).json({ error: 'Plan not found' }); - if (!['active', 'paused'].includes(plan.status)) return res.status(400).json({ error: 'Only active or paused plans can be abandoned' }); - - db.prepare(` - UPDATE snowball_plans SET status = 'abandoned', updated_at = datetime('now') WHERE id = ? - `).run(id); - - const updated = db.prepare('SELECT * FROM snowball_plans WHERE id = ?').get(id); - res.json(enrichPlanWithProgress(db, updated)); - } catch (err) { - console.error('[snowball plans] abandon error:', err.message); - res.status(500).json({ error: 'Failed to abandon plan' }); - } -}); +// POST /api/snowball/plans/:id/{pause,resume,complete,abandon} +router.post('/plans/:id/pause', (req, res) => + transitionPlan(req, res, { allowedFrom: ['active'], setSql: "status = 'paused', paused_at = datetime('now')", action: 'pause', past: 'paused' })); +router.post('/plans/:id/resume', (req, res) => + transitionPlan(req, res, { allowedFrom: ['paused'], setSql: "status = 'active', paused_at = NULL", action: 'resume', past: 'resumed' })); +router.post('/plans/:id/complete', (req, res) => + transitionPlan(req, res, { allowedFrom: ['active', 'paused'], setSql: "status = 'completed', completed_at = datetime('now')", action: 'complete', past: 'completed' })); +router.post('/plans/:id/abandon', (req, res) => + transitionPlan(req, res, { allowedFrom: ['active', 'paused'], setSql: "status = 'abandoned'", action: 'abandon', past: 'abandoned' })); module.exports = router; diff --git a/tests/snowballPlanRoute.test.js b/tests/snowballPlanRoute.test.js new file mode 100644 index 0000000..7b25bcb --- /dev/null +++ b/tests/snowballPlanRoute.test.js @@ -0,0 +1,86 @@ +'use strict'; + +// Snowball review #4/#6/#7: plan lifecycle endpoints — consolidated transitionPlan +// handler (state guards + standardized errors + ownership) and the batched, +// user-scoped enrichPlanWithProgress. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const os = require('node:os'); +const path = require('node:path'); +const fs = require('node:fs'); + +const dbPath = path.join(os.tmpdir(), `bill-tracker-snowball-plan-${process.pid}.sqlite`); +process.env.DB_PATH = dbPath; + +const { getDb, closeDb } = require('../db/database'); +const router = require('../routes/snowball'); + +function handler(method, routePath) { + const layer = router.stack.find(l => l.route?.path === routePath && l.route.methods[method]); + assert.ok(layer, `${method.toUpperCase()} ${routePath} exists`); + return layer.route.stack[layer.route.stack.length - 1].handle; +} +function call(method, routePath, { userId, params = {}, body = {} } = {}) { + const h = handler(method, routePath); + return new Promise((resolve) => { + const req = { params, body, query: {}, user: { id: userId, role: 'user' } }; + const res = { + statusCode: 200, + status(c) { this.statusCode = c; return this; }, + json(d) { resolve({ status: this.statusCode, data: d }); }, + }; + h(req, res); + }); +} + +let userA, userB; +test.before(() => { + const db = getDb(); + userA = db.prepare("INSERT INTO users (username, password_hash, role, active) VALUES ('sb-a','x','user',1)").run().lastInsertRowid; + userB = db.prepare("INSERT INTO users (username, password_hash, role, active) VALUES ('sb-b','x','user',1)").run().lastInsertRowid; + const insBill = db.prepare( + "INSERT INTO bills (user_id, name, due_day, expected_amount, current_balance, minimum_payment, interest_rate, snowball_include, active) VALUES (?, ?, 1, 0, ?, ?, 0, 1, 1)", + ); + insBill.run(userA, 'Card A', 100000, 20000); // $1000 bal, $200 min + insBill.run(userA, 'Card B', 300000, 20000); // $3000 bal, $200 min +}); +test.after(() => { + closeDb(); + for (const s of ['', '-wal', '-shm']) { try { fs.unlinkSync(dbPath + s); } catch {} } +}); + +test('start plan → enriched with user-scoped current_debts (dollars)', async () => { + const { status, data } = await call('post', '/plans', { userId: userA, body: { method: 'snowball' } }); + assert.equal(status, 201); + assert.equal(data.status, 'active'); + assert.ok(Array.isArray(data.current_debts) && data.current_debts.length === 2); + const cardA = data.current_debts.find(d => d.name === 'Card A'); + assert.equal(cardA.current_balance, 1000, 'balance in dollars (fromCents)'); + assert.equal(cardA.starting_balance, 1000); +}); + +test('pause / resume / complete transitions + state guards + standardized errors', async () => { + const plan = (await call('get', '/plans/active', { userId: userA })).data; + const id = String(plan.id); + + assert.equal((await call('post', '/plans/:id/pause', { userId: userA, params: { id } })).data.status, 'paused'); + + // pausing an already-paused plan → 400 with a standardized {message, code} + const bad = await call('post', '/plans/:id/pause', { userId: userA, params: { id } }); + assert.equal(bad.status, 400); + assert.equal(bad.data.code, 'INVALID_PLAN_STATE'); + assert.match(bad.data.message, /Only active plans can be paused/); + + assert.equal((await call('post', '/plans/:id/resume', { userId: userA, params: { id } })).data.status, 'active'); + assert.equal((await call('post', '/plans/:id/complete', { userId: userA, params: { id } })).data.status, 'completed'); +}); + +test('another user cannot transition my plan (ownership)', async () => { + const start = await call('post', '/plans', { userId: userA, body: {} }); + const id = String(start.data.id); + const foreign = await call('post', '/plans/:id/abandon', { userId: userB, params: { id } }); + assert.equal(foreign.status, 404); + assert.equal(foreign.data.code, 'NOT_FOUND'); + // still active for the real owner + assert.equal((await call('get', '/plans/active', { userId: userA })).data.status, 'active'); +});