Reward with negative cost can no longer be created, fixes #11855 (#11870)

* Minor refactoring in scoreTask.js

* Reward value validation added (should be >= 0)

migrations/migration-runner.js

@@ -18,7 +18,7 @@ function setUpServer () {
 setUpServer();
 
 // Replace this with your migration
-const processUsers = () => {}; // require('').default;
+const processUsers = require('./tasks/rewards-flip-negative-costs').default;
 
 processUsers()
   .then(() => {

migrations/tasks/rewards-flip-negative-costs.js

@@ -0,0 +1,54 @@
+// @migrationName = 'RewardsMigrationFlipNegativeCostsValues';
+// @authorName = 'hamboomger';
+// @authorUuid = '80b61b73-2278-4947-b713-a10112cfe7f5';
+
+/*
+ * For each reward with negative cost, make it positive
+ * by assigning it an absolute value of itself
+ */
+
+import { Task } from '../../website/server/models/task';
+
+async function flipNegativeCostsValues () {
+  const query = {
+    type: 'reward',
+    value: { $lt: 0 },
+  };
+
+  const fields = {
+    _id: 1,
+    value: 1,
+  };
+
+  // eslint-disable-next-line no-constant-condition
+  while (true) {
+    // eslint-disable-next-line no-await-in-loop
+    const rewards = await Task
+      .find(query)
+      .limit(250)
+      .sort({ _id: 1 })
+      .select(fields)
+      .lean()
+      .exec();
+
+    if (rewards.length === 0) {
+      break;
+    }
+
+    const promises = rewards.map(reward => {
+      const positiveValue = Math.abs(reward.value);
+      return Task.update({ _id: reward._id }, { $set: { value: positiveValue } }).exec();
+    });
+
+    // eslint-disable-next-line no-await-in-loop
+    await Promise.all(promises);
+
+    query._id = {
+      $gt: rewards[rewards.length - 1]._id,
+    };
+  }
+
+  console.log('All rewards with negative values were updated, migration finished');
+}
+
+export default flipNegativeCostsValues;

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

@@ -55,6 +55,18 @@ describe('POST /tasks/user', () => {
       });
     });
 
+    it('returns an error if reward value is a negative number', async () => {
+      await expect(user.post('/tasks/user', {
+        type: 'reward',
+        text: 'reward with negative value',
+        value: -10,
+      })).to.eventually.be.rejected.and.eql({
+        code: 400,
+        error: 'BadRequest',
+        message: 'reward validation failed',
+      });
+    });
+
     it('does not update user.tasksOrder.{taskType} when the task is not saved because invalid', async () => {
       const originalHabitsOrder = (await user.get('/user')).tasksOrder.habits;
       await expect(user.post('/tasks/user', {

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

@@ -530,5 +530,15 @@ describe('PUT /tasks/:id', () => {
 
       expect(savedReward.value).to.eql(100);
     });
+
+    it('returns an error if reward value is a negative number', async () => {
+      await expect(user.put(`/tasks/${reward._id}`, {
+        value: -10,
+      })).to.eventually.be.rejected.and.eql({
+        code: 400,
+        error: 'BadRequest',
+        message: 'reward validation failed',
+      });
+    });
   });
 });

website/common/script/ops/scoreTask.js

@@ -189,6 +189,37 @@ function _updateCounter (task, direction, times) {
   }
 }
 
+function _lastHistoryEntryWasToday (lastHistoryEntry, user) {
+  if (!lastHistoryEntry || !lastHistoryEntry.date) {
+    return false;
+  }
+
+  const { timezoneOffset } = user.preferences;
+  const { dayStart } = user.preferences;
+
+  // Adjust the last entry date according to the user's timezone and CDS
+  const dateWithTimeZone = moment(lastHistoryEntry.date).zone(timezoneOffset);
+  if (dateWithTimeZone.hour() < dayStart) dateWithTimeZone.subtract(1, 'day');
+
+  return moment().zone(timezoneOffset).isSame(dateWithTimeZone, 'day');
+}
+
+function _updateLastHistoryEntry (lastHistoryEntry, task, direction, times) {
+  lastHistoryEntry.value = task.value;
+  lastHistoryEntry.date = Number(new Date());
+
+  // @TODO remove this extra check after migration
+  // has run to set scoredUp and scoredDown in every task
+  lastHistoryEntry.scoredUp = lastHistoryEntry.scoredUp || 0;
+  lastHistoryEntry.scoredDown = lastHistoryEntry.scoredDown || 0;
+
+  if (direction === 'up') {
+    lastHistoryEntry.scoredUp += times;
+  } else {
+    lastHistoryEntry.scoredDown += times;
+  }
+}
+
 export default function scoreTask (options = {}, req = {}, analytics) {
   const {
     user, task, direction, times = 1, cron = false,
@@ -226,38 +257,14 @@ export default function scoreTask (options = {}, req = {}, analytics) {
 
     // Save history entry for habit
     task.history = task.history || [];
-    const { timezoneOffset } = user.preferences;
-    const { dayStart } = user.preferences;
     const historyLength = task.history.length;
     const lastHistoryEntry = task.history[historyLength - 1];
 
-    // Adjust the last entry date according to the user's timezone and CDS
-    let lastHistoryEntryDate;
-
-    if (lastHistoryEntry && lastHistoryEntry.date) {
-      lastHistoryEntryDate = moment(lastHistoryEntry.date).zone(timezoneOffset);
-      if (lastHistoryEntryDate.hour() < dayStart) lastHistoryEntryDate.subtract(1, 'day');
+    if (_lastHistoryEntryWasToday(lastHistoryEntry, user)) {
+      _updateLastHistoryEntry(lastHistoryEntry, task, direction, times);
+      if (task.markModified) {
+        task.markModified(`history.${historyLength - 1}`);
       }
-
-    if (
-      lastHistoryEntryDate
-      && moment().zone(timezoneOffset).isSame(lastHistoryEntryDate, 'day')
-    ) {
-      lastHistoryEntry.value = task.value;
-      lastHistoryEntry.date = Number(new Date());
-
-      // @TODO remove this extra check after migration
-      // has run to set scoredUp and scoredDown in every task
-      lastHistoryEntry.scoredUp = lastHistoryEntry.scoredUp || 0;
-      lastHistoryEntry.scoredDown = lastHistoryEntry.scoredDown || 0;
-
-      if (direction === 'up') {
-        lastHistoryEntry.scoredUp += times;
-      } else {
-        lastHistoryEntry.scoredDown += times;
-      }
-
-      if (task.markModified) task.markModified(`history.${historyLength - 1}`);
     } else {
       task.history.push({
         date: Number(new Date()),
@@ -334,12 +341,7 @@ export default function scoreTask (options = {}, req = {}, analytics) {
     // Don't adjust values for rewards
     delta += _changeTaskValue(user, task, direction, times, cron);
     // purchase item
-    stats.gp -= Math.abs(task.value);
-    // hp - gp difference
-    if (stats.gp < 0) {
-      stats.hp += stats.gp;
-      stats.gp = 0;
-    }
+    stats.gp -= task.value;
   }
 
   req.yesterDailyScored = task.yesterDailyScored;

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

@@ -105,7 +105,8 @@ const requiredGroupFields = '_id leader tasksOrder name';
  *                                      for "Good habits"-
  * @apiParam (Body) {Boolean} [down=true] Only valid for type "habit" If true, enables
  *                                        the "-" under "Directions/Action" for "Bad habits"
- * @apiParam (Body) {Number} [value=0] Only valid for type "reward." The cost in gold of the reward
+ * @apiParam (Body) {Number} [value=0] Only valid for type "reward." The cost
+ *                                     in gold of the reward. Should be greater then or equal to 0.
  *
  * @apiParamExample {json} Request-Example:
  *     {
@@ -174,6 +175,8 @@ const requiredGroupFields = '_id leader tasksOrder name';
  *                                                     underscores and dashes.
  * @apiError (400) {BadRequest} Value-ValidationFailed `x` is not a valid enum value
  *                                                     for path `(body param)`.
+ * @apiError (400) {BadRequest} Value-ValidationFailed Reward cost should be a
+ *                                                      positive number or 0.`.
  * @apiError (401) {NotAuthorized} NoAccount There is no account that uses those credentials.
  *
  * @apiErrorExample {json} Error-Response:

website/server/models/task.js

@@ -96,7 +96,17 @@ export const TaskSchema = new Schema({
     validate: [v => validator.isUUID(v), 'Invalid uuid for task tags.'],
   }],
   // redness or cost for rewards Required because it must be settable (for rewards)
-  value: { $type: Number, default: 0, required: true },
+  value: {
+    $type: Number,
+    default: 0,
+    required: true,
+    validate: {
+      validator (value) {
+        return this.type === 'reward' ? value >= 0 : true;
+      },
+      msg: 'Reward cost should be a positive number or 0.',
+    },
+  },
   priority: {
     $type: Number,
     default: 1,