Round legacy series once without dirtying them, never close a series on a missed occurrence (#135)

Also delete overrides through a dedicated seam call, look up an occurrence once
per edit, fetch rules only for recurring rows, and match split points to the
second.
This commit is contained in:
makiolaj committed 2026-11-06 16:53:28 +01:00
1 parent 49bd4e5707
commit a139fde3b9
7 files changed
+150 -83

No files matched your search

@@ -10,6 +10,7 @@ import de.jeanlucmakiola.agendula.domain.Task
import de.jeanlucmakiola.agendula.domain.TaskForm
import de.jeanlucmakiola.agendula.domain.TaskReminder
import org.junit.After
import org.junit.Assert.assertThrows
import org.junit.Assume.assumeTrue
import org.junit.Before
import org.junit.Test
@@ -103,19 +104,7 @@ class ExternalProviderRecurrenceTest {
@Test
fun aSeriesStoredWithMillisecondsStillMovesOn() {
// As builds before whole-second writes left it.
val start = Clock.System.now() + 1.hours
val id = context.contentResolver.insert(
TasksContract.tasksUri(resolver.resolve()!!.authority),
ContentValues().apply {
put(Tasks.LIST_ID, listId)
put(Tasks.TITLE, "Old daily")
put(Tasks.DTSTART, start.toEpochMilliseconds() / 1000 * 1000 + 345)
put(Tasks.DUE, (start + 1.hours).toEpochMilliseconds() / 1000 * 1000 + 345)
put(Tasks.TZ, "UTC")
put(Tasks.RRULE, "FREQ=DAILY")
},
)!!.lastPathSegment!!.toLong()
val id = legacySeries()
val task = source.task(id)!!
source.setCompletedInstance(id, task.occurrenceStart!!, completed = true)
@@ -123,6 +112,57 @@ class ExternalProviderRecurrenceTest {
assertSeriesMovedOn(id, task.occurrenceStart)
}
@Test
fun anUnknownOccurrenceDoesNotCloseTheSeries() {
val id = dailyTask()
val anchor = source.task(id)!!.occurrenceStart!!
assertThrows(TaskWriteFailedException::class.java) {
source.setCompletedInstance(id, anchor + 30.hours, completed = true)
}
assertThat(source.task(id)!!.isClosed).isFalse()
}
@Test
fun roundingALegacySeriesLeavesItUnmodified() {
val id = legacySeries()
val before = storedColumn(id, Tasks.LAST_MODIFIED)
source.task(id)
assertThat(storedStart(id)!! % 1000).isEqualTo(0L)
assertThat(storedColumn(id, Tasks.LAST_MODIFIED)).isEqualTo(before)
}
/** A daily series as builds before whole-second writes left it, already in sync with its server. */
private fun legacySeries(): Long {
val start = (Clock.System.now() + 1.hours).toEpochMilliseconds() / 1000 * 1000
return insertAsSyncAdapter(
Tasks.LIST_ID to listId,
Tasks.TITLE to "Old daily",
Tasks.DTSTART to start + 345,
Tasks.DUE to start + 3_600_345,
Tasks.TZ to "UTC",
Tasks.RRULE to "FREQ=DAILY",
).also {
// The app's own reads may already have run the one-time rounding.
context.getSharedPreferences("provider_migrations", Context.MODE_PRIVATE).edit().clear().commit()
}
}
private fun insertAsSyncAdapter(vararg values: Pair<String, Any>): Long {
val uri = TasksContract.asSyncAdapter(
TasksContract.tasksUri(resolver.resolve()!!.authority),
TasksContract.LOCAL_ACCOUNT_NAME,
TasksContract.LOCAL_ACCOUNT_TYPE,
)
val cv = ContentValues().apply {
values.forEach { (k, v) -> if (v is Long) put(k, v) else put(k, v.toString()) }
}
return context.contentResolver.insert(uri, cv)!!.lastPathSegment!!.toLong()
}
/** Edits the series' first occurrence on its own and returns the override that makes. */
private fun overrideOf(id: Long): Task {
val task = source.task(id)!!
@@ -171,7 +211,7 @@ class ExternalProviderRecurrenceTest {
val id = dailyTask()
val override = overrideOf(id)
source.deleteTask(override.taskId)
source.deleteOverride(override.taskId, override.occurrenceStart!!)
// Excluded on the master, not just the override dropped.
assertThat(storedColumn(id, Tasks.EXDATE)).isNotNull()
@@ -116,17 +116,21 @@ class AndroidTasksDataSource @Inject constructor(
* versions (tasks.org's has no `is_recurring`), the by-name mapper nulls what
* is missing, and the repository re-sorts.
*/
private fun queryInstances(selection: String?, args: Array<String>?): List<Task> =
instanceTasks(resolver.query(TasksContract.instancesUri(authority()), null, selection, args, null))
private fun queryInstances(selection: String?, args: Array<String>?): List<Task> {
roundLegacyAnchors()
return instanceTasks(resolver.query(TasksContract.instancesUri(authority()), null, selection, args, null))
}
/** Maps [cursor]'s instance rows, with the rule each series keeps on its `tasks` row, and their tags. */
private fun instanceTasks(cursor: Cursor?): List<Task> {
var flagged = false
val rows = cursor?.use { c ->
flagged = c.getColumnIndex(Instances.IS_RECURRING) >= 0
val reader = CursorColumnReader(c)
buildList { while (c.moveToNext()) add(TaskMapper.instance(reader)) }
}
if (rows.isNullOrEmpty()) return emptyList()
val rules = seriesRules(rows.map { it.ruleOwner })
val rules = seriesRules(rows.filter { !flagged || it.task.isRecurring }.map { it.ruleOwner })
val tasks = rows.map { TaskMapper.withRule(it, rules[it.ruleOwner]) }
val tags = categories(tasks.map { it.taskId })
return tasks.map { task -> tags[task.taskId]?.let { task.copy(tags = it) } ?: task }
@@ -236,7 +240,7 @@ class AndroidTasksDataSource @Inject constructor(
}
override fun updateSeries(seriesId: Long, occurrenceStart: Instant, form: TaskForm, reminders: List<TaskReminder>?) {
val occurrence = task(seriesId, occurrenceStart)
val occurrence = occurrence(seriesId, occurrenceStart) { TaskMapper.instance(it).task } ?: task(seriesId)
val anchors = resolver.query(
taskUri(authority(), seriesId), arrayOf(Tasks.DTSTART, Tasks.DUE), null, null, null,
)?.use { c ->
@@ -334,7 +338,7 @@ class AndroidTasksDataSource @Inject constructor(
buildList {
while (c.moveToNext()) {
val original = r.getLong(Tasks.ORIGINAL_INSTANCE_TIME) ?: continue
if (original >= from.toEpochMilliseconds()) r.getLong(Tasks.ID)?.let(::add)
if (providerMillis(original) >= providerMillis(from.toEpochMilliseconds())) r.getLong(Tasks.ID)?.let(::add)
}
}
} ?: emptyList()
@@ -346,8 +350,10 @@ class AndroidTasksDataSource @Inject constructor(
}
override fun updateInstance(taskId: Long, occurrenceStart: Instant, form: TaskForm, reminders: List<TaskReminder>?) {
overrideFor(taskId, occurrenceStart)?.let { return updateOverride(it, form, reminders) }
if (reminders == null) return writeInstance(taskId, occurrenceStart, form)
val hit = instanceFor(taskId, occurrenceStart)
?: throw TaskWriteFailedException("update instance $taskId@$occurrenceStart: no such occurrence")
hit.overrideId?.let { return updateOverride(it, form, reminders) }
if (reminders == null) return writeInstance(taskId, hit.instanceId, form)
// ⚠️ Written to the series, and then put back. Forking an occurrence
// copies the task's *current* properties onto the new override row, so
// setting the alarm beforehand is the only way to carry it across — and
@@ -356,7 +362,7 @@ class AndroidTasksDataSource @Inject constructor(
val seriesReminders = alarmRows(taskId).map { it.second }
writeReminders(taskId, reminders)
try {
writeInstance(taskId, occurrenceStart, form)
writeInstance(taskId, hit.instanceId, form)
} finally {
setReminders(taskId, seriesReminders)
}
@@ -375,15 +381,7 @@ class AndroidTasksDataSource @Inject constructor(
writeReminders(overrideId, reminders)
}
/** The override row standing for [taskId]'s occurrence at [occurrenceStart], if it has one. */
private fun overrideFor(taskId: Long, occurrenceStart: Instant): Long? =
occurrence(taskId, occurrenceStart) { r ->
r.getLong(Instances.TASK_ID)?.takeIf { r.getLong(Tasks.ORIGINAL_INSTANCE_ID) != null }
}
private fun writeInstance(taskId: Long, occurrenceStart: Instant, form: TaskForm) {
val instanceId = instanceIdFor(taskId, occurrenceStart)
?: throw TaskWriteFailedException("update instance $taskId@$occurrenceStart: no such occurrence")
private fun writeInstance(taskId: Long, instanceId: Long, form: TaskForm) {
val values = TaskWriteMapper.instanceValues(form, ZoneId.systemDefault().id)
val uri = TasksContract.instanceUri(authority(), instanceId)
// The fork copies the series' properties onto the override, so the tags
@@ -410,18 +408,27 @@ class AndroidTasksDataSource @Inject constructor(
}
}
/** An occurrence's instance row, and the override row behind it if it has one. */
private class InstanceHit(val instanceId: Long, val overrideId: Long?)
/**
* The provider's instance row id for one occurrence.
* The provider's instance row for one occurrence.
*
* The seam addresses occurrences by `(taskId, occurrenceStart)`; writing
* through the instances URI still needs the row id, so it is looked up here
* rather than carried around above the data layer.
*/
private fun instanceIdFor(taskId: Long, occurrenceStart: Instant): Long? {
private fun instanceFor(taskId: Long, occurrenceStart: Instant): InstanceHit? {
roundLegacyAnchors()
return occurrence(taskId, occurrenceStart) { it.getLong(Tasks.ID) }
return occurrence(taskId, occurrenceStart) { r ->
r.getLong(Tasks.ID)?.let { id ->
InstanceHit(id, r.getLong(Instances.TASK_ID)?.takeIf { r.getLong(Tasks.ORIGINAL_INSTANCE_ID) != null })
}
}
}
private fun instanceIdFor(taskId: Long, occurrenceStart: Instant): Long? = instanceFor(taskId, occurrenceStart)?.instanceId
/**
* [read] applied to the instance row of [taskId]'s occurrence at
* [occurrenceStart] — its own, or an override's that replaced it — matched in
@@ -457,18 +464,25 @@ class AndroidTasksDataSource @Inject constructor(
private var anchorsRoundedFor: String? = null
/**
* Rounds the series a build before [providerMillis] stored with millisecond
* DTSTART/DUE, once per provider and process. The provider cannot move such a
* series past an occurrence completed through its instance.
* Rounds the DTSTART/DUE a build before [providerMillis] stored with
* milliseconds, which the provider cannot move past a completed occurrence.
* Once per provider, before the first read; written as the rows' sync adapter,
* since the server only ever had whole seconds, so nothing is dirtied or
* re-uploaded. Overrides need nothing: the provider forks them at its own
* whole-second instance times.
*/
private fun roundLegacyAnchors() {
val authority = authority()
if (anchorsRoundedFor == authority) return
anchorsRoundedFor = authority
val prefs = context.getSharedPreferences(MIGRATIONS, Context.MODE_PRIVATE)
val key = "anchors_rounded:$authority"
if (prefs.getBoolean(key, false)) return
val columns = listOf(Tasks.DTSTART, Tasks.DUE)
val stale = resolver.query(
TasksContract.tasksUri(authority),
arrayOf(Tasks.ID, Tasks.DTSTART, Tasks.DUE),
"(${Tasks.RRULE} IS NOT NULL OR ${Tasks.RDATE} IS NOT NULL) AND " +
"(${Tasks.DTSTART} % 1000 != 0 OR ${Tasks.DUE} % 1000 != 0)",
arrayOf(Tasks.ID, Tasks.ACCOUNT_NAME, Tasks.ACCOUNT_TYPE) + columns,
columns.joinToString(" OR ") { "$it % 1000 != 0" },
null,
null,
)?.use { c ->
@@ -477,17 +491,22 @@ class AndroidTasksDataSource @Inject constructor(
while (c.moveToNext()) {
val id = r.getLong(Tasks.ID) ?: continue
val values = buildMap<String, Any?> {
for (column in listOf(Tasks.DTSTART, Tasks.DUE)) {
for (column in columns) {
val millis = r.getLong(column) ?: continue
if (providerMillis(millis) != millis) put(column, providerMillis(millis))
}
}
add(id to values)
val uri = TasksContract.asSyncAdapter(
taskUri(authority, id),
r.getString(Tasks.ACCOUNT_NAME).orEmpty(),
r.getString(Tasks.ACCOUNT_TYPE).orEmpty(),
)
add(uri to values)
}
}
}.orEmpty()
stale.forEach { (id, values) -> resolver.update(taskUri(authority, id), values.toContentValues(), null, null) }
anchorsRoundedFor = authority
stale.forEach { (uri, values) -> resolver.update(uri, values.toContentValues(), null, null) }
prefs.edit().putBoolean(key, true).apply()
}
override fun setReminders(taskId: Long, reminders: List<TaskReminder>) {
@@ -599,7 +618,7 @@ class AndroidTasksDataSource @Inject constructor(
/** Through the instances URI, like [setCompletedInstance]. */
override fun setCancelledInstance(taskId: Long, occurrenceStart: Instant, cancelled: Boolean) {
val instanceId = instanceIdFor(taskId, occurrenceStart)
?: return setCancelled(taskId, cancelled)
?: return plainUnlessSeries(taskId, occurrenceStart) { setCancelled(taskId, cancelled) }
val uri = TasksContract.instanceUri(authority(), instanceId)
val rows = resolver.update(uri, cancelValues(cancelled).toContentValues(), null, null)
if (rows == 0) throw TaskWriteFailedException("cancel instance $instanceId")
@@ -618,12 +637,12 @@ class AndroidTasksDataSource @Inject constructor(
/**
* Through the instances URI, which is what makes the provider fork an override
* rather than close the series. No instance row for the anchor means the task
* is not a series after all — the plain write is then the right one.
* rather than close the series. No instance row for the anchor on a task that
* is not a series means the plain write is the right one.
*/
override fun setCompletedInstance(taskId: Long, occurrenceStart: Instant, completed: Boolean) {
val instanceId = instanceIdFor(taskId, occurrenceStart)
?: return setCompleted(taskId, completed)
?: return plainUnlessSeries(taskId, occurrenceStart) { setCompleted(taskId, completed) }
val values = TaskWriteMapper.completionValues(completed, System.currentTimeMillis())
val uri = TasksContract.instanceUri(authority(), instanceId)
val rows = resolver.update(uri, values.toContentValues(), null, null)
@@ -631,25 +650,27 @@ class AndroidTasksDataSource @Inject constructor(
}
/**
* An override goes through its instance, which the provider turns into an
* EXDATE on the master — the same as the Room store. Deleting its row alone
* leaves the master without one, so a sync would only drop the edit.
* [write] for a task that turned out not to be a series. A series whose
* occurrence was not found fails instead: the plain write would close all of it.
*/
override fun deleteTask(taskId: Long) {
val authority = authority()
val overrideInstance = try {
resolver.query(
TasksContract.instancesUri(authority),
null,
"${Instances.TASK_ID} = ? AND ${Tasks.ORIGINAL_INSTANCE_ID} IS NOT NULL",
arrayOf(taskId.toString()),
null,
)?.use { c -> if (c.moveToFirst()) CursorColumnReader(c).getLong(Tasks.ID) else null }
} catch (_: SQLiteException) {
null
private fun plainUnlessSeries(taskId: Long, occurrenceStart: Instant, write: () -> Unit) {
if (seriesRules(listOf(taskId)).isNotEmpty()) {
throw TaskWriteFailedException("instance $taskId@$occurrenceStart: no such occurrence")
}
if (overrideInstance != null) resolver.delete(TasksContract.instanceUri(authority, overrideInstance), null, null)
else resolver.delete(taskUri(authority, taskId), null, null)
write()
}
override fun deleteTask(taskId: Long) {
resolver.delete(taskUri(authority(), taskId), null, null)
}
/**
* Through its instance, which the provider turns into an EXDATE on the master,
* as the Room store does. Deleting the row alone leaves the master without one.
*/
override fun deleteOverride(taskId: Long, occurrenceStart: Instant) {
val instanceId = instanceIdFor(taskId, occurrenceStart) ?: return deleteTask(taskId)
resolver.delete(TasksContract.instanceUri(authority(), instanceId), null, null)
}
override fun createLocalList(name: String, color: Int): Long {
@@ -728,5 +749,7 @@ private const val OCCURRENCE_OF = "${Instances.TASK_ID} = ? OR ${Tasks.ORIGINAL_
private fun sameSecond(a: Instant, b: Instant): Boolean =
providerMillis(a.toEpochMilliseconds()) == providerMillis(b.toEpochMilliseconds())
private const val MIGRATIONS = "provider_migrations"
/** SQLite's default bound-argument limit on older Android releases. */
private const val MAX_SQL_ARGS = 999
@@ -67,6 +67,7 @@ class ModeRoutingTasksDataSource(
active().setCancelledInstance(taskId, occurrenceStart, cancelled)
override fun deleteTask(taskId: Long) = active().deleteTask(taskId)
override fun deleteOverride(taskId: Long, occurrenceStart: Instant) = active().deleteOverride(taskId, occurrenceStart)
override fun createLocalList(name: String, color: Int): Long = active().createLocalList(name, color)
override fun updateList(listId: Long, name: String, color: Int) = active().updateList(listId, name, color)
override fun deleteList(listId: Long) = active().deleteList(listId)
@@ -14,15 +14,13 @@ import kotlin.time.Instant
object TaskMapper {
/**
* Maps an `instances` row. [rule] is the series' rule set as stored on the
* master's `tasks` row: the dmfs instances view (DB 22 and 23) masks
* `rrule`/`rdate` with `NULL`, so the row alone cannot tell a series from a
* one-off on tasks.org's bundled provider (DB 22), which also lacks
* `is_recurring`. An override names its master in `original_instance_id`.
* Maps an `instances` row, without its series' rule: the dmfs instances view
* (DB 22 and 23) masks `rrule`/`rdate` with `NULL`, so the row alone cannot
* tell a series from a one-off on tasks.org's bundled provider (DB 22), which
* also lacks `is_recurring`. [withRule] completes it with the rule read from
* the master's `tasks` row. An override names its master in
* `original_instance_id`.
*/
fun task(r: ColumnReader, rule: SeriesRule? = null): Task = withRule(instance(r), rule)
/** An `instances` row mapped without its series' rule, for [withRule] once that is known. */
fun instance(r: ColumnReader): InstanceRow {
fun instant(name: String): Instant? =
r.getLong(name)?.let { Instant.fromEpochMilliseconds(it) }
@@ -131,6 +131,9 @@ interface TasksDataSource {
/** Cancel (or restore) one occurrence of a series; the counterpart of [setCompletedInstance]. */
fun setCancelledInstance(taskId: Long, occurrenceStart: Instant, cancelled: Boolean)
fun deleteTask(taskId: Long)
/** Delete the override [taskId] standing for one occurrence, excluding that occurrence from its series. */
fun deleteOverride(taskId: Long, occurrenceStart: Instant) = deleteTask(taskId)
fun createLocalList(name: String, color: Int): Long
/** Rename and recolour [listId]. */
@@ -240,7 +240,7 @@ class TasksRepositoryImpl @Inject constructor(
RecurringScope.ThisOccurrence ->
// An override row deletes itself into an EXDATE; a generated
// occurrence has no row to delete.
if (current.taskId != seriesId) dataSource.deleteTask(current.taskId)
if (current.taskId != seriesId) dataSource.deleteOverride(current.taskId, occurrence)
else dataSource.deleteOccurrence(seriesId, occurrence)
RecurringScope.ThisAndFollowing -> dataSource.deleteFollowing(seriesId, occurrence)
RecurringScope.AllOccurrences -> dataSource.deleteWithSubtasks(seriesId)
@@ -35,7 +35,7 @@ class TaskMapperTest {
),
)
val task = TaskMapper.task(reader)
val task = mapTask(reader)
assertThat(task.taskId).isEqualTo(7L)
assertThat(task.listId).isEqualTo(3L)
@@ -53,7 +53,7 @@ class TaskMapperTest {
@Test
fun `an occurrence is identified by its recurrence-id anchor`() {
fun occurrence(columns: Map<String, Any?>) =
TaskMapper.task(MapColumnReader(columns), SeriesRule(rrule = "FREQ=DAILY", rdate = null))
mapTask(MapColumnReader(columns), SeriesRule(rrule = "FREQ=DAILY", rdate = null))
// instance_original_time is the provider's own RECURRENCE-ID and wins.
val anchored = occurrence(
@@ -77,7 +77,7 @@ class TaskMapperTest {
@Test
fun `a non-recurring task has no occurrence anchor and keys by task id`() {
val task = TaskMapper.task(
val task = mapTask(
MapColumnReader(mapOf(Tasks.ID to 4L, Instances.INSTANCE_START to 500L)),
)
assertThat(task.occurrenceStart).isNull()
@@ -87,7 +87,7 @@ class TaskMapperTest {
@Test
fun `a series is detected from its master rule when the view masks it (#135)`() {
// tasks.org (DB 22): rule masked in the view, no `is_recurring`.
val task = TaskMapper.task(
val task = mapTask(
MapColumnReader(
mapOf(
Tasks.ID to 90L,
@@ -108,7 +108,7 @@ class TaskMapperTest {
@Test
fun `the master rule fills the editor even where is_recurring exists`() {
// OpenTasks (DB 23) flags the row but masks the rule too.
val task = TaskMapper.task(
val task = mapTask(
MapColumnReader(mapOf(Tasks.ID to 90L, Instances.TASK_ID to 9L, Instances.IS_RECURRING to 1)),
SeriesRule(rrule = "FREQ=WEEKLY;BYDAY=MO", rdate = null),
)
@@ -124,7 +124,7 @@ class TaskMapperTest {
@Test
fun `recurrence is detected from rdate alone`() {
val task = TaskMapper.task(
val task = mapTask(
MapColumnReader(mapOf(Tasks.ID to 1L)),
SeriesRule(rrule = null, rdate = "20260720T090000Z"),
)
@@ -134,7 +134,7 @@ class TaskMapperTest {
@Test
fun `an override belongs to its master's series`() {
// An edited occurrence is its own task row, without a rule of its own.
val task = TaskMapper.task(
val task = mapTask(
MapColumnReader(
mapOf(
Tasks.ID to 95L,
@@ -153,13 +153,13 @@ class TaskMapperTest {
@Test
fun `a plain task is not recurring`() {
val task = TaskMapper.task(MapColumnReader(mapOf(Tasks.ID to 1L, Tasks.TITLE to "One-off")))
val task = mapTask(MapColumnReader(mapOf(Tasks.ID to 1L, Tasks.TITLE to "One-off")))
assertThat(task.isRecurring).isFalse()
}
@Test
fun `falls back to instance id when task_id missing, and list color when no task color`() {
val task = TaskMapper.task(
val task = mapTask(
MapColumnReader(mapOf(Tasks.ID to 9L, Tasks.LIST_COLOR to 0x111111)),
)
assertThat(task.taskId).isEqualTo(9L)
@@ -189,3 +189,5 @@ class TaskMapperTest {
assertThat(list.isLocal).isFalse()
}
}
private fun mapTask(r: ColumnReader, rule: SeriesRule? = null) = TaskMapper.withRule(TaskMapper.instance(r), rule)