diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapper.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapper.kt index 10c4e25..6fe47f3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapper.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapper.kt @@ -79,6 +79,9 @@ object VTodoMapper { // SEQUENCE, so the duplicate is unconditional — and under // `Prefer: handling=strict` sabre will not quietly repair it. "SEQUENCE", "PRIORITY", "PERCENT-COMPLETE", "CLASS", + // Parameterised copies stay in the residue verbatim; authoring the + // column beside them would emit the same exclusions twice. + "RDATE", "EXDATE", ) /** What a VTODO yields. Row identity ([TaskEntity.id], `listId`) is the caller's. */ @@ -109,6 +112,31 @@ object VTodoMapper { fun take(property: ICalProperty?, value: T?): T? = value?.also { claim(property) } + /** + * Every `RDATE` / `EXDATE` copy's values, merged. + * + * ⚠️ Both are cardinality-**many**, and Apple Calendar writes one line + * per excluded occurrence. Reading only the first left the rest out of + * the column the expander works from, so a deleted occurrence + * reappeared in the list and in its reminders — while round-tripping + * back to the server perfectly out of the residue, which is what made it + * invisible. + * + * ⚠️ Claimed only when no copy carries a parameter. The column is the + * bare value, so claiming a `TZID=`- or `VALUE=DATE`-qualified property + * would drop that qualifier for good: the property never reaches the + * residue, and `write` re-emits it naked. A floating EXDATE stops + * matching the instance it excluded, and a bare eight-digit RDATE is + * read as a malformed DATE-TIME (§3.3.5) — a permanent 415 under the + * `Prefer: handling=strict` this client sends. + */ + fun dateList(vtodo: ICalComponent, name: String, claim: (ICalProperty?) -> Unit): String? { + val copies = vtodo.properties(name) + if (copies.isEmpty()) return null + if (copies.all { it.params.isEmpty() }) copies.forEach(claim) + return copies.joinToString(",") { it.value.trim() }.takeIf { it.isNotEmpty() } + } + val uidProperty = vtodo.property("UID") // Claimed even when empty: an empty UID must not reach the residue, or // write would emit the caller's minted UID alongside the empty one. @@ -211,8 +239,8 @@ object VTodoMapper { isAllDay = allDay, timezone = timezone, rrule = take(vtodo.property("RRULE"), vtodo.property("RRULE")?.value?.trim()), - rdate = take(vtodo.property("RDATE"), vtodo.property("RDATE")?.value?.trim()), - exdate = take(vtodo.property("EXDATE"), vtodo.property("EXDATE")?.value?.trim()), + rdate = dateList(vtodo, "RDATE", ::claim), + exdate = dateList(vtodo, "EXDATE", ::claim), recurrenceId = recurrenceId.instant, createdAt = created.instant, lastModified = lastModified.instant, @@ -257,7 +285,8 @@ object VTodoMapper { now: Instant = Clock.System.now(), ): ICalComponent { val residue = parseResidue(entity.unknownProperties) - val keptResidue = residue.properties.filterNot { contradictsResidue(it, entity, parentUid) } + val keptResidue = + residue.properties.filterNot { contradictsResidue(it, residue, entity, parentUid) } val suppressed = keptResidue .map { it.name.uppercase() } .filterTo(mutableSetOf()) { it in SUPPRESSED_BY_RESIDUE } @@ -292,8 +321,14 @@ object VTodoMapper { add("DURATION", entity.duration) add("RRULE", entity.rrule) - add("RDATE", entity.rdate) - add("EXDATE", entity.exdate) + // ⚠️ Authored only when the residue's copies contradicted the column — + // i.e. we changed the exclusions ourselves. The parameters come from the + // copy the residue is giving up, so an authored value keeps the TZID the + // list was written in; with no copy to take them from, a date-shaped + // value still needs its VALUE=DATE, since §3.3.5's default is DATE-TIME + // and a bare eight-digit value is malformed. + dateList("RDATE", entity.rdate, residue, suppressed, properties) + dateList("EXDATE", entity.exdate, residue, suppressed, properties) addTime("RECURRENCE-ID", entity.recurrenceId) add("CREATED", entity.createdAt?.let { ICalValues.formatDateTime(it, null) }) @@ -310,6 +345,40 @@ object VTodoMapper { ) } + private fun dateList( + name: String, + value: String?, + residue: ICalComponent, + suppressed: Set, + into: MutableList, + ) { + if (value == null || name in suppressed) return + val inherited = residue.properties(name).firstOrNull { it.params.isNotEmpty() }?.params + into += ICalProperty(name, paramsFor(value, inherited.orEmpty()), value) + } + + /** + * The parameters an authored list may carry, given what is in it. + * + * ⚠️ Checked against the value rather than copied across. §3.3.5 forbids + * `TZID` on a `…Z` value and sabre answers 415 for the pair, and a + * date-shaped value needs `VALUE=DATE` or it is read as a malformed + * DATE-TIME. A `TZID` inherited from a residue copy therefore only survives + * where the value is still a local wall time. + * + * What this cannot repair is an `EXDATE` whose zone differs from `DTSTART`'s: + * one `timezone` column describes the task, so a value appended here is + * written in *that* zone. Legal, and vanishingly rare — every client that + * writes both writes them alike. + */ + private fun paramsFor(value: String, inherited: List): List { + val values = value.split(',').map(String::trim).filter(String::isNotEmpty) + if (values.isEmpty()) return emptyList() + if (values.none { it.contains('T') }) return listOf(ICalParam("VALUE", "DATE")) + if (values.any { it.endsWith("Z") }) return emptyList() + return inherited.filter { it.name.equals("TZID", ignoreCase = true) } + } + /** * True when a suppressing residue property no longer describes what its * column holds — i.e. the user has edited that field since it was imported, @@ -317,9 +386,20 @@ object VTodoMapper { */ private fun contradictsResidue( property: ICalProperty, + residue: ICalComponent, entity: TaskEntity, parentUid: String?, ): Boolean { + // Cardinality-many and list-valued: the column carries every copy's + // values merged, so it contradicts only when that whole set has changed + // — which is what deleting an occurrence does, and nothing else here + // does. All the copies are then dropped together and one authored line + // carries the lot, parameters included. + fun listChanged(column: String?): Boolean { + fun split(text: String?) = + text.orEmpty().split(',').map(String::trim).filter(String::isNotEmpty) + return split(column) != residue.properties(property.name).flatMap { split(it.value) } + } fun sameInstant(column: Instant?) = ICalValues.readInstant(ICalValues.parseTime(property)) == column @@ -328,6 +408,8 @@ object VTodoMapper { "DUE" -> !sameInstant(entity.due) "COMPLETED" -> !sameInstant(entity.completedAt) "RECURRENCE-ID" -> !sameInstant(entity.recurrenceId) + "RDATE" -> listChanged(entity.rdate) + "EXDATE" -> listChanged(entity.exdate) "CREATED" -> !sameInstant(entity.createdAt) "LAST-MODIFIED" -> !sameInstant(entity.lastModified) // The residue only ever holds a STATUS we could not read, which left @@ -337,7 +419,15 @@ object VTodoMapper { // A null parentUid is "the parent is not in this store", not "there is // no parent" — dropping the link there would destroy a relationship // over a row we simply have not fetched. - "RELATED-TO" -> parentUid != null && property.value.trim() != parentUid + // + // ⚠️ Scoped to RELTYPE=PARENT, exactly as the read side is. Nothing + // here models CHILD or SIBLING, and no RELATED-TO is ever claimed, + // so every one of them lives in the residue — where an unscoped + // comparison read a CHILD link as "a PARENT link that disagrees with + // the column" and dropped it on the next PUT. + "RELATED-TO" -> + (property.param("RELTYPE") ?: "PARENT").equals("PARENT", ignoreCase = true) && + parentUid != null && property.value.trim() != parentUid // Each of these reaches the residue only when it could not be parsed, // which left the column at its fallback. A column that has since moved // off that fallback is the user's edit, and it wins. diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpander.kt b/app/src/main/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpander.kt index 29f65e4..9b8031b 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpander.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpander.kt @@ -159,14 +159,24 @@ object RecurrenceExpander { // anyway. Re-read the UNTIL's local fields in the series zone. val until = rule.until if (until != null && until.isFloating) { + // ⚠️ A DATE-valued UNTIL bounds the whole of its last day, not + // midnight on it. Its hour, minute and second fields are all + // zero, so rebuilding from them silently dropped every + // occurrence on the final day of a series anchored at any other + // time — five daily occurrences where there should be six, only + // away from UTC, which is to say for almost everyone. §3.3.10 + // pairs a DATE UNTIL with a DATE DTSTART; Google and Apple emit + // it beside a timed one anyway, and end-of-day is the only + // reading of that which keeps the day the user can see. + val date = until.isAllDay rule.until = DateTime( zone, until.year, until.month, until.dayOfMonth, - until.hours, - until.minutes, - until.seconds, + if (date) 23 else until.hours, + if (date) 59 else until.minutes, + if (date) 59 else until.seconds, ) } } diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapperTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapperTest.kt index dd6fd28..9f08ba0 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapperTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/VTodoMapperTest.kt @@ -127,11 +127,93 @@ class VTodoMapperTest { assertThat(mapped.entity.isAllDay).isTrue() assertThat(mapped.entity.due).isNotNull() } + @Test + fun `every EXDATE line reaches the column the expander reads`() { + // ⚠️ Cardinality-many, and Apple Calendar writes one line per + // excluded occurrence. Reading only the first left the rest out of + // expansion, so a deleted occurrence came back in the list and in + // its reminders — while round-tripping to the server perfectly out + // of the residue, which is what made it invisible. + val mapped = VTodoMapper.read( + vtodo( + "DTSTART:20260101T090000Z", + "RRULE:FREQ=DAILY", + "EXDATE:20260102T090000Z", + "EXDATE:20260103T090000Z", + ), + ) + assertThat(mapped.entity.exdate) + .isEqualTo("20260102T090000Z,20260103T090000Z") + } + + @Test + fun `a parameterised EXDATE keeps its parameters by staying in the residue`() { + // Claiming it would drop the TZID for good: the property never + // reaches the residue and write re-emits it naked, so the exclusion + // stops matching the instance it excluded. + val mapped = VTodoMapper.read( + vtodo( + "DTSTART;TZID=Europe/Berlin:20260101T090000", + "RRULE:FREQ=DAILY", + "EXDATE;TZID=Europe/Berlin:20260102T090000", + ), + ) + assertThat(mapped.entity.exdate).isEqualTo("20260102T090000") + assertThat(mapped.entity.unknownProperties) + .contains("EXDATE;TZID=Europe/Berlin:20260102T090000") + } } @Nested inner class Writing { + @Test + fun `an unchanged parameterised EXDATE is written once, from the residue`() { + val mapped = VTodoMapper.read( + vtodo( + "DTSTART;TZID=Europe/Berlin:20260101T090000", + "RRULE:FREQ=DAILY", + "EXDATE;TZID=Europe/Berlin:20260102T090000", + ), + ) + + val written = VTodoMapper.write(mapped.entity) + + val exdates = written.properties("EXDATE") + assertThat(exdates).hasSize(1) + assertThat(exdates.single().param("TZID")).isEqualTo("Europe/Berlin") + } + + @Test + fun `a changed EXDATE is authored with the parameters the residue gives up`() { + val mapped = VTodoMapper.read( + vtodo( + "DTSTART;TZID=Europe/Berlin:20260101T090000", + "RRULE:FREQ=DAILY", + "EXDATE;TZID=Europe/Berlin:20260102T090000", + ), + ) + // What deleting a second occurrence leaves behind. + val excepted = mapped.entity.copy( + exdate = "${mapped.entity.exdate},20260103T090000", + ) + + val written = VTodoMapper.write(excepted) + + val exdate = written.properties("EXDATE").single() + assertThat(exdate.value).isEqualTo("20260102T090000,20260103T090000") + assertThat(exdate.param("TZID")).isEqualTo("Europe/Berlin") + } + + @Test + fun `a date-shaped EXDATE we author carries VALUE=DATE`() { + // §3.3.5's default is DATE-TIME, so a bare eight-digit value is + // malformed — a permanent 415 under Prefer: handling=strict. + val entity = entity().copy(isAllDay = true, exdate = "20260102") + val exdate = VTodoMapper.write(entity).properties("EXDATE").single() + assertThat(exdate.param("VALUE")).isEqualTo("DATE") + } + @Test fun `SEQUENCE is preserved verbatim and never bumped`() { // It is the Organizer's revision counter (RFC 5545 section 3.8.7.4); diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpanderTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpanderTest.kt index bb98ffc..5230b82 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpanderTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/domain/recurrence/RecurrenceExpanderTest.kt @@ -154,6 +154,35 @@ class RecurrenceExpanderTest { ).inOrder() } + @Test + fun `a date-valued UNTIL covers the whole of its last day`() { + // ⚠️ A DATE-valued UNTIL is midnight, and the repair below rebuilt it + // from hours/minutes/seconds — all zero for a DATE — so a series anchored + // at 09:00 Berlin lost every occurrence on the final day. RFC 5545 + // §3.3.10 puts the bound at the end of that date. + val result = expand( + spec(rrule = "FREQ=DAILY;UNTIL=20250109", anchor = "2025-01-07T08:00:00Z"), + ) + assertThat(result).containsExactly( + "2025-01-07T08:00:00Z", + "2025-01-08T08:00:00Z", + "2025-01-09T08:00:00Z", + ).inOrder() + } + + @Test + fun `an all-day series with a date-valued UNTIL keeps its last day`() { + val result = expand( + spec( + rrule = "FREQ=DAILY;UNTIL=20250109", + anchor = "2025-01-07T00:00:00Z", + isAllDay = true, + timeZone = null, + ), + ) + assertThat(result).hasSize(3) + } + @Test fun `an unbounded rule stops at the occurrence ceiling`() { val result = expand(