diff --git a/src/api/__tests__/email-destroy-partial.test.ts b/src/api/__tests__/email-destroy-partial.test.ts new file mode 100644 index 00000000..fde642bb --- /dev/null +++ b/src/api/__tests__/email-destroy-partial.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('../jmap-client', () => ({ + jmapClient: { + accountId: 'acc-1', + request: vi.fn(), + getMaxObjectsInGet: vi.fn(() => 500), + getMaxObjectsInSet: vi.fn(() => 2), + }, +})); + +import { jmapClient } from '../jmap-client'; +import { destroyEmails } from '../email'; +import { failedIdsOf } from '../jmap-result'; + +const mockRequest = jmapClient.request as ReturnType; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe('destroyEmails', () => { + it('sends every slice and names only the refused messages', async () => { + mockRequest.mockImplementation(async ([[, args]]: Array<[string, { destroy: string[] }, string]>) => { + const refused: string[] = args.destroy.filter((id) => id === 'e2'); + const gone: string[] = args.destroy.filter((id) => id === 'e4'); + return { + methodResponses: [['Email/set', { + destroyed: args.destroy.filter((id) => !refused.includes(id) && !gone.includes(id)), + notDestroyed: Object.fromEntries([ + ...refused.map((id) => [id, { type: 'forbidden' }]), + ...gone.map((id) => [id, { type: 'notFound' }]), + ]), + }, '0']], + }; + }); + const err = await destroyEmails(['e1', 'e2', 'e3', 'e4']).then(() => null, (e: unknown) => e); + expect(mockRequest).toHaveBeenCalledTimes(2); + // A message that is already gone counts as deleted. + expect(failedIdsOf(err)).toEqual(['e2']); + }); +}); diff --git a/src/api/email.ts b/src/api/email.ts index 55786467..2dbfb11a 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -1270,9 +1270,25 @@ export async function setEmailMailboxes( export async function destroyEmails(ids: string[], accountIdOverride?: string): Promise { if (ids.length === 0) return; const accountId = accountIdOverride ?? jmapClient.accountId; + // Every slice goes out before a refusal is thrown, and the error names the + // ids that are still there (failedIds): the server destroyed the rest, and + // the list must drop those rather than keep them until the next refresh + // (webmail ac99c68ef). + let refusal: JMAPSetError | null = null; + const failed: string[] = []; for (const slice of batched(ids, maxInSet())) { const res = await jmapClient.request([['Email/set', { accountId, destroy: slice }, '0']]); - assertEmailsDestroyed(requireMethodResult(res, '0', 'Email/set')); + try { + assertEmailsDestroyed(requireMethodResult(res, '0', 'Email/set')); + } catch (err) { + if (!(err instanceof JMAPSetError)) throw err; + refusal ??= err; + failed.push(...(err.failedIds ?? [])); + } + } + if (refusal) { + refusal.failedIds = failed; + throw refusal; } } @@ -1282,9 +1298,12 @@ function assertEmailsDestroyed(body: Record): void { const real = Object.entries(notDestroyed).filter(([, err]) => err?.type !== 'notFound'); if (real.length > 0) { const [id, err] = real[0]; - throw new Error( + const error = new JMAPSetError( + err.type || 'setError', `Failed to delete ${real.length} email(s), first: ${id} – ${err.type || 'unknown'}${err.description ? ` (${err.description})` : ''}`, ); + error.failedIds = real.map(([failedId]) => failedId); + throw error; } } diff --git a/src/stores/__tests__/email-store.test.ts b/src/stores/__tests__/email-store.test.ts index 0298fc12..0a2805a9 100644 --- a/src/stores/__tests__/email-store.test.ts +++ b/src/stores/__tests__/email-store.test.ts @@ -1032,6 +1032,27 @@ describe('email-store', () => { expect(mockPatchKeywords).toHaveBeenCalledWith(['e1'], { $seen: true }, undefined); useSettingsStore.getState().updateSetting('deleteAction', 'trash'); }); + + it('drops the rows the server deleted when it refused others of the selection', async () => { + const { JMAPSetError } = await import('../../api/jmap-result'); + const refusal = new JMAPSetError('forbidden', 'Failed to delete 1 email(s), first: e2 – forbidden'); + refusal.failedIds = ['e2']; + (emailApi.deleteEmails as ReturnType).mockRejectedValueOnce(refusal); + useEmailStore.setState({ + mailboxes: [ + { id: 'mb-1', name: 'Inbox', role: 'inbox', isShared: false } as any, + { id: 'mb-trash', name: 'Trash', role: 'trash', isShared: false } as any, + ], + currentMailboxId: 'mb-trash', + emails: [ + { id: 'e1', keywords: {}, mailboxIds: { 'mb-trash': true } } as any, + { id: 'e2', keywords: {}, mailboxIds: { 'mb-trash': true } } as any, + ], + }); + + await expect(useEmailStore.getState().deleteEmailsBatch(['e1', 'e2'], 'mb-trash', 'mb-trash')).rejects.toBe(refusal); + expect(useEmailStore.getState().emails.map((e) => e.id)).toEqual(['e2']); + }); }); describe('bulk keyword actions', () => { diff --git a/src/stores/email-store.ts b/src/stores/email-store.ts index 0968034b..8e7926af 100644 --- a/src/stores/email-store.ts +++ b/src/stores/email-store.ts @@ -1885,43 +1885,63 @@ export const useEmailStore = create()( patch: markReadPatch, })), ]; + // The server refuses messages one by one and applies the rest. The + // refused ones keep their rows, the others are reconciled as done, and + // the refusal is still reported (webmail ac99c68ef): before, one refused + // message kept every row of the selection, the deleted ones included. + let refusal: unknown = null; + const kept = new Set(); + const step = async (run: () => Promise, keepRefused: boolean) => { + try { + await run(); + } catch (err) { + const failed = failedIdsOf(err); + if (!failed) throw err; + refusal ??= err; + if (keepRefused) for (const id of failed) kept.add(id); + } + }; await applyOrQueueBatch(ops, async () => { if (toDestroy.length > 0) { - await apiDeleteEmails(toDestroy.map((e) => e.id), trash.id, trash.id, trash.accountId); + await step(() => apiDeleteEmails(toDestroy.map((e) => e.id), trash.id, trash.id, trash.accountId), true); } if (toTrash.length > 0) { - await apiMoveEmails(toTrash.map((e) => e.id), source.id, trash.id, source.accountId); + await step(() => apiMoveEmails(toTrash.map((e) => e.id), source.id, trash.id, source.accountId), true); } if (toMarkRead.length > 0) { - await patchKeywordsForEmails([...markReadIds], markReadPatch, source.accountId); + // Moved but left unread: the row has still left the folder. + await step(() => patchKeywordsForEmails([...markReadIds], markReadPatch, source.accountId), false); } }); + const destroyed = toDestroy.filter((e) => !kept.has(e.id)); + const trashed = toTrash.filter((e) => !kept.has(e.id)); - if (toDestroy.length > 0) dropFromCache(toDestroy.map((e) => e.id), trash.accountId); - for (const e of toTrash) { + if (destroyed.length > 0) dropFromCache(destroyed.map((e) => e.id), trash.accountId); + for (const e of trashed) { patchCache(e.id, { mailboxIds: mailboxesAfterMove(e.mailboxIds, source.id, trash.id), ...(markReadIds.has(e.id) ? { keywords: markReadPatch } : {}), }, source.accountId); } - const removed = new Set(targets.map(rowKeyOf)); + const removed = new Set([...destroyed, ...trashed].map(rowKeyOf)); set({ emails: get().emails.filter((e) => !removed.has(rowKeyOf(e))) }); // Only the moved-to-trash items are recoverable; destroyed ones are gone. - if (toTrash.length > 0) { + if (trashed.length > 0) { set({ pendingUndo: { kind: 'delete', - label: toTrash.length === 1 + label: trashed.length === 1 ? t('email_list.moved_to_trash', 'Email moved to Trash') - : t('email_list.moved_to_trash_count', `${toTrash.length} emails moved to Trash`, { count: toTrash.length }), + : t('email_list.moved_to_trash_count', `${trashed.length} emails moved to Trash`, { count: trashed.length }), createdAt: Date.now(), accountId: source.accountId, - items: toTrash.map((e) => ({ email: e, originalMailboxIds: { ...e.mailboxIds } })), + items: trashed.map((e) => ({ email: e, originalMailboxIds: { ...e.mailboxIds } })), }, }); } + if (refusal) throw refusal; }, setKeywordForEmails: async (emailIds, token, on, viewed) => {