diff --git a/routes/matches.js b/routes/matches.js index 9b211bd..d3f5cd1 100644 --- a/routes/matches.js +++ b/routes/matches.js @@ -7,6 +7,7 @@ const { rejectMatchSuggestion, } = require('../services/matchSuggestionService'); const { learnMerchantRuleFromMatch } = require('../services/billMerchantRuleService'); +const { markMatched, markUnmatched } = require('../services/transactionMatchState'); const { serializePayment } = require('../services/paymentValidation'); const { todayLocal } = require('../utils/dates'); @@ -69,11 +70,7 @@ router.post('/confirm', (req, res) => { const paymentForAccounting = db.prepare('SELECT * FROM payments WHERE id = ?').get(payResult.lastInsertRowid); applyBankPaymentAsSourceOfTruth(db, bill, paymentForAccounting); - db.prepare(` - UPDATE transactions - SET matched_bill_id = ?, match_status = 'matched', updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(billId, txId, req.user.id); + markMatched(db, req.user.id, txId, billId); // Learn a merchant→bill rule from this explicit confirmation so future // synced transactions from the same merchant auto-match. Best-effort. @@ -123,11 +120,7 @@ router.post('/:transactionId/unmatch', (req, res) => { UPDATE payments SET deleted_at = datetime('now'), updated_at = datetime('now') WHERE transaction_id = ? AND payment_source = 'transaction_match' AND deleted_at IS NULL `).run(txId); - db.prepare(` - UPDATE transactions - SET matched_bill_id = NULL, match_status = 'unmatched', updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(txId, req.user.id); + markUnmatched(db, req.user.id, txId); db.exec('COMMIT'); res.json({ ok: true }); } catch (err) { diff --git a/routes/payments.js b/routes/payments.js index 45b73a1..bb17ad0 100644 --- a/routes/payments.js +++ b/routes/payments.js @@ -5,6 +5,7 @@ const { getDb } = require('../db/database'); const { computeBalanceDelta, applyBalanceDelta } = require('../services/billsService'); const { validatePaymentInput, serializePayment } = require('../services/paymentValidation'); const { getCycleRange, resolveDueDate } = require('../services/statusService'); +const { markUnmatched } = require('../services/transactionMatchState'); const { markProvisionalManualPaymentsOverridden, reactivatePaymentsOverriddenBy, @@ -177,11 +178,7 @@ router.post('/:id/undo-auto', (req, res) => { } reactivatePaymentsOverriddenBy(db, payment.id); db.prepare("UPDATE payments SET deleted_at = datetime('now') WHERE id = ?").run(payment.id); - db.prepare(` - UPDATE transactions - SET match_status = 'unmatched', matched_bill_id = NULL, updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(payment.transaction_id, req.user.id); + markUnmatched(db, req.user.id, payment.transaction_id); })(); res.json({ success: true }); } catch (err) { diff --git a/routes/transactions.js b/routes/transactions.js index d9eb3f1..30569c4 100644 --- a/routes/transactions.js +++ b/routes/transactions.js @@ -8,6 +8,7 @@ const { } = require('../services/transactionService'); const { checkTransaction: advisoryCheck } = require('../services/advisoryFilterService'); const { getBankSyncConfig } = require('../services/bankSyncConfigService'); +const { markUnmatched } = require('../services/transactionMatchState'); const { ignoreTransaction, matchTransactionToBill, @@ -794,11 +795,7 @@ router.post('/unmatch-bulk', (req, res) => { reactivatePaymentsOverriddenBy(db, payment.id); db.prepare("UPDATE payments SET deleted_at = datetime('now') WHERE id = ?").run(payment.id); } - db.prepare(` - UPDATE transactions - SET match_status = 'unmatched', matched_bill_id = NULL, updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(txId, userId); + markUnmatched(db, userId, txId); results.push({ transaction_id: txId, ok: true }); } else { // Standard service unmatch (restores balance for transaction_match payments) diff --git a/services/transactionMatchService.js b/services/transactionMatchService.js index fe6fe97..167595a 100644 --- a/services/transactionMatchService.js +++ b/services/transactionMatchService.js @@ -11,6 +11,7 @@ const { getTransactionForUser, } = require('./transactionService'); const { serializePayment } = require('./paymentValidation'); +const { markMatched, markUnmatched, markIgnored } = require('./transactionMatchState'); const MATCH_PAYMENT_SOURCE = 'transaction_match'; const MATCH_PAYMENT_METHOD = 'transaction_match'; @@ -255,14 +256,7 @@ function matchTransactionToBill(userId, transactionId, billId, opts = {}) { const bill = getOwnedBill(db, userId, billId); const paymentId = createOrUpdateMatchPayment(db, userId, transaction, bill); - db.prepare(` - UPDATE transactions - SET matched_bill_id = ?, - match_status = 'matched', - ignored = 0, - updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(bill.id, transaction.id, userId); + markMatched(db, userId, transaction.id, bill.id, { resetIgnored: true }); if (opts.learnMerchant) { const { learnMerchantRuleFromMatch } = require('./billMerchantRuleService'); @@ -281,14 +275,7 @@ function unmatchTransaction(userId, transactionId) { const transaction = getOwnedTransaction(db, userId, transactionId); const removedPayment = unlinkPaymentForTransaction(db, userId, transaction.id); - db.prepare(` - UPDATE transactions - SET matched_bill_id = NULL, - match_status = 'unmatched', - ignored = 0, - updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(transaction.id, userId); + markUnmatched(db, userId, transaction.id, { resetIgnored: true }); return responseForTransaction(db, userId, transaction.id, null, { removed_payment: removedPayment }); }); @@ -302,14 +289,7 @@ function ignoreTransaction(userId, transactionId) { const transaction = getOwnedTransaction(db, userId, transactionId); const removedPayment = unlinkPaymentForTransaction(db, userId, transaction.id); - db.prepare(` - UPDATE transactions - SET ignored = 1, - match_status = 'ignored', - matched_bill_id = NULL, - updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(transaction.id, userId); + markIgnored(db, userId, transaction.id); return responseForTransaction(db, userId, transaction.id, null, { removed_payment: removedPayment }); }); @@ -322,14 +302,7 @@ function unignoreTransaction(userId, transactionId) { const tx = db.transaction(() => { const transaction = getOwnedTransaction(db, userId, transactionId); - db.prepare(` - UPDATE transactions - SET ignored = 0, - match_status = 'unmatched', - matched_bill_id = NULL, - updated_at = datetime('now') - WHERE id = ? AND user_id = ? - `).run(transaction.id, userId); + markUnmatched(db, userId, transaction.id, { resetIgnored: true }); return responseForTransaction(db, userId, transaction.id); }); diff --git a/services/transactionMatchState.js b/services/transactionMatchState.js new file mode 100644 index 0000000..9745fcc --- /dev/null +++ b/services/transactionMatchState.js @@ -0,0 +1,52 @@ +'use strict'; + +// Canonical writers for a transaction's match state. +// +// A transaction's match state lives in three columns that must move together: +// match_status 'unmatched' | 'matched' | 'ignored' +// matched_bill_id the bill when matched, else NULL +// ignored 1 when explicitly ignored, else 0 +// +// These were previously updated by copy-pasted inline UPDATEs across routes and +// services, which is exactly how they drift out of sync — e.g. QA-B5-04, where a +// bill purge left match_status='matched' with a NULL bill. Routing every +// single-transaction transition through these helpers keeps the columns +// consistent by construction. All are ownership-scoped (user_id) and return the +// number of rows changed. Bulk transitions (auto-match sweep, retention purge) +// stay in their own services where the WHERE clause is fundamentally different. + +/** + * Mark a transaction matched to a bill. + * @param {boolean} [opts.resetIgnored] also clear the ignored flag (used when + * matching a transaction directly, which implicitly un-ignores it). + */ +function markMatched(db, userId, transactionId, billId, { resetIgnored = false } = {}) { + return db.prepare(` + UPDATE transactions + SET matched_bill_id = ?, match_status = 'matched'${resetIgnored ? ', ignored = 0' : ''}, updated_at = datetime('now') + WHERE id = ? AND user_id = ? + `).run(billId, transactionId, userId).changes; +} + +/** + * Clear a transaction's match — back to 'unmatched' with no bill. + * @param {boolean} [opts.resetIgnored] also clear the ignored flag (un-ignore). + */ +function markUnmatched(db, userId, transactionId, { resetIgnored = false } = {}) { + return db.prepare(` + UPDATE transactions + SET matched_bill_id = NULL, match_status = 'unmatched'${resetIgnored ? ', ignored = 0' : ''}, updated_at = datetime('now') + WHERE id = ? AND user_id = ? + `).run(transactionId, userId).changes; +} + +/** Mark a transaction ignored — dropped from matching, no bill. */ +function markIgnored(db, userId, transactionId) { + return db.prepare(` + UPDATE transactions + SET ignored = 1, match_status = 'ignored', matched_bill_id = NULL, updated_at = datetime('now') + WHERE id = ? AND user_id = ? + `).run(transactionId, userId).changes; +} + +module.exports = { markMatched, markUnmatched, markIgnored }; diff --git a/tests/transactionMatchState.test.js b/tests/transactionMatchState.test.js new file mode 100644 index 0000000..8d59ad1 --- /dev/null +++ b/tests/transactionMatchState.test.js @@ -0,0 +1,80 @@ +'use strict'; + +// IMP-CODE-03: the canonical match-state writers keep match_status, +// matched_bill_id and ignored moving together, so no path can leave the columns +// inconsistent (the QA-B5-04 class of bug). +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-match-state-${process.pid}.sqlite`); +process.env.DB_PATH = dbPath; + +const { getDb, closeDb } = require('../db/database'); +const { markMatched, markUnmatched, markIgnored } = require('../services/transactionMatchState'); + +let db, userId, otherId, billId; + +function newTxn(uid, { status = 'unmatched', bill = null, ignored = 0 } = {}) { + return db.prepare( + 'INSERT INTO transactions (user_id, source_type, provider_transaction_id, amount, match_status, matched_bill_id, ignored) VALUES (?, ?, ?, -1000, ?, ?, ?)', + ).run(uid, 'manual', `ms-${Math.random().toString(36).slice(2)}`, status, bill, ignored).lastInsertRowid; +} +const row = (id) => db.prepare('SELECT match_status, matched_bill_id, ignored FROM transactions WHERE id = ?').get(id); + +test.before(() => { + db = getDb(); + userId = db.prepare("INSERT INTO users (username, password_hash, role, active) VALUES ('ms-user','x','user',1)").run().lastInsertRowid; + otherId = db.prepare("INSERT INTO users (username, password_hash, role, active) VALUES ('ms-other','x','user',1)").run().lastInsertRowid; + billId = db.prepare('INSERT INTO bills (user_id, name, due_day, expected_amount, active) VALUES (?, ?, 1, 1000, 1)').run(userId, 'Bill').lastInsertRowid; +}); + +test.after(() => { + closeDb(); + for (const suffix of ['', '-wal', '-shm']) { try { fs.unlinkSync(dbPath + suffix); } catch {} } +}); + +test('markMatched sets status + bill together', () => { + const id = newTxn(userId); + const changes = markMatched(db, userId, id, billId); + assert.equal(changes, 1); + assert.deepEqual(row(id), { match_status: 'matched', matched_bill_id: billId, ignored: 0 }); +}); + +test('markMatched leaves the ignored flag unless resetIgnored is set', () => { + const kept = newTxn(userId, { ignored: 1, status: 'ignored' }); + markMatched(db, userId, kept, billId); + assert.equal(row(kept).ignored, 1, 'ignored preserved by default'); + + const cleared = newTxn(userId, { ignored: 1, status: 'ignored' }); + markMatched(db, userId, cleared, billId, { resetIgnored: true }); + assert.equal(row(cleared).ignored, 0, 'resetIgnored clears it'); +}); + +test('markUnmatched clears the bill and status (never leaves matched+NULL)', () => { + const id = newTxn(userId, { status: 'matched', bill: billId }); + markUnmatched(db, userId, id); + assert.deepEqual(row(id), { match_status: 'unmatched', matched_bill_id: null, ignored: 0 }); +}); + +test('markUnmatched with resetIgnored un-ignores', () => { + const id = newTxn(userId, { status: 'ignored', ignored: 1 }); + markUnmatched(db, userId, id, { resetIgnored: true }); + assert.deepEqual(row(id), { match_status: 'unmatched', matched_bill_id: null, ignored: 0 }); +}); + +test('markIgnored sets ignored + status, clears bill', () => { + const id = newTxn(userId, { status: 'matched', bill: billId }); + markIgnored(db, userId, id); + assert.deepEqual(row(id), { match_status: 'ignored', matched_bill_id: null, ignored: 1 }); +}); + +test('all writers are ownership-scoped — never touch another user\'s transaction', () => { + const foreign = newTxn(otherId, { status: 'matched', bill: null }); + assert.equal(markMatched(db, userId, foreign, billId), 0); + assert.equal(markUnmatched(db, userId, foreign), 0); + assert.equal(markIgnored(db, userId, foreign), 0); + assert.equal(row(foreign).match_status, 'matched', 'foreign row untouched'); +});