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.

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' } });

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<typeof createHarness>[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 });
+  });
 });

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 };
 }
 
 /**