From 118e3580d62326df7f2f03a377433714d1142b03 Mon Sep 17 00:00:00 2001
From: Matteo Pagliazzi <matteopagliazzi@gmail.com>
Date: Sun, 1 Mar 2020 20:06:24 +0100
Subject: [PATCH] refactor notifications cleanup

---
 test/api/unit/models/user.test.js         | 22 +++++++++++++++++-
 website/server/models/user/hooks.js       | 28 +++++++++++++++--------
 website/server/models/userNotification.js | 24 +++++++++----------
 3 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/test/api/unit/models/user.test.js b/test/api/unit/models/user.test.js
index 711c4fd4b6..84a10e80e3 100644
--- a/test/api/unit/models/user.test.js
+++ b/test/api/unit/models/user.test.js
@@ -181,7 +181,7 @@ describe('User Model', () => {
     });
   });
 
-  context('notifications', () => {
+  context.only('notifications', () => {
     it('can add notifications without data', () => {
       const user = new User();
 
@@ -215,6 +215,26 @@ describe('User Model', () => {
       expect(userToJSON.notifications[0].id).to.equal('123');
     });
 
+    it('removes invalid notifications when saving', async () => {
+      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
+      ];
+
+      await user.save();
+      expect(user.notifications.length).to.equal(1);
+
+      expect(user.notifications[0]).to.have.all.keys(['data', 'id', 'type', 'seen']);
+      expect(user.notifications[0].type).to.equal('ABC');
+      expect(user.notifications[0].id).to.equal('123');
+    });
+
     it('can add notifications with data and already marked as seen', () => {
       const user = new User();
 
diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js
index 5b9e5fe8f2..35c47422f7 100644
--- a/website/server/models/user/hooks.js
+++ b/website/server/models/user/hooks.js
@@ -25,11 +25,6 @@ schema.plugin(baseModel, {
 
     delete plainObj.filters;
 
-    if (originalDoc.notifications) {
-      plainObj.notifications = UserNotification
-        .convertNotificationsToSafeJson(originalDoc.notifications);
-    }
-
     return plainObj;
   },
 });
@@ -182,6 +177,22 @@ 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);
+  }
+
+  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 +269,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 d5d160d255..63b550bf15 100644
--- a/website/server/models/userNotification.js
+++ b/website/server/models/userNotification.js
@@ -88,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, {