From 0ba47de43d940cf417188aa80416ecf36bf666d2 Mon Sep 17 00:00:00 2001 From: Alys Date: Sat, 2 Apr 2016 11:42:15 +1000 Subject: [PATCH] adjust cron so that it prevents a double cron when timezone decreases (timezoneOffset increases) * start to fix cron for when timezones change * add some incomplete code * change cron to identify when timezone changes and run only if both zones agree it should run - BAD - does cron an hour early one day later * change timezone offset variable names * improve handling of each individual timezone change possibility * remove unnecessary casting to number * remove duplicated code * finish code for dangerous timezone change * fix linting errors * remove console.log lines and other testing code * add a TODO comment for further actions for timezone changes in the "safe" direction --- common/script/cron.js | 17 +++++- common/script/fns/cron.js | 84 ++++++++++++++++++++++++-- common/script/public/userServices.js | 3 +- website/src/controllers/api-v2/user.js | 2 +- 4 files changed, 98 insertions(+), 8 deletions(-) diff --git a/common/script/cron.js b/common/script/cron.js index 639bbf6b65..2c0760fcc9 100644 --- a/common/script/cron.js +++ b/common/script/cron.js @@ -25,8 +25,21 @@ export const DAY_MAPPING = { function sanitizeOptions (o) { 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) + + let timezoneOffset; + let timezoneOffsetDefault = Number(moment().zone()); + if (_.isFinite(o.timezoneOffsetOverride)) { + timezoneOffset = Number(o.timezoneOffsetOverride); + } else if (_.isFinite(o.timezoneOffset)) { + timezoneOffset = Number(o.timezoneOffset); + } else { + timezoneOffset = timezoneOffsetDefault; + } + if (timezoneOffset > 720 || timezoneOffset < -840) { + // timezones range from -12 (offset +720) to +14 (offset -840) + timezoneOffset = timezoneOffsetDefault; + } + let now = o.now ? moment(o.now).zone(timezoneOffset) : moment().zone(timezoneOffset); // return a new object, we don't want to add "now" to user object diff --git a/common/script/fns/cron.js b/common/script/fns/cron.js index 4cd7387271..1e3470d7f9 100644 --- a/common/script/fns/cron.js +++ b/common/script/fns/cron.js @@ -23,22 +23,98 @@ import { */ module.exports = function(user, options) { - var _progress, analyticsData, base, base1, base2, base3, base4, clearBuffs, dailyChecked, dailyDueUnchecked, daysMissed, expTally, lvl, lvlDiv2, multiDaysCountAsOneDay, now, perfect, plan, progress, ref, ref1, ref2, ref3, todoTally; + var _progress, analyticsData, base, base1, base2, base3, base4, clearBuffs, dailyChecked, dailyDueUnchecked, daysMissed, expTally, lvl, lvlDiv2, multiDaysCountAsOneDay, now, perfect, plan, progress, ref, ref1, ref2, ref3, todoTally, timezoneOffsetFromUserPrefs, timezoneOffsetFromBrowser, timezoneOffsetAtLastCron; if (options == null) { options = {}; } now = +options.now || +(new Date); + + // If the user's timezone has changed (due to travel or daylight savings), + // cron can be triggered twice in one day, so we check for that and use + // both timezones to work out if cron should run. + // CDS = Custom Day Start time. + timezoneOffsetFromUserPrefs = user.preferences.timezoneOffset || 0; + timezoneOffsetAtLastCron = (_.isFinite(user.preferences.timezoneOffsetAtLastCron)) ? user.preferences.timezoneOffsetAtLastCron : timezoneOffsetFromUserPrefs; + timezoneOffsetFromBrowser = (_.isFinite(+options.timezoneOffset)) ? +options.timezoneOffset : timezoneOffsetFromUserPrefs; + // NB: All timezone offsets can be 0, so can't use `... || ...` to apply non-zero defaults + + if (timezoneOffsetFromBrowser !== timezoneOffsetFromUserPrefs) { + // The user's browser has just told Habitica that the user's timezone has + // changed so store and use the new zone. + user.preferences.timezoneOffset = timezoneOffsetFromBrowser; + timezoneOffsetFromUserPrefs = timezoneOffsetFromBrowser; + } + + // How many days have we missed using the user's current timezone: daysMissed = daysSince(user.lastCron, _.defaults({ now: now }, user.preferences)); + + if (timezoneOffsetAtLastCron != timezoneOffsetFromUserPrefs) { + // Since cron last ran, the user's timezone has changed. + // How many days have we missed using the old timezone: + let daysMissedNewZone = daysMissed; + let daysMissedOldZone = daysSince(user.lastCron, _.defaults({ + now: now, + timezoneOffsetOverride: timezoneOffsetAtLastCron, + }, user.preferences)); + + if (timezoneOffsetAtLastCron < timezoneOffsetFromUserPrefs) { + // The timezone change was in the unsafe direction. + // E.g., timezone changes from UTC+1 (offset -60) to UTC+0 (offset 0). + // or timezone changes from UTC-4 (offset 240) to UTC-5 (offset 300). + // Local time changed from, for example, 03:00 to 02:00. + + if (daysMissedOldZone > 0 && daysMissedNewZone > 0) { + // Both old and new timezones indicate that we SHOULD run cron, so + // it is safe to do so immediately. + daysMissed = Math.min(daysMissedOldZone, daysMissedNewZone); + // use minimum value to be nice to user + } + else if (daysMissedOldZone > 0) { + // The old timezone says that cron should run; the new timezone does not. + // This should be impossible for this direction of timezone change, but + // just in case I'm wrong... + console.log("zone has changed - old zone says run cron, NEW zone says no - stop cron now only -- SHOULD NOT HAVE GOT TO HERE", timezoneOffsetAtLastCron, timezoneOffsetFromUserPrefs, now); // used in production for confirming this never happens + } + else if (daysMissedNewZone > 0) { + // The old timezone says that cron should NOT run -- i.e., cron has + // already run today, from the old timezone's point of view. + // The new timezone says that cron SHOULD run, but this is almost + // certainly incorrect. + // This happens when cron occurred at a time soon after the CDS. When + // you reinterpret that time in the new timezone, it looks like it + // was before the CDS, because local time has stepped backwards. + // To fix this, rewrite the cron time to a time that the new + // timezone interprets as being in today. + + daysMissed = 0; // prevent cron running now + let timezoneOffsetDiff = timezoneOffsetAtLastCron - timezoneOffsetFromUserPrefs; + // e.g., for dangerous zone change: 240 - 300 = -60 or -660 - -600 = -60 + + user.lastCron = moment(user.lastCron).subtract(timezoneOffsetDiff, 'minutes'); + // NB: We don't change user.auth.timestamps.loggedin so that will still record the time that the previous cron actually ran. + // From now on we can ignore the old timezone: + user.preferences.timezoneOffsetAtLastCron = timezoneOffsetFromUserPrefs; + } + else { + // Both old and new timezones indicate that cron should + // NOT run. + daysMissed = 0; // prevent cron running now + } + } + else if (timezoneOffsetAtLastCron > timezoneOffsetFromUserPrefs) { + daysMissed = daysMissedNewZone; + // TODO: Either confirm that there is nothing that could possibly go wrong here and remove the need for this else branch, or fix stuff. There are probably situations where the Dailies do not reset early enough for a user who was expecting the zone change and wants to use all their Dailies immediately in the new zone; if so, we should provide an option for easy reset of Dailies (can't be automatic because there will be other situations where the user was not prepared). + } + } + if (!(daysMissed > 0)) { return; } user.auth.timestamps.loggedin = new Date(); user.lastCron = now; - if (_.isFinite(+user.preferences.timezoneOffset)) { - user.preferences.timezoneOffsetAtLastCron = user.preferences.timezoneOffset; - } + user.preferences.timezoneOffsetAtLastCron = timezoneOffsetFromUserPrefs; if (user.items.lastDrop.count > 0) { user.items.lastDrop.count = 0; } diff --git a/common/script/public/userServices.js b/common/script/public/userServices.js index ba02382fc4..e5b1790086 100644 --- a/common/script/public/userServices.js +++ b/common/script/public/userServices.js @@ -170,8 +170,10 @@ angular.module('habitrpg') authenticate: function (uuid, token, cb) { if (!!uuid && !!token) { + var offset = moment().zone(); // eg, 240 - this will be converted on server as -(offset/60) $http.defaults.headers.common['x-api-user'] = uuid; $http.defaults.headers.common['x-api-key'] = token; + $http.defaults.headers.common['x-user-timezoneOffset'] = offset; authenticated = true; settings.auth.apiId = uuid; settings.auth.apiToken = token; @@ -179,7 +181,6 @@ angular.module('habitrpg') if (user && user._v) user._v--; // shortcut to always fetch new updates on page reload userServices.log({}, function(){ // If they don't have timezone, set it - var offset = moment().zone(); // eg, 240 - this will be converted on server as -(offset/60) if (user.preferences.timezoneOffset !== offset) userServices.set({'preferences.timezoneOffset': offset}); cb && cb(); diff --git a/website/src/controllers/api-v2/user.js b/website/src/controllers/api-v2/user.js index 88942d9f60..8084edc53b 100644 --- a/website/src/controllers/api-v2/user.js +++ b/website/src/controllers/api-v2/user.js @@ -373,7 +373,7 @@ api.update = (req, res, next) => { api.cron = function(req, res, next) { var user = res.locals.user, - progress = user.fns.cron({analytics:utils.analytics}), + progress = user.fns.cron({analytics:utils.analytics, timezoneOffset:req.headers['x-user-timezoneoffset']}), ranCron = user.isModified(), quest = shared.content.quests[user.party.quest.key];