Feature/enhance get memebers for a challenge fixes #12481 (#12716)

* refactor(api-members): separate handler for the GET /challenges/:challengeId/members route

* refactor(api-members): challenges-related code removed from _getMembersForItem handler function

* feat(api-members): added support to the new includeTasks query parameter for the GET /challenges/:challengeId/members route

* fix(api-members): adjustments to the GET /challenges/:challengeId/members route

* fix(api-members): merge of _getMembersTasksFromChallenge and additional check for a test suite

* refactor(api-members): includeAllMembers query parameter got removed from the GET /challenges/:challengeId/members route

* GET-challenges_challengeId_members.test.js: use _id

* members.js: use _id instead of id

* use id instead of _id

* _id instead of id

Co-authored-by: Matteo Pagliazzi <matteopagliazzi@gmail.com>

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

@@ -117,26 +117,7 @@ describe('GET /challenges/:challengeId/members', () => {
     expect(res[0].profile).to.have.all.keys(['name']);
   });
 
-  it('returns only first 30 members if req.query.includeAllMembers is not true and req.query.limit is undefined', async () => {
+  it('returns only first 30 members if req.query.limit is undefined', async () => {
-    const group = await generateGroup(user, { type: 'party', name: generateUUID() });
-    const challenge = await generateChallenge(user, group);
-    await user.post(`/challenges/${challenge._id}/join`);
-
-    const usersToGenerate = [];
-    for (let i = 0; i < 31; i += 1) {
-      usersToGenerate.push(generateUser({ challenges: [challenge._id] }));
-    }
-    await Promise.all(usersToGenerate);
-
-    const res = await user.get(`/challenges/${challenge._id}/members?includeAllMembers=not-true`);
-    expect(res.length).to.equal(30);
-    res.forEach(member => {
-      expect(member).to.have.all.keys(['_id', 'auth', 'flags', 'id', 'profile']);
-      expect(member.profile).to.have.all.keys(['name']);
-    });
-  });
-
-  it('returns only first 30 members if req.query.includeAllMembers is not defined and req.query.limit is undefined', async () => {
     const group = await generateGroup(user, { type: 'party', name: generateUUID() });
     const challenge = await generateChallenge(user, group);
     await user.post(`/challenges/${challenge._id}/join`);
@@ -217,25 +198,6 @@ describe('GET /challenges/:challengeId/members', () => {
     });
   }).timeout(30000);
 
-  it('returns all members if req.query.includeAllMembers is true', async () => {
-    const group = await generateGroup(user, { type: 'party', name: generateUUID() });
-    const challenge = await generateChallenge(user, group);
-    await user.post(`/challenges/${challenge._id}/join`);
-
-    const usersToGenerate = [];
-    for (let i = 0; i < 31; i += 1) {
-      usersToGenerate.push(generateUser({ challenges: [challenge._id] }));
-    }
-    await Promise.all(usersToGenerate);
-
-    const res = await user.get(`/challenges/${challenge._id}/members?includeAllMembers=true`);
-    expect(res.length).to.equal(32);
-    res.forEach(member => {
-      expect(member).to.have.all.keys(['_id', 'auth', 'flags', 'id', 'profile']);
-      expect(member.profile).to.have.all.keys(['name']);
-    });
-  });
-
   it('supports using req.query.lastId to get more members', async function test () {
     this.timeout(30000); // @TODO: times out after 8 seconds
     const group = await generateGroup(user, { type: 'party', name: generateUUID() });
@@ -259,6 +221,34 @@ describe('GET /challenges/:challengeId/members', () => {
     expect(resIds).to.eql(expectedIds.sort());
   });
 
+  it('supports using req.query.includeTasks in order to add challenge-related tasks of all members', async () => {
+    const group = await generateGroup(user, { type: 'party', name: generateUUID() });
+    const challenge = await generateChallenge(user, group);
+    await user.post(`/challenges/${challenge._id}/join`);
+
+    const usersToGenerate = [];
+    for (let i = 0; i < 8; i += 1) {
+      usersToGenerate.push(generateUser({ challenges: [challenge._id] }));
+    }
+    await Promise.all(usersToGenerate);
+    await user.post(`/tasks/challenge/${challenge._id}`, [{ type: 'habit', text: 'Some task' }]);
+    await user.post(`/tasks/challenge/${challenge._id}`, [{ type: 'daily', text: 'Some different task' }]);
+
+    const res = await user.get(`/challenges/${challenge._id}/members?includeTasks=true`);
+    expect(res.length).to.equal(9);
+    res.forEach(member => {
+      expect(member).to.have.property('tasks');
+      expect(member.tasks).to.be.an('array');
+      expect(member.tasks).to.have.lengthOf(2);
+      member.tasks.forEach(task => {
+        expect(task).to.include.all.keys(['type', 'value', 'priority', 'text', '_id', 'userId']);
+        expect(task).to.not.have.any.keys(['tags', 'checklist']);
+        expect(task.challenge.id).to.be.equal(challenge._id);
+        expect(task.userId).to.be.equal(member._id);
+      });
+    });
+  });
+
   it('supports using req.query.search to get search members', async () => {
     const group = await generateGroup(user, { type: 'party', name: generateUUID() });
     const challenge = await generateChallenge(user, group);

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

@@ -116,7 +116,6 @@ describe('GET /challenges/:challengeId/members/:memberId', () => {
     }]);
 
     const memberProgress = await user.get(`/challenges/${challenge._id}/members/${user._id}`);
-    expect(memberProgress.tasks[0]).not.to.have.key('tags');
+    expect(memberProgress.tasks[0]).to.not.have.any.keys(['tags', 'checklist']);
-    expect(memberProgress.tasks[0].checklist).to.eql([]);
   });
 });

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

@@ -26,6 +26,7 @@ import {
   sanitizeText as sanitizeMessageText,
 } from '../../models/message';
 import highlightMentions from '../../libs/highlightMentions';
+import { handleGetMembersForChallenge } from '../../libs/challenges/handleGetMembersForChallenge';
 
 const { achievements } = common;
 
@@ -278,16 +279,12 @@ api.getMemberAchievements = {
 // We should create factory functions. See Webhooks for a good example
 function _getMembersForItem (type) {
   // check for allowed `type`
-  if (['group-members', 'group-invites', 'challenge-members'].indexOf(type) === -1) {
+  if (['group-members', 'group-invites'].indexOf(type) === -1) {
-    throw new Error('Type must be one of "group-members", "group-invites", "challenge-members"');
+    throw new Error('Type must be one of "group-members", "group-invites"');
   }
 
   return async function handleGetMembersForItem (req, res) {
-    if (type === 'challenge-members') {
+    req.checkParams('groupId', res.t('groupIdRequired')).notEmpty();
-      req.checkParams('challengeId', res.t('challengeIdRequired')).notEmpty().isUUID();
-    } else {
-      req.checkParams('groupId', res.t('groupIdRequired')).notEmpty();
-    }
     req.checkQuery('lastId').optional().notEmpty().isUUID();
     // Allow an arbitrary number of results (up to 60)
     req.checkQuery('limit', res.t('groupIdRequired')).optional().notEmpty().isInt({ min: 1, max: 60 });
@@ -296,49 +293,18 @@ function _getMembersForItem (type) {
     if (validationErrors) throw validationErrors;
 
     const { groupId } = req.params;
-    const { challengeId } = req.params;
     const { lastId } = req.query;
     const { user } = res.locals;
-    let challenge;
-    let group;
 
-    if (type === 'challenge-members') {
+    const group = await Group.getGroup({ user, groupId, fields: '_id type' });
-      challenge = await Challenge.findById(challengeId).select('_id type leader group').exec();
+    if (!group) throw new NotFound(res.t('groupNotFound'));
-      if (!challenge) throw new NotFound(res.t('challengeNotFound'));
-
-      // optionalMembership is set to true because even
-      // if you're not member of the group you may be able to access the challenge
-      // for example if you've been booted from it, are the leader or a site admin
-      group = await Group.getGroup({
-        user,
-        groupId: challenge.group,
-        fields: '_id type privacy',
-        optionalMembership: true,
-      });
-
-      if (!group || !challenge.canView(user, group)) throw new NotFound(res.t('challengeNotFound'));
-    } else {
-      group = await Group.getGroup({ user, groupId, fields: '_id type' });
-      if (!group) throw new NotFound(res.t('groupNotFound'));
-    }
 
     const query = {};
     let fields = nameFields;
     // add computes stats to the member info when items and stats are available
     let addComputedStats = false;
 
-    if (type === 'challenge-members') {
+    if (type === 'group-members') {
-      query.challenges = challenge._id;
-
-      if (req.query.includeAllPublicFields === 'true') {
-        fields = memberFields;
-        addComputedStats = true;
-      }
-
-      if (req.query.search) {
-        query['auth.local.username'] = { $regex: req.query.search };
-      }
-    } else if (type === 'group-members') {
       if (group.type === 'guild') {
         query.guilds = group._id;
 
@@ -381,12 +347,7 @@ function _getMembersForItem (type) {
 
     if (lastId) query._id = { $gt: lastId };
 
-    let limit = req.query.limit ? Number(req.query.limit) : 30;
+    const limit = req.query.limit ? Number(req.query.limit) : 30;
-
-    // Allow for all challenges members to be returned
-    if (type === 'challenge-members' && req.query.includeAllMembers === 'true') {
-      limit = 0; // no limit
-    }
 
     const members = await User
       .find(query)
@@ -420,6 +381,9 @@ function _getMembersForItem (type) {
  *                                                    then all public fields for members
  *                                                    will be returned (similar to when making
  *                                                    a request for a single member).
+ * @apiParam (Query) {Boolean} includeTasks If set to `true`, then
+ *                                response should include all tasks per user
+ *                                related to the challenge
  *
  * @apiSuccess {Array} data An array of members, sorted by _id
  *
@@ -511,12 +475,12 @@ api.getInvitesForGroup = {
  *                                 get the next batch of results.
  * @apiParam (Query) {Number} limit=30 BETA Query parameter to
  *                                     specify the number of results to return. Max is 60.
+ * @apiParam (Query) {Boolean} includeTasks BETA Query parameter - If 'true'
+ *                                                    then include challenge tasks of each member
  * @apiParam (Query) {Boolean} includeAllPublicFields If set to `true`
  *                                                    then all public fields for members
  *                                                    will be returned (similar to when making
  *                                                    a request for a single member).
- * @apiParam (Query) {String} includeAllMembers BETA Query parameter - If 'true' all
- *                                              challenge members are returned.
 
  * @apiSuccess {Array} data An array of members, sorted by _id
  *
@@ -527,7 +491,7 @@ api.getMembersForChallenge = {
   method: 'GET',
   url: '/challenges/:challengeId/members',
   middlewares: [authWithHeaders()],
-  handler: _getMembersForItem('challenge-members'),
+  handler: handleGetMembersForChallenge,
 };
 
 /**
@@ -603,10 +567,8 @@ api.getChallengeMemberProgress = {
 
     const member = await User.findById(memberId).select(`${nameFields} challenges`).exec();
     if (!member) throw new NotFound(res.t('userWithIDNotFound', { userId: memberId }));
-
     const challenge = await Challenge.findById(challengeId).exec();
     if (!challenge) throw new NotFound(res.t('challengeNotFound'));
-
     // optionalMembership is set to true because even if you're
     // not member of the group you may be able to access the challenge
     // for example if you've been booted from it, are the leader or a site admin
@@ -616,20 +578,18 @@ api.getChallengeMemberProgress = {
     if (!group || !challenge.canView(user, group)) throw new NotFound(res.t('challengeNotFound'));
     if (!challenge.isMember(member)) throw new NotFound(res.t('challengeMemberNotFound'));
 
-    const chalTasks = await Tasks.Task.find({
+    const challengeTasks = await Tasks.Task.find({
-      userId: memberId,
+      userId: member._id,
-      'challenge.id': challengeId,
+      'challenge.id': challenge._id,
     })
-      .select('-tags') // We don't want to return the tags publicly TODO same for other data?
+      .select('-tags -checklist') // We don't want to return tags and checklists publicly
+      .lean()
       .exec();
 
     // manually call toJSON with minimize: true so empty paths aren't returned
     const response = member.toJSON({ minimize: true });
     delete response.challenges;
-    response.tasks = chalTasks.map(chalTask => {
+    response.tasks = challengeTasks;
-      chalTask.checklist = []; // Clear checklists as they are private
-      return chalTask.toJSON({ minimize: true });
-    });
     res.respond(200, response);
   },
 };

website/server/libs/challenges/handleGetMembersForChallenge.js

@@ -0,0 +1,90 @@
+import _ from 'lodash';
+import {
+  model as User,
+  publicFields as memberFields,
+  nameFields,
+} from '../../models/user';
+import { model as Challenge } from '../../models/challenge';
+import { model as Group } from '../../models/group';
+import * as Tasks from '../../models/task';
+import { NotFound } from '../errors';
+
+async function getMembersTasksForChallenge (members, challenge) {
+  const challengeTasks = await Tasks.Task.find({
+    userId: { $in: members.map(m => m._id) },
+    'challenge.id': challenge._id,
+  })
+    .select('-tags -checklist') // We don't want to return tags and checklists publicly
+    .lean()
+    .exec();
+
+  return _.groupBy(challengeTasks, 'userId');
+}
+
+export async function handleGetMembersForChallenge (req, res) {
+  req.checkParams('challengeId', res.t('challengeIdRequired')).notEmpty().isUUID();
+  req.checkQuery('lastId').optional().notEmpty().isUUID();
+  // Allow an arbitrary number of results (up to 60)
+  req.checkQuery('limit', res.t('groupIdRequired')).optional().notEmpty().isInt({ min: 1, max: 60 });
+
+  const validationErrors = req.validationErrors();
+  if (validationErrors) throw validationErrors;
+
+  const { challengeId } = req.params;
+  const { lastId } = req.query;
+  const { user } = res.locals;
+
+  const challenge = await Challenge.findById(challengeId).select('_id type leader group').exec();
+  if (!challenge) throw new NotFound(res.t('challengeNotFound'));
+
+  // optionalMembership is set to true because even
+  // if you're not member of the group you may be able to access the challenge
+  // for example if you've been booted from it, are the leader or a site admin
+  const group = await Group.getGroup({
+    user,
+    groupId: challenge.group,
+    fields: '_id type privacy',
+    optionalMembership: true,
+  });
+
+  if (!group || !challenge.canView(user, group)) throw new NotFound(res.t('challengeNotFound'));
+
+  const query = {};
+  let fields = nameFields;
+  // add computes stats to the member info when items and stats are available
+  let addComputedStats = false;
+  query.challenges = challenge._id;
+
+  if (req.query.includeAllPublicFields === 'true') {
+    fields = memberFields;
+    addComputedStats = true;
+  }
+
+  if (req.query.search) {
+    query['auth.local.username'] = { $regex: req.query.search };
+  }
+
+  if (lastId) query._id = { $gt: lastId };
+
+  const limit = req.query.limit ? Number(req.query.limit) : 30;
+
+  const members = await User
+    .find(query)
+    .sort({ _id: 1 })
+    .limit(limit)
+    .select(fields)
+    .lean()
+    .exec();
+
+  const includeTasks = req.query.includeTasks === 'true';
+  let memberIdToTasksMap;
+  if (includeTasks) {
+    memberIdToTasksMap = await getMembersTasksForChallenge(members, challenge);
+  }
+
+  members.forEach(member => {
+    User.transformJSONUser(member, addComputedStats);
+    if (includeTasks) member.tasks = memberIdToTasksMap[member._id];
+  });
+  res.respond(200, members);
+}