diff --git a/src/api/__tests__/files-trash.test.ts b/src/api/__tests__/files-trash.test.ts index 9b6ff70f..39056804 100644 --- a/src/api/__tests__/files-trash.test.ts +++ b/src/api/__tests__/files-trash.test.ts @@ -34,7 +34,14 @@ function serve(method: string, args: Record) { if (method === 'FileNode/get') return { list: store.map((n) => ({ ...n })) }; calls.push(args); const out: Record = {}; + // Like Stalwart, two nodes of one folder can't share a name. + const clashes = (parentId: string | null, name: string, self?: string) => + store.some((n) => n.parentId === parentId && n.name === name && n.id !== self); for (const [key, props] of Object.entries((args.create ?? {}) as Record)) { + if (clashes(props.parentId ?? null, props.name)) { + (out.notCreated ??= {})[key] = { type: 'alreadyExists', description: 'taken' }; + continue; + } const id = `n${nextId++}`; store.push({ id, parentId: props.parentId ?? null, name: props.name, blobId: props.blobId ?? null }); (out.created ??= {})[key] = { id }; @@ -45,6 +52,11 @@ function serve(method: string, args: Record) { (out.notUpdated ??= {})[id] = { type: 'forbidden', description: 'no' }; continue; } + const parentId = 'parentId' in patch ? patch.parentId : node.parentId; + if (clashes(parentId, patch.name ?? node.name, id)) { + (out.notUpdated ??= {})[id] = { type: 'alreadyExists', description: 'taken' }; + continue; + } Object.assign(node, patch); (out.updated ??= {})[id] = null; } @@ -164,7 +176,7 @@ describe('restoreFromTrash', () => { await moveToTrash(nodes, nodes.filter((n) => n.parentId === 'docs'), 1730112000007); nodes = listed(); const batch = store.find((n) => n.name === '1730112000007~docs')!; - await restoreFromTrash(nodes, trashedItems(nodes)); + await restoreFromTrash(trashedItems(nodes)); expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a.txt' }); expect(byId('b')).toMatchObject({ parentId: 'docs', name: 'b.txt' }); expect(byId(batch.id)).toBeUndefined(); @@ -178,7 +190,7 @@ describe('restoreFromTrash', () => { await moveToTrash(nodes, nodes.filter((n) => n.parentId === 'docs'), 1730112000007); nodes = listed(); const batch = store.find((n) => n.name === '1730112000007~docs')!; - await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'a')); + await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'a')); expect(byId(batch.id)).toBeDefined(); expect(byId('b')!.parentId).toBe(batch.id); }); @@ -191,26 +203,59 @@ describe('restoreFromTrash', () => { await moveToTrash(nodes, nodes.filter((n) => n.id === 'docs'), 1730112000008); nodes = listed(); // Docs is in the trash itself now: a.txt goes to the root. - await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'a')); + await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'a')); expect(byId('a')).toMatchObject({ parentId: null, name: 'a.txt' }); nodes = listed(); - await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'docs')); + await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'docs')); await moveToTrash(listed(), listed().filter((n) => n.id === 'a2'), 1730112000009); store.push({ id: 'a3', parentId: 'docs', name: 'a.txt', blobId: 'ba3' }); nodes = listed(); - await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'a2')); + await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'a2')); expect(byId('a2')).toMatchObject({ parentId: 'docs', name: 'a (1).txt' }); }); it('undoes a delete with the listing from before it, without renaming', async () => { const before = listed(); const { done } = await moveToTrash(before, before.filter((n) => n.parentId === 'docs'), 1730112000007); - await restoreFromTrash(before, done); + await restoreFromTrash(done); expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a.txt' }); expect(byId('b')).toMatchObject({ parentId: 'docs', name: 'b.txt' }); expect(store.some((n) => n.name === '1730112000007~docs')).toBe(false); }); + + it('undoes into the root when the folder went to the trash after the delete', async () => { + const before = listed(); + const { done } = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007); + await moveToTrash(listed(), listed().filter((n) => n.id === 'docs'), 1730112000008); + const { done: back } = await restoreFromTrash(done); + expect(back).toHaveLength(1); + expect(byId('a')).toMatchObject({ parentId: null, name: 'a.txt' }); + }); + + it('undoes under a free name when the old one was taken after the delete', async () => { + const before = listed(); + const { done } = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007); + store.push({ id: 'a2', parentId: 'docs', name: 'a.txt', blobId: 'ba2' }); + await restoreFromTrash(done); + expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a (1).txt' }); + expect(byId('a2')).toMatchObject({ parentId: 'docs', name: 'a.txt' }); + }); + + it('leaves an item alone that left the trash since the rows were listed', async () => { + let nodes = listed(); + await moveToTrash(nodes, nodes.filter((n) => n.id === 'a'), 1730112000007); + nodes = listed(); + const rows = trashedItems(nodes); + // Restored on another device and moved on from there. + byId('a')!.parentId = 'top-dir'; + store.push({ id: 'top-dir', parentId: null, name: 'Elsewhere', blobId: null }); + const sent = calls.length; + const { done } = await restoreFromTrash(rows); + expect(done).toEqual([]); + expect(calls.slice(sent).filter((c) => c.update)).toEqual([]); + expect(byId('a')!.parentId).toBe('top-dir'); + }); }); describe('purgeFromTrash and emptyTrash', () => { diff --git a/src/api/__tests__/live-stalwart-files.test.ts b/src/api/__tests__/live-stalwart-files.test.ts index 85dd47b9..7600ac61 100644 --- a/src/api/__tests__/live-stalwart-files.test.ts +++ b/src/api/__tests__/live-stalwart-files.test.ts @@ -298,14 +298,14 @@ live('live Stalwart files trash', () => { const item = all.find((i) => i.node.name === 'trash-me.txt' && i.fromParentId === scratch)!; const sibling = all.find((i) => i.node.name === 'Trash me' && i.batchId === item.batchId)!; let list = await own(); - const { done } = await restoreFromTrash(list, [item]); + const { done } = await restoreFromTrash([item]); expect(done).toHaveLength(1); list = await own(); expect(list.find((n) => n.id === item.node.id)).toMatchObject({ name: 'trash-me.txt', parentId: scratch }); // The other item still holds the trash folder. expect(list.some((n) => n.id === item.batchId)).toBe(true); - await restoreFromTrash(list, [sibling]); + await restoreFromTrash([sibling]); list = await own(); expect(list.find((n) => n.id === sibling.node.id)?.parentId).toBe(scratch); expect(list.some((n) => n.id === item.batchId)).toBe(false); @@ -324,7 +324,7 @@ live('live Stalwart files trash', () => { await deleteFileNodes([sub.id]); list = await own(); - await restoreFromTrash(list, [(await trashed(a.id))!, (await trashed(b.id))!]); + await restoreFromTrash([(await trashed(a.id))!, (await trashed(b.id))!]); list = await own(); expect(list.find((n) => n.id === a.id)).toMatchObject({ name: 'clash (1).txt', parentId: scratch }); const orphan = list.find((n) => n.id === b.id)!; @@ -342,7 +342,7 @@ live('live Stalwart files trash', () => { const { done } = await moveToTrash(list, [list.find((n) => n.id === file.id)!]); batches.add(done[0].batchId!); // The rows on screen still show it where it was. - await restoreFromTrash(list, done); + await restoreFromTrash(done); const after = await own(); expect(after.find((n) => n.id === file.id)).toMatchObject({ name: 'undo-me.txt', parentId: scratch }); expect(after.some((n) => n.id === done[0].batchId)).toBe(false); diff --git a/src/api/files-trash.ts b/src/api/files-trash.ts index f7db1464..a51af3da 100644 --- a/src/api/files-trash.ts +++ b/src/api/files-trash.ts @@ -100,23 +100,34 @@ export async function moveToTrash( /** * Put items of the trash back into the folder each came from, or the root * when that folder is gone or in the trash itself. A name taken there by now - * gets " (1)" and so on. Their trash folders go once they are empty. Throws - * when nothing went. + * gets " (1)" and so on. Their trash folders go once they are empty. + * + * The drive is read again first, and that is what counts: the listing on + * screen (for an undo, the one from before the delete) misses a folder + * trashed since and a name taken since. Items no longer in the trash are left + * where they are. Throws when nothing of what is still there went. */ -export async function restoreFromTrash(nodes: FileNode[], items: TrashedItem[]): Promise { +export async function restoreFromTrash(items: TrashedItem[]): Promise { const result: TrashWriteResult = { done: [], refused: [] }; if (items.length === 0) return result; - const hidden = trashSubtreeIds(nodes, findTrashFolder(nodes)?.id); - const restoring = new Set(items.map((item) => item.node.id)); - // Names taken in each folder, so a restored item never clashes; the items - // themselves don't count (after an undo the listing may still show them - // where they were). + const nodes = await getAllFileNodes(); + const trash = findTrashFolder(nodes); + const hidden = trashSubtreeIds(nodes, trash?.id); + // The items as the trash holds them now: their name and trash folder may + // have changed on another device. + const current = new Map(trashedItems(nodes, trash).map((item) => [item.node.id, item])); + const pending = [...new Set(items.map((item) => item.node.id))].flatMap((id) => { + const item = current.get(id); + return item ? [item] : []; + }); + if (pending.length === 0) return result; + // Names taken in each folder, so a restored item never clashes. const taken = new Map>(); const namesIn = (parentId: string | null): Set => { let names = taken.get(parentId); if (!names) { names = new Set(nodes - .filter((n) => (n.parentId ?? null) === parentId && !n.isShared && !hidden.has(n.id) && !restoring.has(n.id)) + .filter((n) => (n.parentId ?? null) === parentId && !n.isShared && !hidden.has(n.id)) .map((n) => n.name)); taken.set(parentId, names); } @@ -125,7 +136,7 @@ export async function restoreFromTrash(nodes: FileNode[], items: TrashedItem[]): const updates: Record>> = {}; const targets = new Map(); - for (const item of items) { + for (const item of pending) { const target = restoreTarget(item, nodes); const used = namesIn(target); const name = getUniqueName(item.node.name, used); @@ -141,7 +152,7 @@ export async function restoreFromTrash(nodes: FileNode[], items: TrashedItem[]): if (restored.size === 0) { throw result.refused[0] ?? new JMAPSetError('setError', 'Failed to restore'); } - for (const item of items.filter((i) => restored.has(i.node.id))) { + for (const item of pending.filter((i) => restored.has(i.node.id))) { const name = updates[item.node.id].name ?? item.node.name; result.done.push({ ...item, node: { ...item.node, name, parentId: targets.get(item.node.id) ?? null } }); } diff --git a/src/screens/FilesScreen.tsx b/src/screens/FilesScreen.tsx index e671c927..8edef477 100644 --- a/src/screens/FilesScreen.tsx +++ b/src/screens/FilesScreen.tsx @@ -744,12 +744,12 @@ export default function FilesScreen() { } }, [uploading, currentParentId, allNodes, loadFiles, t]); - // Undo of a delete: the items go back from the trash, the way Restore puts - // them back, judged against the drive as it was before the delete. - const undoTrash = useCallback(async (listing: FileNode[], items: TrashedItem[]) => { + // Undo of a delete: the items go back from the trash the way Restore puts + // them back, judged against the drive as it is by then. + const undoTrash = useCallback(async (items: TrashedItem[]) => { try { - await restoreFromTrash(listing, items); - toast.success(t('files.undo_success', 'Action undone')); + const { done } = await restoreFromTrash(items); + if (done.length > 0) toast.success(t('files.undo_success', 'Action undone')); } catch (e) { toast.error(t('files.undo_error', 'Failed to undo'), fileErrorMessage(e, t)); } @@ -770,7 +770,7 @@ export default function FilesScreen() { } toast.success( t('files.moved_to_trash', '{count, plural, one {Moved to trash} other {# items moved to trash}}', { count: done.length }), - { action: { label: t('files.undo', 'Undo'), onPress: () => void undoTrash(listing, done) } }, + { action: { label: t('files.undo', 'Undo'), onPress: () => void undoTrash(done) } }, ); } catch (e) { Alert.alert(t('files.delete_error', 'Failed to delete'), fileErrorMessage(e, t)); @@ -812,16 +812,18 @@ export default function FilesScreen() { if (items.length === 0) return; clearSelection(); try { - const { done, refused } = await restoreFromTrash(allNodes, items); + const { done, refused } = await restoreFromTrash(items); if (refused.length > 0) { Alert.alert(t('files.restore_error', "Couldn't restore"), fileErrorMessage(refused[0], t)); } - toast.success(t('files.restored', '{count, plural, one {Item restored} other {# items restored}}', { count: done.length })); + if (done.length > 0) { + toast.success(t('files.restored', '{count, plural, one {Item restored} other {# items restored}}', { count: done.length })); + } } catch (e) { Alert.alert(t('files.restore_error', "Couldn't restore"), fileErrorMessage(e, t)); } await loadFiles('refresh'); - }, [allNodes, clearSelection, loadFiles, t]); + }, [clearSelection, loadFiles, t]); const performEmptyTrash = useCallback(async () => { setConfirmEmptyTrash(false);