From c1c529d4bda454e67b4115c077622d7eb6fd4473 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 15 Oct 2026 16:25:34 +0200 Subject: [PATCH] Keep deleted and edited occurrences when moving a series (#404) Moving a recurring event to another calendar re-inserted its exception rows under the new event. That event has no `_sync_id` yet, so the rows never attached: deleted occurrences came back, and edited ones showed twice (original plus the edited copy). The move now does what delete and edit-this-occurrence already do for unsynced series: cancelled occurrences become EXDATE entries on the moved series, edited ones become EXDATE plus a standalone event on the target calendar. The standalone copies are rolled back with the rest if the move fails. `buildCopiedExceptionValues` is gone with its tests, and `ExceptionRowSnapshot`/`ExceptionProjection` only read the columns still used. Adds `MoveEventExceptionsTest` (instrumented), which fails on the old code and passes now. Co-authored-by: Jean-Luc Makiola Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/404 --- .../data/calendar/MoveEventExceptionsTest.kt | 91 ++++++++++++++++++ .../data/calendar/CalendarDataSource.kt | 96 ++++++++++--------- .../data/calendar/EventWriteMapper.kt | 56 ++++------- .../calendula/data/calendar/Projections.kt | 24 +---- .../data/calendar/EventWriteMapperTest.kt | 54 +++-------- 5 files changed, 176 insertions(+), 145 deletions(-) create mode 100644 app/src/androidTest/java/de/jeanlucmakiola/calendula/data/calendar/MoveEventExceptionsTest.kt diff --git a/app/src/androidTest/java/de/jeanlucmakiola/calendula/data/calendar/MoveEventExceptionsTest.kt b/app/src/androidTest/java/de/jeanlucmakiola/calendula/data/calendar/MoveEventExceptionsTest.kt new file mode 100644 index 00000000..c9668d5c --- /dev/null +++ b/app/src/androidTest/java/de/jeanlucmakiola/calendula/data/calendar/MoveEventExceptionsTest.kt @@ -0,0 +1,91 @@ +package de.jeanlucmakiola.calendula.data.calendar + +import android.Manifest +import android.content.ContentUris +import android.content.ContentValues +import android.provider.CalendarContract +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import androidx.test.rule.GrantPermissionRule +import com.google.common.truth.Truth.assertThat +import de.jeanlucmakiola.calendula.domain.toEditForm +import kotlinx.datetime.TimeZone +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Moving a synced series keeps its deleted and edited occurrences, although the + * moved row has no `_sync_id` for exception rows to attach to. + */ +@RunWith(AndroidJUnit4::class) +class MoveEventExceptionsTest { + + @get:Rule + val permissionRule: GrantPermissionRule = GrantPermissionRule.grant( + Manifest.permission.READ_CALENDAR, + Manifest.permission.WRITE_CALENDAR, + ) + + private val context = InstrumentationRegistry.getInstrumentation().targetContext + private val resolver = context.contentResolver + private val ds = AndroidCalendarDataSource(context) + private val zone = TimeZone.currentSystemDefault() + + private val day = 86_400_000L + private val start = 1_893_492_000_000L // 2030-01-01T10:00Z + private val hour = 3_600_000L + + /** "day offset:title" of every instance on [calendarId]. */ + private fun occurrences(calendarId: Long): List = + ds.instances(start - day, start + 10 * day) + .filter { it.calendarId == calendarId } + .map { "${(it.start.toEpochMilliseconds() - start) / day}:${it.title}" } + .sorted() + + @Test + fun movedSeriesKeepsCancelledAndModifiedOccurrences() { + val a = ds.createLocalCalendar("move-test-a", 0xFF336699.toInt(), null) + val b = ds.createLocalCalendar("move-test-b", 0xFF993366.toInt(), null) + try { + val src = insertSyncedSeries(a) + ds.deleteOccurrence(src, start + 2 * day) + val occurrence = ds.eventDetail(src, 540)!!.toEditForm(start + 3 * day, start + 3 * day + hour, zone) + ds.updateOccurrence(src, start + 3 * day, occurrence, occurrence.copy(title = "edited"), 540) + val expected = listOf("0:series", "1:series", "3:edited", "4:series") + assertThat(occurrences(a)).isEqualTo(expected) + + val form = ds.eventDetail(src, 540)!!.toEditForm(start, start + hour, zone) + ds.moveEvent(src, b, form, form, 540) + + assertThat(occurrences(a)).isEmpty() + assertThat(occurrences(b)).isEqualTo(expected) + } finally { + ds.deleteCalendar(a) + ds.deleteCalendar(b) + } + } + + /** A daily series carrying a `_sync_id`, so its exceptions are real exception rows. */ + private fun insertSyncedSeries(calendarId: Long): Long { + val accountName = resolver.query( + ContentUris.withAppendedId(CalendarContract.Calendars.CONTENT_URI, calendarId), + arrayOf(CalendarContract.Calendars.ACCOUNT_NAME), null, null, null, + )!!.use { it.moveToFirst(); it.getString(0) } + val uri = CalendarContract.Events.CONTENT_URI.buildUpon() + .appendQueryParameter(CalendarContract.CALLER_IS_SYNCADAPTER, "true") + .appendQueryParameter(CalendarContract.Calendars.ACCOUNT_NAME, accountName) + .appendQueryParameter(CalendarContract.Calendars.ACCOUNT_TYPE, CalendarContract.ACCOUNT_TYPE_LOCAL) + .build() + val values = ContentValues().apply { + put(CalendarContract.Events.CALENDAR_ID, calendarId) + put(CalendarContract.Events.TITLE, "series") + put(CalendarContract.Events.DTSTART, start) + put(CalendarContract.Events.DURATION, "P3600S") + put(CalendarContract.Events.RRULE, "FREQ=DAILY;COUNT=5") + put(CalendarContract.Events.EVENT_TIMEZONE, "UTC") + put(CalendarContract.Events._SYNC_ID, "move-test-series") + } + return ContentUris.parseId(resolver.insert(uri, values)!!) + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt index b5f3f5cf..65054173 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt @@ -36,7 +36,10 @@ import de.jeanlucmakiola.calendula.domain.contacts.SpecialDateType import de.jeanlucmakiola.calendula.domain.ics.IcsEvent import de.jeanlucmakiola.calendula.domain.ics.ParsedIcsEvent import de.jeanlucmakiola.calendula.domain.ics.semanticReminderMinutes +import de.jeanlucmakiola.calendula.domain.ics.parseRfc2445DurationMillis import de.jeanlucmakiola.calendula.domain.rruleTruncatedAt +import de.jeanlucmakiola.calendula.domain.toEditForm +import kotlinx.datetime.TimeZone import kotlinx.datetime.toJavaLocalDate import java.time.Instant import java.time.LocalDate @@ -238,8 +241,10 @@ interface CalendarDataSource { * and cancelled occurrences) to [targetCalendarId], returning the new * `Events._ID`. `CALENDAR_ID` is sync-adapter-owned and can't be updated in * place, so this is copy+delete: the row is re-inserted on the target (its - * `UID_2445` preserved), its exceptions, reminders and editable guests - * replayed, the [original]→[updated] field edits applied, then the source + * `UID_2445` preserved), its reminders and editable guests copied, its + * exceptions turned into EXDATEs (modified ones also into standalone events, + * as the new row has no `_sync_id` to attach them to), the + * [original]→[updated] field edits applied, then the source * deleted. The source is removed only once the copy fully succeeds; a failure * before that rolls the new event back, leaving the original untouched. * [allDayReminderTimeMinutes]: see [insertEvent]. @@ -1073,10 +1078,11 @@ class AndroidCalendarDataSource @Inject constructor( buildMovedMasterValues(master.event, targetCalendarId, uid).toContentValues(), )?.let(ContentUris::parseId) ?: throw WriteFailedException("insert moved event into calendar id=$targetCalendarId") + val detached = mutableListOf() try { copyReminderRows(fromEventId = eventId, toEventId = newEventId) insertAttendees(newEventId, editableAttendees(eventId)) - copyExceptions(fromEventId = eventId, toEventId = newEventId) + copyExceptions(eventId, newEventId, targetCalendarId, allDayReminderTimeMinutes, detached) // Apply the user's field edits exactly as a same-calendar "all // events" save would: the moved master carries the source's values, // so the dirty diff against [original] writes only what changed @@ -1086,6 +1092,7 @@ class AndroidCalendarDataSource @Inject constructor( // Undo the partial copy so a failed move leaves nothing behind; the // source is still intact (it's deleted only past this point). runCatching { deleteEvent(newEventId) } + detached.forEach(::rollBackDetached) throw t } deleteEvent(eventId) @@ -1162,44 +1169,47 @@ class AndroidCalendarDataSource @Inject constructor( } /** - * Replay every exception of the series [fromEventId] against the moved series - * [toEventId]. The copy preserved the recurrence skeleton, so each occurrence - * time still resolves: a cancelled occurrence is re-hidden with a cancelled - * exception, a modified one is re-inserted with its fields (then its reminders - * reconciled and editable guests copied). No-op for a non-recurring event. + * Carry every exception of the series [fromEventId] over to the moved series + * [toEventId]. The moved row has no `_sync_id` yet, so exception rows would + * never attach to it ([deleteOccurrence], [detachOccurrence]): a cancelled + * occurrence becomes an EXDATE, a modified one an EXDATE plus a standalone + * event on [targetCalendarId], its id added to [detached] for rollback. */ - private fun copyExceptions(fromEventId: Long, toEventId: Long) { - queryExceptionRows(fromEventId).forEach { ex -> - if (ex.isCancelled) { - val values = ContentValues().apply { - put(CalendarContract.Events.ORIGINAL_INSTANCE_TIME, ex.originalInstanceMillis) - put(CalendarContract.Events.STATUS, CalendarContract.Events.STATUS_CANCELED) - } - resolver.insert( - ContentUris.withAppendedId( - CalendarContract.Events.CONTENT_EXCEPTION_URI, toEventId, - ), - values, - ) ?: throw WriteFailedException("copy cancelled occurrence to event id=$toEventId") - } else { - val uri = resolver.insert( - ContentUris.withAppendedId( - CalendarContract.Events.CONTENT_EXCEPTION_URI, toEventId, - ), - buildCopiedExceptionValues(ex).toContentValues(), - ) ?: throw WriteFailedException("copy modified occurrence to event id=$toEventId") - val newExceptionId = ContentUris.parseId(uri) - // The provider may clone the parent's reminders onto the new - // exception; reconcile to the source exception's exact set so - // they neither double nor drop. Same DTSTART → same all-day - // encoding, so the stored offsets match directly. - reconcileReminders( - newExceptionId, - queryReminders(ex.exceptionEventId).map { it.minutes }, - ) - insertAttendees(newExceptionId, editableAttendees(ex.exceptionEventId)) - } + private fun copyExceptions( + fromEventId: Long, + toEventId: Long, + targetCalendarId: Long, + allDayReminderTimeMinutes: Int, + detached: MutableList, + ) { + val exceptions = queryExceptionRows(fromEventId) + if (exceptions.isEmpty()) return + val zone = TimeZone.currentSystemDefault() + exceptions.filterNot { it.isCancelled }.forEach { ex -> + val detail = eventDetail(ex.exceptionEventId, allDayReminderTimeMinutes) + ?: throw WriteFailedException("read modified occurrence id=${ex.exceptionEventId}") + val endMillis = ex.dtEndMillis ?: (ex.dtStartMillis + parseRfc2445DurationMillis(ex.duration)) + val form = detail.toEditForm(ex.dtStartMillis, endMillis, zone) + .copy(calendarId = targetCalendarId) + .toDetachedOccurrence() + detached += insertEvent(form, allDayReminderTimeMinutes) } + val row = querySeriesRow(toEventId) + val values = buildOccurrencesExdateValues( + existingExdate = row.exdate, + occurrences = exceptions.map { it.originalInstanceMillis }, + dtStartMillis = row.dtStartMillis, + rrule = row.rrule, + rdate = row.rdate, + duration = row.duration, + timezone = row.timezone, + allDay = row.allDay, + ) + val updated = resolver.update( + ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, toEventId), + values.toContentValues(), null, null, + ) + if (updated == 0) throw WriteFailedException("exdate moved exceptions event id=$toEventId") } /** The series' exception rows (modified + cancelled), oldest occurrence first. */ @@ -1218,18 +1228,10 @@ class AndroidCalendarDataSource @Inject constructor( exceptionEventId = r.getLong(ExceptionProjection.IDX_ID), originalInstanceMillis = r.getLong(ExceptionProjection.IDX_ORIGINAL_INSTANCE_TIME), isCancelled = status == CalendarContract.Events.STATUS_CANCELED, - status = status, - title = r.getString(ExceptionProjection.IDX_TITLE).orEmpty(), - isAllDay = r.getInt(ExceptionProjection.IDX_ALL_DAY) != 0, dtStartMillis = r.getLong(ExceptionProjection.IDX_DTSTART), dtEndMillis = r.getLong(ExceptionProjection.IDX_DTEND) .takeUnless { r.isNull(ExceptionProjection.IDX_DTEND) }, duration = r.getString(ExceptionProjection.IDX_DURATION), - timezone = r.getString(ExceptionProjection.IDX_EVENT_TIMEZONE), - availability = r.getInt(ExceptionProjection.IDX_AVAILABILITY), - accessLevel = r.getInt(ExceptionProjection.IDX_ACCESS_LEVEL), - location = r.getString(ExceptionProjection.IDX_LOCATION), - description = r.getString(ExceptionProjection.IDX_DESCRIPTION), ) } } ?: emptyList() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt index 9750c7ea..fe5903f2 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt @@ -372,48 +372,11 @@ internal data class ExceptionRowSnapshot( val exceptionEventId: Long, val originalInstanceMillis: Long, val isCancelled: Boolean, - val status: Int?, - val title: String, - val isAllDay: Boolean, val dtStartMillis: Long, val dtEndMillis: Long?, val duration: String?, - val timezone: String?, - val availability: Int, - val accessLevel: Int, - val location: String?, - val description: String?, ) -/** - * Column values replaying a *modified* occurrence [snapshot] against the moved - * series via `CONTENT_EXCEPTION_URI`. Like [buildOccurrenceExceptionValues] the - * length travels as DURATION (the provider rejects DTEND on an exception). A - * cancelled occurrence is written separately (ORIGINAL_INSTANCE_TIME + - * STATUS_CANCELED) — this builder is only for the modified case. - */ -internal fun buildCopiedExceptionValues(snapshot: ExceptionRowSnapshot): Map = - buildMap { - put(CalendarContract.Events.ORIGINAL_INSTANCE_TIME, snapshot.originalInstanceMillis) - put(CalendarContract.Events.TITLE, snapshot.title) - put(CalendarContract.Events.ALL_DAY, if (snapshot.isAllDay) 1 else 0) - put(CalendarContract.Events.DTSTART, snapshot.dtStartMillis) - put( - CalendarContract.Events.DURATION, - snapshot.duration?.takeIf { it.isNotBlank() } - ?: rfc2445Duration( - (snapshot.dtEndMillis ?: snapshot.dtStartMillis) - snapshot.dtStartMillis, - snapshot.isAllDay, - ), - ) - put(CalendarContract.Events.EVENT_TIMEZONE, snapshot.timezone ?: "UTC") - put(CalendarContract.Events.AVAILABILITY, snapshot.availability) - put(CalendarContract.Events.ACCESS_LEVEL, snapshot.accessLevel) - put(CalendarContract.Events.EVENT_LOCATION, snapshot.location?.ifEmpty { null }) - put(CalendarContract.Events.DESCRIPTION, snapshot.description?.ifEmpty { null }) - snapshot.status?.let { put(CalendarContract.Events.STATUS, it) } - } - /** * Column values for a *cancelled*-occurrence exception row ("delete only this * event"): inserting them at `Events.CONTENT_EXCEPTION_URI/` makes the @@ -479,9 +442,24 @@ internal fun buildOccurrenceExdateValues( duration: String?, timezone: String?, allDay: Int, +): Map = buildOccurrencesExdateValues( + existingExdate, listOf(occurrenceMillis), dtStartMillis, rrule, rdate, duration, timezone, allDay, +) + +/** [buildOccurrenceExdateValues] excluding every one of [occurrences] in one write. */ +internal fun buildOccurrencesExdateValues( + existingExdate: String?, + occurrences: List, + dtStartMillis: Long, + rrule: String?, + rdate: String?, + duration: String?, + timezone: String?, + allDay: Int, ): Map = mapOf( - CalendarContract.Events.EXDATE to - mergedExdate(existingExdate, occurrenceMillis, allDay != 0, timezone), + CalendarContract.Events.EXDATE to occurrences.fold(existingExdate) { exdate, millis -> + mergedExdate(exdate, millis, allDay != 0, timezone) + }, CalendarContract.Events.DTSTART to dtStartMillis, CalendarContract.Events.RRULE to rrule, CalendarContract.Events.RDATE to rdate, diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt index e9e5a467..47caa39e 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt @@ -251,40 +251,26 @@ internal object MoveMasterProjection { /** * The exception rows of a recurring series (`ORIGINAL_ID` = the series), read to - * replay them against a moved copy (see [ExceptionRowSnapshot]). Both modified + * carry them over to a moved copy (see [ExceptionRowSnapshot]). Both modified * occurrences and cancellations (`STATUS_CANCELED`) are read; the query filters - * `DELETED = 0` so provider tombstones aren't replayed. + * `DELETED = 0` so provider tombstones aren't carried over. */ internal object ExceptionProjection { val COLUMNS: Array = arrayOf( CalendarContract.Events._ID, CalendarContract.Events.ORIGINAL_INSTANCE_TIME, CalendarContract.Events.STATUS, - CalendarContract.Events.TITLE, - CalendarContract.Events.ALL_DAY, CalendarContract.Events.DTSTART, CalendarContract.Events.DTEND, CalendarContract.Events.DURATION, - CalendarContract.Events.EVENT_TIMEZONE, - CalendarContract.Events.AVAILABILITY, - CalendarContract.Events.ACCESS_LEVEL, - CalendarContract.Events.EVENT_LOCATION, - CalendarContract.Events.DESCRIPTION, ) const val IDX_ID = 0 const val IDX_ORIGINAL_INSTANCE_TIME = 1 const val IDX_STATUS = 2 - const val IDX_TITLE = 3 - const val IDX_ALL_DAY = 4 - const val IDX_DTSTART = 5 - const val IDX_DTEND = 6 - const val IDX_DURATION = 7 - const val IDX_EVENT_TIMEZONE = 8 - const val IDX_AVAILABILITY = 9 - const val IDX_ACCESS_LEVEL = 10 - const val IDX_LOCATION = 11 - const val IDX_DESCRIPTION = 12 + const val IDX_DTSTART = 3 + const val IDX_DTEND = 4 + const val IDX_DURATION = 5 } internal object AttendeeProjection { diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt index 4404914e..802d89da 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt @@ -1179,47 +1179,21 @@ class EventWriteMapperTest { assertThat(values[CalendarContract.Events.DURATION]).isEqualTo("P5400S") } - // --- buildCopiedExceptionValues (calendar move: exception replay) --- - - private fun exceptionSnapshot( - isCancelled: Boolean = false, - duration: String? = null, - // Occurrence starts one hour into the day and runs 30 minutes. - dtEndMillis: Long? = 1_781_164_800_000L + 3_600_000L + 1_800_000L, - ) = ExceptionRowSnapshot( - exceptionEventId = 42L, - originalInstanceMillis = 1_781_164_800_000L, - isCancelled = isCancelled, - status = if (isCancelled) CalendarContract.Events.STATUS_CANCELED else CalendarContract.Events.STATUS_CONFIRMED, - title = "Moved occurrence", - isAllDay = false, - dtStartMillis = 1_781_164_800_000L + 3_600_000L, - dtEndMillis = dtEndMillis, - duration = duration, - timezone = "Europe/Berlin", - availability = CalendarContract.Events.AVAILABILITY_BUSY, - accessLevel = CalendarContract.Events.ACCESS_DEFAULT, - location = "", - description = "Notes", - ) - @Test - fun `copied modified occurrence carries the original instance and DURATION not DTEND`() { - val values = buildCopiedExceptionValues(exceptionSnapshot()) - assertThat(values[CalendarContract.Events.ORIGINAL_INSTANCE_TIME]) - .isEqualTo(1_781_164_800_000L) - assertThat(values[CalendarContract.Events.TITLE]).isEqualTo("Moved occurrence") - assertThat(values[CalendarContract.Events.DTSTART]).isEqualTo(1_781_164_800_000L + 3_600_000L) - // 30-minute occurrence derived from DTEND, written as DURATION only. - assertThat(values[CalendarContract.Events.DURATION]).isEqualTo("P1800S") - assertThat(values).doesNotContainKey(CalendarContract.Events.DTEND) - assertThat(values).containsEntry(CalendarContract.Events.EVENT_LOCATION, null) - } - - @Test - fun `copied modified occurrence keeps a stored duration verbatim`() { - val values = buildCopiedExceptionValues(exceptionSnapshot(duration = "P900S", dtEndMillis = null)) - assertThat(values[CalendarContract.Events.DURATION]).isEqualTo("P900S") + fun `several occurrences are excluded in one exdate write`() { + val values = buildOccurrencesExdateValues( + existingExdate = "20260601T090000Z", + occurrences = listOf(1_781_168_400_000L, 1_781_254_800_000L, 1_781_168_400_000L), + dtStartMillis = 1_780_304_400_000L, + rrule = "FREQ=DAILY", + rdate = null, + duration = "P3600S", + timezone = "Europe/Berlin", + allDay = 0, + ) + assertThat(values[CalendarContract.Events.EXDATE]) + .isEqualTo("20260601T090000Z,20260611T090000Z,20260612T090000Z") + assertThat(values[CalendarContract.Events.RRULE]).isEqualTo("FREQ=DAILY") } @Test