v3: review some more files, add logging for unhandled promises

test/api/v3/unit/models/challenge.test.js

@@ -62,7 +62,7 @@ describe('Challenge Model', () => {
   each(tasksToTest, (taskValue, taskType) => {
     context(`${taskType}`, () => {
       beforeEach(async() => {
-        task = new Tasks[`${taskType}`](Tasks.Task.sanitizeCreate(taskValue));
+        task = new Tasks[`${taskType}`](Tasks.Task.sanitize(taskValue));
         task.challenge.id = challenge._id;
         await task.save();
       });

test/api/v3/unit/models/task.test.js

@@ -54,7 +54,7 @@ describe('Task Model', () => {
   each(tasksToTest, (taskValue, taskType) => {
     context(`${taskType}`, () => {
       beforeEach(async() => {
-        task = new Tasks[`${taskType}`](Tasks.Task.sanitizeCreate(taskValue));
+        task = new Tasks[`${taskType}`](Tasks.Task.sanitize(taskValue));
         task.challenge.id = challenge._id;
         task.history = generateHistory(396);
         await task.save();

website/src/libs/api-v3/analyticsService.js

@@ -210,7 +210,6 @@ let _sendPurchaseDataToGoogle = (data) => {
   });
 };
 
-// TODO log errors...
 function track (eventType, data) {
   return Q.all([
     _sendDataToAmplitude(eventType, data),

website/src/libs/api-v3/encryption.js

@@ -4,7 +4,6 @@ import {
 } from 'crypto';
 import nconf from 'nconf';
 
-// TODO check this is secure
 const algorithm = 'aes-256-ctr';
 const SESSION_SECRET = nconf.get('SESSION_SECRET');
 
@@ -22,4 +21,4 @@ export function decrypt (text) {
 
   dec += decipher.final('utf8');
   return dec;
-}
+}

website/src/libs/api-v3/i18n.js

@@ -104,15 +104,3 @@ export let multipleVersionsLanguages = {
     'zh-tw': 'zh_TW',
   },
 };
-
-// Export en strings only, temporary solution for mobile
-// This is copied from middlewares/locals#t()
-// TODO review if this can be removed since the old mobile app is no longer active
-// stringName and vars are the allowed parameters
-export function enTranslations (...args) {
-  let language = _.find(availableLanguages, {code: 'en'});
-
-  // language.momentLang = ((!isStaticPage && i18n.momentLangs[language.code]) || undefined);
-  args.push(language.code);
-  return shared.i18n.t(...args);
-}

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

@@ -20,4 +20,15 @@ if (IS_PROD) {
     });
 }
 
+// Logs unhandled promises errors
+// when no catch is attached to a promise a unhandledRejection event will be triggered
+process.on('unhandledRejection', function handlePromiseRejection (reason, promise) {
+  let stack = reason.stack || reason.message || reason;
+
+  logger.error(stack, {
+    promise,
+    fullError: reason,
+  });
+});
+
 module.exports = logger;

website/src/libs/api-v3/pushNotifications.js

@@ -9,7 +9,8 @@ let gcm = GCM_API_KEY ? pushNotify.gcm({
   retries: 3,
 }) : undefined;
 
-// TODO log
+// TODO review and test this file when push notifications are added back
+
 if (gcm) {
   gcm.on('transmitted', (/* result, message, registrationId */) => {
     // console.info("transmitted", result, message, registrationId);
@@ -24,7 +25,6 @@ if (gcm) {
   });
 }
 
-// TODO test
 module.exports = function sendNotification (user, title, message, timeToLive = 15) {
   // TODO need investigation:
   // https://github.com/HabitRPG/habitrpg/issues/5252
@@ -50,7 +50,6 @@ module.exports = function sendNotification (user, title, message, timeToLive = 1
         break;
 
       case 'ios':
-        // TODO implement
         break;
     }
   });

website/src/libs/api-v3/setupPassport.js

@@ -14,7 +14,7 @@ const FacebookStrategy = passportFacebook.Strategy;
 passport.serializeUser((user, done) => done(null, user));
 passport.deserializeUser((obj, done) => done(null, obj));
 
-// TODO
+// TODO remove?
 // This auth strategy is no longer used. It's just kept around for auth.js#loginFacebook() (passport._strategies.facebook.userProfile)
 // The proper fix would be to move to a general OAuth module simply to verify accessTokens
 passport.use(new FacebookStrategy({

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

@@ -40,7 +40,6 @@ export function authWithHeaders (optional = false) {
 }
 
 // Authenticate a request through a valid session
-// TODO should use json web token
 export function authWithSession (req, res, next) {
   let userId = req.session.userId;
 

website/src/middlewares/api-v3/domain.js

@@ -1,6 +1,3 @@
-// TODO in api-v2 this module also checked memory usage every x minutes and
-// threw an error in case of low memory avalible (possible memory leak)
-// it's yet to be decided whether to keep it or not
 import domainMiddleware from 'domain-middleware';
 
 module.exports = function implementDomainMiddleware (server, mongoose) {

website/src/middlewares/api-v3/errorHandler.js

@@ -32,8 +32,6 @@ module.exports = function errorHandler (err, req, res, next) { // eslint-disable
     responseErr.message = err.message;
   }
 
-  // TODO make mongoose and express-validator errors more recognizable
-
   // Handle errors by express-validator
   if (Array.isArray(err) && err[0].param && err[0].msg) {
     responseErr = new BadRequest(res.t('invalidReqParams'));

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

@@ -49,7 +49,7 @@ module.exports = function attachMiddlewares (app, server) {
     extended: true, // Uses 'qs' library as old connect middleware
   }));
   app.use(bodyParser.json());
-  app.use(methodOverride()); // TODO still needed in 2016?
+  app.use(methodOverride());
 
   app.use(cookieSession({
     name: 'connect:sess', // Used to keep backward compatibility with Express 3 cookies

website/src/middlewares/api-v3/locals.js

@@ -12,7 +12,6 @@ import { mods } from '../../models/user';
 
 // To avoid stringifying more data then we need,
 // items from `env` used on the client will have to be specified in this array
-// TODO where is this used?
 const CLIENT_VARS = ['language', 'isStaticPage', 'availableLanguages', 'translations',
                   'FACEBOOK_KEY', 'NODE_ENV', 'BASE_URL', 'GA_ID',
                   'AMAZON_PAYMENTS', 'STRIPE_PUB_KEY', 'AMPLITUDE_KEY',

website/src/models/challenge.js

@@ -114,7 +114,7 @@ schema.methods.syncToUser = async function syncChallengeToUser (user) {
     let matchingTask = _.find(userTasks, userTask => userTask.challenge.taskId === chalTask._id);
 
     if (!matchingTask) { // If the task is new, create it
-      matchingTask = new Tasks[chalTask.type](Tasks.Task.sanitizeCreate(_syncableAttrs(chalTask)));
+      matchingTask = new Tasks[chalTask.type](Tasks.Task.sanitize(_syncableAttrs(chalTask)));
       matchingTask.challenge = {taskId: chalTask._id, id: challenge._id};
       matchingTask.userId = user._id;
       user.tasksOrder[`${chalTask.type}s`].push(matchingTask._id);
@@ -152,13 +152,14 @@ schema.methods.addTasks = async function challengeAddTasks (tasks) {
   let membersIds = await _fetchMembersIds(challenge._id);
 
   // Sync each user sequentially
+  // TODO are we sure it's the best solution?
   for (let memberId of membersIds) {
     let updateTasksOrderQ = {$push: {}};
     let toSave = [];
 
-    // TODO eslint complaints about ahving a function inside a loop -> make sure it works
+    // TODO eslint complaints about having a function inside a loop -> make sure it works
     tasks.forEach(chalTask => { // eslint-disable-line no-loop-func
-      let userTask = new Tasks[chalTask.type](Tasks.Task.sanitizeCreate(_syncableAttrs(chalTask)));
+      let userTask = new Tasks[chalTask.type](Tasks.Task.sanitize(_syncableAttrs(chalTask)));
       userTask.challenge = {taskId: chalTask._id, id: challenge._id};
       userTask.userId = memberId;
 
@@ -192,7 +193,6 @@ schema.methods.updateTask = async function challengeUpdateTask (task) {
     updateCmd.$set[key] = syncableAttrs[key];
   }
 
-  // TODO reveiw
   // Updating instead of loading and saving for performances, risks becoming a problem if we introduce more complexity in tasks
   await Tasks.Task.update({
     userId: {$exists: true},

website/src/models/group.js

@@ -212,13 +212,13 @@ schema.methods.removeGroupInvitations = async function removeGroupInvitations ()
   let group = this;
 
   let usersToRemoveInvitationsFrom = await User.find({
-    // TODO id -> _id ?
     [`invitations.${group.type}${group.type === 'guild' ? 's' : ''}.id`]: group._id,
   }).exec();
 
   let userUpdates = usersToRemoveInvitationsFrom.map(user => {
     if (group.type === 'party') {
-      user.invitations.party = {}; // TODO mark modified
+      user.invitations.party = {};
+      this.markModified('invitations.party');
     } else {
       removeFromArray(user.invitations.guilds, { id: group._id });
     }
@@ -395,7 +395,6 @@ function _cleanQuestProgress (merge) {
   return clean;
 }
 
-// TODO move to User.cleanQuestProgress?
 schema.statics.cleanQuestProgress = _cleanQuestProgress;
 
 // returns a clean object for group.quest
@@ -619,7 +618,6 @@ schema.statics.tavernBoss = async function tavernBoss (user, progress) {
     _.assign(tavernQuest, tavern.quest.toObject());
     return tavern.save();
   }
-  // TODO catch
 };
 
 schema.methods.leave = async function leaveGroup (user, keep = 'keep-all') {

website/src/models/tag.js

@@ -5,7 +5,7 @@ let Schema = mongoose.Schema;
 
 export let schema = new Schema({
   name: {type: String, required: true},
-  challenge: {type: String}, // TODO validate
+  challenge: {type: String},
 }, {
   minimize: true, // So empty objects are returned
   strict: true,

website/src/models/task.js

@@ -57,7 +57,7 @@ export let TaskSchema = new Schema({
 TaskSchema.plugin(baseModel, {
   noSet: ['challenge', 'userId', 'completed', 'history', 'streak', 'dateCompleted', 'completed'],
   sanitizeTransform (taskObj) {
-    if (taskObj.type !== 'reward') { // value should be settable directly only for rewards
+    if (taskObj.type && taskObj.type !== 'reward') { // value should be settable directly only for rewards
       delete taskObj.value;
     }
 

website/src/models/user.js

@@ -49,7 +49,6 @@ export let schema = new Schema({
   // We want to know *every* time an object updates. Mongoose uses __v to designate when an object contains arrays which
   // have been updated (http://goo.gl/gQLz41), but we want *every* update
   _v: { type: Number, default: 0 },
-  // TODO give all this a default of 0?
   achievements: {
     originalUser: Boolean,
     habitSurveys: Number,
@@ -99,8 +98,11 @@ export let schema = new Schema({
 
   contributor: {
     // 1-9, see https://trello.com/c/wkFzONhE/277-contributor-gear https://github.com/HabitRPG/habitrpg/issues/3801
-    // TODO validate
+    level: {
-    level: Number,
+      type: Number,
+      min: 0,
+      max: 9,
+    },
     admin: Boolean,
     sudo: Boolean,
     // Artisan, Friend, Blacksmith, etc
@@ -119,7 +121,6 @@ export let schema = new Schema({
   purchased: {
     ads: {type: Boolean, default: false},
     // eg, {skeleton: true, pumpkin: true, eb052b: true}
-    // TODO dictionary
     skin: {type: Schema.Types.Mixed, default: () => {
       return {};
     }},
@@ -421,7 +422,7 @@ export let schema = new Schema({
     displayInviteToPartyWhenPartyIs1: {type: Boolean, default: true},
     webhooks: {type: Schema.Types.Mixed, default: () => {
       return {};
-    }}, // TODO array? and proper controller... unless VersionError becomes problematic
+    }},
     // For the following fields make sure to use strict comparison when searching for falsey values (=== false)
     // As users who didn't login after these were introduced may have them undefined/null
     emailNotifications: {
@@ -541,7 +542,6 @@ schema.plugin(baseModel, {
 });
 
 // A list of publicly accessible fields (not everything from preferences because there are also a lot of settings tha should remain private)
-// TODO is all party data meant to be public?
 export let publicFields = `preferences.size preferences.hair preferences.skin preferences.shirt
   preferences.costume preferences.sleep preferences.background profile stats achievements party
   backer contributor auth.timestamps items`;
@@ -823,6 +823,4 @@ mongoose.model('User')
   .then((foundMods) => {
     // Using push to maintain the reference to mods
     mods.push(...foundMods);
-  }, (err) => { // TODO replace with .catch which for some reason was throwing an error
+  }); // In case of failure we don't want this to crash the whole server
-    throw err; // TODO ?
-  });