Merge pull request #225 from Coffey-Labs/feat/skip-duplicate-uids-on-import

Skip events on re-import that the calendar already has

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" },

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" },

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" },

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

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" },

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" },

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

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

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 —

web/src/store/__tests__/ics-import.test.ts

@@ -38,8 +38,9 @@ interface SetArgs { create?: Record<string, Record<string, unknown>>; 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<string, unknown>; max?: number; failOn?: number } = {}) {
+function server(parsed: unknown, opts: { notCreated?: Record<string, unknown>; max?: number; failOn?: number; existing?: Array<{ id: string; uid: string; calendarIds: Record<string, boolean> }> } = {}) {
   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, unknown>, string][] };
     const methodResponses = body.methodCalls.map(([name, args, id]) => {
@@ -63,6 +64,16 @@ function server(parsed: unknown, opts: { notCreated?: Record<string, unknown>; 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("[email protected]")] });
+    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("[email protected]", "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("[email protected]")] });
+    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: "[email protected]" }];
+    const sets = server(both, { existing: [here("[email protected]"), here("[email protected]")] });
+    await expect(useCalendar.getState().importIcs("x", "cal1")).resolves.toEqual({ created: 0, skipped: 2 });
+    expect(sets).toHaveLength(0);
+  });
+});

web/src/store/calendar.ts

@@ -296,8 +296,8 @@ interface CalendarState {
   findByUid(uid: string): Promise<CalendarEvent | null>;
   parseIcs(blobId: Id): Promise<CalendarEvent[]>;
   importEvent(event: Partial<CalendarEvent>, calendarId: Id): Promise<Id>;
-  /** Import a whole .ics file. Returns how many events it created. */
-  importIcs(text: string, calendarId: Id): Promise<number>;
+  /** 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<string>): void;
   invalidate(): void;
   setDraft(draft: EventDraft | null): void;
@@ -330,6 +330,41 @@ function forImport(event: Partial<CalendarEvent>): Partial<CalendarEvent> {
   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<Set<string>> {
+  const uids = new Set<string>();
+  const page = client.maxObjectsInGet;
+  for (let position = 0; ; ) {
+    const q = await client.call<QueryResponse>("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<GetResponse<CalendarEvent>>("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<CalendarState>((set, get) => ({
   accountId: null,
   available: false,
@@ -827,14 +862,28 @@ export const useCalendar = create<CalendarState>((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<string, unknown> = {};
+    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<CalendarState>((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) {

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