Collection quest ignoring items (#13401)

* update tests to check for issue #12664

* rewrite processCollectionQuest to allocate quest items proportionally

Removes need to check for excess items, fixing issue #12664

* move test for this bug to the correct context section & remove redundant test

* refactor processCollectionQuest more

* tweak confusing comments

test/api/unit/models/group.test.js

@@ -488,20 +488,6 @@ describe('Group Model', () => {
           expect(party.quest.progress.collect.soapBars).to.eq(5);
         });
 
-        it('does not drop an item if not need when on a collection quest', async () => {
-          party.quest.key = 'dilatoryDistress1';
-          party.quest.active = false;
-          await party.startQuest(questLeader);
-          party.quest.progress.collect.fireCoral = 20;
-          await party.save();
-
-          await Group.processQuestProgress(participatingMember, progress);
-
-          party = await Group.findOne({ _id: party._id });
-
-          expect(party.quest.progress.collect.fireCoral).to.eq(20);
-        });
-
         it('sends a chat message about progress', async () => {
           await Group.processQuestProgress(participatingMember, progress);
 
@@ -538,8 +524,8 @@ describe('Group Model', () => {
           });
         });
 
-        describe('collection quests with multiple items', () => {
+        describe('collection quests with multiple item types', () => {
-          it('sends a chat message if no progress is made on quest with multiple items', async () => {
+          it('sends a chat message if no progress is made', async () => {
             progress.collectedItems = 0;
             party.quest.key = 'dilatoryDistress1';
             party.quest.active = false;
@@ -609,6 +595,28 @@ describe('Group Model', () => {
           });
         });
 
+        it('does not drop items when an item type becomes full', async () => {
+          progress.collectedItems = 20;
+          party.quest.key = 'dilatoryDistress1';
+          party.quest.active = false;
+
+          await party.startQuest(questLeader);
+          party.quest.progress.collect.fireCoral = 19;
+          await party.save();
+
+          await Group.processQuestProgress(participatingMember, progress);
+
+          party = await Group.findOne({ _id: party._id });
+
+          // There is a very small chance (~1 in 500K) that blueFins will
+          // be 19 or 20 due to randomness and not any bug. In these cases, this
+          // test doesn't actually verify anything, but it's rare enough that it
+          // shouldn't be a problem, and to make it deterministic would require
+          // stubbing out methods in implementation-specific ways.
+          expect(party.quest.progress.collect.fireCoral).to.be.within(19, 20);
+          expect(party.quest.progress.collect.blueFins).to.be.within(19, 20);
+        });
+
         it('sends message about victory', async () => {
           progress.collectedItems = 500;
 

website/server/models/group.js

@@ -1141,32 +1141,26 @@ schema.methods._processCollectionQuest = async function processCollectionQuest (
   const group = this;
   const quest = questScrolls[group.quest.key];
   const itemsFound = {};
-
+  Object.keys(quest.collect).forEach(item => {
-  const possibleItemKeys = Object.keys(quest.collect)
-    .filter(key => group.quest.progress.collect[key] < quest.collect[key].count);
-
-  const possibleItemsToCollect = possibleItemKeys.reduce((accumulator, current, index) => {
-    accumulator[possibleItemKeys[index]] = quest.collect[current];
-    return accumulator;
-  }, {});
-
-  _.times(progress.collectedItems, () => {
-    const item = shared.randomVal(possibleItemsToCollect, { key: true });
-
-    if (group.quest.progress.collect[item] < quest.collect[item].count) {
-      if (!itemsFound[item]) {
     itemsFound[item] = 0;
-      }
-      itemsFound[item] += 1;
-      group.quest.progress.collect[item] += 1;
-    }
   });
 
-  // Add 0 for all items not found
+  // Create an array of item names, one item name per item that still needs to
-  Object.keys(this.quest.progress.collect).forEach(item => {
+  // be collected so that items are found proportionally to how many are needed.
-    if (!itemsFound[item]) {
+  const remainingItems = [].concat(...Object.keys(quest.collect).map(item => {
-      itemsFound[item] = 0;
+    let count = quest.collect[item].count - (group.quest.progress.collect[item] || 0);
+    if (count < 0) { // This could only happen if there's a bug, but just in case.
+      count = 0;
     }
+    return Array(count).fill(item);
+  }));
+
+  // slice() will grab only what is available even if requested slice is larger
+  // than the array, so we don't need to worry about overfilling quest items.
+  const collectedItems = _.shuffle(remainingItems).slice(0, progress.collectedItems);
+  collectedItems.forEach(item => {
+    itemsFound[item] += 1;
+    group.quest.progress.collect[item] += 1;
   });
 
   let foundText = _.reduce(itemsFound, (m, v, k) => {
@@ -1186,13 +1180,10 @@ schema.methods._processCollectionQuest = async function processCollectionQuest (
   });
   group.markModified('quest.progress.collect');
 
-  // Still needs completing
+  const promises = [group.save(), foundChat.save()];
-  const needsCompleted = _.find(quest.collect, (v, k) => group.quest.progress.collect[k] < v.count);
-
-  if (needsCompleted) {
-    return Promise.all([group.save(), foundChat.save()]);
-  }
 
+  const questFinished = collectedItems.length === remainingItems.length;
+  if (questFinished) {
     await group.finishQuest(quest);
     const allItemsFoundChat = group.sendChat({
       message: `\`${shared.i18n.t('chatItemQuestFinish', 'en')}\``,
@@ -1201,7 +1192,8 @@ schema.methods._processCollectionQuest = async function processCollectionQuest (
       },
     });
 
-  const promises = [group.save(), foundChat.save(), allItemsFoundChat.save()];
+    promises.push(allItemsFoundChat.save());
+  }
 
   return Promise.all(promises);
 };