v3: first review of common code and task models

common/script/fns/dotGet.js

@@ -1,6 +1,6 @@
 import _ from 'lodash';
 
-// TODO remove completely, use _.get
+// TODO remove completely, use _.get, only used in client
 
 module.exports = function dotGet (user, path) {
   return _.get(user, path);

common/script/fns/dotSet.js

@@ -7,7 +7,7 @@ import _ from 'lodash';
  Angular sets object properties directly - in which case, this function will be used.
 */
 
-// TODO use directly _.set and remove this fn
+// TODO use directly _.set and remove this fn, only used in client
 
 module.exports = function dotSet (user, path, val) {
   return _.set(user, path, val);

common/script/index.js

@@ -221,7 +221,6 @@ api.wrap = function wrapUser (user, main = true) {
   user._wrapped = true;
 
   // Make markModified available on the client side as a noop function
-  // TODO move to client?
   if (!user.markModified) {
     user.markModified = function noopMarkModified () {};
   }
@@ -305,14 +304,4 @@ api.wrap = function wrapUser (user, main = true) {
       return computed;
     },
   });
-
-  if (typeof window !== 'undefined') {
-    // TODO kept for compatibility with the client that relies on v2, remove once the client is adapted
-    Object.defineProperty(user, 'tasks', {
-      get () {
-        let tasks = user.habits.concat(user.dailys).concat(user.todos).concat(user.rewards);
-        return _.object(_.pluck(tasks, 'id'), tasks);
-      },
-    });
-  }
 };

common/script/libs/dotGet.js

@@ -1,5 +1,5 @@
 import _ from 'lodash';
 
-// TODO remove completely
+// TODO remove completely, only used in client
 
 module.exports = _.get;

common/script/libs/dotSet.js

@@ -1,5 +1,5 @@
 import _ from 'lodash';
 
-// TODO remove completely
+// TODO remove completely, only used in client
 
 module.exports = _.set;

common/script/libs/preenTodos.js

@@ -1,8 +1,7 @@
 import moment from 'moment';
 import _ from 'lodash';
 
-// TODO used only in v2 client
+// TODO used only in v2
-// TODO test
 
 module.exports = function preenTodos (tasks) {
   return _.filter(tasks, (t) => {

common/script/libs/taskDefaults.js

@@ -6,7 +6,6 @@ import moment from 'moment';
 // sending up to the server for performance
 
 // TODO move to client code?
-// TODO test?
 
 const tasksTypes = ['habit', 'daily', 'todo', 'reward'];
 
@@ -17,7 +16,7 @@ module.exports = function taskDefaults (task = {}) {
 
   let defaultId = uuid();
   let defaults = {
-    _id: defaultId, // TODO convert all occurencies of id to _id
+    _id: defaultId,
     text: task._id || defaultId,
     notes: '',
     tags: [],

common/script/ops/scoreTask.js

@@ -197,7 +197,7 @@ module.exports = function scoreTask (options = {}, req = {}) {
 
     // Add history entry, even more than 1 per day
     task.history.push({
-      date: Number(new Date()), // TODO are we going to cast history entries?
+      date: Number(new Date()),
       value: task.value,
     });
   } else if (task.type === 'daily') {

common/script/ops/unlock.js

@@ -1,7 +1,6 @@
 import i18n from '../i18n';
 import _ from 'lodash';
 import splitWhitespace from '../libs/splitWhitespace';
-import dotSet from '../libs/dotSet';
 import {
   NotAuthorized,
   BadRequest,
@@ -37,11 +36,11 @@ module.exports = function unlock (user, req = {}, analytics) {
   if (isFullSet) {
     _.each(path.split(','), function markItemsAsPurchased (pathPart) {
       if (path.indexOf('gear.') !== -1) {
-        dotSet(user, pathPart, true);
+        _.set(user, pathPart, true);
         return true;
       }
 
-      dotSet(user, `purchased.${pathPart}`, true);
+      _.set(user, `purchased.${pathPart}`, true);
       return true;
     });
   } else {
@@ -52,11 +51,11 @@ module.exports = function unlock (user, req = {}, analytics) {
       if (key === 'background' && value === user.preferences.background) {
         value = '';
       }
-      dotSet(user, `preferences.${key}`, value);
+      _.set(user, `preferences.${key}`, value);
 
       throw new NotAuthorized(i18n.t('alreadyUnlocked', req.language));
     }
-    dotSet(user, `purchased.${path}`, true);
+    _.set(user, `purchased.${path}`, true);
   }
 
   if (path.indexOf('gear.') === -1) {

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

@@ -121,6 +121,7 @@ describe('POST /tasks/user', () => {
         completed: true,
         streak: 25,
         dateCompleted: 'never',
+        value: 324, // ignored because not a reward
       });
 
       expect(task.userId).to.equal(user._id);
@@ -131,6 +132,7 @@ describe('POST /tasks/user', () => {
       expect(task.completed).to.equal(false);
       expect(task.streak).to.equal(0);
       expect(task.streak).not.to.equal('never');
+      expect(task.value).not.to.equal(324);
     });
 
     it('ignores invalid fields', async () => {

test/api/v3/integration/tasks/PUT-tasks_challenge_challengeId.test.js

@@ -80,6 +80,7 @@ describe('PUT /tasks/:id', () => {
         completed: true,
         streak: 25,
         dateCompleted: 'never',
+        value: 324, // ignored because not a reward
       });
 
       expect(savedTask._id).to.equal(task._id);
@@ -92,6 +93,7 @@ describe('PUT /tasks/:id', () => {
       expect(savedTask.completed).to.equal(task.completed);
       expect(savedTask.streak).to.equal(task.streak);
       expect(savedTask.dateCompleted).to.equal(task.dateCompleted);
+      expect(savedTask.value).to.equal(task.value);
     });
 
     it('ignores invalid fields', async () => {
@@ -302,12 +304,12 @@ describe('PUT /tasks/:id', () => {
       let savedReward = await user.put(`/tasks/${reward._id}`, {
         text: 'some new text',
         notes: 'some new notes',
-        value: 10,
+        value: 11,
       });
 
       expect(savedReward.text).to.eql('some new text');
       expect(savedReward.notes).to.eql('some new notes');
-      expect(savedReward.value).to.eql(10);
+      expect(savedReward.value).to.eql(11);
     });
 
     it('requires value to be coerced into a number', async () => {

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

@@ -25,7 +25,7 @@ async function _createTasks (req, res, user, challenge) {
     if (!taskData || Tasks.tasksTypes.indexOf(taskData.type) === -1) throw new BadRequest(res.t('invalidTaskType'));
 
     let taskType = taskData.type;
-    let newTask = new Tasks[taskType](Tasks.Task.sanitizeCreate(taskData));
+    let newTask = new Tasks[taskType](Tasks.Task.sanitize(taskData));
 
     if (challenge) {
       newTask.challenge.id = challenge.id;
@@ -304,10 +304,9 @@ api.updateTask = {
       throw new NotFound(res.t('taskNotFound'));
     }
 
-    Tasks.Task.sanitize(req.body);
     // TODO 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(task, common.ops.updateTask(task.toObject(), req));
+    _.assign(task, Tasks.Task.sanitize(common.ops.updateTask(task.toObject(), req)));
     // TODO console.log(task.modifiedPaths(), task.toObject().repeat === tep)
     // repeat is always among modifiedPaths because mongoose changes the other of the keys when using .toObject()
     // see https://github.com/Automattic/mongoose/issues/2749

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

@@ -8,7 +8,7 @@ module.exports = function baseModel (schema, options = {}) {
     _id: {
       type: String,
       default: uuid,
-      validate: [validator.isUUID, 'Invalid uuid.'], // TODO check for UUID version
+      validate: [validator.isUUID, 'Invalid uuid.'],
     },
   });
 

website/src/models/task.js

@@ -25,7 +25,15 @@ export let TaskSchema = new Schema({
     validate: [validator.isUUID, 'Invalid uuid.'],
   }],
   value: {type: Number, default: 0, required: true}, // redness or cost for rewards Required because it must be settable (for rewards)
-  priority: {type: Number, default: 1, required: true}, // TODO enum?
+  priority: {
+    type: Number,
+    default: 1,
+    required: true,
+    validate: [
+      (val) => [0.1, 1, 1.5, 2].indexOf(val) !== -1,
+      'Valid priority values are 0.1, 1, 1.5, 2.',
+    ],
+  },
   attribute: {type: String, default: 'str', enum: ['str', 'con', 'int', 'per']},
   userId: {type: String, ref: 'User', validate: [validator.isUUID, 'Invalid uuid.']}, // When not set it belongs to a challenge
 
@@ -33,7 +41,7 @@ export let TaskSchema = new Schema({
     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?
     broken: {type: String, enum: ['CHALLENGE_DELETED', 'TASK_DELETED', 'UNSUBSCRIBED', 'CHALLENGE_CLOSED']},
-    winner: String, // user.profile.name TODO necessary?
+    winner: String, // user.profile.name of the winner
   },
 
   reminders: [{
@@ -47,19 +55,18 @@ export let TaskSchema = new Schema({
 }, discriminatorOptions));
 
 TaskSchema.plugin(baseModel, {
-   // TODO checklist fields editable?
+  noSet: ['challenge', 'userId', 'completed', 'history', 'streak', 'dateCompleted', 'completed'],
-   // TODO value should be settable only for rewards
+  sanitizeTransform (taskObj) {
-  noSet: ['challenge', 'userId', 'completed', 'history', 'streak', 'dateCompleted'],
+    if (taskObj.type !== 'reward') { // value should be settable directly only for rewards
+      delete taskObj.value;
+    }
+
+    return taskObj;
+  },
   private: [],
   timestamps: true,
 });
 
-// A list of additional fields that cannot be set on creation (but can be set on updare)
-let noCreate = ['completed']; // TODO completed should be removed for updates too?
-TaskSchema.statics.sanitizeCreate = function sanitizeCreate (createObj) {
-  return this.sanitize(createObj, noCreate);
-};
-
 // Sanitize checklist objects (disallowing _id)
 TaskSchema.statics.sanitizeChecklist = function sanitizeChecklist (checklistObj) {
   delete checklistObj._id;
@@ -68,7 +75,7 @@ TaskSchema.statics.sanitizeChecklist = function sanitizeChecklist (checklistObj)
 
 // Sanitize reminder objects (disallowing id)
 TaskSchema.statics.sanitizeReminder = function sanitizeReminder (reminderObj) {
-  delete reminderObj.id;
+  delete reminderObj.id; // TODO convert to _id?
   return reminderObj;
 };
 
@@ -188,8 +195,7 @@ export let daily = Task.discriminator('daily', DailySchema);
 
 export let TodoSchema = new Schema(_.defaults({
   dateCompleted: Date,
-  // FIXME 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
-  // TODO change field name
   date: String, // due date for todos
 }, dailyTodoSchema()), subDiscriminatorOptions);
 export let todo = Task.discriminator('todo', TodoSchema);