v3: start cleaning up TODO comemnts

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

@@ -127,7 +127,7 @@ api.registerLocal = {
       newUser = fbUser;
     } else {
       newUser = new User(newUser);
-      newUser.registeredThrough = req.headers['x-client']; // TODO is this saved somewhere?
+      newUser.registeredThrough = req.headers['x-client']; // Not saved, used to create the correct tasks based on the device used
     }
 
     // we check for partyInvite for backward compatibility

website/src/controllers/api-v3/challenges.js

@@ -475,7 +475,7 @@ export async function _closeChal (challenge, broken = {}) {
       ]);
     }
 
-    sendPushNotification(savedWinner, shared.i18n.t('wonChallenge'), challenge.name); // TODO translate
+    sendPushNotification(savedWinner, shared.i18n.t('wonChallenge'), challenge.name);
   }
 
   // Run some operations in the background withouth blocking the thread
@@ -501,7 +501,7 @@ export async function _closeChal (challenge, broken = {}) {
     }, {multi: true}).exec(),
   ];
 
-  Q.allSettled(backgroundTasks); // TODO look if allSettled could be useful somewhere else
+  Q.all(backgroundTasks);
 }
 
 /**

website/src/controllers/api-v3/chat.js

@@ -12,6 +12,7 @@ import _ from 'lodash';
 import { removeFromArray } from '../../libs/api-v3/collectionManipulators';
 import { sendTxn } from '../../libs/api-v3/email';
 import nconf from 'nconf';
+import Q from 'q';
 
 const FLAG_REPORT_EMAILS = nconf.get('FLAG_REPORT_EMAIL').split(',').map((email) => {
   return { email, canSend: true };
@@ -87,12 +88,14 @@ api.postChat = {
 
     group.sendChat(req.body.message, user);
 
+    let toSave = [group.save()];
+
     if (group.type === 'party') {
       user.party.lastMessageSeen = group.chat[0].id;
-      user.save(); // TODO why this is non-blocking? must catch?
+      toSave.push(user.save());
     }
 
-    let savedGroup = await group.save();
+    let [savedGroup] = await Q.all(toSave);
     if (chatUpdated) {
       res.respond(200, {chat: Group.toJSONCleanChat(savedGroup, user).chat});
     } else {

website/src/controllers/api-v3/groups.js

@@ -95,7 +95,6 @@ api.getGroups = {
     let validationErrors = req.validationErrors();
     if (validationErrors) throw validationErrors;
 
-    // TODO validate types are acceptable? probably not necessary
     let types = req.query.type.split(',');
     let groupFields = basicGroupFields.concat('description memberCount balance');
     let sort = '-memberCount';
@@ -444,7 +443,7 @@ api.removeGroupMember = {
         group.quest.leader = undefined;
       } else if (group.quest && group.quest.members) {
         // remove member from quest
-        group.quest.members[member._id] = undefined; // TODO remmeber to check these are mark modified everywhere
+        group.quest.members[member._id] = undefined;
         group.markModified('quest.members');
       }
 

website/src/controllers/api-v3/members.js

@@ -16,6 +16,7 @@ import {
   sendTxn as sendTxnEmail,
 } from '../../libs/api-v3/email';
 import Q from 'q';
+import sendPushNotification from '../../libs/api-v3/pushNotifications';
 
 let api = {};
 
@@ -349,8 +350,7 @@ api.transferGems = {
       ]);
     }
 
-    // TODO: Add push notifications
-    // pushNotify.sendNotify(sender, res.t('giftedGems'), res.t('giftedGemsInfo', { amount: gemAmount, name: byUsername }));
+    sendPushNotification(sender, res.t('giftedGems'), res.t('giftedGemsInfo', { amount: gemAmount, name: byUsername }));
 
     res.respond(200, {});
   },

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

@@ -85,7 +85,7 @@ api.createUserTasks = {
  */
 api.createChallengeTasks = {
   method: 'POST',
-  url: '/tasks/challenge/:challengeId', // TODO should be /tasks/challengeS/:challengeId ? plural?
+  url: '/tasks/challenge/:challengeId',
   middlewares: [authWithHeaders()],
   async handler (req, res) {
     req.checkParams('challengeId', res.t('challengeIdRequired')).notEmpty().isUUID();
@@ -303,7 +303,6 @@ api.updateTask = {
     }
 
     // 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
     let [updatedTaskObj] = common.ops.updateTask(task.toObject(), req);
     _.assign(task, Tasks.Task.sanitize(updatedTaskObj));
     // console.log(task.modifiedPaths(), task.toObject().repeat === tep)
@@ -360,7 +359,7 @@ api.scoreTask = {
   middlewares: [authWithHeaders()],
   async handler (req, res) {
     req.checkParams('taskId', res.t('taskIdRequired')).notEmpty().isUUID();
-    req.checkParams('direction', res.t('directionUpDown')).notEmpty().isIn(['up', 'down']); // TODO what about rewards? maybe separate route?
+    req.checkParams('direction', res.t('directionUpDown')).notEmpty().isIn(['up', 'down']);
 
     let validationErrors = req.validationErrors();
     if (validationErrors) throw validationErrors;
@@ -389,7 +388,7 @@ api.scoreTask = {
       } else if (wasCompleted && !task.completed) {
         let hasTask = removeFromArray(user.tasksOrder.todos, task._id);
         if (!hasTask) {
-          user.tasksOrder.todos.push(task._id); // TODO push at the top?
+          user.tasksOrder.todos.push(task._id);
         } // If for some reason it hadn't been removed previously don't do anything
       }
     }

website/src/controllers/top-level/dataexport.js

@@ -171,7 +171,7 @@ api.exportUserAvatarHtml = {
     if (!member) throw new NotFound(res.t('userWithIDNotFound', {userId: memberId}));
     res.render('avatar-static', {
       title: member.profile.name,
-      env: _.defaults({member}, res.locals.habitrpg), // TODO review once static pages are done
+      env: _.defaults({member}, res.locals.habitrpg),
     });
   },
 };

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

@@ -262,7 +262,7 @@ export function cron (options = {}) {
     gaLabel: 'Cron Count',
     gaValue: user.flags.cronCount,
     uuid: user._id,
-    user, // TODO is it really necessary passing the whole user object?
+    user,
     resting: user.preferences.sleep,
     cronCount: user.flags.cronCount,
     progressUp: _.min([_progress.up, 900]),

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

@@ -30,12 +30,12 @@ Subscribers and challenges:
 - 1 value each year for the previous years
  */
 export function preenHistory (history, isSubscribed, timezoneOffset) {
-  // history = _.filter(history, historyEntry => Boolean(historyEntry)); // Filter missing entries TODO add to migration
+  // history = _.filter(history, historyEntry => Boolean(historyEntry)); // Filter missing entries
   let now = timezoneOffset ? moment().zone(timezoneOffset) : moment();
   // Date after which to begin compressing data
   let cutOff = now.subtract(isSubscribed ? 365 : 60, 'days').startOf('day');
 
-  // Keep uncompressed entries (modifies history)
+  // Keep uncompressed entries (modifies history and returns removed items)
   let newHistory = _.remove(history, entry => {
     let date = moment(entry.date);
     return date.isSame(cutOff) || date.isAfter(cutOff);

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

@@ -26,10 +26,7 @@ if (gcm) {
 }
 
 module.exports = function sendNotification (user, title, message, timeToLive = 15) {
-  // TODO need investigation:
-  // https://github.com/HabitRPG/habitrpg/issues/5252
-
-  if (!user) throw new Error('User is required.');
+  if (!user) return;
 
   _.each(user.pushDevices, pushDevice => {
     switch (pushDevice.type) {

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

@@ -5,7 +5,7 @@ import {
   model as User,
 } from '../../models/user';
 
-// TODO how to translate the strings here since getUserLanguage hasn't run yet?
+// Strins won't be translated here because getUserLanguage has not run yet
 
 // Authenticate a request through the x-api-user and x-api key header
 // If optional is true, don't error on missing authentication

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

@@ -123,12 +123,12 @@ module.exports = function cronMiddleware (req, res, next) {
         $lt: moment(now).subtract(user.isSubscribed() ? 90 : 30, 'days').toDate(),
       },
       'challenge.id': {$exists: false},
-    }).exec(); // TODO wait before returning?
+    }).exec();
 
     let ranCron = user.isModified();
     let quest = common.content.quests[user.party.quest.key];
 
-    // if (ranCron) res.locals.wasModified = true; // TODO remove?
+    // if (ranCron) res.locals.wasModified = true; // TODO remove after v2 is retired
     if (!ranCron) return next();
 
     // Group.tavernBoss(user, progress);

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

@@ -52,7 +52,6 @@ module.exports = function attachMiddlewares (app, server) {
   app.use(forceSSL);
   app.use(forceHabitica);
 
-  // TODO if we don't manage to move the client off $resource the limit for bodyParser.json must be increased to 1mb from 100kb (default)
   app.use(bodyParser.urlencoded({
     extended: true, // Uses 'qs' library as old connect middleware
   }));

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

@@ -1,4 +1,4 @@
-// TODO tests?
+// TODO test this middleware
 module.exports = function setupBodyMiddleware (req, res, next) {
   req.body = req.body || {};
   next();

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

@@ -19,8 +19,8 @@ v2app.use(responseHandler);
 
 // Custom Directives
 v2app.use('/', require('../../routes/api-v2/auth'));
-v2app.use('/', require('../../routes/api-v2/coupon')); // TODO REMOVE - ONLY v3
-v2app.use('/', require('../../routes/api-v2/unsubscription')); // TODO REMOVE - ONLY v3
+// v2app.use('/', require('../../routes/api-v2/coupon')); // TODO REMOVE - ONLY v3
+// v2app.use('/', require('../../routes/api-v2/unsubscription')); // TODO REMOVE - ONLY v3
 
 require('../../routes/api-v2/swagger')(swagger, v2app);
 

website/src/middlewares/apiThrottle.js

@@ -4,10 +4,9 @@ var limiter = require('connect-ratelimit');
 var IS_PROD = nconf.get('NODE_ENV') === 'production';
 
 // TODO since Habitica runs on many different servers this module is pretty useless
-// as it will only block requests that go to the same server
+// as it will only block requests that go to the same server but anyway we should probably have a rate limiter in place
 
 module.exports = function(app) {
-  // TODO review later
   // disable the rate limiter middleware
   if (/*!IS_PROD || */true) return;
   app.use(limiter({

website/src/middlewares/forceRefresh.js

@@ -1,4 +1,4 @@
-// TODO do we need this module?
+// TODO do we need this module anymore in v3? No
 
 module.exports.siteVersion = 1;
 

website/src/models/challenge.js

@@ -123,7 +123,7 @@ schema.methods.syncToUser = async function syncChallengeToUser (user) {
       user.tasksOrder[`${chalTask.type}s`].push(matchingTask._id);
     } else {
       _.merge(matchingTask, _syncableAttrs(chalTask));
-      // Make sure the task is in user.tasksOrder TODO necessary?
+      // Make sure the task is in user.tasksOrder
       let orderList = user.tasksOrder[`${chalTask.type}s`];
       if (orderList.indexOf(matchingTask._id) === -1 && (matchingTask.type !== 'todo' || !matchingTask.completed)) orderList.push(matchingTask._id);
     }
@@ -155,7 +155,7 @@ 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?
+  // TODO are we sure it's the best solution? Use cwait
   // use bulk ops? http://stackoverflow.com/questions/16726330/mongoose-mongodb-batch-insert
   for (let memberId of membersIds) {
     let updateTasksOrderQ = {$push: {}};

website/src/models/group.js

@@ -420,7 +420,7 @@ schema.methods.finishQuest = function finishQuest (quest) {
   let updates = {$inc: {}, $set: {}};
 
   updates.$inc[`achievements.quests.${questK}`] = 1;
-  updates.$inc['stats.gp'] = Number(quest.drop.gp); // TODO are this castings necessary?
+  updates.$inc['stats.gp'] = Number(quest.drop.gp);
   updates.$inc['stats.exp'] = Number(quest.drop.exp);
   updates.$inc._v = 1;
 
@@ -530,7 +530,8 @@ schema.statics.bossQuest = async function bossQuest (user, progress) {
   }, {multi: true}).exec();
   // Apply changes the currently cronning user locally so we don't have to reload it to get the updated state
   // TODO how to mark not modified? https://github.com/Automattic/mongoose/pull/1167
-  // must be notModified or otherwise could overwrite future changes
+  // must be notModified or otherwise could overwrite future changes: if the user is saved it'll save
+  // the modified user.stats.hp but that must not happen as the hp value has already been updated by the User.update above
   // if (down) user.stats.hp += down;
 
   // Boss slain, finish quest

website/src/models/task.js

@@ -39,7 +39,7 @@ export let TaskSchema = new Schema({
 
   challenge: {
     id: {type: String, ref: 'Challenge', validate: [validator.isUUID, 'Invalid uuid.']}, // When set (and userId not set) it's the original task
-    taskId: {type: String, ref: 'Task', validate: [validator.isUUID, 'Invalid uuid.']}, // When not set but challenge.id defined it's the original task TODO unique index?
+    taskId: {type: String, ref: 'Task', validate: [validator.isUUID, 'Invalid uuid.']}, // When not set but challenge.id defined it's the original task
     broken: {type: String, enum: ['CHALLENGE_DELETED', 'TASK_DELETED', 'UNSUBSCRIBED', 'CHALLENGE_CLOSED']},
     winner: String, // user.profile.name of the winner
   },
@@ -149,7 +149,7 @@ export let Task = mongoose.model('Task', TaskSchema);
 
 // habits and dailies shared fields
 let habitDailySchema = () => {
-  return {history: Array}; // [{date:Date, value:Number}], // this causes major performance problems TODO revisit
+  return {history: Array}; // [{date:Date, value:Number}], // this causes major performance problems
 };
 
 // dailys and todos shared fields
@@ -197,7 +197,7 @@ export let daily = Task.discriminator('daily', DailySchema);
 
 export let TodoSchema = new Schema(_.defaults({
   dateCompleted: Date,
-  // TODO we're getting parse errors, people have stored as "today" and "3/13". Need to run a migration & put this back to type: Date
+  // TODO we're getting parse errors, people have stored as "today" and "3/13". Need to run a migration & put this back to type: Date see http://stackoverflow.com/questions/1353684/detecting-an-invalid-date-date-instance-in-javascript
   date: String, // due date for todos
 }, dailyTodoSchema()), subDiscriminatorOptions);
 export let todo = Task.discriminator('todo', TodoSchema);

website/src/models/user.js

@@ -30,7 +30,7 @@ export let schema = new Schema({
     local: {
       email: {
         type: String,
-        validate: [validator.isEmail, shared.i18n.t('invalidEmail')], // TODO translate error messages here, use preferences.language?
+        validate: [validator.isEmail, shared.i18n.t('invalidEmail')],
       },
       username: {
         type: String,
@@ -526,14 +526,14 @@ 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
-  // TODO this is a only used for creating an user, on update we use a whitelist
+  // This is not really used as updating uses a whitelist and creating only accepts specific params (password, email, username, ...)
   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.filters = {}; TODO Not saved
-    plainObj._tmp = originalDoc._tmp; // be sure to send down drop notifs TODO how to test?
+    // plainObj.filters = {}; TODO Not saved, remove?
+    plainObj._tmp = originalDoc._tmp; // be sure to send down drop notifs
 
     return plainObj;
   },
@@ -590,7 +590,7 @@ function _populateDefaultTasks (user, taskTypes) {
       return newTask.save();
     });
 
-    tasksToCreate.push(...tasksOfType); // TODO find better way since this creates each task individually
+    tasksToCreate.push(...tasksOfType);
   });
 
   return Q.all(tasksToCreate)