From 6458796a3615c19d9aebfba8b8d0a35af46c5454 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Tue, 12 Apr 2016 19:30:39 +0200 Subject: [PATCH] v3: first review of common code and task models --- common/script/fns/dotGet.js | 2 +- common/script/fns/dotSet.js | 2 +- common/script/index.js | 11 ------ common/script/libs/dotGet.js | 2 +- common/script/libs/dotSet.js | 2 +- common/script/libs/preenTodos.js | 3 +- common/script/libs/taskDefaults.js | 3 +- common/script/ops/scoreTask.js | 2 +- common/script/ops/unlock.js | 9 +++-- .../integration/tasks/POST-tasks_user.test.js | 2 ++ .../PUT-tasks_challenge_challengeId.test.js | 6 ++-- website/src/controllers/api-v3/tasks.js | 5 ++- website/src/libs/api-v3/baseModel.js | 2 +- website/src/models/task.js | 34 +++++++++++-------- 14 files changed, 40 insertions(+), 45 deletions(-) diff --git a/common/script/fns/dotGet.js b/common/script/fns/dotGet.js index 0988fec191..3b45e54c71 100644 --- a/common/script/fns/dotGet.js +++ b/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); diff --git a/common/script/fns/dotSet.js b/common/script/fns/dotSet.js index f6ade2d0e0..ceb21605af 100644 --- a/common/script/fns/dotSet.js +++ b/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); diff --git a/common/script/index.js b/common/script/index.js index 0d734f8ba2..91c0a021ca 100644 --- a/common/script/index.js +++ b/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); - }, - }); - } }; diff --git a/common/script/libs/dotGet.js b/common/script/libs/dotGet.js index bd7605154d..d8ce026b82 100644 --- a/common/script/libs/dotGet.js +++ b/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; diff --git a/common/script/libs/dotSet.js b/common/script/libs/dotSet.js index 29d7d559e0..b664c54d05 100644 --- a/common/script/libs/dotSet.js +++ b/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; diff --git a/common/script/libs/preenTodos.js b/common/script/libs/preenTodos.js index 1fad4a6c1c..fcf8775d5f 100644 --- a/common/script/libs/preenTodos.js +++ b/common/script/libs/preenTodos.js @@ -1,8 +1,7 @@ import moment from 'moment'; import _ from 'lodash'; -// TODO used only in v2 client -// TODO test +// TODO used only in v2 module.exports = function preenTodos (tasks) { return _.filter(tasks, (t) => { diff --git a/common/script/libs/taskDefaults.js b/common/script/libs/taskDefaults.js index 04fcba3f48..c3077eb95d 100644 --- a/common/script/libs/taskDefaults.js +++ b/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: [], diff --git a/common/script/ops/scoreTask.js b/common/script/ops/scoreTask.js index 352e2bc330..ddaf0a155c 100644 --- a/common/script/ops/scoreTask.js +++ b/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') { diff --git a/common/script/ops/unlock.js b/common/script/ops/unlock.js index 74eb9fd8ac..08ec175dcc 100644 --- a/common/script/ops/unlock.js +++ b/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) { diff --git a/test/api/v3/integration/tasks/POST-tasks_user.test.js b/test/api/v3/integration/tasks/POST-tasks_user.test.js index 99d2896e3b..9834a3968d 100644 --- a/test/api/v3/integration/tasks/POST-tasks_user.test.js +++ b/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 () => { diff --git a/test/api/v3/integration/tasks/PUT-tasks_challenge_challengeId.test.js b/test/api/v3/integration/tasks/PUT-tasks_challenge_challengeId.test.js index 99b368de00..88343cb47c 100644 --- a/test/api/v3/integration/tasks/PUT-tasks_challenge_challengeId.test.js +++ b/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 () => { diff --git a/website/src/controllers/api-v3/tasks.js b/website/src/controllers/api-v3/tasks.js index 16c0457bbf..479b7a0521 100644 --- a/website/src/controllers/api-v3/tasks.js +++ b/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 diff --git a/website/src/libs/api-v3/baseModel.js b/website/src/libs/api-v3/baseModel.js index 878bebfa14..71ea879423 100644 --- a/website/src/libs/api-v3/baseModel.js +++ b/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.'], }, }); diff --git a/website/src/models/task.js b/website/src/models/task.js index 6bd581b53b..79e617a77d 100644 --- a/website/src/models/task.js +++ b/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? - // TODO value should be settable only for rewards - noSet: ['challenge', 'userId', 'completed', 'history', 'streak', 'dateCompleted'], + noSet: ['challenge', 'userId', 'completed', 'history', 'streak', 'dateCompleted', 'completed'], + sanitizeTransform (taskObj) { + 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 change field name + // 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 date: String, // due date for todos }, dailyTodoSchema()), subDiscriminatorOptions); export let todo = Task.discriminator('todo', TodoSchema);