fix: guard the detach path against a second copy, and say what it costs (#234)
Review pass on the detach path. Three changes, none to the write shape itself. A second detach of the same occurrence is now refused. Reached from a stale screen still pointing at the parent, it used to succeed twice over: the EXDATE merge folds the repeated stamp away and the parent update still reports one row changed, so the save looked fine and left a *second* standalone copy of one occurrence, this time with neither copy in the series to make it obvious. The occurrence is already gone from the series at that point, so the write reports it as gone — and EventEditViewModel now maps NoSuchEventException from the write to the same "this event no longer exists" answer its pre-check already gives, instead of a bare "couldn't save" for something that isn't there. The rollback comment claimed more than the code delivers. ContentResolver.update returns rows touched, not occurrences excluded, so the zero check catches the series row vanishing mid-write and nothing else — an EXDATE the provider's expansion fails to match reports success and leaves the duplicate standing. Renamed the variable to match, and the catch now mirrors moveEvent's Throwable idiom rather than inventing a narrower one for the same insert-then-roll-back shape. The rest is honesty about what a detached row can't do, since none of it is recoverable later and the KDoc previously mentioned only the calendar-move case: a whole-series delete leaves it standing where an exception row would have gone with the parent; a series-wide *time* edit moves the generated instances but not the absolute-instant EXDATE hole, so the occurrence returns alongside its copy; and building the row from the form rather than cloning the parent drops ORGANIZER, STATUS and the organizer/resource attendee rows, exactly as moveEvent does. The EXDATE staleness is not new — a #47 delete resurrects the same way — but a duplicate is a louder symptom than a resurrection, and it wants fixing at the series-update end, not here. Also notes why the branch predicate is stricter than deleteOccurrence's: EXDATE only means something on a row that recurs, so a row without an RRULE keeps the old path rather than having a recurrence set written onto a one-off event. Tests: the tautological "detached == copy(rrule = null)" assertion is replaced by one that pins every edited field through to the inserted columns, and exdateContains gets its own cases (timed, all-day, absent, a neighbouring occurrence, and a multi-entry list with whitespace). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1200,6 +1200,12 @@ class AndroidCalendarDataSource @Inject constructor(
|
||||
allDayReminderTimeMinutes: Int,
|
||||
): Long {
|
||||
val row = querySeriesRow(eventId)
|
||||
// Deliberately stricter than deleteOccurrence's bare _sync_id check: the
|
||||
// detach path drops the occurrence with EXDATE, which only means anything
|
||||
// on a row that actually recurs. Without an RRULE there is nothing to
|
||||
// exclude from, so such a row keeps the existing path rather than getting
|
||||
// a recurrence set written onto a one-off event. The UI only offers the
|
||||
// scope choice for a recurring event, so neither case is reachable today.
|
||||
if (row.syncId == null && !row.rrule.isNullOrBlank()) {
|
||||
return detachOccurrence(eventId, beginMillis, row, form, allDayReminderTimeMinutes)
|
||||
}
|
||||
@@ -1239,9 +1245,25 @@ class AndroidCalendarDataSource @Inject constructor(
|
||||
* EXDATE + a standalone row needs no parent link, and is what a detached
|
||||
* instance degrades to when there is no `RECURRENCE-ID` to carry it: the user
|
||||
* sees one edited event where the occurrence was, and the rest of the series
|
||||
* untouched. It does mean the edited occurrence stops travelling with the
|
||||
* series — moving the series later won't move it — which is the honest cost of
|
||||
* a link the provider cannot store here.
|
||||
* untouched.
|
||||
*
|
||||
* What that costs, plainly, because none of it is recoverable later — the
|
||||
* detached row has *no* stored link back to its series, which is the whole
|
||||
* reason this path exists:
|
||||
* - It stops travelling with the series. Moving the series to another
|
||||
* calendar leaves it behind ([moveEvent] copies the master and its
|
||||
* `ORIGINAL_ID` children; this is neither), and deleting the whole series
|
||||
* leaves it standing where an exception row would have gone with the parent.
|
||||
* - Its EXDATE hole is an absolute instant. Editing the series' *time* for all
|
||||
* events moves the generated instances but not the hole, so the occurrence
|
||||
* comes back alongside the detached copy. That staleness predates this path
|
||||
* — a #47 delete resurrects the same way — but a duplicate is a louder
|
||||
* symptom than a resurrection, and it wants fixing at the series-update end.
|
||||
* - It is built from the form, not cloned from the parent, so columns the form
|
||||
* doesn't model are dropped rather than inherited: `ORGANIZER`, `STATUS`,
|
||||
* and the organizer/resource attendee rows [reconcileAttendees] otherwise
|
||||
* preserves. Same limitation as [moveEvent]. Rare on the calendars that
|
||||
* reach this path, which are locally authored, but not impossible.
|
||||
*
|
||||
* Order is deliberate. The insert goes first, so a failure there leaves the
|
||||
* series completely untouched (the same discipline as
|
||||
@@ -1259,6 +1281,14 @@ class AndroidCalendarDataSource @Inject constructor(
|
||||
form: EventForm,
|
||||
allDayReminderTimeMinutes: Int,
|
||||
): Long {
|
||||
// Already detached (or deleted): the series no longer contains this
|
||||
// occurrence, so there is nothing here to edit. Reached from a stale
|
||||
// screen still pointing at the parent — without this the EXDATE merge
|
||||
// would fold the repeat away, the update would still report a changed
|
||||
// row, and the save would quietly leave a *second* standalone copy.
|
||||
if (exdateContains(row.exdate, beginMillis, isAllDay = row.allDay != 0)) {
|
||||
throw NoSuchEventException(eventId)
|
||||
}
|
||||
// Carries the form's reminders, guests and colour like any new event —
|
||||
// and a fresh UID, because the detached row really is a separate event
|
||||
// now: sharing the parent's would collide with it in .ics restore dedup.
|
||||
@@ -1272,16 +1302,20 @@ class AndroidCalendarDataSource @Inject constructor(
|
||||
timezone = row.timezone,
|
||||
allDay = row.allDay,
|
||||
)
|
||||
val excluded = try {
|
||||
// Rows touched, not occurrences excluded: this is 1 whenever the series
|
||||
// row still exists. It catches the row disappearing under us, not an
|
||||
// EXDATE the provider's expansion fails to match — that would report
|
||||
// success and leave the duplicate. Same rollback idiom as moveEvent.
|
||||
val updatedRows = try {
|
||||
resolver.update(
|
||||
ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, eventId),
|
||||
values.toContentValues(), null, null,
|
||||
)
|
||||
} catch (e: RuntimeException) {
|
||||
} catch (t: Throwable) {
|
||||
rollBackDetached(detachedId)
|
||||
throw e
|
||||
throw t
|
||||
}
|
||||
if (excluded == 0) {
|
||||
if (updatedRows == 0) {
|
||||
rollBackDetached(detachedId)
|
||||
throw WriteFailedException(
|
||||
"exdate occurrence for edit, event id=$eventId begin=$beginMillis",
|
||||
|
||||
@@ -455,6 +455,26 @@ internal fun buildOccurrenceExdateValues(
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether [existingExdate] already excludes the occurrence at [occurrenceMillis]
|
||||
* — i.e. this occurrence has already been dropped from the series (deleted, or
|
||||
* detached into its own event).
|
||||
*
|
||||
* Guards the detach path against running twice for the same occurrence, which a
|
||||
* stale detail screen still pointing at the parent can otherwise reach. The
|
||||
* EXDATE merge folds the repeat away silently and the parent update still
|
||||
* reports one row changed, so without this check the second save would leave a
|
||||
* *second* standalone copy and call it a success.
|
||||
*/
|
||||
internal fun exdateContains(
|
||||
existingExdate: String?,
|
||||
occurrenceMillis: Long,
|
||||
isAllDay: Boolean,
|
||||
): Boolean {
|
||||
val stamp = formatExdateStamp(occurrenceMillis, isAllDay)
|
||||
return existingExdate?.split(',')?.any { it.trim() == stamp } == true
|
||||
}
|
||||
|
||||
/**
|
||||
* One EXDATE entry for the occurrence starting at [occurrenceMillis]. Both forms
|
||||
* are UTC: the provider stores an all-day DTSTART at UTC midnight, so its date
|
||||
|
||||
@@ -754,6 +754,11 @@ class EventEditViewModel @Inject constructor(
|
||||
throw e
|
||||
} catch (e: SecurityException) {
|
||||
SaveUiState.NeedsPermission
|
||||
} catch (e: NoSuchEventException) {
|
||||
// The write found the event (or the occurrence) already gone —
|
||||
// the same answer the pre-check gives, and a far better one than
|
||||
// a bare "couldn't save" for something that no longer exists.
|
||||
SaveUiState.Gone
|
||||
} catch (e: Exception) {
|
||||
// The user only gets a generic snackbar, so without this a
|
||||
// failed write leaves no trace at all to report (Codeberg #234).
|
||||
|
||||
@@ -585,23 +585,39 @@ class EventWriteMapperTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a detached occurrence keeps everything but the rule`() {
|
||||
fun `a detached occurrence carries every edited field onto the new row`() {
|
||||
// The detached row is built from the form rather than cloned from the
|
||||
// parent, so anything the user edited has to survive the trip — a field
|
||||
// dropped here is an edit silently lost.
|
||||
val edited = form(timezone = "America/New_York").copy(
|
||||
title = "Standup",
|
||||
title = " Standup ",
|
||||
location = "Room 2",
|
||||
description = "notes",
|
||||
reminders = listOf(10),
|
||||
availability = Availability.Free,
|
||||
accessLevel = AccessLevel.Private,
|
||||
colorKey = "7",
|
||||
rrule = "FREQ=DAILY",
|
||||
)
|
||||
val detached = edited.toDetachedOccurrence()
|
||||
|
||||
assertThat(detached.rrule).isNull()
|
||||
assertThat(detached).isEqualTo(edited.copy(rrule = null))
|
||||
// Reminders and guests ride along on the form: the insert path seeds them
|
||||
// from it, so the detached row keeps the reminders the user just edited.
|
||||
val values = buildEventInsertValues(
|
||||
form = detached,
|
||||
uid = "uid@calendula",
|
||||
times = detached.toWriteTimes(berlin),
|
||||
)
|
||||
assertThat(values[CalendarContract.Events.TITLE]).isEqualTo("Standup")
|
||||
assertThat(values[CalendarContract.Events.EVENT_LOCATION]).isEqualTo("Room 2")
|
||||
assertThat(values[CalendarContract.Events.DESCRIPTION]).isEqualTo("notes")
|
||||
assertThat(values[CalendarContract.Events.AVAILABILITY])
|
||||
.isEqualTo(CalendarContract.Events.AVAILABILITY_FREE)
|
||||
assertThat(values[CalendarContract.Events.ACCESS_LEVEL])
|
||||
.isEqualTo(CalendarContract.Events.ACCESS_PRIVATE)
|
||||
// The pinned zone survives too — a detached occurrence must not be
|
||||
// silently re-anchored to the device.
|
||||
assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("America/New_York")
|
||||
assertThat(values[CalendarContract.Events.UID_2445]).isEqualTo("uid@calendula")
|
||||
// Reminders and guests aren't columns — the insert path seeds them from
|
||||
// the form, so they only have to survive on the form itself.
|
||||
assertThat(detached.reminders).containsExactly(10)
|
||||
}
|
||||
|
||||
@@ -682,6 +698,43 @@ class EventWriteMapperTest {
|
||||
assertThat(inserted[CalendarContract.Events.DTSTART]).isEqualTo(1_781_136_000_000L)
|
||||
}
|
||||
|
||||
// --- exdateContains (guards a second detach of the same occurrence) ---
|
||||
|
||||
@Test
|
||||
fun `an occurrence already excluded is recognised, timed and all-day`() {
|
||||
// Detaching twice would fold the repeated EXDATE away and still report a
|
||||
// changed row, leaving a second standalone copy of one occurrence.
|
||||
assertThat(
|
||||
exdateContains("20260611T080000Z", 1_781_164_800_000L, isAllDay = false),
|
||||
).isTrue()
|
||||
assertThat(
|
||||
exdateContains("20260611", 1_781_136_000_000L, isAllDay = true),
|
||||
).isTrue()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an occurrence not in the exdate list is not mistaken for an excluded one`() {
|
||||
assertThat(exdateContains(null, 1_781_164_800_000L, isAllDay = false)).isFalse()
|
||||
assertThat(exdateContains("", 1_781_164_800_000L, isAllDay = false)).isFalse()
|
||||
// A neighbouring occurrence must not match — the guard is per-instant.
|
||||
assertThat(
|
||||
exdateContains("20260610T080000Z", 1_781_164_800_000L, isAllDay = false),
|
||||
).isFalse()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an exclusion is found anywhere in a multi-entry exdate list`() {
|
||||
// Whitespace after a comma is legal in the stored column and must not
|
||||
// hide an exclusion — that would let a duplicate through.
|
||||
assertThat(
|
||||
exdateContains(
|
||||
"20260604T080000Z, 20260611T080000Z,20260618T080000Z",
|
||||
1_781_164_800_000L,
|
||||
isAllDay = false,
|
||||
),
|
||||
).isTrue()
|
||||
}
|
||||
|
||||
// --- per-event colour ---
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user