Ensure official challenges are listed first (fixes #11018) (#11030)

* Ensure official challenges are listed first

* Fix lint errors

* Move query creation into separate function

* switching branches

* Fixes and tests

* Formatting fixes

* Linting

* fix tests

test/api/v3/integration/challenges/GET-challenges_group_groupid.test.js

@@ -187,8 +187,7 @@ describe('GET challenges/groups/:groupId', () => {
   });
 
   context('official challenge is present', () => {
-    let publicGuild; let user; let officialChallenge; let challenge; let
-      challenge2;
+    let publicGuild; let user; let officialChallenge; let unofficialChallenges;
 
     before(async () => {
       const { group, groupLeader } = await createAndPopulateGroup({
@@ -214,10 +213,14 @@ describe('GET challenges/groups/:groupId', () => {
       });
       await user.post(`/challenges/${officialChallenge._id}/join`);
 
-      challenge = await generateChallenge(user, group);
-      await user.post(`/challenges/${challenge._id}/join`);
-      challenge2 = await generateChallenge(user, group);
-      await user.post(`/challenges/${challenge2._id}/join`);
+      // We add 10 extra challenges to test whether the official challenge
+      // (the oldest) makes it to the front page.
+      unofficialChallenges = [];
+      for (let i = 0; i < 10; i += 1) {
+        const challenge = await generateChallenge(user, group); // eslint-disable-line
+        await user.post(`/challenges/${challenge._id}/join`); // eslint-disable-line
+        unofficialChallenges.push(challenge);
+      }
     });
 
     it('should return official challenges first', async () => {
@@ -230,18 +233,17 @@ describe('GET challenges/groups/:groupId', () => {
     it('should return newest challenges first, after official ones', async () => {
       let challenges = await user.get(`/challenges/groups/${publicGuild._id}`);
 
-      let foundChallengeIndex = _.findIndex(challenges, { _id: challenge._id });
-      expect(foundChallengeIndex).to.eql(2);
-
-      foundChallengeIndex = _.findIndex(challenges, { _id: challenge2._id });
-      expect(foundChallengeIndex).to.eql(1);
+      unofficialChallenges.forEach((chal, index) => {
+        const foundChallengeIndex = _.findIndex(challenges, { _id: chal._id });
+        expect(foundChallengeIndex).to.eql(10 - index);
+      });
 
       const newChallenge = await generateChallenge(user, publicGuild);
       await user.post(`/challenges/${newChallenge._id}/join`);
 
       challenges = await user.get(`/challenges/groups/${publicGuild._id}`);
 
-      foundChallengeIndex = _.findIndex(challenges, { _id: newChallenge._id });
+      const foundChallengeIndex = _.findIndex(challenges, { _id: newChallenge._id });
       expect(foundChallengeIndex).to.eql(1);
     });
   });

test/api/v3/integration/challenges/GET-challenges_user.test.js

@@ -242,7 +242,7 @@ describe('GET challenges/user', () => {
   });
 
   context('official challenge is present', () => {
-    let user; let officialChallenge; let challenge; let challenge2; let
+    let user; let officialChallenge; let unofficialChallenges; let
       publicGuild;
 
     before(async () => {
@@ -270,10 +270,14 @@ describe('GET challenges/user', () => {
       });
       await user.post(`/challenges/${officialChallenge._id}/join`);
 
-      challenge = await generateChallenge(user, group);
-      await user.post(`/challenges/${challenge._id}/join`);
-      challenge2 = await generateChallenge(user, group);
-      await user.post(`/challenges/${challenge2._id}/join`);
+      // We add 10 extra challenges to test whether the official challenge
+      // (the oldest) makes it to the front page.
+      unofficialChallenges = [];
+      for (let i = 0; i < 10; i += 1) {
+        const challenge = await generateChallenge(user, group); // eslint-disable-line
+        await user.post(`/challenges/${challenge._id}/join`); // eslint-disable-line
+        unofficialChallenges.push(challenge);
+      }
     });
 
     it('should return official challenges first', async () => {
@@ -284,20 +288,23 @@ describe('GET challenges/user', () => {
     });
 
     it('should return newest challenges first, after official ones', async () => {
-      let challenges = await user.get('/challenges/user');
+      let challenges = await user.get('/challenges/user?page=0');
 
-      let foundChallengeIndex = _.findIndex(challenges, { _id: challenge._id });
-      expect(foundChallengeIndex).to.eql(2);
-
-      foundChallengeIndex = _.findIndex(challenges, { _id: challenge2._id });
-      expect(foundChallengeIndex).to.eql(1);
+      unofficialChallenges.forEach((chal, index) => {
+        const foundChallengeIndex = _.findIndex(challenges, { _id: chal._id });
+        if (index === 0) {
+          expect(foundChallengeIndex).to.eql(-1);
+        } else {
+          expect(foundChallengeIndex).to.eql(10 - index);
+        }
+      });
 
       const newChallenge = await generateChallenge(user, publicGuild);
       await user.post(`/challenges/${newChallenge._id}/join`);
 
       challenges = await user.get('/challenges/user');
 
-      foundChallengeIndex = _.findIndex(challenges, { _id: newChallenge._id });
+      const foundChallengeIndex = _.findIndex(challenges, { _id: newChallenge._id });
       expect(foundChallengeIndex).to.eql(1);
     });
   });

website/server/controllers/api-v3/challenges.js

@@ -26,6 +26,7 @@ import {
   getChallengeGroupResponse,
   createChallenge,
   cleanUpTask,
+  createChallengeQuery,
 } from '../../libs/challenges';
 import apiError from '../../libs/apiError';
 
@@ -409,13 +410,12 @@ api.getUserChallenges = {
       query.categories = { $elemMatch: { slug: { $in: categorySlugs } } };
     }
 
-    let mongoQuery = Challenge.find(query)
-      .sort('-createdAt');
-
+    // Ensure that official challenges are always first
+    let mongoQuery = createChallengeQuery(query);
     if (page) {
       mongoQuery = mongoQuery
-        .limit(CHALLENGES_PER_PAGE)
-        .skip(CHALLENGES_PER_PAGE * page);
+        .skip(CHALLENGES_PER_PAGE * page)
+        .limit(CHALLENGES_PER_PAGE);
     }
 
     // see below why we're not using populate
@@ -423,9 +423,8 @@ api.getUserChallenges = {
     // .populate('leader', nameFields)
     const challenges = await mongoQuery.exec();
 
-    let resChals = challenges.map(challenge => challenge.toJSON());
-
-    resChals = _.orderBy(resChals, [challenge => challenge.categories.map(category => category.slug).includes('habitica_official')], ['desc']);
+    // Unserialize, then serialize the challenges to fill in default fields
+    const resChals = challenges.map(chal => (new Challenge(chal)).toJSON());
 
     // Instead of populate we make a find call manually because of https://github.com/Automattic/mongoose/issues/3833
     await Promise.all(resChals.map((chal, index) => Promise.all([
@@ -483,19 +482,12 @@ api.getGroupChallenges = {
     const group = await Group.getGroup({ user, groupId });
     if (!group) throw new NotFound(res.t('groupNotFound'));
 
-    const challenges = await Challenge.find({ group: groupId })
-      .sort('-createdAt')
+    const challenges = await createChallengeQuery({ group: groupId })
       // Only populate the leader as the group is implicit // see below why we're not using populate
       // .populate('leader', nameFields)
       .exec();
 
-    let resChals = challenges.map(challenge => challenge.toJSON());
-
-    resChals = _.orderBy(
-      resChals,
-      [challenge => challenge.categories.map(category => category.slug).includes('habitica_official')],
-      ['desc'],
-    );
+    const resChals = challenges.map(challenge => (new Challenge(challenge)).toJSON());
 
     // Instead of populate we make a find call manually because of https://github.com/Automattic/mongoose/issues/3833
     await Promise.all(resChals.map((chal, index) => User

website/server/libs/challenges/index.js

@@ -109,3 +109,29 @@ export function cleanUpTask (task) {
 
   return cleansedTask;
 }
+
+// Create an aggregation query for querying challenges.
+// Ensures that official challenges are listed first.
+export function createChallengeQuery (query) {
+  return Challenge.aggregate()
+    .match(query)
+    .addFields({
+      isOfficial: {
+        $gt: [
+          {
+            $size: {
+              $filter: {
+                input: '$categories',
+                as: 'cat',
+                cond: {
+                  $eq: ['$$cat.slug', 'habitica_official'],
+                },
+              },
+            },
+          },
+          0,
+        ],
+      },
+    })
+    .sort('-isOfficial -createdAt');
+}