notifications: fixes

test/api/v3/unit/middlewares/response.js

@@ -90,8 +90,15 @@ describe('response middleware', () => {
   });
 
   it('returns notifications if a user is authenticated', () => {
-    res.locals.user.notifications.push({type: 'NEW_CONTRIBUTOR_LEVEL'});
-    let notification = res.locals.user.notifications[0].toJSON();
+    const user = res.locals.user;
+
+    user.notifications = [
+      null, // invalid, not an object
+      {seen: true}, // invalid, no type or id
+      {id: 123}, // invalid, no type
+      // {type: 'ABC'}, // invalid, no id, not included here because the id would be added automatically
+      {type: 'ABC', id: '123'}, // valid
+    ];
 
     responseMiddleware(req, res, next);
     res.respond(200, {field: 1});
@@ -103,8 +110,8 @@ describe('response middleware', () => {
       data: {field: 1},
       notifications: [
         {
-          type: notification.type,
-          id: notification.id,
+          type: 'ABC',
+          id: '123',
           data: {},
           seen: false,
         },

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

@@ -64,6 +64,25 @@ describe('User Model', () => {
       expect(userToJSON.notifications[0].seen).to.eql(false);
     });
 
+    it('removes invalid notifications when calling toJSON', () => {
+      let user = new User();
+
+      user.notifications = [
+        null, // invalid, not an object
+        {seen: true}, // invalid, no type or id
+        {id: 123}, // invalid, no type
+        // {type: 'ABC'}, // invalid, no id, not included here because the id would be added automatically
+        {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', () => {
       let user = new User();
 
@@ -78,7 +97,7 @@ describe('User Model', () => {
     });
 
     context('static push method', () => {
-      it('adds notifications for a single member via static method', async() => {
+      it('adds notifications for a single member via static method', async () => {
         let user = new User();
         await user.save();
 
@@ -93,7 +112,7 @@ describe('User Model', () => {
         expect(userToJSON.notifications[0].data).to.eql({});
       });
 
-      it('validates notifications via static method', async() => {
+      it('validates notifications via static method', async () => {
         let user = new User();
         await user.save();
 
@@ -127,7 +146,7 @@ describe('User Model', () => {
         expect(userToJSON.notifications[0].seen).to.eql(false);
       });
 
-      it('adds notifications with data and seen status for all given users via static method', async() => {
+      it('adds notifications with data and seen status for all given users via static method', async () => {
         let user = new User();
         let otherUser = new User();
         await Bluebird.all([user.save(), otherUser.save()]);

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

@@ -0,0 +1,21 @@
+import { model as UserNotification } from '../../../../../website/server/models/userNotification';
+
+describe('UserNotification Model', () => {
+  context('convertNotificationsToSafeJson', () => {
+    it('converts an array of notifications to a safe version', () => {
+      const notifications = [
+        null, // invalid, not an object
+        {seen: true}, // invalid, no type or id
+        {id: 123}, // invalid, no type
+        {type: 'ABC'}, // invalid, no id
+        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');
+    });
+  });
+});

website/common/script/ops/openMysteryItem.js

@@ -7,7 +7,7 @@ import cloneDeep from 'lodash/cloneDeep';
 
 function markNotificationAsRead (user) {
   const index = user.notifications.findIndex(notification => {
-    return notification.type === 'NEW_MYSTERY_ITEMS';
+    return notification && notification.type === 'NEW_MYSTERY_ITEMS';
   });
 
   if (index !== -1) user.notifications.splice(index, 1);

website/common/script/ops/readCard.js

@@ -11,7 +11,9 @@ import content from '../content/index';
 function markNotificationAsRead (user, cardType) {
   const indexToRemove = user.notifications.findIndex(notification => {
     if (
+      notification &&
       notification.type === 'CARD_RECEIVED' &&
+      notification.data &&
       notification.data.card === cardType
     ) return true;
   });

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

@@ -502,7 +502,7 @@ api.seenChat = {
 
     // Remove from response
     user.notifications = user.notifications.filter(n => {
-      if (n.type === 'NEW_CHAT_MESSAGE' && n.data.group.id === groupId) {
+      if (n && n.type === 'NEW_CHAT_MESSAGE' && n.data.group.id === groupId) {
         return false;
       }
 

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

@@ -703,7 +703,7 @@ function _removeMessagesFromMember (member, groupId) {
   }
 
   member.notifications = member.notifications.filter(n => {
-    if (n.type === 'NEW_CHAT_MESSAGE' && n.data.group &&  n.data.group.id === groupId) {
+    if (n && n.type === 'NEW_CHAT_MESSAGE' && n.date && n.data.group && n.data.group.id === groupId) {
       return false;
     }
 
@@ -1277,7 +1277,7 @@ api.removeGroupManager = {
 
     let manager = await User.findById(managerId, 'notifications').exec();
     let newNotifications = manager.notifications.filter((notification) => {
-      const isGroupTaskNotification = notification.type.indexOf('GROUP_TASK_') === 0;
+      const isGroupTaskNotification = notification && notification.type && notification.type.indexOf('GROUP_TASK_') === 0;
 
       return !isGroupTaskNotification;
     });

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

@@ -92,7 +92,7 @@ api.tellMeLaterNews = {
     user.flags.newStuff = false;
 
     const existingNotificationIndex = user.notifications.findIndex(n => {
-      return n.type === 'NEW_STUFF';
+      return n && n.type === 'NEW_STUFF';
     });
     if (existingNotificationIndex !== -1) user.notifications.splice(existingNotificationIndex, 1);
     user.addNotification('NEW_STUFF', { title: LAST_ANNOUNCEMENT_TITLE }, true); // seen by default

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

@@ -5,6 +5,9 @@ import {
 import {
   model as User,
 } from '../../models/user';
+import {
+  model as UserNotification,
+} from '../../models/userNotification';
 
 let api = {};
 
@@ -30,7 +33,7 @@ api.readNotification = {
     if (validationErrors) throw validationErrors;
 
     const index = user.notifications.findIndex(n => {
-      return n.id === req.params.notificationId;
+      return n && n.id === req.params.notificationId;
     });
 
     if (index === -1) {
@@ -48,7 +51,7 @@ api.readNotification = {
       $pull: { notifications: { id: req.params.notificationId } },
     }).exec();
 
-    res.respond(200, user.notifications);
+    res.respond(200, UserNotification.convertNotificationsToSafeJson(user.notifications));
   },
 };
 
@@ -74,7 +77,7 @@ api.readNotifications = {
     let notificationsIds = req.body.notificationIds;
     for (let notificationId of notificationsIds) {
       const index = user.notifications.findIndex(n => {
-        return n.id === notificationId;
+        return n && n.id === notificationId;
       });
 
       if (index === -1) {
@@ -93,7 +96,7 @@ api.readNotifications = {
     // See https://github.com/HabitRPG/habitica/pull/9321#issuecomment-354187666 for more info
     user._v++;
 
-    res.respond(200, user.notifications);
+    res.respond(200, UserNotification.convertNotificationsToSafeJson(user.notifications));
   },
 };
 
@@ -122,7 +125,7 @@ api.seeNotification = {
     const notificationId = req.params.notificationId;
 
     const notification = user.notifications.find(n => {
-      return n.id === notificationId;
+      return n && n.id === notificationId;
     });
 
     if (!notification) {
@@ -172,7 +175,7 @@ api.seeNotifications = {
 
     for (let notificationId of notificationsIds) {
       const notification = user.notifications.find(n => {
-        return n.id === notificationId;
+        return n && n.id === notificationId;
       });
 
       if (!notification) {
@@ -184,7 +187,7 @@ api.seeNotifications = {
 
     await user.save();
 
-    res.respond(200, user.notifications);
+    res.respond(200, UserNotification.convertNotificationsToSafeJson(user.notifications));
   },
 };
 

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

@@ -1,4 +1,3 @@
-import findIndex from 'lodash/findIndex';
 import { authWithHeaders } from '../../../middlewares/auth';
 import Bluebird from 'bluebird';
 import * as Tasks from '../../../models/task';
@@ -316,8 +315,8 @@ api.approveTask = {
 
     // Get task direction
     const firstManagerNotifications = managers[0].notifications;
-    const firstNotificationIndex =  findIndex(firstManagerNotifications, (notification) => {
-      return notification.data.taskId === task._id && notification.type === 'GROUP_TASK_APPROVAL';
+    const firstNotificationIndex = firstManagerNotifications.findIndex((notification) => {
+      return notification && notification.data && notification.data.taskId === task._id && notification.type === 'GROUP_TASK_APPROVAL';
     });
     let direction = 'up';
     if (firstManagerNotifications[firstNotificationIndex]) {
@@ -327,8 +326,8 @@ api.approveTask = {
     // Remove old notifications
     let managerPromises = [];
     managers.forEach((manager) => {
-      let notificationIndex = findIndex(manager.notifications, function findNotification (notification) {
-        return notification.data.taskId === task._id && notification.type === 'GROUP_TASK_APPROVAL';
+      let notificationIndex = manager.notifications.findIndex(function findNotification (notification) {
+        return notification && notification.data && notification.data.taskId === task._id && notification.type === 'GROUP_TASK_APPROVAL';
       });
 
       if (notificationIndex !== -1) {
@@ -416,8 +415,8 @@ api.taskNeedsWork = {
 
     // Remove old notifications
     managers.forEach((manager) => {
-      let notificationIndex = findIndex(manager.notifications, function findNotification (notification) {
-        return notification.data.taskId === task._id && notification.type === 'GROUP_TASK_APPROVAL';
+      let notificationIndex = manager.notifications.findIndex(function findNotification (notification) {
+        return notification && notification.data && notification.data.taskId === task._id && notification.type === 'GROUP_TASK_APPROVAL';
       });
 
       if (notificationIndex !== -1) {

website/server/libs/cron.js

@@ -181,11 +181,9 @@ function awardLoginIncentives (user) {
   if (!loginIncentives[user.loginIncentives].rewardKey && user._ABtests && user._ABtests.checkInModals === '20161221_noCheckInPreviews') return;
 
   //  Remove old notifications if they exists
-  user.notifications
-    .toObject()
-    .forEach((notif, index) => {
-      if (notif.type === 'LOGIN_INCENTIVE') user.notifications.splice(index, 1);
-    });
+  user.notifications.forEach((notif, index) => {
+    if (notif && notif.type === 'LOGIN_INCENTIVE') user.notifications.splice(index, 1);
+  });
 
   let notificationData = {};
   notificationData.message = i18n.t('checkinEarned', user.preferences.language);
@@ -442,8 +440,8 @@ export function cron (options = {}) {
 
   // First remove a possible previous cron notification
   // we don't want to flood the users with many cron notifications at once
-  let oldCronNotif = user.notifications.toObject().find((notif, index) => {
-    if (notif.type === 'CRON') {
+  let oldCronNotif = user.notifications.find((notif, index) => {
+    if (notif && notif.type === 'CRON') {
       user.notifications.splice(index, 1);
       return true;
     } else {

website/server/middlewares/response.js

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

website/server/models/user/hooks.js

@@ -4,6 +4,9 @@ import moment from 'moment';
 import Bluebird from 'bluebird';
 import baseModel from '../../libs/baseModel';
 import * as Tasks from '../task';
+import {
+  model as UserNotification,
+} from '../userNotification';
 
 import schema from './schema';
 
@@ -15,6 +18,8 @@ schema.plugin(baseModel, {
     plainObj._tmp = originalDoc._tmp; // be sure to send down drop notifs
     delete plainObj.filters;
 
+    plainObj.notifications = UserNotification.convertNotificationsToSafeJson(originalDoc.notifications);
+
     return plainObj;
   },
 });
@@ -255,7 +260,7 @@ schema.pre('save', true, function preSaveUser (next, done) {
 
     // Sometimes there can be more than 1 notification
     const existingNotifications = this.notifications.filter(notification => {
-      return notification.type === 'UNALLOCATED_STATS_POINTS';
+      return notification && notification.type === 'UNALLOCATED_STATS_POINTS';
     });
 
     const existingNotificationsLength = existingNotifications.length;
@@ -277,7 +282,7 @@ schema.pre('save', true, function preSaveUser (next, done) {
       let notificationsRemoved = 0;
 
       this.notifications = this.notifications.filter(notification => {
-        if (notification.type !== 'UNALLOCATED_STATS_POINTS') return true;
+        if (notification && notification.type !== 'UNALLOCATED_STATS_POINTS') return true;
         if (notificationsRemoved === notificationsToRemove) return true;
 
         notificationsRemoved++;

website/server/models/user/methods.js

@@ -163,10 +163,12 @@ schema.methods.addNotification = function addUserNotification (type, data = {},
  */
 schema.statics.pushNotification = async function pushNotification (query, type, data = {}, seen = false) {
   let newNotification = new UserNotification({type, data, seen});
+
   let validationResult = newNotification.validateSync();
   if (validationResult) {
     throw validationResult;
   }
+
   await this.update(query, {$push: {notifications: newNotification.toObject()}}, {multi: true}).exec();
 };
 

website/server/models/userNotification.js

@@ -38,10 +38,14 @@ export let schema = new Schema({
     type: String,
     default: uuid,
     validate: [validator.isUUID, 'Invalid uuid.'],
-    // required: true, // @TODO: Add these back once we figure out the issue with notifications
+    // @TODO: Add these back once we figure out the issue with notifications
+    // See Fix for https://github.com/HabitRPG/habitica/issues/9923
+    // 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,
     enum: NOTIFICATION_TYPES,
   },
@@ -60,6 +64,24 @@ export let schema = new Schema({
   _id: false, // use id instead of _id
 });
 
+/**
+ * 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
+ */
+schema.statics.convertNotificationsToSafeJson = function convertNotificationsToSafeJson (notifications) {
+  return 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;
+    return true;
+  }).map(n => {
+    return n.toJSON();
+  });
+};
+
 schema.plugin(baseModel, {
   noSet: ['_id', 'id'],
   // timestamps: true, // Temporarily removed to debug a possible bug