refactor(snowball): consolidate plan endpoints, standardize errors, fix N+1 (Snowball #4,#6,#7)
- The four plan-lifecycle routes (pause/resume/complete/abandon) were near
-duplicate copies returning a plain {error} shape; folded into one
transitionPlan(req,res,{allowedFrom,setSql,action,past}) helper that returns
standardizeError {message, code}, keeps the state guards and ownership scoping.
- Standardized the remaining plan endpoints' error responses (start/list/active/
patch) to standardizeError too.
- enrichPlanWithProgress fetched each snapshot bill one-by-one and wasn't user
-scoped; now a single WHERE id IN (…) AND user_id = ? batch.
Test: tests/snowballPlanRoute.test.js (transitions, INVALID_PLAN_STATE guard,
ownership 404, dollar-denominated current_debts). Server 154 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null
parent
db4d33513e
commit
2b315f5d18
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
Loading…
Reference in New Issue