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 3792608..4804f14 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 @@ -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", 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 483c08e..484a0d9 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 @@ -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 diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt index f184150..5944402 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt @@ -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). 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 5f8865f..b6813b4 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 @@ -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