[WIP] Add initial fixes for concurrency (#9321)

* Add initial fixes for concurrency

* Added memory edit for notifications

* Fixed tag delete

* Fixed adding and moving task order

* Updated delete task

* Fixed lint

* Fixed task adding

* Switch to mongoose push and pull

.eslintignore

@@ -6,6 +6,9 @@ website/transpiled-babel/
 website/common/transpiled-babel/
 dist/
 dist-client/
+apidoc_build/
+content_cache/
+node_modules/
 
 # Not linted
 website/client-old/

test/api/v3/integration/tasks/DELETE-tasks_id.test.js

@@ -141,6 +141,16 @@ describe('DELETE /tasks/:id', () => {
       });
     });
 
-    it('removes a task from user.tasksOrder'); // TODO
+    it('removes a task from user.tasksOrder', async () => {
+      let task = await user.post('/tasks/user', {
+        text: 'test habit',
+        type: 'habit',
+      });
+
+      await user.del(`/tasks/${task._id}`);
+      await user.sync();
+
+      expect(user.tasksOrder.habits.indexOf(task._id)).to.eql(-1);
+    });
   });
 });

test/api/v3/integration/tasks/POST-tasks_move_taskId_to_position.test.js

@@ -40,9 +40,13 @@ describe('POST /tasks/:taskId/move/to/:position', () => {
 
     let taskToMove = tasks[1];
     expect(taskToMove.text).to.equal('habit 2');
+
     let newOrder = await user.post(`/tasks/${tasks[1]._id}/move/to/3`);
+    await user.sync();
+
     expect(newOrder[3]).to.equal(taskToMove._id);
     expect(newOrder.length).to.equal(5);
+    expect(user.tasksOrder.habits).to.eql(newOrder);
   });
 
   it('can move task to new position using alias', async () => {

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

@@ -37,7 +37,10 @@ api.readNotification = {
     }
 
     user.notifications.splice(index, 1);
-    await user.save();
+
+    await user.update({
+      $pull: { notifications: req.params.notificationId },
+    }).exec();
 
     res.respond(200, user.notifications);
   },
@@ -77,7 +80,9 @@ api.readNotifications = {
       user.notifications.splice(index, 1);
     }
 
-    await user.save();
+    await user.update({
+      $pullAll: { notifications },
+    }).exec();
 
     res.respond(200, user.notifications);
   },

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

@@ -5,7 +5,7 @@ import {
   NotFound,
 } from '../../libs/errors';
 import _ from 'lodash';
-import { removeFromArray } from '../../libs/collectionManipulators';
+import find from 'lodash/find';
 
 /**
  * @apiDefine TagNotFound
@@ -216,19 +216,24 @@ api.deleteTag = {
     let validationErrors = req.validationErrors();
     if (validationErrors) throw validationErrors;
 
-    let tag = removeFromArray(user.tags, { id: req.params.tagId });
-    if (!tag) throw new NotFound(res.t('tagNotFound'));
+    let tagFound = find(user.tags, (tag) => {
+      return tag.id === req.params.tagId;
+    });
+    if (!tagFound) throw new NotFound(res.t('tagNotFound'));
+
+    await user.update({
+      $pull: { tags: { id: tagFound.id } },
+    }).exec();
 
     // Remove from all the tasks TODO test
     await Tasks.Task.update({
       userId: user._id,
     }, {
       $pull: {
-        tags: tag.id,
+        tags: tagFound.id,
       },
     }, {multi: true}).exec();
 
-    await user.save();
     res.respond(200, {});
   },
 };

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

@@ -581,12 +581,17 @@ api.scoreTask = {
     // TODO move to common code?
     if (task.type === 'todo') {
       if (!wasCompleted && task.completed) {
-        removeFromArray(user.tasksOrder.todos, task._id);
-      } else if (wasCompleted && !task.completed) {
-        let hasTask = removeFromArray(user.tasksOrder.todos, task._id);
-        if (!hasTask) {
-          user.tasksOrder.todos.push(task._id);
-        } // If for some reason it hadn't been removed previously don't do anything
+        // @TODO: mongoose's push and pull should be atomic and help with
+        // our concurrency issues. If not, we need to use this update $pull and $push
+        // await user.update({
+        //   $pull: { 'tasksOrder.todos': task._id },
+        // }).exec();
+        user.tasksOrder.todos.pull(task._id);
+      } else if (wasCompleted && !task.completed && user.tasksOrder.todos.indexOf(task._id) === -1) {
+        // await user.update({
+        //   $push: { 'tasksOrder.todos': task._id },
+        // }).exec();
+        user.tasksOrder.todos.push(task._id);
       }
     }
 
@@ -685,11 +690,28 @@ api.moveTask = {
 
     if (!task) throw new NotFound(res.t('taskNotFound'));
     if (task.type === 'todo' && task.completed) throw new BadRequest(res.t('cantMoveCompletedTodo'));
-    let order = user.tasksOrder[`${task.type}s`];
 
+    // In memory updates
+    let order = user.tasksOrder[`${task.type}s`];
     moveTask(order, task._id, to);
 
-    await user.save();
+    // Server updates
+    // @TODO: maybe bulk op?
+    let pullQuery = { $pull: {} };
+    pullQuery.$pull[`tasksOrder.${task.type}s`] = task.id;
+    await user.update(pullQuery).exec();
+
+    // Handle push to bottom
+    let position = to;
+    if (to === -1) position = [`tasksOrder.${task.type}s`].length - 1;
+
+    let updateQuery = { $push: {} };
+    updateQuery.$push[`tasksOrder.${task.type}s`] = {
+      $each: [task._id],
+      $position: position,
+    };
+    await user.update(updateQuery).exec();
+
     res.respond(200, order);
   },
 };
@@ -1245,7 +1267,12 @@ api.deleteTask = {
 
     if (task.type !== 'todo' || !task.completed) {
       removeFromArray((challenge || user).tasksOrder[`${task.type}s`], taskId);
-      await Bluebird.all([(challenge || user).save(), task.remove()]);
+
+      let pullQuery = {$pull: {}};
+      pullQuery.$pull[`tasksOrder.${task.type}s`] = task._id;
+      let taskOrderUpdate = (challenge || user).update(pullQuery).exec();
+
+      await Bluebird.all([taskOrderUpdate, task.remove()]);
     } else {
       await task.remove();
     }

website/server/libs/cron.js

@@ -447,7 +447,6 @@ export function cron (options = {}) {
 
   // First remove a possible previous cron notification
   // we don't want to flood the users with many cron notifications at once
-
   let oldCronNotif = user.notifications.toObject().find((notif, index) => {
     if (notif.type === 'CRON') {
       user.notifications.splice(index, 1);

website/server/libs/taskManager.js

@@ -77,6 +77,8 @@ export async function createTasks (req, res, options = {}) {
 
   let toSave = Array.isArray(req.body) ? req.body : [req.body];
 
+  let taskOrderToAdd = {};
+
   toSave = toSave.map(taskData => {
     // Validate that task.type is valid
     if (!taskData || Tasks.tasksTypes.indexOf(taskData.type) === -1) throw new BadRequest(res.t('invalidTaskType'));
@@ -103,11 +105,23 @@ export async function createTasks (req, res, options = {}) {
     if (validationErrors) throw validationErrors;
 
     // Otherwise update the user/challenge/group
-    owner.tasksOrder[`${taskType}s`].unshift(newTask._id);
+    if (!taskOrderToAdd[`${taskType}s`]) taskOrderToAdd[`${taskType}s`] = [];
+    taskOrderToAdd[`${taskType}s`].unshift(newTask._id);
 
     return newTask;
   });
 
+  // Push all task ids
+  let taskOrderUpdateQuery = {$push: {}};
+  for (let taskType in taskOrderToAdd) {
+    taskOrderUpdateQuery.$push[`tasksOrder.${taskType}`] = {
+      $each: taskOrderToAdd[taskType],
+      $position: 0,
+    };
+  }
+
+  await owner.update(taskOrderUpdateQuery).exec();
+
   // tasks with aliases need to be validated asyncronously
   await _validateTaskAlias(toSave, res);