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