From 6248bb9825784635805035a2d6f7b8155f71e837 Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Tue, 21 Jul 2026 19:58:25 +0200 Subject: [PATCH] fix(email): preserve read state and remove source on cross-account move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving a message across the account boundary (own ↔ shared folder, or between two owners' shared folders) left the original in the source folder and showed the moved copy as unread. Same-account moves were fine. Cause (verified against a live Stalwart): - Email/copy drops keywords unless the create sets them, so the copy lost $seen and arrived unread. - onSuccessDestroyOriginal is unreliable — the implicit destroy reports notFound and leaves the original behind (flaky), so the move duplicated. Fix (copyEmailAcrossAccounts): read the source keywords and carry them into the Email/copy create, then destroy the original with an explicit Email/set on the source account instead of onSuccessDestroyOriginal. Tests: 08-shared-moves now asserts the source is gone and the read state survives on every cross-account case, and adds a cross-owner shared → shared move (alice's folder → bob's folder). Confirmed red on the old code (3 cross-account cases fail), green with the fix. --- integration/tests/08-shared-moves.spec.ts | 82 +++++++++++++++++------ lib/jmap/client.ts | 23 ++++++- 2 files changed, 83 insertions(+), 22 deletions(-) diff --git a/integration/tests/08-shared-moves.spec.ts b/integration/tests/08-shared-moves.spec.ts index 0ac079ff..32901562 100644 --- a/integration/tests/08-shared-moves.spec.ts +++ b/integration/tests/08-shared-moves.spec.ts @@ -13,39 +13,50 @@ import { /** * Moving mail across the own-account / shared-folder boundary, in both - * directions, and between two shared folders. The move is driven from the list - * context menu's "Move to" submenu; the authoritative check is the server-side - * mailbox the message ends up in, with the reliably-updating (own-account) - * counters checked in the UI too. + * directions, and between two shared folders (same owner and across owners). + * The move is driven from the list context menu's "Move to" submenu; the + * authoritative check is the server-side mailbox the message ends up in. Each + * cross-account case also asserts the source copy is gone (no duplicate) and + * the read state survives the move (Email/copy drops keywords unless carried). */ -const { alice, carol } = ACCOUNTS; +const { alice, bob, carol } = ACCOUNTS; const subj = (l: string) => `IT ${l} ${Date.now()}`; test.describe('Shared-folder moves', () => { - let ja: JmapClient; // owner + let ja: JmapClient; // owner A + let jb: JmapClient; // owner B (cross-owner shared → shared) let jc: JmapClient; // grantee let teamA: string; let teamB: string; + let teamC: string; // owned by bob test.beforeEach(async () => { ja = await JmapClient.connect(alice.email, alice.password); + jb = await JmapClient.connect(bob.email, bob.password); jc = await JmapClient.connect(carol.email, carol.password); await ja.reset(); + await jb.reset(); await jc.reset(); teamA = await ja.createSharedFolder('TeamA', carol.email); teamB = await ja.createSharedFolder('TeamB', carol.email); + teamC = await jb.createSharedFolder('TeamC', carol.email); }); - async function seedInto(mailboxId: string, subject: string, owner = ja): Promise { - const acct = owner === ja ? alice : carol; + // Seed a message into a mailbox and mark it read, so a lost $seen after the + // move is observable as the moved copy coming back unread. + async function seedRead(mailboxId: string, subject: string, owner = ja): Promise { + const acct = owner === ja ? alice : owner === jb ? bob : carol; await sendMail({ from: acct.email, authPass: acct.password, to: acct.email, subject, body: 'x' }); const m = await owner.waitForEmail(subject); await owner.moveEmail(m.id, mailboxId); + await owner.setSeen(m.id, true); } - test('shared folder A -> shared folder B', async ({ page }) => { + const seenOf = (m: any) => Boolean(m?.keywords?.$seen); + + test('shared folder A -> shared folder B (same owner)', async ({ page }) => { const s = subj('mv-a2b'); - await seedInto(teamA, s); + await seedRead(teamA, s); await login(page, carol); await expandSharedFolders(page, alice.email); @@ -56,13 +67,15 @@ test.describe('Shared-folder moves', () => { await moveEmailTo(page, s, dest); await page.waitForTimeout(1500); - expect(await ja.findEmailBySubject(s, teamB), 'message in TeamB').toBeTruthy(); + const inB = await ja.findEmailBySubject(s, teamB); + expect(inB, 'message in TeamB').toBeTruthy(); expect(await ja.findEmailBySubject(s, teamA), 'message left TeamA').toBeFalsy(); + expect(seenOf(inB), 'read state kept').toBe(true); }); - test('shared folder B -> shared folder A', async ({ page }) => { + test('shared folder B -> shared folder A (same owner)', async ({ page }) => { const s = subj('mv-b2a'); - await seedInto(teamB, s); + await seedRead(teamB, s); await login(page, carol); await expandSharedFolders(page, alice.email); @@ -73,8 +86,32 @@ test.describe('Shared-folder moves', () => { await moveEmailTo(page, s, dest); await page.waitForTimeout(1500); - expect(await ja.findEmailBySubject(s, teamA), 'message in TeamA').toBeTruthy(); + const inA = await ja.findEmailBySubject(s, teamA); + expect(inA, 'message in TeamA').toBeTruthy(); expect(await ja.findEmailBySubject(s, teamB), 'message left TeamB').toBeFalsy(); + expect(seenOf(inA), 'read state kept').toBe(true); + }); + + // Cross-owner shared → shared: source is alice's account, destination is bob's, + // so this exercises the true cross-account Email/copy + destroy path. + test('shared folder (owner A) -> shared folder (owner B)', async ({ page }) => { + const s = subj('mv-a2c'); + await seedRead(teamA, s); + + await login(page, carol); + await expandSharedFolders(page, alice.email); + await expandSharedFolders(page, bob.email); + const dest = await folderMailboxId(page, { name: 'TeamC', shared: true }); + await openFolder(page, { name: 'TeamA', shared: true }); + await forceSync(page); + + await moveEmailTo(page, s, dest); + await page.waitForTimeout(2000); + + const inC = await jb.findEmailBySubject(s, teamC); + expect(inC, 'message in bob TeamC').toBeTruthy(); + expect(await ja.findEmailBySubject(s, teamA), 'message left alice TeamA').toBeFalsy(); + expect(seenOf(inC), 'read state kept').toBe(true); }); // The "Move to" submenu relocates a message across the account boundary @@ -82,7 +119,8 @@ test.describe('Shared-folder moves', () => { test('own account -> shared folder', async ({ page }) => { const s = subj('mv-own2sh'); await sendMail({ from: carol.email, authPass: carol.password, to: carol.email, subject: s, body: 'x' }); - await jc.waitForEmail(s); + const own = await jc.waitForEmail(s); + await jc.setSeen(own.id, true); await login(page, carol); await expandSharedFolders(page, alice.email); @@ -93,13 +131,15 @@ test.describe('Shared-folder moves', () => { await moveEmailTo(page, s, dest); await page.waitForTimeout(2000); - // Expected (once supported): the message moves to the owner's shared TeamA. - expect(await ja.findEmailBySubject(s, teamA), 'message in shared TeamA').toBeTruthy(); + const inTeam = await ja.findEmailBySubject(s, teamA); + expect(inTeam, 'message in shared TeamA').toBeTruthy(); + expect(await jc.findEmailBySubject(s), 'message left own account').toBeFalsy(); + expect(seenOf(inTeam), 'read state kept').toBe(true); }); test('shared folder -> own account', async ({ page }) => { const s = subj('mv-sh2own'); - await seedInto(teamA, s); + await seedRead(teamA, s); await login(page, carol); await expandSharedFolders(page, alice.email); @@ -110,7 +150,9 @@ test.describe('Shared-folder moves', () => { await moveEmailTo(page, s, dest); await page.waitForTimeout(2000); - // Expected (once supported): the message arrives in carol's own Inbox. - expect(await jc.findEmailBySubject(s), 'message in own account').toBeTruthy(); + const inOwn = await jc.findEmailBySubject(s); + expect(inOwn, 'message in own account').toBeTruthy(); + expect(await ja.findEmailBySubject(s, teamA), 'message left shared TeamA').toBeFalsy(); + expect(seenOf(inOwn), 'read state kept').toBe(true); }); }); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index cddecc5b..ca18ff54 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -6400,12 +6400,18 @@ export class JMAPClient implements IJMAPClient { toAccountId: string, destMailboxId: string, ): Promise { + // Email/copy drops keywords unless the create sets them, so carry the + // source's over — otherwise the moved message shows up as unread. + const srcResp = await this.request([ + ["Email/get", { accountId: fromAccountId, ids: [emailId], properties: ["keywords"] }, "0"], + ]); + const keywords = srcResp.methodResponses?.[0]?.[1]?.list?.[0]?.keywords ?? {}; + const response = await this.request([ ["Email/copy", { fromAccountId, accountId: toAccountId, - create: { c: { id: emailId, mailboxIds: { [destMailboxId]: true } } }, - onSuccessDestroyOriginal: true, + create: { c: { id: emailId, mailboxIds: { [destMailboxId]: true }, keywords } }, }, "0"], ]); const res = response.methodResponses?.[0]?.[1]; @@ -6417,6 +6423,19 @@ export class JMAPClient implements IJMAPClient { if (!id) { throw new Error("Email/copy succeeded but no ID returned"); } + + // onSuccessDestroyOriginal is unreliable on Stalwart (implicit destroy reports + // notFound and leaves the original behind), so remove the source explicitly. + const delResp = await this.request([ + ["Email/set", { accountId: fromAccountId, destroy: [emailId] }, "0"], + ]); + const notDestroyed = delResp.methodResponses?.[0]?.[1]?.notDestroyed?.[emailId]; + if (notDestroyed) { + throw new Error( + notDestroyed.description || notDestroyed.type || + "Copied email but failed to remove the original from the source folder", + ); + } return id; }