diff --git a/test/api/v3/unit/middlewares/response.js b/test/api/v3/unit/middlewares/response.js index 71a8bf3a22..88dcbb7cc8 100644 --- a/test/api/v3/unit/middlewares/response.js +++ b/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, }, diff --git a/test/api/v3/unit/models/user.test.js b/test/api/v3/unit/models/user.test.js index 4a7dd00b49..7305401d5a 100644 --- a/test/api/v3/unit/models/user.test.js +++ b/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()]); diff --git a/test/api/v3/unit/models/userNotification.test.js b/test/api/v3/unit/models/userNotification.test.js new file mode 100644 index 0000000000..0bd87f4bcb --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/website/common/script/ops/openMysteryItem.js b/website/common/script/ops/openMysteryItem.js index b9041be61b..4773565b89 100644 --- a/website/common/script/ops/openMysteryItem.js +++ b/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); diff --git a/website/common/script/ops/readCard.js b/website/common/script/ops/readCard.js index f7466b09eb..a651238af1 100644 --- a/website/common/script/ops/readCard.js +++ b/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; }); diff --git a/website/server/controllers/api-v3/chat.js b/website/server/controllers/api-v3/chat.js index 0565ed54aa..e66c3de062 100644 --- a/website/server/controllers/api-v3/chat.js +++ b/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; } diff --git a/website/server/controllers/api-v3/groups.js b/website/server/controllers/api-v3/groups.js index 7a608a9a9e..b23d240e75 100644 --- a/website/server/controllers/api-v3/groups.js +++ b/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; }); diff --git a/website/server/controllers/api-v3/news.js b/website/server/controllers/api-v3/news.js index 9c2e67f5c6..3364389795 100644 --- a/website/server/controllers/api-v3/news.js +++ b/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 diff --git a/website/server/controllers/api-v3/notifications.js b/website/server/controllers/api-v3/notifications.js index a6c0131f6b..114017c2d8 100644 --- a/website/server/controllers/api-v3/notifications.js +++ b/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)); }, }; diff --git a/website/server/controllers/api-v3/tasks/groups.js b/website/server/controllers/api-v3/tasks/groups.js index d1a5d1022e..2f1a1675d8 100644 --- a/website/server/controllers/api-v3/tasks/groups.js +++ b/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) { diff --git a/website/server/libs/cron.js b/website/server/libs/cron.js index 6cfbd8cc64..6c4cb90aeb 100644 --- a/website/server/libs/cron.js +++ b/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 { diff --git a/website/server/middlewares/response.js b/website/server/middlewares/response.js index 085a1c8087..420ba9e529 100644 --- a/website/server/middlewares/response.js +++ b/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; } diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js index 99673374a3..123db1aa4d 100644 --- a/website/server/models/user/hooks.js +++ b/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++; diff --git a/website/server/models/user/methods.js b/website/server/models/user/methods.js index fbe6ee6809..fab636972d 100644 --- a/website/server/models/user/methods.js +++ b/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(); }; diff --git a/website/server/models/userNotification.js b/website/server/models/userNotification.js index d19965a63d..9add87cc13 100644 --- a/website/server/models/userNotification.js +++ b/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