Fix snooze causing duplicate notifications

app/src/androidTest/java/com/todoroo/astrid/alarms/AlarmJobServiceTest.kt

@@ -1,108 +1,258 @@
 package com.todoroo.astrid.alarms
 
-import com.natpryce.makeiteasy.MakeItEasy.with
-import com.todoroo.andlib.utility.DateUtilities
 import dagger.hilt.android.testing.HiltAndroidTest
 import dagger.hilt.android.testing.UninstallModules
 import kotlinx.coroutines.runBlocking
 import org.junit.Assert.assertEquals
 import org.junit.Test
-import org.tasks.data.entity.Alarm
-import org.tasks.data.entity.Alarm.Companion.TYPE_DATE_TIME
-import org.tasks.data.entity.Alarm.Companion.TYPE_RANDOM
-import org.tasks.data.entity.Alarm.Companion.TYPE_SNOOZE
-import org.tasks.data.entity.Alarm.Companion.whenDue
-import org.tasks.data.entity.Alarm.Companion.whenOverdue
-import org.tasks.data.dao.AlarmDao
+import org.tasks.SuspendFreeze.Companion.freezeAt
+import org.tasks.data.createDueDate
 import org.tasks.data.dao.TaskDao
-import org.tasks.date.DateTimeUtils.newDateTime
+import org.tasks.data.entity.Alarm
+import org.tasks.data.entity.Notification
+import org.tasks.data.entity.Task
 import org.tasks.injection.InjectingTestCase
 import org.tasks.injection.ProductionModule
-import org.tasks.jobs.AlarmEntry
-import org.tasks.makers.TaskMaker.COMPLETION_TIME
-import org.tasks.makers.TaskMaker.DELETION_TIME
-import org.tasks.makers.TaskMaker.DUE_DATE
-import org.tasks.makers.TaskMaker.DUE_TIME
-import org.tasks.makers.TaskMaker.REMINDER_LAST
-import org.tasks.makers.TaskMaker.newTask
 import org.tasks.time.DateTime
+import org.tasks.time.DateTimeUtils2
+import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 
 @UninstallModules(ProductionModule::class)
 @HiltAndroidTest
 class AlarmJobServiceTest : InjectingTestCase() {
-    @Inject lateinit var alarmDao: AlarmDao
     @Inject lateinit var taskDao: TaskDao
     @Inject lateinit var alarmService: AlarmService
 
     @Test
-    fun scheduleAlarm() = runBlocking {
-        val task = taskDao.createNew(newTask())
-        val alarm = insertAlarm(Alarm(task, DateTime(2017, 9, 24, 19, 57).millis, TYPE_DATE_TIME))
+    fun testNoAlarms() = runBlocking {
+        testResults(emptyList(), 0)
+    }
 
-        verify(overdue = listOf(AlarmEntry(alarm, task, DateTime(2017, 9, 24, 19, 57).millis, TYPE_DATE_TIME)))
+    @Test
+    fun futureAlarmWithNoPastAlarm() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 18).millis
+                    )
+                )
+            )
+            alarmService.synchronizeAlarms(1, mutableSetOf(Alarm(0, 0, Alarm.TYPE_REL_END)))
+
+            testResults(emptyList(), DateTime(2024, 5, 18, 18, 0).millis)
+        }
     }
 
     @Test
-    fun ignoreStaleAlarm() = runBlocking {
-        val alarmTime = DateTime(2017, 9, 24, 19, 57)
-        val task = taskDao.createNew(newTask(with(REMINDER_LAST, alarmTime.endOfMinute())))
-        alarmDao.insert(Alarm(task, alarmTime.millis, TYPE_DATE_TIME))
+    fun pastAlarmWithNoFutureAlarm() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    )
+                )
+            )
+            alarmService.synchronizeAlarms(1, mutableSetOf(Alarm(0, 0, Alarm.TYPE_REL_END)))
 
-        verify()
+            testResults(
+                listOf(
+                    Notification(
+                        taskId = 1L,
+                        timestamp = DateTimeUtils2.currentTimeMillis(),
+                        type = Alarm.TYPE_REL_END
+                    )
+                ),
+                0
+            )
+        }
     }
 
     @Test
-    fun dontScheduleReminderForCompletedTask() = runBlocking {
-        val task = taskDao.insert(
-            newTask(
-                with(DUE_DATE, newDateTime()),
-                with(COMPLETION_TIME, newDateTime())
+    fun pastRecurringAlarmWithFutureRecurrence() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    )
+                )
+            )
+            alarmService.synchronizeAlarms(
+                1,
+                mutableSetOf(
+                    Alarm(
+                        0,
+                        0,
+                        Alarm.TYPE_REL_END,
+                        repeat = 1,
+                        interval = TimeUnit.HOURS.toMillis(6)
+                    )
+                )
             )
-        )
-        alarmDao.insert(whenDue(task))
 
-        verify()
+            testResults(
+                listOf(
+                    Notification(
+                        taskId = 1L,
+                        timestamp = DateTimeUtils2.currentTimeMillis(),
+                        type = Alarm.TYPE_REL_END
+                    )
+                ),
+                DateTime(2024, 5, 18, 0, 0).millis
+            )
+        }
     }
 
     @Test
-    fun dontScheduleReminderForDeletedTask() = runBlocking {
-        val task = taskDao.insert(
-            newTask(
-                with(DUE_DATE, newDateTime()),
-                with(DELETION_TIME, newDateTime())
+    fun pastAlarmsRemoveSnoozed() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    )
+                )
+            )
+            alarmService.synchronizeAlarms(
+                1,
+                mutableSetOf(
+                    Alarm(0, 0, Alarm.TYPE_REL_END),
+                    Alarm(0, DateTimeUtils2.currentTimeMillis(), Alarm.TYPE_SNOOZE)
+                )
+            )
+
+            testResults(
+                listOf(
+                    Notification(
+                        taskId = 1L,
+                        timestamp = DateTimeUtils2.currentTimeMillis(),
+                        type = Alarm.TYPE_REL_END
+                    )
+                ),
+                0
             )
-        )
-        alarmDao.insert(whenDue(task))
 
-        verify()
+            assertEquals(
+                listOf(Alarm(id = 1, task = 1, time = 0, type = Alarm.TYPE_REL_END)),
+                alarmService.getAlarms(1)
+            )
+        }
     }
 
     @Test
-    fun snoozeOverridesAll() = runBlocking {
-        val now = newDateTime()
-        val task = taskDao.insert(newTask(with(DUE_TIME, now)))
+    fun futureSnoozeOverrideOverdue() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    )
+                )
+            )
+            alarmService.synchronizeAlarms(
+                1,
+                mutableSetOf(
+                    Alarm(0, 0, Alarm.TYPE_REL_END),
+                    Alarm(
+                        0,
+                        DateTimeUtils2.currentTimeMillis() + TimeUnit.MINUTES.toMillis(5),
+                        Alarm.TYPE_SNOOZE
+                    )
+                )
+            )
+
+            testResults(
+                emptyList(),
+                DateTimeUtils2.currentTimeMillis() + TimeUnit.MINUTES.toMillis(5)
+            )
+        }
+    }
 
-        alarmDao.insert(whenDue(task))
-        alarmDao.insert(whenOverdue(task))
-        alarmDao.insert(Alarm(task, DateUtilities.ONE_HOUR, TYPE_RANDOM))
-        val alarm = alarmDao.insert(Alarm(task, now.plusMonths(12).millis, TYPE_SNOOZE))
+    @Test
+    fun ignoreStaleAlarm() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    ),
+                    reminderLast = DateTime(2024, 5, 17, 18, 0).millis,
+                )
+            )
+            alarmService.synchronizeAlarms(
+                1,
+                mutableSetOf(Alarm(0, 0, Alarm.TYPE_REL_END))
+            )
 
-        verify(future = listOf(AlarmEntry(alarm, task, now.plusMonths(12).millis, TYPE_SNOOZE)))
+            testResults(
+                emptyList(),
+                0
+            )
+        }
     }
 
-    private suspend fun insertAlarm(alarm: Alarm): Long {
-        alarm.id = alarmDao.insert(alarm)
-        return alarm.id
+    @Test
+    fun dontScheduleForCompletedTask() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    ),
+                    completionDate = DateTime(2024, 5, 17, 14, 0).millis,
+                )
+            )
+            alarmService.synchronizeAlarms(
+                1,
+                mutableSetOf(Alarm(0, 0, Alarm.TYPE_REL_END))
+            )
+
+            testResults(
+                emptyList(),
+                0
+            )
+        }
     }
 
-    private suspend fun verify(
-        overdue: List<AlarmEntry> = emptyList(),
-        future: List<AlarmEntry> = emptyList(),
-    ) {
-        val (actualOverdue, actualFuture) = alarmService.getAlarms()
+    @Test
+    fun dontScheduleForDeletedTask() = runBlocking {
+        freezeAt(DateTime(2024, 5, 17, 23, 20)) {
+            taskDao.insert(
+                Task(
+                    dueDate = createDueDate(
+                        Task.URGENCY_SPECIFIC_DAY,
+                        DateTime(2024, 5, 17).millis
+                    ),
+                    deletionDate = DateTime(2024, 5, 17, 14, 0).millis,
+                )
+            )
+            alarmService.synchronizeAlarms(
+                1,
+                mutableSetOf(Alarm(0, 0, Alarm.TYPE_REL_END))
+            )
+
+            testResults(
+                emptyList(),
+                0
+            )
+        }
+    }
 
-        assertEquals(overdue, actualOverdue)
-        assertEquals(future, actualFuture)
+    private suspend fun testResults(notifications: List<Notification>, nextAlarm: Long) {
+        val actualNextAlarm = alarmService.triggerAlarms {
+            assertEquals(notifications, it)
+            it.forEach { taskDao.setLastNotified(it.taskId, DateTimeUtils2.currentTimeMillis()) }
+        }
+        assertEquals(nextAlarm, actualNextAlarm)
     }
 }

app/src/main/java/com/todoroo/astrid/alarms/AlarmService.kt

@@ -8,11 +8,14 @@ package com.todoroo.astrid.alarms
 import org.tasks.LocalBroadcastManager
 import org.tasks.data.dao.AlarmDao
 import org.tasks.data.dao.TaskDao
+import org.tasks.data.db.DbUtils
 import org.tasks.data.entity.Alarm
 import org.tasks.data.entity.Alarm.Companion.TYPE_SNOOZE
+import org.tasks.data.entity.Notification
 import org.tasks.jobs.AlarmEntry
 import org.tasks.jobs.WorkManager
 import org.tasks.notifications.NotificationManager
+import org.tasks.preferences.Preferences
 import org.tasks.time.DateTime
 import org.tasks.time.DateTimeUtils2.currentTimeMillis
 import timber.log.Timber
@@ -30,6 +33,7 @@ class AlarmService @Inject constructor(
     private val notificationManager: NotificationManager,
     private val workManager: WorkManager,
     private val alarmCalculator: AlarmCalculator,
+    private val preferences: Preferences,
 ) {
     suspend fun getAlarms(taskId: Long): List<Alarm> = alarmDao.getAlarms(taskId)
 
@@ -70,7 +74,33 @@ class AlarmService @Inject constructor(
         workManager.triggerNotifications()
     }
 
-    suspend fun getAlarms(): Pair<List<AlarmEntry>, List<AlarmEntry>> {
+    suspend fun triggerAlarms(
+        trigger: suspend (List<Notification>) -> Unit
+    ): Long {
+        if (preferences.isCurrentlyQuietHours) {
+            return preferences.adjustForQuietHours(currentTimeMillis())
+        }
+        val (overdue, _) = getAlarms()
+        overdue
+            .sortedBy { it.time }
+            .also { alarms ->
+                alarms
+                    .map { it.taskId }
+                    .chunked(DbUtils.MAX_SQLITE_ARGS)
+                    .onEach { alarmDao.deleteSnoozed(it) }
+            }
+            .map { it.toNotification() }
+            .let { trigger(it) }
+        val alreadyTriggered = overdue.map { it.taskId }.toSet()
+        val (moreOverdue, future) = getAlarms()
+        return moreOverdue
+            .filterNot { it.type == Alarm.TYPE_RANDOM || alreadyTriggered.contains(it.taskId) }
+            .plus(future)
+            .minOfOrNull { it.time }
+            ?: 0
+    }
+
+    internal suspend fun getAlarms(): Pair<List<AlarmEntry>, List<AlarmEntry>> {
         val start = currentTimeMillis()
         val overdue = ArrayList<AlarmEntry>()
         val future = ArrayList<AlarmEntry>()
@@ -83,7 +113,8 @@ class AlarmService @Inject constructor(
                 }
                 val (now, later) = alarmEntries.partition { it.time <= DateTime().startOfMinute().plusMinutes(1).millis }
                 later
-                    .find { it.type == TYPE_SNOOZE }
+                    .filter { it.type == TYPE_SNOOZE }
+                    .maxByOrNull { it.time }
                     ?.let { future.add(it) }
                     ?: run {
                         now.firstOrNull()?.let { overdue.add(it) }

app/src/main/java/org/tasks/jobs/NotificationWork.kt

@@ -11,13 +11,8 @@ import dagger.assisted.AssistedInject
 import org.tasks.Notifier
 import org.tasks.R
 import org.tasks.analytics.Firebase
-import org.tasks.data.entity.Alarm.Companion.TYPE_RANDOM
-import org.tasks.data.entity.Alarm.Companion.TYPE_SNOOZE
-import org.tasks.data.dao.AlarmDao
 import org.tasks.date.DateTimeUtils.toDateTime
 import org.tasks.notifications.NotificationManager
-import org.tasks.preferences.Preferences
-import org.tasks.time.DateTimeUtils2.currentTimeMillis
 import timber.log.Timber
 
 @HiltWorker
@@ -27,44 +22,19 @@ class NotificationWork @AssistedInject constructor(
     firebase: Firebase,
     private val workManager: WorkManager,
     private val alarmService: AlarmService,
-    private val alarmDao: AlarmDao,
-    private val preferences: Preferences,
     private val notifier: Notifier,
 ) : RepeatingWorker(context, workerParams, firebase) {
     private var nextAlarm: Long = 0
 
     override suspend fun run(): Result {
-        if (preferences.isCurrentlyQuietHours) {
-            nextAlarm = preferences.adjustForQuietHours(currentTimeMillis())
-            return Result.success()
-        }
-        val (overdue, _) = alarmService.getAlarms()
-        if (overdue.isNotEmpty()) {
-            overdue
-                .sortedBy { it.time }
-                .also { alarms ->
-                    alarms
-                        .filter { it.type == TYPE_SNOOZE }
-                        .map { it.id }
-                        .let { alarmDao.deleteByIds(it) }
-                }
-                .map { it.toNotification() }
-                .let { notifier.triggerNotifications(it) }
-        }
-        val alreadyTriggered = overdue.map { it.taskId }.toSet()
-        val (moreOverdue, future) = alarmService.getAlarms()
-        nextAlarm = moreOverdue
-            .filterNot { it.type == TYPE_RANDOM || alreadyTriggered.contains(it.taskId)}
-            .plus(future)
-            .minOfOrNull { it.time }
-            ?: 0
+        nextAlarm = alarmService.triggerAlarms { notifier.triggerNotifications(it) }
         return Result.success()
     }
 
     override suspend fun scheduleNext() {
         if (nextAlarm > 0) {
             Timber.d("nextAlarm=${nextAlarm.toDateTime()}")
-            workManager.scheduleNotification(preferences.adjustForQuietHours(nextAlarm))
+            workManager.scheduleNotification(nextAlarm)
         }
     }
 

data/src/main/kotlin/org/tasks/data/dao/AlarmDao.kt

@@ -4,9 +4,9 @@ import androidx.room.Dao
 import androidx.room.Delete
 import androidx.room.Insert
 import androidx.room.Query
-import org.tasks.data.entity.Task
 import org.tasks.data.entity.Alarm
 import org.tasks.data.entity.Alarm.Companion.TYPE_SNOOZE
+import org.tasks.data.entity.Task
 
 @Dao
 interface AlarmDao {
@@ -35,8 +35,8 @@ WHERE tasks._id = :taskId
     @Query("SELECT * FROM alarms WHERE task = :taskId")
     suspend fun getAlarms(taskId: Long): List<Alarm>
 
-    @Query("DELETE FROM alarms WHERE _id IN(:alarmIds)")
-    suspend fun deleteByIds(alarmIds: List<Long>)
+    @Query("DELETE FROM alarms WHERE type = $TYPE_SNOOZE AND task IN(:taskIds)")
+    suspend fun deleteSnoozed(taskIds: List<Long>)
 
     @Delete
     suspend fun delete(alarm: Alarm)

data/src/main/kotlin/org/tasks/data/entity/Alarm.kt

@@ -1,18 +1,19 @@
 package org.tasks.data.entity
 
-import android.os.Parcel
 import android.os.Parcelable
 import androidx.room.ColumnInfo
 import androidx.room.Entity
 import androidx.room.ForeignKey
 import androidx.room.Ignore
 import androidx.room.PrimaryKey
+import kotlinx.parcelize.Parcelize
 import kotlinx.serialization.Serializable
 import kotlinx.serialization.Transient
 import org.tasks.data.db.Table
 import org.tasks.time.printTimestamp
 import java.util.concurrent.TimeUnit
 
+@Parcelize
 @Serializable
 @Entity(
     tableName = Alarm.TABLE_NAME,
@@ -25,64 +26,33 @@ import java.util.concurrent.TimeUnit
         )
     ]
 )
-class Alarm : Parcelable {
+data class Alarm(
     @PrimaryKey(autoGenerate = true)
     @ColumnInfo(name = "_id")
     @Transient
-    var id: Long = 0
-
+    var id: Long = 0,
     @ColumnInfo(name = "task", index = true)
     @Transient
-    var task: Long = 0
-
+    var task: Long = 0,
     @ColumnInfo(name = "time")
-    var time: Long = 0
-
+    var time: Long = 0,
     @ColumnInfo(name = "type", defaultValue = "0")
-    var type: Int = 0
-
+    var type: Int = 0,
     @ColumnInfo(name = "repeat", defaultValue = "0")
-    var repeat: Int = 0
-
+    var repeat: Int = 0,
     @ColumnInfo(name = "interval", defaultValue = "0")
-    var interval: Long = 0
-
-    constructor()
-
+    var interval: Long = 0,
+) : Parcelable {
     @Ignore
-    constructor(parcel: Parcel) {
-        id = parcel.readLong()
-        task = parcel.readLong()
-        time = parcel.readLong()
-        type = parcel.readInt()
-        repeat = parcel.readInt()
-        interval = parcel.readLong()
-    }
-
-    @Ignore
-    constructor(task: Long, time: Long, type: Int, repeat: Int = 0, interval: Long = 0) {
-        this.task = task
-        this.time = time
-        this.type = type
-        this.repeat = repeat
-        this.interval = interval
-    }
-
-    override fun writeToParcel(parcel: Parcel, flags: Int) {
-        parcel.writeLong(id)
-        parcel.writeLong(task)
-        parcel.writeLong(time)
-        parcel.writeInt(type)
-        parcel.writeInt(repeat)
-        parcel.writeLong(interval)
-    }
-
-    override fun describeContents() = 0
+    constructor(task: Long, time: Long, type: Int, repeat: Int = 0, interval: Long = 0): this(
+        id = 0,
+        task = task,
+        time = time,
+        type = type,
+        repeat = repeat,
+        interval = interval
+    )
 
-    override fun toString(): String {
-        val timestamp = if (type == TYPE_DATE_TIME) printTimestamp(time) else time
-        return "Alarm(id=$id, task=$task, time=$timestamp, type=$type, repeat=$repeat, interval=$interval)"
-    }
 
     fun same(other: Alarm) =
         type == other.type &&
@@ -90,30 +60,9 @@ class Alarm : Parcelable {
                 repeat == other.repeat &&
                 interval == other.interval
 
-    override fun equals(other: Any?): Boolean {
-        if (this === other) return true
-        if (javaClass != other?.javaClass) return false
-
-        other as Alarm
-
-        if (id != other.id) return false
-        if (task != other.task) return false
-        if (time != other.time) return false
-        if (type != other.type) return false
-        if (repeat != other.repeat) return false
-        if (interval != other.interval) return false
-
-        return true
-    }
-
-    override fun hashCode(): Int {
-        var result = id.hashCode()
-        result = 31 * result + task.hashCode()
-        result = 31 * result + time.hashCode()
-        result = 31 * result + type
-        result = 31 * result + repeat
-        result = 31 * result + interval.hashCode()
-        return result
+    override fun toString(): String {
+        val timestamp = if (type == TYPE_DATE_TIME) printTimestamp(time) else time
+        return "Alarm(id=$id, task=$task, time=$timestamp, type=$type, repeat=$repeat, interval=$interval)"
     }
 
     companion object {
@@ -137,12 +86,5 @@ class Alarm : Parcelable {
 
         fun whenOverdue(task: Long) =
             Alarm(task, TimeUnit.DAYS.toMillis(1), TYPE_REL_END, 6, TimeUnit.DAYS.toMillis(1))
-
-        @JvmField
-        val CREATOR = object : Parcelable.Creator<Alarm> {
-            override fun createFromParcel(parcel: Parcel) = Alarm(parcel)
-
-            override fun newArray(size: Int): Array<Alarm?> = arrayOfNulls(size)
-        }
     }
 }

data/src/main/kotlin/org/tasks/data/entity/Notification.kt

@@ -21,26 +21,19 @@ import org.tasks.data.db.Table
         )
     ]
 )
-class Notification {
+data class Notification(
     @PrimaryKey(autoGenerate = true)
     @ColumnInfo(name = "uid")
-    var uid = 0
-
+    var uid: Long = 0,
     @ColumnInfo(name = "task")
-    var taskId: Long = 0
-
+    var taskId: Long = 0,
     @ColumnInfo(name = "timestamp")
-    var timestamp: Long = 0
-
+    var timestamp: Long = 0,
     @ColumnInfo(name = "type")
-    var type = 0
-
+    var type: Int = 0,
     @ColumnInfo(name = "location")
-    var location: Long? = null
-
-    override fun toString(): String =
-            "Notification(uid=$uid, taskId=$taskId, timestamp=$timestamp, type=$type, location=$location)"
-
+    var location: Long? = null,
+) {
     companion object {
         const val TABLE_NAME = "notification"
         val TABLE = Table(TABLE_NAME)