fix cron tests

test/api/v3/unit/libs/cron.test.js

@@ -34,15 +34,6 @@ describe('cron', () => {
     };
   });
 
-  it('updates user.auth.timestamps.loggedin and lastCron', () => {
-    let now = new Date();
-
-    cron({user, tasksByType, daysMissed, analytics, now});
-
-    expect(user.auth.timestamps.loggedin).to.equal(now);
-    expect(user.lastCron).to.equal(now);
-  });
-
   it('updates user.preferences.timezoneOffsetAtLastCron', () => {
     let timezoneOffsetFromUserPrefs = 1;
 

test/api/v3/unit/middlewares/cronMiddleware.js

@@ -1,7 +1,6 @@
 import {
   generateRes,
   generateReq,
-  generateNext,
   generateTodo,
   generateDaily,
 } from '../../../../helpers/api-unit.helper';
@@ -14,13 +13,12 @@ import analyticsService from '../../../../../website/server/libs/api-v3/analytic
 import { v4 as generateUUID } from 'uuid';
 
 describe('cron middleware', () => {
-  let res, req, next;
+  let res, req;
   let user;
 
-  beforeEach(() => {
+  beforeEach((done) => {
     res = generateRes();
     req = generateReq();
-    next = generateNext();
     user = new User({
       auth: {
         local: {
@@ -33,24 +31,27 @@ describe('cron middleware', () => {
       },
     });
 
-    user._statsComputed = {
+    user.save()
-      mp: 10,
+    .then(savedUser => {
-      maxMP: 100,
+      savedUser._statsComputed = {
-    };
+        mp: 10,
+        maxMP: 100,
+      };
 
-    res.locals.user = user;
+      res.locals.user = savedUser;
-    res.analytics = analyticsService;
+      res.analytics = analyticsService;
+      done();
+    })
+    .catch(done);
   });
 
-  it('calls next when user is not attached', () => {
+  it('calls next when user is not attached', (done) => {
     res.locals.user = null;
-    cronMiddleware(req, res, next);
+    cronMiddleware(req, res, (err) => done(err));
-    expect(next).to.be.calledOnce;
   });
 
-  it('calls next when days have not been missed', () => {
+  it('calls next when days have not been missed', (done) => {
-    cronMiddleware(req, res, next);
+    cronMiddleware(req, res, (err) => done(err));
-    expect(next).to.be.calledOnce;
   });
 
   it('should clear todos older than 30 days for free users', async (done) => {
@@ -59,35 +60,37 @@ describe('cron middleware', () => {
     task.dateCompleted = moment(new Date()).subtract({days: 31});
     task.completed = true;
     await task.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
+    cronMiddleware(req, res, (err) => {
-      Tasks.Task.findOne({_id: task}, function (err, taskFound) {
+      Tasks.Task.findOne({_id: task}, function (secondErr, taskFound) {
-        expect(err).to.not.exist;
+        expect(secondErr).to.not.exist;
         expect(taskFound).to.not.exist;
-        done();
+        done(err);
       });
     });
   });
 
-  it('should not clear todos older than 30 days for subscribed users', (done) => {
+  it('should not clear todos older than 30 days for subscribed users', async (done) => {
     user.purchased.plan.customerId = 'subscribedId';
     user.purchased.plan.dateUpdated = moment('012013', 'MMYYYY');
     user.lastCron = moment(new Date()).subtract({days: 2});
     let task = generateTodo(user);
     task.dateCompleted = moment(new Date()).subtract({days: 31});
     task.completed = true;
-    task.save();
+    await task.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
+    cronMiddleware(req, res, (err) => {
-      Tasks.Task.findOne({_id: task}, function (err, taskFound) {
+      Tasks.Task.findOne({_id: task}, function (secondErr, taskFound) {
-        expect(err).to.not.exist;
+        expect(secondErr).to.not.exist;
         expect(taskFound).to.exist;
-        done();
+        done(err);
       });
     });
   });
 
-  it('should clear todos older than 90 days for subscribed users', (done) => {
+  it('should clear todos older than 90 days for subscribed users', async (done) => {
     user.purchased.plan.customerId = 'subscribedId';
     user.purchased.plan.dateUpdated = moment('012013', 'MMYYYY');
     user.lastCron = moment(new Date()).subtract({days: 2});
@@ -95,46 +98,60 @@ describe('cron middleware', () => {
     let task = generateTodo(user);
     task.dateCompleted = moment(new Date()).subtract({days: 91});
     task.completed = true;
-    task.save();
+    await task.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
+    cronMiddleware(req, res, (err) => {
-      Tasks.Task.findOne({_id: task}, function (err, taskFound) {
+      Tasks.Task.findOne({_id: task}, function (secondErr, taskFound) {
-        expect(err).to.not.exist;
+        expect(secondErr).to.not.exist;
         expect(taskFound).to.not.exist;
-        done();
+        done(err);
       });
     });
   });
 
-  it('should call next is user was not modified after cron', (done) => {
+  it('should call next if user was not modified after cron', async (done) => {
     let hpBefore = user.stats.hp;
     user.lastCron = moment(new Date()).subtract({days: 2});
+    await user.save();
 
-    user.save().then(function () {
+    cronMiddleware(req, res, (err) => {
-      cronMiddleware(req, res, function () {
+      expect(hpBefore).to.equal(user.stats.hp);
-        expect(hpBefore).to.equal(user.stats.hp);
+      done(err);
-        done();
-      });
     });
   });
 
-  it('does damage for missing dailies', (done) => {
+  it('updates user.auth.timestamps.loggedin and lastCron', async (done) => {
+    user.lastCron = moment(new Date()).subtract({days: 2});
+    let now = new Date();
+    await user.save();
+
+    cronMiddleware(req, res, (err) => {
+      expect(moment(now).isSame(user.lastCron, 'day'));
+      expect(moment(now).isSame(user.auth.timestamps.loggedin, 'day'));
+      done(err);
+    });
+  });
+
+  it('does damage for missing dailies', async (done) => {
     let hpBefore = user.stats.hp;
     user.lastCron = moment(new Date()).subtract({days: 2});
     let daily = generateDaily(user);
     daily.startDate = moment(new Date()).subtract({days: 2});
-    daily.save();
+    await daily.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
+    cronMiddleware(req, res, (err) => {
       expect(user.stats.hp).to.be.lessThan(hpBefore);
-      done();
+      done(err);
     });
   });
 
-  it('updates tasks', (done) => {
+  it('updates tasks', async (done) => {
     user.lastCron = moment(new Date()).subtract({days: 2});
     let todo = generateTodo(user);
     let todoValueBefore = todo.value;
+    await user.save();
 
     cronMiddleware(req, res, () => {
       Tasks.Task.findOne({_id: todo._id}, function (err, todoFound) {
@@ -150,7 +167,7 @@ describe('cron middleware', () => {
     user.lastCron = moment(new Date()).subtract({days: 2});
     let daily = generateDaily(user);
     daily.startDate = moment(new Date()).subtract({days: 2});
-    daily.save();
+    await daily.save();
 
     let questKey = 'dilatory';
     user.party.quest.key = questKey;

website/server/middlewares/api-v3/cron.js

@@ -10,44 +10,36 @@ import { v4 as uuid } from 'uuid';
 
 const daysSince = common.daysSince;
 
-async function recoverCron (status, req, res, next) {
+async function recoverCron (status, req, res) {
-  try {
+  let user = res.locals.user;
-    let user = res.locals.user;
 
-    await Bluebird.delay(300);
+  await Bluebird.delay(300);
-    let reloadedUser = await User.findOne({_id: user._id}).exec();
+  let reloadedUser = await User.findOne({_id: user._id}).exec();
 
-    if (!reloadedUser) {
+  if (!reloadedUser) {
-      throw new Error(`User ${user._id} not found while recovering.`);
+    throw new Error(`User ${user._id} not found while recovering.`);
-    } else if (reloadedUser._cronSignature !== 'NOT_RUNNING') {
+  } else if (reloadedUser._cronSignature !== 'NOT_RUNNING') {
-      console.log((new Date()).toISOString(), 'RECOVERED FROM CRON - STILL RUNNING', req.originalUrl, req.method);
+    status.times++;
-      status.times++;
+
-      if (status.times < 4) {
+    if (status.times < 4) {
-        await recoverCron(status, req, res, next);
+      await recoverCron(status, req, res);
-      } else {
-        throw new Error(`Impossible to recover from cron for user ${user._id}.`);
-      }
     } else {
-      console.log((new Date()).toISOString(), 'RECOVERED FROM CRON', req.originalUrl, req.method);
+      throw new Error(`Impossible to recover from cron for user ${user._id}.`);
-      res.locals.user = reloadedUser;
-      return next();
     }
-  } catch (err) {
+  } else {
-    return next(err);
+    res.locals.user = reloadedUser;
+    return null;
   }
 }
 
-module.exports = async function cronMiddleware (req, res, next) {
+async function cronAsync (req, res) {
   let user = res.locals.user;
-  if (!user) return next(); // User might not be available when authentication is not mandatory
+  if (!user) return null; // User might not be available when authentication is not mandatory
 
   let analytics = res.analytics;
   let now = new Date();
-  let _cronSignature = uuid();
 
   try {
-    console.log((new Date()).toISOString(), 'CHECKING RUN CRON', req.originalUrl, req.method);
-
     // 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.
@@ -128,10 +120,10 @@ module.exports = async function cronMiddleware (req, res, next) {
 
     if (daysMissed <= 0) {
       if (user.isModified()) await user.save();
-      return next();
+      return null;
     }
 
-    console.log((new Date()).toISOString(), 'RUNNING CRON FOR REAL', req.originalUrl, req.method);
+    let _cronSignature = uuid();
 
     // To avoid double cron we first set _cronSignature to now and then check that it's not changed while processing
     let userUpdateResult = await User.update({
@@ -142,7 +134,6 @@ module.exports = async function cronMiddleware (req, res, next) {
         _cronSignature,
       },
     }).exec();
-    console.log((new Date()).toISOString(), 'FIRST USER UPDATE?', userUpdateResult, req.originalUrl, req.method);
 
     // If the cron signature is already set, cron is running in another request
     // throw an error and recover later,
@@ -208,11 +199,10 @@ module.exports = async function cronMiddleware (req, res, next) {
 
     // Reload user
     res.locals.user = await User.findOne({_id: user._id}).exec();
-    return next();
+    return null;
   } catch (err) {
     // If cron was aborted for a race condition try to recover from it
     if (err.message === 'CRON_ALREADY_RUNNING') {
-      console.log((new Date()).toISOString(), 'RECOVERING FROM CRON', req.originalUrl, req.method);
       // Recovering after abort, wait 300ms and reload user
       // do it for max 4 times then reset _cronSignature so that it doesn't prevent cron from running
       // at the next request
@@ -220,17 +210,31 @@ module.exports = async function cronMiddleware (req, res, next) {
         times: 0,
       };
 
-      recoverCron(recoveryStatus, req, res, next);
+      recoverCron(recoveryStatus, req, res);
     } else {
       // For any other error make sure to reset _cronSignature so that it doesn't prevent cron from running
       // at the next request
-      await User.update({
+      try {
-        _id: user._id,
+        await User.update({
-      }, {
+          _id: user._id,
-        _cronSignature: 'NOT_RUNNING',
+        }, {
-      }).exec()
+          _cronSignature: 'NOT_RUNNING',
-      .then(() => next(err))
+        }).exec();
-      .catch(secondError => next(secondError));
+
+        throw err; // re-throw original error
+      } catch (secondErr) {
+        throw secondErr;
+      }
     }
   }
+}
+
+module.exports = function cronMiddleware (req, res, next) {
+  cronAsync(req, res)
+    .then(() => {
+      next();
+    })
+    .catch(err => {
+      next(err);
+    });
 };