feat: Add removeFromArray function

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

@@ -1,11 +1,11 @@
 import baseModel from '../../../../../website/src/libs/api-v3/baseModel';
-import { Schema } from 'mongoose';
+import mongoose from 'mongoose';
 
 describe('Base model plugin', () => {
   let schema;
 
   beforeEach(() => {
-    schema = new Schema();
+    schema = new mongoose.Schema();
     sandbox.stub(schema, 'add');
   });
 

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

@@ -0,0 +1,88 @@
+import mongoose from 'mongoose';
+import {
+ removeFromArray,
+} from '../../../../../website/src/libs/api-v3/collectionManipulators';
+
+describe('Collection Manipulators', () => {
+  describe('removeFromArray', () => {
+    it('removes item from specified array on document', () => {
+      let array = ['a', 'b', 'c', 'd'];
+
+      removeFromArray(array, 'c');
+
+      expect(array).to.not.include('c');
+    });
+
+    it('removes object from array', () => {
+      let array = [
+        { id: 'a', foo: 'bar' },
+        { id: 'b', foo: 'bar' },
+        { id: 'c', foo: 'bar' },
+        { id: 'd', foo: 'bar' },
+        { id: 'e', foo: 'bar' },
+      ];
+
+      removeFromArray(array, { id: 'c' });
+
+      expect(array).to.not.include({ id: 'c', foo: 'bar' });
+    });
+
+    it('does not change array if value is not found', () => {
+      let array = ['a', 'b', 'c', 'd'];
+
+      removeFromArray(array, 'z');
+
+      expect(array).to.have.a.lengthOf(4);
+      expect(array[0]).to.eql('a');
+      expect(array[1]).to.eql('b');
+      expect(array[2]).to.eql('c');
+      expect(array[3]).to.eql('d');
+    });
+
+    it('returns the removed element', () => {
+      let array = ['a', 'b', 'c'];
+
+      let result = removeFromArray(array, 'b');
+
+      expect(result).to.eql('b');
+    });
+
+    it('returns the removed object element', () => {
+      let array = [
+        { id: 'a', foo: 'bar' },
+        { id: 'b', foo: 'bar' },
+        { id: 'c', foo: 'bar' },
+        { id: 'd', foo: 'bar' },
+        { id: 'e', foo: 'bar' },
+      ];
+
+      let result = removeFromArray(array, { id: 'c' });
+
+      expect(result).to.eql({ id: 'c', foo: 'bar' });
+    });
+
+    it('returns false if item is not found', () => {
+      let array = ['a', 'b', 'c'];
+
+      let result = removeFromArray(array, 'z');
+
+      expect(result).to.eql(false);
+    });
+
+    it('removal of element persists when mongoose document is saved', async () => {
+      let schema = new mongoose.Schema({
+        array: Array,
+      });
+      let Model = mongoose.model('ModelToTestRemoveFromArray', schema);
+      let model = await new Model({
+        array: ['a', 'b', 'c'],
+      }).save(); // Initial creation
+
+      removeFromArray(model.array, 'b');
+
+      let savedModel = await model.save();
+
+      expect(savedModel.array).to.not.include('b');
+    });
+  });
+});

test/helpers/api-unit.helper.js

@@ -1,10 +1,27 @@
 import '../../website/src/libs/api-v3/i18n';
+import mongoose from 'mongoose';
 import { defaultsDeep as defaults } from 'lodash';
 import { model as User } from '../../website/src/models/user';
 import { model as Group } from '../../website/src/models/group';
 
-afterEach(() => {
+mongoose.Promise = require('q').Promise;
+
+mongoose.connect('mongodb://localhost/habitica-unit-tests');
+let connection = mongoose.connection;
+
+before((done) => {
+  connection.on('open', () => {
+    connection.db.dropDatabase(done);
+  });
+});
+
+after((done) => {
+  connection.close(done);
+});
+
+afterEach((done) => {
   sandbox.restore();
+  connection.db.dropDatabase(done);
 });
 
 export function generateUser (options = {}) {

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

@@ -7,6 +7,7 @@ import {
   NotAuthorized,
 } from '../../libs/api-v3/errors';
 import _ from 'lodash';
+import { removeFromArray } from '../../libs/api-v3/collectionManipulators';
 import { sendTxn } from '../../libs/api-v3/email';
 import nconf from 'nconf';
 
@@ -385,8 +386,7 @@ api.deleteChat = {
 
     if (chatUpdated) {
       group = group.toJSON();
-      let i = _.findIndex(group.chat, {id: chatId});
-      if (i !== -1) group.chat.splice(i, 1);
+      removeFromArray(group.chat, {id: chatId});
       res.respond(200, group.chat);
     } else {
       res.respond(200, {});

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

@@ -13,6 +13,7 @@ import {
   BadRequest,
   NotAuthorized,
 } from '../../libs/api-v3/errors';
+import { removeFromArray } from '../../libs/api-v3/collectionManipulators';
 import * as firebase from '../../libs/api-v3/firebase';
 import { sendTxn as sendTxnEmail } from '../../libs/api-v3/email';
 import { encrypt } from '../../libs/api-v3/encryption';
@@ -254,11 +255,10 @@ api.joinGroup = {
 
       isUserInvited = true;
     } else if (group.type === 'guild') {
-      let i = _.findIndex(user.invitations.guilds, {id: group._id});
+      let hasInvitation = removeFromArray(user.invitations.guilds, { id: group._id });
 
-      if (i !== -1) {
+      if (hasInvitation) {
         isUserInvited = true;
-        user.invitations.guilds.splice(i, 1); // Remove invitation
       } else {
         isUserInvited = group.privacy === 'private' ? false : true;
       }
@@ -403,8 +403,7 @@ api.removeGroupMember = {
       }
 
       if (isInGroup === 'guild') {
-        let i = member.guilds.indexOf(group._id);
-        if (i !== -1) member.guilds.splice(i, 1);
+        removeFromArray(member.guilds, group._id);
       }
       if (isInGroup === 'party') member.party._id = undefined; // TODO remove quest information too?
 
@@ -418,8 +417,7 @@ api.removeGroupMember = {
       }
     } else if (isInvited) {
       if (isInvited === 'guild') {
-        let i = _.findIndex(member.invitations.guilds, {id: group._id});
-        if (i !== -1) member.invitations.guilds.splice(i, 1);
+        removeFromArray(member.invitations.guilds, { id: group._id });
       }
       if (isInvited === 'party') user.invitations.party = {}; // TODO mark modified?
     } else {

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

@@ -1,6 +1,7 @@
 import { authWithHeaders } from '../../middlewares/api-v3/auth';
 import cron from '../../middlewares/api-v3/cron';
 import { sendTaskWebhook } from '../../libs/api-v3/webhook';
+import { removeFromArray } from '../../libs/api-v3/collectionManipulators';
 import * as Tasks from '../../models/task';
 import { model as Challenge } from '../../models/challenge';
 import {
@@ -383,14 +384,12 @@ api.scoreTask = {
     // If a todo was completed or uncompleted move it in or out of the user.tasksOrder.todos list
     if (task.type === 'todo') {
       if (!wasCompleted && task.completed) {
-        let i = user.tasksOrder.todos.indexOf(task._id);
-        if (i !== -1) user.tasksOrder.todos.splice(i, 1);
+        removeFromArray(user.tasksOrder.todos, task._id);
       } else if (wasCompleted && !task.completed) {
-        let i = user.tasksOrder.todos.indexOf(task._id);
-        if (i === -1) {
+        let hasTask = removeFromArray(user.tasksOrder.todos, task._id);
+        if (!hasTask) {
           user.tasksOrder.todos.push(task._id); // TODO push at the top?
         } else { // If for some reason it hadn't been removed TODO ok?
-          user.tasksOrder.todos.splice(i, 1);
           user.tasksOrder.push(task._id);
         }
       }
@@ -684,10 +683,8 @@ api.removeChecklistItem = {
     }
     if (task.type !== 'daily' && task.type !== 'todo') throw new BadRequest(res.t('checklistOnlyDailyTodo'));
 
-    let itemI = _.findIndex(task.checklist, {_id: req.params.itemId});
-    if (itemI === -1) throw new NotFound(res.t('checklistItemNotFound'));
-
-    task.checklist.splice(itemI, 1);
+    let hasItem = removeFromArray(task.checklist, { _id: req.params.itemId });
+    if (!hasItem) throw new NotFound(res.t('checklistItemNotFound'));
 
     let savedTask = await task.save();
     res.respond(200, {}); // TODO what to return
@@ -769,24 +766,14 @@ api.removeTagFromTask = {
 
     if (!task) throw new NotFound(res.t('taskNotFound'));
 
-    let tagI = task.tags.indexOf(req.params.tagId);
-    if (tagI === -1) throw new NotFound(res.t('tagNotFound'));
-
-    task.tags.splice(tagI, 1);
+    let hasTag = removeFromArray(task.tags, req.params.tagId);
+    if (!hasTag) throw new NotFound(res.t('tagNotFound'));
 
     await task.save();
     res.respond(200, {}); // TODO what to return
   },
 };
 
-// Remove a task from (user|challenge).tasksOrder
-function _removeTaskTasksOrder (userOrChallenge, taskId, taskType) {
-  let list = userOrChallenge.tasksOrder[`${taskType}s`];
-  let index = list.indexOf(taskId);
-
-  if (index !== -1) list.splice(index, 1);
-}
-
 // TODO this method needs some limitation, like to check if the challenge is really broken?
 /**
  * @api {post} /tasks/unlink/:taskId Unlink a challenge task
@@ -826,7 +813,7 @@ api.unlinkTask = {
       await task.save();
     } else { // remove
       if (task.type !== 'todo' || !task.completed) { // eslint-disable-line no-lonely-if
-        _removeTaskTasksOrder(user, taskId, task.type);
+        removeFromArray(user.tasksOrder[`${task.type}s`], taskId);
         await Q.all([user.save(), task.remove()]);
       } else {
         await task.remove();
@@ -904,7 +891,7 @@ api.deleteTask = {
     }
 
     if (task.type !== 'todo' || !task.completed) {
-      _removeTaskTasksOrder(challenge || user, taskId, task.type);
+      removeFromArray((challenge || user).tasksOrder[`${task.type}s`], taskId);
       await Q.all([(challenge || user).save(), task.remove()]);
     } else {
       await task.remove();

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

@@ -0,0 +1,19 @@
+import { findIndex } from 'lodash';
+
+export function removeFromArray (array, element) {
+  let elementIndex;
+
+  if (typeof element === 'object') {
+    elementIndex = findIndex(array, element);
+  } else {
+    elementIndex = array.indexOf(element);
+  }
+
+  if (elementIndex !== -1) {
+    let removedElement = array[elementIndex];
+    array.splice(elementIndex, 1);
+    return removedElement;
+  }
+
+  return false;
+}

website/src/models/group.js

@@ -7,6 +7,7 @@ import shared from '../../../common';
 import _  from 'lodash';
 import { model as Challenge} from './challenge';
 import validator from 'validator';
+import { removeFromArray } from '../libs/api-v3/collectionManipulators';
 import * as firebase from '../libs/api-v2/firebase';
 import baseModel from '../libs/api-v3/baseModel';
 import Q from 'q';
@@ -147,8 +148,7 @@ schema.methods.removeGroupInvitations = async function removeGroupInvitations ()
     if (group.type === 'party') {
       user.invitations.party = {}; // TODO mark modified
     } else {
-      let i = _.findIndex(user.invitations.guilds, {id: group._id});
-      user.invitations.guilds.splice(i, 1);
+      removeFromArray(user.invitations.guilds, { id: group._id });
     }
     return user.save();
   });

website/src/models/user.js

@@ -6,6 +6,7 @@ import moment from 'moment';
 import * as Tasks from './task';
 import Q from 'q';
 import { schema as TagSchema } from './tag';
+import { removeFromArray } from '../libs/api-v3/collectionManipulators';
 import baseModel from '../libs/api-v3/baseModel';
 // import {model as Challenge} from './challenge';
 
@@ -661,8 +662,7 @@ schema.methods.unlinkChallengeTasks = async function unlinkChallengeTasks (chall
     'challenge.id': challengeId,
   };
 
-  let challengeIndex = user.challenges.indexOf(challengeId);
-  if (challengeIndex !== -1) user.challenges.splice(challengeIndex, 1);
+  removeFromArray(user.challenges, challengeId);
 
   if (keep === 'keep-all') {
     await Tasks.Task.update(findQuery, {
@@ -675,9 +675,7 @@ schema.methods.unlinkChallengeTasks = async function unlinkChallengeTasks (chall
     let taskPromises = tasks.map(task => {
       // Remove task from user.tasksOrder and delete them
       if (task.type !== 'todo' || !task.completed) {
-        let list = user.tasksOrder[`${task.type}s`];
-        let index = list.indexOf(task._id);
-        if (index !== -1) list.splice(index, 1);
+        removeFromArray(user.tasksOrder[`${task.type}s`], task._id);
       }
 
       return task.remove();