From 05e3605ec7f54db3a974d251ca6749dfc3aa2bc0 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:16:43 +0200 Subject: [PATCH] fix: keep a group rename when a membership change of the group uploads first #34 Accepting a membership patch of a group card wrote the server's title over the device's and cleared DIRTY. A rename that had not reached the server in the same run, because the server refused it or the user made it after the group's own upload was planned, was silently undone. The accepted write now keeps a pending rename that the accepted card does not show: it stores the new shadow but leaves the title and DIRTY, so the rename uploads against it. A card that shows the device's title, or a different one from a concurrent server rename, is taken as before. Checked with a planner test and end to end through the engine with a refused rename and a membership change in the same run. --- .../__tests__/contacts/groups.test.ts | 19 +++++++++++++ .../__tests__/contacts/real-engine.test.ts | 27 ++++++++++++++++++- src/device-sync/contacts/groups.ts | 13 ++++++--- 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/device-sync/__tests__/contacts/groups.test.ts b/src/device-sync/__tests__/contacts/groups.test.ts index c83c2637..d9ac7869 100644 --- a/src/device-sync/__tests__/contacts/groups.test.ts +++ b/src/device-sync/__tests__/contacts/groups.test.ts @@ -111,6 +111,25 @@ describe('contacts groups: membership edits', () => { expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([]); }); + it('keeps a rename the server does not have when a membership patch of the group is accepted', async () => { + const h = new Harness(); + const gid = h.addCard({ uid: 'urn:uuid:g2', kind: 'group', name: { full: 'Work' }, members: { 'urn:uuid:someone': true } }); + await h.download(gid); + const { rawId } = await h.seed(appleCard()); + const g = await localGroup(h, gid); + h.device.user.addToGroup(rawId, g.groupId); + await h.upload(rawId); + // Renamed after the group's own upload was planned, before the membership phase read it. + h.device.user.updateGroup(g.groupId, { [Groups.TITLE]: 'Colleagues' }); + const read = await localGroup(h, gid); + h.send(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)); + await h.applyOk(h.planner.planGroupAccepted(read, h.card(gid), h.ctx).ops); + expect(await localGroup(h, gid)).toMatchObject({ title: 'Colleagues', dirty: true, shadow: { members: { [appleCard().uid as string]: true } } }); + expect(h.planner.planGroupUpload(await localGroup(h, gid), h.ctx)).toEqual({ + kind: 'upload', actions: [{ kind: 'update', id: gid, patch: { 'name/full': 'Colleagues' } }], + }); + }); + it('sends the whole map for the first member of an empty group', async () => { const h = new Harness(); const empty = h.addCard({ uid: 'urn:uuid:g3', kind: 'group', name: { full: 'Empty' } }); diff --git a/src/device-sync/__tests__/contacts/real-engine.test.ts b/src/device-sync/__tests__/contacts/real-engine.test.ts index 79207b23..758b5e3c 100644 --- a/src/device-sync/__tests__/contacts/real-engine.test.ts +++ b/src/device-sync/__tests__/contacts/real-engine.test.ts @@ -3,10 +3,11 @@ // themselves are tested in the other files of this folder. import { describe, expect, it } from 'vitest'; -import { MimeType } from '../../android-columns'; +import { Groups, MimeType } from '../../android-columns'; import { calendarPlanner } from '../../calendar/planner'; import { contactsPlanner } from '../../contacts/planner'; import { addServerCards, CONTACTS_AUTHORITY, createHarness, type Harness } from '../engine/harness'; +import type { SetTarget } from '../fakes/fake-jmap-server'; function real(options: Parameters[0] = {}): Harness { const h = createHarness(options); @@ -76,4 +77,28 @@ describe('contacts through the engine', () => { // Turning sync off finds nothing waiting. expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 0 }); }); + + it('keeps a group rename the server refused when a membership patch of that group goes through', async () => { + const h = real(); + const [ada] = addServerCards(h, ['Ada Lovelace']); + const group = h.server.addCard('a', { uid: 'g1', kind: 'group', name: { full: 'Friends' }, members: { other: true }, addressBookIds: { [h.book]: true } }); + expect((await h.run()).outcome).toBe('ok'); + const row = () => h.device.rows('groups').find((g) => g.sourceid === `a/${group}`)!; + const groupId = Number(row()._id); + h.device.user.updateGroup(groupId, { [Groups.TITLE]: 'Best friends' }); + h.device.user.addToGroup(h.contactNamed('Ada Lovelace')!.id, groupId); + const rename = (t: SetTarget) => t.op === 'update' && t.id === group && !!t.object && 'name/full' in t.object; + h.server.setErrorFor('ContactCard', 'a', rename, { type: 'invalidProperties', properties: ['name'] }); + + const report = await h.run(); + expect(report.itemErrors).toMatchObject([{ ref: `a/${group}`, type: 'invalidProperties' }]); + const uid = h.server.get('ContactCard', 'a', ada)!.uid as string; + expect(h.server.get('ContactCard', 'a', group)).toMatchObject({ name: { full: 'Friends' }, members: { other: true, [uid]: true } }); + expect(row()).toMatchObject({ [Groups.TITLE]: 'Best friends', [Groups.DIRTY]: 1 }); + + // The rename is still the device's and goes up once the server takes it. + expect((await h.run()).outcome).toBe('ok'); + expect(h.server.get('ContactCard', 'a', group)).toMatchObject({ name: { full: 'Best friends' } }); + expect(row()).toMatchObject({ [Groups.TITLE]: 'Best friends', [Groups.DIRTY]: 0 }); + }); }); diff --git a/src/device-sync/contacts/groups.ts b/src/device-sync/contacts/groups.ts index c9d0b3f7..45829807 100644 --- a/src/device-sync/contacts/groups.ts +++ b/src/device-sync/contacts/groups.ts @@ -193,12 +193,17 @@ export function planGroupAccepted(local: LocalGroup, server: ContactCardWire, ct const identity = objectRef(account, server.id); const raw: WriteRow = { [Groups.SOURCE_ID]: identity, [Groups.SYNC3]: null, [Groups.SYNC4]: null, ...cardColumns(server, ctx) }; const title = groupTitle(server); + const keepDirtyOps = { ref: identity, ops: [updateGroup(local, raw)] }; + // A rename the server still lacks (the upload it accepted was another one, a membership + // patch) stays the device's: title and DIRTY are kept, and it uploads against the new shadow. + const shadowTitle = groupTitle(local.shadow); + const renamed = local.dirty && !!local.title?.trim() && !sameCell(local.title, shadowTitle); + if (renamed && sameCell(title, shadowTitle)) { + return { ops: { ref: identity, ops: [assertGroup(local, null), updateGroup(local, raw)] }, keepDirtyOps }; + } const ops: WriteRow = { ...raw, [Groups.DIRTY]: 0 }; if (!sameCell(local.title, title)) ops[Groups.TITLE] = title ?? ''; - return { - ops: { ref: identity, ops: [assertGroup(local, null), updateGroup(local, ops)] }, - keepDirtyOps: { ref: identity, ops: [updateGroup(local, raw)] }, - }; + return { ops: { ref: identity, ops: [assertGroup(local, null), updateGroup(local, ops)] }, keepDirtyOps }; } /**