From 962287565900d95232b45b61ba01111f86a03239 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Wed, 2 Sep 2026 12:45:30 -0700 Subject: [PATCH] Say how much an LDIF re-import duplicated, without acting on it The half of #223 that can move while the matching question is still open. Mozilla's schema defines no UID, so the import invents one and a re-import duplicates everything. Whether to guess an identity from a name and an address instead is the reporter's call and he has not made it -- but the harm that was actually reported was confusion rather than duplication: somebody imports a file twice and cannot tell what happened. So the import now counts how many of the entries look like contacts the book already held, and says so in a second message. Every card is still imported. Nothing is skipped and nothing is merged, which is the point: counting is a different act from matching, and it takes no decision away from the person who still owes us one. The likeness key is name plus one address, and it is wrong in both directions by design -- two colleagues sharing a name and an alias collapse, somebody whose address changed since the last export looks like a stranger. That is tolerable for a number on a toast and would not be tolerable for a merge, which is exactly why the number is all it does. The scan the vCard import already makes for UIDs now collects names and addresses on the same request, so this costs no extra round trip. It is read before anything is created, so a file that repeats a person twice counts as two new cards rather than as a duplicate of itself. If the answer comes back "match on name and email", the matching is written and becomes a skip instead of a count. Refs #223. --- web/src/locales/de.ts | 1 + web/src/locales/es.ts | 1 + web/src/locales/fr.ts | 1 + web/src/locales/ja.ts | 1 + web/src/locales/nl.ts | 1 + web/src/locales/pt-BR.ts | 1 + web/src/locales/ru.ts | 1 + web/src/locales/uk.ts | 1 + web/src/locales/zh-Hans.ts | 1 + .../__tests__/contact-set-batching.test.ts | 8 +- web/src/store/__tests__/ldif-import.test.ts | 6 +- web/src/store/__tests__/ldif-likeness.test.ts | 138 ++++++++++++++++++ web/src/store/__tests__/vcard-dedupe.test.ts | 12 +- web/src/store/contacts.ts | 66 +++++++-- web/src/views/contacts/ContactsView.tsx | 14 +- 15 files changed, 229 insertions(+), 24 deletions(-) create mode 100644 web/src/store/__tests__/ldif-likeness.test.ts diff --git a/web/src/locales/de.ts b/web/src/locales/de.ts index b40e64e..8fe687b 100644 --- a/web/src/locales/de.ts +++ b/web/src/locales/de.ts @@ -1079,6 +1079,7 @@ export const catalog: Catalog = { "Nothing unread here": "Hier ist nichts ungelesen", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} davon sieht aus wie ein Kontakt, den Sie schon hatten", other: "{n} davon sehen aus wie Kontakte, die Sie schon hatten" }, "Your administrator changed {n} settings": { one: "Ihre Administration hat {n} Einstellung geändert", other: "Ihre Administration hat {n} Einstellungen geändert" }, "Already here: {n} contacts, nothing imported": { one: "Bereits vorhanden: {n} Kontakt, nichts importiert", other: "Bereits vorhanden: {n} Kontakte, nichts importiert" }, "All {n} are already in your contacts": { one: "Bereits in Ihren Kontakten", other: "Alle {n} sind bereits in Ihren Kontakten" }, diff --git a/web/src/locales/es.ts b/web/src/locales/es.ts index 91b2831..d97c7cf 100644 --- a/web/src/locales/es.ts +++ b/web/src/locales/es.ts @@ -1052,6 +1052,7 @@ export const catalog: Catalog = { "Nothing unread here": "Aquí no hay nada sin leer", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} de ellos se parece a un contacto que ya tenías", other: "{n} de ellos se parecen a contactos que ya tenías" }, "Your administrator changed {n} settings": { one: "Tu administración cambió {n} ajuste", other: "Tu administración cambió {n} ajustes" }, "Already here: {n} contacts, nothing imported": { one: "Ya estaba aquí: {n} contacto, no se importó nada", other: "Ya estaban aquí: {n} contactos, no se importó nada" }, "All {n} are already in your contacts": { one: "Ya está en tus contactos", other: "Los {n} ya están en tus contactos" }, diff --git a/web/src/locales/fr.ts b/web/src/locales/fr.ts index a621da1..179ee17 100644 --- a/web/src/locales/fr.ts +++ b/web/src/locales/fr.ts @@ -1057,6 +1057,7 @@ export const catalog: Catalog = { "Nothing unread here": "Rien de non lu ici", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} d’entre eux ressemble à un contact que vous aviez déjà", other: "{n} d’entre eux ressemblent à des contacts que vous aviez déjà" }, "Your administrator changed {n} settings": { one: "Votre administration a modifié {n} paramètre", other: "Votre administration a modifié {n} paramètres" }, "Already here: {n} contacts, nothing imported": { one: "Déjà présent : {n} contact, rien d’importé", other: "Déjà présents : {n} contacts, rien d’importé" }, "All {n} are already in your contacts": { one: "Déjà dans vos contacts", other: "Les {n} sont déjà dans vos contacts" }, diff --git a/web/src/locales/ja.ts b/web/src/locales/ja.ts index 0276882..dee7385 100644 --- a/web/src/locales/ja.ts +++ b/web/src/locales/ja.ts @@ -1060,6 +1060,7 @@ export const catalog: Catalog = { "Nothing unread here": "ここに未読はありません", }, plurals: { + "{n} of them look like contacts you already had": { other: "うち {n} 件はすでにある連絡先に似ています" }, "Your administrator changed {n} settings": { other: "管理者が {n} 件の設定を変更しました" }, "Already here: {n} contacts, nothing imported": { other: "すでに存在: {n} 件、インポートなし" }, "All {n} are already in your contacts": { other: "{n} 件はすでに連絡先にあります" }, diff --git a/web/src/locales/nl.ts b/web/src/locales/nl.ts index 68e6bd3..2d45772 100644 --- a/web/src/locales/nl.ts +++ b/web/src/locales/nl.ts @@ -1048,6 +1048,7 @@ export const catalog: Catalog = { "Nothing unread here": "Hier is niets ongelezen", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} daarvan lijkt op een contact dat u al had", other: "{n} daarvan lijken op contacten die u al had" }, "Your administrator changed {n} settings": { one: "Uw beheerder heeft {n} instelling gewijzigd", other: "Uw beheerder heeft {n} instellingen gewijzigd" }, "Already here: {n} contacts, nothing imported": { one: "Al aanwezig: {n} contact, niets geïmporteerd", other: "Al aanwezig: {n} contacten, niets geïmporteerd" }, "All {n} are already in your contacts": { one: "Staat al in uw contacten", other: "Alle {n} staan al in uw contacten" }, diff --git a/web/src/locales/pt-BR.ts b/web/src/locales/pt-BR.ts index 9260146..db29a4b 100644 --- a/web/src/locales/pt-BR.ts +++ b/web/src/locales/pt-BR.ts @@ -1055,6 +1055,7 @@ export const catalog: Catalog = { "Nothing unread here": "Não há nada não lido aqui", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} deles parece um contato que você já tinha", other: "{n} deles parecem contatos que você já tinha" }, "Your administrator changed {n} settings": { one: "Sua administração alterou {n} configuração", other: "Sua administração alterou {n} configurações" }, "Already here: {n} contacts, nothing imported": { one: "Já estava aqui: {n} contato, nada importado", other: "Já estavam aqui: {n} contatos, nada importado" }, "All {n} are already in your contacts": { one: "Já está nos seus contatos", other: "Todos os {n} já estão nos seus contatos" }, diff --git a/web/src/locales/ru.ts b/web/src/locales/ru.ts index e5ce14c..ee4a535 100644 --- a/web/src/locales/ru.ts +++ b/web/src/locales/ru.ts @@ -1054,6 +1054,7 @@ export const catalog: Catalog = { "Nothing unread here": "Здесь нет непрочитанного", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} из них похож на контакт, который уже был", few: "{n} из них похожи на контакты, которые уже были", many: "{n} из них похожи на контакты, которые уже были", other: "{n} из них похожи на контакты, которые уже были" }, "Your administrator changed {n} settings": { one: "Администратор изменил {n} настройку", few: "Администратор изменил {n} настройки", many: "Администратор изменил {n} настроек", other: "Администратор изменил {n} настройки" }, "Already here: {n} contacts, nothing imported": { one: "Уже есть: {n} контакт, ничего не импортировано", few: "Уже есть: {n} контакта, ничего не импортировано", many: "Уже есть: {n} контактов, ничего не импортировано", other: "Уже есть: {n} контакта, ничего не импортировано" }, "All {n} are already in your contacts": { one: "Уже в ваших контактах", few: "Все {n} уже в ваших контактах", many: "Все {n} уже в ваших контактах", other: "Все {n} уже в ваших контактах" }, diff --git a/web/src/locales/uk.ts b/web/src/locales/uk.ts index 2df98b3..2a1244c 100644 --- a/web/src/locales/uk.ts +++ b/web/src/locales/uk.ts @@ -1048,6 +1048,7 @@ export const catalog: Catalog = { "Nothing unread here": "Тут немає непрочитаного", }, plurals: { + "{n} of them look like contacts you already had": { one: "{n} з них схожий на контакт, який уже був", few: "{n} з них схожі на контакти, які вже були", many: "{n} з них схожі на контакти, які вже були", other: "{n} з них схожі на контакти, які вже були" }, "Your administrator changed {n} settings": { one: "Адміністратор змінив {n} налаштування", few: "Адміністратор змінив {n} налаштування", many: "Адміністратор змінив {n} налаштувань", other: "Адміністратор змінив {n} налаштування" }, "Already here: {n} contacts, nothing imported": { one: "Уже є: {n} контакт, нічого не імпортовано", few: "Уже є: {n} контакти, нічого не імпортовано", many: "Уже є: {n} контактів, нічого не імпортовано", other: "Уже є: {n} контакти, нічого не імпортовано" }, "All {n} are already in your contacts": { one: "Уже у ваших контактах", few: "Усі {n} уже у ваших контактах", many: "Усі {n} уже у ваших контактах", other: "Усі {n} уже у ваших контактах" }, diff --git a/web/src/locales/zh-Hans.ts b/web/src/locales/zh-Hans.ts index 330a28d..3549075 100644 --- a/web/src/locales/zh-Hans.ts +++ b/web/src/locales/zh-Hans.ts @@ -1059,6 +1059,7 @@ export const catalog: Catalog = { "Nothing unread here": "这里没有未读邮件", }, plurals: { + "{n} of them look like contacts you already had": { other: "其中 {n} 个与您已有的联系人相似" }, "Your administrator changed {n} settings": { other: "管理员更改了 {n} 项设置" }, "Already here: {n} contacts, nothing imported": { other: "已存在 {n} 个，未导入" }, "All {n} are already in your contacts": { other: "这 {n} 个已在您的联系人中" }, diff --git a/web/src/store/__tests__/contact-set-batching.test.ts b/web/src/store/__tests__/contact-set-batching.test.ts index 6cb5f03..9c536cf 100644 --- a/web/src/store/__tests__/contact-set-batching.test.ts +++ b/web/src/store/__tests__/contact-set-batching.test.ts @@ -100,14 +100,14 @@ afterEach(() => { describe("importing an LDIF bigger than the server will take at once", () => { it("splits it into calls the server will accept, and files all of it", async () => { const sets = server({ max: MAX }); - await expect(useContacts.getState().importLdif(ldifOf(1200), "book1")).resolves.toEqual({ created: 1200, skipped: 0 }); + await expect(useContacts.getState().importLdif(ldifOf(1200), "book1")).resolves.toEqual({ created: 1200, skipped: 0, alike: 0 }); expect(sizes(sets)).toEqual([500, 500, 200]); }); it("splits by what the session advertises, not by a number of its own", async () => { client.session!.capabilities[CAP.core] = { maxObjectsInGet: 40, maxObjectsInSet: 40 }; const sets = server({ max: 40 }); - await expect(useContacts.getState().importLdif(ldifOf(100), "book1")).resolves.toEqual({ created: 100, skipped: 0 }); + await expect(useContacts.getState().importLdif(ldifOf(100), "book1")).resolves.toEqual({ created: 100, skipped: 0, alike: 0 }); expect(sizes(sets)).toEqual([40, 40, 20]); }); @@ -134,7 +134,7 @@ describe("importing an LDIF bigger than the server will take at once", () => { describe("importing a vCard file bigger than the server will take at once", () => { it("splits it into calls the server will accept, and files all of it", async () => { const sets = server({ max: MAX, parsed: vcardsOf(1200) }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1200, skipped: 0 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1200, skipped: 0, alike: 0 }); expect(sizes(sets)).toEqual([500, 500, 200]); }); @@ -155,7 +155,7 @@ describe("importing a vCard file bigger than the server will take at once", () = it("counts what got in when only some of it did", async () => { server({ max: MAX, parsed: vcardsOf(2), notCreated: { c1: { type: "invalidProperties" } } }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 }); }); }); diff --git a/web/src/store/__tests__/ldif-import.test.ts b/web/src/store/__tests__/ldif-import.test.ts index 19d3cab..3be9da6 100644 --- a/web/src/store/__tests__/ldif-import.test.ts +++ b/web/src/store/__tests__/ldif-import.test.ts @@ -63,7 +63,7 @@ describe("importing an LDIF address book", () => { it("creates every entry in one call, not one call each", async () => { const sets = server(); const n = await useContacts.getState().importLdif(TWO, "book1"); - expect(n).toEqual({ created: 2, skipped: 0 }); + expect(n).toEqual({ created: 2, skipped: 0, alike: 0 }); expect(sets).toHaveLength(1); expect(Object.keys(sets[0]!.create!)).toEqual(["c0", "c1"]); }); @@ -105,7 +105,7 @@ describe("importing an LDIF address book", () => { it("skips entries too empty to be a person, and imports the rest", async () => { const sets = server(); const n = await useContacts.getState().importLdif(`${TWO}\ndn: cn=Nobody\nobjectClass: top\n`, "book1"); - expect(n).toEqual({ created: 2, skipped: 0 }); + expect(n).toEqual({ created: 2, skipped: 0, alike: 0 }); expect(Object.keys(sets[0]!.create!)).toHaveLength(2); }); @@ -116,6 +116,6 @@ describe("importing an LDIF address book", () => { it("counts what got in when only some of it did", async () => { server({ notCreated: { c1: { type: "invalidProperties" } } }); - await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 1, skipped: 0 }); + await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 }); }); }); diff --git a/web/src/store/__tests__/ldif-likeness.test.ts b/web/src/store/__tests__/ldif-likeness.test.ts new file mode 100644 index 0000000..ab102e1 --- /dev/null +++ b/web/src/store/__tests__/ldif-likeness.test.ts @@ -0,0 +1,138 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { CAP, client } from "@/jmap/client"; +import { useContacts } from "@/store/contacts"; +import type { ContactCard, JmapSession } from "@/jmap/types"; + +/* + * Counting look-alikes on an LDIF import, without acting on them. + * + * Mozilla's schema has no UID, so the import invents one and a re-import + * duplicates everything. Whether to guess an identity from a name and an + * address is still open on #223 -- and the harm that was actually reported was + * confusion rather than duplication: somebody imports a file twice and cannot + * tell what happened. So this counts and says so, and imports every card + * regardless. Reporting is not matching. + */ + +interface SetArgs { create?: Record> } + +function server(existing: Array & { id: string }>) { + const sets: SetArgs[] = []; + const fetchMock = vi.fn(async (_url: string, init: RequestInit) => { + const body = JSON.parse(init.body as string) as { methodCalls: [string, Record, string][] }; + const methodResponses = body.methodCalls.map(([name, args, id]) => { + if (name === "ContactCard/query") { + const position = (args.position as number) ?? 0; + return [name, { accountId: "a1", queryState: "1", canCalculateChanges: false, position, ids: position ? [] : existing.map((c) => c.id), total: existing.length }, id]; + } + if (name === "ContactCard/get") { + const want = new Set((args.ids as string[]) ?? []); + return [name, { accountId: "a1", state: "1", list: existing.filter((c) => want.has(c.id)), notFound: [] }, id]; + } + if (name === "ContactCard/set") { + sets.push({ create: args.create as Record> }); + return [name, { + accountId: "a1", oldState: "1", newState: "2", + created: Object.fromEntries(Object.keys((args.create ?? {}) as object).map((k) => [k, { id: `new-${k}` }])), + notCreated: {}, + }, id]; + } + return [name, { accountId: "a1", state: "1", list: [], notFound: [], ids: [], total: 0, queryState: "q", position: 0, canCalculateChanges: false }, id]; + }); + return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "1" }) } as Response; + }); + vi.stubGlobal("fetch", fetchMock); + return sets; +} + +/** A card already in the book, with the two fields likeness is read from. */ +const card = (id: string, full: string, ...emails: string[]) => ({ + id, uid: `uid-${id}`, addressBookIds: { book1: true }, + name: { full }, + emails: Object.fromEntries(emails.map((address, i) => [`e${i}`, { address }])), +}) as unknown as Partial & { id: string }; + +const entry = (cn: string, mail: string) => + `dn: cn=${cn}\ngivenName: ${cn.split(" ")[0]}\nsn: ${cn.split(" ").slice(-1)[0]}\ncn: ${cn}\nmail: ${mail}\n`; + +beforeEach(() => { + client.session = { + capabilities: { [CAP.core]: { maxObjectsInGet: 500, maxObjectsInSet: 500 }, [CAP.contacts]: {} }, + accounts: {}, primaryAccounts: {}, state: "s1", + } as unknown as JmapSession; + useContacts.setState({ accountId: "a1", available: true, books: {}, cards: {} as Record }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe("telling somebody what an LDIF re-import duplicated", () => { + it("counts an entry that matches an existing card on name and address", async () => { + server([card("c1", "Jane Doe", "jane@example.com")]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r).toEqual({ created: 1, skipped: 0, alike: 1 }); + }); + + it("imports it anyway, which is the whole point of counting rather than matching", async () => { + const sets = server([card("c1", "Jane Doe", "jane@example.com")]); + await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(Object.keys(sets[0]!.create!)).toHaveLength(1); + }); + + it("does not count a name match with a different address", async () => { + // Two people who share a name are two people. This is exactly the guess + // the counting refuses to make on anyone's behalf. + server([card("c1", "Jane Doe", "jane.doe@other.example")]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r.alike).toBe(0); + }); + + it("does not count an address match under a different name", async () => { + server([card("c1", "Someone Else", "jane@example.com")]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r.alike).toBe(0); + }); + + it("recognises a match on a second address", async () => { + server([card("c1", "Jane Doe", "old@example.com", "jane@example.com")]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r.alike).toBe(1); + }); + + it("ignores case and spacing, which an export and a hand-typed card differ in", async () => { + server([card("c1", " JANE DOE ", "Jane@Example.com")]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r.alike).toBe(1); + }); + + it("counts each entry once however many of its addresses match", async () => { + server([card("c1", "Jane Doe", "jane@example.com", "j@example.com")]); + const two = `dn: cn=Jane Doe\ncn: Jane Doe\nmail: jane@example.com\nmozillaSecondEmail: j@example.com\n`; + const r = await useContacts.getState().importLdif(two, "book1"); + expect(r.alike).toBe(1); + }); + + it("looks only at the book being imported into", async () => { + const elsewhere = { ...card("c1", "Jane Doe", "jane@example.com"), addressBookIds: { book2: true } }; + server([elsewhere]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r.alike).toBe(0); + }); + + it("counts nothing against an empty book", async () => { + server([]); + const r = await useContacts.getState().importLdif(entry("Jane Doe", "jane@example.com"), "book1"); + expect(r).toEqual({ created: 1, skipped: 0, alike: 0 }); + }); + + it("does not count the file against itself", async () => { + // Two of the same person in one file are two new cards, not a duplicate of + // something that was already here. The scan is read before anything lands. + server([]); + const twice = entry("Jane Doe", "jane@example.com") + "\n" + entry("Jane Doe", "jane@example.com"); + const r = await useContacts.getState().importLdif(twice, "book1"); + expect(r).toEqual({ created: 2, skipped: 0, alike: 0 }); + }); +}); diff --git a/web/src/store/__tests__/vcard-dedupe.test.ts b/web/src/store/__tests__/vcard-dedupe.test.ts index 5cd4e60..4bfb1ab 100644 --- a/web/src/store/__tests__/vcard-dedupe.test.ts +++ b/web/src/store/__tests__/vcard-dedupe.test.ts @@ -73,7 +73,7 @@ afterEach(() => { describe("re-importing vCards the book already has", () => { it("skips a card whose uid is already in this book", async () => { const sets = server({ parsed: [card("ada@x", "Ada"), card("alan@x", "Alan")], existing: [here("ada@x")] }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 1 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 1, alike: 0 }); expect(Object.values(sets[0]!.create!).map((c) => (c.name as { full: string }).full)).toEqual(["Alan"]); }); @@ -81,26 +81,26 @@ describe("re-importing vCards the book already has", () => { // The same person legitimately filed in two address books is not a // duplicate, any more than the same event in two calendars is. const sets = server({ parsed: [card("ada@x", "Ada")], existing: [here("ada@x", "book2")] }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 }); expect(Object.keys(sets[0]!.create!)).toHaveLength(1); }); it("imports a card that arrived with no uid, rather than guessing at one", async () => { const noUid = { "@type": "Card", version: "1.0", kind: "individual", name: { full: "Anon" } }; const sets = server({ parsed: [noUid], existing: [here("ada@x")] }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 1, skipped: 0, alike: 0 }); expect(Object.values(sets[0]!.create!)[0]!.uid).toEqual(expect.any(String)); }); it("sends nothing at all when the whole file is already here", async () => { const sets = server({ parsed: [card("ada@x", "Ada"), card("alan@x", "Alan")], existing: [here("ada@x"), here("alan@x")] }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 0, skipped: 2 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 0, skipped: 2, alike: 0 }); expect(sets).toHaveLength(0); }); it("imports everything into an empty book", async () => { const sets = server({ parsed: [card("ada@x", "Ada"), card("alan@x", "Alan")] }); - await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 2, skipped: 0 }); + await expect(useContacts.getState().importVCard("BEGIN:VCARD", "book1")).resolves.toEqual({ created: 2, skipped: 0, alike: 0 }); expect(Object.keys(sets[0]!.create!)).toHaveLength(2); }); }); @@ -116,6 +116,6 @@ describe("LDIF, which has nothing to match on", () => { * name and an address instead is the open question on #223. */ server({ existing: [here("anything")] }); - await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 2, skipped: 0 }); + await expect(useContacts.getState().importLdif(TWO, "book1")).resolves.toEqual({ created: 2, skipped: 0, alike: 0 }); }); }); diff --git a/web/src/store/contacts.ts b/web/src/store/contacts.ts index e19c969..2f08567 100644 --- a/web/src/store/contacts.ts +++ b/web/src/store/contacts.ts @@ -41,22 +41,53 @@ import { useMail } from "./mail"; * happens to be holding costs one pass over a list nobody imports into twice a * day. */ -async function uidsInBook(accountId: Id, addressBookId: Id): Promise> { +async function scanBook(accountId: Id, addressBookId: Id): Promise<{ uids: Set; likeness: Set }> { const uids = new Set(); + const likeness = new Set(); const page = client.maxObjectsInGet; for (let position = 0; ; ) { const q = await client.call("ContactCard/query", { accountId, position, limit: page, calculateTotal: true }); const ids = q.ids ?? []; if (!ids.length) break; for (const part of chunk(ids, page)) { - const g = await client.call>("ContactCard/get", { accountId, ids: part, properties: ["uid", "addressBookIds"] }); - for (const c of g.list) if (c.uid && c.addressBookIds?.[addressBookId]) uids.add(c.uid); + const g = await client.call>("ContactCard/get", { accountId, ids: part, properties: ["uid", "addressBookIds", "name", "emails"] }); + for (const c of g.list) { + if (!c.addressBookIds?.[addressBookId]) continue; + if (c.uid) uids.add(c.uid); + for (const key of likenessKeys(c)) likeness.add(key); + } } position += ids.length; // `total` is optional, so the empty page above is what actually ends this. if (q.total != null && position >= q.total) break; } - return uids; + return { uids, likeness }; +} + +/** + * What makes two cards *look* like the same person -- name and one address. + * + * Deliberately not used to skip or merge anything. It is a guess, and it is + * wrong in both directions: two colleagues who share a name and a shared alias + * collapse into one, and somebody whose address changed since the last export + * looks like a stranger. Either mistake is silent and one of them is + * unrecoverable, which is why #223 leaves the decision open. + * + * Counting is a different act from acting. An LDIF re-import duplicates + * everything -- Mozilla's schema has no UID, so the import invents one and + * nothing can match -- and the reported harm was confusion rather than data + * loss: somebody imports a file twice and cannot tell what happened. Being told + * "40 of these look like contacts you already had" answers that without + * touching a single card. + * + * One key per address, so a person whose second address matches is still + * recognised. + */ +function likenessKeys(c: Partial): string[] { + const name = contactDisplayName(c as ContactCard).trim().toLowerCase(); + if (!name) return []; + const addresses = Object.values(c.emails ?? {}).map((e) => e.address?.trim().toLowerCase()).filter(Boolean); + return addresses.map((a) => `${name}\u0000${a}`); } async function createCards(accountId: Id, create: Record): Promise<{ created: number; refused?: SetError }> { @@ -154,7 +185,7 @@ interface ContactsState { updateBook(id: Id, patch: Partial): Promise; destroyBook(id: Id): Promise; /** Import vCards, skipping any whose UID this book already holds. */ - importVCard(text: string, addressBookId: Id): Promise<{ created: number; skipped: number }>; + importVCard(text: string, addressBookId: Id): Promise<{ created: number; skipped: number; alike: number }>; /** * Import an address book in LDIF, read against Mozilla's schema. * @@ -162,7 +193,7 @@ interface ContactsState { * recognise a re-import by. Answered in the same shape as the vCard import so * the caller does not have to know which one it called. */ - importLdif(text: string, addressBookId: Id): Promise<{ created: number; skipped: number }>; + importLdif(text: string, addressBookId: Id): Promise<{ created: number; skipped: number; alike: number }>; loadPrincipals(): Promise; suggest(query: string, limit?: number): Promise; addRecent(addrs: EmailAddress[]): void; @@ -460,7 +491,7 @@ export const useContacts = create((set, get) => ({ const entry = parsed.parsed?.[up.blobId]; const cards: ContactCard[] = entry ? (Array.isArray(entry) ? entry : [entry]) : []; if (!cards.length) throw new Error("No contacts found in file"); - const already = await uidsInBook(accountId, addressBookId); + const already = (await scanBook(accountId, addressBookId)).uids; const create: Record = {}; let skipped = 0; cards.forEach((c, i) => { @@ -482,7 +513,7 @@ export const useContacts = create((set, get) => ({ // The whole file was already here. Nothing to send, and nothing wrong. if (!Object.keys(create).length) { await get().loadAll(); - return { created: 0, skipped }; + return { created: 0, skipped, alike: 0 }; } try { const { created, refused } = await createCards(accountId, create); @@ -490,7 +521,9 @@ export const useContacts = create((set, get) => ({ // though the file had been empty. The LDIF import said this already; a // vCard import that quietly returned 0 was the odd one out. if (!created) throw new Error(refused ? setErrorMessage(refused) : "the server did not accept any of its contacts"); - return { created, skipped }; + /* No likeness count here: a vCard carries a UID, so anything that was + already present was skipped by name above rather than guessed at. */ + return { created, skipped, alike: 0 }; } finally { await get().loadAll(); } @@ -509,8 +542,16 @@ export const useContacts = create((set, get) => ({ const accountId = get().accountId!; const cards = parseLdif(text).map(cardFromLdif).filter((c): c is Partial => c !== null); if (!cards.length) throw new Error("it has no contacts in it"); + /* + * Read before anything is created, so "already had" means before this + * import rather than including it. Every card here is imported either way; + * this only counts. + */ + const before = await scanBook(accountId, addressBookId); + let alike = 0; const create: Record = {}; cards.forEach((c, i) => { + if (likenessKeys(c).some((k) => before.likeness.has(k))) alike++; // Built here rather than read from the file: LDIF identifies an entry by // its distinguished name, which says where it sat in somebody's // directory and is no use as a contact's identity anywhere else. @@ -529,8 +570,13 @@ export const useContacts = create((set, get) => ({ * here because Mozilla's schema does not define one, so a re-import has no * identity to be recognised by -- see #223, where whether to guess at one * from a name and an address is still an open question. + * + * `alike` is what can be said without answering it: how many of these look + * like contacts that were already here. Reporting is not matching -- every + * card was imported -- and it is the confusion rather than the duplication + * that was reported as the harm. */ - return { created, skipped: 0 }; + return { created, skipped: 0, alike }; }, async loadPrincipals() { diff --git a/web/src/views/contacts/ContactsView.tsx b/web/src/views/contacts/ContactsView.tsx index 08ce7e1..a35bddb 100644 --- a/web/src/views/contacts/ContactsView.tsx +++ b/web/src/views/contacts/ContactsView.tsx @@ -136,7 +136,7 @@ export function ContactsView({ id }: { id?: string }) { * LDIF may arrive as .ldif, .ldi, .txt or with no extension at all, and * the name is the least reliable thing about it. */ - const { created, skipped } = /^\s*BEGIN:VCARD/im.test(text) + const { created, skipped, alike } = /^\s*BEGIN:VCARD/im.test(text) ? await contacts.importVCard(text, book.id) : await contacts.importLdif(text, book.id); /* @@ -149,6 +149,18 @@ export function ContactsView({ id }: { id?: string }) { if (!created) toast.success(plural(skipped, { one: "Already here: {n} contact, nothing imported", other: "Already here: {n} contacts, nothing imported" })); else if (skipped) toast.success(`${imported} · ${plural(skipped, { one: "{n} was already here", other: "{n} were already here" })}`); else toast.success(imported); + /* + * Said separately, and after, because it is a different kind of fact. + * LDIF has no UID to match on, so nothing was skipped and nothing was + * merged -- these are simply here twice now, and saying so is the whole + * of what can honestly be said without guessing (#223). + */ + if (alike) { + toast.show(plural(alike, { + one: "{n} of them looks like a contact you already had", + other: "{n} of them look like contacts you already had", + }), { duration: 9000 }); + } } catch (err) { toast.error(translate("Could not import this file: {error}", { error: (err as Error).message })); }