Fix repeat until crash

app/src/main/java/com/todoroo/astrid/repeats/RepeatTaskHelper.kt

@@ -49,24 +49,20 @@ class RepeatTaskHelper @Inject constructor(
             }
             newDueDate = computeNextDueDate(task, recurrence, repeatAfterCompletion)
             if (newDueDate == -1L) {
+                broadcastCompletion(task)
                 return
             }
         } catch (e: ParseException) {
             Timber.e(e)
             return
         }
-        val oldDueDate = task.dueDate
-        val repeatUntil = task.repeatUntil
-        if (repeatFinished(newDueDate, repeatUntil)) {
-            broadcastCompletion(task)
-            return
-        }
         if (count > 1) {
             rrule.count = count - 1
             task.setRecurrence(rrule)
         }
         task.reminderLast = 0L
         task.completionDate = 0L
+        val oldDueDate = task.dueDate
         task.setDueDateAdjustingHideUntil(newDueDate)
         gcalHelper.rescheduleRepeatingTask(task)
         taskDao.save(task)
@@ -124,29 +120,35 @@ class RepeatTaskHelper @Inject constructor(
 
     companion object {
         private val weekdayCompare = Comparator { object1: WeekDay, object2: WeekDay -> WeekDay.getCalendarDay(object1) - WeekDay.getCalendarDay(object2) }
-        private fun repeatFinished(newDueDate: Long, repeatUntil: Long): Boolean {
-            return repeatUntil > 0 && newDateTime(newDueDate).startOfDay().millis > repeatUntil
-        }
 
         /** Compute next due date  */
         @Throws(ParseException::class)
         fun computeNextDueDate(task: Task, recurrence: String, repeatAfterCompletion: Boolean): Long {
             val rrule = initRRule(recurrence)
+            if (rrule.until != null && rrule.until is Date && task.hasDueTime()) {
+                // Tasks lets you create tasks with due date-times, but recurrence until with due dates
+                // This violates the spec and should be fixed in the picker
+                rrule.until = DateTime.from(rrule.until).endOfDay().toDateTime()
+            }
 
             // initialize startDateAsDV
             val original = setUpStartDate(task, repeatAfterCompletion, rrule.frequency)
             val startDateAsDV = setUpStartDateAsDV(task, original)
-            return if (rrule.frequency == Recur.Frequency.HOURLY || rrule.frequency == Recur.Frequency.MINUTELY) {
-                handleSubdayRepeat(original, rrule)
-            } else if (rrule.frequency == Recur.Frequency.WEEKLY && rrule.dayList.isNotEmpty() && repeatAfterCompletion) {
-                handleWeeklyRepeatAfterComplete(rrule, original, task.hasDueTime())
-            } else if (rrule.frequency == Recur.Frequency.MONTHLY && rrule.dayList.isEmpty()) {
-                handleMonthlyRepeat(original, startDateAsDV, task.hasDueTime(), rrule)
-            } else {
-                invokeRecurrence(rrule, original, startDateAsDV)
+            return when {
+                rrule.frequency == Recur.Frequency.SECONDLY ||
+                rrule.frequency == Recur.Frequency.MINUTELY ||
+                rrule.frequency == Recur.Frequency.HOURLY ->
+                    handleSubdayRepeat(original, rrule)
+                rrule.frequency == Recur.Frequency.WEEKLY && rrule.dayList.isNotEmpty() && repeatAfterCompletion ->
+                    handleWeeklyRepeatAfterComplete(rrule, original, task.hasDueTime())
+                rrule.frequency == Recur.Frequency.MONTHLY && rrule.dayList.isEmpty() ->
+                    handleMonthlyRepeat(original, startDateAsDV, task.hasDueTime(), rrule)
+                else ->
+                    invokeRecurrence(rrule, original, startDateAsDV)
             }
         }
 
+        @Deprecated("probably don't need this?")
         private fun handleWeeklyRepeatAfterComplete(
                 recur: Recur, original: DateTime, hasDueTime: Boolean): Long {
             val byDay = recur.dayList
@@ -166,6 +168,7 @@ class RepeatTaskHelper @Inject constructor(
             }
         }
 
+        @Deprecated("Properly support last day of month and remove this")
         private fun handleMonthlyRepeat(
                 original: DateTime, startDateAsDV: Date, hasDueTime: Boolean, recur: Recur): Long {
             return if (original.isLastDayOfMonth) {
@@ -195,7 +198,7 @@ class RepeatTaskHelper @Inject constructor(
         private fun invokeRecurrence(recur: Recur, original: DateTime, startDateAsDV: Date): Long {
             return recur.getNextDate(startDateAsDV, startDateAsDV)
                 ?.let { buildNewDueDate(original, it) }
-                ?: throw IllegalStateException("recur=$recur original=$original startDateAsDv=$startDateAsDV")
+                ?: -1
         }
 
         /** Compute long due date from DateValue  */
@@ -253,6 +256,7 @@ class RepeatTaskHelper @Inject constructor(
             }
         }
 
+        @Deprecated("probably don't need this?")
         private fun handleSubdayRepeat(startDate: DateTime, recur: Recur): Long {
             val millis: Long = when (recur.frequency) {
                 Recur.Frequency.HOURLY -> DateUtilities.ONE_HOUR

app/src/test/java/com/todoroo/astrid/repeats/RepeatTaskHelperTests.kt

@@ -1,7 +1,9 @@
 package com.todoroo.astrid.repeats
 
 import com.natpryce.makeiteasy.MakeItEasy.with
-import org.junit.Assert.*
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.tasks.Freeze.Companion.freezeAt
 import org.tasks.makers.TaskMaker.COMPLETION_TIME
@@ -62,19 +64,4 @@ class RepeatTaskHelperTests : RepeatTests() {
 
         assertEquals(newDay(2021, 2, 2), next)
     }
-
-    @Test
-    fun useNowWhenNoCompletion() {
-        val task = newFromDue(
-                "FREQ=DAILY;INTERVAL=1",
-                newDay(2021, 3, 15),
-                afterComplete = true
-        )
-
-        val next = freezeAt(newDayTime(2021, 3, 29, 14, 32)) {
-            calculateNextDueDate(task)
-        }
-
-        assertEquals(newDayTime(2021, 3, 30, 12, 0), next)
-    }
 }

app/src/test/java/com/todoroo/astrid/repeats/RepeatTests.kt

@@ -6,11 +6,12 @@ import com.todoroo.astrid.alarms.AlarmService
 import com.todoroo.astrid.dao.TaskDao
 import com.todoroo.astrid.data.Task
 import com.todoroo.astrid.gcal.GCalHelper
-import com.todoroo.astrid.service.TaskCompleter
 import kotlinx.coroutines.runBlocking
 import org.junit.Before
-import org.mockito.Mockito
-import org.mockito.Mockito.*
+import org.mockito.Mockito.anyLong
+import org.mockito.Mockito.anySet
+import org.mockito.Mockito.mock
+import org.mockito.Mockito.`when`
 import org.tasks.LocalBroadcastManager
 import org.tasks.makers.TaskMaker
 import org.tasks.time.DateTime
@@ -62,6 +63,7 @@ abstract class RepeatTests {
             MakeItEasy.with(TaskMaker.RECUR, recur),
             MakeItEasy.with(TaskMaker.AFTER_COMPLETE, afterComplete),
             MakeItEasy.with(TaskMaker.DUE_TIME, due),
+            MakeItEasy.with(TaskMaker.COMPLETION_TIME, DateTime()),
             *properties
     )
 }

app/src/test/java/com/todoroo/astrid/repeats/RepeatWeeklyTests.kt

@@ -2,6 +2,7 @@ package com.todoroo.astrid.repeats
 
 import com.natpryce.makeiteasy.MakeItEasy.with
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.tasks.makers.TaskMaker.COMPLETION_TIME
 
@@ -196,4 +197,28 @@ class RepeatWeeklyTests : RepeatTests() {
 
         assertEquals(newDayTime(2016, 9, 12, 0, 25), next)
     }
+
+    @Test
+    fun weeklyByDayFromDueDateOnUntil() {
+        val task = newFromDue(
+            "FREQ=WEEKLY;UNTIL=20240315;BYDAY=MO,TU,WE,TH,FR,SU",
+            newDayTime(2024, 3, 14, 3, 45)
+        )
+
+        val next = calculateNextDueDate(task)
+
+        assertEquals(newDayTime(2024, 3, 15, 3, 45), next)
+    }
+
+    @Test
+    fun weeklyByDayFromDueDateEndAfterUntil() {
+        val task = newFromDue(
+            "FREQ=WEEKLY;UNTIL=20240315;BYDAY=MO,TU,WE,TH,FR,SU",
+            newDayTime(2024, 3, 15, 3, 45)
+        )
+
+        calculateNextDueDate(task)
+
+        assertTrue(task.isCompleted)
+    }
 }