diff --git a/migrations/challenges/sync-all-challenges.js b/migrations/challenges/sync-all-challenges.js index 5e6e7df2e1..d966e8b1b8 100644 --- a/migrations/challenges/sync-all-challenges.js +++ b/migrations/challenges/sync-all-challenges.js @@ -10,7 +10,7 @@ async function syncChallengeToMembers (challenges) { const promises = []; users.forEach(user => { - promises.push(challenge.syncToUser(user)); + promises.push(challenge.syncTasksToUser(user)); promises.push(challenge.save()); promises.push(user.save()); }); diff --git a/test/api/unit/models/challenge.test.js b/test/api/unit/models/challenge.test.js index f95908f298..5c69ef4c27 100644 --- a/test/api/unit/models/challenge.test.js +++ b/test/api/unit/models/challenge.test.js @@ -90,7 +90,37 @@ describe('Challenge Model', () => { expect(syncedTask.tags[0]).to.eql(challenge._id); }); - it('syncs a challenge to a user', async () => { + it('adds a challenge to a user', async () => { + const newMember = new User({ + guilds: [guild._id], + }); + await newMember.save(); + + const addedSuccessfully = await challenge.addToUser(newMember); + + const updatedNewMember = await User.findById(newMember._id); + + expect(addedSuccessfully).to.eql(true); + expect(updatedNewMember.challenges).to.contain(challenge._id); + }); + + it('does not add a challenge to a user that already in the challenge', async () => { + const newMember = new User({ + guilds: [guild._id], + challenges: [challenge._id], + }); + await newMember.save(); + + const addedSuccessfully = await challenge.addToUser(newMember); + + const updatedNewMember = await User.findById(newMember._id); + + expect(addedSuccessfully).to.eql(false); + expect(updatedNewMember.challenges).to.contain(challenge._id); + expect(updatedNewMember.challenges.length).to.eql(1); + }); + + it('syncs challenge tasks to a user', async () => { await challenge.addTasks([task]); const newMember = new User({ @@ -98,7 +128,7 @@ describe('Challenge Model', () => { }); await newMember.save(); - await challenge.syncToUser(newMember); + await challenge.syncTasksToUser(newMember); const updatedNewMember = await User.findById(newMember._id); const updatedNewMemberTasks = await Tasks.Task.find({ _id: { $in: updatedNewMember.tasksOrder[`${taskType}s`] } }); @@ -110,14 +140,13 @@ describe('Challenge Model', () => { ), ); - expect(updatedNewMember.challenges).to.contain(challenge._id); expect(updatedNewMember.tags[7].id).to.equal(challenge._id); expect(updatedNewMember.tags[7].name).to.equal(challenge.shortName); expect(syncedTask).to.exist; expect(syncedTask.attribute).to.eql('str'); }); - it('syncs a challenge to a user with the existing task', async () => { + it('syncs challenge tasks to a user with the existing task', async () => { await challenge.addTasks([task]); let updatedLeader = await User.findOne({ _id: leader._id }); @@ -134,7 +163,7 @@ describe('Challenge Model', () => { task.text = newTitle; task.attribute = 'int'; await task.save(); - await challenge.syncToUser(leader); + await challenge.syncTasksToUser(leader); updatedLeader = await User.findOne({ _id: leader._id }); updatedLeadersTasks = await Tasks.Task.find({ _id: { $in: updatedLeader.tasksOrder[`${taskType}s`] } }); diff --git a/website/server/controllers/api-v3/challenges.js b/website/server/controllers/api-v3/challenges.js index 236bf198c4..00e283add7 100644 --- a/website/server/controllers/api-v3/challenges.js +++ b/website/server/controllers/api-v3/challenges.js @@ -254,19 +254,23 @@ api.joinChallenge = { const challenge = await Challenge.findOne({ _id: req.params.challengeId }).exec(); if (!challenge) throw new NotFound(res.t('challengeNotFound')); - if (challenge.isMember(user)) throw new NotAuthorized(res.t('userAlreadyInChallenge')); const group = await Group.getGroup({ user, groupId: challenge.group, fields: basicGroupFields, optionalMembership: true, }); if (!group || !challenge.canJoin(user, group)) throw new NotFound(res.t('challengeNotFound')); + const addedSuccessfully = await challenge.addToUser(user); + if (!addedSuccessfully) { + throw new NotAuthorized(res.t('userAlreadyInChallenge')); + } + challenge.memberCount += 1; addUserJoinChallengeNotification(user); // Add all challenge's tasks to user's tasks and save the challenge - const results = await Promise.all([challenge.syncToUser(user), challenge.save()]); + const results = await Promise.all([challenge.syncTasksToUser(user), challenge.save()]); const response = results[1].toJSON(); response.group = getChallengeGroupResponse(group); diff --git a/website/server/models/challenge.js b/website/server/models/challenge.js index 3a545e3975..3707b2a929 100644 --- a/website/server/models/challenge.js +++ b/website/server/models/challenge.js @@ -94,6 +94,23 @@ schema.methods.canJoin = function canJoinChallenge (user, group) { return user.getGroups().indexOf(this.group) !== -1; }; +// Returns true if the challenge was successfully added to the user +// or false if the user already in the challenge +schema.methods.addToUser = async function addChallengeToUser (user) { + // Add challenge to users challenges atomically (with a condition that checks that it + // is not there already) to prevent multiple concurrent requests from passing through + // see https://github.com/HabitRPG/habitica/issues/11295 + const result = await User.update( + { + _id: user._id, + challenges: { $nin: [this._id] }, + }, + { $push: { challenges: this._id } }, + ).exec(); + + return !!result.nModified; +}; + // Returns true if user can view the challenge // Different from canJoin because you can see challenges of groups // you've been removed from if you're participating in them @@ -102,19 +119,12 @@ schema.methods.canView = function canViewChallenge (user, group) { return this.canJoin(user, group); }; -// Sync challenge to user, including tasks and tags. +// Sync challenge tasks to user, including tags. // Used when user joins the challenge or to force sync. -schema.methods.syncToUser = async function syncChallengeToUser (user) { +schema.methods.syncTasksToUser = async function syncChallengeTasksToUser (user) { const challenge = this; challenge.shortName = challenge.shortName || challenge.name; - // Add challenge to user.challenges - if (!_.includes(user.challenges, challenge._id)) { - // using concat because mongoose's protection against - // concurrent array modification isn't working as expected. - // see https://github.com/HabitRPG/habitica/pull/7787#issuecomment-232972394 - user.challenges = user.challenges.concat([challenge._id]); - } // Sync tags const userTags = user.tags; const i = _.findIndex(userTags, { id: challenge._id });