From 355b844e42172dc3f34e340392dc974a8ca7090a Mon Sep 17 00:00:00 2001 From: razyd Date: Sun, 11 Oct 2026 12:35:24 +0200 Subject: [PATCH] fix(cashier): final voucher states, atomic voucher creation, guarded totals - cashier-voucher-repository.ts: only a pending voucher can change its amount, reason or notes, be justified or be cancelled; the write itself checks the status. createForShift locks the shift, counts its vouchers and creates and links the new one in one transaction; create, canAddVoucherToShift and associateWithShift go. - cashier-voucher-controller.ts: create uses createForShift and answers domain errors with their status. - cashier-errors.ts: the new voucher errors. - cashier-routes.ts: GET /daily, /shifts/:id/history and /vouchers/stats require canViewReports, like the other history and report routes. - voucher-lifecycle.test.ts: transitions, the locked creation, the limit and the three guarded routes. Why: a cancelled voucher could be justified, a justified one cancelled and the amount of either changed; the 5-voucher limit could be passed in parallel and going over it left a voucher without a shift; reception saw history and totals meant for admin. Decided on 2026-10-11: justified and cancelled are final. Checked with curl against the local database: reception gets 403 on the three routes and admin 200; a justified voucher refuses a new amount, cancel and justify; a voucher on a missing shift answers 404 and creates nothing. --- backend/controllers/cashier/cashier-errors.ts | 4 + .../cashier/cashier-voucher-controller.ts | 17 +-- .../cashier/cashier-voucher-repository.ts | 115 ++++++++-------- backend/routes/cashier/cashier-routes.ts | 11 +- .../tests/cashier/voucher-lifecycle.test.ts | 124 ++++++++++++++++++ 5 files changed, 199 insertions(+), 72 deletions(-) create mode 100644 backend/tests/cashier/voucher-lifecycle.test.ts diff --git a/backend/controllers/cashier/cashier-errors.ts b/backend/controllers/cashier/cashier-errors.ts index 6631cc78..e848e0cc 100644 --- a/backend/controllers/cashier/cashier-errors.ts +++ b/backend/controllers/cashier/cashier-errors.ts @@ -17,6 +17,10 @@ export const CASHIER_DOMAIN_ERRORS: Record = { 'El vale ya está justificado': 409, 'El vale ya está cancelado': 409, 'Solo se pueden eliminar vales pendientes': 409, + 'Solo se pueden modificar vales pendientes': 409, + 'Solo se pueden justificar vales pendientes': 409, + 'Solo se pueden cancelar vales pendientes': 409, + 'El turno ya tiene el máximo de 5 vales': 400, } /** Known domain errors keep their message; anything else is a generic 500 (no SQL details). */ diff --git a/backend/controllers/cashier/cashier-voucher-controller.ts b/backend/controllers/cashier/cashier-voucher-controller.ts index 3ea6576e..865e6f7b 100644 --- a/backend/controllers/cashier/cashier-voucher-controller.ts +++ b/backend/controllers/cashier/cashier-voucher-controller.ts @@ -133,18 +133,8 @@ export class CashierVoucherController { } const voucherData = { ...parsed.data, created_by: userId } - // Verificar límite de 5 vales por turno - const canAdd = await CashierVoucherRepository.canAddVoucherToShift(parseInt(shiftId)) - if (!canAdd) { - res.status(400).json({ error: 'El turno ya tiene el máximo de 5 vales' }) - return - } - - // Crear vale - const voucher = await CashierVoucherRepository.create(voucherData) - - // Asociar con turno - await CashierVoucherRepository.associateWithShift(voucher.id, parseInt(shiftId)) + // Crear el vale y asociarlo al turno (límite de 5, en una transacción) + const voucher = await CashierVoucherRepository.createForShift(voucherData, parseInt(shiftId)) // Registrar en historial await CashierHistoryRepository.create({ @@ -158,8 +148,7 @@ export class CashierVoucherController { res.status(201).json(voucher) } catch (error) { - logger.error({ err: error }, 'Error al crear vale') - res.status(500).json({ error: 'Error al crear vale' }) + sendCashierError(res, error, 'Error al crear vale') } } diff --git a/backend/repositories/cashier/cashier-voucher-repository.ts b/backend/repositories/cashier/cashier-voucher-repository.ts index 43848cb4..4dd944c1 100644 --- a/backend/repositories/cashier/cashier-voucher-repository.ts +++ b/backend/repositories/cashier/cashier-voucher-repository.ts @@ -11,32 +11,6 @@ import { } from '../../models/cashier/index.js' export class CashierVoucherRepository { - /** - * Crear un vale - */ - static async create(data: CreateVoucherDTO): Promise { - const query = ` - INSERT INTO cashier_vouchers ( - amount, - reason, - created_by, - notes - ) VALUES (?, ?, ?, ?) - ` - - const [result] = await db.query(query, [ - data.amount, - data.reason, - data.created_by, - data.notes || null, - ]) - - const created = await this.getById(result.insertId) - if (!created) throw new Error('Error al recuperar vale creado') - - return created - } - /** * Obtener vale por ID */ @@ -199,14 +173,19 @@ export class CashierVoucherRepository { throw new Error('No hay campos para actualizar') } + // Justified and cancelled are final (2026-10-11): only a pending voucher changes + const current = await this.getById(id) + if (!current) throw new Error('Vale no encontrado') + const query = ` - UPDATE cashier_vouchers + UPDATE cashier_vouchers SET ${updates.join(', ')} - WHERE id = ? + WHERE id = ? AND status = 'pending' ` params.push(id) - await db.query(query, params) + const [result] = await db.query(query, params) + if (result.affectedRows === 0) throw new Error('Solo se pueden modificar vales pendientes') const updated = await this.getById(id) if (!updated) throw new Error('Error al recuperar vale actualizado') @@ -221,19 +200,23 @@ export class CashierVoucherRepository { const voucher = await this.getById(id) if (!voucher) throw new Error('Vale no encontrado') if (voucher.status === 'justified') throw new Error('El vale ya está justificado') + if (voucher.status !== 'pending') throw new Error('Solo se pueden justificar vales pendientes') const connection = await db.getConnection() try { await connection.beginTransaction() - // Actualizar estado del vale - await connection.query( - `UPDATE cashier_vouchers - SET status = 'justified', justified_at = NOW() - WHERE id = ?`, + // Actualizar estado del vale (solo si sigue pendiente) + const [updated] = await connection.query( + `UPDATE cashier_vouchers + SET status = 'justified', justified_at = NOW() + WHERE id = ? AND status = 'pending'`, [id] ) + if (updated.affectedRows === 0) { + throw new Error('Solo se pueden justificar vales pendientes') + } // Asociar vale con turno await connection.query( @@ -262,14 +245,16 @@ export class CashierVoucherRepository { const voucher = await this.getById(id) if (!voucher) throw new Error('Vale no encontrado') if (voucher.status === 'cancelled') throw new Error('El vale ya está cancelado') + if (voucher.status !== 'pending') throw new Error('Solo se pueden cancelar vales pendientes') const query = ` UPDATE cashier_vouchers SET status = 'cancelled', cancelled_at = NOW() - WHERE id = ? + WHERE id = ? AND status = 'pending' ` - await db.query(query, [id]) + const [result] = await db.query(query, [id]) + if (result.affectedRows === 0) throw new Error('Solo se pueden cancelar vales pendientes') // Eliminar asociación con turnos si existe await db.query('DELETE FROM cashier_shift_vouchers WHERE voucher_id = ?', [id]) @@ -356,29 +341,49 @@ export class CashierVoucherRepository { } /** - * Verificar si se puede añadir más vales a un turno (límite: 5) + * Crear un vale en un turno: comprueba el límite de 5 y lo asocia en una sola + * transacción, con el turno bloqueado, para que dos altas a la vez no lo salten + * ni dejen un vale sin turno */ - static async canAddVoucherToShift(shiftId: number): Promise { - const count = await this.countByShift(shiftId) - return count < 5 - } + static async createForShift(data: CreateVoucherDTO, shiftId: number): Promise { + const connection = await db.getConnection() + let voucherId: number + try { + await connection.beginTransaction() - /** - * Asociar vale existente con un turno - */ - static async associateWithShift(voucherId: number, shiftId: number): Promise { - // Verificar límite de 5 vales por turno - const canAdd = await this.canAddVoucherToShift(shiftId) - if (!canAdd) { - throw new Error('El turno ya tiene el máximo de 5 vales') - } + const [shift] = await connection.query( + 'SELECT id FROM cashier_shifts WHERE id = ? FOR UPDATE', + [shiftId] + ) + if (!shift[0]) throw new Error('Turno no encontrado') - const query = ` - INSERT INTO cashier_shift_vouchers (shift_id, voucher_id) - VALUES (?, ?) - ` + const [count] = await connection.query( + 'SELECT COUNT(*) AS total FROM cashier_shift_vouchers WHERE shift_id = ?', + [shiftId] + ) + if (Number(count[0].total) >= 5) throw new Error('El turno ya tiene el máximo de 5 vales') + + const [result] = await connection.query( + 'INSERT INTO cashier_vouchers (amount, reason, created_by, notes) VALUES (?, ?, ?, ?)', + [data.amount, data.reason, data.created_by, data.notes || null] + ) + voucherId = result.insertId + await connection.query( + 'INSERT INTO cashier_shift_vouchers (shift_id, voucher_id) VALUES (?, ?)', + [shiftId, voucherId] + ) - await db.query(query, [shiftId, voucherId]) + await connection.commit() + } catch (error) { + await connection.rollback() + throw error + } finally { + connection.release() + } + + const created = await this.getById(voucherId) + if (!created) throw new Error('Error al recuperar vale creado') + return created } /** diff --git a/backend/routes/cashier/cashier-routes.ts b/backend/routes/cashier/cashier-routes.ts index 882e4718..5de1de08 100644 --- a/backend/routes/cashier/cashier-routes.ts +++ b/backend/routes/cashier/cashier-routes.ts @@ -100,7 +100,7 @@ router.get('/daily/:date/summary', authenticateToken, CashierDailyController.get * GET /api/cashier/daily * Listar días con paginación */ -router.get('/daily', authenticateToken, CashierDailyController.getAll) +router.get('/daily', authenticateToken, canViewReports, CashierDailyController.getAll) /** * GET /api/cashier/reports/monthly/:year/:month @@ -163,7 +163,12 @@ router.delete('/shifts/:id', authenticateToken, isAdmin, CashierShiftController. * GET /api/cashier/shifts/:id/history * Obtener historial de un turno */ -router.get('/shifts/:id/history', authenticateToken, CashierShiftController.getHistory) +router.get( + '/shifts/:id/history', + authenticateToken, + canViewReports, + CashierShiftController.getHistory +) // ============================================ // 🎫 VOUCHER ROUTES (Vales) @@ -185,7 +190,7 @@ router.get('/vouchers/active', authenticateToken, CashierVoucherController.getAc * GET /api/cashier/vouchers/stats * Obtener estadísticas de vales */ -router.get('/vouchers/stats', authenticateToken, CashierVoucherController.getStats) +router.get('/vouchers/stats', authenticateToken, canViewReports, CashierVoucherController.getStats) /** * GET /api/cashier/vouchers/:id diff --git a/backend/tests/cashier/voucher-lifecycle.test.ts b/backend/tests/cashier/voucher-lifecycle.test.ts new file mode 100644 index 00000000..c36f20d3 --- /dev/null +++ b/backend/tests/cashier/voucher-lifecycle.test.ts @@ -0,0 +1,124 @@ +// tests/cashier/voucher-lifecycle.test.ts +// Regression tests for the cashier vouchers and reports: +// - a cancelled voucher could be justified, a justified one cancelled and the +// amount of either changed. Decided on 2026-10-11: justified and cancelled +// are final, only a pending voucher changes. +// - the 5-voucher limit was checked before writing, without a lock, and the +// voucher was created before linking it, so going over the limit left it +// without a shift. +// - GET /daily, /shifts/:id/history and /vouchers/stats only needed a session. + +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { readFileSync } from 'node:fs' +import { join, dirname } from 'node:path' +import { fileURLToPath } from 'node:url' + +const connection = { + beginTransaction: vi.fn(), + commit: vi.fn(), + rollback: vi.fn(), + release: vi.fn(), + query: vi.fn(), +} + +vi.mock('../../config/db.js', () => ({ + default: { getConnection: vi.fn(async () => connection), query: vi.fn() }, +})) + +const { CashierVoucherRepository: repo } = + await import('../../repositories/cashier/cashier-voucher-repository.js') +const { default: db } = await import('../../config/db.js') + +const voucher = (status: string) => ({ id: 4, status, amount: 20, shift_id: 1 }) as never + +describe('voucher lifecycle', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.restoreAllMocks() + connection.query.mockReset() + }) + + it.each(['justified', 'cancelled'])('a %s voucher cannot change its amount', async (status) => { + vi.spyOn(repo, 'getById').mockResolvedValue(voucher(status)) + vi.mocked(db.query).mockResolvedValue([{ affectedRows: 0 }] as never) + await expect(repo.update(4, { amount: 99 })).rejects.toThrow( + 'Solo se pueden modificar vales pendientes' + ) + expect(String(vi.mocked(db.query).mock.calls[0][0])).toMatch(/status = 'pending'/) + }) + + it('a cancelled voucher cannot be justified', async () => { + vi.spyOn(repo, 'getById').mockResolvedValue(voucher('cancelled')) + await expect(repo.justify(4, 1)).rejects.toThrow('Solo se pueden justificar vales pendientes') + expect(connection.query).not.toHaveBeenCalled() + }) + + it('a justified voucher cannot be cancelled', async () => { + vi.spyOn(repo, 'getById').mockResolvedValue(voucher('justified')) + await expect(repo.cancel(4)).rejects.toThrow('Solo se pueden cancelar vales pendientes') + expect(db.query).not.toHaveBeenCalled() + }) + + it('a pending voucher is justified only if it is still pending when written', async () => { + vi.spyOn(repo, 'getById').mockResolvedValue(voucher('pending')) + connection.query.mockResolvedValueOnce([{ affectedRows: 0 }]) // changed meanwhile + await expect(repo.justify(4, 1)).rejects.toThrow('Solo se pueden justificar vales pendientes') + expect(connection.rollback).toHaveBeenCalledOnce() + }) +}) + +describe('creating a voucher in a shift', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.restoreAllMocks() + connection.query.mockReset() + }) + + it('locks the shift, counts inside the transaction and links the voucher', async () => { + vi.spyOn(repo, 'getById').mockResolvedValue(voucher('pending')) + connection.query + .mockResolvedValueOnce([[{ id: 1 }]]) + .mockResolvedValueOnce([[{ total: 4 }]]) + .mockResolvedValueOnce([{ insertId: 4 }]) + .mockResolvedValueOnce([{}]) + + await repo.createForShift({ amount: 20, reason: 'Taxi', created_by: 'u1' } as never, 1) + + const sql = connection.query.mock.calls.map((c) => String(c[0])) + expect(sql[0]).toMatch(/FROM cashier_shifts WHERE id = \? FOR UPDATE/) + expect(sql[2]).toMatch(/INSERT INTO cashier_vouchers/) + expect(sql[3]).toMatch(/INSERT INTO cashier_shift_vouchers/) + expect(connection.commit).toHaveBeenCalledOnce() + }) + + it('refuses the sixth voucher and creates nothing', async () => { + connection.query.mockResolvedValueOnce([[{ id: 1 }]]).mockResolvedValueOnce([[{ total: 5 }]]) + + await expect( + repo.createForShift({ amount: 20, reason: 'Taxi', created_by: 'u1' } as never, 1) + ).rejects.toThrow('El turno ya tiene el máximo de 5 vales') + expect(connection.query).toHaveBeenCalledTimes(2) + expect(connection.rollback).toHaveBeenCalledOnce() + }) +}) + +describe('cashier history and totals', () => { + it('need canViewReports', () => { + const routes = readFileSync( + join( + dirname(fileURLToPath(import.meta.url)), + '..', + '..', + 'routes', + 'cashier', + 'cashier-routes.ts' + ), + 'utf8' + ).replace(/\s+/g, ' ') + for (const path of ["'/daily',", "'/shifts/:id/history',", "'/vouchers/stats',"]) { + expect(routes, path).toMatch( + new RegExp(`router\\.get\\( ?${path} authenticateToken, canViewReports,`) + ) + } + }) +})