Merge pull request #8558 from HabitRPG/fix-achievements

Attempt to fix achievements awarding

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

@@ -210,4 +210,47 @@ describe('User Model', () => {
       expect(user.hasNotCancelled()).to.be.false;
     });
   });
+
+  context('pre-save hook', () => {
+    it('does not try to award achievements when achievements or items not selected in query', async () => {
+      let user = new User();
+      user = await user.save(); // necessary for user.isSelected to work correctly
+
+      // Create conditions for the Beast Master achievement to be awarded
+      user.achievements.beastMasterCount = 3;
+      expect(user.achievements.beastMaster).to.not.equal(true); // verify that it was not awarded initially
+
+      user = await user.save();
+      // verify that it's been awarded
+      expect(user.achievements.beastMaster).to.equal(true);
+
+      // reset the user
+      user.achievements.beastMasterCount = 0;
+      user.achievements.beastMaster = false;
+
+      user = await user.save();
+      // verify it's been removed
+      expect(user.achievements.beastMaster).to.equal(false);
+
+      // fetch the user without selecting the 'items' field
+      user = await User.findById(user._id).select('-items').exec();
+      expect(user.isSelected('items')).to.equal(false);
+
+      // create the conditions for the beast master achievement but this time it should not be awarded
+      user.achievements.beastMasterCount = 3;
+      user = await user.save();
+      expect(user.achievements.beastMaster).to.equal(false);
+
+      // reset
+      user.achievements.beastMasterCount = 0;
+      user = await user.save();
+
+      // this time with achievements not selected
+      user = await User.findById(user._id).select('-achievements').exec();
+      expect(user.isSelected('achievements')).to.equal(false);
+      user.achievements.beastMasterCount = 3;
+      user = await user.save();
+      expect(user.achievements.beastMaster).to.not.equal(true);
+    });
+  });
 });

website/server/models/user/hooks.js

@@ -153,31 +153,51 @@ schema.pre('validate', function preValidateUser (next) {
 schema.pre('save', true, function preSaveUser (next, done) {
   next();
 
-  if (_.isNaN(this.preferences.dayStart) || this.preferences.dayStart < 0 || this.preferences.dayStart > 23) {
-    this.preferences.dayStart = 0;
-  }
+  // VERY IMPORTANT NOTE: when only some fields from an user document are selected
+  // using `.select('field1 field2')` when the user is saved we must make sure that
+  // these hooks do not run using default data. For example if user.items is missing
+  // we do not want to run any hook that relies on user.items because it will
+  // use the default values defined in the user schema and not the real ones.
+  //
+  // To check if a field was selected Document.isSelected('field') can be used.
+  // more info on its usage can be found at http://mongoosejs.com/docs/api.html#document_Document-isSelected
+  // IMPORTANT NOTE2 : due to a bug in mongoose (https://github.com/Automattic/mongoose/issues/5063)
+  // document.isSelected('items') will return true even if only a sub field (like 'items.mounts')
+  // was selected. So this fix only works as long as the entire subdoc is selected
+  // For example in the code below it won't work if only `achievements.beastMasterCount` is selected
+  // which is why we should only ever select the full paths and not subdocs,
+  // or if we really have to do the document.isSelected() calls should check for
+  // every specific subpath (items.mounts, items.pets, ...) but it's better to avoid it
+  // since it'll break as soon as a new field is added to the schema but not here.
 
-  // Determines if Beast Master should be awarded
-  let beastMasterProgress = shared.count.beastMasterProgress(this.items.pets);
+  // do not calculate achievements if items or achievements are not selected
+  if (this.isSelected('items') && this.isSelected('achievements')) {
+    // Determines if Beast Master should be awarded
+    let beastMasterProgress = shared.count.beastMasterProgress(this.items.pets);
 
-  if (beastMasterProgress >= 90 || this.achievements.beastMasterCount > 0) {
-    this.achievements.beastMaster = true;
-  }
+    if (beastMasterProgress >= 90 || this.achievements.beastMasterCount > 0) {
+      this.achievements.beastMaster = true;
+    }
 
-  // Determines if Mount Master should be awarded
-  let mountMasterProgress = shared.count.mountMasterProgress(this.items.mounts);
+    // Determines if Mount Master should be awarded
+    let mountMasterProgress = shared.count.mountMasterProgress(this.items.mounts);
 
-  if (mountMasterProgress >= 90 || this.achievements.mountMasterCount > 0) {
-    this.achievements.mountMaster = true;
-  }
+    if (mountMasterProgress >= 90 || this.achievements.mountMasterCount > 0) {
+      this.achievements.mountMaster = true;
+    }
 
-  // Determines if Triad Bingo should be awarded
+    // Determines if Triad Bingo should be awarded
+    let dropPetCount = shared.count.dropPetsCurrentlyOwned(this.items.pets);
+    let qualifiesForTriad = dropPetCount >= 90 && mountMasterProgress >= 90;
 
-  let dropPetCount = shared.count.dropPetsCurrentlyOwned(this.items.pets);
-  let qualifiesForTriad = dropPetCount >= 90 && mountMasterProgress >= 90;
+    if (qualifiesForTriad || this.achievements.triadBingoCount > 0) {
+      this.achievements.triadBingo = true;
+    }
 
-  if (qualifiesForTriad || this.achievements.triadBingoCount > 0) {
-    this.achievements.triadBingo = true;
+    // EXAMPLE CODE for allowing all existing and new players to be
+    // automatically granted an item during a certain time period:
+    // if (!this.items.pets['JackOLantern-Base'] && moment().isBefore('2014-11-01'))
+    // this.items.pets['JackOLantern-Base'] = 5;
   }
 
   // Enable weekly recap emails for old users who sign in
@@ -188,10 +208,9 @@ schema.pre('save', true, function preSaveUser (next, done) {
     this.flags.lastWeeklyRecapDiscriminator = undefined;
   }
 
-  // EXAMPLE CODE for allowing all existing and new players to be
-  // automatically granted an item during a certain time period:
-  // if (!this.items.pets['JackOLantern-Base'] && moment().isBefore('2014-11-01'))
-  // this.items.pets['JackOLantern-Base'] = 5;
+  if (_.isNaN(this.preferences.dayStart) || this.preferences.dayStart < 0 || this.preferences.dayStart > 23) {
+    this.preferences.dayStart = 0;
+  }
 
   // our own version incrementer
   if (_.isNaN(this._v) || !_.isNumber(this._v)) this._v = 0;