From 74f6d1d0aa19631d6fc9e4dce20e4c4f1923561f Mon Sep 17 00:00:00 2001 From: John Coffey Date: Wed, 2 Sep 2026 08:52:33 -0700 Subject: [PATCH] Skip events on re-import that the calendar already has Importing an export twice left second copies of everything. The import has kept the file's own UID since it was written -- inventing one only where an event arrives without -- so what was needed to recognise an event that is already here was there all along, and nothing looked at it. Asked for on #173 after the reporter's colleague hit the duplication in testing, and decided there: skip on a UID the calendar already holds, import what arrives without one. An event with no UID is not one anything can match to, and a softer match -- title and time, say -- guesses in both directions. The UIDs are read once per import rather than once per event. CalendarEvent/ query does take a uid filter, which is what findByUid uses, but a file of two thousand events would be two thousand queries. Read without expandRecurrences so a weekly series is one event with one UID rather than one per occurrence, and narrowed to the target calendar from calendarIds rather than through an inCalendar filter this client has not confirmed the server supports. Matching is per calendar. A UID is what makes an event the same event across calendars, so the same event being in two of them is not a duplicate and the second calendar still gets its copy. importIcs now answers with both counts. "Imported 40 events" over a file of 240 reads as a failure when 200 of them were simply already there, and a re-import of an unchanged file would otherwise report importing nothing at all rather than saying everything was already here. The three import toasts are translated in all nine catalogues while the messages were being written -- the plural for the existing one had never been added and was falling back to English. Closes #222. --- web/src/locales/de.ts | 3 + web/src/locales/es.ts | 3 + web/src/locales/fr.ts | 3 + web/src/locales/ja.ts | 3 + web/src/locales/nl.ts | 3 + web/src/locales/pt-BR.ts | 3 + web/src/locales/ru.ts | 3 + web/src/locales/uk.ts | 3 + web/src/locales/zh-Hans.ts | 3 + web/src/store/__tests__/ics-import.test.ts | 66 ++++++++++++++++++++-- web/src/store/calendar.ts | 61 ++++++++++++++++++-- web/src/views/calendar/CalendarSidebar.tsx | 12 +++- 12 files changed, 152 insertions(+), 14 deletions(-) diff --git a/web/src/locales/de.ts b/web/src/locales/de.ts index 3a8c017..9fc828b 100644 --- a/web/src/locales/de.ts +++ b/web/src/locales/de.ts @@ -1073,6 +1073,9 @@ export const catalog: Catalog = { "Nothing unread here": "Hier ist nichts ungelesen", }, plurals: { + "Imported {n} events": { one: "{n} Termin importiert", other: "{n} Termine importiert" }, + "Already here: {n} events, nothing imported": { one: "Bereits vorhanden: {n} Termin, nichts importiert", other: "Bereits vorhanden: {n} Termine, nichts importiert" }, + "{n} were already here": { one: "{n} war bereits vorhanden", other: "{n} waren bereits vorhanden" }, "{n} messages": { one: "{n} Nachricht", other: "{n} Nachrichten" }, "{n} selected": { one: "{n} ausgewählt", other: "{n} ausgewählt" }, "{n} conversations": { one: "{n} Konversation", other: "{n} Konversationen" }, diff --git a/web/src/locales/es.ts b/web/src/locales/es.ts index 9fe81f6..4b5d042 100644 --- a/web/src/locales/es.ts +++ b/web/src/locales/es.ts @@ -1046,6 +1046,9 @@ export const catalog: Catalog = { "Nothing unread here": "Aquí no hay nada sin leer", }, plurals: { + "Imported {n} events": { one: "{n} evento importado", other: "{n} eventos importados" }, + "Already here: {n} events, nothing imported": { one: "Ya estaba aquí: {n} evento, no se importó nada", other: "Ya estaban aquí: {n} eventos, no se importó nada" }, + "{n} were already here": { one: "{n} ya estaba aquí", other: "{n} ya estaban aquí" }, "{n} messages": { one: "{n} mensaje", other: "{n} mensajes" }, "{n} selected": { one: "{n} seleccionado", other: "{n} seleccionados" }, "{n} conversations": { one: "{n} conversación", other: "{n} conversaciones" }, diff --git a/web/src/locales/fr.ts b/web/src/locales/fr.ts index 39cc784..49ae055 100644 --- a/web/src/locales/fr.ts +++ b/web/src/locales/fr.ts @@ -1051,6 +1051,9 @@ export const catalog: Catalog = { "Nothing unread here": "Rien de non lu ici", }, plurals: { + "Imported {n} events": { one: "{n} événement importé", other: "{n} événements importés" }, + "Already here: {n} events, nothing imported": { one: "Déjà présent : {n} événement, rien d’importé", other: "Déjà présents : {n} événements, rien d’importé" }, + "{n} were already here": { one: "{n} était déjà présent", other: "{n} étaient déjà présents" }, "{n} messages": { one: "{n} message", other: "{n} messages" }, "{n} selected": { one: "{n} sélectionné", other: "{n} sélectionnés" }, "{n} conversations": { one: "{n} conversation", other: "{n} conversations" }, diff --git a/web/src/locales/ja.ts b/web/src/locales/ja.ts index ae10631..17420e1 100644 --- a/web/src/locales/ja.ts +++ b/web/src/locales/ja.ts @@ -1054,6 +1054,9 @@ export const catalog: Catalog = { "Nothing unread here": "ここに未読はありません", }, plurals: { + "Imported {n} events": { other: "{n} 件の予定をインポートしました" }, + "Already here: {n} events, nothing imported": { other: "すでに存在: {n} 件、インポートなし" }, + "{n} were already here": { other: "{n} 件はすでに存在していました" }, /* * One form each, because Japanese has one. Intl.PluralRules returns * `other` for every number, so `one`, `few` and `many` would never be diff --git a/web/src/locales/nl.ts b/web/src/locales/nl.ts index 19ab17d..e0be62e 100644 --- a/web/src/locales/nl.ts +++ b/web/src/locales/nl.ts @@ -1042,6 +1042,9 @@ export const catalog: Catalog = { "Nothing unread here": "Hier is niets ongelezen", }, plurals: { + "Imported {n} events": { one: "{n} afspraak geïmporteerd", other: "{n} afspraken geïmporteerd" }, + "Already here: {n} events, nothing imported": { one: "Al aanwezig: {n} afspraak, niets geïmporteerd", other: "Al aanwezig: {n} afspraken, niets geïmporteerd" }, + "{n} were already here": { one: "{n} was er al", other: "{n} waren er al" }, "{n} messages": { one: "{n} bericht", other: "{n} berichten" }, "{n} selected": { one: "{n} geselecteerd", other: "{n} geselecteerd" }, "{n} conversations": { one: "{n} gesprek", other: "{n} gesprekken" }, diff --git a/web/src/locales/pt-BR.ts b/web/src/locales/pt-BR.ts index 2790cf7..ac8752c 100644 --- a/web/src/locales/pt-BR.ts +++ b/web/src/locales/pt-BR.ts @@ -1049,6 +1049,9 @@ export const catalog: Catalog = { "Nothing unread here": "Não há nada não lido aqui", }, plurals: { + "Imported {n} events": { one: "{n} evento importado", other: "{n} eventos importados" }, + "Already here: {n} events, nothing imported": { one: "Já estava aqui: {n} evento, nada importado", other: "Já estavam aqui: {n} eventos, nada importado" }, + "{n} were already here": { one: "{n} já estava aqui", other: "{n} já estavam aqui" }, "{n} messages": { one: "{n} mensagem", other: "{n} mensagens" }, "{n} selected": { one: "{n} selecionada", other: "{n} selecionadas" }, "{n} conversations": { one: "{n} conversa", other: "{n} conversas" }, diff --git a/web/src/locales/ru.ts b/web/src/locales/ru.ts index 70a623b..08794ce 100644 --- a/web/src/locales/ru.ts +++ b/web/src/locales/ru.ts @@ -1048,6 +1048,9 @@ export const catalog: Catalog = { "Nothing unread here": "Здесь нет непрочитанного", }, plurals: { + "Imported {n} events": { one: "Импортировано {n} событие", few: "Импортировано {n} события", many: "Импортировано {n} событий", other: "Импортировано {n} события" }, + "Already here: {n} events, nothing imported": { one: "Уже есть: {n} событие, ничего не импортировано", few: "Уже есть: {n} события, ничего не импортировано", many: "Уже есть: {n} событий, ничего не импортировано", other: "Уже есть: {n} события, ничего не импортировано" }, + "{n} were already here": { one: "{n} уже было здесь", few: "{n} уже были здесь", many: "{n} уже были здесь", other: "{n} уже были здесь" }, /* * Three forms, which is the whole reason plural() takes a map rather than * (one, other). Intl.PluralRules picks: 1 is `one`, 2-4 are `few`, 5-20 diff --git a/web/src/locales/uk.ts b/web/src/locales/uk.ts index 4119488..840a38d 100644 --- a/web/src/locales/uk.ts +++ b/web/src/locales/uk.ts @@ -1042,6 +1042,9 @@ export const catalog: Catalog = { "Nothing unread here": "Тут немає непрочитаного", }, plurals: { + "Imported {n} events": { one: "Імпортовано {n} подію", few: "Імпортовано {n} події", many: "Імпортовано {n} подій", other: "Імпортовано {n} події" }, + "Already here: {n} events, nothing imported": { one: "Уже є: {n} подія, нічого не імпортовано", few: "Уже є: {n} події, нічого не імпортовано", many: "Уже є: {n} подій, нічого не імпортовано", other: "Уже є: {n} події, нічого не імпортовано" }, + "{n} were already here": { one: "{n} уже була тут", few: "{n} уже були тут", many: "{n} уже були тут", other: "{n} уже були тут" }, /* * Ukrainian takes the same three forms as Russian and the same rule, but * not the same words. Sharing a plural structure is not sharing a diff --git a/web/src/locales/zh-Hans.ts b/web/src/locales/zh-Hans.ts index a429853..97b0fda 100644 --- a/web/src/locales/zh-Hans.ts +++ b/web/src/locales/zh-Hans.ts @@ -1053,6 +1053,9 @@ export const catalog: Catalog = { "Nothing unread here": "这里没有未读邮件", }, plurals: { + "Imported {n} events": { other: "已导入 {n} 个日程" }, + "Already here: {n} events, nothing imported": { other: "已存在 {n} 个，未导入" }, + "{n} were already here": { other: "{n} 个已存在" }, /* * One form each, because Chinese has one. Intl.PluralRules returns `other` * for every number, so `one`, `few` and `many` would never be selected — diff --git a/web/src/store/__tests__/ics-import.test.ts b/web/src/store/__tests__/ics-import.test.ts index 049b661..f300f0b 100644 --- a/web/src/store/__tests__/ics-import.test.ts +++ b/web/src/store/__tests__/ics-import.test.ts @@ -38,8 +38,9 @@ interface SetArgs { create?: Record>; sendSchedu * refuses it: the whole call, creating nothing. * @param failOn which `/set` call (0-based) answers with an error instead. */ -function server(parsed: unknown, opts: { notCreated?: Record; max?: number; failOn?: number } = {}) { +function server(parsed: unknown, opts: { notCreated?: Record; max?: number; failOn?: number; existing?: Array<{ id: string; uid: string; calendarIds: Record }> } = {}) { const sets: SetArgs[] = []; + const existing = opts.existing ?? []; 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]) => { @@ -63,6 +64,16 @@ function server(parsed: unknown, opts: { notCreated?: Record; m notCreated, }, id]; } + // The scan for UIDs already in the calendar: a query for the account's + // events, then their uid and calendarIds. + if (name === "CalendarEvent/query") { + const position = (args.position as number) ?? 0; + return [name, { accountId: "a1", queryState: "1", canCalculateChanges: false, position, ids: position ? [] : existing.map((e) => e.id), total: existing.length }, id]; + } + if (name === "CalendarEvent/get") { + const want = new Set((args.ids as string[]) ?? []); + return [name, { accountId: "a1", state: "1", list: existing.filter((e) => want.has(e.id)), notFound: [] }, id]; + } return [name, { accountId: "a1", state: "1", list: [], notFound: [] }, id]; }); return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "1" }) } as Response; @@ -111,7 +122,7 @@ describe("importing an .ics file", () => { it("creates every event in one call when the file fits in one, not one call each", async () => { const sets = server(PARSED); const n = await useCalendar.getState().importIcs("x", "cal1"); - expect(n).toBe(2); + expect(n).toEqual({ created: 2, skipped: 0 }); expect(sets).toHaveLength(1); expect(Object.keys(sets[0]!.create!)).toEqual(["e0", "e1"]); }); @@ -152,7 +163,7 @@ describe("importing an .ics file", () => { it("takes a single event, which is what a one-event file parses to", async () => { const sets = server(PARSED[0]); const n = await useCalendar.getState().importIcs("x", "cal1"); - expect(n).toBe(1); + expect(n).toEqual({ created: 1, skipped: 0 }); expect(Object.keys(sets[0]!.create!)).toEqual(["e0"]); }); @@ -168,7 +179,7 @@ describe("importing an .ics file", () => { it("counts what got in when only some of it did", async () => { server(PARSED, { notCreated: { e1: { type: "invalidProperties" } } }); - await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toBe(1); + await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 1, skipped: 0 }); }); }); @@ -191,14 +202,14 @@ describe("importing a 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(many(1200), { max: MAX }); - await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toBe(1200); + await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 1200, skipped: 0 }); expect(sets.map((s) => Object.keys(s.create!).length)).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(many(100), { max: 40 }); - await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toBe(100); + await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 100, skipped: 0 }); expect(sets.map((s) => Object.keys(s.create!).length)).toEqual([40, 40, 20]); }); @@ -237,3 +248,46 @@ describe("importing a file bigger than the server will take at once", () => { await expect(useCalendar.getState().importIcs("x", "cal1")).rejects.toThrow(/roof fell in/); }); }); + +/* + * Re-importing the same file. + * + * The import kept the file's own UID from the day it was written, which is the + * whole of what is needed to recognise an event that is already here -- and + * nothing looked. Importing an export twice left second copies of everything, + * which the reporter's colleague hit during testing (#173, decided there: + * "duplicate checks on UIDs if UID present in event"). Issue #222. + */ +describe("re-importing events the calendar already has", () => { + const here = (uid: string, calendarId = "cal1") => ({ id: `srv-${uid}`, uid, calendarIds: { [calendarId]: true } }); + + it("skips an event whose uid is already in this calendar", async () => { + const sets = server(PARSED, { existing: [here("uid-one@example.org")] }); + await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 1, skipped: 1 }); + // Only the second event, which has no uid of its own, was sent. + expect(Object.values(sets[0]!.create!).map((e) => e.title)).toEqual(["Retro (no uid)"]); + }); + + it("imports an event whose uid is in a different calendar", async () => { + // A UID is what makes an event the same event *across* calendars, so the + // same event legitimately being in two of them is not a duplicate. + const sets = server(PARSED, { existing: [here("uid-one@example.org", "cal2")] }); + await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 2, skipped: 0 }); + expect(Object.keys(sets[0]!.create!)).toHaveLength(2); + }); + + it("imports an event that arrived with no uid, rather than guessing", async () => { + const sets = server(PARSED, { existing: [here("uid-one@example.org")] }); + await useCalendar.getState().importIcs("x", "cal1"); + 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 () => { + // A file whose every event carries a uid the calendar holds: there is + // nothing to create, and nothing wrong either. + const both = [PARSED[0], { ...PARSED[1], uid: "uid-two@example.org" }]; + const sets = server(both, { existing: [here("uid-one@example.org"), here("uid-two@example.org")] }); + await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 0, skipped: 2 }); + expect(sets).toHaveLength(0); + }); +}); diff --git a/web/src/store/calendar.ts b/web/src/store/calendar.ts index f25ecbe..a137809 100644 --- a/web/src/store/calendar.ts +++ b/web/src/store/calendar.ts @@ -296,8 +296,8 @@ interface CalendarState { findByUid(uid: string): Promise; parseIcs(blobId: Id): Promise; importEvent(event: Partial, calendarId: Id): Promise; - /** Import a whole .ics file. Returns how many events it created. */ - importIcs(text: string, calendarId: Id): Promise; + /** Import a whole .ics file. Says how many it created, and how many were already here. */ + importIcs(text: string, calendarId: Id): Promise<{ created: number; skipped: number }>; applyChanges(types: Set): void; invalidate(): void; setDraft(draft: EventDraft | null): void; @@ -330,6 +330,41 @@ function forImport(event: Partial): Partial { return rest; } +/** + * The UIDs a calendar already holds. + * + * A UID is what makes an event the same event across calendars, and the import + * already keeps the file's own wherever there is one -- so the thing needed to + * recognise a re-import was there all along and nothing looked at it. Asked for + * once per import rather than once per event: `CalendarEvent/query` does take a + * `uid` filter, but a file of two thousand events would be two thousand + * queries. + * + * Read without `expandRecurrences`, so a weekly series is one event with one + * UID rather than one per occurrence, and filtered to the target calendar here + * rather than in the query -- the same event legitimately lives in two + * calendars, and `calendarIds` says which without relying on a filter this + * client has not confirmed the server supports. + */ +async function uidsInCalendar(accountId: Id, calendarId: Id): Promise> { + const uids = new Set(); + const page = client.maxObjectsInGet; + for (let position = 0; ; ) { + const q = await client.call("CalendarEvent/query", { accountId, position, limit: page }); + const ids = q.ids ?? []; + if (!ids.length) break; + for (const part of chunk(ids, page)) { + const g = await client.call>("CalendarEvent/get", { accountId, ids: part, properties: ["uid", "calendarIds"] }); + for (const e of g.list) if (e.uid && e.calendarIds?.[calendarId]) uids.add(e.uid); + } + position += ids.length; + // `total` is optional, so the empty page above is what actually ends this; + // this only saves the round trip that would find it. + if (q.total != null && position >= q.total) break; + } + return uids; +} + export const useCalendar = create((set, get) => ({ accountId: null, available: false, @@ -827,14 +862,28 @@ export const useCalendar = create((set, get) => ({ const up = await client.upload(accountId, new Blob([text], { type: "text/calendar" }), { type: "text/calendar" }); const events = await get().parseIcs(up.blobId); if (!events.length) throw new Error("it has no events in it"); + const already = await uidsInCalendar(accountId, calendarId); const create: Record = {}; + let skipped = 0; events.forEach((e, i) => { const rest = forImport(e); - // A UID is what makes an event the same event across calendars, so the - // file's own is kept wherever it has one. Only what arrives without gets - // invented, and an event with no UID is not one anything can match to. + /* + * A UID is what makes an event the same event across calendars, so the + * file's own is kept wherever it has one. Only what arrives without gets + * invented, and an event with no UID is not one anything can match to -- + * which is also why an event without one is imported rather than guessed + * about. Re-importing an export used to leave second copies of + * everything; asked for on #173, decided there. + */ + if (rest.uid && already.has(rest.uid)) { + skipped++; + return; + } create[`e${i}`] = { "@type": "Event", ...rest, uid: rest.uid || crypto.randomUUID(), calendarIds: { [calendarId]: true } }; }); + // Everything in the file was already here. Nothing to send, and nothing + // wrong either -- say so rather than reporting an import of no events. + if (!Object.keys(create).length) return { created: 0, skipped }; const keys = Object.keys(create); let created = 0; let refused: SetError | undefined; @@ -858,7 +907,7 @@ export const useCalendar = create((set, get) => ({ // Nothing at all got in: say why rather than report importing zero events // as though the file had been empty. if (!created) throw new Error(refused ? setErrorMessage(refused) : "the server did not accept any of its events"); - return created; + return { created, skipped }; }, applyChanges(types) { diff --git a/web/src/views/calendar/CalendarSidebar.tsx b/web/src/views/calendar/CalendarSidebar.tsx index c41329d..499d8e1 100644 --- a/web/src/views/calendar/CalendarSidebar.tsx +++ b/web/src/views/calendar/CalendarSidebar.tsx @@ -45,8 +45,16 @@ export function CalendarSidebar() { const calendarId = importInto.current; if (!calendarId) return; try { - const n = await cal.importIcs(await file.text(), calendarId); - toast.success(plural(n, { one: "Imported {n} event", other: "Imported {n} events" })); + const { created, skipped } = await cal.importIcs(await file.text(), calendarId); + /* + * The two counts are kept apart on purpose. "Imported 40 events" over a + * file of 240 reads as a failure when 200 of them were simply already + * here, and a re-import where everything is already here would otherwise + * report importing nothing at all. + */ + if (!created) toast.success(plural(skipped, { one: "Already here: {n} event, nothing imported", other: "Already here: {n} events, nothing imported" })); + else if (skipped) toast.success(`${plural(created, { one: "Imported {n} event", other: "Imported {n} events" })} · ${plural(skipped, { one: "{n} was already here", other: "{n} were already here" })}`); + else toast.success(plural(created, { one: "Imported {n} event", other: "Imported {n} events" })); } catch (err) { toast.error(t("Could not import this file: {error}", { error: (err as Error).message })); }