fix: confirm no user objects reference a group before deleting it when the member count reaches 0 (#8267)

* fix: confirm no user objects reference a group before deleting it when the member count reaches 0

* Updating mongo queries to return promises and use the select statement.

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

@@ -628,24 +628,89 @@ describe('Group Model', () => {
         });
       });
 
-      it('deletes a private group when the last member leaves', async () => {
+      it('deletes a private party when the last member leaves', async () => {
-        party.memberCount = 1;
-
         await party.leave(participatingMember);
+        await party.leave(questLeader);
+        await party.leave(nonParticipatingMember);
+        await party.leave(undecidedMember);
 
         party = await Group.findOne({_id: party._id});
         expect(party).to.not.exist;
       });
 
       it('does not delete a public group when the last member leaves', async () => {
-        party.memberCount = 1;
         party.privacy = 'public';
 
+        await party.leave(participatingMember);
+        await party.leave(questLeader);
+        await party.leave(nonParticipatingMember);
+        await party.leave(undecidedMember);
+
+        party = await Group.findOne({_id: party._id});
+        expect(party).to.exist;
+      });
+
+      it('does not delete a private party when the member count reaches zero if there are still members', async () => {
+        party.memberCount = 1;
+
         await party.leave(participatingMember);
 
         party = await Group.findOne({_id: party._id});
         expect(party).to.exist;
       });
+
+      it('deletes a private guild when the last member leaves', async () => {
+        let guild = new Group({
+          name: 'test guild',
+          type: 'guild',
+          memberCount: 1,
+        });
+
+        let leader = new User({
+          guilds: [guild._id],
+        });
+
+        guild.leader = leader._id;
+
+        await Promise.all([
+          guild.save(),
+          leader.save(),
+        ]);
+
+        await guild.leave(leader);
+
+        guild = await Group.findOne({_id: guild._id});
+        expect(guild).to.not.exist;
+      });
+
+      it('does not delete a private guild when the member count reaches zero if there are still members', async () => {
+        let guild = new Group({
+          name: 'test guild',
+          type: 'guild',
+          memberCount: 1,
+        });
+
+        let leader = new User({
+          guilds: [guild._id],
+        });
+
+        let member = new User({
+          guilds: [guild._id],
+        });
+
+        guild.leader = leader._id;
+
+        await Promise.all([
+          guild.save(),
+          leader.save(),
+          member.save(),
+        ]);
+
+        await guild.leave(member);
+
+        guild = await Group.findOne({_id: guild._id});
+        expect(guild).to.exist;
+      });
     });
 
     describe('#sendChat', () => {

website/server/models/group.js

@@ -857,9 +857,7 @@ schema.statics.tavernBoss = async function tavernBoss (user, progress) {
 
 schema.methods.leave = async function leaveGroup (user, keep = 'keep-all') {
   let group = this;
-  let update = {
+  let update = {};
-    $inc: {memberCount: -1},
-  };
 
   let challenges = await Challenge.find({
     _id: {$in: user.challenges},
@@ -889,18 +887,30 @@ schema.methods.leave = async function leaveGroup (user, keep = 'keep-all') {
 
   // If user is the last one in group and group is private, delete it
   if (group.memberCount <= 1 && group.privacy === 'private') {
-    promises.push(group.remove());
+    // double check the member count is correct so we don't accidentally delete a group that still has users in it
-  } else { // otherwise If the leader is leaving (or if the leader previously left, and this wasn't accounted for)
+    let members;
-    if (group.leader === user._id) {
+    if (group.type === 'guild') {
-      let query = group.type === 'party' ? {'party._id': group._id} : {guilds: group._id};
+      members = await User.find({guilds: group._id}).select('_id').exec();
-      query._id = {$ne: user._id};
+    } else {
-      let seniorMember = await User.findOne(query).select('_id').exec();
+      members = await User.find({'party._id': group._id}).select('_id').exec();
-
+    }
-      // could be missing in case of public guild (that can have 0 members) with 1 member who is leaving
+    _.remove(members, {_id: user._id});
-      if (seniorMember) update.$set = {leader: seniorMember._id};
+    if (members.length === 0) {
+      promises.push(group.remove());
+      return await Bluebird.all(promises);
     }
-    promises.push(group.update(update).exec());
   }
+  // otherwise If the leader is leaving (or if the leader previously left, and this wasn't accounted for)
+  update.$inc = {memberCount: -1};
+  if (group.leader === user._id) {
+    let query = group.type === 'party' ? {'party._id': group._id} : {guilds: group._id};
+    query._id = {$ne: user._id};
+    let seniorMember = await User.findOne(query).select('_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());
 
   return await Bluebird.all(promises);
 };