Merge pull request #11916 from HabitRPG/fix-corrupt-data

Fix corrupt data

test/api/unit/middlewares/response.js

@@ -34,7 +34,7 @@ describe('response middleware', () => {
     expect(res.json).to.be.calledWith({
       success: true,
       data: { field: 1 },
-      notifications: [],
+      notifications: res.locals.user.notifications,
       userV: res.locals.user._v,
       appVersion: packageInfo.version,
     });
@@ -52,7 +52,7 @@ describe('response middleware', () => {
       success: true,
       data: { field: 1 },
       message: 'hello',
-      notifications: [],
+      notifications: res.locals.user.notifications,
       userV: res.locals.user._v,
       appVersion: packageInfo.version,
     });
@@ -69,7 +69,7 @@ describe('response middleware', () => {
     expect(res.json).to.be.calledWith({
       success: false,
       data: { field: 1 },
-      notifications: [],
+      notifications: res.locals.user.notifications,
       userV: res.locals.user._v,
       appVersion: packageInfo.version,
     });
@@ -84,42 +84,9 @@ describe('response middleware', () => {
     expect(res.json).to.be.calledWith({
       success: true,
       data: { field: 1 },
-      notifications: [],
+      notifications: res.locals.user.notifications,
       userV: 0,
       appVersion: packageInfo.version,
     });
   });
-
-  it('returns notifications if a user is authenticated', () => {
-    const { user } = res.locals;
-
-    user.notifications = [
-      null, // invalid, not an object
-      { seen: true }, // invalid, no type or id
-      { id: 123 }, // invalid, no type
-      // invalid, no id, not included here because the id would be added automatically
-      // {type: 'ABC'},
-      { type: 'ABC', id: '123' }, // valid
-    ];
-
-    responseMiddleware(req, res, next);
-    res.respond(200, { field: 1 });
-
-    expect(res.json).to.be.calledOnce;
-
-    expect(res.json).to.be.calledWith({
-      success: true,
-      data: { field: 1 },
-      notifications: [
-        {
-          type: 'ABC',
-          id: '123',
-          data: {},
-          seen: false,
-        },
-      ],
-      userV: res.locals.user._v,
-      appVersion: packageInfo.version,
-    });
-  });
 });

test/api/unit/models/pushDevice.test.js

@@ -0,0 +1,33 @@
+import { model as PushDevice } from '../../../../website/server/models/pushDevice';
+
+describe('PushDevice Model', () => {
+  context('cleanupCorruptData', () => {
+    it('converts an array of push devices to a safe version', () => {
+      const pushDevices = [
+        null, // invalid, not an object
+        { regId: '123' }, // invalid, no type
+        { type: 'android' }, // invalid, no regId
+        new PushDevice({ type: 'android', regId: '1234' }), // valid
+      ];
+
+      const safePushDevices = PushDevice.cleanupCorruptData(pushDevices);
+      expect(safePushDevices.length).to.equal(1);
+      expect(safePushDevices[0].type).to.equal('android');
+      expect(safePushDevices[0].regId).to.equal('1234');
+    });
+
+    it('removes duplicates', () => {
+      const pushDevices = [
+        new PushDevice({ type: 'android', regId: '1234' }),
+        new PushDevice({ type: 'android', regId: '1234' }),
+        new PushDevice({ type: 'iphone', regId: '1234' }), // not duplicate
+        new PushDevice({ type: 'android', regId: '12345' }), // not duplicate
+      ];
+
+      const safePushDevices = PushDevice.cleanupCorruptData(pushDevices);
+      expect(safePushDevices.length).to.equal(3);
+      expect(safePushDevices[0].type).to.equal('android');
+      expect(safePushDevices[0].regId).to.equal('1234');
+    });
+  });
+});

test/api/unit/models/tag.test.js

@@ -0,0 +1,19 @@
+import { model as Tag } from '../../../../website/server/models/tag';
+
+describe('Tag Model', () => {
+  context('cleanupCorruptData', () => {
+    it('converts an array of tags to a safe version', () => {
+      const tags = [
+        null, // invalid, not an object
+        { name: '123' }, // invalid, no id
+        { id: '123' }, // invalid, no name
+        new Tag({ name: 'ABC', id: 123 }), // valid
+      ];
+
+      const safetags = Tag.cleanupCorruptData(tags);
+      expect(safetags.length).to.equal(1);
+      expect(safetags[0].name).to.equal('ABC');
+      expect(safetags[0].id).to.equal('123');
+    });
+  });
+});

test/api/unit/models/user.test.js

@@ -181,6 +181,146 @@ describe('User Model', () => {
     });
   });
 
+  context('post init', () => {
+    it('removes invalid tags when loading the user', async () => {
+      let user = new User();
+      await user.save();
+      await user.update({
+        $set: {
+          tags: [
+            null, // invalid, not an object
+            // { name: '123' }, // invalid, no id - generated automatically
+            { id: '123' }, // invalid, no name
+            { name: 'ABC', id: '1234' }, // valid
+          ],
+        },
+      }).exec();
+
+      user = await User.findById(user._id).exec();
+
+      const userToJSON = user.toJSON();
+      expect(userToJSON.tags.length).to.equal(1);
+
+      expect(userToJSON.tags[0]).to.have.all.keys(['id', 'name']);
+      expect(userToJSON.tags[0].id).to.equal('1234');
+      expect(userToJSON.tags[0].name).to.equal('ABC');
+    });
+
+    it('removes invalid push devices when loading the user', async () => {
+      let user = new User();
+      await user.save();
+      await user.update({
+        $set: {
+          pushDevices: [
+            null, // invalid, not an object
+            { regId: '123' }, // invalid, no type
+            { type: 'android' }, // invalid, no regId
+            { type: 'android', regId: '1234' }, // valid
+          ],
+        },
+      }).exec();
+
+      user = await User.findById(user._id).exec();
+
+      const userToJSON = user.toJSON();
+      expect(userToJSON.pushDevices.length).to.equal(1);
+
+      expect(userToJSON.pushDevices[0]).to.have.all.keys(['regId', 'type', 'createdAt', 'updatedAt']);
+      expect(userToJSON.pushDevices[0].type).to.equal('android');
+      expect(userToJSON.pushDevices[0].regId).to.equal('1234');
+    });
+
+    it('removes duplicate push devices when loading the user', async () => {
+      let user = new User();
+      await user.save();
+      await user.update({
+        $set: {
+          pushDevices: [
+            { type: 'android', regId: '1234' },
+            { type: 'android', regId: '1234' },
+          ],
+        },
+      }).exec();
+
+      user = await User.findById(user._id).exec();
+
+      const userToJSON = user.toJSON();
+      expect(userToJSON.pushDevices.length).to.equal(1);
+
+      expect(userToJSON.pushDevices[0]).to.have.all.keys(['regId', 'type', 'createdAt', 'updatedAt']);
+      expect(userToJSON.pushDevices[0].type).to.equal('android');
+      expect(userToJSON.pushDevices[0].regId).to.equal('1234');
+    });
+
+    it('removes invalid notifications when loading the user', async () => {
+      let user = new User();
+      await user.save();
+      await user.update({
+        $set: {
+          notifications: [
+            null, // invalid, not an object
+            { seen: true }, // invalid, no type or id
+            { id: 123 }, // invalid, no type
+            // invalid, no id, not included here because the id would be added automatically
+            // {type: 'ABC'},
+            { type: 'ABC', id: '123' }, // valid
+          ],
+        },
+      }).exec();
+
+      user = await User.findById(user._id).exec();
+
+      const userToJSON = user.toJSON();
+      expect(userToJSON.notifications.length).to.equal(1);
+
+      expect(userToJSON.notifications[0]).to.have.all.keys(['data', 'id', 'type', 'seen']);
+      expect(userToJSON.notifications[0].type).to.equal('ABC');
+      expect(userToJSON.notifications[0].id).to.equal('123');
+    });
+
+    it('removes multiple NEW_CHAT_MESSAGE for the same group', async () => {
+      let user = new User();
+      await user.save();
+      await user.update({
+        $set: {
+          notifications: [
+            {
+              type: 'NEW_CHAT_MESSAGE',
+              id: 123,
+              data: { group: { id: 12345 } },
+            },
+            {
+              type: 'NEW_CHAT_MESSAGE',
+              id: 1234,
+              data: { group: { id: 12345 } },
+            },
+            {
+              type: 'NEW_CHAT_MESSAGE',
+              id: 123,
+              data: { group: { id: 123456 } },
+            }, // not duplicate, different group
+            {
+              type: 'NEW_CHAT_MESSAGE_DIFF',
+              id: 123,
+              data: { group: { id: 12345 } },
+            }, // not duplicate, different type
+          ],
+        },
+      }).exec();
+
+      user = await User.findById(user._id).exec();
+
+      const userToJSON = user.toJSON();
+      expect(userToJSON.notifications.length).to.equal(3);
+
+      expect(userToJSON.notifications[0]).to.have.all.keys(['data', 'id', 'type', 'seen']);
+      expect(userToJSON.notifications[0].type).to.equal('NEW_CHAT_MESSAGE');
+      expect(userToJSON.notifications[0].id).to.equal('123');
+      expect(userToJSON.notifications[0].data).to.deep.equal({ group: { id: 12345 } });
+      expect(userToJSON.notifications[0].seen).to.equal(false);
+    });
+  });
+
   context('notifications', () => {
     it('can add notifications without data', () => {
       const user = new User();
@@ -195,26 +335,6 @@ describe('User Model', () => {
       expect(userToJSON.notifications[0].seen).to.eql(false);
     });
 
-    it('removes invalid notifications when calling toJSON', () => {
-      const user = new User();
-
-      user.notifications = [
-        null, // invalid, not an object
-        { seen: true }, // invalid, no type or id
-        { id: 123 }, // invalid, no type
-        // invalid, no id, not included here because the id would be added automatically
-        // {type: 'ABC'},
-        { type: 'ABC', id: '123' }, // valid
-      ];
-
-      const userToJSON = user.toJSON();
-      expect(userToJSON.notifications.length).to.equal(1);
-
-      expect(userToJSON.notifications[0]).to.have.all.keys(['data', 'id', 'type', 'seen']);
-      expect(userToJSON.notifications[0].type).to.equal('ABC');
-      expect(userToJSON.notifications[0].id).to.equal('123');
-    });
-
     it('can add notifications with data and already marked as seen', () => {
       const user = new User();
 

test/api/unit/models/userNotification.test.js

@@ -1,7 +1,7 @@
 import { model as UserNotification } from '../../../../website/server/models/userNotification';
 
 describe('UserNotification Model', () => {
-  context('convertNotificationsToSafeJson', () => {
+  context('cleanupCorruptData', () => {
     it('converts an array of notifications to a safe version', () => {
       const notifications = [
         null, // invalid, not an object
@@ -11,11 +11,44 @@ describe('UserNotification Model', () => {
         new UserNotification({ type: 'ABC', id: 123 }), // valid
       ];
 
-      const notificationsToJSON = UserNotification.convertNotificationsToSafeJson(notifications);
-      expect(notificationsToJSON.length).to.equal(1);
-      expect(notificationsToJSON[0]).to.have.all.keys(['data', 'id', 'type', 'seen']);
-      expect(notificationsToJSON[0].type).to.equal('ABC');
-      expect(notificationsToJSON[0].id).to.equal('123');
+      const safeNotifications = UserNotification.cleanupCorruptData(notifications);
+      expect(safeNotifications.length).to.equal(1);
+      expect(safeNotifications[0].data).to.deep.equal({});
+      expect(safeNotifications[0].seen).to.equal(false);
+      expect(safeNotifications[0].type).to.equal('ABC');
+      expect(safeNotifications[0].id).to.equal('123');
+    });
+
+    it('removes multiple NEW_CHAT_MESSAGE for the same group', () => {
+      const notifications = [
+        new UserNotification({
+          type: 'NEW_CHAT_MESSAGE',
+          id: 123,
+          data: { group: { id: 12345 } },
+        }),
+        new UserNotification({
+          type: 'NEW_CHAT_MESSAGE',
+          id: 1234,
+          data: { group: { id: 12345 } },
+        }),
+        new UserNotification({
+          type: 'NEW_CHAT_MESSAGE',
+          id: 123,
+          data: { group: { id: 123456 } },
+        }), // not duplicate, different group
+        new UserNotification({
+          type: 'NEW_CHAT_MESSAGE_DIFF',
+          id: 123,
+          data: { group: { id: 12345 } },
+        }), // not duplicate, different type
+      ];
+
+      const safeNotifications = UserNotification.cleanupCorruptData(notifications);
+      expect(safeNotifications.length).to.equal(3);
+      expect(safeNotifications[0].data).to.deep.equal({ group: { id: 12345 } });
+      expect(safeNotifications[0].seen).to.equal(false);
+      expect(safeNotifications[0].type).to.equal('NEW_CHAT_MESSAGE');
+      expect(safeNotifications[0].id).to.equal('123');
     });
   });
 });

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

@@ -5,9 +5,6 @@ import {
 import {
   model as User,
 } from '../../models/user';
-import {
-  model as UserNotification,
-} from '../../models/userNotification';
 
 const api = {};
 
@@ -49,7 +46,7 @@ api.readNotification = {
       $pull: { notifications: { id: req.params.notificationId } },
     }).exec();
 
-    res.respond(200, UserNotification.convertNotificationsToSafeJson(user.notifications));
+    res.respond(200, user.notifications);
   },
 };
 
@@ -92,7 +89,7 @@ api.readNotifications = {
     // See https://github.com/HabitRPG/habitica/pull/9321#issuecomment-354187666 for more info
     user._v += 1;
 
-    res.respond(200, UserNotification.convertNotificationsToSafeJson(user.notifications));
+    res.respond(200, user.notifications);
   },
 };
 
@@ -181,7 +178,7 @@ api.seeNotifications = {
 
     await user.save();
 
-    res.respond(200, UserNotification.convertNotificationsToSafeJson(user.notifications));
+    res.respond(200, user.notifications);
   },
 };
 

website/server/middlewares/response.js

@@ -1,7 +1,4 @@
 import packageInfo from '../../../package.json';
-import {
-  model as UserNotification,
-} from '../models/userNotification';
 
 export default function responseHandler (req, res, next) {
   // Only used for successful responses
@@ -16,7 +13,7 @@ export default function responseHandler (req, res, next) {
     if (message) response.message = message;
 
     if (user) {
-      response.notifications = UserNotification.convertNotificationsToSafeJson(user.notifications);
+      response.notifications = user.notifications;
       response.userV = user._v;
     }
 

website/server/models/pushDevice.js

@@ -1,4 +1,5 @@
 import mongoose from 'mongoose';
+import _ from 'lodash';
 import baseModel from '../libs/baseModel';
 
 const { Schema } = mongoose;
@@ -19,4 +20,30 @@ schema.plugin(baseModel, {
   _id: false,
 });
 
+/**
+ * Remove invalid data from an array of push devices.
+ * Fix for https://github.com/HabitRPG/habitica/issues/11805
+ * and https://github.com/HabitRPG/habitica/issues/11868
+ * Called by user's post init hook (models/user/hooks.js)
+ */
+schema.statics.cleanupCorruptData = function cleanupCorruptPushDevicesData (pushDevices) {
+  if (!pushDevices) return pushDevices;
+
+  let filteredPushDevices = pushDevices.filter(pushDevice => {
+    // Exclude push devices with a nullish value, no id or no type
+    if (!pushDevice || !pushDevice.regId || !pushDevice.type) return false;
+    return true;
+  });
+
+  // Remove duplicate push devices
+  // can be caused by a race condition when adding a new push device
+  filteredPushDevices = _.uniqWith(filteredPushDevices, (val, otherVal) => {
+    if (val.regId === otherVal.regId && val.type === otherVal.type) return true;
+    return false;
+  });
+
+  return filteredPushDevices;
+};
+
+
 export const model = mongoose.model('PushDevice', schema);

website/server/models/tag.js

@@ -33,4 +33,19 @@ schema.statics.sanitizeUpdate = function sanitizeUpdate (updateObj) {
   return this.sanitize(updateObj, noUpdate);
 };
 
+/**
+ * Remove invalid data from an array of tags.
+ * Fix for https://github.com/HabitRPG/habitica/issues/10688
+ * Called by user's post init hook (models/user/hooks.js)
+ */
+schema.statics.cleanupCorruptData = function cleanupCorruptTagsData (tags) {
+  if (!tags) return tags;
+
+  return tags.filter(tag => {
+    // Exclude tags with a nullish value or no id
+    if (!tag || !tag.id || !tag.name) return false;
+    return true;
+  });
+};
+
 export const model = mongoose.model('Tag', schema);

website/server/models/user/hooks.js

@@ -6,6 +6,12 @@ import * as Tasks from '../task';
 import {
   model as UserNotification,
 } from '../userNotification';
+import {
+  model as PushDevice,
+} from '../pushDevice';
+import {
+  model as Tag,
+} from '../tag';
 import { // eslint-disable-line import/no-cycle
   userActivityWebhook,
 } from '../../libs/webhook';
@@ -25,11 +31,6 @@ schema.plugin(baseModel, {
 
     delete plainObj.filters;
 
-    if (originalDoc.notifications) {
-      plainObj.notifications = UserNotification
-        .convertNotificationsToSafeJson(originalDoc.notifications);
-    }
-
     return plainObj;
   },
 });
@@ -182,6 +183,32 @@ function _setProfileName (user) {
   return localUsername || anonymous;
 }
 
+schema.post('init', function postInitUser () {
+  // Cleanup any corrupt data that could have ended up inside the user schema.
+  // In particular:
+  // - tags https://github.com/HabitRPG/habitica/issues/10688
+  // - notifications https://github.com/HabitRPG/habitica/issues/9923
+  // - push devices https://github.com/HabitRPG/habitica/issues/11805
+  //            and https://github.com/HabitRPG/habitica/issues/11868
+
+  // Make sure notifications are loaded
+  if (this.isDirectSelected('notifications')) {
+    this.notifications = UserNotification.cleanupCorruptData(this.notifications);
+  }
+
+  // Make sure pushDevices are loaded
+  if (this.isDirectSelected('pushDevices')) {
+    this.pushDevices = PushDevice.cleanupCorruptData(this.pushDevices);
+  }
+
+  // Make sure tags are loaded
+  if (this.isDirectSelected('tags')) {
+    this.tags = Tag.cleanupCorruptData(this.tags);
+  }
+
+  return true;
+});
+
 schema.pre('validate', function preValidateUser (next) {
   // Populate new user with profile name, not running in pre('save') because the field
   // is required and validation fails if it doesn't exists like for new users
@@ -258,11 +285,8 @@ schema.pre('save', true, function preSaveUser (next, done) {
     const unallocatedPointsNotifications = [];
 
     this.notifications = this.notifications.filter(notification => {
-      // Remove corrupt notifications
-      if (!notification || !notification.type) return false;
-
-      // Remove all unsallocated stats points
-      if (notification && notification.type === 'UNALLOCATED_STATS_POINTS') {
+      // Remove all unallocated stats points
+      if (notification.type === 'UNALLOCATED_STATS_POINTS') {
         unallocatedPointsNotifications.push(notification);
         return false;
       }

website/server/models/userNotification.js

@@ -63,15 +63,11 @@ export const schema = new Schema({
     $type: String,
     default: uuid,
     validate: [v => validator.isUUID(v), 'Invalid uuid for userNotification.'],
-    // @TODO: Add these back once we figure out the issue with notifications
-    // See Fix for https://github.com/HabitRPG/habitica/issues/9923
-    // required: true,
+    required: true,
   },
   type: {
     $type: String,
-    // @TODO: Add these back once we figure out the issue with notifications
-    // See Fix for https://github.com/HabitRPG/habitica/issues/9923
-    // required: true,
+    required: true,
     enum: NOTIFICATION_TYPES,
   },
   data: {
@@ -92,30 +88,30 @@ export const schema = new Schema({
 });
 
 /**
- * Convert notifications to JSON making sure to return only valid data.
- * Fix for https://github.com/HabitRPG/habitica/issues/9923#issuecomment-362869881
- * @TODO Remove once https://github.com/HabitRPG/habitica/issues/9923
- * is fixed
+ * Remove invalid data from an array of notifications.
+ * Fix for https://github.com/HabitRPG/habitica/issues/9923
+ * Called by user's post init hook (models/user/hooks.js)
  */
-schema.statics.convertNotificationsToSafeJson = function convNotifsToSafeJson (notifications) {
+schema.statics.cleanupCorruptData = function cleanupCorruptNotificationsData (notifications) {
   if (!notifications) return notifications;
 
-  let filteredNotifications = notifications.filter(n => {
-    // Exclude notifications with a nullish value
-    if (!n) return false;
-    // Exclude notifications without an id or a type
-    if (!n.id || !n.type) return false;
+  let filteredNotifications = notifications.filter(notification => {
+    // Exclude notifications with a nullish value, no id or no type
+    if (!notification || !notification.id || !notification.type) return false;
     return true;
   });
 
+  // Remove duplicate NEW_CHAT_MESSAGES notifications
+  // can be caused by a race condition when adding a new notification of this type
+  // in group.sendChat if two messages are posted at the same time
   filteredNotifications = _.uniqWith(filteredNotifications, (val, otherVal) => {
-    if (val.type === otherVal.type && val.type === 'NEW_CHAT_MESSAGE') {
+    if (val.type === 'NEW_CHAT_MESSAGE' && val.type === otherVal.type) {
       return val.data.group.id === otherVal.data.group.id;
     }
     return false;
   });
 
-  return filteredNotifications.map(n => n.toJSON());
+  return filteredNotifications;
 };
 
 schema.plugin(baseModel, {