From f67e065de2c9536576ee12b4b396290ab6419f93 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Sat, 11 Mar 2017 10:28:51 +0100 Subject: [PATCH 1/3] attempt to fix achievements awarding --- website/server/models/user/hooks.js | 54 +++++++++++++++++------------ 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js index 1663d97cf5..8e570b5477 100644 --- a/website/server/models/user/hooks.js +++ b/website/server/models/user/hooks.js @@ -153,31 +153,42 @@ 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 - // 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 +199,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; From e0d499ababe36ee922388cb3a41bebcb1e8441f8 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Sat, 11 Mar 2017 10:54:43 +0100 Subject: [PATCH 2/3] expand explanation --- website/server/models/user/hooks.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js index 8e570b5477..f2a4389446 100644 --- a/website/server/models/user/hooks.js +++ b/website/server/models/user/hooks.js @@ -158,8 +158,17 @@ schema.pre('save', true, function preSaveUser (next, done) { // 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. // do not calculate achievements if items or achievements are not selected if (!this.isSelected('items') || !this.isSelected('achievements')) { From a8ebd04ac8aa08018dd9a3612c6a610e5a998ce9 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Mon, 13 Mar 2017 21:43:40 +0100 Subject: [PATCH 3/3] fix typo in if condition and write test --- test/api/v3/unit/models/user.test.js | 43 ++++++++++++++++++++++++++++ website/server/models/user/hooks.js | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/test/api/v3/unit/models/user.test.js b/test/api/v3/unit/models/user.test.js index 0192fe42ee..0f128cd296 100644 --- a/test/api/v3/unit/models/user.test.js +++ b/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); + }); + }); }); diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js index f2a4389446..7d1685581f 100644 --- a/website/server/models/user/hooks.js +++ b/website/server/models/user/hooks.js @@ -171,7 +171,7 @@ schema.pre('save', true, function preSaveUser (next, done) { // since it'll break as soon as a new field is added to the schema but not here. // do not calculate achievements if items or achievements are not selected - if (!this.isSelected('items') || !this.isSelected('achievements')) { + if (this.isSelected('items') && this.isSelected('achievements')) { // Determines if Beast Master should be awarded let beastMasterProgress = shared.count.beastMasterProgress(this.items.pets);