fix(documents): folder service · audit + portId + audit-log placement
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) <noreply@anthropic.com>
This commit is contained in:
@@ -121,6 +121,7 @@ export async function renameFolder(
|
|||||||
portId: string,
|
portId: string,
|
||||||
folderId: string,
|
folderId: string,
|
||||||
newName: string,
|
newName: string,
|
||||||
|
userId: string,
|
||||||
): Promise<DocumentFolder> {
|
): Promise<DocumentFolder> {
|
||||||
const trimmed = newName.trim();
|
const trimmed = newName.trim();
|
||||||
if (!trimmed) throw new ValidationError('Folder name cannot be empty');
|
if (!trimmed) throw new ValidationError('Folder name cannot be empty');
|
||||||
@@ -138,6 +139,17 @@ export async function renameFolder(
|
|||||||
.where(eq(documentFolders.id, folderId))
|
.where(eq(documentFolders.id, folderId))
|
||||||
.returning();
|
.returning();
|
||||||
if (!updated) throw new NotFoundError('Folder');
|
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;
|
return updated;
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
if (isSiblingNameConflict(err)) {
|
if (isSiblingNameConflict(err)) {
|
||||||
@@ -157,6 +169,7 @@ export async function moveFolder(
|
|||||||
portId: string,
|
portId: string,
|
||||||
folderId: string,
|
folderId: string,
|
||||||
newParentId: string | null,
|
newParentId: string | null,
|
||||||
|
userId: string,
|
||||||
): Promise<DocumentFolder> {
|
): Promise<DocumentFolder> {
|
||||||
if (newParentId === folderId) {
|
if (newParentId === folderId) {
|
||||||
throw new ValidationError('Cannot move a folder under itself (cycle)');
|
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
|
if (seen.has(cursor)) break; // defensive — pre-existing cycle, bail
|
||||||
seen.add(cursor);
|
seen.add(cursor);
|
||||||
const next = await db.query.documentFolders.findFirst({
|
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 },
|
columns: { parentId: true },
|
||||||
});
|
});
|
||||||
cursor = next?.parentId ?? null;
|
cursor = next?.parentId ?? null;
|
||||||
@@ -199,6 +212,18 @@ export async function moveFolder(
|
|||||||
.where(eq(documentFolders.id, folderId))
|
.where(eq(documentFolders.id, folderId))
|
||||||
.returning();
|
.returning();
|
||||||
if (!updated) throw new NotFoundError('Folder');
|
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;
|
return updated;
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
if (isSiblingNameConflict(err)) {
|
if (isSiblingNameConflict(err)) {
|
||||||
@@ -219,39 +244,34 @@ export async function deleteFolderSoftRescue(
|
|||||||
folderId: string,
|
folderId: string,
|
||||||
userId: string,
|
userId: string,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
|
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) => {
|
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
|
await tx
|
||||||
.update(documentFolders)
|
.update(documentFolders)
|
||||||
.set({ parentId: newParent, updatedAt: new Date() })
|
.set({ parentId: newParent, updatedAt: new Date() })
|
||||||
.where(
|
.where(and(eq(documentFolders.parentId, folderId), eq(documentFolders.portId, portId)));
|
||||||
and(
|
|
||||||
eq(documentFolders.parentId, folderId),
|
|
||||||
eq(documentFolders.portId, portId),
|
|
||||||
),
|
|
||||||
);
|
|
||||||
|
|
||||||
await tx
|
await tx
|
||||||
.update(documents)
|
.update(documents)
|
||||||
.set({ folderId: newParent, updatedAt: new Date() })
|
.set({ folderId: newParent })
|
||||||
.where(and(eq(documents.folderId, folderId), eq(documents.portId, portId)));
|
.where(and(eq(documents.folderId, folderId), eq(documents.portId, portId)));
|
||||||
|
|
||||||
await tx.delete(documentFolders).where(eq(documentFolders.id, folderId));
|
await tx.delete(documentFolders).where(eq(documentFolders.id, folderId));
|
||||||
|
});
|
||||||
|
|
||||||
void createAuditLog({
|
void createAuditLog({
|
||||||
userId,
|
userId,
|
||||||
portId,
|
portId,
|
||||||
action: 'delete',
|
action: 'delete',
|
||||||
entityType: 'document_folder',
|
entityType: 'document_folder',
|
||||||
entityId: folderId,
|
entityId: folderId,
|
||||||
oldValue: { name: folder.name, parentId: folder.parentId },
|
oldValue: { name: folder.name, parentId: folder.parentId },
|
||||||
metadata: { rescuedTo: newParent },
|
metadata: { rescuedTo: newParent },
|
||||||
});
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -116,7 +116,7 @@ describe('document-folders service · renameFolder', () => {
|
|||||||
const folder = await createFolder(portId, TEST_USER_ID, { name: 'Old', parentId: null });
|
const folder = await createFolder(portId, TEST_USER_ID, { name: 'Old', parentId: null });
|
||||||
const before = folder.updatedAt.getTime();
|
const before = folder.updatedAt.getTime();
|
||||||
await new Promise((r) => setTimeout(r, 10));
|
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.name).toBe('New');
|
||||||
expect(renamed.updatedAt.getTime()).toBeGreaterThan(before);
|
expect(renamed.updatedAt.getTime()).toBeGreaterThan(before);
|
||||||
});
|
});
|
||||||
@@ -124,13 +124,13 @@ describe('document-folders service · renameFolder', () => {
|
|||||||
it('rejects rename to an existing sibling name', async () => {
|
it('rejects rename to an existing sibling name', async () => {
|
||||||
await createFolder(portId, TEST_USER_ID, { name: 'Existing', parentId: null });
|
await createFolder(portId, TEST_USER_ID, { name: 'Existing', parentId: null });
|
||||||
const folder = await createFolder(portId, TEST_USER_ID, { name: 'Mine', 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 () => {
|
it('throws NotFound when the folder belongs to another port', async () => {
|
||||||
const otherPort = await makePort();
|
const otherPort = await makePort();
|
||||||
const folder = await createFolder(otherPort.id, TEST_USER_ID, { name: 'X', parentId: null });
|
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 () => {
|
it('moves a folder under a new parent', async () => {
|
||||||
const root = await createFolder(portId, TEST_USER_ID, { name: 'Root', parentId: null });
|
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 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);
|
expect(moved.parentId).toBe(root.id);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('moves a folder back to root with parentId=null', async () => {
|
it('moves a folder back to root with parentId=null', async () => {
|
||||||
const root = await createFolder(portId, TEST_USER_ID, { name: 'Root', parentId: null });
|
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 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();
|
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 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 });
|
const c = await createFolder(portId, TEST_USER_ID, { name: 'C', parentId: b.id });
|
||||||
// moving A under C would create A → B → C → A
|
// 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 () => {
|
it('rejects moving a folder under itself', async () => {
|
||||||
const a = await createFolder(portId, TEST_USER_ID, { name: 'A', parentId: null });
|
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);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user