From 33bd0b5d06af030d9e8951eb32d88a7851e7bfb8 Mon Sep 17 00:00:00 2001 From: Alex Baker Date: Fri, 22 Sep 2017 17:43:18 -0500 Subject: [PATCH] Overdue reminders occur every 24 hours --- .../astrid/reminders/ReminderServiceTest.java | 147 +++++++++--------- .../astrid/reminders/ReminderService.java | 58 ++----- .../org/tasks/preferences/Preferences.java | 5 + 3 files changed, 90 insertions(+), 120 deletions(-) diff --git a/app/src/androidTest/java/com/todoroo/astrid/reminders/ReminderServiceTest.java b/app/src/androidTest/java/com/todoroo/astrid/reminders/ReminderServiceTest.java index 26a3066e5..8c23c1cad 100644 --- a/app/src/androidTest/java/com/todoroo/astrid/reminders/ReminderServiceTest.java +++ b/app/src/androidTest/java/com/todoroo/astrid/reminders/ReminderServiceTest.java @@ -6,10 +6,10 @@ import com.todoroo.astrid.data.Task; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InOrder; +import org.tasks.R; import org.tasks.Snippet; import org.tasks.injection.InjectingTestCase; import org.tasks.injection.TestComponent; @@ -19,6 +19,8 @@ import org.tasks.preferences.Preferences; import org.tasks.reminders.Random; import org.tasks.time.DateTime; +import java.util.concurrent.TimeUnit; + import javax.inject.Inject; import static com.natpryce.makeiteasy.MakeItEasy.with; @@ -39,13 +41,11 @@ import static org.tasks.makers.TaskMaker.DELETION_TIME; import static org.tasks.makers.TaskMaker.DUE_DATE; import static org.tasks.makers.TaskMaker.DUE_TIME; import static org.tasks.makers.TaskMaker.ID; -import static org.tasks.makers.TaskMaker.PRIORITY; import static org.tasks.makers.TaskMaker.RANDOM_REMINDER_PERIOD; import static org.tasks.makers.TaskMaker.REMINDERS; import static org.tasks.makers.TaskMaker.REMINDER_LAST; import static org.tasks.makers.TaskMaker.SNOOZE_TIME; import static org.tasks.makers.TaskMaker.newTask; -import static org.tasks.time.DateTimeUtils.currentTimeMillis; @RunWith(AndroidJUnit4.class) public class ReminderServiceTest extends InjectingTestCase { @@ -61,6 +61,7 @@ public class ReminderServiceTest extends InjectingTestCase { jobs = mock(JobQueue.class); random = mock(Random.class); when(random.nextFloat()).thenReturn(1.0f); + preferences.reset(); service = new ReminderService(preferences, jobs, random); } @@ -121,6 +122,7 @@ public class ReminderServiceTest extends InjectingTestCase { with(ID, 1L), with(DUE_DATE, now), with(REMINDERS, NOTIFY_AT_DEADLINE)); + service.scheduleAlarm(null, task); InOrder order = inOrder(jobs); @@ -257,113 +259,110 @@ public class ReminderServiceTest extends InjectingTestCase { } @Test - public void scheduleOverdueForFutureDueDate() { - freezeClock().thawAfter(new Snippet() {{ - when(random.nextFloat()).thenReturn(0.3865f); - Task task = newTask( - with(ID, 1L), - with(DUE_TIME, newDateTime().plusMinutes(5)), - with(REMINDERS, NOTIFY_AFTER_DEADLINE)); + public void scheduleOverdueNoLastReminder() { + Task task = newTask( + with(ID, 1L), + with(DUE_TIME, new DateTime(2017, 9, 22, 15, 30)), + with(REMINDER_LAST, (DateTime) null), + with(REMINDERS, NOTIFY_AFTER_DEADLINE)); - service.scheduleAlarm(null, task); + service.scheduleAlarm(null, task); - InOrder order = inOrder(jobs); - order.verify(jobs).cancelReminder(1); - order.verify(jobs).add(new Reminder(1L, task.getDueDate() + 4582800, ReminderService.TYPE_OVERDUE)); - }}); + InOrder order = inOrder(jobs); + order.verify(jobs).cancelReminder(1); + order.verify(jobs).add(new Reminder(1L, new DateTime(2017, 9, 23, 15, 30, 1, 0).getMillis(), ReminderService.TYPE_OVERDUE)); } @Test - public void scheduleOverdueForPastDueDateWithNoReminderPastDueDate() { - freezeClock().thawAfter(new Snippet() {{ - DateTime now = newDateTime(); - Task task = newTask( - with(ID, 1L), - with(DUE_TIME, now.minusMinutes(5)), - with(REMINDERS, NOTIFY_AFTER_DEADLINE)); + public void scheduleOverduePastLastReminder() { + Task task = newTask( + with(ID, 1L), + with(DUE_TIME, new DateTime(2017, 9, 22, 15, 30)), + with(REMINDER_LAST, new DateTime(2017, 9, 24, 12, 0)), + with(REMINDERS, NOTIFY_AFTER_DEADLINE)); - service.scheduleAlarm(null, task); + service.scheduleAlarm(null, task); - InOrder order = inOrder(jobs); - order.verify(jobs).cancelReminder(1); - order.verify(jobs).add(new Reminder(1L, currentTimeMillis(), ReminderService.TYPE_OVERDUE)); - }}); + InOrder order = inOrder(jobs); + order.verify(jobs).cancelReminder(1); + order.verify(jobs).add(new Reminder(1L, new DateTime(2017, 9, 24, 15, 30, 1, 0).getMillis(), ReminderService.TYPE_OVERDUE)); } @Test - public void scheduleOverdueForPastDueDateLastReminderSixHoursAgo() { - freezeClock().thawAfter(new Snippet() {{ - Task task = newTask( - with(ID, 1L), - with(DUE_TIME, newDateTime().minusHours(12)), - with(REMINDER_LAST, newDateTime().minusHours(6)), - with(REMINDERS, NOTIFY_AFTER_DEADLINE)); + public void scheduleOverdueBeforeLastReminder() { + Task task = newTask( + with(ID, 1L), + with(DUE_TIME, new DateTime(2017, 9, 22, 12, 30)), + with(REMINDER_LAST, new DateTime(2017, 9, 24, 15, 0)), + with(REMINDERS, NOTIFY_AFTER_DEADLINE)); - service.scheduleAlarm(null, task); + service.scheduleAlarm(null, task); - InOrder order = inOrder(jobs); - order.verify(jobs).cancelReminder(1); - order.verify(jobs).add(new Reminder(1L, currentTimeMillis(), ReminderService.TYPE_OVERDUE)); - }}); + InOrder order = inOrder(jobs); + order.verify(jobs).cancelReminder(1); + order.verify(jobs).add(new Reminder(1L, new DateTime(2017, 9, 25, 12, 30, 1, 0).getMillis(), ReminderService.TYPE_OVERDUE)); } @Test - public void scheduleOverdueForPastDueDateLastReminderWithinSixHours() { - freezeClock().thawAfter(new Snippet() {{ - when(random.nextFloat()).thenReturn(0.3865f); - Task task = newTask( - with(ID, 1L), - with(DUE_TIME, newDateTime().minusHours(12)), - with(PRIORITY, 2), - with(REMINDER_LAST, newDateTime().minusHours(3)), - with(REMINDERS, NOTIFY_AFTER_DEADLINE)); + public void scheduleOverdueWithNoDueTime() { + preferences.setInt(R.string.p_rmd_time, (int) TimeUnit.HOURS.toMillis(15)); + Task task = newTask( + with(ID, 1L), + with(DUE_DATE, new DateTime(2017, 9, 22)), + with(REMINDER_LAST, new DateTime(2017, 9, 23, 12, 17, 59, 999)), + with(REMINDERS, NOTIFY_AFTER_DEADLINE)); - service.scheduleAlarm(null, task); + service.scheduleAlarm(null, task); - InOrder order = inOrder(jobs); - order.verify(jobs).cancelReminder(1); - order.verify(jobs).add(new Reminder(1L, currentTimeMillis() + 22748400, ReminderService.TYPE_OVERDUE)); - }}); + InOrder order = inOrder(jobs); + order.verify(jobs).cancelReminder(1); + order.verify(jobs).add(new Reminder(1L, new DateTime(2017, 9, 23, 15, 0, 0, 0).getMillis(), ReminderService.TYPE_OVERDUE)); } @Test - public void snoozeOverridesAll() { - DateTime now = newDateTime(); + public void scheduleSubsequentOverdueReminder() { Task task = newTask( with(ID, 1L), - with(DUE_TIME, now), - with(SNOOZE_TIME, now.plusMonths(12)), - with(REMINDERS, NOTIFY_AT_DEADLINE | NOTIFY_AFTER_DEADLINE), - with(RANDOM_REMINDER_PERIOD, ONE_HOUR)); + with(DUE_TIME, new DateTime(2017, 9, 22, 15, 30)), + with(REMINDER_LAST, new DateTime(2017, 9, 23, 15, 30, 59, 999)), + with(REMINDERS, NOTIFY_AFTER_DEADLINE)); service.scheduleAlarm(null, task); InOrder order = inOrder(jobs); order.verify(jobs).cancelReminder(1); - order.verify(jobs).add(new Reminder(1, now.plusMonths(12).getMillis(), ReminderService.TYPE_SNOOZE)); + order.verify(jobs).add(new Reminder(1L, new DateTime(2017, 9, 24, 15, 30, 1, 0).getMillis(), ReminderService.TYPE_OVERDUE)); } @Test - @Ignore - public void randomReminderBeforeDueAndOverdue() { - - } + public void scheduleOverdueAfterLastReminder() { + Task task = newTask( + with(ID, 1L), + with(DUE_TIME, new DateTime(2017, 9, 22, 15, 30)), + with(REMINDER_LAST, new DateTime(2017, 9, 23, 12, 17, 59, 999)), + with(REMINDERS, NOTIFY_AFTER_DEADLINE)); - @Test - @Ignore - public void randomReminderAfterDue() { + service.scheduleAlarm(null, task); + InOrder order = inOrder(jobs); + order.verify(jobs).cancelReminder(1); + order.verify(jobs).add(new Reminder(1L, new DateTime(2017, 9, 23, 15, 30, 1, 0).getMillis(), ReminderService.TYPE_OVERDUE)); } @Test - @Ignore - public void randomReminderAfterOverdue() { - - } + public void snoozeOverridesAll() { + DateTime now = newDateTime(); + Task task = newTask( + with(ID, 1L), + with(DUE_TIME, now), + with(SNOOZE_TIME, now.plusMonths(12)), + with(REMINDERS, NOTIFY_AT_DEADLINE | NOTIFY_AFTER_DEADLINE), + with(RANDOM_REMINDER_PERIOD, ONE_HOUR)); - @Test - @Ignore - public void dueDateBeforeOverdue() { + service.scheduleAlarm(null, task); + InOrder order = inOrder(jobs); + order.verify(jobs).cancelReminder(1); + order.verify(jobs).add(new Reminder(1, now.plusMonths(12).getMillis(), ReminderService.TYPE_SNOOZE)); } } diff --git a/app/src/main/java/com/todoroo/astrid/reminders/ReminderService.java b/app/src/main/java/com/todoroo/astrid/reminders/ReminderService.java index e28e2c9c5..bd788206a 100644 --- a/app/src/main/java/com/todoroo/astrid/reminders/ReminderService.java +++ b/app/src/main/java/com/todoroo/astrid/reminders/ReminderService.java @@ -13,7 +13,6 @@ import com.todoroo.astrid.dao.TaskDao; import com.todoroo.astrid.dao.TaskDao.TaskCriteria; import com.todoroo.astrid.data.Task; -import org.tasks.R; import org.tasks.injection.ApplicationScope; import org.tasks.jobs.JobQueue; import org.tasks.jobs.Reminder; @@ -23,8 +22,6 @@ import org.tasks.time.DateTime; import javax.inject.Inject; -import static org.tasks.date.DateTimeUtils.newDateTime; - @ApplicationScope public final class ReminderService { @@ -73,20 +70,11 @@ public final class ReminderService { this.random = random; } - private static final int MILLIS_PER_HOUR = 60 * 60 * 1000; - public void scheduleAllAlarms(TaskDao taskDao) { - now = DateUtilities.now(); // Before mass scheduling, initialize now variable Query query = Query.select(NOTIFICATION_PROPERTIES).where(Criterion.and( TaskCriteria.isActive(), Criterion.or(Task.REMINDER_FLAGS.gt(0), Task.REMINDER_PERIOD.gt(0)))); taskDao.forEach(query, task -> scheduleAlarm(null, task)); - now = -1; // Signal done with now variable - } - - private long getNowValue() { - // If we're in the midst of mass scheduling, use the prestored now var - return (now == -1 ? DateUtilities.now() : now); } public void scheduleAlarm(TaskDao taskDao, Task task) { @@ -128,14 +116,6 @@ public final class ReminderService { // notifications after due date long whenOverdue = calculateNextOverdueReminder(task); - if (whenDueDate <= now) { - whenDueDate = now; - } - - if (whenOverdue <= now) { - whenOverdue = now; - } - // if random reminders are too close to due date, favor due date if(whenRandom != NO_ALARM && whenDueDate - whenRandom < DateUtilities.ONE_DAY) { whenRandom = NO_ALARM; @@ -160,41 +140,27 @@ public final class ReminderService { return NO_ALARM; } - /** - * Calculate the next alarm time for overdue reminders. - *

- * We schedule an alarm for after the due date (which could be in the past), - * with the exception that if a reminder was recently issued, we move - * the alarm time to the near future. - */ private long calculateNextOverdueReminder(Task task) { // Uses getNowValue() instead of DateUtilities.now() if(task.hasDueDate() && task.isNotifyAfterDeadline()) { - DateTime due = newDateTime(task.getDueDate()); + DateTime overdueDate = new DateTime(task.getDueDate()).plusDays(1); if (!task.hasDueTime()) { - due = due - .withHourOfDay(23) - .withMinuteOfHour(59) - .withSecondOfMinute(59); + overdueDate = overdueDate + .withMillisOfDay(preferences.getDefaultDueTime()); } - long dueDateForOverdue = due.getMillis(); - long lastReminder = task.getReminderLast(); - if(dueDateForOverdue > getNowValue()) { - return dueDateForOverdue + (long) ((0.5f + 2f * random.nextFloat()) * DateUtilities.ONE_HOUR); - } + DateTime lastReminder = new DateTime(task.getReminderLast()); - if(lastReminder < dueDateForOverdue) { - return getNowValue(); + if (overdueDate.isAfter(lastReminder)) { + return overdueDate.getMillis(); } - if(getNowValue() - lastReminder < 6 * DateUtilities.ONE_HOUR) { - return getNowValue() + (long) ((2.0f + - task.getImportance() + - 6f * random.nextFloat()) * DateUtilities.ONE_HOUR); - } + overdueDate = lastReminder + .withMillisOfDay(overdueDate.getMillisOfDay()); - return getNowValue(); + return overdueDate.isAfter(lastReminder) + ? overdueDate.getMillis() + : overdueDate.plusDays(1).getMillis(); } return NO_ALARM; } @@ -220,7 +186,7 @@ public final class ReminderService { dueDateAlarm = dueDate; } else { dueDateAlarm = new DateTime(dueDate) - .withMillisOfDay(preferences.getInt(R.string.p_rmd_time, 18 * MILLIS_PER_HOUR)) + .withMillisOfDay(preferences.getDefaultDueTime()) .getMillis(); } diff --git a/app/src/main/java/org/tasks/preferences/Preferences.java b/app/src/main/java/org/tasks/preferences/Preferences.java index 86715e0d2..c396cf74b 100644 --- a/app/src/main/java/org/tasks/preferences/Preferences.java +++ b/app/src/main/java/org/tasks/preferences/Preferences.java @@ -20,6 +20,7 @@ import org.tasks.injection.ForApplication; import org.tasks.time.DateTime; import java.io.File; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import javax.inject.Inject; @@ -90,6 +91,10 @@ public class Preferences { return getBoolean(R.string.p_rmd_enable_quiet, false); } + public int getDefaultDueTime() { + return getInt(R.string.p_rmd_time, (int) TimeUnit.HOURS.toMillis(18)); + } + public int getQuietHoursStart() { return getMillisPerDayPref(R.string.p_rmd_quietStart, R.integer.default_quiet_hours_start); }