From bacd35b89503c87904689e8ea411b3220d8fc73c Mon Sep 17 00:00:00 2001 From: Alys Date: Wed, 24 Jun 2015 09:06:53 +1000 Subject: [PATCH 1/8] refactor shouldDo check for Daily's startDate being in the future --- common/script/index.coffee | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index 45ed81d3e8..d99a1e57ad 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -84,16 +84,16 @@ api.shouldDo = (day, dailyTask, options = {}) -> day = api.startOfDay(_.defaults {now:day}, o) dayOfWeekNum = day.day() # e.g. 1 for Monday if week starts on Mon - # check if event is today or in the future - hasStartedCheck = day >= api.startOfDay(_.defaults {now:dailyTask.startDate}, o) + if day < api.startOfDay(_.defaults {now:dailyTask.startDate}, o) + return false # Daily starts in the future if dailyTask.frequency == 'daily' daysSinceTaskStart = api.numDaysApart(day.startOf('day'), dailyTask.startDate, o) everyXCheck = (daysSinceTaskStart % dailyTask.everyX == 0) - return everyXCheck && hasStartedCheck + return everyXCheck else if dailyTask.frequency == 'weekly' dayOfWeekCheck = dailyTask.repeat[api.dayMapping[dayOfWeekNum]] - return dayOfWeekCheck && hasStartedCheck + return dayOfWeekCheck else # unexpected frequency string return false From f6cdc572280c9df4e5f7455db1a87e2d381bdc81 Mon Sep 17 00:00:00 2001 From: Alys Date: Wed, 24 Jun 2015 13:58:45 +1000 Subject: [PATCH 2/8] move dailyTask.repeat error handling to correct location; add dailyTask.everyX error handling (might not be needed) --- common/script/index.coffee | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index d99a1e57ad..f229ae6de6 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -75,7 +75,7 @@ api.daysSince = (yesterday, options = {}) -> Should the user do this task on this date, given the task's repeat options and user.preferences.dayStart? ### api.shouldDo = (day, dailyTask, options = {}) -> - return false unless dailyTask.type == 'daily' && dailyTask.repeat + return false unless dailyTask.type == 'daily' if !dailyTask.startDate dailyTask.startDate = moment().toDate() if dailyTask.startDate instanceof String @@ -88,10 +88,14 @@ api.shouldDo = (day, dailyTask, options = {}) -> return false # Daily starts in the future if dailyTask.frequency == 'daily' + if !dailyTask.everyX + return false # error condition daysSinceTaskStart = api.numDaysApart(day.startOf('day'), dailyTask.startDate, o) everyXCheck = (daysSinceTaskStart % dailyTask.everyX == 0) return everyXCheck else if dailyTask.frequency == 'weekly' + if !dailyTask.repeat + return false # error condition dayOfWeekCheck = dailyTask.repeat[api.dayMapping[dayOfWeekNum]] return dayOfWeekCheck else From 99bc09fb2528982cc0b47903335325951ca32112 Mon Sep 17 00:00:00 2001 From: Alys Date: Wed, 24 Jun 2015 14:18:16 +1000 Subject: [PATCH 3/8] move dayOfWeekNum to correct scope and improve comment --- common/script/index.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index f229ae6de6..61649f3d0f 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -82,7 +82,6 @@ api.shouldDo = (day, dailyTask, options = {}) -> dailyTask.startDate = moment(dailyTask.startDate).toDate() o = sanitizeOptions options day = api.startOfDay(_.defaults {now:day}, o) - dayOfWeekNum = day.day() # e.g. 1 for Monday if week starts on Mon if day < api.startOfDay(_.defaults {now:dailyTask.startDate}, o) return false # Daily starts in the future @@ -96,6 +95,7 @@ api.shouldDo = (day, dailyTask, options = {}) -> else if dailyTask.frequency == 'weekly' if !dailyTask.repeat return false # error condition + dayOfWeekNum = day.day() # e.g. 0 for Sunday dayOfWeekCheck = dailyTask.repeat[api.dayMapping[dayOfWeekNum]] return dayOfWeekCheck else From 266055c0d4f09667c8c87dc2c974247e125307d7 Mon Sep 17 00:00:00 2001 From: Alys Date: Wed, 24 Jun 2015 14:28:55 +1000 Subject: [PATCH 4/8] rename 'day' variable to 'startOfDayWithCDSTime' (more verbose, sorry, but clearer) --- common/script/index.coffee | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index 61649f3d0f..3917e643b1 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -81,21 +81,21 @@ api.shouldDo = (day, dailyTask, options = {}) -> if dailyTask.startDate instanceof String dailyTask.startDate = moment(dailyTask.startDate).toDate() o = sanitizeOptions options - day = api.startOfDay(_.defaults {now:day}, o) + startOfDayWithCDSTime = api.startOfDay(_.defaults {now:day}, o) - if day < api.startOfDay(_.defaults {now:dailyTask.startDate}, o) + if startOfDayWithCDSTime < api.startOfDay(_.defaults {now:dailyTask.startDate}, o) return false # Daily starts in the future if dailyTask.frequency == 'daily' if !dailyTask.everyX return false # error condition - daysSinceTaskStart = api.numDaysApart(day.startOf('day'), dailyTask.startDate, o) + daysSinceTaskStart = api.numDaysApart(startOfDayWithCDSTime.startOf('day'), dailyTask.startDate, o) everyXCheck = (daysSinceTaskStart % dailyTask.everyX == 0) return everyXCheck else if dailyTask.frequency == 'weekly' if !dailyTask.repeat return false # error condition - dayOfWeekNum = day.day() # e.g. 0 for Sunday + dayOfWeekNum = startOfDayWithCDSTime.day() # e.g. 0 for Sunday dayOfWeekCheck = dailyTask.repeat[api.dayMapping[dayOfWeekNum]] return dayOfWeekCheck else From e7e82a2aed2dcff21947941a7f9f065c6ab3e47c Mon Sep 17 00:00:00 2001 From: Alys Date: Wed, 24 Jun 2015 16:55:07 +1000 Subject: [PATCH 5/8] keep startDate as a moment(), fix check for Start Date in future (ignore time, don't do incorrect CDS conversion), add comments --- common/script/index.coffee | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index 3917e643b1..6e8d5f4e04 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -56,6 +56,10 @@ api.startOfWeek = api.startOfWeek = (options={}) -> moment(o.now).startOf('week') api.startOfDay = (options={}) -> + # This is designed for use with any date that has an important time portion (e.g., when comparing the current date-time with the previous cron's date-time for determing if cron should run now). + # It changes the time portion of the date-time to be the Custom Day Start hour, so that the date-time is now the user's correct start of day. + # It SUBTRACTS a day if the date-time's original hour is before CDS (e.g., if your CDS is 5am and it's currently 4am, it's still the previous day). + # This is NOT suitable for manipulating any dates that are displayed to the user as a date with no time portion, such as a Daily's Start Dates (e.g., a Start Date of today shows only the date, so it should be considered to be today even if the hidden time portion is before CDS). o = sanitizeOptions(options) dayStart = moment(o.now).startOf('day').add({hours:o.dayStart}) if moment(o.now).hour() < o.dayStart @@ -77,22 +81,26 @@ api.daysSince = (yesterday, options = {}) -> api.shouldDo = (day, dailyTask, options = {}) -> return false unless dailyTask.type == 'daily' if !dailyTask.startDate - dailyTask.startDate = moment().toDate() - if dailyTask.startDate instanceof String - dailyTask.startDate = moment(dailyTask.startDate).toDate() - o = sanitizeOptions options - startOfDayWithCDSTime = api.startOfDay(_.defaults {now:day}, o) + dailyTask.startDate = moment() + # The time portion of the Start Date is never visible to or modifiable by the user so we must ignore it: + dailyTask.startDate = moment(dailyTask.startDate).startOf('day'); - if startOfDayWithCDSTime < api.startOfDay(_.defaults {now:dailyTask.startDate}, o) + o = sanitizeOptions options + startOfDayWithCDSTime = api.startOfDay(_.defaults {now:day}, o) # a moment() + + # Work out if the Daily's Start Date is in the future. + # Since we are ignoring the time portion of Start Date, we must also ignore the time portion of the user's day start. + # (NB: The user's day start date has already been converted to the PREVIOUS day's date if the time portion was before CDS.) + if dailyTask.startDate > startOfDayWithCDSTime.startOf('day') return false # Daily starts in the future - if dailyTask.frequency == 'daily' + if dailyTask.frequency == 'daily' # "Every X Days" if !dailyTask.everyX return false # error condition daysSinceTaskStart = api.numDaysApart(startOfDayWithCDSTime.startOf('day'), dailyTask.startDate, o) everyXCheck = (daysSinceTaskStart % dailyTask.everyX == 0) return everyXCheck - else if dailyTask.frequency == 'weekly' + else if dailyTask.frequency == 'weekly' # "On Certain Days of the Week" if !dailyTask.repeat return false # error condition dayOfWeekNum = startOfDayWithCDSTime.day() # e.g. 0 for Sunday From 9b8b7af7d567363d64eb36a8725c074a44cc2762 Mon Sep 17 00:00:00 2001 From: Alys Date: Wed, 24 Jun 2015 17:32:08 +1000 Subject: [PATCH 6/8] replace api.numDaysApart (not used elsewhere and new to the EveryXDays code) with moment#diff --- common/script/index.coffee | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index 6e8d5f4e04..6033e61114 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -97,7 +97,7 @@ api.shouldDo = (day, dailyTask, options = {}) -> if dailyTask.frequency == 'daily' # "Every X Days" if !dailyTask.everyX return false # error condition - daysSinceTaskStart = api.numDaysApart(startOfDayWithCDSTime.startOf('day'), dailyTask.startDate, o) + daysSinceTaskStart = startOfDayWithCDSTime.startOf('day').diff(dailyTask.startDate, 'days') everyXCheck = (daysSinceTaskStart % dailyTask.everyX == 0) return everyXCheck else if dailyTask.frequency == 'weekly' # "On Certain Days of the Week" @@ -110,12 +110,6 @@ api.shouldDo = (day, dailyTask, options = {}) -> # unexpected frequency string return false -api.numDaysApart = (day1, day2, o) -> - startOfDay1 = api.startOfDay(_.defaults {now:day1}, o) - startOfDay2 = api.startOfDay(_.defaults {now:day2}, o) - numDays = Math.abs(startOfDay1.diff(startOfDay2, 'days')) - return numDays - ### ------------------------------------------------------ Level cap From 51fb2784382aa7e9ab458ede8c0c86bd7f8fb37a Mon Sep 17 00:00:00 2001 From: Alys Date: Thu, 25 Jun 2015 11:47:00 +1000 Subject: [PATCH 7/8] stop changing the task's actual Start Date, move related task Start Date code tothe same place, adjust comments --- common/script/index.coffee | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/common/script/index.coffee b/common/script/index.coffee index 6033e61114..e0cb9dcb98 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -80,24 +80,21 @@ api.daysSince = (yesterday, options = {}) -> ### api.shouldDo = (day, dailyTask, options = {}) -> return false unless dailyTask.type == 'daily' - if !dailyTask.startDate - dailyTask.startDate = moment() - # The time portion of the Start Date is never visible to or modifiable by the user so we must ignore it: - dailyTask.startDate = moment(dailyTask.startDate).startOf('day'); - o = sanitizeOptions options startOfDayWithCDSTime = api.startOfDay(_.defaults {now:day}, o) # a moment() - # Work out if the Daily's Start Date is in the future. - # Since we are ignoring the time portion of Start Date, we must also ignore the time portion of the user's day start. - # (NB: The user's day start date has already been converted to the PREVIOUS day's date if the time portion was before CDS.) - if dailyTask.startDate > startOfDayWithCDSTime.startOf('day') + # Work out if the Daily's Start Date (taskStartDate) is in the future. + # 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. + taskStartDate = moment(dailyTask.startDate || now()).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 - daysSinceTaskStart = startOfDayWithCDSTime.startOf('day').diff(dailyTask.startDate, 'days') + daysSinceTaskStart = startOfDayWithCDSTime.startOf('day').diff(taskStartDate, 'days') everyXCheck = (daysSinceTaskStart % dailyTask.everyX == 0) return everyXCheck else if dailyTask.frequency == 'weekly' # "On Certain Days of the Week" From d7a203ccc266878b0a933a7dbc2da32291c50c90 Mon Sep 17 00:00:00 2001 From: Alys Date: Sat, 27 Jun 2015 19:14:52 +1000 Subject: [PATCH 8/8] set all daily repeat code and tests to use true/false instead of 1/0 for consistency with DailySchema; comment-out unused default Daily repeat code --- common/script/content.coffee | 2 +- common/script/index.coffee | 2 +- test/common/algos.mocha.coffee | 12 ++++++------ test/common/dailies.coffee | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/script/content.coffee b/common/script/content.coffee index 9d2333f68d..69c7b67eaf 100644 --- a/common/script/content.coffee +++ b/common/script/content.coffee @@ -1919,7 +1919,7 @@ api.subscriptionBlocks = basic_12mo: months:12, price:48 _.each api.subscriptionBlocks, (b,k)->b.key = k -repeat = {m:true,t:true,w:true,th:true,f:true,s:true,su:true} +# repeat = {m:true,t:true,w:true,th:true,f:true,s:true,su:true} api.userDefaults = habits: [ {type: 'habit', text: t('defaultHabit1Text'), value: 0, up: true, down: false, attribute: 'per' } diff --git a/common/script/index.coffee b/common/script/index.coffee index e0cb9dcb98..065971ceb4 100644 --- a/common/script/index.coffee +++ b/common/script/index.coffee @@ -238,7 +238,7 @@ api.taskDefaults = (task={}) -> _.defaults(task, {up:true,down:true}) if task.type is 'habit' _.defaults(task, {history: []}) if task.type in ['habit', 'daily'] _.defaults(task, {completed:false}) if task.type in ['daily', 'todo'] - _.defaults(task, {streak:0, repeat: {su:1,m:1,t:1,w:1,th:1,f:1,s:1}}, startDate: new Date(), everyX: 1, frequency: 'weekly') if task.type is 'daily' + _.defaults(task, {streak:0, repeat: {su:true,m:true,t:true,w:true,th:true,f:true,s:true}}, startDate: new Date(), everyX: 1, frequency: 'weekly') if task.type is 'daily' task._id = task.id # may need this for TaskSchema if we go back to using it, see http://goo.gl/a5irq4 task.value ?= if task.type is 'reward' then 10 else 0 task.priority = 1 unless _.isNumber(task.priority) # hotfix for apiv1. once we're off apiv1, we can remove this diff --git a/test/common/algos.mocha.coffee b/test/common/algos.mocha.coffee index 9c0422cad9..64bda3ccf4 100644 --- a/test/common/algos.mocha.coffee +++ b/test/common/algos.mocha.coffee @@ -127,7 +127,7 @@ cycle = (array)-> return array[n % array.length] repeatWithoutLastWeekday = ()-> - repeat = {su:1,m:1,t:1,w:1,th:1,f:1,s:1} + repeat = {su:true,m:true,t:true,w:true,th:true,f:true,s:true} if shared.startOfWeek(moment().zone(0)).isoWeekday() == 1 # Monday repeat.su = false else @@ -178,7 +178,7 @@ describe 'User', -> # Handle greyed-out dailys yesterday = moment().subtract(1,'days') - user.dailys[0].repeat[shared.dayMapping[yesterday.day()]] = 0 + user.dailys[0].repeat[shared.dayMapping[yesterday.day()]] = false _.each user.dailys[1..], (d)->d.completed = true cron() expect(user.stats.buffs.str).to.be 1 @@ -251,7 +251,7 @@ describe 'User', -> it 'does not reset checklist on grey incomplete dailies', -> yesterday = moment().subtract(1,'days') - user.dailys[0].repeat[shared.dayMapping[yesterday.day()]] = 0 + user.dailys[0].repeat[shared.dayMapping[yesterday.day()]] = false user.dailys[0].checklist = [ { "text" : "1", @@ -276,7 +276,7 @@ describe 'User', -> it 'resets checklist on complete grey complete dailies', -> yesterday = moment().subtract(1,'days') - user.dailys[0].repeat[shared.dayMapping[yesterday.day()]] = 0 + user.dailys[0].repeat[shared.dayMapping[yesterday.day()]] = false user.dailys[0].checklist = [ { "text" : "1", @@ -905,7 +905,7 @@ describe 'Cron', -> 'due today': # NOTE: a strange thing here, moment().startOf('week') is Sunday, but moment.zone(myTimeZone).startOf('week') is Monday. - defaults: {repeat:{su:1,m:true,t:1,w:1,th:1,f:1,s:1}} + defaults: {repeat:{su:true,m:true,t:true,w:true,th:true,f:true,s:true}} steps: 'pre-dayStart': defaults: {currentHour:3, dayStart:4, shouldDo:true} @@ -919,7 +919,7 @@ describe 'Cron', -> 'unchecked': {checked:false, expect: 'losePoints'} 'NOT due today': - defaults: {repeat:{su:1,m:false,t:1,w:1,th:1,f:1,s:1}} + defaults: {repeat:{su:true,m:false,t:true,w:true,th:true,f:true,s:true}} steps: 'pre-dayStart': defaults: {currentHour:3, dayStart:4, shouldDo:true} diff --git a/test/common/dailies.coffee b/test/common/dailies.coffee index d26c8d012e..fc88adfbf3 100644 --- a/test/common/dailies.coffee +++ b/test/common/dailies.coffee @@ -6,7 +6,7 @@ shared = require '../../common/script/index.coffee' shared.i18n.translations = require('../../website/src/i18n.js').translations repeatWithoutLastWeekday = ()-> - repeat = {su:1,m:1,t:1,w:1,th:1,f:1,s:1} + repeat = {su:true,m:true,t:true,w:true,th:true,f:true,s:true} if shared.startOfWeek(moment().zone(0)).isoWeekday() == 1 # Monday repeat.su = false else @@ -64,7 +64,7 @@ describe 'daily/weekly that repeats everyday (default)', -> user = newUser() user.dailys = [ shared.taskDefaults({type:'daily', startDate: moment().add(7, 'days'), frequency: 'daily'}) - shared.taskDefaults({type:'daily', startDate: moment().add(7, 'days'), frequency: 'weekly', repeat: {su:1,m:1,t:1,w:1,th:1,f:1,s:1}}) + shared.taskDefaults({type:'daily', startDate: moment().add(7, 'days'), frequency: 'weekly', repeat: {su:true,m:true,t:true,w:true,th:true,f:true,s:true}}) ] daily = user.dailys[0] weekly = user.dailys[1]