refactor api.shouldDo and associated code; fix new bug in sanitizeOptions()

common/script/cron.js

@@ -22,8 +22,8 @@ export const DAY_MAPPING = {
   {now} is also passed in for various purposes, one example being the test scripts scripts testing different "now" times.
  */
 
-export function sanitizeOptions (o) {
+function sanitizeOptions (o) {
-  let ref = Number(o.dayStart);
+  let ref = Number(o.dayStart || 0);
   let dayStart = !_.isNaN(ref) && ref >= 0 && ref <= 24 ? ref : 0;
   let timezoneOffset = o.timezoneOffset ? Number(o.timezoneOffset) : Number(moment().zone());
   // TODO: check and clean timezoneOffset as for dayStart (e.g., might not be a number)
@@ -70,3 +70,41 @@ export function daysSince (yesterday, options = {}) {
   return startOfDay(_.defaults({ now: o.now }, o)).diff(startOfDay(_.defaults({ now: yesterday }, o)), 'days');
 }
 
+/*
+  Should the user do this task on this date, given the task's repeat options and user.preferences.dayStart?
+ */
+
+export function shouldDo (day, dailyTask, options = {}) {
+  if (dailyTask.type !== 'daily') {
+    return false;
+  }
+  let o = sanitizeOptions(options);
+  let startOfDayWithCDSTime = startOfDay(_.defaults({ now: day }, o));
+
+  // The time portion of the Start Date is never visible to or modifiable by the user so we must ignore it.
+  // Therefore, we must also ignore the time portion of the user's day start (startOfDayWithCDSTime), otherwise the date comparison will be wrong for some times.
+  // NB: The user's day start date has already been converted to the PREVIOUS day's date if the time portion was before CDS.
+  let taskStartDate = moment(dailyTask.startDate).zone(o.timezoneOffset);
+
+  taskStartDate = moment(taskStartDate).startOf('day');
+  if (taskStartDate > startOfDayWithCDSTime.startOf('day')) {
+    return false; // Daily starts in the future
+  }
+  if (dailyTask.frequency === 'daily') { // "Every X Days"
+    if (!dailyTask.everyX) {
+      return false; // error condition
+    }
+    let daysSinceTaskStart = startOfDayWithCDSTime.startOf('day').diff(taskStartDate, 'days');
+
+    return daysSinceTaskStart % dailyTask.everyX === 0;
+  } else if (dailyTask.frequency === 'weekly') { // "On Certain Days of the Week"
+    if (!dailyTask.repeat) {
+      return false; // error condition
+    }
+    let dayOfWeekNum = startOfDayWithCDSTime.day(); // e.g., 0 for Sunday
+
+    return dailyTask.repeat[DAY_MAPPING[dayOfWeekNum]];
+  } else {
+    return false; // error condition - unexpected frequency string
+  }
+}

common/script/index.js

@@ -1,8 +1,6 @@
 import {
-  DAY_MAPPING,     // temporary, pending further refactoring
-  sanitizeOptions, // temporary, pending further refactoring
-  startOfDay,      // temporary, pending further refactoring
   daysSince,
+  shouldDo,
 } from '../../common/script/cron';
 
 var $w, _, api, content, i18n, moment, preenHistory, sortOrder,
@@ -19,6 +17,7 @@ i18n = require('./i18n');
 api = module.exports = {};
 
 api.i18n = i18n;
+api.shouldDo = shouldDo;
 
 $w = api.$w = function(s) {
   return s.split(' ');
@@ -75,47 +74,6 @@ api.planGemLimits = {
   ------------------------------------------------------
  */
 
-/*
-  Should the user do this task on this date, given the task's repeat options and user.preferences.dayStart?
- */
-
-api.shouldDo = function(day, dailyTask, options) {
-  var dayOfWeekCheck, dayOfWeekNum, daysSinceTaskStart, everyXCheck, o, startOfDayWithCDSTime, taskStartDate;
-  if (options == null) {
-    options = {};
-  }
-  if (dailyTask.type !== 'daily') {
-    return false;
-  }
-  o = sanitizeOptions(options);
-  startOfDayWithCDSTime = startOfDay(_.defaults({
-    now: day
-  }, o));
-  taskStartDate = moment(dailyTask.startDate).zone(o.timezoneOffset);
-  taskStartDate = moment(taskStartDate).startOf('day');
-  if (taskStartDate > startOfDayWithCDSTime.startOf('day')) {
-    return false;
-  }
-  if (dailyTask.frequency === 'daily') {
-    if (!dailyTask.everyX) {
-      return false;
-    }
-    daysSinceTaskStart = startOfDayWithCDSTime.startOf('day').diff(taskStartDate, 'days');
-    everyXCheck = daysSinceTaskStart % dailyTask.everyX === 0;
-    return everyXCheck;
-  } else if (dailyTask.frequency === 'weekly') {
-    if (!dailyTask.repeat) {
-      return false;
-    }
-    dayOfWeekNum = startOfDayWithCDSTime.day();
-    dayOfWeekCheck = dailyTask.repeat[DAY_MAPPING[dayOfWeekNum]];
-    return dayOfWeekCheck;
-  } else {
-    return false;
-  }
-};
-
-
 /*
   ------------------------------------------------------
   Level cap
@@ -468,7 +426,7 @@ api.taskClasses = function(task, filters, dayStart, lastCron, showCompleted, mai
     classes += " beingEdited";
   }
   if (type === 'todo' || type === 'daily') {
-    if (completed || (type === 'daily' && !api.shouldDo(+(new Date), task, {
+    if (completed || (type === 'daily' && !shouldDo(+(new Date), task, {
       dayStart: dayStart
     }))) {
       classes += " completed";
@@ -2532,7 +2490,7 @@ api.wrap = function(user, main) {
           thatDay = moment(now).subtract({
             days: 1
           });
-          if (api.shouldDo(thatDay.toDate(), daily, user.preferences) || completed) {
+          if (shouldDo(thatDay.toDate(), daily, user.preferences) || completed) {
             _.each(daily.checklist, (function(box) {
               box.completed = false;
               return true;
@@ -2586,7 +2544,7 @@ api.wrap = function(user, main) {
             thatDay = moment(now).subtract({
               days: n + 1
             });
-            if (api.shouldDo(thatDay.toDate(), task, user.preferences)) {
+            if (shouldDo(thatDay.toDate(), task, user.preferences)) {
               scheduleMisses++;
               if (user.stats.buffs.stealth) {
                 user.stats.buffs.stealth--;