diff --git a/backend/controllers/checklist/checklist-comments-controllers.ts b/backend/controllers/checklist/checklist-comments-controllers.ts index f32c4ff6..29e3c29c 100644 --- a/backend/controllers/checklist/checklist-comments-controllers.ts +++ b/backend/controllers/checklist/checklist-comments-controllers.ts @@ -6,6 +6,20 @@ import { createCommentSchema } from '../../validations/checklist/checklist-schem import { logger } from '../../config/logger.js' import { fetchStoredFile, sendPrivateFile } from '../../services/uploads/private-files.js' +// Answers a refused delete; returns true when it did +function refuseRemove( + res: Response, + result: commentsService.RemoveResult, + what: 'comentario' | 'adjunto' +): boolean { + if (result === 'deleted') return false + if (result === 'not_found') res.status(404).json({ error: `No existe ese ${what}` }) + else if (result === 'closed') + res.status(409).json({ error: 'El día está cerrado: solo un administrador puede borrar' }) + else res.status(403).json({ error: `No tienes permiso para eliminar este ${what}` }) + return true +} + function isAdmin(req: Request): boolean { return req.user?.role?.toLowerCase() === 'admin' } @@ -45,17 +59,13 @@ export async function addCommentController(req: Request, res: Response): Promise // DELETE /api/checklists/:id/steps/:stepId/comments/:commentId export async function deleteCommentController(req: Request, res: Response): Promise { try { - const { id, commentId } = req.params - const deleted = await commentsService.removeComment( - id, + const { commentId } = req.params + const result = await commentsService.removeComment( Number(commentId), req.user!.id, isAdmin(req) ) - if (!deleted) { - res.status(403).json({ error: 'No tienes permiso para eliminar este comentario' }) - return - } + if (refuseRemove(res, result, 'comentario')) return res.json({ success: true }) } catch (err) { logger.error({ err }, '[checklist] deleteComment') @@ -113,17 +123,13 @@ export async function addAttachmentController(req: Request, res: Response): Prom // DELETE /api/checklists/:id/steps/:stepId/attachments/:attachmentId export async function deleteAttachmentController(req: Request, res: Response): Promise { try { - const { id, attachmentId } = req.params - const deleted = await commentsService.removeAttachment( - id, + const { attachmentId } = req.params + const result = await commentsService.removeAttachment( Number(attachmentId), req.user!.id, isAdmin(req) ) - if (!deleted) { - res.status(403).json({ error: 'No tienes permiso para eliminar este adjunto' }) - return - } + if (refuseRemove(res, result, 'adjunto')) return res.json({ success: true }) } catch (err) { logger.error({ err }, '[checklist] deleteAttachment') diff --git a/backend/repositories/checklist/checklist-comments.repository.ts b/backend/repositories/checklist/checklist-comments.repository.ts index 3a715c32..d42b7f8d 100644 --- a/backend/repositories/checklist/checklist-comments.repository.ts +++ b/backend/repositories/checklist/checklist-comments.repository.ts @@ -60,19 +60,27 @@ export async function createComment( return rows[0] } -export async function deleteComment( - commentId: number, - userId: string, - isAdmin: boolean -): Promise { +// The comment with its run, to check who may delete it and whether its day is closed +export async function findComment( + commentId: number +): Promise<{ user_id: string; run_id: number; run_closed: boolean } | null> { const [rows] = await db.execute( - `SELECT user_id FROM checklist_step_comments WHERE id = ?`, + `SELECT c.user_id, c.run_id, r.reset_at IS NOT NULL AS run_closed + FROM checklist_step_comments c + JOIN checklist_runs r ON r.id = c.run_id + WHERE c.id = ?`, [commentId] ) - if (!rows[0]) return false - if (!isAdmin && rows[0].user_id !== userId) return false + if (!rows[0]) return null + return { + user_id: rows[0].user_id, + run_id: rows[0].run_id, + run_closed: Boolean(rows[0].run_closed), + } +} + +export async function deleteComment(commentId: number): Promise { await db.execute(`DELETE FROM checklist_step_comments WHERE id = ?`, [commentId]) - return true } // ── Attachments ─────────────────────────────────────────── @@ -113,10 +121,15 @@ export async function createAttachment( return rows[0] } -export async function findAttachment(attachmentId: number): Promise { - const [rows] = await db.execute( - `SELECT a.*, u.username FROM checklist_step_attachments a - JOIN users u ON u.id = a.user_id WHERE a.id = ?`, +export async function findAttachment( + attachmentId: number +): Promise<(Attachment & { run_closed: number }) | null> { + const [rows] = await db.execute<(Attachment & { run_closed: number })[]>( + `SELECT a.*, u.username, r.reset_at IS NOT NULL AS run_closed + FROM checklist_step_attachments a + JOIN users u ON u.id = a.user_id + JOIN checklist_runs r ON r.id = a.run_id + WHERE a.id = ?`, [attachmentId] ) return rows[0] ?? null diff --git a/backend/services/checklist/checklist-comments.service.ts b/backend/services/checklist/checklist-comments.service.ts index dec4d479..d728eb4b 100644 --- a/backend/services/checklist/checklist-comments.service.ts +++ b/backend/services/checklist/checklist-comments.service.ts @@ -11,6 +11,21 @@ async function resolveRun(checklistId: string): Promise<{ id: number }> { return run } +// Result of deleting a comment or an attachment. A closed day (its run has +// reset_at) is history: since 2026-10-11 only admin may delete there +export type RemoveResult = 'deleted' | 'not_found' | 'forbidden' | 'closed' + +function canRemove( + item: { user_id: string; run_closed: boolean | number }, + userId: string, + isAdmin: boolean +): RemoveResult | null { + if (isAdmin) return null + if (item.user_id !== userId) return 'forbidden' + if (item.run_closed) return 'closed' + return null +} + // ── Comments ────────────────────────────────────────────── export async function getComments(checklistId: string, stepId: string): Promise { @@ -31,16 +46,20 @@ export async function addComment( } export async function removeComment( - checklistId: string, commentId: number, userId: string, isAdmin: boolean -): Promise { - const run = await resolveRun(checklistId) - const deleted = await commentsRepo.deleteComment(commentId, userId, isAdmin) - if (deleted) - await checklistRepo.logEvent(run.id, null, userId, 'comment', { deleted_comment_id: commentId }) - return deleted +): Promise { + const comment = await commentsRepo.findComment(commentId) + if (!comment) return 'not_found' + const refused = canRemove(comment, userId, isAdmin) + if (refused) return refused + await commentsRepo.deleteComment(commentId) + // Logged in the comment's own run, not in today's + await checklistRepo.logEvent(comment.run_id, null, userId, 'comment', { + deleted_comment_id: commentId, + }) + return 'deleted' } // ── Attachments ─────────────────────────────────────────── @@ -78,21 +97,21 @@ export async function addAttachment( } export async function removeAttachment( - checklistId: string, attachmentId: number, userId: string, isAdmin: boolean -): Promise { +): Promise { const attachment = await commentsRepo.findAttachment(attachmentId) - if (!attachment) return false - if (!isAdmin && attachment.user_id !== userId) return false + if (!attachment) return 'not_found' + const refused = canRemove(attachment, userId, isAdmin) + if (refused) return refused await CloudinaryService.deleteImage(attachment.public_id) await commentsRepo.deleteAttachment(attachmentId) - const run = await resolveRun(checklistId) - await checklistRepo.logEvent(run.id, null, userId, 'attach', { + // Logged in the attachment's own run, not in today's + await checklistRepo.logEvent(attachment.run_id, null, userId, 'attach', { deleted_attachment_id: attachmentId, }) - return true + return 'deleted' } // Stored Cloudinary URL of an attachment, for the server to serve it diff --git a/backend/tests/checklist/closed-day-deletes.test.ts b/backend/tests/checklist/closed-day-deletes.test.ts new file mode 100644 index 00000000..e6295280 --- /dev/null +++ b/backend/tests/checklist/closed-day-deletes.test.ts @@ -0,0 +1,67 @@ +// tests/checklist/closed-day-deletes.test.ts +// Regression tests: the author of a comment or a photo could delete it from a +// closed day (rewriting its history), and the deletion was logged in today's +// run as if it were a new one. Decided on 2026-10-11: on a closed day only +// admin deletes; the event goes to the item's own run. + +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('../../repositories/checklist/checklist-comments.repository.js', () => ({ + findComment: vi.fn(), + deleteComment: vi.fn(), + findAttachment: vi.fn(), + deleteAttachment: vi.fn(), +})) +vi.mock('../../repositories/checklist/checklist-repository.js', () => ({ + logEvent: vi.fn(), + getOrCreateRun: vi.fn(async () => ({ id: 999 })), // today's run: must not be used +})) +vi.mock('../../services/blacklist/cloudinary-service.js', () => ({ + CloudinaryService: { deleteImage: vi.fn() }, +})) + +const service = await import('../../services/checklist/checklist-comments.service.js') +const repo = await import('../../repositories/checklist/checklist-comments.repository.js') +const checklistRepo = await import('../../repositories/checklist/checklist-repository.js') + +const closed = { user_id: 'author', run_id: 7, run_closed: true } +const open = { user_id: 'author', run_id: 8, run_closed: false } + +describe('deleting comments and photos', () => { + beforeEach(() => vi.clearAllMocks()) + + it('the author cannot delete from a closed day', async () => { + vi.mocked(repo.findComment).mockResolvedValue(closed) + vi.mocked(repo.findAttachment).mockResolvedValue({ ...closed, public_id: 'p' } as never) + + expect(await service.removeComment(1, 'author', false)).toBe('closed') + expect(await service.removeAttachment(1, 'author', false)).toBe('closed') + expect(repo.deleteComment).not.toHaveBeenCalled() + expect(repo.deleteAttachment).not.toHaveBeenCalled() + }) + + it('admin can, and the event goes to the closed run', async () => { + vi.mocked(repo.findComment).mockResolvedValue(closed) + + expect(await service.removeComment(1, 'admin-id', true)).toBe('deleted') + expect(checklistRepo.logEvent).toHaveBeenCalledWith(7, null, 'admin-id', 'comment', { + deleted_comment_id: 1, + }) + expect(checklistRepo.getOrCreateRun).not.toHaveBeenCalled() + }) + + it('on an open day the author deletes, someone else cannot', async () => { + vi.mocked(repo.findComment).mockResolvedValue(open) + + expect(await service.removeComment(1, 'someone-else', false)).toBe('forbidden') + expect(await service.removeComment(1, 'author', false)).toBe('deleted') + expect(checklistRepo.logEvent).toHaveBeenCalledWith(8, null, 'author', 'comment', { + deleted_comment_id: 1, + }) + }) + + it('an unknown id is not found', async () => { + vi.mocked(repo.findComment).mockResolvedValue(null) + expect(await service.removeComment(1, 'author', false)).toBe('not_found') + }) +})