makiolaj/calendula
1
0
Fork 1
Code Actions Releases 32 Activity

fix: editing a single occurrence does nothing on unsynced calendars (#234) (#245)

Browse Source 
## What was wrong

"Edit only this event" wrote a modified-occurrence exception unconditionally. That is the right shape for a synced series and the wrong one for everything else: an exception attaches to its parent through `ORIGINAL_SYNC_ID`, so on a series row with no `_sync_id` the link never forms — the insert fails or lands an orphan, the generic catch in `EventEditViewModel.performSave` turns it into a snackbar, and the scope dialog just closes again. To the reporter that read as "nothing happens, ever", with a stray copy of the event the one time the insert did land.

This is the same constraint `deleteOccurrence` has documented since #47, and the same calendars: a local calendar, Calendula's own contact special-date calendars, and — the reporter's case — a Google calendar whose rows the sync adapter has not stamped yet, which is exactly why it "shows as on-device". `deleteOccurrence` got the `_sync_id` guard in 4fea176; `updateOccurrence` never did.

## What changed

`updateOccurrence` now branches on `_sync_id` the way `deleteOccurrence` does.

- **Synced series**: the exception path, untouched. Its write shape is load-bearing and on-device verified (#16, #47).
- **No `_sync_id`**: the occurrence is excluded from the parent via EXDATE and the edited values are inserted as a standalone event on the same calendar — a detached instance, minus the `RECURRENCE-ID` the provider has no way to store here.

Two things shape the write. The parent update reuses `buildOccurrenceExdateValues` unchanged, so it keeps carrying the whole time/recurrence set — an EXDATE-only update is not a recurrence change to the provider and leaves the expanded instances standing (#47's first quirk). And the form's RRULE is stripped before the insert: the exception path gets an inherited rule cleared for free by DTSTART + DURATION, but nothing clears one here, so leaving it would insert a second *series* overlapping the first.

Ordering is chosen for the failure cases. The insert runs first, so a failure there leaves the series completely untouched — the discipline `updateEventFromOccurrence` already follows. If the EXDATE update then fails, the new row is a visible duplicate of an occurrence still in the series, so it is rolled back (best effort) before the failure surfaces. The reverse order could strand an occurrence excluded from its series with nothing standing in for it, turning an edit into a silent delete.

A second detach of the same occurrence is refused rather than silently making a second copy (reachable from a stale detail screen: the EXDATE merge folds the repeat away and the update still reports a changed row). `NoSuchEventException` from a write now maps to the same "no longer exists" state the pre-check already gives.

Smaller, in the same area: a failed save was invisible precisely because this bug was — the failure snackbar gets the long duration instead of a flash, and the catch logs the scope and event id (never the form's content) so a failure leaves something to report.

## What this costs

The detached row has no stored link back to its series — that is the whole reason the path exists — and the KDoc now says so plainly. A whole-series delete leaves it standing where an exception row would have gone with the parent; a calendar move leaves it behind; a series-wide *time* edit moves the generated instances but not the absolute-instant EXDATE hole, so the occurrence returns alongside the 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 after a series time edit — but a duplicate is a louder symptom than a resurrection. It wants fixing at the series-update end (re-stamping EXDATE alongside the DTSTART shift in `buildEventUpdateValues`, and carrying surviving stamps into the split series in `updateEventFromOccurrence`), which is a change to the "all events" path for *every* calendar type and does not belong in a targeted fix. Worth its own issue.

## How it was verified

- `./gradlew :app:testDebugUnitTest` — BUILD SUCCESSFUL, 62 suites, 0 failures.
- `./gradlew :app:lintDebug` — BUILD SUCCESSFUL, no new findings.
- `./gradlew :app:assembleDebug` — BUILD SUCCESSFUL.
- Independent adversarial code review, whose findings drove the second commit (the double-detach guard, the corrected rollback claim, and the cost documentation above). It confirmed no interleaving loses an occurrence, and cleared the drag-to-reschedule path: `RescheduleViewModel.undoFor` returns null for a recurring single-occurrence move, so the changed return value (a new event id rather than an exception id) never reaches the undo machinery.

New JVM tests cover the pure halves: the rule is dropped and the row becomes a one-off with DTEND, every edited field survives onto the inserted columns, all-day stays on UTC midnights, the detached row's DTSTART agrees with the EXDATE stamp that removes it from the parent (timed and all-day), and `exdateContains` recognises an already-excluded occurrence without matching a neighbouring one.

## What still needs a device

The provider behaviour itself cannot be confirmed on the JVM — `AndroidCalendarDataSource` has no fake-resolver harness, so `detachOccurrence`'s branch, its insert-then-EXDATE ordering and its rollback have no unit coverage. On a device, on a **local or unsynced** calendar:

1. The reported case end to end: recurring series, edit one occurrence's title, "Only this event" — the edit sticks, that occurrence alone changes, and the rest of the series survives (the #47 collapse must not reappear).
2. The same for an **all-day** yearly series (a contact birthday calendar is the natural subject) — the date-only EXDATE form excludes the right day, not the one before it.
3. A series pinned to a **non-device timezone**, and an occurrence across a **DST boundary** — the hole and the detached row must land on the same instant.
4. Editing the occurrence's **time**, not just its title, and editing the **first** occurrence of a series (DTSTART then points at an excluded instant — expected to be fine, same property the #47 delete path already has, but untested).
5. Reminders and guests on the detached row, and a colour from an account palette.
6. Regression on a **DAVx5 / Google synced** calendar: "Only this event" must still go down the exception path and behave exactly as before.
7. Drag-to-reschedule a single occurrence on an unsynced series — same path, different caller.

Closes #234

Co-authored-by: Jean-Luc Makiola <business@jeanlucmakiola.de>
Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/245
This commit is contained in:
Jean-Luc Makiola
2026-08-27 20:43:42 +02:00
parent 398cfc8906
commit cf72dce0e7
6 changed files with 327 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

103
app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt
View File

@@ -232,10 +232,15 @@ interface CalendarDataSource {
): Long
/**
* Change a single occurrence of a recurring event by inserting a
* modified-occurrence exception at [beginMillis] (the occurrence's
* `Instances.BEGIN`) carrying [form]'s values; returns the exception
* row's `Events._ID`. [allDayReminderTimeMinutes]: see [insertEvent].
* Change a single occurrence of a recurring event at [beginMillis] (the
* occurrence's `Instances.BEGIN`) to [form]'s values; returns the
* `Events._ID` of the row now holding them.
*
* A series with a `_sync_id` gets a modified-occurrence exception. One
* without gets the occurrence excluded from the parent via EXDATE plus a
* standalone event carrying the edits — an exception cannot link to its
* parent there (Codeberg #234, the same constraint as [deleteOccurrence]).
* [allDayReminderTimeMinutes]: see [insertEvent].
*/
fun updateOccurrence(
eventId: Long,
@@ -1194,6 +1199,13 @@ class AndroidCalendarDataSource @Inject constructor(
form: EventForm,
allDayReminderTimeMinutes: Int,
): Long {
val row = querySeriesRow(eventId)
// Stricter than deleteOccurrence's bare _sync_id check: EXDATE only means
// something on a row that recurs, so a non-recurring one keeps the
// exception path rather than getting a recurrence set written onto it.
if (row.syncId == null && !row.rrule.isNullOrBlank()) {
return detachOccurrence(eventId, beginMillis, row, form, allDayReminderTimeMinutes)
}
// The provider clones the series row and applies these values on top.
val values = buildOccurrenceExceptionValues(
form = form,
@@ -1212,6 +1224,89 @@ class AndroidCalendarDataSource @Inject constructor(
return exceptionId
}
/**
* "Edit only this event" on a series with **no `_sync_id`**: drop the
* occurrence from the parent with EXDATE and insert the edited values as a
* standalone event on the same calendar.
*
* A modified exception attaches to its parent only through `ORIGINAL_SYNC_ID`,
* exactly like the cancelled one [deleteOccurrence] documents; with no
* `_sync_id` the link never forms and the edit is lost (Codeberg #234).
* EXDATE plus a standalone row needs no link — what a detached instance
* degrades to without a `RECURRENCE-ID` to carry it.
*
* The detached row keeps no stored link back to its series, so: it no longer
* travels with it ([moveEvent] copies the master and its `ORIGINAL_ID`
* children, and this is neither); its EXDATE hole is an absolute instant, so
* re-timing the whole series brings the occurrence back beside the copy (a
* #47 delete resurrects the same way); and it is built from the form, not
* cloned, so `ORGANIZER`, `STATUS` and the attendee rows [reconcileAttendees]
* preserves are dropped — the same limitation as [moveEvent].
*
* Insert first, so a failure leaves the series untouched
* ([updateEventFromOccurrence]'s discipline); roll the new row back if the
* EXDATE update then fails, since it would be a visible duplicate. The
* reverse order risks the worse outcome — an excluded occurrence with no
* replacement, i.e. an edit that quietly deletes.
*/
private fun detachOccurrence(
eventId: Long,
beginMillis: Long,
row: SeriesRow,
form: EventForm,
allDayReminderTimeMinutes: Int,
): Long {
// Already detached (or deleted) from a stale screen still pointing at the
// parent: the EXDATE merge would fold the repeat away and still report a
// changed row, quietly leaving a *second* standalone copy.
if (exdateContains(row.exdate, beginMillis, isAllDay = row.allDay != 0)) {
throw NoSuchEventException(eventId)
}
// Reminders, guests and colour come along like any new event, and so does
// a fresh UID — the detached row is a separate event now, and sharing the
// parent's would collide with it in .ics restore dedup.
val detachedId = insertEvent(form.toDetachedOccurrence(), allDayReminderTimeMinutes)
val values = buildOccurrenceExdateValues(
existingExdate = row.exdate,
occurrenceMillis = beginMillis,
dtStartMillis = row.dtStartMillis,
rrule = row.rrule,
duration = row.duration,
timezone = row.timezone,
allDay = row.allDay,
)
// Rows touched, not occurrences excluded — 1 whenever the series row still
// exists. It catches the row disappearing under us, not an EXDATE the
// provider's expansion fails to match.
val updatedRows = try {
resolver.update(
ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, eventId),
values.toContentValues(), null, null,
)
} catch (t: Throwable) {
rollBackDetached(detachedId)
throw t
}
if (updatedRows == 0) {
rollBackDetached(detachedId)
throw WriteFailedException(
"exdate occurrence for edit, event id=$eventId begin=$beginMillis",
)
}
return detachedId
}
/**
* Undo the standalone row [detachOccurrence] inserted before its EXDATE
* update failed. Best effort: the caller is already throwing, and the worst
* case is the duplicate we were avoiding — never a lost occurrence.
*/
private fun rollBackDetached(detachedId: Long) {
runCatching { deleteEvent(detachedId) }.onFailure {
Log.w(TAG, "Failed to roll back detached occurrence $detachedId", it)
}
}
override fun updateEventFromOccurrence(
eventId: Long,
beginMillis: Long,

32
app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt
View File

@@ -243,6 +243,21 @@ internal fun buildOccurrenceExceptionValues(
putAll(eventColorColumns(form.colorKey, form.color))
}
/**
* The form as a **detached occurrence**: the same edited values, with the
* series rule dropped so [buildEventInsertValues] writes a standalone one-off
* row (DTSTART + DTEND, no RRULE/DURATION) at the occurrence's own times.
*
* The "edit only this event" shape for a series with **no `_sync_id`**, where an
* exception row can't attach to its parent at all (Codeberg #234).
*
* The exception path gets the rule dropped for free — the provider clears the
* RRULE it cloned when an exception carries DTSTART + DURATION
* ([buildOccurrenceExceptionValues]). Here nothing is cloned, so it is stripped
* by hand; leaving it on would insert a second *series* overlapping the first.
*/
internal fun EventForm.toDetachedOccurrence(): EventForm = copy(rrule = null)
/**
* Raw provider snapshot of a master/one-off Events row, enough to re-insert it
* verbatim on another calendar (a calendar move is copy+delete — `CALENDAR_ID`
@@ -435,6 +450,23 @@ internal fun buildOccurrenceExdateValues(
)
}
/**
* Whether [existingExdate] already excludes the occurrence at [occurrenceMillis]
* — i.e. it has already been dropped from the series, deleted or detached.
*
* Guards the detach path against running twice from a stale screen: the EXDATE
* merge folds the repeat away silently and the update still reports one row
* changed, so a second save would leave a second standalone copy.
*/
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

8
app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditScreen.kt
View File

@@ -64,6 +64,7 @@ import androidx.compose.material3.Scaffold
import androidx.compose.material3.SegmentedButton
import androidx.compose.material3.SegmentedButtonDefaults
import androidx.compose.material3.SingleChoiceSegmentedButtonRow
import androidx.compose.material3.SnackbarDuration
import androidx.compose.material3.SnackbarHost
import androidx.compose.material3.SnackbarHostState
import androidx.compose.material3.Surface
@@ -264,13 +265,16 @@ fun EventEditScreen(
viewModel.reset()
onSaved()
}
// A failed save leaves the form looking unchanged, so the snackbar is
// the only sign anything happened — long rather than the default
// flash (Codeberg #234: it read as "nothing happens at all").
SaveUiState.Failed -> {
viewModel.consumeSaveResult()
snackbarHostState.showSnackbar(saveFailedMessage)
snackbarHostState.showSnackbar(saveFailedMessage, duration = SnackbarDuration.Long)
}
SaveUiState.NeedsPermission -> {
viewModel.consumeSaveResult()
snackbarHostState.showSnackbar(writeDeniedMessage)
snackbarHostState.showSnackbar(writeDeniedMessage, duration = SnackbarDuration.Long)
}
// AwaitingScope/AwaitingConflict/Gone render as dialogs below.
else -> Unit

11
app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt
View File

@@ -1,5 +1,6 @@
package de.jeanlucmakiola.calendula.ui.edit
import android.util.Log
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import dagger.hilt.android.lifecycle.HiltViewModel
@@ -56,6 +57,8 @@ import kotlin.time.Duration.Companion.minutes
import kotlin.time.Instant
import javax.inject.Inject
private const val TAG = "EventEdit"
/**
* Where a prefilled [EventEditViewModel.openImported] form came from. The sources
* want different reminder handling (#49), and differ in whether they own the
@@ -751,7 +754,15 @@ class EventEditViewModel @Inject constructor(
throw e
} catch (e: SecurityException) {
SaveUiState.NeedsPermission
} catch (e: NoSuchEventException) {
// The event or occurrence is already gone: the same answer the
// pre-check gives, and better than a bare "couldn't save".
SaveUiState.Gone
} catch (e: Exception) {
// The user only gets a generic snackbar, so without this a failed
// write leaves nothing to report (Codeberg #234). Scope and event
// id only — never the form's content.
Log.w(TAG, "Save failed (scope=$scope, eventId=${target?.eventId})", e)
SaveUiState.Failed
}
}

168
app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt
View File

@@ -558,6 +558,174 @@ class EventWriteMapperTest {
assertThat(values[CalendarContract.Events.ALL_DAY]).isEqualTo(1)
}
// --- toDetachedOccurrence ("edit only this event", no _sync_id) ---
@Test
fun `a detached occurrence drops the series rule and becomes a one-off row`() {
val edited = form().copy(title = "Moved", rrule = "FREQ=WEEKLY;BYDAY=TH")
val detached = edited.toDetachedOccurrence()
val values = buildEventInsertValues(
form = detached,
uid = "uid@calendula",
times = detached.toWriteTimes(berlin),
)
assertThat(values[CalendarContract.Events.TITLE]).isEqualTo("Moved")
// A surviving rule would insert a second *series* overlapping the first
// (Codeberg #234's stray duplicate).
assertThat(values).doesNotContainKey(CalendarContract.Events.RRULE)
assertThat(values).doesNotContainKey(CalendarContract.Events.DURATION)
// A one-off row carries DTEND rather than a duration.
assertThat(values[CalendarContract.Events.DTSTART]).isEqualTo(1_781_164_800_000L)
assertThat(values[CalendarContract.Events.DTEND]).isEqualTo(1_781_170_200_000L)
assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("Europe/Berlin")
}
@Test
fun `a detached occurrence carries every edited field onto the new row`() {
// Built from the form, not cloned from the parent: a field dropped here
// is an edit silently lost.
val edited = form(timezone = "America/New_York").copy(
title = " Standup ",
location = "Room 2",
description = "notes",
reminders = listOf(10),
availability = Availability.Free,
accessLevel = AccessLevel.Private,
rrule = "FREQ=DAILY",
)
val detached = edited.toDetachedOccurrence()
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 — never 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 aren't columns; the insert path seeds them from the form.
assertThat(detached.reminders).containsExactly(10)
}
@Test
fun `a detached all-day occurrence stays on UTC midnights`() {
val edited = form(
isAllDay = true,
start = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)),
end = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)),
).copy(title = "Birthday", rrule = "FREQ=YEARLY")
val detached = edited.toDetachedOccurrence()
val values = buildEventInsertValues(
form = detached,
uid = "uid@calendula",
times = detached.toWriteTimes(berlin),
)
assertThat(values[CalendarContract.Events.ALL_DAY]).isEqualTo(1)
assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("UTC")
assertThat(values[CalendarContract.Events.DTSTART]).isEqualTo(1_781_136_000_000L)
// Exclusive DTEND — the next UTC midnight.
assertThat(values[CalendarContract.Events.DTEND]).isEqualTo(1_781_222_400_000L)
assertThat(values).doesNotContainKey(CalendarContract.Events.RRULE)
}
@Test
fun `the detached row lands exactly where the parent's exdate removes it`() {
// The two halves must agree on the instant, or the user sees the
// occurrence twice or not at all.
val edited = form().copy(title = "Renamed", rrule = "FREQ=WEEKLY")
val occurrenceMillis = 1_781_164_800_000L
val parent = buildOccurrenceExdateValues(
existingExdate = null,
occurrenceMillis = occurrenceMillis,
dtStartMillis = 1_780_560_000_000L,
rrule = "FREQ=WEEKLY",
duration = "P5400S",
timezone = "Europe/Berlin",
allDay = 0,
)
val detached = edited.toDetachedOccurrence()
val inserted = buildEventInsertValues(
form = detached,
uid = "uid@calendula",
times = detached.toWriteTimes(berlin),
)
assertThat(parent[CalendarContract.Events.EXDATE]).isEqualTo("20260611T080000Z")
assertThat(inserted[CalendarContract.Events.DTSTART]).isEqualTo(occurrenceMillis)
// The parent keeps its own anchor and rule — only this occurrence leaves.
assertThat(parent[CalendarContract.Events.DTSTART]).isEqualTo(1_780_560_000_000L)
assertThat(parent[CalendarContract.Events.RRULE]).isEqualTo("FREQ=WEEKLY")
}
@Test
fun `a detached all-day occurrence matches its date-only exdate stamp`() {
val parent = buildOccurrenceExdateValues(
existingExdate = null,
occurrenceMillis = 1_781_136_000_000L, // 2026-06-11T00:00:00Z
dtStartMillis = 1_749_600_000_000L,
rrule = "FREQ=YEARLY",
duration = "P1D",
timezone = "UTC",
allDay = 1,
)
val detached = form(
isAllDay = true,
start = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)),
end = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)),
).copy(rrule = "FREQ=YEARLY").toDetachedOccurrence()
val inserted = buildEventInsertValues(
form = detached,
uid = "uid@calendula",
times = detached.toWriteTimes(berlin),
)
assertThat(parent[CalendarContract.Events.EXDATE]).isEqualTo("20260611")
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 leave a second standalone copy.
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.
assertThat(
exdateContains(
"20260604T080000Z, 20260611T080000Z,20260618T080000Z",
1_781_164_800_000L,
isAllDay = false,
),
).isTrue()
}
// --- per-event colour ---
@Test