v3: misc fixes

website/server/controllers/api-v2/auth.js

@@ -134,7 +134,6 @@ api.registerUser = function(req, res, next) {
         user.save(function(err, savedUser){
           if (err) return cb(err);
           // Clean previous email preferences
-          // TODO when emails added to EmailUnsubcription they should use lowercase version
           EmailUnsubscription.remove({email: savedUser.auth.local.email}, function(){
             utils.txnEmail(savedUser, 'welcome');
           });

website/server/controllers/api-v2/challenges.js

@@ -55,7 +55,7 @@ api.list = async function(req, res, next) {
       return obj;
     });
 
-    // TODO Instead of populate we make a find call manually because of https://github.com/Automattic/mongoose/issues/3833
+    // Instead of populate we make a find call manually because of https://github.com/Automattic/mongoose/issues/3833
     await Bluebird.all(resChals.map((chal, index) => {
       return Bluebird.all([
         User.findById(chal.leader).select(nameFields).exec(),
@@ -268,8 +268,7 @@ api.update = function(req, res, next){
           async.forEachOf(newTasksObj, function(newTask, taskId, cb2){
             // some properties can't be changed
             newTask = Tasks.Task.sanitize(newTask);
-            // TODO we have to convert task to an object because otherwise things don't get merged correctly. Bad for performances?
+            // we have to convert task to an object because otherwise things don't get merged correctly. Bad for performances?
-            // TODO regarding comment above, make sure other models with nested fields are using this trick too
             _.assign(updatedTasks[taskId], shared.ops.updateTask(updatedTasks[taskId].toObject(), {body: newTask}));
             _before.chal.updateTask(updatedTasks[taskId]).then(cb2).catch(cb2);
           }, cb1);

website/server/controllers/api-v2/user.js

@@ -429,7 +429,7 @@ api.delete = function(req, res, next) {
     return res.status(400).json({err:"You have an active subscription, cancel your plan before deleting your account."});
   }
 
-  let types = ['party', 'publicGuilds', 'privateGuilds'];
+  let types = ['party', 'guilds'];
   let groupFields = basicGroupFields.concat(' leader memberCount');
 
   Group.getGroups({user, types, groupFields})

website/server/controllers/api-v3/email.js

@@ -44,9 +44,9 @@ api.unsubscribe = {
 
       res.send(`<h1>${res.t('unsubscribedSuccessfully')}</h1> ${res.t('unsubscribedTextUsers')}`);
     } else {
-      let unsubscribedEmail = await EmailUnsubscription.findOne({email: data.email});
+      let unsubscribedEmail = await EmailUnsubscription.findOne({email: data.email.toLowerCase()});
       let okResponse = `<h1>${res.t('unsubscribedSuccessfully')}</h1> ${res.t('unsubscribedTextOthers')}`;
-      if (!unsubscribedEmail) await EmailUnsubscription.create({email: data.email});
+      if (!unsubscribedEmail) await EmailUnsubscription.create({email: data.email.toLowerCase()});
       res.send(okResponse);
     }
   },

website/server/controllers/api-v3/quests.js

@@ -383,7 +383,6 @@ api.abortQuest = {
       'party._id': groupId,
     }, {
       $set: {'party.quest': Group.cleanQuestProgress()},
-      $inc: {_v: 1}, // TODO update middleware
     }, {multi: true}).exec();
 
     let questLeaderUpdate = User.update({

website/server/controllers/api-v3/tasks.js

@@ -323,7 +323,7 @@ function _generateWebhookTaskData (task, direction, delta, stats, user) {
   let extendedStats = _.extend(stats, {
     toNextLevel: common.tnl(user.stats.lvl),
     maxHealth: common.maxHealth,
-    maxMP: user._statsComputed.maxMP, // TODO refactor as method not getter
+    maxMP: common.statsComputed(user).maxMP,
   });
 
   let userData = {
@@ -428,7 +428,6 @@ api.scoreTask = {
 };
 
 // completed todos cannot be moved, they'll be returned ordered by date of completion
-// TODO support challenges?
 /**
  * @api {post} /api/v3/tasks/:taskId/move/to/:position Move a task to a new position
  * @apiVersion 3.0.0

website/server/controllers/api-v3/user.js

@@ -36,10 +36,10 @@ api.getUser = {
     // Remove apiToken from response TODO make it private at the user level? returned in signup/login
     delete user.apiToken;
 
-    // TODO move to model (maybe virtuals, maybe in toJSON)
+    // TODO move to model? (maybe virtuals, maybe in toJSON)
     user.stats.toNextLevel = common.tnl(user.stats.lvl);
     user.stats.maxHealth = common.maxHealth;
-    user.stats.maxMP = res.locals.user._statsComputed.maxMP;
+    user.stats.maxMP = common.statsComputed(user).maxMP;
 
     return res.respond(200, user);
   },
@@ -210,7 +210,7 @@ api.deleteUser = {
       throw new NotAuthorized(res.t('cannotDeleteActiveAccount'));
     }
 
-    let types = ['party', 'publicGuilds', 'privateGuilds'];
+    let types = ['party', 'guilds'];
     let groupFields = basicGroupFields.concat(' leader memberCount');
 
     let groupsUserIsMemberOf = await Group.getGroups({user, types, groupFields});

website/server/controllers/top-level/pages.js

@@ -27,8 +27,7 @@ api.getFrontPage = {
   },
 };
 
-// TODO remove api static page
+let staticPages = ['front', 'privacy', 'terms', 'api-v2', 'features',
-let staticPages = ['front', 'privacy', 'terms', 'api', 'features',
             'videos', 'contact', 'plans', 'new-stuff', 'community-guidelines',
             'old-news', 'press-kit', 'faq', 'overview', 'apps',
             'clear-browser-data', 'merch'];

website/server/libs/api-v3/baseModel.js

@@ -32,6 +32,10 @@ module.exports = function baseModel (schema, options = {}) {
       if (!this.isNew) this.updatedAt = Date.now();
       next();
     });
+
+    schema.pre('update', function preUpdateModel () {
+      this.update({}, { $set: { updatedAt: new Date() } });
+    });
   }
 
   let noSetFields = ['createdAt', 'updatedAt'];

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

@@ -24,8 +24,8 @@ function grantEndOfTheMonthPerks (user, now) {
     plan.dateUpdated = now;
     // For every month, inc their "consecutive months" counter. Give perks based on consecutive blocks
     // If they already got perks for those blocks (eg, 6mo subscription, subscription gifts, etc) - then dec the offset until it hits 0
-    // TODO use month diff instead of ++ / --?
+    // TODO use month diff instead of ++ / --? see https://github.com/HabitRPG/habitrpg/issues/4317
-    _.defaults(plan.consecutive, {count: 0, offset: 0, trinkets: 0, gemCapExtra: 0}); // FIXME see https://github.com/HabitRPG/habitrpg/issues/4317
+    _.defaults(plan.consecutive, {count: 0, offset: 0, trinkets: 0, gemCapExtra: 0});
 
     plan.consecutive.count++;
 

website/server/libs/api-v3/logger.js

@@ -2,7 +2,6 @@
 import winston from 'winston';
 import nconf from 'nconf';
 import _ from 'lodash';
-import Bluebird from 'bluebird';
 
 const IS_PROD = nconf.get('IS_PROD');
 const IS_TEST = nconf.get('IS_TEST');
@@ -11,8 +10,6 @@ const ENABLE_CONSOLE_LOGS_IN_PROD = nconf.get('ENABLE_CONSOLE_LOGS_IN_PROD') ===
 const logger = new winston.Logger();
 
 if (IS_PROD) {
-  // TODO production logging, use loggly and new relic too
-
   if (ENABLE_CONSOLE_LOGS_IN_PROD === 'true') {
     logger.add(winston.transports.Console, {
       colorize: true,
@@ -54,15 +51,6 @@ let loggerInterface = {
   },
 };
 
-// Disable warnings for missed returns in Bluebird.
-// See https://github.com/petkaantonov/bluebird/issues/903
-Bluebird.config({
-  // Enables all warnings except forgotten return statements.
-  warnings: {
-    wForgottenReturn: false,
-  },
-});
-
 // Logs unhandled promises errors
 // when no catch is attached to a promise a unhandledRejection event will be triggered
 process.on('unhandledRejection', function handlePromiseRejection (reason) {

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

@@ -29,7 +29,7 @@ export function authWithHeaders (optional = false) {
       if (user.auth.blocked) throw new NotAuthorized(res.t('accountSuspended', {userId: user._id}));
 
       res.locals.user = user;
-      // TODO use either session/cookie or headers, not both
+
       req.session.userId = user._id;
       return next();
     })

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

@@ -128,7 +128,7 @@ module.exports = function cronMiddleware (req, res, next) {
     let ranCron = user.isModified();
     let quest = common.content.quests[user.party.quest.key];
 
-    // if (ranCron) res.locals.wasModified = true; // TODO remove after v2 is retired
+    if (ranCron) res.locals.wasModified = true; // TODO remove after v2 is retired
     if (!ranCron) return next();
 
     // Group.tavernBoss(user, progress);

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

@@ -61,7 +61,7 @@ module.exports = function attachMiddlewares (app, server) {
   app.use(cookieSession({
     name: 'connect:sess', // Used to keep backward compatibility with Express 3 cookies
     secret: SESSION_SECRET,
-    httpOnly: false, // TODO this should be true for security, what about https only?
+    httpOnly: false, // TODO this should be true for security, what about https only (secure) ?
     maxAge: TWO_WEEKS,
   }));
 

website/server/models/challenge.js

@@ -74,7 +74,7 @@ schema.methods.canView = function canViewChallenge (user, group) {
 function _syncableAttrs (task) {
   let t = task.toObject(); // lodash doesn't seem to like _.omit on Document
   // only sync/compare important attrs
-  let omitAttrs = ['_id', 'userId', 'challenge', 'history', 'tags', 'completed', 'streak', 'notes']; // TODO what to do with updatedAt?
+  let omitAttrs = ['_id', 'userId', 'challenge', 'history', 'tags', 'completed', 'streak', 'notes', 'updatedAt'];
   if (t.type !== 'reward') omitAttrs.push('value');
   return _.omit(t, omitAttrs);
 }
@@ -222,7 +222,7 @@ schema.methods.removeTask = async function challengeRemoveTask (task) {
     'challenge.id': challenge.id,
     'challenge.taskId': task._id,
   }, {
-    $set: {'challenge.broken': 'TASK_DELETED'}, // TODO what about updatedAt?
+    $set: {'challenge.broken': 'TASK_DELETED'},
   }, {multi: true}).exec();
 };
 
@@ -238,7 +238,7 @@ schema.methods.unlinkTasks = async function challengeUnlinkTasks (user, keep) {
 
   if (keep === 'keep-all') {
     await Tasks.Task.update(findQuery, {
-      $set: {challenge: {}}, // TODO what about updatedAt?
+      $set: {challenge: {}},
     }, {multi: true}).exec();
 
     await user.save();
@@ -259,7 +259,7 @@ schema.methods.unlinkTasks = async function challengeUnlinkTasks (user, keep) {
 };
 
 // TODO everything here should be moved to a worker
-// actually even for a worker it's probably just too big and will kill mongo
+// actually even for a worker it's probably just too big and will kill mongo, figure out something else
 schema.methods.closeChal = async function closeChal (broken = {}) {
   let challenge = this;
 

website/server/models/group.js

@@ -33,7 +33,6 @@ export let schema = new Schema({
   leader: {type: String, ref: 'User', validate: [validator.isUUID, 'Invalid uuid.'], required: true},
   type: {type: String, enum: ['guild', 'party'], required: true},
   privacy: {type: String, enum: ['private', 'public'], default: 'private', required: true},
-  // _v: {type: Number,'default': 0}, // TODO ?
   chat: Array,
   /*
   #    [{
@@ -94,7 +93,6 @@ schema.statics.sanitizeUpdate = function sanitizeUpdate (updateObj) {
 // Basic fields to fetch for populating a group info
 export let basicFields = 'name type privacy';
 
-// TODO test
 schema.pre('remove', true, async function preRemoveGroup (next, done) {
   next();
   try {
@@ -179,14 +177,16 @@ schema.statics.getGroups = async function getGroups (options = {}) {
         queries.push(privateGuildsQuery);
         break;
       }
+      // NOTE: when returning publicGuilds we use `.lean()` so all mongoose methods won't be available.
+      // Docs are going to be plain javascript objects
       case 'publicGuilds': {
         let publicGuildsQuery = this.find({
           type: 'guild',
           privacy: 'public',
         }).select(groupFields);
         if (populateLeader === true) publicGuildsQuery.populate('leader', nameFields);
-        publicGuildsQuery.sort(sort).exec();
+        publicGuildsQuery.sort(sort).lean().exec();
-        queries.push(publicGuildsQuery); // TODO use lean?
+        queries.push(publicGuildsQuery);
         break;
       }
       case 'tavern': {

website/server/models/task.js

@@ -142,7 +142,6 @@ TaskSchema.statics.fromJSONV2 = function fromJSONV2 (taskObj) {
   return taskObj;
 };
 
-
 // END of API v2 methods
 
 export let Task = mongoose.model('Task', TaskSchema);

website/server/models/user.js

@@ -525,11 +525,8 @@ export let schema = new Schema({
 });
 
 schema.plugin(baseModel, {
-  // TODO revisit a lot of things are missing. Given how many attributes we do have here we should white-list the ones that can be updated
+  // noSet is not used as updating uses a whitelist and creating only accepts specific params (password, email, username, ...)
-  // This is not really used as updating uses a whitelist and creating only accepts specific params (password, email, username, ...)
+  noSet: [],
-  noSet: ['_id', 'apiToken', 'auth.blocked', 'auth.timestamps', 'lastCron', 'auth.local.hashed_password',
-    'auth.local.salt', 'tasksOrder', 'tags', 'stats', 'challenges', 'guilds', 'party._id', 'party.quest',
-    'invitations', 'balance', 'backer', 'contributor'],
   private: ['auth.local.hashed_password', 'auth.local.salt'],
   toJSONTransform: function userToJSON (plainObj, originalDoc) {
     plainObj.id = plainObj._id;
@@ -638,7 +635,6 @@ function _setProfileName (user) {
 schema.pre('save', true, function preSaveUser (next, done) {
   next();
 
-  // TODO remove all unnecessary checks
   if (_.isNaN(this.preferences.dayStart) || this.preferences.dayStart < 0 || this.preferences.dayStart > 23) {
     this.preferences.dayStart = 0;
   }
@@ -697,7 +693,10 @@ schema.pre('save', true, function preSaveUser (next, done) {
   }
 });
 
-// TODO unit test this?
+schema.pre('update', function preUpdateUser () {
+  this.update({}, {$inc: {_v: 1}});
+});
+
 schema.methods.isSubscribed = function isSubscribed () {
   return !!this.purchased.plan.customerId; // eslint-disable-line no-implicit-coercion
 };