From 4ec0004867e8f9ba11e85917d11575a3c86f45a0 Mon Sep 17 00:00:00 2001 From: Matt Date: Sat, 9 May 2026 19:50:51 +0200 Subject: [PATCH] =?UTF-8?q?fix(documents):=20folder=20service=20=C2=B7=20a?= =?UTF-8?q?udit=20+=20portId=20+=20audit-log=20placement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review followups on e9251a3: - Move createAuditLog OUT of the deleteFolderSoftRescue transaction callback so a rolled-back transaction can't leave a phantom audit row. Pattern matches clients.service.ts, expense-dedup.service.ts. - Add portId filter to the moveFolder ancestor-walk findFirst — defense-in-depth so corrupted parentId pointing at another port short-circuits the walk instead of silently traversing it. - Drop updatedAt bump on rescued documents — folder rescue is an administrative storage op, not a content change; bumping made every rescued doc appear "recently modified" in list views. - Add userId param + audit-log emission on renameFolder and moveFolder for parity with createFolder + deleteFolderSoftRescue. Tests updated to pass TEST_USER_ID as the new 4th arg. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/services/document-folders.service.ts | 68 ++++++++++++------- .../integration/document-folders-crud.test.ts | 14 ++-- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/lib/services/document-folders.service.ts b/src/lib/services/document-folders.service.ts index 35c52b8a..2b132d59 100644 --- a/src/lib/services/document-folders.service.ts +++ b/src/lib/services/document-folders.service.ts @@ -121,6 +121,7 @@ export async function renameFolder( portId: string, folderId: string, newName: string, + userId: string, ): Promise { const trimmed = newName.trim(); if (!trimmed) throw new ValidationError('Folder name cannot be empty'); @@ -138,6 +139,17 @@ export async function renameFolder( .where(eq(documentFolders.id, folderId)) .returning(); if (!updated) throw new NotFoundError('Folder'); + + void createAuditLog({ + userId, + portId, + action: 'update', + entityType: 'document_folder', + entityId: folderId, + oldValue: { name: existing.name }, + newValue: { name: trimmed }, + }); + return updated; } catch (err) { if (isSiblingNameConflict(err)) { @@ -157,6 +169,7 @@ export async function moveFolder( portId: string, folderId: string, newParentId: string | null, + userId: string, ): Promise { if (newParentId === folderId) { throw new ValidationError('Cannot move a folder under itself (cycle)'); @@ -185,7 +198,7 @@ export async function moveFolder( if (seen.has(cursor)) break; // defensive — pre-existing cycle, bail seen.add(cursor); const next = await db.query.documentFolders.findFirst({ - where: eq(documentFolders.id, cursor), + where: and(eq(documentFolders.id, cursor), eq(documentFolders.portId, portId)), columns: { parentId: true }, }); cursor = next?.parentId ?? null; @@ -199,6 +212,18 @@ export async function moveFolder( .where(eq(documentFolders.id, folderId)) .returning(); if (!updated) throw new NotFoundError('Folder'); + + void createAuditLog({ + userId, + portId, + action: 'update', + entityType: 'document_folder', + entityId: folderId, + oldValue: { parentId: folder.parentId }, + newValue: { parentId: newParentId }, + metadata: { type: 'folder_move' }, + }); + return updated; } catch (err) { if (isSiblingNameConflict(err)) { @@ -219,39 +244,34 @@ export async function deleteFolderSoftRescue( folderId: string, userId: string, ): Promise { + const folder = await db.query.documentFolders.findFirst({ + where: and(eq(documentFolders.id, folderId), eq(documentFolders.portId, portId)), + }); + if (!folder) throw new NotFoundError('Folder'); + + const newParent = folder.parentId; // null = re-parent to root + await db.transaction(async (tx) => { - const folder = await tx.query.documentFolders.findFirst({ - where: and(eq(documentFolders.id, folderId), eq(documentFolders.portId, portId)), - }); - if (!folder) throw new NotFoundError('Folder'); - - const newParent = folder.parentId; // null = re-parent to root - await tx .update(documentFolders) .set({ parentId: newParent, updatedAt: new Date() }) - .where( - and( - eq(documentFolders.parentId, folderId), - eq(documentFolders.portId, portId), - ), - ); + .where(and(eq(documentFolders.parentId, folderId), eq(documentFolders.portId, portId))); await tx .update(documents) - .set({ folderId: newParent, updatedAt: new Date() }) + .set({ folderId: newParent }) .where(and(eq(documents.folderId, folderId), eq(documents.portId, portId))); await tx.delete(documentFolders).where(eq(documentFolders.id, folderId)); + }); - void createAuditLog({ - userId, - portId, - action: 'delete', - entityType: 'document_folder', - entityId: folderId, - oldValue: { name: folder.name, parentId: folder.parentId }, - metadata: { rescuedTo: newParent }, - }); + void createAuditLog({ + userId, + portId, + action: 'delete', + entityType: 'document_folder', + entityId: folderId, + oldValue: { name: folder.name, parentId: folder.parentId }, + metadata: { rescuedTo: newParent }, }); } diff --git a/tests/integration/document-folders-crud.test.ts b/tests/integration/document-folders-crud.test.ts index 68119b4b..d6b19404 100644 --- a/tests/integration/document-folders-crud.test.ts +++ b/tests/integration/document-folders-crud.test.ts @@ -116,7 +116,7 @@ describe('document-folders service · renameFolder', () => { const folder = await createFolder(portId, TEST_USER_ID, { name: 'Old', parentId: null }); const before = folder.updatedAt.getTime(); await new Promise((r) => setTimeout(r, 10)); - const renamed = await renameFolder(portId, folder.id, 'New'); + const renamed = await renameFolder(portId, folder.id, 'New', TEST_USER_ID); expect(renamed.name).toBe('New'); expect(renamed.updatedAt.getTime()).toBeGreaterThan(before); }); @@ -124,13 +124,13 @@ describe('document-folders service · renameFolder', () => { it('rejects rename to an existing sibling name', async () => { await createFolder(portId, TEST_USER_ID, { name: 'Existing', parentId: null }); const folder = await createFolder(portId, TEST_USER_ID, { name: 'Mine', parentId: null }); - await expect(renameFolder(portId, folder.id, 'Existing')).rejects.toThrow(/already exists/i); + await expect(renameFolder(portId, folder.id, 'Existing', TEST_USER_ID)).rejects.toThrow(/already exists/i); }); it('throws NotFound when the folder belongs to another port', async () => { const otherPort = await makePort(); const folder = await createFolder(otherPort.id, TEST_USER_ID, { name: 'X', parentId: null }); - await expect(renameFolder(portId, folder.id, 'Y')).rejects.toThrow(/couldn't find/i); + await expect(renameFolder(portId, folder.id, 'Y', TEST_USER_ID)).rejects.toThrow(/couldn't find/i); }); }); @@ -146,14 +146,14 @@ describe('document-folders service · moveFolder', () => { it('moves a folder under a new parent', async () => { const root = await createFolder(portId, TEST_USER_ID, { name: 'Root', parentId: null }); const orphan = await createFolder(portId, TEST_USER_ID, { name: 'Orphan', parentId: null }); - const moved = await moveFolder(portId, orphan.id, root.id); + const moved = await moveFolder(portId, orphan.id, root.id, TEST_USER_ID); expect(moved.parentId).toBe(root.id); }); it('moves a folder back to root with parentId=null', async () => { const root = await createFolder(portId, TEST_USER_ID, { name: 'Root', parentId: null }); const child = await createFolder(portId, TEST_USER_ID, { name: 'Child', parentId: root.id }); - const moved = await moveFolder(portId, child.id, null); + const moved = await moveFolder(portId, child.id, null, TEST_USER_ID); expect(moved.parentId).toBeNull(); }); @@ -162,11 +162,11 @@ describe('document-folders service · moveFolder', () => { const b = await createFolder(portId, TEST_USER_ID, { name: 'B', parentId: a.id }); const c = await createFolder(portId, TEST_USER_ID, { name: 'C', parentId: b.id }); // moving A under C would create A → B → C → A - await expect(moveFolder(portId, a.id, c.id)).rejects.toThrow(/cycle/i); + await expect(moveFolder(portId, a.id, c.id, TEST_USER_ID)).rejects.toThrow(/cycle/i); }); it('rejects moving a folder under itself', async () => { const a = await createFolder(portId, TEST_USER_ID, { name: 'A', parentId: null }); - await expect(moveFolder(portId, a.id, a.id)).rejects.toThrow(/cycle/i); + await expect(moveFolder(portId, a.id, a.id, TEST_USER_ID)).rejects.toThrow(/cycle/i); }); });