From ef6afaedb4b4d6839f1cd5fdcadeb35e0b287096 Mon Sep 17 00:00:00 2001 From: Keith Holliday Date: Tue, 12 Jan 2016 18:00:37 -0600 Subject: [PATCH] Added intitial group leave tests Fixed test readability, updated party test, and updated challenge update code when leaving group Updated library, added group existance check, and reset full party Updated syntax, added new userUnlinkChallenges, and added some initial testing for challenges Added challenge tasks tests Added try/catch to group remove, add more party tests, fixed broken challenge test, removed useless return value Added public guild tests, added more tests to party, and abstracted remove invitations logic Closes #6506 --- .../groups/POST-groups_groupId_leave.js | 195 ++++++++++++++++++ test/helpers/api-integration/v3/index.js | 8 + website/src/controllers/api-v3/groups.js | 2 + website/src/controllers/api-v3/tasks.js | 1 + website/src/models/group.js | 149 ++++++------- website/src/models/user.js | 13 +- 6 files changed, 280 insertions(+), 88 deletions(-) create mode 100644 test/api/v3/integration/groups/POST-groups_groupId_leave.js diff --git a/test/api/v3/integration/groups/POST-groups_groupId_leave.js b/test/api/v3/integration/groups/POST-groups_groupId_leave.js new file mode 100644 index 0000000000..59031300ba --- /dev/null +++ b/test/api/v3/integration/groups/POST-groups_groupId_leave.js @@ -0,0 +1,195 @@ +import { + generateChallenge, + checkExistence, + createAndPopulateGroup, + sleep, +} from '../../../../helpers/api-v3-integration.helper'; +import { + each, +} from 'lodash'; + +describe('POST /groups/:groupId/leave', () => { + let typesOfGroups = { + 'public guild': { type: 'guild', privacy: 'public' }, + 'private guild': { type: 'guild', privacy: 'private' }, + party: { type: 'party', privacy: 'private' }, + }; + + each(typesOfGroups, (groupDetails, groupType) => { + context(`Leaving a ${groupType}`, () => { + let groupToLeave; + let leader; + let member; + + beforeEach(async () => { + let { group, groupLeader, members } = await createAndPopulateGroup({ + groupDetails, + members: 1, + }); + + groupToLeave = group; + leader = groupLeader; + member = members[0]; + }); + + it(`lets user leave a ${groupType}`, async () => { + await member.post(`/groups/${groupToLeave._id}/leave`); + + let userThatLeftGroup = await member.get('/user'); + + expect(userThatLeftGroup.guilds).to.be.empty; + expect(userThatLeftGroup.party._id).to.not.exist; + }); + + it(`sets a new group leader when leader leaves a ${groupType}`, async () => { + await leader.post(`/groups/${groupToLeave._id}/leave`); + + let groupToLeaveWithNewLeader = await member.get(`/groups/${groupToLeave._id}`); + + expect(groupToLeaveWithNewLeader.leader._id).to.equal(member._id); + }); + + context('With challenges', () => { + let challenge; + + beforeEach(async () => { + challenge = await generateChallenge(leader, groupToLeave); + + await leader.post(`/tasks/challenge/${challenge._id}`, { + text: 'test habit', + type: 'habit', + }); + + await sleep(0.5); + }); + + it('removes all challenge tasks when keep parameter is set to remove', async () => { + await leader.post(`/groups/${groupToLeave._id}/leave?keep=remove-all`); + + let userWithoutChallengeTasks = await leader.get('/user'); + + expect(userWithoutChallengeTasks.challenges).to.not.include(challenge._id); + expect(userWithoutChallengeTasks.tasksOrder.habits).to.be.empty; + }); + + it('keeps all challenge tasks when keep parameter is not set', async () => { + await leader.post(`/groups/${groupToLeave._id}/leave`); + + let userWithChallengeTasks = await leader.get('/user'); + + expect(userWithChallengeTasks.challenges).to.not.include(challenge._id); + // @TODO find elegant way to assert against the task existing + expect(userWithChallengeTasks.tasksOrder.habits).to.not.be.empty; + }); + }); + + it('prevents quest leader from leaving a groupToLeave'); + it('prevents a user from leaving during an active quest'); + }); + }); + + context('Leaving a group as the last member', () => { + context('private guild', () => { + let privateGuild; + let leader; + let invitedUser; + + beforeEach(async () => { + let { group, groupLeader, invitees } = await createAndPopulateGroup({ + groupDetails: { + name: 'Test Private Guild', + type: 'guild', + }, + invites: 1, + }); + + privateGuild = group; + leader = groupLeader; + invitedUser = invitees[0]; + }); + + it('removes a group when the last member leaves', async () => { + await leader.post(`/groups/${privateGuild._id}/leave`); + + await expect(checkExistence('groups', privateGuild._id)).to.eventually.equal(false); + }); + + it('removes invitations when the last member leaves', async () => { + await leader.post(`/groups/${privateGuild._id}/leave`); + + let userWithoutInvitation = await invitedUser.get('/user'); + + expect(userWithoutInvitation.invitations.guilds).to.be.empty; + }); + }); + + context('public guild', () => { + let publicGuild; + let leader; + let invitedUser; + + beforeEach(async () => { + let { group, groupLeader, invitees } = await createAndPopulateGroup({ + groupDetails: { + name: 'Test Public Guild', + type: 'guild', + privacy: 'public', + }, + invites: 1, + }); + + publicGuild = group; + leader = groupLeader; + invitedUser = invitees[0]; + }); + + it('keeps the group when the last member leaves', async () => { + await leader.post(`/groups/${publicGuild._id}/leave`); + + await expect(checkExistence('groups', publicGuild._id)).to.eventually.equal(true); + }); + + it('keeps the invitations when the last member leaves a public guild', async () => { + await leader.post(`/groups/${publicGuild._id}/leave`); + + let userWithoutInvitation = await invitedUser.get('/user'); + + expect(userWithoutInvitation.invitations.guilds).to.not.be.empty; + }); + }); + + context('party', () => { + let party; + let leader; + let invitedUser; + + beforeEach(async () => { + let { group, groupLeader, invitees } = await createAndPopulateGroup({ + groupDetails: { + name: 'Test Party', + type: 'party', + }, + invites: 1, + }); + + party = group; + leader = groupLeader; + invitedUser = invitees[0]; + }); + + it('removes a group when the last member leaves a party', async () => { + await leader.post(`/groups/${party._id}/leave`); + + await expect(checkExistence('party', party._id)).to.eventually.equal(false); + }); + + it('removes invitations when the last member leaves a party', async () => { + await leader.post(`/groups/${party._id}/leave`); + + let userWithoutInvitation = await invitedUser.get('/user'); + + expect(userWithoutInvitation.invitations.party).to.be.empty; + }); + }); + }); +}); diff --git a/test/helpers/api-integration/v3/index.js b/test/helpers/api-integration/v3/index.js index 0c440ade26..880f88f41a 100644 --- a/test/helpers/api-integration/v3/index.js +++ b/test/helpers/api-integration/v3/index.js @@ -8,3 +8,11 @@ export { requester }; export { translate } from '../translate'; export { checkExistence, resetHabiticaDB } from '../mongo'; export * from './object-generators'; + +export async function sleep (seconds) { + let milliseconds = seconds * 1000; + + return new Promise((resolve) => { + setTimeout(resolve, milliseconds); + }); +} diff --git a/website/src/controllers/api-v3/groups.js b/website/src/controllers/api-v3/groups.js index d7d3eedbd3..05a68831e9 100644 --- a/website/src/controllers/api-v3/groups.js +++ b/website/src/controllers/api-v3/groups.js @@ -251,6 +251,8 @@ api.joinGroup = { if (group.memberCount === 0) group.leader = user._id; // If new user is only member -> set as leader + group.memberCount += 1; + let promises = [group.save(), user.save()]; if (group.type === 'party' && inviter) { diff --git a/website/src/controllers/api-v3/tasks.js b/website/src/controllers/api-v3/tasks.js index 7f2a488735..acdd456beb 100644 --- a/website/src/controllers/api-v3/tasks.js +++ b/website/src/controllers/api-v3/tasks.js @@ -102,6 +102,7 @@ api.createChallengeTasks = { if (challenge.leader !== user._id) throw new NotAuthorized(res.t('onlyChalLeaderEditTasks')); let tasks = await _createTasks(req, res, user, challenge); + res.respond(201, tasks.length === 1 ? tasks[0] : tasks); // If adding tasks to a challenge -> sync users diff --git a/website/src/models/group.js b/website/src/models/group.js index 5ff1eda3dd..8e21623c7c 100644 --- a/website/src/models/group.js +++ b/website/src/models/group.js @@ -98,30 +98,15 @@ schema.statics.sanitizeUpdate = function sanitizeUpdate (updateObj) { }*/ // TODO test -schema.pre('remove', true, function preRemoveGroup (next, done) { +schema.pre('remove', true, async function preRemoveGroup (next, done) { next(); let group = this; - - // Remove invitations when group is deleted - // TODO verify it works fir everything - User.find({ - // TODO id -> _id ? - [`invitations.${group.type}${group.type === 'guild' ? 's' : ''}.id`]: group._id, - }).exec() - .then(users => { - return Q.all(users.map(user => { - if (group.type === 'party') { - user.invitations.party = {}; // TODO mark modified - } else { - let i = _.findIndex(user.invitations.guilds, {id: group._id}); - user.invitations.guilds.splice(i, 1); - } - - return user.save(); - })); - }) - .then(done) - .catch(done); + try { + await group.removeGroupInvitations(); + done(); + } catch (err) { + done(err); + } }); schema.post('remove', function postRemoveGroup (group) { @@ -150,6 +135,27 @@ schema.statics.getGroup = function getGroup (options = {}) { // TODO purge chat flags info? in tojson? }; +schema.methods.removeGroupInvitations = async function removeGroupInvitations () { + let group = this; + + let usersToRemoveInvitationsFrom = await User.find({ + // TODO id -> _id ? + [`invitations.${group.type}${group.type === 'guild' ? 's' : ''}.id`]: group._id, + }).exec(); + + let userUpdates = usersToRemoveInvitationsFrom.map(user => { + if (group.type === 'party') { + user.invitations.party = {}; // TODO mark modified + } else { + let i = _.findIndex(user.invitations.guilds, {id: group._id}); + user.invitations.guilds.splice(i, 1); + } + return user.save(); + }); + + return Q.all(userUpdates); +}; + // Return true if user is a member of the group schema.methods.isMember = function isGroupMember (user) { if (this._id === 'habitrpg') { @@ -452,71 +458,46 @@ schema.statics.bossQuest = function bossQuest (user, progress) { }); }; -// Remove user from this group -// TODO this is highly inefficient -schema.methods.leave = function leaveGroup (user, keep) { +schema.methods.leave = async function leaveGroup (user, keep = 'keep-all') { let group = this; - return Q.all([ - // Remove user from group challenges - - // First find relevant Challenges - Challenge.find({ - _id: {$in: user.challenges}, // Challenges I am in - group: group._id, // that belong to the group I am leaving - }).then(challenges => { - // Update each challenge - return Challenge.update( - {_id: {$in: _.pluck(challenges, '_id')}}, - {$pull: {members: user._id}}, - {multi: true} - ).then(() => challenges); // pass `challenges` above to next promise - }).then(challenges => { - return Q.all(challenges.map(chal => { - let i = user.challenges.indexOf(chal._id); - if (i !== -1) user.challenges.splice(i, 1); - return user.unlinkChallengeTasks(chal._id, keep); - })); - }), - - // Update the group - (() => { - // If user is the last one in group and group is private, delete it - if (group.members.length === 1 && ( - group.type === 'party' || - group.type === 'guild' && group.privacy === 'private' - )) return group.remove(); - - let update = {}; - // otherwise just remove a member TODO create User.methods.removeFromGroup? - if (group.type === 'guild') { - _.pull(user.guilds, group._id); - } else { - user.party._id = undefined; // TODO remove quest information too? - } - - // If the leader is leaving (or if the leader previously left, and this wasn't accounted for) - let leader = group.leader; - - if (leader === user._id || group.members.indexOf(leader) === -1) { - let seniorMember = _.find(group.members, m => m !== user._id); - - // could be missing in case of public guild (that can have 0 members) with 1 member who is leaving - if (seniorMember) update.$set = {leader: seniorMember}; - } - - update.$inc = {memberCount: -1}; - return Q.all([ - model.update({_id: group._id}, update).exec(), // eslint-disable-line no-use-before-define - user.save(), - ]); - })(), - ]).then(() => { - firebase.removeUserFromGroup(group._id, user._id); - return; // TODO ok not to return promise? - }).catch(err => { // TODO do we have to catch err if we return the promise? - throw err; + let challenges = await Challenge.find({ + _id: {$in: user.challenges}, + groupId: group._id, }); + + let challengesToRemoveUserFrom = challenges.map(chal => { + return user.unlinkChallengeTasks(chal._id, keep); + }); + await Q.all(challengesToRemoveUserFrom); + + let promises = []; + + // If user is the last one in group and group is private, delete it + if (group.memberCount <= 1 && group.privacy === 'private') { + return await group.remove(); + } + + // otherwise just remove a member TODO create User.methods.removeFromGroup? + if (group.type === 'guild') { + promises.push(User.update({_id: user._id}, {$pull: {guilds: group._id } }).exec()); + } else { + promises.push(User.update({_id: user._id}, {$set: {party: {} } }).exec()); + } + + // If the leader is leaving (or if the leader previously left, and this wasn't accounted for) + let update = { memberCount: group.memberCount - 1 }; + if (group.leader === user._id) { + let query = group.type === 'party' ? {'party._id': group._id} : {guilds: group._id}; + let seniorMember = await User.findOne({query, _id: {$ne: user._id}}).exec(); + + // could be missing in case of public guild (that can have 0 members) with 1 member who is leaving + if (seniorMember) update.$set = {leader: seniorMember._id}; + } + promises.push(group.update(update).exec()); + firebase.removeUserFromGroup(group._id, user._id); + + return Q.all(promises); }; export const INVITES_LIMIT = 100; diff --git a/website/src/models/user.js b/website/src/models/user.js index 8942a8afd2..6ea4ede281 100644 --- a/website/src/models/user.js +++ b/website/src/models/user.js @@ -661,13 +661,18 @@ schema.methods.unlinkChallengeTasks = async function unlinkChallengeTasks (chall 'challenge.id': challengeId, }; + let challengeIndex = user.challenges.indexOf(challengeId); + if (challengeIndex !== -1) user.challenges.splice(challengeIndex, 1); + if (keep === 'keep-all') { await Tasks.Task.update(findQuery, { $set: {challenge: {}}, // TODO what about updatedAt? }, {multi: true}).exec(); + + await user.save(); } else { // keep = 'remove-all' - let tasks = Tasks.Task.find(findQuery).select('_id type completed').exec(); - tasks = tasks.map(task => { + let tasks = await Tasks.Task.find(findQuery).select('_id type completed').exec(); + let taskPromises = tasks.map(task => { // Remove task from user.tasksOrder and delete them if (task.type !== 'todo' || !task.completed) { let list = user.tasksOrder[`${task.type}s`]; @@ -678,8 +683,8 @@ schema.methods.unlinkChallengeTasks = async function unlinkChallengeTasks (chall return task.remove(); }); - tasks.push(user.save()); - await Q.all(tasks); + taskPromises.push(user.save()); + return Q.all(taskPromises); } };