diff --git a/test/api/unit/middlewares/response.js b/test/api/unit/middlewares/response.js index b8157287be..dbc689276a 100644 --- a/test/api/unit/middlewares/response.js +++ b/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, - }); - }); }); diff --git a/test/api/unit/models/pushDevice.test.js b/test/api/unit/models/pushDevice.test.js new file mode 100644 index 0000000000..6077a75113 --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/test/api/unit/models/tag.test.js b/test/api/unit/models/tag.test.js new file mode 100644 index 0000000000..1585dd11ad --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/test/api/unit/models/user.test.js b/test/api/unit/models/user.test.js index 711c4fd4b6..49ec464c96 100644 --- a/test/api/unit/models/user.test.js +++ b/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(); diff --git a/test/api/unit/models/userNotification.test.js b/test/api/unit/models/userNotification.test.js index c52a44bd21..772e8dba8f 100644 --- a/test/api/unit/models/userNotification.test.js +++ b/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'); }); }); }); diff --git a/website/server/controllers/api-v3/notifications.js b/website/server/controllers/api-v3/notifications.js index 16a0410511..8ded24d5eb 100644 --- a/website/server/controllers/api-v3/notifications.js +++ b/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); }, }; diff --git a/website/server/middlewares/response.js b/website/server/middlewares/response.js index 945cf19dea..3190214cd2 100644 --- a/website/server/middlewares/response.js +++ b/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; } diff --git a/website/server/models/pushDevice.js b/website/server/models/pushDevice.js index b84a16aac4..0fc4bd3f69 100644 --- a/website/server/models/pushDevice.js +++ b/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); diff --git a/website/server/models/tag.js b/website/server/models/tag.js index 1fa8bc59a7..94afd95bd8 100644 --- a/website/server/models/tag.js +++ b/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); diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js index 5b9e5fe8f2..63aa42ed57 100644 --- a/website/server/models/user/hooks.js +++ b/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; } diff --git a/website/server/models/userNotification.js b/website/server/models/userNotification.js index cbadd05a3a..63b550bf15 100644 --- a/website/server/models/userNotification.js +++ b/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, {