From bda118bc556408fd54571ab4fd794e75b3473967 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 04:03:05 +0200 Subject: [PATCH] fix: keep occurrence changes on their days when a series moves or ends Three faults in how a series' changed and cancelled occurrences follow a change of the whole series (found in review, live on 1.0 and 0.16.23 for the web's version of the same code): - A rule that names its days - a series on Mondays and Tuesdays, one on the 5th and the 6th of each month - keeps its occurrences on those days when its start moves. Moving every occurrence by a day moved each override along anyway, so the Monday's landed on the Tuesday's key and one silently overwrote the other: the cancelled Tuesday came back with the Monday's title, and the Monday lost its change. Under such a rule every override but the first occurrence's now stays on its day, at the series' new time; the same holds for the new series of "this and following". Two overrides that still meet on one key (an hourly series turned all-day) no longer overwrite each other: the one that stays where it is, else the first, is kept, and the others are left out and logged. - "Does not repeat" for every occurrence kept the first occurrence's override on the single event, because a series' start counts as an occurrence of any rule: cancelled, 1.0 left the event out of every range query (still stored); retitled, 0.16.23 listed it twice. A series made single now keeps no overrides. - The SEQUENCE a change of every occurrence raises (0.16.6 to 0.16.23 refuse a lower one) came from the series as the app held it. A series opened from a reminder or a link was that copy, possibly older than the server's. The store now reads a series from the server every time a scope edit asks for it, so the raise, the overrides a split or a move rewrites and the rule all start from what the server holds. --- .../__tests__/recurrence-scope-edit.test.ts | 141 ++++++++++++++++++ src/lib/recurrence-scope-edit.ts | 65 +++++++- .../calendar-store-series-edits.test.ts | 33 ++++ src/stores/__tests__/calendar-store.test.ts | 16 +- src/stores/calendar-store.ts | 25 ++-- 5 files changed, 254 insertions(+), 26 deletions(-) diff --git a/src/lib/__tests__/recurrence-scope-edit.test.ts b/src/lib/__tests__/recurrence-scope-edit.test.ts index 888e7eab..0dab8934 100644 --- a/src/lib/__tests__/recurrence-scope-edit.test.ts +++ b/src/lib/__tests__/recurrence-scope-edit.test.ts @@ -1239,3 +1239,144 @@ describe.each<[string, Version]>([['1.0', '1.0'], ['0.16', '0.16']])('the series expect(deps.deleteEvent).not.toHaveBeenCalled(); }); }); + +// A rule that names its days keeps its occurrences on them: a Monday and +// Tuesday series moved by a day for every occurrence still meets on Mondays +// and Tuesdays. Moved along, the Monday's override landed on the Tuesday's +// and one overwrote the other: the cancelled Tuesday came back with the +// Monday's title, and the Monday lost its change (live on 1.0 and 0.16.23). +describe.each<[string, Version]>([['1.0', '1.0'], ['0.16', '0.16']])('overrides of a series whose rule names its days on Stalwart %s', (_label, version) => { + const participants = { org: participant(version, ORGANIZER, true, 'accepted'), b: participant(version, ATTENDEE, false, 'accepted') }; + const special = (start: string, title: string) => (version === '1.0' + ? { title } + : { updated: '2026-11-20T09:00:00Z', start, duration: 'PT1H', participants, title }); + function withDays(rule: Record, overrides: Record, start = '2026-12-07T10:00:00'): CalendarEvent { + return { + ...series(version), start, utcStart: berlinUtc(start), utcEnd: berlinUtc(start, 1), + participants, recurrenceRules: [{ '@type': 'RecurrenceRule', ...rule }], recurrenceOverrides: overrides, + } as unknown as CalendarEvent; + } + const mondayAndTuesday = () => withDays( + { frequency: 'weekly', byDay: [{ day: 'mo' }, { day: 'tu' }], count: 8 }, + { '2026-12-14T10:00:00': special('2026-12-14T10:00:00', 'Planning, Monday special'), '2026-12-15T10:00:00': { excluded: true } }, + ); + const allFrom = (master: CalendarEvent, recurrenceId: string, hours: number) => { + const occurrence = { + ...master, id: 'maaaaaf', baseEventId: 'b', start: recurrenceId, recurrenceId, recurrenceIdTimeZone: 'Europe/Berlin', + utcStart: berlinUtc(recurrenceId), utcEnd: berlinUtc(recurrenceId, 1), + } as CalendarEvent; + const updates = seriesUpdatesFromOccurrence(occurrence, master, editorUpdates(occurrence), editorOn(occurrence, moveBy(occurrence, hours)), context); + // Unchanged overrides are not written again. + return { updates, overrides: (updates.recurrenceOverrides ?? master.recurrenceOverrides) as Record> }; + }; + + it('keeps each override on its day when every occurrence moves by a day', () => { + const master = mondayAndTuesday(); + const { updates, overrides } = allFrom(master, '2026-12-21T10:00:00', 24); + + expect(updates.start).toBe('2026-12-08T10:00:00'); + expect(Object.keys(overrides).sort()).toEqual(['2026-12-14T10:00:00', '2026-12-15T10:00:00']); + expect(overrides['2026-12-14T10:00:00']).toMatchObject({ title: 'Planning, Monday special' }); + if (version === '0.16') expect(overrides['2026-12-14T10:00:00'].start).toBe('2026-12-14T10:00:00'); + expect(overrides['2026-12-15T10:00:00']).toEqual({ excluded: true }); + }); + + it('keeps each override on its day, at the new time, when the series also moves by an hour', () => { + const master = mondayAndTuesday(); + const { overrides } = allFrom(master, '2026-12-21T10:00:00', 25); + + expect(Object.keys(overrides).sort()).toEqual(['2026-12-14T11:00:00', '2026-12-15T11:00:00']); + expect(overrides['2026-12-14T11:00:00']).toMatchObject({ title: 'Planning, Monday special' }); + if (version === '0.16') expect(overrides['2026-12-14T11:00:00'].start).toBe('2026-12-14T11:00:00'); + expect(overrides['2026-12-15T11:00:00']).toEqual({ excluded: true }); + }); + + it('keeps month days as well', () => { + // The 5th and the 6th of each month; 5 January retitled, 6 January cancelled. + const master = withDays( + { frequency: 'monthly', byMonthDay: [5, 6], count: 6 }, + { '2027-01-05T10:00:00': special('2027-01-05T10:00:00', 'Planning, January'), '2027-01-06T10:00:00': { excluded: true } }, + '2026-12-05T10:00:00', + ); + const { overrides } = allFrom(master, '2027-02-05T10:00:00', 24); + + expect(Object.keys(overrides).sort()).toEqual(['2027-01-05T10:00:00', '2027-01-06T10:00:00']); + expect(overrides['2027-01-05T10:00:00']).toMatchObject({ title: 'Planning, January' }); + expect(overrides['2027-01-06T10:00:00']).toEqual({ excluded: true }); + }); + + it('keeps them on their days for this and the following occurrences too', async () => { + const master = mondayAndTuesday(); + const occurrence = { + ...master, id: 'maaaaaf', baseEventId: 'b', start: '2026-12-08T10:00:00', recurrenceId: '2026-12-08T10:00:00', + recurrenceIdTimeZone: 'Europe/Berlin', utcStart: berlinUtc('2026-12-08T10:00:00'), utcEnd: berlinUtc('2026-12-08T10:00:00', 1), + } as CalendarEvent; + const deps = actions(master); + + // From Tuesday 8 December on, moved by a day to the Wednesday. + await saveOccurrenceEdit('this_and_future', { + event: occurrence, updates: editorUpdates(occurrence), editor: editorOn(occurrence, moveBy(occurrence, 24)), + sendScheduling: true, userEmails: [ORGANIZER], + }, deps); + + const created = deps.createEvent.mock.calls[0][0]; + expect(created.start).toBe('2026-12-09T10:00:00'); + expect(Object.keys(created.recurrenceOverrides ?? {}).sort()).toEqual(['2026-12-14T10:00:00', '2026-12-15T10:00:00']); + expect(created.recurrenceOverrides!['2026-12-14T10:00:00']).toMatchObject({ title: 'Planning, Monday special' }); + expect(created.recurrenceOverrides!['2026-12-15T10:00:00']).toEqual({ excluded: true }); + }); +}); + +// Two occurrences can still meet on one key - an hourly series turned +// all-day puts every hour of a day on that day. One override is kept, the +// first; none overwrites another. +describe('overrides that would meet on one occurrence', () => { + it('keeps the first and leaves out the others instead of overwriting it', () => { + const master = { + ...series('1.0'), start: '2026-12-07T10:00:00', utcStart: '2026-12-07T09:00:00Z', utcEnd: '2026-12-07T10:00:00Z', + recurrenceRules: [{ '@type': 'RecurrenceRule', frequency: 'hourly', count: 3 }], + recurrenceOverrides: { + '2026-12-07T10:00:00': { title: 'First hour' }, + '2026-12-07T11:00:00': { title: 'Second hour' }, + '2026-12-07T12:00:00': { excluded: true }, + }, + } as unknown as CalendarEvent; + const occurrence = { + ...master, id: 'maaaaag', baseEventId: 'b', start: '2026-12-07T11:00:00', recurrenceId: '2026-12-07T11:00:00', + utcStart: '2026-12-07T10:00:00Z', utcEnd: '2026-12-07T11:00:00Z', + } as CalendarEvent; + const day = startOfDay(getEventStartDate(occurrence)); + + const updates = seriesUpdatesFromOccurrence( + occurrence, master, + editorUpdates(occurrence, { start: '2026-12-07T00:00:00', duration: 'P1D', timeZone: null, showWithoutTime: true }), + editorOn(occurrence, { when: { allDay: true, start: day, end: day } }), context, + ); + + expect(updates.recurrenceOverrides).toEqual({ '2026-12-07T00:00:00': { title: 'First hour' } }); + }); +}); + +// "Repeat: none" for every occurrence makes the series one event. Its start +// is an occurrence of any rule, so the first occurrence's override stayed on +// the single event: cancelled, 1.0 left the event out of every range query +// (still stored); retitled, 0.16.23 listed it twice (live). +describe.each<[string, Version]>([['1.0', '1.0'], ['0.16', '0.16']])('a series made one event on Stalwart %s', (_label, version) => { + it('keeps none of its overrides', () => { + const first = version === '1.0' + ? { excluded: true } + : { start: '2026-10-05T10:00:00', duration: 'PT1H', title: 'Kick-off', participants: series(version).participants }; + const master = { ...series(version), recurrenceOverrides: { ...series(version).recurrenceOverrides, '2026-10-05T10:00:00': first } } as CalendarEvent; + const occurrence = serverOccurrence(version); + + const updates = seriesUpdatesFromOccurrence( + occurrence, master, + // What the editor writes for "Does not repeat" on a series. + editorUpdates(occurrence, { recurrenceRules: null, recurrenceOverrides: null, excludedRecurrenceRules: null }), + editorOn(occurrence, { ruleChanged: true }), context, + ); + + expect(updates.recurrenceRules).toBeNull(); + expect(updates.recurrenceOverrides).toBeNull(); + }); +}); diff --git a/src/lib/recurrence-scope-edit.ts b/src/lib/recurrence-scope-edit.ts index 6d615179..08a7d0de 100644 --- a/src/lib/recurrence-scope-edit.ts +++ b/src/lib/recurrence-scope-edit.ts @@ -43,6 +43,7 @@ import { isValidTimeZone, localDateTimeToInstant, } from './calendar-timezone'; +import { debug } from './debug'; /** What the event editor had when it saved, besides the event data it wrote. */ export interface SeriesEdit { @@ -103,6 +104,15 @@ interface SeriesFrame { recurrenceRules: CalendarEvent['recurrenceRules']; } +/** + * Whether `rules` name the days their occurrences fall on (BYDAY, + * BYMONTHDAY, BYYEARDAY): a start moved by days does not take those + * occurrences along. + */ +function namesItsDays(rules: CalendarEvent['recurrenceRules']): boolean { + return (rules ?? []).some((rule) => !!rule.byDay?.length || !!rule.byMonthDay?.length || !!rule.byYearDay?.length); +} + /** * The key an override of the series `from` keeps in the series `to`, or * null when `to` has no such occurrence any more. It moves with its @@ -112,16 +122,52 @@ interface SeriesFrame { * where it was - a weekday rule the start moved away from - keeps the key, * and an occurrence the new rule does not give is gone. A date the override * adds to the series, which no rule gives, moves along. + * + * A rule that names its days keeps every occurrence but the anchor on its + * day, at the series' new time: moved by a day, the Monday of a Monday and + * Tuesday series is still that Monday. Moved along, its override landed on + * the Tuesday's and took its place: a cancelled Tuesday came back with the + * Monday's title, and the Monday lost its change (live on 1.0 and 0.16.23). */ function overrideKeyIn(key: string, from: SeriesFrame, to: SeriesFrame, anchor = from.start): string | null { const shifted = movedLike(key, anchor, to.start); const moved = to.showWithoutTime ? `${shifted.slice(0, 10)}T00:00:00` : shifted; if (!seriesOccursAt(from.start, from.recurrenceRules, key)) return moved; + if (namesItsDays(to.recurrenceRules) && wallClockMs(key) !== wallClockMs(anchor)) { + const sameDay = `${key.slice(0, 10)}${moved.slice(10)}`; + return seriesOccursAt(to.start, to.recurrenceRules, sameDay) ? sameDay : null; + } if (seriesOccursAt(to.start, to.recurrenceRules, moved)) return moved; if (moved !== key && seriesOccursAt(to.start, to.recurrenceRules, key)) return key; return null; } +/** + * `overrides` with the key each takes (keyOf), those without one left out, + * and never two on one key: an override that stays where it is comes first, + * and one that would land on a key already taken is left out and logged + * rather than overwrite it. Two occurrences of one series never share a + * recurrence id. + */ +function rekeyed( + overrides: OverrideMap, + keyOf: (key: string) => string | null, +): Array<{ key: string; target: string; override: OverrideMap[string] }> { + const moves = Object.entries(overrides) + .map(([key, override]) => ({ key, target: keyOf(key), override })) + .filter((move): move is { key: string; target: string; override: OverrideMap[string] } => move.target !== null); + moves.sort((a, b) => Number(b.target === b.key) - Number(a.target === a.key)); + const taken = new Set(); + return moves.filter((move) => { + if (!taken.has(move.target)) { + taken.add(move.target); + return true; + } + debug.warn('calendar', 'Two recurrence overrides map to one occurrence; keeping the first', move.target, move.key); + return false; + }); +} + /** * `entry` with its attendees asked again (participantsAskedAgain): a whole * participants map (0.16) with their answers back at needs-action, a @@ -362,13 +408,17 @@ function seriesOverrides( showWithoutTime: series.showWithoutTime ?? !!master.showWithoutTime, recurrenceRules: options.ruleChanged ? series.recurrenceRules ?? null : master.recurrenceRules, }; + // A series made single ("Repeat: none") keeps no overrides. Its start is + // an occurrence of any rule (seriesOccursAt), so the first occurrence's + // override stayed on the single event: cancelled, it hid the event from + // every range query on 1.0 (still stored); retitled, 0.16.23 listed it + // twice (live). + if (options.ruleChanged && !to.recurrenceRules?.length) return null; const turned = from.showWithoutTime !== to.showWithoutTime; const opened = resolveOverrideKey({ start: occurrence.start, recurrenceId: occurrence.recurrenceId ?? master.start }, overrides); const result: OverrideMap = {}; - for (const [key, override] of Object.entries(overrides)) { - const target = overrideKeyIn(key, from, to); - if (!target) continue; + for (const { key, target, override } of rekeyed(overrides, (key) => overrideKeyIn(key, from, to))) { const { updated: _updated, ...rest } = override as OverrideEntry; let entry: OverrideEntry = rest; if (!entry.excluded) { @@ -380,7 +430,10 @@ function seriesOverrides( const pinned = wallClockMs(entry.start) === wallClockMs(key) && (entry.timeZone === undefined || entry.timeZone === master.timeZone); follows = pinned || key === opened; - if (follows) entry.start = movedLike(entry.start, from.start, to.start); + // The time its occurrence had is the time it has now; one of its + // own takes the editor's move when the editor was opened on it. + if (pinned) entry.start = target; + else if (follows) entry.start = movedLike(entry.start, from.start, to.start); } if (resized && typeof entry.duration === 'string' && (key === opened || parseDurationSeconds(entry.duration) === parseDurationSeconds(master.duration))) { @@ -730,9 +783,7 @@ function followingOverrides( const turned = from.showWithoutTime !== to.showWithoutTime; const resized = parseDurationSeconds(to.duration) !== parseDurationSeconds(series.duration); const result: OverrideMap = {}; - for (const [key, override] of Object.entries(overrides)) { - const target = overrideKeyIn(key, from, to, splitKey); - if (!target) continue; + for (const { key, target, override } of rekeyed(overrides, (key) => overrideKeyIn(key, from, to, splitKey))) { const opened = key === splitKey; const { updated: _updated, ...rest } = override as OverrideEntry; let entry = withParticipantsOf(rest, to.participants); diff --git a/src/stores/__tests__/calendar-store-series-edits.test.ts b/src/stores/__tests__/calendar-store-series-edits.test.ts index c6c5e572..32746a22 100644 --- a/src/stores/__tests__/calendar-store-series-edits.test.ts +++ b/src/stores/__tests__/calendar-store-series-edits.test.ts @@ -113,6 +113,8 @@ describe('the series itself opened from a reminder on the calendar store', () => it.each(['0.16.23', '1.0'])('overrides its first occurrence in the owner\'s account on Stalwart %s', async () => { useCalendarStore.setState({ events: [] }); + // As the owner's account holds it: no overrides yet. + mockGetEvents.mockImplementation(async (ids: string[]) => (ids[0] === 'b' ? [{ ...master, recurrenceOverrides: null }] : [])); const store = useCalendarStore.getState(); await saveOccurrenceEdit('this', { @@ -192,3 +194,34 @@ describe('a change of an occurrence whose override has no organizer (Stalwart 0. }); }); }); + +// The SEQUENCE a change of every occurrence raises is the server's: a series +// opened from a reminder a while ago - or shown as itself in the grid - is +// an older copy, and a raise from its number goes out lower than what the +// attendees hold, which 0.16 (0.16.6 to 0.16.23) refuses. +describe.each(['0.16.23', '1.0'])('a change of every occurrence from a series opened a while ago on Stalwart %s', (version) => { + it('raises the SEQUENCE the server holds, not the stale copy\'s', async () => { + const overrides = version === '1.0' + ? { '2026-10-19T10:00:00': { title: 'Planning, special' } } + : { '2026-10-19T10:00:00': { title: 'Planning, special', start: '2026-10-19T10:00:00', duration: 'PT1H', sequence: 2 } }; + const stale = { ...master, sequence: 2, recurrenceOverrides: overrides } as unknown as CalendarEvent; + const current = { + ...master, sequence: 5, + recurrenceOverrides: version === '1.0' ? overrides : { '2026-10-19T10:00:00': { ...overrides['2026-10-19T10:00:00'], sequence: 5 } }, + } as unknown as CalendarEvent; + mockGetEvents.mockImplementation(async (ids: string[]) => (ids[0] === 'b' ? [current] : [])); + useCalendarStore.setState({ events: [] }); + const store = useCalendarStore.getState(); + + await saveOccurrenceEdit('all', { + event: stale, + updates: { title: 'Planning, room 2', start: stale.start, duration: 'PT1H', timeZone: 'Europe/Berlin', participants }, + editor: { attendees: [{ name: '', email: 'userb@example.org' }], when: editorTimesFromEvent(stale) }, + sendScheduling: true, userEmails: [ORGANIZER], + }, { updateEvent: store.updateEvent, createEvent: store.createEvent, getMasterEvent: store.getMasterEvent }); + + expect(mockUpdate.mock.calls.map(([id, patch, schedule]) => [id, patch, schedule])).toEqual([ + ['b', { title: 'Planning, room 2', sequence: 6, 'recurrenceOverrides/2026-10-19T10:00:00/sequence': 6 }, true], + ]); + }); +}); diff --git a/src/stores/__tests__/calendar-store.test.ts b/src/stores/__tests__/calendar-store.test.ts index 2d9a8b68..8076f3da 100644 --- a/src/stores/__tests__/calendar-store.test.ts +++ b/src/stores/__tests__/calendar-store.test.ts @@ -1034,10 +1034,18 @@ describe('calendar-store', () => { expect(master?.start).toBe('2026-03-02T09:00:00'); }); - it('getMasterEvent returns a master as-is', async () => { - const ev = { id: 'ev1', recurrenceRules: [{ frequency: 'daily' }] } as any; - expect(await useCalendarStore.getState().getMasterEvent(ev)).toBe(ev); - expect(mockGetEvents).not.toHaveBeenCalled(); + // A copy the app holds - opened from a reminder a while ago, or shown as + // itself in the grid - may be older than the server's: the scope edits + // build on its overrides and its SEQUENCE. + it('getMasterEvent reads a master again from the server', async () => { + useCalendarStore.setState({ calendars: [], events: [] }); + const ev = { id: 'ev1', recurrenceRules: [{ frequency: 'daily' }], sequence: 1 } as any; + mockGetEvents.mockResolvedValue([{ ...ev, sequence: 4 }]); + + const master = await useCalendarStore.getState().getMasterEvent(ev); + + expect(mockGetEvents).toHaveBeenCalledWith(['ev1'], undefined); + expect(master?.sequence).toBe(4); }); }); diff --git a/src/stores/calendar-store.ts b/src/stores/calendar-store.ts index eaeefa45..49c9698b 100644 --- a/src/stores/calendar-store.ts +++ b/src/stores/calendar-store.ts @@ -254,8 +254,8 @@ export interface CalendarState { options?: { sendSchedulingMessages?: boolean }, ) => Promise; deleteEvent: (id: string) => Promise; - // Resolve a client-side expanded occurrence (or a master) to its master - // event, fetching it from the server when the expansion replaced it. + // Resolve an expanded occurrence (or a master) to its master event, read + // from the server: a copy the app holds may be older. getMasterEvent: (event: CalendarEvent) => Promise; // Invitations / scheduling rsvpEvent: ( @@ -1044,25 +1044,20 @@ export const useCalendarStore = create()( }, getMasterEvent: async (event) => { - if (event.recurrenceRules?.length && !event.recurrenceId && !isServerRecurrenceInstance(event)) { - // The series itself, opened from a reminder or a link rather than the - // grid: known from here on, so a change to it is sent to its own - // account under its raw id (resolveMutationTarget). - if (!get().events.some((e) => e.id === event.id)) knownMasters.set(event.id, event); - return event; - } // Expansion replaces the master with its occurrences, each pointing back // at the stored event: through baseEventId when the server expanded it, - // through originalId when the device did. + // through originalId when the device did. The stored event is read + // again every time, also for the series itself (opened from a reminder + // or a link) and one the grid shows as itself: a copy held here can be + // older than the server's, and the scope edits build on its overrides + // and its SEQUENCE - a change that raises it from a stale one goes out + // lower than the attendees' copies, which 0.16 refuses. const realId = seriesIdOf(event); - const inStore = get().events.find( - (e) => !e.recurrenceId && !!e.recurrenceRules?.length && !isServerRecurrenceInstance(e) - && seriesIdOf(e) === realId && (e.accountId ?? undefined) === (event.accountId ?? undefined), - ); - if (inStore) return inStore; const fetched = (await fetchEvents([realId], event.accountId)) ?? []; const master = fetched[0]; if (!master) return null; + // Known from here on, so a change to it is sent to its own account under + // its raw id (resolveMutationTarget), whether or not the grid shows it. const mapped = mapServerEventToStoreEvent(master, get().calendars, event.accountId); knownMasters.set(mapped.id, mapped); return mapped;